Re: [PATCH v6 0/9] target/arm/kvm: enable SVE in guests

[Andrew Jones]

On Mon, Oct 21, 2019 at 02:54:30PM +0200, Philippe Mathieu-Daudé wrote:
> On 10/21/19 2:36 PM, Peter Maydell wrote:
> > On Wed, 16 Oct 2019 at 09:54, Andrew Jones <address@hidden> wrote:
> > > 
> > > Since Linux kernel v5.2-rc1 KVM has support for enabling SVE in guests.
> > > This series provides the QEMU bits for that enablement. First, we
> > > select existing CPU properties representing features we want to
> > > advertise in addition to the SVE vector lengths and prepare
> > > them for a qmp query. Then we introduce the qmp query, applying
> > > it immediately to those selected features. We also document ARM CPU
> > > features at this time. We next add a qtest for the selected CPU
> > > features that uses the qmp query for its tests - and we continue to
> > > add tests as we add CPU features with the following patches. So then,
> > > once we have the support we need for CPU feature querying and testing,
> > > we add our first SVE CPU feature property, 'sve', which just allows
> > > SVE to be completely enabled/disabled. Following that feature property,
> > > we add all 16 vector length properties along with the input validation
> > > they need and tests to prove the validation works. At this point the
> > > SVE features are still only for TCG, so we provide some patches to
> > > prepare for KVM and then a patch that allows the 'max' CPU type to
> > > enable SVE with KVM, but at first without vector length properties.
> > > After a bit more preparation we add the SVE vector length properties
> > > to the KVM-enabled 'max' CPU type along with the additional input
> > > validation and tests that that needs.  Finally we allow the 'host'
> > > CPU type to also enjoy these properties by simply sharing them with it.
> > 
> > This fails 'make check' on an aarch32 box with KVM support:
> > 
> > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> > QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm QTEST_QEMU_IMG=qemu-img
> > tests/arm-cpu-features -m=quick -k --tap < /dev/null |
> > ./scripts/tap-driver.pl --test-name="arm-cpu-features"
> > PASS 1 arm-cpu-features /arm/arm/query-cpu-model-expansion
> > **
> > ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:420:test_query_cpu_model_expansion_kvm:
> > assertion failed: (resp_has_props(_resp))
> 
> Glad you could test it, I was wondering how this test work because it first
> unconditionally assert the host has PMU feature (failing the test) then
> there is a unreachable if(!aarch64) "'pmu' feature not supported" warning.
>

Yes, this is the case you were concerned about, but the problem is for a
slightly different reason. *If* the PMU feature was getting added to the
kvm32 'host' CPU type, then both tests here would be correct: the feature
would be present, but arm_set_pmu() would report "feature not supported",
as that's what KVM would report on kvm32. However, I see now that
target/arm/kvm32.c:kvm_arm_get_host_cpu_features() does *not* add
ARM_FEATURE_PMU, like it does in the target/arm/kvm64.c implementation
and in the 32-bit CPU models like the Cortex-A15. Indeed, had this
test been run with '-cpu cortex-a15', and the kvm32 host allowed that
CPU type, then this test would also work. We want to use 'host' here
though, as that's more appropriate, so the fix is simply to remove the
tests on kvm32 like below.

Peter, would you mind running your test on the kvm32 machine with this
change before I send a v7? Actually, do I need to send a v7? Or is
type of fixup OK to do on merge?

Thanks,
drew


diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
index 92668efb8f56..14100ebd8521 100644
--- a/tests/arm-cpu-features.c
+++ b/tests/arm-cpu-features.c
@@ -417,8 +417,6 @@ static void test_query_cpu_model_expansion_kvm(const void 
*data)
 
     qts = qtest_init(MACHINE "-accel kvm -cpu host");
 
-    assert_has_feature(qts, "host", "pmu");
-
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         bool kvm_supports_sve;
         char max_name[8], name[8];
@@ -428,6 +426,7 @@ static void test_query_cpu_model_expansion_kvm(const void 
*data)
         char *error;
 
         assert_has_feature(qts, "host", "aarch64");
+        assert_has_feature(qts, "host", "pmu");
 
         assert_error(qts, "cortex-a15",
             "We cannot guarantee the CPU type 'cortex-a15' works "
@@ -497,9 +496,6 @@ static void test_query_cpu_model_expansion_kvm(const void 
*data)
         }
     } else {
         assert_has_not_feature(qts, "host", "sve");
-        assert_error(qts, "host",
-                     "'pmu' feature not supported by KVM on this host",
-                     "{ 'pmu': true }");
     }
 
     qtest_quit(qts);