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

[Reiji Watanabe]

Hi Fuad,

> > +/*
> > + * Override entries in @orig_ftrp with the ones in @new_ftrp when their shift
> > + * fields match.  The last entry of @orig_ftrp and @new_ftrp must be
> > + * ARM64_FTR_END (.width == 0).
> > + */
> > +static void arm64_ftr_reg_bits_overrite(struct arm64_ftr_bits *orig_ftrp,
>
> s/overrite/override

Thank you for catching this. I will fix it.


> > +                                       struct arm64_ftr_bits *new_ftrp)
>
> Should this be const struct arm64_ftr_bits *new_ftrp, which would also
> make it consistent with copy_arm64_ftr_bits()?

Yes, I will make new_ftrp const.

>
> > +{
> > +       struct arm64_ftr_bits *o_ftrp, *n_ftrp;
> > +
> > +       for (n_ftrp = new_ftrp; n_ftrp->width; n_ftrp++) {
> > +               for (o_ftrp = orig_ftrp; o_ftrp->width; o_ftrp++) {
> > +                       if (o_ftrp->shift == n_ftrp->shift) {
> > +                               *o_ftrp = *n_ftrp;
> > +                               break;
> > +                       }
> > +               }
> > +       }
> > +}
> > +
>
> ...
>
> > +/*
> > + * Initialize arm64_ftr_bits_kvm.  Copy arm64_ftr_bits for each ID register
> > + * from arm64_ftr_regs to arm64_ftr_bits_kvm, and then override entries in
> > + * arm64_ftr_bits_kvm with ones in arm64_ftr_bits_kvm_override.
> > + */
> > +static int init_arm64_ftr_bits_kvm(void)
> > +{
> > +       struct arm64_ftr_bits ftr_temp[MAX_FTR_BITS_LEN];
> > +       static struct __ftr_reg_bits_entry *reg_bits_array, *bits, *o_bits;
> > +       int i, j, nent, ret;
> > +
> > +       mutex_lock(&arm64_ftr_bits_kvm_lock);
> > +       if (arm64_ftr_bits_kvm) {
> > +               /* Already initialized */
> > +               ret = 0;
> > +               goto unlock_exit;
> > +       }
> > +
> > +       nent = ARRAY_SIZE(arm64_ftr_regs);
> > +       reg_bits_array = kcalloc(nent, sizeof(struct __ftr_reg_bits_entry),
> > +                                GFP_KERNEL);
> > +       if (!reg_bits_array) {
> > +               ret = ENOMEM;
>
> Should this be -ENOMEM?

Yes, I will fix it.


> > +               goto unlock_exit;
> > +       }
> > +
> > +       /* Copy entries from arm64_ftr_regs to reg_bits_array */
> > +       for (i = 0; i < nent; i++) {
> > +               bits = &reg_bits_array[i];
> > +               bits->sys_id = arm64_ftr_regs[i].sys_id;
> > +               bits->ftr_bits = (struct arm64_ftr_bits *)arm64_ftr_regs[i].reg->ftr_bits;
> > +       };
> > +
> > +       /*
> > +        * Override the entries in reg_bits_array with the ones in
> > +        * arm64_ftr_bits_kvm_override.
> > +        */
> > +       for (i = 0; i < ARRAY_SIZE(arm64_ftr_bits_kvm_override); i++) {
> > +               o_bits = &arm64_ftr_bits_kvm_override[i];
> > +               for (j = 0; j < nent; j++) {
> > +                       bits = &reg_bits_array[j];
> > +                       if (bits->sys_id != o_bits->sys_id)
> > +                               continue;
> > +
> > +                       memset(ftr_temp, 0, sizeof(ftr_temp));
> > +
> > +                       /*
> > +                        * Temporary save all entries in o_bits->ftr_bits
> > +                        * to ftr_temp.
> > +                        */
> > +                       copy_arm64_ftr_bits(ftr_temp, o_bits->ftr_bits);
> > +
> > +                       /*
> > +                        * Copy entries from bits->ftr_bits to o_bits->ftr_bits.
> > +                        */
> > +                       copy_arm64_ftr_bits(o_bits->ftr_bits, bits->ftr_bits);
> > +
> > +                       /*
> > +                        * Override entries in o_bits->ftr_bits with the
> > +                        * saved ones, and update bits->ftr_bits with
> > +                        * o_bits->ftr_bits.
> > +                        */
> > +                       arm64_ftr_reg_bits_overrite(o_bits->ftr_bits, ftr_temp);
> > +                       bits->ftr_bits = o_bits->ftr_bits;
> > +                       break;
> > +               }
> > +       }
>
> 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.


>
>
> > +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() ?

>
> > +
> > +               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