diff mbox series

[v16,96/99] tests/qtest: split the cdrom-test into arm/aarch64

Message ID 20210604155312.15902-97-alex.bennee@linaro.org
State New
Headers show
Series arm tcg/kvm refactor and split with kvm only support | expand

Commit Message

Alex Bennée June 4, 2021, 3:53 p.m. UTC
The assumption that the qemu-system-aarch64 image can run all 32 bit
machines is about to be broken and besides it's not likely this is
improving out coverage by much. Test the "virt" machine for both arm
and aarch64 as it can be used by either architecture.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 tests/qtest/cdrom-test.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.20.1

Comments

Richard Henderson June 5, 2021, 10:47 p.m. UTC | #1
On 6/4/21 8:53 AM, Alex Bennée wrote:
> The assumption that the qemu-system-aarch64 image can run all 32 bit

> machines is about to be broken...


Um, what?


r~



  and besides it's not likely this is
> improving out coverage by much. Test the "virt" machine for both arm

> and aarch64 as it can be used by either architecture.

> 

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>   tests/qtest/cdrom-test.c | 5 ++++-

>   1 file changed, 4 insertions(+), 1 deletion(-)

> 

> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c

> index 5af944a5fb..1e74354624 100644

> --- a/tests/qtest/cdrom-test.c

> +++ b/tests/qtest/cdrom-test.c

> @@ -220,13 +220,16 @@ int main(int argc, char **argv)

>               "magnum", "malta", "pica61", NULL

>           };

>           add_cdrom_param_tests(mips64machines);

> -    } else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {

> +    } else if (g_str_equal(arch, "arm")) {

>           const char *armmachines[] = {

>               "realview-eb", "realview-eb-mpcore", "realview-pb-a8",

>               "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",

>               "vexpress-a9", "virt", NULL

>           };

>           add_cdrom_param_tests(armmachines);

> +    } else if (g_str_equal(arch, "aarch64")) {

> +        const char *aarch64machines[] = { "virt", NULL };

> +        add_cdrom_param_tests(aarch64machines);

>       } else {

>           const char *nonemachine[] = { "none", NULL };

>           add_cdrom_param_tests(nonemachine);

>
John Snow June 8, 2021, 1:42 p.m. UTC | #2
On 6/4/21 11:53 AM, Alex Bennée wrote:
> The assumption that the qemu-system-aarch64 image can run all 32 bit

> machines is about to be broken and besides it's not likely this is


What's changing? I'm not deeply familiar with aarch64. Why is this 
assumption about to be broken?

> improving out coverage by much. Test the "virt" machine for both arm

> and aarch64 as it can be used by either architecture.

> 


Sounds fine to me, but I didn't write this part of the test. Thomas, you 
wrote this section IIRC -- does this reduce coverage in any meaningful way?

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>   tests/qtest/cdrom-test.c | 5 ++++-

>   1 file changed, 4 insertions(+), 1 deletion(-)

> 

> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c

> index 5af944a5fb..1e74354624 100644

> --- a/tests/qtest/cdrom-test.c

> +++ b/tests/qtest/cdrom-test.c

> @@ -220,13 +220,16 @@ int main(int argc, char **argv)

>               "magnum", "malta", "pica61", NULL

>           };

>           add_cdrom_param_tests(mips64machines);

> -    } else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {

> +    } else if (g_str_equal(arch, "arm")) {

>           const char *armmachines[] = {

>               "realview-eb", "realview-eb-mpcore", "realview-pb-a8",

>               "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",

>               "vexpress-a9", "virt", NULL

>           };

>           add_cdrom_param_tests(armmachines);

> +    } else if (g_str_equal(arch, "aarch64")) {

> +        const char *aarch64machines[] = { "virt", NULL };

> +        add_cdrom_param_tests(aarch64machines);

>       } else {

>           const char *nonemachine[] = { "none", NULL };

>           add_cdrom_param_tests(nonemachine);

>
Alex Bennée June 8, 2021, 2:27 p.m. UTC | #3
Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/4/21 8:53 AM, Alex Bennée wrote:

>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>> machines is about to be broken...

>

> Um, what?


Really what we want is to probe the -M (machines) that a binary
supports rather than just barfing the test because we've built a QEMU
that doesn't support all the random 32 bit machines.

> r~

>

>

>

>  and besides it's not likely this is

>> improving out coverage by much. Test the "virt" machine for both arm

>> and aarch64 as it can be used by either architecture.


I think this point still stands though, I don't think we get much from
running the cdrom test with realview et all on qemu-system-aarch64.


>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> ---

>>   tests/qtest/cdrom-test.c | 5 ++++-

>>   1 file changed, 4 insertions(+), 1 deletion(-)

>> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c

>> index 5af944a5fb..1e74354624 100644

>> --- a/tests/qtest/cdrom-test.c

>> +++ b/tests/qtest/cdrom-test.c

>> @@ -220,13 +220,16 @@ int main(int argc, char **argv)

>>               "magnum", "malta", "pica61", NULL

>>           };

>>           add_cdrom_param_tests(mips64machines);

>> -    } else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {

>> +    } else if (g_str_equal(arch, "arm")) {

>>           const char *armmachines[] = {

>>               "realview-eb", "realview-eb-mpcore", "realview-pb-a8",

>>               "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",

>>               "vexpress-a9", "virt", NULL

>>           };

>>           add_cdrom_param_tests(armmachines);

>> +    } else if (g_str_equal(arch, "aarch64")) {

>> +        const char *aarch64machines[] = { "virt", NULL };

>> +        add_cdrom_param_tests(aarch64machines);

>>       } else {

>>           const char *nonemachine[] = { "none", NULL };

>>           add_cdrom_param_tests(nonemachine);

>> 



-- 
Alex Bennée
Thomas Huth June 8, 2021, 2:36 p.m. UTC | #4
On 08/06/2021 15.42, John Snow wrote:
> On 6/4/21 11:53 AM, Alex Bennée wrote:

>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>> machines is about to be broken and besides it's not likely this is

> 

> What's changing? I'm not deeply familiar with aarch64. Why is this 

> assumption about to be broken?


That's also quite a surprise to me. Do you have any pointers?

>> improving out coverage by much. Test the "virt" machine for both arm

>> and aarch64 as it can be used by either architecture.

>>

> 

> Sounds fine to me, but I didn't write this part of the test. Thomas, you 

> wrote this section IIRC -- does this reduce coverage in any meaningful way?


I think we built only aarch64 in a couple of our CI pipelines, assuming that 
it covers all normal arm machines, too ... so we might want to revisit our 
CI pipelines to check whether we then need more test coverage for 
qemu-system-arm instead. But apart from that, the patch looks ok to me.

  Thomas
Alex Bennée June 8, 2021, 2:41 p.m. UTC | #5
Thomas Huth <thuth@redhat.com> writes:

> On 08/06/2021 15.42, John Snow wrote:

>> On 6/4/21 11:53 AM, Alex Bennée wrote:

>>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>>> machines is about to be broken and besides it's not likely this is

>> What's changing? I'm not deeply familiar with aarch64. Why is this

>> assumption about to be broken?

>

> That's also quite a surprise to me. Do you have any pointers?


It's at the top of the series. If you build qemu-system-aarch64 with a
custom config you won't be able to instantiate those machines. Ideally
it would probe and maybe fail safe if the binary doesn't support the
model. Is that possible in qtest?

>

>>> improving out coverage by much. Test the "virt" machine for both arm

>>> and aarch64 as it can be used by either architecture.

>>>

>> Sounds fine to me, but I didn't write this part of the test. Thomas,

>> you wrote this section IIRC -- does this reduce coverage in any

>> meaningful way?

>

> I think we built only aarch64 in a couple of our CI pipelines,

> assuming that it covers all normal arm machines, too ... so we might

> want to revisit our CI pipelines to check whether we then need more

> test coverage for qemu-system-arm instead. But apart from that, the

> patch looks ok to me.

>

>  Thomas



-- 
Alex Bennée
Thomas Huth June 8, 2021, 2:42 p.m. UTC | #6
On 08/06/2021 16.27, Alex Bennée wrote:
> 

> Richard Henderson <richard.henderson@linaro.org> writes:

> 

>> On 6/4/21 8:53 AM, Alex Bennée wrote:

>>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>>> machines is about to be broken...

>>

>> Um, what?

> 

> Really what we want is to probe the -M (machines) that a binary

> supports rather than just barfing the test because we've built a QEMU

> that doesn't support all the random 32 bit machines.


Ok, so this is rather about being able to use the qtests with custom QEMU 
builds where you tweaked the default-configs? That's certainly a valid 
endeavor, but then I think this patch is the wrong way to go. You should 
rather add a qtest_has_machine() function that checks whether a machine is 
available in the target binary or not, and then only add the tests to the 
test plan that match the list of available machines. (we have similar issues 
in downstream RHEL where we want to limit the amount of machines, so a 
qtest_has_machine() function would certainly be welcome)

  Thomas
Thomas Huth June 8, 2021, 2:45 p.m. UTC | #7
On 08/06/2021 16.41, Alex Bennée wrote:
> 

> Thomas Huth <thuth@redhat.com> writes:

> 

>> On 08/06/2021 15.42, John Snow wrote:

>>> On 6/4/21 11:53 AM, Alex Bennée wrote:

>>>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>>>> machines is about to be broken and besides it's not likely this is

>>> What's changing? I'm not deeply familiar with aarch64. Why is this

>>> assumption about to be broken?

>>

>> That's also quite a surprise to me. Do you have any pointers?

> 

> It's at the top of the series. If you build qemu-system-aarch64 with a

> custom config you won't be able to instantiate those machines. Ideally

> it would probe and maybe fail safe if the binary doesn't support the

> model. Is that possible in qtest?


Not yet. You'd need to do something similar like Philippe did in patch 
03/99, but instead of running query-accel, you have to run query-machines 
instead.

  Thomas
Richard Henderson June 8, 2021, 2:57 p.m. UTC | #8
On 6/8/21 7:27 AM, Alex Bennée wrote:
> 

> Richard Henderson <richard.henderson@linaro.org> writes:

> 

>> On 6/4/21 8:53 AM, Alex Bennée wrote:

>>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>>> machines is about to be broken...

>>

>> Um, what?

> 

> Really what we want is to probe the -M (machines) that a binary

> supports rather than just barfing the test because we've built a QEMU

> that doesn't support all the random 32 bit machines.

> 

>> r~

>>

>>

>>

>>   and besides it's not likely this is

>>> improving out coverage by much. Test the "virt" machine for both arm

>>> and aarch64 as it can be used by either architecture.

> 

> I think this point still stands though, I don't think we get much from

> running the cdrom test with realview et all on qemu-system-aarch64.


That is commentary I can get behind.  The former has connotations of doom and 
gloom and the opposite of where I think we should go.


r~
Thomas Huth June 8, 2021, 3:01 p.m. UTC | #9
On 08/06/2021 16.27, Alex Bennée wrote:
> 

> Richard Henderson <richard.henderson@linaro.org> writes:

> 

>> On 6/4/21 8:53 AM, Alex Bennée wrote:

>>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>>> machines is about to be broken...

>>

>> Um, what?

> 

> Really what we want is to probe the -M (machines) that a binary

> supports rather than just barfing the test because we've built a QEMU

> that doesn't support all the random 32 bit machines.

> 

>> r~

>>

>>

>>

>>   and besides it's not likely this is

>>> improving out coverage by much. Test the "virt" machine for both arm

>>> and aarch64 as it can be used by either architecture.

> 

> I think this point still stands though, I don't think we get much from

> running the cdrom test with realview et all on qemu-system-aarch64.


In a lot of CI pipelines, we are either building aarch64 or arm, but not 
both, so I think it might be good to keep the tests in here.

  Thomas
Alex Bennée June 8, 2021, 3:35 p.m. UTC | #10
Thomas Huth <thuth@redhat.com> writes:

> On 08/06/2021 16.27, Alex Bennée wrote:

>> Richard Henderson <richard.henderson@linaro.org> writes:

>> 

>>> On 6/4/21 8:53 AM, Alex Bennée wrote:

>>>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>>>> machines is about to be broken...

>>>

>>> Um, what?

>> Really what we want is to probe the -M (machines) that a binary

>> supports rather than just barfing the test because we've built a QEMU

>> that doesn't support all the random 32 bit machines.

>> 

>>> r~

>>>

>>>

>>>

>>>   and besides it's not likely this is

>>>> improving out coverage by much. Test the "virt" machine for both arm

>>>> and aarch64 as it can be used by either architecture.

>> I think this point still stands though, I don't think we get much

>> from

>> running the cdrom test with realview et all on qemu-system-aarch64.

>

> In a lot of CI pipelines, we are either building aarch64 or arm, but

> not both, so I think it might be good to keep the tests in here.


We do test instantiating the cdrom with -M virt, exactly how many extra
lines of coverage do we get for the rest?

>

>  Thomas



-- 
Alex Bennée
John Snow June 8, 2021, 3:36 p.m. UTC | #11
On 6/8/21 11:01 AM, Thomas Huth wrote:
> On 08/06/2021 16.27, Alex Bennée wrote:

>>

>> Richard Henderson <richard.henderson@linaro.org> writes:

>>

>>> On 6/4/21 8:53 AM, Alex Bennée wrote:

>>>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>>>> machines is about to be broken...

>>>

>>> Um, what?

>>

>> Really what we want is to probe the -M (machines) that a binary

>> supports rather than just barfing the test because we've built a QEMU

>> that doesn't support all the random 32 bit machines.

>>

>>> r~

>>>

>>>

>>>

>>>   and besides it's not likely this is

>>>> improving out coverage by much. Test the "virt" machine for both arm

>>>> and aarch64 as it can be used by either architecture.

>>

>> I think this point still stands though, I don't think we get much from

>> running the cdrom test with realview et all on qemu-system-aarch64.

> 

> In a lot of CI pipelines, we are either building aarch64 or arm, but not 

> both, so I think it might be good to keep the tests in here.

> 

>   Thomas

> 


I'm deferring to Thomas on this -- it has just a little bit less to do 
with the CDROM itself and more to do with machine configuration, which I 
consider outside of my wheelhouse here.

--js
Thomas Huth June 8, 2021, 5:23 p.m. UTC | #12
On 08/06/2021 17.35, Alex Bennée wrote:
> 

> Thomas Huth <thuth@redhat.com> writes:

> 

>> On 08/06/2021 16.27, Alex Bennée wrote:

>>> Richard Henderson <richard.henderson@linaro.org> writes:

>>>

>>>> On 6/4/21 8:53 AM, Alex Bennée wrote:

>>>>> The assumption that the qemu-system-aarch64 image can run all 32 bit

>>>>> machines is about to be broken...

>>>>

>>>> Um, what?

>>> Really what we want is to probe the -M (machines) that a binary

>>> supports rather than just barfing the test because we've built a QEMU

>>> that doesn't support all the random 32 bit machines.

>>>

>>>> r~

>>>>

>>>>

>>>>

>>>>    and besides it's not likely this is

>>>>> improving out coverage by much. Test the "virt" machine for both arm

>>>>> and aarch64 as it can be used by either architecture.

>>> I think this point still stands though, I don't think we get much

>>> from

>>> running the cdrom test with realview et all on qemu-system-aarch64.

>>

>> In a lot of CI pipelines, we are either building aarch64 or arm, but

>> not both, so I think it might be good to keep the tests in here.

> 

> We do test instantiating the cdrom with -M virt, exactly how many extra

> lines of coverage do we get for the rest?


$ grep -r block_default_type.*IF_ hw/arm/
hw/arm/tosa.c:    mc->block_default_type = IF_IDE;
hw/arm/cubieboard.c:    mc->block_default_type = IF_IDE;
hw/arm/virt.c:    mc->block_default_type = IF_VIRTIO;
hw/arm/spitz.c:    mc->block_default_type = IF_IDE;
hw/arm/orangepi.c:    mc->block_default_type = IF_SD;
hw/arm/raspi.c:    mc->block_default_type = IF_SD;
hw/arm/realview.c:    mc->block_default_type = IF_SCSI;
hw/arm/realview.c:    mc->block_default_type = IF_SCSI;
hw/arm/xlnx-zcu102.c:    mc->block_default_type = IF_IDE;
hw/arm/highbank.c:    mc->block_default_type = IF_IDE;
hw/arm/highbank.c:    mc->block_default_type = IF_IDE;
hw/arm/sbsa-ref.c:    mc->block_default_type = IF_IDE;
hw/arm/versatilepb.c:    mc->block_default_type = IF_SCSI;
hw/arm/versatilepb.c:    mc->block_default_type = IF_SCSI;

... thus these tests check quite a bit of different ways to pass a cdrom 
drive to the machine. I'd rather suggest to keep them, but you're the arm 
guy here, so if you don't like these tests anymore, feel free to drop them.

  Thomas
diff mbox series

Patch

diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index 5af944a5fb..1e74354624 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -220,13 +220,16 @@  int main(int argc, char **argv)
             "magnum", "malta", "pica61", NULL
         };
         add_cdrom_param_tests(mips64machines);
-    } else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {
+    } else if (g_str_equal(arch, "arm")) {
         const char *armmachines[] = {
             "realview-eb", "realview-eb-mpcore", "realview-pb-a8",
             "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
             "vexpress-a9", "virt", NULL
         };
         add_cdrom_param_tests(armmachines);
+    } else if (g_str_equal(arch, "aarch64")) {
+        const char *aarch64machines[] = { "virt", NULL };
+        add_cdrom_param_tests(aarch64machines);
     } else {
         const char *nonemachine[] = { "none", NULL };
         add_cdrom_param_tests(nonemachine);