Hi Fuad,
On Tue, Feb 1, 2022 at 6:14 AM Fuad Tabba <tabba@xxxxxxxxxx> wrote:
>
> 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.
Yes, I will add a comment for it.
>
> >
> > >
> > >
> > > > +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?
The equality check (simply returns the new value if new == cur) could
change a return value of arm64_ftr_safe_value only if ftr_ovr == ftr_new
for FTR_EXACT case. For init_cpu_ftr_reg, since ftr_ovr value doesn't
matter if ftr_ovr == ftr_new, I would think the override behavior itself
stays the same although the message that will be printed by
init_cpu_ftr_reg() will change ("ignoring override" => "already set").
Having said that, since the change (having arm64_ftr_safe_value does
the equality check) isn't necessary, either way is fine, and
I can keep the current implementation of arm64_ftr_safe_value().
Thanks,
Reiji
>
> 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
