On Wed, Feb 02, 2022 at 10:31:26PM -0800, Reiji Watanabe wrote:
> Hi Ricardo,
>
> On Tue, Feb 1, 2022 at 10:39 AM Ricardo Koller <ricarkol@xxxxxxxxxx> wrote:
> >
> > Hey Reiji,
> >
> > On Mon, Jan 31, 2022 at 10:00:40PM -0800, Reiji Watanabe wrote:
> > > Hi Ricardo,
> > >
> > > On Sun, Jan 30, 2022 at 7:40 PM Ricardo Koller <ricarkol@xxxxxxxxxx> 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).
> >
> > Let me double check my understanding using the DEBUGVER example, please.
> > The safe_value would be DEBUGVER=5, and it contradicts the initial VM
> > value calculated on the KVM side. Q1: Can a contradiction like this
> > occur in practice? Q2: If the user saves and restores this id-reg on the
> > same kernel, the AA64DFR0 userspace write would fail (ftr_val !=
> > arm64_ftr_safe_value), right?
>
> Thank you for the comment!
>
> For Q1, yes, we might possibly create a bug that makes a contradiction
> between KVM and cpufeature.c.
> For Q2, even with such a contradiction, userspace will still be able to
> save and restore the id reg on the same kernel on the same system in most
> cases because @limit that KVM will specify for arm64_check_features()
> will mostly be the same as the initial value for the guest (except for
> fields corresponding to opt-in CPU features, which are configured with
> KVM_ARM_VCPU_INIT or etc) and arm64_check_features does an equality check
> per field. Having said that, as you suggested, it might be better to run
> arm64_check_features for the initial value against the host value so we
> can catch such a bug. I'll look into doing that in v5.
>
Thanks Reiji. Looking forward to v5.
> Thanks,
> Reiji
