Re: [RFC PATCH v4 01/26] KVM: arm64: Introduce a validation function for an ID register

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

 



Hi Ricardo,

On Tue, Jan 25, 2022 at 8:30 PM Ricardo Koller <ricarkol@xxxxxxxxxx> wrote:
>
> Hey Reiji,
>
> On Wed, Jan 05, 2022 at 08:26:43PM -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@xxxxxxxxxx>
> > ---
> >  arch/arm64/include/asm/cpufeature.h |   1 +
> >  arch/arm64/kernel/cpufeature.c      | 228 ++++++++++++++++++++++++++++
> >  2 files changed, 229 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> > index ef6be92b1921..eda7ddbed8cf 100644
> > --- a/arch/arm64/include/asm/cpufeature.h
> > +++ b/arch/arm64/include/asm/cpufeature.h
> > @@ -631,6 +631,7 @@ void check_local_cpu_capabilities(void);
> >
> >  u64 read_sanitised_ftr_reg(u32 id);
> >  u64 __read_sysreg_by_encoding(u32 sys_id);
> > +int arm64_check_features(u32 sys_reg, u64 val, u64 limit);
> >
> >  static inline bool cpu_supports_mixed_endian_el0(void)
> >  {
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 6f3e677d88f1..48dff8b101d9 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -3140,3 +3140,231 @@ ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr,
> >               return sprintf(buf, "Vulnerable\n");
> >       }
> >  }
> > +
> > +#ifdef CONFIG_KVM
> > +/*
> > + * arm64_ftr_bits_kvm[] is used for KVM to check if features that are
> > + * indicated in an ID register value for the guest are available on the host.
> > + * arm64_ftr_bits_kvm[] is created based on arm64_ftr_regs[].  But, for
> > + * registers for which arm64_ftr_bits_kvm_override[] has a corresponding
> > + * entry, replace arm64_ftr_bits entries in arm64_ftr_bits_kvm[] with the
> > + * ones in arm64_ftr_bits_kvm_override[].
> > + */
> > +static struct __ftr_reg_bits_entry *arm64_ftr_bits_kvm;
> > +static size_t arm64_ftr_bits_kvm_nentries;
>
> I don't think this is really needed, as arm64_ftr_bits_kvm_override has
> to have the same size as arm64_ftr_bits_kvm. You could use
> ARRAY_SIZE(arm64_ftr_regs) like in get_arm64_ftr_reg_nowarn().

Thanks for the review!
Yes, you are right. I will remove arm64_ftr_bits_kvm_nentries,
and use ARRAY_SIZE(arm64_ftr_regs) instead.

> > +static DEFINE_MUTEX(arm64_ftr_bits_kvm_lock);
> > +
> > +/*
> > + * Number of arm64_ftr_bits entries for each register.
> > + * (Number of 4 bits fields in 64 bit register + 1 entry for ARM64_FTR_END)
> > + */
> > +#define      MAX_FTR_BITS_LEN        17
> > +
> > +/* Use FTR_LOWER_SAFE for AA64DFR0_EL1.PMUVER and AA64DFR0_EL1.DEBUGVER. */
> > +static struct arm64_ftr_bits ftr_id_aa64dfr0_kvm[MAX_FTR_BITS_LEN] = {
> > +     S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
> > +     ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
> > +     ARM64_FTR_END,
> > +};
> > +
> > +#define      ARM64_FTR_REG_BITS(id, table)   {       \
> > +     .sys_id = id,                           \
> > +     .ftr_bits = &((table)[0]),              \
> > +}
> > +
> > +struct __ftr_reg_bits_entry {
> > +     u32     sys_id;
> > +     struct arm64_ftr_bits   *ftr_bits;
> > +};
> > +
> > +/*
> > + * All entries in arm64_ftr_bits_kvm_override[] are used to override
> > + * the corresponding entries in arm64_ftr_bits_kvm[].
> > + */
> > +static struct __ftr_reg_bits_entry arm64_ftr_bits_kvm_override[] = {
> > +     ARM64_FTR_REG_BITS(SYS_ID_AA64DFR0_EL1, ftr_id_aa64dfr0_kvm),
> > +};
> > +
> > +/*
> > + * Override entries in @orig_ftrp with the ones in @new_ftrp when their shift
> > + * fields match.  The last entry of @orig_ftrp and @new_ftrp must be
> > + * ARM64_FTR_END (.width == 0).
> > + */
> > +static void arm64_ftr_reg_bits_overrite(struct arm64_ftr_bits *orig_ftrp,
> > +                                     struct arm64_ftr_bits *new_ftrp)
> > +{
> > +     struct arm64_ftr_bits *o_ftrp, *n_ftrp;
> > +
> > +     for (n_ftrp = new_ftrp; n_ftrp->width; n_ftrp++) {
> > +             for (o_ftrp = orig_ftrp; o_ftrp->width; o_ftrp++) {
> > +                     if (o_ftrp->shift == n_ftrp->shift) {
> > +                             *o_ftrp = *n_ftrp;
> > +                             break;
> > +                     }
> > +             }
> > +     }
> > +}
> > +
> > +/*
> > + * Copy arm64_ftr_bits entries from @src_ftrp to @dst_ftrp.  The last entries
> > + * of @dst_ftrp and @src_ftrp must be ARM64_FTR_END (.width == 0).
> > + */
> > +static void copy_arm64_ftr_bits(struct arm64_ftr_bits *dst_ftrp,
> > +                             const struct arm64_ftr_bits *src_ftrp)
> > +{
> > +     int i = 0;
> > +
> > +     for (; src_ftrp[i].width; i++) {
> > +             if (WARN_ON_ONCE(i >= (MAX_FTR_BITS_LEN - 1)))
> > +                     break;
> > +
> > +             dst_ftrp[i] = src_ftrp[i];
> > +     }
> > +
> > +     dst_ftrp[i].width = 0;
> > +}
> > +
> > +/*
> > + * Initialize arm64_ftr_bits_kvm.  Copy arm64_ftr_bits for each ID register
> > + * from arm64_ftr_regs to arm64_ftr_bits_kvm, and then override entries in
> > + * arm64_ftr_bits_kvm with ones in arm64_ftr_bits_kvm_override.
> > + */
> > +static int init_arm64_ftr_bits_kvm(void)
> > +{
> > +     struct arm64_ftr_bits ftr_temp[MAX_FTR_BITS_LEN];
> > +     static struct __ftr_reg_bits_entry *reg_bits_array, *bits, *o_bits;
> > +     int i, j, nent, ret;
> > +
> > +     mutex_lock(&arm64_ftr_bits_kvm_lock);
>
> This is initialized lazily, whenever KVM calls arm64_check_features(). I
> guess that's why it needs the lock (and possibly a barrier as you
> mentoin in your reply). Would it be possible to simplify things by
> initializing arm64_ftr_bits_kvm somewhere at boot time (in
> init_cpu_features maybe?)?

I agree that it could simplify the code.
I will look into initializing that earlier.


>
> > +     if (arm64_ftr_bits_kvm) {
> > +             /* Already initialized */
> > +             ret = 0;
> > +             goto unlock_exit;
> > +     }
> > +
> > +     nent = ARRAY_SIZE(arm64_ftr_regs);
> > +     reg_bits_array = kcalloc(nent, sizeof(struct __ftr_reg_bits_entry),
> > +                              GFP_KERNEL);
> > +     if (!reg_bits_array) {
> > +             ret = ENOMEM;
> > +             goto unlock_exit;
> > +     }
> > +
> > +     /* Copy entries from arm64_ftr_regs to reg_bits_array */
> > +     for (i = 0; i < nent; i++) {
> > +             bits = &reg_bits_array[i];
> > +             bits->sys_id = arm64_ftr_regs[i].sys_id;
> > +             bits->ftr_bits = (struct arm64_ftr_bits *)arm64_ftr_regs[i].reg->ftr_bits;
> > +     };
> > +
> > +     /*
> > +      * Override the entries in reg_bits_array with the ones in
> > +      * arm64_ftr_bits_kvm_override.
> > +      */
> > +     for (i = 0; i < ARRAY_SIZE(arm64_ftr_bits_kvm_override); i++) {
> > +             o_bits = &arm64_ftr_bits_kvm_override[i];
> > +             for (j = 0; j < nent; j++) {
> > +                     bits = &reg_bits_array[j];
> > +                     if (bits->sys_id != o_bits->sys_id)
> > +                             continue;
> > +
> > +                     memset(ftr_temp, 0, sizeof(ftr_temp));
> > +
> > +                     /*
> > +                      * Temporary save all entries in o_bits->ftr_bits
> > +                      * to ftr_temp.
> > +                      */
> > +                     copy_arm64_ftr_bits(ftr_temp, o_bits->ftr_bits);
> > +
> > +                     /*
> > +                      * Copy entries from bits->ftr_bits to o_bits->ftr_bits.
> > +                      */
> > +                     copy_arm64_ftr_bits(o_bits->ftr_bits, bits->ftr_bits);
> > +
> > +                     /*
> > +                      * Override entries in o_bits->ftr_bits with the
> > +                      * saved ones, and update bits->ftr_bits with
> > +                      * o_bits->ftr_bits.
> > +                      */
> > +                     arm64_ftr_reg_bits_overrite(o_bits->ftr_bits, ftr_temp);
> > +                     bits->ftr_bits = o_bits->ftr_bits;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     arm64_ftr_bits_kvm_nentries = nent;
> > +     arm64_ftr_bits_kvm = reg_bits_array;
> > +     ret = 0;
> > +
> > +unlock_exit:
> > +     mutex_unlock(&arm64_ftr_bits_kvm_lock);
> > +     return ret;
> > +}
> > +
> > +static int search_cmp_ftr_reg_bits(const void *id, const void *regp)
> > +{
> > +     return ((int)(unsigned long)id -
> > +             (int)((const struct __ftr_reg_bits_entry *)regp)->sys_id);
> > +}
> > +
> > +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();
> > +             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);
>
> Given that this is to be used only by KVM (and it's inside CONFIG_KVM),
> it might be better to have "kvm" somewhere in its name.

Yes, that might be better. I will change the name.

Thanks,
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