| Message ID | 20220106042708.2869332-2-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
On Wed, Jan 5, 2022 at 8:28 PM Reiji Watanabe <reijiw@google.com> 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@google.com> > --- > 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; > +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); > + 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 = ®_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 = ®_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; I've just noticed that the patch has a problem in terms of memory ordering. I'm thinking of fixing the code above as follows in the v5 patch. --- <...> arm64_ftr_bits_kvm_nentries = nent; /* * Make sure any data written earlier in this function are visible * from other CPUs before setting arm64_ftr_bits_kvm. */ smp_wmb(); WRITE_ONCE(arm64_ftr_bits_kvm, reg_bits_array); <...> --- Also, I will fix the reader side code of those data in get_arm64_ftr_bits_kvm(). Thanks, Reiji > + 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); > + u64 exposed_mask = 0; > + > + if (!ftrp) > + return -ENOENT; > + > + for (; ftrp->width; ftrp++) { > + s64 ftr_val = arm64_ftr_value(ftrp, val); > + s64 ftr_lim = arm64_ftr_value(ftrp, limit); > + > + exposed_mask |= arm64_ftr_mask(ftrp); > + > + if (ftr_val == ftr_lim) > + continue; > + > + if (ftr_val != arm64_ftr_safe_value(ftrp, ftr_val, ftr_lim)) > + return -E2BIG; > + } > + > + /* Make sure that no unrecognized fields are set in @val. */ > + if (val & ~exposed_mask) > + return -E2BIG; > + > + return 0; > +} > +#endif /* CONFIG_KVM */ > -- > 2.34.1.448.ga2b2bfdf31-goog >
Hi Reiji, ... > + > +/* > + * 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, s/overrite/override > + struct arm64_ftr_bits *new_ftrp) Should this be const struct arm64_ftr_bits *new_ftrp, which would also make it consistent with copy_arm64_ftr_bits()? > +{ > + 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; > + } > + } > + } > +} > + ... > +/* > + * 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); > + 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; Should this be -ENOMEM? > + goto unlock_exit; > + } > + > + /* Copy entries from arm64_ftr_regs to reg_bits_array */ > + for (i = 0; i < nent; i++) { > + bits = ®_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 = ®_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; > + } > + } Could you please explain using ftr_temp[] and changing the value in arm64_ftr_bits_kvm_override, rather than just arm64_ftr_reg_bits_overrite(bits->ftr_bits, o_bits->ftr_bits)? > +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(); Rather than doing this check, can we just initialize it earlier, maybe (indirectly) via kvm_arch_init_vm() or around the same time? > + 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); > + u64 exposed_mask = 0; > + > + if (!ftrp) > + return -ENOENT; > + > + for (; ftrp->width; ftrp++) { > + s64 ftr_val = arm64_ftr_value(ftrp, val); > + s64 ftr_lim = arm64_ftr_value(ftrp, limit); > + > + exposed_mask |= arm64_ftr_mask(ftrp); > + > + if (ftr_val == ftr_lim) > + continue; At first I thought that this check isn't necessary, it should be covered by the check below (arm64_ftr_safe_value. However, I think that it's needed for the FTR_HIGHER_OR_ZERO_SAFE case. If my understanding is correct, it might be worth adding a comment, or even encapsulating this logic in a arm64_is_safe_value() function for clarity. > + > + if (ftr_val != arm64_ftr_safe_value(ftrp, ftr_val, ftr_lim)) > + return -E2BIG; > + } > + > + /* Make sure that no unrecognized fields are set in @val. */ > + if (val & ~exposed_mask) > + return -E2BIG; > + > + return 0; > +} Thanks, /fuad > +#endif /* CONFIG_KVM */ > -- > 2.34.1.448.ga2b2bfdf31-goog > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
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@google.com> > --- > 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(). > +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?)? > + 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 = ®_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 = ®_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. > + u64 exposed_mask = 0; > + > + if (!ftrp) > + return -ENOENT; > + > + for (; ftrp->width; ftrp++) { > + s64 ftr_val = arm64_ftr_value(ftrp, val); > + s64 ftr_lim = arm64_ftr_value(ftrp, limit); > + > + exposed_mask |= arm64_ftr_mask(ftrp); > + > + if (ftr_val == ftr_lim) > + continue; > + > + if (ftr_val != arm64_ftr_safe_value(ftrp, ftr_val, ftr_lim)) > + return -E2BIG; > + } > + > + /* Make sure that no unrecognized fields are set in @val. */ > + if (val & ~exposed_mask) > + return -E2BIG; > + > + return 0; > +} > +#endif /* CONFIG_KVM */ > -- > 2.34.1.448.ga2b2bfdf31-goog >
Hi Fuad, > > +/* > > + * 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, > > s/overrite/override Thank you for catching this. I will fix it. > > + struct arm64_ftr_bits *new_ftrp) > > Should this be const struct arm64_ftr_bits *new_ftrp, which would also > make it consistent with copy_arm64_ftr_bits()? Yes, I will make new_ftrp const. > > > +{ > > + 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; > > + } > > + } > > + } > > +} > > + > > ... > > > +/* > > + * 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); > > + 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; > > Should this be -ENOMEM? Yes, I will fix it. > > + goto unlock_exit; > > + } > > + > > + /* Copy entries from arm64_ftr_regs to reg_bits_array */ > > + for (i = 0; i < nent; i++) { > > + bits = ®_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 = ®_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; > > + } > > + } > > Could you please explain using ftr_temp[] and changing the value in > arm64_ftr_bits_kvm_override, rather than just > arm64_ftr_reg_bits_overrite(bits->ftr_bits, o_bits->ftr_bits)? I would like to maintain the order of the entries in the original ftr_bits so that (future) functions that work for the original ones also work for the KVM's. The copy and override is an easy way to do that. The same thing can be done without ftr_temp[], but it would look a bit tricky. If we assume the order shouldn't matter or entries in ftr_bits are always in descending order, just changing the value in arm64_ftr_bits_kvm_override would be a much simpler way though. > > > > +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(); > > Rather than doing this check, can we just initialize it earlier, maybe > (indirectly) via kvm_arch_init_vm() or around the same time? Thank you for the comment. I will consider when it should be initialized. ( perhaps even earlier than kvm_arch_init_vm()) > > > > + 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); > > + u64 exposed_mask = 0; > > + > > + if (!ftrp) > > + return -ENOENT; > > + > > + for (; ftrp->width; ftrp++) { > > + s64 ftr_val = arm64_ftr_value(ftrp, val); > > + s64 ftr_lim = arm64_ftr_value(ftrp, limit); > > + > > + exposed_mask |= arm64_ftr_mask(ftrp); > > + > > + if (ftr_val == ftr_lim) > > + continue; > > At first I thought that this check isn't necessary, it should be > covered by the check below (arm64_ftr_safe_value. However, I think > that it's needed for the FTR_HIGHER_OR_ZERO_SAFE case. If my > understanding is correct, it might be worth adding a comment, or even > encapsulating this logic in a arm64_is_safe_value() function for > clarity. In my understanding, arm64_ftr_safe_value() provides a safe value when two values are different, and I think there is nothing special about the usage of this function (This is actually how the function is used by update_cpu_ftr_reg()). Without the check, it won't work for FTR_EXACT, but there might be more in the future. Perhaps it might be more straightforward to add the equality check into arm64_ftr_safe_value() ? > > > + > > + if (ftr_val != arm64_ftr_safe_value(ftrp, ftr_val, ftr_lim)) > > + return -E2BIG; > > + } > > + > > + /* Make sure that no unrecognized fields are set in @val. */ > > + if (val & ~exposed_mask) > > + return -E2BIG; > > + > > + return 0; > > +} Thanks, Reiji
Hi Ricardo, On Tue, Jan 25, 2022 at 8:30 PM Ricardo Koller <ricarkol@google.com> 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@google.com> > > --- > > 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 = ®_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 = ®_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
Hi Reiji, ... > > Could you please explain using ftr_temp[] and changing the value in > > arm64_ftr_bits_kvm_override, rather than just > > arm64_ftr_reg_bits_overrite(bits->ftr_bits, o_bits->ftr_bits)? > > I would like to maintain the order of the entries in the original > ftr_bits so that (future) functions that work for the original ones > also work for the KVM's. > The copy and override is an easy way to do that. The same thing can > be done without ftr_temp[], but it would look a bit tricky. > > If we assume the order shouldn't matter or entries in ftr_bits > are always in descending order, just changing the value in > arm64_ftr_bits_kvm_override would be a much simpler way though. Could you please add a comment in that case? I did find it to be confusing until I read your explanation here. > > > > > > > > +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(); > > > > Rather than doing this check, can we just initialize it earlier, maybe > > (indirectly) via kvm_arch_init_vm() or around the same time? > > Thank you for the comment. > I will consider when it should be initialized. > ( perhaps even earlier than kvm_arch_init_vm()) > > > > > > > > + 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); > > > + u64 exposed_mask = 0; > > > + > > > + if (!ftrp) > > > + return -ENOENT; > > > + > > > + for (; ftrp->width; ftrp++) { > > > + s64 ftr_val = arm64_ftr_value(ftrp, val); > > > + s64 ftr_lim = arm64_ftr_value(ftrp, limit); > > > + > > > + exposed_mask |= arm64_ftr_mask(ftrp); > > > + > > > + if (ftr_val == ftr_lim) > > > + continue; > > > > At first I thought that this check isn't necessary, it should be > > covered by the check below (arm64_ftr_safe_value. However, I think > > that it's needed for the FTR_HIGHER_OR_ZERO_SAFE case. If my > > understanding is correct, it might be worth adding a comment, or even > > encapsulating this logic in a arm64_is_safe_value() function for > > clarity. > > In my understanding, arm64_ftr_safe_value() provides a safe value > when two values are different, and I think there is nothing special > about the usage of this function (This is actually how the function > is used by update_cpu_ftr_reg()). > Without the check, it won't work for FTR_EXACT, but there might be > more in the future. > > Perhaps it might be more straightforward to add the equality check > into arm64_ftr_safe_value() ? I don't think this would work for all callers of arm64_ftr_safe_value(). The thing is arm64_ftr_safe_value() doesn't check whether the value is safe, but it returns the safe value that supports the highest feature. Whereas arm64_check_features() on the other hand is trying to determine whether a value is safe. If you move the equality check there it would work for arm64_check_features(), but I am not convinced it wouldn't change the behavior for init_cpu_ftr_reg() in the case of FTR_EXACT, unless this never applies to override->val. What do you think? Thanks, /fuad > > > > > + > > > + if (ftr_val != arm64_ftr_safe_value(ftrp, ftr_val, ftr_lim)) > > > + return -E2BIG; > > > + } > > > + > > > + /* Make sure that no unrecognized fields are set in @val. */ > > > + if (val & ~exposed_mask) > > > + return -E2BIG; > > > + > > > + return 0; > > > +} > > Thanks, > Reiji
Hi Fuad, On Tue, Feb 1, 2022 at 6:14 AM Fuad Tabba <tabba@google.com> wrote: > > Hi Reiji, > > ... > > > > Could you please explain using ftr_temp[] and changing the value in > > > arm64_ftr_bits_kvm_override, rather than just > > > arm64_ftr_reg_bits_overrite(bits->ftr_bits, o_bits->ftr_bits)? > > > > I would like to maintain the order of the entries in the original > > ftr_bits so that (future) functions that work for the original ones > > also work for the KVM's. > > The copy and override is an easy way to do that. The same thing can > > be done without ftr_temp[], but it would look a bit tricky. > > > > If we assume the order shouldn't matter or entries in ftr_bits > > are always in descending order, just changing the value in > > arm64_ftr_bits_kvm_override would be a much simpler way though. > > Could you please add a comment in that case? I did find it to be > confusing until I read your explanation here. Yes, I will add a comment for it. > > > > > > > > > > > > > +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(); > > > > > > Rather than doing this check, can we just initialize it earlier, maybe > > > (indirectly) via kvm_arch_init_vm() or around the same time? > > > > Thank you for the comment. > > I will consider when it should be initialized. > > ( perhaps even earlier than kvm_arch_init_vm()) > > > > > > > > > > > > + 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); > > > > + u64 exposed_mask = 0; > > > > + > > > > + if (!ftrp) > > > > + return -ENOENT; > > > > + > > > > + for (; ftrp->width; ftrp++) { > > > > + s64 ftr_val = arm64_ftr_value(ftrp, val); > > > > + s64 ftr_lim = arm64_ftr_value(ftrp, limit); > > > > + > > > > + exposed_mask |= arm64_ftr_mask(ftrp); > > > > + > > > > + if (ftr_val == ftr_lim) > > > > + continue; > > > > > > At first I thought that this check isn't necessary, it should be > > > covered by the check below (arm64_ftr_safe_value. However, I think > > > that it's needed for the FTR_HIGHER_OR_ZERO_SAFE case. If my > > > understanding is correct, it might be worth adding a comment, or even > > > encapsulating this logic in a arm64_is_safe_value() function for > > > clarity. > > > > In my understanding, arm64_ftr_safe_value() provides a safe value > > when two values are different, and I think there is nothing special > > about the usage of this function (This is actually how the function > > is used by update_cpu_ftr_reg()). > > Without the check, it won't work for FTR_EXACT, but there might be > > more in the future. > > > > Perhaps it might be more straightforward to add the equality check > > into arm64_ftr_safe_value() ? > > I don't think this would work for all callers of > arm64_ftr_safe_value(). The thing is arm64_ftr_safe_value() doesn't > check whether the value is safe, but it returns the safe value that > supports the highest feature. Whereas arm64_check_features() on the > other hand is trying to determine whether a value is safe. > > If you move the equality check there it would work for > arm64_check_features(), but I am not convinced it wouldn't change the > behavior for init_cpu_ftr_reg() in the case of FTR_EXACT, unless this > never applies to override->val. What do you think? The equality check (simply returns the new value if new == cur) could change a return value of arm64_ftr_safe_value only if ftr_ovr == ftr_new for FTR_EXACT case. For init_cpu_ftr_reg, since ftr_ovr value doesn't matter if ftr_ovr == ftr_new, I would think the override behavior itself stays the same although the message that will be printed by init_cpu_ftr_reg() will change ("ignoring override" => "already set"). Having said that, since the change (having arm64_ftr_safe_value does the equality check) isn't necessary, either way is fine, and I can keep the current implementation of arm64_ftr_safe_value(). Thanks, Reiji > > Thanks, > /fuad > > > > > > > > > + > > > > + if (ftr_val != arm64_ftr_safe_value(ftrp, ftr_val, ftr_lim)) > > > > + return -E2BIG; > > > > + } > > > > + > > > > + /* Make sure that no unrecognized fields are set in @val. */ > > > > + if (val & ~exposed_mask) > > > > + return -E2BIG; > > > > + > > > > + return 0; > > > > +} > > > > Thanks, > > Reiji
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; +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); + 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 = ®_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 = ®_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); + u64 exposed_mask = 0; + + if (!ftrp) + return -ENOENT; + + for (; ftrp->width; ftrp++) { + s64 ftr_val = arm64_ftr_value(ftrp, val); + s64 ftr_lim = arm64_ftr_value(ftrp, limit); + + exposed_mask |= arm64_ftr_mask(ftrp); + + if (ftr_val == ftr_lim) + continue; + + if (ftr_val != arm64_ftr_safe_value(ftrp, ftr_val, ftr_lim)) + return -E2BIG; + } + + /* Make sure that no unrecognized fields are set in @val. */ + if (val & ~exposed_mask) + return -E2BIG; + + return 0; +} +#endif /* CONFIG_KVM */
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@google.com> --- arch/arm64/include/asm/cpufeature.h | 1 + arch/arm64/kernel/cpufeature.c | 228 ++++++++++++++++++++++++++++ 2 files changed, 229 insertions(+)