Re: [RFC PATCH v4 01/26] KVM: arm64: Introduce a validation function for an ID register

[Fuad Tabba <tabba@google.com>]

Hi Reiji,

...

> > Could you please explain using ftr_temp[] and changing the value in
> > arm64_ftr_bits_kvm_override, rather than just
> > arm64_ftr_reg_bits_overrite(bits->ftr_bits, o_bits->ftr_bits)?
>
> I would like to maintain the order of the entries in the original
> ftr_bits so that (future) functions that work for the original ones
> also work for the KVM's.
> The copy and override is an easy way to do that.  The same thing can
> be done without ftr_temp[], but it would look a bit tricky.
>
> If we assume the order shouldn't matter or entries in ftr_bits
> are always in descending order, just changing the value in
> arm64_ftr_bits_kvm_override would be a much simpler way though.

Could you please add a comment in that case? I did find it to be
confusing until I read your explanation here.

>
> >
> >
> > > +static const struct arm64_ftr_bits *get_arm64_ftr_bits_kvm(u32 sys_id)
> > > +{
> > > +       const struct __ftr_reg_bits_entry *ret;
> > > +       int err;
> > > +
> > > +       if (!arm64_ftr_bits_kvm) {
> > > +               /* arm64_ftr_bits_kvm is not initialized yet. */
> > > +               err = init_arm64_ftr_bits_kvm();
> >
> > Rather than doing this check, can we just initialize it earlier, maybe
> > (indirectly) via kvm_arch_init_vm() or around the same time?
>
> Thank you for the comment.
> I will consider when it should be initialized.
> ( perhaps even earlier than kvm_arch_init_vm())
>
> >
> >
> > > +               if (err)
> > > +                       return NULL;
> > > +       }
> > > +
> > > +       ret = bsearch((const void *)(unsigned long)sys_id,
> > > +                     arm64_ftr_bits_kvm,
> > > +                     arm64_ftr_bits_kvm_nentries,
> > > +                     sizeof(arm64_ftr_bits_kvm[0]),
> > > +                     search_cmp_ftr_reg_bits);
> > > +       if (ret)
> > > +               return ret->ftr_bits;
> > > +
> > > +       return NULL;
> > > +}
> > > +
> > > +/*
> > > + * Check if features (or levels of features) that are indicated in the ID
> > > + * register value @val are also indicated in @limit.
> > > + * This function is for KVM to check if features that are indicated in @val,
> > > + * which will be used as the ID register value for its guest, are supported
> > > + * on the host.
> > > + * For AA64MMFR0_EL1.TGranX_2 fields, which don't follow the standard ID
> > > + * scheme, the function checks if values of the fields in @val are the same
> > > + * as the ones in @limit.
> > > + */
> > > +int arm64_check_features(u32 sys_reg, u64 val, u64 limit)
> > > +{
> > > +       const struct arm64_ftr_bits *ftrp = get_arm64_ftr_bits_kvm(sys_reg);
> > > +       u64 exposed_mask = 0;
> > > +
> > > +       if (!ftrp)
> > > +               return -ENOENT;
> > > +
> > > +       for (; ftrp->width; ftrp++) {
> > > +               s64 ftr_val = arm64_ftr_value(ftrp, val);
> > > +               s64 ftr_lim = arm64_ftr_value(ftrp, limit);
> > > +
> > > +               exposed_mask |= arm64_ftr_mask(ftrp);
> > > +
> > > +               if (ftr_val == ftr_lim)
> > > +                       continue;
> >
> > At first I thought that this check isn't necessary, it should be
> > covered by the check below (arm64_ftr_safe_value. However, I think
> > that it's needed for the FTR_HIGHER_OR_ZERO_SAFE case. If my
> > understanding is correct, it might be worth adding a comment, or even
> > encapsulating this logic in a arm64_is_safe_value() function for
> > clarity.
>
> In my understanding, arm64_ftr_safe_value() provides a safe value
> when two values are different, and I think there is nothing special
> about the usage of this function (This is actually how the function
> is used by update_cpu_ftr_reg()).
> Without the check, it won't work for FTR_EXACT, but there might be
> more in the future.
>
> Perhaps it might be more straightforward to add the equality check
> into arm64_ftr_safe_value() ?

I don't think this would work for all callers of
arm64_ftr_safe_value(). The thing is arm64_ftr_safe_value() doesn't
check whether the value is safe, but it returns the safe value that
supports the highest feature. Whereas arm64_check_features() on the
other hand is trying to determine whether a value is safe.

If you move the equality check there it would work for
arm64_check_features(), but I am not convinced it wouldn't change the
behavior for init_cpu_ftr_reg() in the case of FTR_EXACT, unless this
never applies to override->val. What do you think?

Thanks,
/fuad


> >
> > > +
> > > +               if (ftr_val != arm64_ftr_safe_value(ftrp, ftr_val, ftr_lim))
> > > +                       return -E2BIG;
> > > +       }
> > > +
> > > +       /* Make sure that no unrecognized fields are set in @val. */
> > > +       if (val & ~exposed_mask)
> > > +               return -E2BIG;
> > > +
> > > +       return 0;
> > > +}
>
> Thanks,
> Reiji