Re: [PATCH 1/9] tests/qtest/arm-cpu-features: Remove Cortex-A15 check

[Andrew Jones]

On Fri, Feb 05, 2021 at 05:03:08PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/5/21 4:33 PM, Andrew Jones wrote:
> > On Fri, Feb 05, 2021 at 04:15:45PM +0100, Philippe Mathieu-Daudé wrote:
> >> Hi Drew,
> >>
> >> On 2/5/21 3:59 PM, Andrew Jones wrote:
> >>> On Fri, Feb 05, 2021 at 03:43:37PM +0100, Philippe Mathieu-Daudé wrote:
> >>>> Support for ARMv7 has been dropped in commit 82bf7ae84ce
> >>>> ("target/arm: Remove KVM support for 32-bit Arm hosts"),
> >>>> no need to check for Cortex A15 host cpu anymore.
> >>>>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>>> ---
> >>>>  tests/qtest/arm-cpu-features.c | 4 ----
> >>>>  1 file changed, 4 deletions(-)
> >>>>
> >>>> diff --git a/tests/qtest/arm-cpu-features.c 
> >>>> b/tests/qtest/arm-cpu-features.c
> >>>> index 8252b85bb85..c59c3cb002b 100644
> >>>> --- a/tests/qtest/arm-cpu-features.c
> >>>> +++ b/tests/qtest/arm-cpu-features.c
> >>>> @@ -515,10 +515,6 @@ static void 
> >>>> test_query_cpu_model_expansion_kvm(const void *data)
> >>>>          QDict *resp;
> >>>>          char *error;
> >>>>  
> >>>> -        assert_error(qts, "cortex-a15",
> >>>> -            "We cannot guarantee the CPU type 'cortex-a15' works "
> >>>> -            "with KVM on this host", NULL);
> >>>> -
> >>>
> >>> This isn't testing anything regarding 32-bit KVM host support. It's
> >>> testing that an error is returned when a given cpu type that can't
> >>> be known to work with KVM is used. We know that the cortex-a15 can't
> >>> be known to work. If we were to use a 64-bit cpu type here then there's
> >>> a chance that it would work, failing the test that an error be returned.
> >>
> >> This was my first understanding, but then why does it fail?
> >>
> >> PASS 1 qtest-aarch64/arm-cpu-features 
> >> /aarch64/arm/query-cpu-model-expansion
> >> **
> >> ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
> >> assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
> >> 'cortex-a15' works " "with KVM on this host"))
> >> ERROR qtest-aarch64/arm-cpu-features - Bail out!
> >> ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
> >> assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
> >> 'cortex-a15' works " "with KVM on this host"))
> >> make: *** [Makefile.mtest:905: run-test-111] Error 1
> >>
> >> FWIW when tracing (cavium thunderX1 host, dmesg reports 0x431f0a11):
> >> kvm_vcpu_ioctl cpu_index 0, type 0x4020aeae, arg 0xffff9b7f9b18
> > 
> > Hmm... I don't know. It works for me
> > 
> > $ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/arm-cpu-features
> > /aarch64/arm/query-cpu-model-expansion: OK
> > /aarch64/arm/kvm/query-cpu-model-expansion: OK
> > /aarch64/arm/kvm/query-cpu-model-expansion/sve-off: OK
> > /aarch64/arm/max/query-cpu-model-expansion/sve-max-vq-8: OK
> > /aarch64/arm/max/query-cpu-model-expansion/sve-off: OK
> 
> Thanks, that helped.
> 
> I ran my tests including the "Restrict v7A TCG cpus to TCG accel"
> patch which removes the A15 in KVM-only build:
> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg08051.html

Oh, I see. So this patch is OK if we merge "Restrict v7A TCG cpus to
TCG accel", but we should change the commit message of this patch
to point to the real reason for it, which is "Restrict v7A TCG cpus to
TCG accel". Also, I'd prefer we don't remove this test, but rather find
another way to perform it without the cortex-a15 cpu type.

>  So when TCG is disabled,
> 
> So I get:
> 
> { "execute": "query-cpu-model-expansion", 'arguments': { 'type': 'full',
> 'model': { 'name': 'cortex-a15' }}}
> {
>     "error": {
>         "class": "GenericError",
>         "desc": "The CPU type 'cortex-a15' is not a recognized ARM CPU type"
>     }
> }
> 
> which fails the g_str_equal().
> 
> BTW is there some easy way to dump QMP traffic on stdio?

You can use scripts/qmp/qmp-shell to manually test stuff.

Thanks,
drew

> 
> > 
> > $ lscpu
> > Architecture:        aarch64
> > Byte Order:          Little Endian
> > CPU(s):              48
> > On-line CPU(s) list: 0-47
> > Thread(s) per core:  1
> > Core(s) per cluster: 16
> > Socket(s):           -
> > Cluster(s):          3
> > NUMA node(s):        1
> > Vendor ID:           Cavium
> > Model:               1
> > Model name:          ThunderX 88XX
> > Stepping:            0x1
> > BogoMIPS:            200.00
> > NUMA node0 CPU(s):   0-47
> > Flags:               fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
> > 
> >>
> >>>
> >>>>          assert_has_feature_enabled(qts, "host", "aarch64");
> >>>>  
> >>>>          /* Enabling and disabling pmu should always work. */
> >>>> -- 
> >>>> 2.26.2
> >>>>
> >>>>
> >>>
> >>> This file could use a cleanup patch regarding the dropping of 32-bit KVM
> >>> support though. At least the comment in main(), "For now we only run KVM
> >>> specific tests..." could be reworded. It was written that way when we
> >>> planned to try testing on 32-bit KVM too eventually, but we never did,
> >>> and now we'll never need to.
> >>>
> >>> Thanks,
> >>> drew
> >>>
> >>>
> >>
> > 
> > 
>