| 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 |
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); >
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); >
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
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
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
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
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
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~
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
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
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
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 --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);
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