[PATCH v6 01/25] KVM: arm64: Introduce a validation function for an ID register

[Reiji Watanabe]

Hi Oliver,

> On Thu, Mar 10, 2022 at 08:47:47PM -0800, Reiji Watanabe wrote:
> > Introduce arm64_check_features(), which does a basic validity checking
> > of an ID register value against the register's limit value, which is
> > generally the host's sanitized value.
> >
> > This function will be used by the following patches to check if an ID
> > register value that userspace tries to set for a guest can be supported
> > on the host.
> >
> > The validation is done using arm64_ftr_bits_kvm, which is created from
> > arm64_ftr_regs, with some entries overwritten by entries from
> > arm64_ftr_bits_kvm_override.
> >
> > Signed-off-by: Reiji Watanabe <reijiw at google.com>
>
> I have some concerns regarding the API between cpufeature and KVM that's
> being proposed here. It would appear that we are adding some of KVM's
> implementation details into the cpufeature code. In particular, I see
> that KVM's limitations on AA64DFR0 are being copied here.

I assume "KVM's limitation details" you meant is about
ftr_id_aa64dfr0_kvm.
Entries in arm64_ftr_bits_kvm_override shouldn't be added based
on KVM's implementation.  When cpufeature.c doesn't handle lower level
of (or fewer) features as the "safe" value for fields, the field should
be added to arm64_ftr_bits_kvm_override.  As PMUVER and DEBUGVER are not
treated as LOWER_SAFE, they were added in arm64_ftr_bits_kvm_override.
Having said that, although ftr_id_aa64dfr0 has PMUVER as signed field,
I didn't fix that in ftr_id_aa64dfr0_kvm, and the code abused that....
I should make PMUVER unsigned field, and fix cpufeature.c to validate
the field based on its special ID scheme for cleaner abstraction.
(And KVM should skip the cpufeature.c's PMUVER validation using
 id_reg_desc's ignore_mask and have KVM validate it locally based on
 the KVM's special requirement)


> Additionally, I think it would be preferable to expose this in a manner
> that does not require CONFIG_KVM guards in other parts of the kernel.
>
> WDYT about having KVM keep its set of feature overrides and otherwise
> rely on the kernel's feature tables? I messed around with the idea a
> bit until I could get something workable (attached). I only compile
> tested this :)

Thanks for the proposal!
But, providing the overrides to arm64_ftr_reg_valid() means that its
consumer knows implementation details of cpufeture.c (values of entries
in arm64_ftr_regs[]), which is a similar abstraction problem I want to
avoid (the default validation by cpufeature.c should be purely based on
ID schemes even with this option).

Another option that I considered earlier was having a full set of
arm64_ftr_bits in KVM for its validation. At the time, I thought
statically) having a full set of arm64_ftr_bits in KVM is not good in
terms of maintenance.  But, considering that again, since most of
fields are unsigned and lower safe fields, and KVM doesn't necessarily
have to statically have a full set of arm64_ftr_bits (can dynamically
generate during KVM's initialization), it may not be that bad.
So, I am thinking of exploring this option.

More specifically, changes in cpufeature.c from patch-1 will be below
and remove all other newly added codes from cpufeature.c.
(Need more changes in KVM)

---
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -3357,9 +3357,9 @@ static const struct arm64_ftr_bits
*get_arm64_ftr_bits_kvm(u32 sys_id)
  * scheme, the function checks if values of the fields in @val are the same
  * as the ones in @limit.
  */
-int arm64_check_features_kvm(u32 sys_reg, u64 val, u64 limit)
+int arm64_check_features(u32 sys_reg, const struct arm64_ftr_bits *ftrp,
+                            u64 val, u64 limit)
 {
-       const struct arm64_ftr_bits *ftrp = get_arm64_ftr_bits_kvm(sys_reg);
        u64 exposed_mask = 0;

        if (!ftrp)
---

Thanks,
Reiji