Hi Alex,
On Wed, Dec 1, 2021 at 7:24 AM Alexandru Elisei
<alexandru.elisei@xxxxxxx> wrote:
>
> Hi Reiji,
>
> On Tue, Nov 16, 2021 at 10:43:33PM -0800, Reiji Watanabe wrote:
> > This patch lays the groundwork to make ID registers writable.
> >
> > Introduce struct id_reg_info for an ID register to manage the
> > register specific control of its value for the guest, and provide set
> > of functions commonly used for ID registers to make them writable.
> >
> > The id_reg_info is used to do register specific initialization,
> > validation of the ID register and etc. Not all ID registers must
> > have the id_reg_info. ID registers that don't have the id_reg_info
> > are handled in a common way that is applied to all ID registers.
> >
> > At present, changing an ID register from userspace is allowed only
> > if the ID register has the id_reg_info, but that will be changed
> > by the following patches.
> >
> > No ID register has the structure yet and the following patches
> > will add the id_reg_info for some ID registers.
> >
> > Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> > ---
> > arch/arm64/include/asm/sysreg.h | 1 +
> > arch/arm64/kvm/sys_regs.c | 226 ++++++++++++++++++++++++++++++--
> > 2 files changed, 218 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index 16b3f1a1d468..597609f26331 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -1197,6 +1197,7 @@
> > #define ICH_VTR_TDS_MASK (1 << ICH_VTR_TDS_SHIFT)
> >
> > #define ARM64_FEATURE_FIELD_BITS 4
> > +#define ARM64_FEATURE_FIELD_MASK ((1ull << ARM64_FEATURE_FIELD_BITS) - 1)
> >
> > /* Create a mask for the feature bits of the specified feature. */
> > #define ARM64_FEATURE_MASK(x) (GENMASK_ULL(x##_SHIFT + ARM64_FEATURE_FIELD_BITS - 1, x##_SHIFT))
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 5608d3410660..1552cd5581b7 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -265,6 +265,181 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
> > return read_zero(vcpu, p);
> > }
> >
> > +/*
> > + * A value for FCT_LOWER_SAFE must be zero and changing that will affect
> > + * ftr_check_types of id_reg_info.
> > + */
> > +enum feature_check_type {
> > + FCT_LOWER_SAFE = 0,
> > + FCT_HIGHER_SAFE,
> > + FCT_HIGHER_OR_ZERO_SAFE,
> > + FCT_EXACT,
> > + FCT_EXACT_OR_ZERO_SAFE,
> > + FCT_IGNORE, /* Don't check (any value is fine) */
> > +};
> > +
> > +static int arm64_check_feature_one(enum feature_check_type type, int val,
> > + int limit)
> > +{
> > + bool is_safe = false;
> > +
> > + if (val == limit)
> > + return 0;
> > +
> > + switch (type) {
> > + case FCT_LOWER_SAFE:
> > + is_safe = (val <= limit);
> > + break;
> > + case FCT_HIGHER_OR_ZERO_SAFE:
> > + if (val == 0) {
> > + is_safe = true;
> > + break;
> > + }
> > + fallthrough;
> > + case FCT_HIGHER_SAFE:
> > + is_safe = (val >= limit);
> > + break;
> > + case FCT_EXACT:
> > + break;
> > + case FCT_EXACT_OR_ZERO_SAFE:
> > + is_safe = (val == 0);
> > + break;
> > + case FCT_IGNORE:
>
> What happens if the a new feature is added and the field has a particular
> meaning? How are you going to deal with old userspace implementations that
> use a value here which now is not allowed or it affects the guest?
With this v3 series, unless KVM is changed for the new field,
a new feature will be treated as lower safe (that's the default).
If the field won't fit any of those cases, FCT_IGNORE needs to be
used for the field, and the ID register specific validation function,
which will be registered in id_reg_info, needs to validate the field.
Old userspace implementation shouldn't be affected because the default
values (the values right after the first KVM_ARM_VCPU_INIT) for
ID registers won't be changed by this series (patch-9 changes
AA64DFR0.PMUVER/DFR0.PERFMON but it is due to a bug fix), and the
default value, which is basically same as @limit (or indicates
less or smaller level of features than @limit for features that
can be configured by KVM_ARM_VCPU_INIT, etc), is always allowed
by arm64_check_feature_one().
Having said that, arm64_check_feature_one() will be gone from the next
version, and the similar checking will be done by a new function in
arch/arm64/kernel/cpufeature.c that will use arm64_ftr_bits instead.
https://lore.kernel.org/all/CAAeT=FxwzRF0YZmmoEmq3xRHnhun-BCx_FeEQrOVLgzwseSy4w@xxxxxxxxxxxxxx/
Unless KVM is changed for the new field, it will be validated based
on arm64_ftr_bits for the field. If KVM needs to handle the field
differently, then we will have the new function ignore the field,
and will have the ID register specific validation function handle
the field.
Thanks,
Reiji
