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

[Ricardo Koller <ricarkol@google.com>]

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.

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