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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
