Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers

[Ricardo Koller <ricarkol@google.com>]

On Wed, May 26, 2021 at 08:57:24AM +0200, Andrew Jones wrote:
> 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@redhat.com>
> > > ---
> > >  .../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.
> 

I agree, the bools are better than the bits.

My solution was to completely ignore the SVE test in print_reg (not the
best solution).

> > 
> > 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
>

Ack on all the comments.

> > 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 for considering the changes.

> 
> Thanks,
> drew
>

Thanks,
Ricardo