Re: [RFC PATCH v3 03/29] KVM: arm64: Introduce struct id_reg_info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux