Hi Oliver,
On Wed, Aug 2, 2023 at 10:28 AM Oliver Upton <oliver.upton@xxxxxxxxx> wrote:
>
> Hi Jing,
>
> On Tue, Aug 01, 2023 at 08:20:06AM -0700, Jing Zhang wrote:
> > Add tests to verify setting ID registers from userapce is handled
> > correctly by KVM. Also add a test case to use ioctl
> > KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS to get writable masks.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > ---
> > tools/arch/arm64/include/uapi/asm/kvm.h | 25 +++
> > tools/include/uapi/linux/kvm.h | 2 +
>
> Why is this diff needed? I thought we wound up using the latest headers
> from the kernel.
You are right. These changes are not needed. Will drop them.
>
> > tools/testing/selftests/kvm/Makefile | 1 +
>
> Need to add your file to .gitignore too.
Will do.
>
> > .../selftests/kvm/aarch64/set_id_regs.c | 191 ++++++++++++++++++
> > 4 files changed, 219 insertions(+)
> > create mode 100644 tools/testing/selftests/kvm/aarch64/set_id_regs.c
>
> [...]
>
> > diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> > new file mode 100644
> > index 000000000000..9c8f439ac7b3
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> > @@ -0,0 +1,191 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * set_id_regs - Test for setting ID register from usersapce.
> > + *
> > + * Copyright (c) 2023 Google LLC.
> > + *
> > + *
> > + * Test that KVM supports setting ID registers from userspace and handles the
> > + * feature set correctly.
> > + */
> > +
> > +#include <stdint.h>
> > +#include "kvm_util.h"
> > +#include "processor.h"
> > +#include "test_util.h"
> > +#include <linux/bitfield.h>
> > +
> > +#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffsl(_mask) - 1))
> > +#define field_prep(_mask, _val) (((_val) << (ffsl(_mask) - 1)) & (_mask))
> > +
>
> Shadowing the naming of the kernel's own FIELD_{GET,PREP}() is a bit
> awkward. I'm guessing that you're working around @_mask not being a
> compile-time constant?
Yes, they are used to work around @_mask not being a compile-time constant.
As we discussed offline, I'll port automatic system registers
definition generation in selftests and use those definition instead.
>
> > +struct reg_feature {
> > + uint64_t reg;
> > + uint64_t ftr_mask;
> > +};
> > +
> > +static void guest_code(void)
> > +{
> > + for (;;)
> > + GUEST_SYNC(0);
> > +}
>
> The test should check that the written values are visible both from the
> guest as well as userspace.
Sure. Will add checks in guest too.
>
> > +static struct reg_feature lower_safe_reg_ftrs[] = {
> > + { KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), ARM64_FEATURE_MASK(ID_AA64DFR0_WRPS) },
> > + { KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), ARM64_FEATURE_MASK(ID_AA64PFR0_EL3) },
> > + { KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR0_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR0_FGT) },
> > + { KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR1_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR1_PAN) },
> > + { KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR2_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR2_FWB) },
> > +};
>
> My preference would be to organize the field descriptors by register
> rather than the policy. This matches what the kernel does in cpufeature.c
> quite closely and allows us to easily reason about which fields are/aren't
> tested.
Sure. Will do.
>
> > +static void test_user_set_lower_safe(struct kvm_vcpu *vcpu)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(lower_safe_reg_ftrs); i++) {
> > + struct reg_feature *reg_ftr = lower_safe_reg_ftrs + i;
> > + uint64_t val, new_val, ftr;
> > +
> > + vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > + ftr = field_get(reg_ftr->ftr_mask, val);
> > +
> > + /* Set a safe value for the feature */
> > + if (ftr > 0)
> > + ftr--;
> > +
> > + val &= ~reg_ftr->ftr_mask;
> > + val |= field_prep(reg_ftr->ftr_mask, ftr);
> > +
> > + vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > + vcpu_get_reg(vcpu, reg_ftr->reg, &new_val);
> > + ASSERT_EQ(new_val, val);
> > + }
> > +}
> > +
> > +static void test_user_set_fail(struct kvm_vcpu *vcpu)
> > +{
> > + int i, r;
> > +
> > + for (i = 0; i < ARRAY_SIZE(lower_safe_reg_ftrs); i++) {
> > + struct reg_feature *reg_ftr = lower_safe_reg_ftrs + i;
> > + uint64_t val, old_val, ftr;
> > +
> > + vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > + ftr = field_get(reg_ftr->ftr_mask, val);
> > +
> > + /* Set a invalid value (too big) for the feature */
> > + if (ftr >= GENMASK_ULL(ARM64_FEATURE_FIELD_BITS - 1, 0))
> > + continue;
>
> This assumes that the fields in the register are unsigned, but there are
> several which are not.
This is based on the reg/fields which are defined previously. Those
are carefully chosen without unsigned ones.
>
> > + ftr++;
> > +
> > + old_val = val;
> > + val &= ~reg_ftr->ftr_mask;
> > + val |= field_prep(reg_ftr->ftr_mask, ftr);
> > +
> > + r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > + TEST_ASSERT(r < 0 && errno == EINVAL,
> > + "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
> > +
> > + vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > + ASSERT_EQ(val, old_val);
> > + }
> > +}
> > +
> > +static struct reg_feature exact_reg_ftrs[] = {
> > + /* Items will be added when there is appropriate field of type
> > + * FTR_EXACT enabled writing from userspace later.
> > + */
> > +};
> > +
> > +static void test_user_set_exact(struct kvm_vcpu *vcpu)
> > +{
> > + int i, r;
> > +
> > + for (i = 0; i < ARRAY_SIZE(exact_reg_ftrs); i++) {
> > + struct reg_feature *reg_ftr = exact_reg_ftrs + i;
> > + uint64_t val, old_val, ftr;
> > +
> > + vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > + ftr = field_get(reg_ftr->ftr_mask, val);
> > + old_val = val;
> > +
> > + /* Exact match */
> > + vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > + vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > + ASSERT_EQ(val, old_val);
> > +
> > + /* Smaller value */
> > + if (ftr > 0) {
> > + ftr--;
> > + val &= ~reg_ftr->ftr_mask;
> > + val |= field_prep(reg_ftr->ftr_mask, ftr);
> > + r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > + TEST_ASSERT(r < 0 && errno == EINVAL,
> > + "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
> > + vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > + ASSERT_EQ(val, old_val);
> > + ftr++;
> > + }
> > +
> > + /* Bigger value */
> > + ftr++;
> > + val &= ~reg_ftr->ftr_mask;
> > + val |= field_prep(reg_ftr->ftr_mask, ftr);
> > + r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > + TEST_ASSERT(r < 0 && errno == EINVAL,
> > + "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
> > + vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > + ASSERT_EQ(val, old_val);
> > + }
> > +}
>
> Don't add dead code, this can be added when we actually test FTR_EXACT
> fields. Are there not any in the registers exposed by this series?
Nope, there is no in the registers exposed by this series.
Anyway, I'll drop this code in this series.
>
> > +static uint32_t writable_regs[] = {
> > + SYS_ID_DFR0_EL1,
> > + SYS_ID_AA64DFR0_EL1,
> > + SYS_ID_AA64PFR0_EL1,
> > + SYS_ID_AA64MMFR0_EL1,
> > + SYS_ID_AA64MMFR1_EL1,
> > + SYS_ID_AA64MMFR2_EL1,
> > +};
> > +
> > +void test_user_get_writable_masks(struct kvm_vm *vm)
> > +{
> > + struct feature_id_writable_masks masks;
> > +
> > + vm_ioctl(vm, KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS, &masks);
> > +
> > + for (int i = 0; i < ARRAY_SIZE(writable_regs); i++) {
> > + uint32_t reg = writable_regs[i];
> > + int idx = ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(reg),
> > + sys_reg_Op1(reg), sys_reg_CRn(reg),
> > + sys_reg_CRm(reg), sys_reg_Op2(reg));
> > +
> > + ASSERT_EQ(masks.mask[idx], GENMASK_ULL(63, 0));
> > + }
> > +}
>
> The more robust test would be to check that every field this test knows
> is writable is actually advertised as such in the ioctl. So you could
> fetch this array at the start of the entire test and pass it through to
> the routines that do granular checks against the fields of every
> register.
Makes sense. Will do.
>
> It'd also be good to see basic sanity tests on the ioctl (i.e. call
> fails if ::rsvd is nonzero), since KVM has screwed that up on several
> occasions in past.
Sure. Will add some basic sanity tests on the ioctl.
>
> > +int main(void)
> > +{
> > + struct kvm_vcpu *vcpu;
> > + struct kvm_vm *vm;
> > +
> > + vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> > +
> > + ksft_print_header();
> > + ksft_set_plan(4);
> > +
> > + test_user_get_writable_masks(vm);
> > + ksft_test_result_pass("test_user_get_writable_masks\n");
> > +
> > + test_user_set_exact(vcpu);
> > + ksft_test_result_pass("test_user_set_exact\n");
> > +
> > + test_user_set_fail(vcpu);
> > + ksft_test_result_pass("test_user_set_fail\n");
> > +
> > + test_user_set_lower_safe(vcpu);
> > + ksft_test_result_pass("test_user_set_lower_safe\n");
> > +
> > + kvm_vm_free(vm);
> > +
> > + ksft_finished();
> > +}
> > --
> > 2.41.0.585.gd2178a4bd4-goog
> >
>
> --
> Thanks,
> Oliver
Thanks,
Jing
