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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
