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().
> +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
>
