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

[Andrew Jones]

On Tue, Oct 22, 2019 at 03:49:51PM +0200, Andrew Jones wrote:
> On Tue, Oct 22, 2019 at 11:29:05AM +0100, Peter Maydell wrote:
> > On Mon, 21 Oct 2019 at 17:18, Peter Maydell <address@hidden> wrote:
> > >
> > > On Mon, 21 Oct 2019 at 17:12, Andrew Jones <address@hidden> wrote:
> > > >
> > > > On Mon, Oct 21, 2019 at 04:43:22PM +0100, Peter Maydell wrote:
> > > > > On Mon, 21 Oct 2019 at 15:23, Andrew Jones <address@hidden> wrote:
> > > > > > Peter, would you mind running your test on the kvm32 machine with 
> > > > > > this
> > > > > > change before I send a v7?
> > > > >
> > > > > Still fails:
> > > > >
> > > > > pm215@pm-ct:~/qemu/build/arm$
> > > > > QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm tests/arm-cpu-features
> > > > > /arm/arm/query-cpu-model-expansion: OK
> > > > > /arm/arm/kvm/query-cpu-model-expansion: **
> > > > > ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:498:test_query_cpu_model_expansion_kvm:
> > > > > assertion failed: (resp_has_props(_resp))
> > > > > Aborted
> > > > >
> > > > > This is asserting on the line:
> > > > > 498             assert_has_not_feature(qts, "host", "sve");
> > > > >
> > > >
> > > > Oh, I see. It's not failing the specific absence of 'sve', but the test
> > > > code (assert_has_not_feature()) is assuming at least one property is
> > > > present. This isn't the case for kvm32 'host' cpus. They apparently
> > > > have none. We need this patch too, then
> > > >
> > > > diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> > > > index 14100ebd8521..9aa728ed8469 100644
> > > > --- a/tests/arm-cpu-features.c
> > > > +++ b/tests/arm-cpu-features.c
> > > > @@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const 
> > > > char *feature)
> > > >  ({                                                                     
> > > > \
> > > >      QDict *_resp = do_query_no_props(qts, cpu_type);                   
> > > > \
> > > >      g_assert(_resp);                                                   
> > > > \
> > > > -    g_assert(resp_has_props(_resp));                                   
> > > > \
> > > > -    g_assert(!qdict_get(resp_get_props(_resp), feature));              
> > > > \
> > > > +    g_assert(!resp_has_props(_resp) ||                                 
> > > > \
> > > > +             !qdict_get(resp_get_props(_resp), feature));              
> > > > \
> > > >      qobject_unref(_resp);                                              
> > > > \
> > > >  })
> > >
> > > Yep, with that extra the test passes. I'm just rerunning the
> > > full 'make check'...
> > 
> > ...which passed OK. For the record, the changes on top of the
> > patchset were:
> > 
> > diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> > index 92668efb8f5..9aa728ed846 100644
> > --- a/tests/arm-cpu-features.c
> > +++ b/tests/arm-cpu-features.c
> > @@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const
> > char *feature)
> >  ({                                                                     \
> >      QDict *_resp = do_query_no_props(qts, cpu_type);                   \
> >      g_assert(_resp);                                                   \
> > -    g_assert(resp_has_props(_resp));                                   \
> > -    g_assert(!qdict_get(resp_get_props(_resp), feature));              \
> > +    g_assert(!resp_has_props(_resp) ||                                 \
> > +             !qdict_get(resp_get_props(_resp), feature));              \
> >      qobject_unref(_resp);                                              \
> >  })
> > 
> > @@ -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);
> >
> 
> Thanks Peter!
> 
> The changes look good to me. If we wanted to, we could also add
> 
>  assert_has_not_feature(qts, "host", "pmu");
> 
> in that kvm32 block where we have the
> 
>  assert_has_not_feature(qts, "host", "sve");
> 
> and we could even add
> 
>  assert_has_not_feature(qts, "host", "aarch64");
> 
> there as well.
> 
> If I need to spin a v7, then I'll do that. I could also submit just those
> additions as another patch, though, or even just not worry too much about
> those cases and not add the tests...
>

I've decided to send a v7 because the changes above generate several
collisions, I want to add those two has-not tests, and because I have
more tags to pick up. I'll be sending it shortly.

Thanks,
drew