Re: [RFC PATCH v4 02/26] KVM: arm64: Save ID registers' sanitized value per guest

[Reiji Watanabe <reijiw@google.com>]

Hi Ricardo,

On Sun, Jan 30, 2022 at 7:40 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Fri, Jan 28, 2022 at 09:52:21PM -0800, Reiji Watanabe wrote:
> > Hi Ricardo,
> >
> > > > > > +
> > > > > > +/*
> > > > > > + * Set the guest's ID registers that are defined in sys_reg_descs[]
> > > > > > + * with ID_SANITISED() to the host's sanitized value.
> > > > > > + */
> > > > > > +void set_default_id_regs(struct kvm *kvm)
> > > > > > +{
> > > > > > +     int i;
> > > > > > +     u32 id;
> > > > > > +     const struct sys_reg_desc *rd;
> > > > > > +     u64 val;
> > > > > > +
> > > > > > +     for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
> > > > > > +             rd = &sys_reg_descs[i];
> > > > > > +             if (rd->access != access_id_reg)
> > > > > > +                     /* Not ID register, or hidden/reserved ID register */
> > > > > > +                     continue;
> > > > > > +
> > > > > > +             id = reg_to_encoding(rd);
> > > > > > +             if (WARN_ON_ONCE(!is_id_reg(id)))
> > > > > > +                     /* Shouldn't happen */
> > > > > > +                     continue;
> > > > > > +
> > > > > > +             val = read_sanitised_ftr_reg(id);
> > > > >
> > > > > I'm a bit confused. Shouldn't the default+sanitized values already use
> > > > > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)?
> > > >
> > > > I'm not sure if I understand your question.
> > > > arm64_ftr_bits_kvm is used for feature support checkings when
> > > > userspace tries to modify a value of ID registers.
> > > > With this patch, KVM just saves the sanitized values in the kvm's
> > > > buffer, but userspace is still not allowed to modify values of ID
> > > > registers yet.
> > > > I hope it answers your question.
> > >
> > > Based on the previous commit I was assuming that some registers, like
> > > id_aa64dfr0,
> > > would default to the overwritten values as the sanitized values. More
> > > specifically: if
> > > userspace doesn't modify any ID reg, shouldn't the defaults have the
> > > KVM overwritten
> > > values (arm64_ftr_bits_kvm)?
> >
> > arm64_ftr_bits_kvm doesn't have arm64_ftr_reg but arm64_ftr_bits,
> > and arm64_ftr_bits_kvm doesn't have the sanitized values.
> >
> > Thanks,
>
> Hey Reiji,
>
> Sorry, I wasn't very clear. This is what I meant.
>
> If I set DEBUGVER to 0x5 (w/ FTR_EXACT) using this patch on top of the
> series:
>
>  static struct arm64_ftr_bits ftr_id_aa64dfr0_kvm[MAX_FTR_BITS_LEN] = {
>         S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
> -       ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
> +       ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x5),
>
> it means that userspace would not be able to set DEBUGVER to anything
> but 0x5. But I'm not sure what it should mean for the default KVM value
> of DEBUGVER, specifically the value calculated in set_default_id_regs().
> As it is, KVM is still setting the guest-visible value to 0x6, and my
> "desire" to only allow booting VMs with DEBUGVER=0x5 is being ignored: I
> booted a VM and the DEBUGVER value from inside is still 0x6. I was
> expecting it to not boot, or to show a warning.

Thank you for the explanation!

FTR_EXACT (in the existing code) means that the safe_val should be
used if values of the field are not identical between CPUs (see how
update_cpu_ftr_reg() uses arm64_ftr_safe_value()). For KVM usage,
it means that if the field value for a vCPU is different from the one
for the host's sanitized value, only the safe_val can be used safely
for the guest (purely in terms of CPU feature).

If KVM wants to restrict some features due to some reasons (e.g.
a feature for guests is not supported by the KVM yet), it should
be done by KVM (not by cpufeature.c), and  'validate' function in
"struct id_reg_info", which is introduced in patch-3, will be used
for such cases (the following patches actually use).

Thanks,
Reiji

>
> I think this has some implications for migrations. It would not be
> possible to migrate the example VM on the patched kernel from above: you
> can boot a VM with DEBUGVER=0x5 but you can't migrate it.
>
> Thanks,
> Ricardo