On Tue, May 25, 2021 at 01:09:22PM -0700, Ricardo Koller wrote:
> On Wed, May 19, 2021 at 04:07:26PM +0200, Andrew Jones wrote:
> > Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
> > userspace when not available") the get-reg-list* tests have been
> > failing with
> >
> > ...
> > ... There are 74 missing registers.
> > The following lines are missing registers:
> > ...
> >
> > where the 74 missing registers are all PMU registers. This isn't a
> > bug in KVM that the selftest found, even though it's true that a
> > KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
> > flag, but still expecting the PMU registers to be in the reg-list,
> > would suddenly no longer have their expectations met. In that case,
> > the expectations were wrong, though, so that KVM userspace needs to
> > be fixed, and so does this selftest. The fix for this selftest is to
> > pull the PMU registers out of the base register sublist into their
> > own sublist and then create new, pmu-enabled vcpu configs which can
> > be tested.
> >
> > Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> > ---
> > .../selftests/kvm/aarch64/get-reg-list.c | 46 +++++++++++++++----
> > 1 file changed, 38 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > index dc06a28bfb74..78d8949bddbd 100644
> > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > @@ -47,6 +47,7 @@ struct reg_sublist {
> > struct vcpu_config {
> > const char *name;
> > bool sve;
> > + bool pmu;
> > struct reg_sublist sublists[];
> > };
>
> I think it's possible that the number of sublists keeps increasing: it
> would be very nice/useful if KVM allowed enabling/disabling more
> features from userspace (besides SVE, PMU etc). In that case, it might
> be easier if adding a new feature to get-reg-list just requires defining
> a new config and not dealing with the internals of vcpu_config.
Yes, adding the bools is a bit ugly, but how will we easily check if a
given feature is present in a given config? We could put a copy of the
vcpu_init features bitmap in vcpu_config, but I'm not sure if not touching
the vcpu_config structure is worth having to use test_bit() and friends
everywhere.
>
> Do you think it's possible in general to associate a sublist to a
> capability and a feature? It works for the PMU and SVE. If that is
> possible, what do you think of something like this? this would be the
> config for sve+pmu:
>
> static struct vcpu_config sve_pmu_config = {
> "sve+pmu",
> .sublists = {
> { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
> { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
> {0},
> },
> };
>
> Appended a rough patch at the end to make this idea more concrete.
Comments below
> diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> index 78d8949bddbd..33b8735bdb15 100644
> --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> @@ -38,6 +38,11 @@ static struct kvm_reg_list *reg_list;
> static __u64 *blessed_reg, blessed_n;
>
> struct reg_sublist {
> + const char *name;
> + bool base;
> + int feature;
> + int capability;
> + bool finalize;
> __u64 *regs;
> __u64 regs_n;
> __u64 *rejects_set;
> @@ -46,8 +51,6 @@ struct reg_sublist {
>
> struct vcpu_config {
> const char *name;
> - bool sve;
> - bool pmu;
> struct reg_sublist sublists[];
> };
>
> @@ -257,10 +260,7 @@ static void print_reg(struct vcpu_config *c, __u64 id)
> printf("\tKVM_REG_ARM_FW_REG(%lld),\n", id & 0xffff);
> break;
> case KVM_REG_ARM64_SVE:
> - if (c->sve)
> - printf("\t%s,\n", sve_id_to_str(c, id));
> - else
> - TEST_FAIL("%s: KVM_REG_ARM64_SVE is an unexpected coproc type in reg id: 0x%llx", c->name, id);
> + printf("\t%s,\n", sve_id_to_str(c, id));
I'd rather not lose this test. What we were doing here is making sure we
don't see registers with KVM_REG_ARM64_SVE when sve is not enabled.
> break;
> default:
> TEST_FAIL("%s: Unexpected coproc type: 0x%llx in reg id: 0x%llx",
> @@ -327,31 +327,42 @@ static void core_reg_fixup(void)
>
> static void prepare_vcpu_init(struct vcpu_config *c, struct kvm_vcpu_init *init)
> {
> - if (c->sve)
> - init->features[0] |= 1 << KVM_ARM_VCPU_SVE;
> - if (c->pmu)
> - init->features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> + struct reg_sublist *s;
> +
> + for_each_sublist(c, s) {
> + if (s->base)
> + continue;
> + init->features[0] |= 1 << s->feature;
> + }
If we want this to be general then we should ensure s->feature is < 32,
otherwise we need to move to the next word. Granted we only have a few
features so far for all the years we've had Arm KVM, so we probably don't
need to worry about this any time soon...
> }
>
> static void finalize_vcpu(struct kvm_vm *vm, uint32_t vcpuid, struct vcpu_config *c)
> {
> + struct reg_sublist *s;
> int feature;
>
> - if (c->sve) {
> - feature = KVM_ARM_VCPU_SVE;
> - vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_FINALIZE, &feature);
> + for_each_sublist(c, s) {
> + if (s->base)
> + continue;
Probably don't need the if (s->base) continue, since base registers won't
have s->finalize.
> + if (s->finalize) {
> + feature = s->feature;
> + vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_FINALIZE, &feature);
> + }
> }
> }
>
> static void check_supported(struct vcpu_config *c)
> {
> - if (c->sve && !kvm_check_cap(KVM_CAP_ARM_SVE)) {
> - fprintf(stderr, "%s: SVE not available, skipping tests\n", c->name);
> - exit(KSFT_SKIP);
> - }
> - if (c->pmu && !kvm_check_cap(KVM_CAP_ARM_PMU_V3)) {
> - fprintf(stderr, "%s: PMU not available, skipping tests\n", c->name);
> - exit(KSFT_SKIP);
> + struct reg_sublist *s;
> +
> + for_each_sublist(c, s) {
> + if (s->base)
> + continue;
Also don't need the if (s->base) continue, since base registers won't have
capabilities.
> + if (!kvm_check_cap(s->capability)) {
> + fprintf(stderr, "%s: %s not available, skipping tests\n", c->name, s->name);
> + exit(KSFT_SKIP);
> +
> + }
> }
> }
>
> @@ -975,34 +986,34 @@ static __u64 sve_rejects_set[] = {
> static struct vcpu_config vregs_config = {
> "vregs",
> .sublists = {
> - { base_regs, ARRAY_SIZE(base_regs), },
> - { vregs, ARRAY_SIZE(vregs), },
> + { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> + { "vregs", true, 0, 0, false, vregs, ARRAY_SIZE(vregs), },
> {0},
> },
> };
> static struct vcpu_config vregs_pmu_config = {
> - "vregs+pmu", .pmu = true,
> + "vregs+pmu",
> .sublists = {
> - { base_regs, ARRAY_SIZE(base_regs), },
> - { vregs, ARRAY_SIZE(vregs), },
> - { pmu_regs, ARRAY_SIZE(pmu_regs), },
> + { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> + { "vregs", true, 0, 0, false, vregs, ARRAY_SIZE(vregs), },
> + { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
> {0},
> },
> };
> static struct vcpu_config sve_config = {
> - "sve", .sve = true,
> + "sve",
> .sublists = {
> - { base_regs, ARRAY_SIZE(base_regs), },
> - { sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
> + { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> + { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
> {0},
> },
> };
> static struct vcpu_config sve_pmu_config = {
> - "sve+pmu", .sve = true, .pmu = true,
> + "sve+pmu",
> .sublists = {
> - { base_regs, ARRAY_SIZE(base_regs), },
> - { sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
> - { pmu_regs, ARRAY_SIZE(pmu_regs), },
> + { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> + { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
> + { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
> {0},
> },
> };
>
It looks pretty good to me. While I don't really care about needing to add
booleans to vcpu_config, the biggest advantage I see is not needing to
modify prepare_vcpu_init, finalize_vcpu, and check_supported, and that the
feature bits and caps are better associated with the sublists.
These tables are getting wordy, though, so we'll probably want some
macros.
I'll experiment with this to see if I can integrate some of your
suggestions into a v3.
Thanks,
drew
