From: Marc Zyngier <maz@kernel.org> To: Andrew Jones <drjones@redhat.com> Cc: Ricardo Koller <ricarkol@google.com>, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, eric.auger@redhat.com, alexandru.elisei@arm.com, pbonzini@redhat.com Subject: Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers Date: Wed, 26 May 2021 11:15:01 +0100 [thread overview] Message-ID: <87y2c1vm1m.wl-maz@kernel.org> (raw) In-Reply-To: <20210526093211.loppoo42twel6inw@gator.home> On Wed, 26 May 2021 10:32:11 +0100, Andrew Jones <drjones@redhat.com> wrote: > > On Wed, May 26, 2021 at 09:44:56AM +0100, Marc Zyngier wrote: > > 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. > > It's my hope that this test, especially now after its refactoring, will > allow us to test all configurations easily and therefore frequently. > > > > > 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. > > > > I'm not sure I understand how we should change the development and > review processes. I'm worried that the current ratio of development vs review vs testing is simply not right. We have a huge reviewing deficit, and we end-up merging buggy code. Some of the features we simply cannot test. It was OK up to 3 years ago, but I'm not sure it is sustainable anymore. So making more and more things optional seems to go further in the direction of an uncontrolled bitrot. > As for testing, with simple tests like this one, > we can actually achieve exhaustive configuration testing fast, at > least with respect to checking for expected registers and checking > that we can get/set_one_reg on them. If we were to try and setup > QEMU migration tests for all the possible configurations, then it > would take way too long. I'm not worried about this get/set thing. I'm worried about the full end-to-end migration, which hardly anyone tests in anger, with all the variability of the architecture and options. M. -- Without deviation from the norm, progress is not possible.
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Andrew Jones <drjones@redhat.com> Cc: kvm@vger.kernel.org, pbonzini@redhat.com, kvmarm@lists.cs.columbia.edu Subject: Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers Date: Wed, 26 May 2021 11:15:01 +0100 [thread overview] Message-ID: <87y2c1vm1m.wl-maz@kernel.org> (raw) In-Reply-To: <20210526093211.loppoo42twel6inw@gator.home> On Wed, 26 May 2021 10:32:11 +0100, Andrew Jones <drjones@redhat.com> wrote: > > On Wed, May 26, 2021 at 09:44:56AM +0100, Marc Zyngier wrote: > > 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. > > It's my hope that this test, especially now after its refactoring, will > allow us to test all configurations easily and therefore frequently. > > > > > 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. > > > > I'm not sure I understand how we should change the development and > review processes. I'm worried that the current ratio of development vs review vs testing is simply not right. We have a huge reviewing deficit, and we end-up merging buggy code. Some of the features we simply cannot test. It was OK up to 3 years ago, but I'm not sure it is sustainable anymore. So making more and more things optional seems to go further in the direction of an uncontrolled bitrot. > As for testing, with simple tests like this one, > we can actually achieve exhaustive configuration testing fast, at > least with respect to checking for expected registers and checking > that we can get/set_one_reg on them. If we were to try and setup > QEMU migration tests for all the possible configurations, then it > would take way too long. I'm not worried about this get/set thing. I'm worried about the full end-to-end migration, which hardly anyone tests in anger, with all the variability of the architecture and options. M. -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply other threads:[~2021-05-26 10:15 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-19 14:07 [PATCH v2 0/5] KVM: arm64: selftests: Fix get-reg-list Andrew Jones 2021-05-19 14:07 ` Andrew Jones 2021-05-19 14:07 ` [PATCH v2 1/5] KVM: arm64: selftests: get-reg-list: Introduce vcpu configs Andrew Jones 2021-05-19 14:07 ` Andrew Jones 2021-05-19 14:07 ` [PATCH v2 2/5] KVM: arm64: selftests: get-reg-list: Prepare to run multiple configs at once Andrew Jones 2021-05-19 14:07 ` Andrew Jones 2021-05-19 14:07 ` [PATCH v2 3/5] KVM: arm64: selftests: get-reg-list: Provide config selection option Andrew Jones 2021-05-19 14:07 ` Andrew Jones 2021-05-19 14:07 ` [PATCH v2 4/5] KVM: arm64: selftests: get-reg-list: Remove get-reg-list-sve Andrew Jones 2021-05-19 14:07 ` Andrew Jones 2021-05-19 14:07 ` [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers Andrew Jones 2021-05-19 14:07 ` Andrew Jones 2021-05-25 20:09 ` Ricardo Koller 2021-05-25 20:09 ` Ricardo Koller 2021-05-26 6:57 ` Andrew Jones 2021-05-26 6:57 ` Andrew Jones 2021-05-26 17:37 ` Ricardo Koller 2021-05-26 17:37 ` Ricardo Koller 2021-05-26 8:44 ` Marc Zyngier 2021-05-26 8:44 ` Marc Zyngier 2021-05-26 9:32 ` Andrew Jones 2021-05-26 9:32 ` Andrew Jones 2021-05-26 10:15 ` Marc Zyngier [this message] 2021-05-26 10:15 ` Marc Zyngier 2021-05-26 11:53 ` Andrew Jones 2021-05-26 11:53 ` Andrew Jones
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87y2c1vm1m.wl-maz@kernel.org \ --to=maz@kernel.org \ --cc=alexandru.elisei@arm.com \ --cc=drjones@redhat.com \ --cc=eric.auger@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=pbonzini@redhat.com \ --cc=ricarkol@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.