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

[Marc Zyngier <maz@kernel.org>]

On Tue, 25 May 2021 21:09:22 +0100,
Ricardo Koller <ricarkol@google.com> 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).

[tangential semi-rant]

While this is a very noble goal, it also doubles the validation space
each time you add an option. Given how little testing gets done
relative to the diversity of features and implementations, that's a
*big* problem.

I'm not against it for big ticket items that result in a substantial
amount of state to be context-switched (SVE, NV). However, doing that
for more discrete features would require a radical change in the way
we develop, review and test KVM/arm64.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.