Re: [PATCH v7 10/10] KVM: arm64: selftests: Test for setting ID register from usersapce

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

 



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




[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