Hi Eric,
On Wed, Nov 24, 2021 at 10:22 AM Eric Auger <eauger@xxxxxxxxxx> wrote:
>
> Hi Reiji,
>
> On 11/19/21 5:47 AM, Reiji Watanabe wrote:
> > Hi Eric,
> >
> > On Thu, Nov 18, 2021 at 12:36 PM Eric Auger <eauger@xxxxxxxxxx> wrote:
> >>
> >> Hi Reiji,
> >>
> >> On 11/17/21 7:43 AM, 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) */
> >> Maybe you can remove the _SAFE suffix (EXACT does not have it).
> >
> > I am inclined to keep 'SAFE' (otherwise, I am likely to forget
> > if lower is safe or not).
> >
> >> s/EXACT/EQUAL ?
> >
> > I will fix that FCT_EXACT to FCT_EQUAL_SAFE.
> >
> >>> +};
> >>> +
> >>> +static int arm64_check_feature_one(enum feature_check_type type, int val,
> >>> + int limit)
> >>> +{
> >>> + bool is_safe = false;
> >>> +
> >>> + if (val == limit)
> >>> + return 0;
> >> even if the type is unexpected?
> >
> > I will remove it.
> then you need to modify the handling of FCT_EXACT*.
Thank you for the comment. Yes, I understand.
That being said, I might probably make "val == limit" safe
unconditionally as I will move those implementations
to arch/arm64/kernel/cpufeature.c and utilize existing things
as much as I can.
(https://lore.kernel.org/linux-arm-kernel/CAAeT=FxwzRF0YZmmoEmq3xRHnhun-BCx_FeEQrOVLgzwseSy4w@xxxxxxxxxxxxxx/)
> >>> +
> >>> + 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:
> >>> + is_safe = true;
> >>> + break;
> >>> + default:
> >>> + WARN_ONCE(1, "Unexpected feature_check_type (%d)\n", type);
> >>> + break;
> >>> + }
> >>> +
> >>> + return is_safe ? 0 : -1;
> >>> +}
> >>> +
> >>> +#define FCT_TYPE_MASK 0x7
> >>> +#define FCT_TYPE_SHIFT 1
> >>> +#define FCT_SIGN_MASK 0x1
> >>> +#define FCT_SIGN_SHIFT 0
> >>> +#define FCT_TYPE(val) ((val >> FCT_TYPE_SHIFT) & FCT_TYPE_MASK)
> >>> +#define FCT_SIGN(val) ((val >> FCT_SIGN_SHIFT) & FCT_SIGN_MASK)
> >>> +
> >>> +#define MAKE_FCT(shift, type, sign) \
> >>> + ((u64)((((type) & FCT_TYPE_MASK) << FCT_TYPE_SHIFT) | \
> >>> + (((sign) & FCT_SIGN_MASK) << FCT_SIGN_SHIFT)) << (shift))
> >>> +
> >>> +/* For signed field */
> >>> +#define S_FCT(shift, type) MAKE_FCT(shift, type, 1)
> >>> +/* For unigned field */
> >>> +#define U_FCT(shift, type) MAKE_FCT(shift, type, 0)
> >>> +
> >>> +/*
> >>> + * @val and @lim are both a value of the ID register. The function checks
> >>> + * if all features indicated in @val can be supported for guests on the host,
> >>> + * which supports features indicated in @lim. @check_types indicates how> + * features in the ID register needs to be checked.
> >>> + * See comments for id_reg_info's ftr_check_types field for more detail.
> >> What about RES0 fields which may exist? add a comment to reassure about
> >> the fact they are properly handled if there are?
> >
> > Any fields including RES0 should be checked based on check_types.
> > I will explicitly state that in the comment.
> >
> >>> + */
> >>> +static int arm64_check_features(u64 check_types, u64 val, u64 lim)
> >>> +{
> >>> + int i;
> >>> +
> >>> + for (i = 0; i < 64; i += ARM64_FEATURE_FIELD_BITS) {
> >>> + u8 ftr_check = (check_types >> i) & ARM64_FEATURE_FIELD_MASK;
> >>> + bool is_sign = FCT_SIGN(ftr_check);
> >>> + enum feature_check_type fctype = FCT_TYPE(ftr_check);
> >>> + int fval, flim, ret;
> >>> +
> >>> + fval = cpuid_feature_extract_field(val, i, is_sign);
> >>> + flim = cpuid_feature_extract_field(lim, i, is_sign);
> >>> +
> >>> + ret = arm64_check_feature_one(fctype, fval, flim);
> >>> + if (ret)
> >>> + return -E2BIG;
> >>> + }
> >>> + return 0;
> >>> +}
> >>> +
> >>> +struct id_reg_info {
> >>> + u32 sys_reg; /* Register ID */
> >> use struct kernel-doc comments instead?
> >>> +
> >>> + /*
> >>> + * Limit value of the register for a vcpu. The value is the sanitized
> >>> + * system value with bits cleared for unsupported features for the
> >>> + * guest.
> >>> + */
> >>> + u64 vcpu_limit_val;
> >>> +
> >>> + /*
> >>> + * The ftr_check_types is comprised of a set of 4 bits fields.
> >> nit: s/bits field/bit field here and below
> >
> > I will fix them.
> >
> >>> + * Each 4 bits field is for a feature indicated by the same bits
> >>> + * field of the ID register and indicates how the feature support
> >>> + * for guests needs to be checked.
> >>> + * The bit 0 indicates that the corresponding ID register field
> >>> + * is signed(1) or unsigned(0).
> >>> + * The bits [3:1] hold feature_check_type for the field.
> >>> + * If all zero, all features in the ID register are treated as unsigned
> >>> + * fields and checked based on Principles of the ID scheme for fields
> >>> + * in ID registers (FCT_LOWER_SAFE of feature_check_type).
> >> values set by the guest are checked against host ID field values
> >> according to FCT_LOWER_SAFE test? You do not actually explicitly explain
> >> what the check is about although this may be obvious for you?
> >
> > How about this ?
> >
> > /*
> > * The ftr_check_types is comprised of a set of 4 bit fields.
> > * Each 4 bit field is for a feature indicated by the same bit field
> > * of the ID register and indicates how the field needs to be checked
> > * (by arm64_check_feature_one) against the host's ID field when
> > * userspace tries to set the register.
> > * The bit 0 indicates that the corresponding ID register field is
> > * signed(1) or unsigned(0). The bits [3:1] hold feature_check_type
> > * for the field (FCT_LOWER_SAFE == 0, etc).
> > * e.g. for ID_AA64PFR0_EL1.SVE(bits [35:32]), bits[35:32] of
> > * ftr_check_types for the register should be 0. It means the SVE
> > * field is treated as an unsigned field, and userspace can set the
> > * field to a equal or lower value than the host's ID field value.
> > */
> yep sounds clearer to me.
Thank you for the review !
(ftr_check_types will be gone due to the same reason above though...)
Regards,
Reiji
