| Message ID | 20230228192628.26140-10-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Allow CONFIG_TCG=n builds | expand |
Fabiano Rosas <farosas@suse.de> wrote: > It is possible to have a build with both TCG and KVM disabled due to > Xen requiring the i386 and x86_64 binaries to be present in an aarch64 > host. Ouch. Just curious: why are they needed? > > If we build with --disable-tcg on the aarch64 host, we will end-up > with a QEMU binary (x86) that does not support TCG nor KVM. > > Fix tests that crash or hang in the above scenario. Do not include any > test cases if TCG and KVM are missing. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > This currently affects Arm, but will also affect x86 after the xenpvh > series gets merged. This patch fixes both scenarios. Reviewed-by: Juan Quintela <quintela@redhat.com>
Juan Quintela <quintela@redhat.com> writes: > Fabiano Rosas <farosas@suse.de> wrote: >> It is possible to have a build with both TCG and KVM disabled due to >> Xen requiring the i386 and x86_64 binaries to be present in an aarch64 >> host. > > Ouch. > > Just curious: why are they needed? > From https://wiki.xenproject.org/wiki/QEMU_Upstream: Why is qemu-system-i386 used even on x86_64 and even non-x86? QEMU in a Xen system only provides device model (DM) emulation and not any CPU instruction emulation, so the nominal arch doesn't actually matter and Xen builds i386 everywhere as a basically arbitrary choice. It happens that the Xen DM part of QEMU is quite closely tied to the x86 scaffolding for various historical reasons, so we end up using qemu-system-i386 even e.g. on ARM! There is no practical difference between qemu-system-i386 and qemu-system-x86_64, they should be interchangeable. However only qemu-system-i386 is regularly tested by Xen Project (via osstest). >> >> If we build with --disable-tcg on the aarch64 host, we will end-up >> with a QEMU binary (x86) that does not support TCG nor KVM. >> >> Fix tests that crash or hang in the above scenario. Do not include any >> test cases if TCG and KVM are missing. >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> This currently affects Arm, but will also affect x86 after the xenpvh >> series gets merged. This patch fixes both scenarios. > > Reviewed-by: Juan Quintela <quintela@redhat.com> Thanks!
Fabiano Rosas <farosas@suse.de> writes: > Juan Quintela <quintela@redhat.com> writes: > >> Fabiano Rosas <farosas@suse.de> wrote: >>> It is possible to have a build with both TCG and KVM disabled due to >>> Xen requiring the i386 and x86_64 binaries to be present in an aarch64 >>> host. >> >> Ouch. >> >> Just curious: why are they needed? >> > > From https://wiki.xenproject.org/wiki/QEMU_Upstream: > > Why is qemu-system-i386 used even on x86_64 and even non-x86? > > QEMU in a Xen system only provides device model (DM) emulation and not > any CPU instruction emulation, so the nominal arch doesn't actually > matter and Xen builds i386 everywhere as a basically arbitrary choice. > > It happens that the Xen DM part of QEMU is quite closely tied to the x86 > scaffolding for various historical reasons, so we end up using > qemu-system-i386 even e.g. on ARM! There is no practical difference > between qemu-system-i386 and qemu-system-x86_64, they should be > interchangeable. However only qemu-system-i386 is regularly tested by > Xen Project (via osstest). That said with the xenpvh model that was added recently we should be able to finally build a Xen only qemu-system-aarch64 which while functionally the same will be less head scratching for users. > >>> >>> If we build with --disable-tcg on the aarch64 host, we will end-up >>> with a QEMU binary (x86) that does not support TCG nor KVM. >>> >>> Fix tests that crash or hang in the above scenario. Do not include any >>> test cases if TCG and KVM are missing. >>> >>> Signed-off-by: Fabiano Rosas <farosas@suse.de> >>> --- >>> This currently affects Arm, but will also affect x86 after the xenpvh >>> series gets merged. This patch fixes both scenarios. >> >> Reviewed-by: Juan Quintela <quintela@redhat.com> > > Thanks!
On 28/02/2023 20.26, Fabiano Rosas wrote: > It is possible to have a build with both TCG and KVM disabled due to > Xen requiring the i386 and x86_64 binaries to be present in an aarch64 > host. > > If we build with --disable-tcg on the aarch64 host, we will end-up > with a QEMU binary (x86) that does not support TCG nor KVM. > > Fix tests that crash or hang in the above scenario. Do not include any > test cases if TCG and KVM are missing. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- ... > diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c > index 3aef3a97a9..45490f5931 100644 > --- a/tests/qtest/boot-serial-test.c > +++ b/tests/qtest/boot-serial-test.c > @@ -17,6 +17,9 @@ > #include "libqtest.h" > #include "libqos/libqos-spapr.h" > > +static bool has_tcg; > +static bool has_kvm; Any special reason for putting these here instead of making them local variables in the main() function? > static const uint8_t bios_avr[] = { > 0x88, 0xe0, /* ldi r24, 0x08 */ > 0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */ > @@ -285,6 +288,13 @@ int main(int argc, char *argv[]) > const char *arch = qtest_get_arch(); > int i; > > + has_tcg = qtest_has_accel("tcg"); > + has_kvm = qtest_has_accel("kvm"); > + > + if (!has_tcg && !has_kvm) { > + return 0; > + } > + > g_test_init(&argc, &argv, NULL); Could you please put the new code below the g_test_init() ? Just to avoid the problem that has been reported here: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg08331.html Thanks, Thomas > for (i = 0; tests[i].arch != NULL; i++) { > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 109bc8e7b1..a6e3ca9f7d 100644 > --- a/tests/qtest/migration-test.c > +++ b/tests/qtest/migration-test.c > @@ -2460,11 +2460,16 @@ static bool kvm_dirty_ring_supported(void) > int main(int argc, char **argv) > { > const bool has_kvm = qtest_has_accel("kvm"); > + const bool has_tcg = qtest_has_accel("tcg"); > const bool has_uffd = ufd_version_check(); > const char *arch = qtest_get_arch(); > g_autoptr(GError) err = NULL; > int ret; > > + if (!has_tcg && !has_kvm) { > + return 0; > + } > + > g_test_init(&argc, &argv, NULL); > > /* > diff --git a/tests/qtest/pxe-test.c b/tests/qtest/pxe-test.c > index 62b6eef464..05575f7687 100644 > --- a/tests/qtest/pxe-test.c > +++ b/tests/qtest/pxe-test.c > @@ -130,6 +130,12 @@ int main(int argc, char *argv[]) > { > int ret; > const char *arch = qtest_get_arch(); > + bool has_tcg = qtest_has_accel("tcg"); > + bool has_kvm = qtest_has_accel("kvm"); > + > + if (!has_tcg && !has_kvm) { > + return 0; > + } > > ret = boot_sector_init(disk); > if(ret) > diff --git a/tests/qtest/vmgenid-test.c b/tests/qtest/vmgenid-test.c > index efba76e716..8045d3d706 100644 > --- a/tests/qtest/vmgenid-test.c > +++ b/tests/qtest/vmgenid-test.c > @@ -164,6 +164,12 @@ static void vmgenid_query_monitor_test(void) > int main(int argc, char **argv) > { > int ret; > + bool has_tcg = qtest_has_accel("tcg"); > + bool has_kvm = qtest_has_accel("kvm"); > + > + if (!has_tcg && !has_kvm) { > + return 0; > + } > > ret = boot_sector_init(disk); > if (ret) {
Thomas Huth <thuth@redhat.com> writes: > On 28/02/2023 20.26, Fabiano Rosas wrote: >> It is possible to have a build with both TCG and KVM disabled due to >> Xen requiring the i386 and x86_64 binaries to be present in an aarch64 >> host. >> >> If we build with --disable-tcg on the aarch64 host, we will end-up >> with a QEMU binary (x86) that does not support TCG nor KVM. >> >> Fix tests that crash or hang in the above scenario. Do not include any >> test cases if TCG and KVM are missing. >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- > ... >> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c >> index 3aef3a97a9..45490f5931 100644 >> --- a/tests/qtest/boot-serial-test.c >> +++ b/tests/qtest/boot-serial-test.c >> @@ -17,6 +17,9 @@ >> #include "libqtest.h" >> #include "libqos/libqos-spapr.h" >> >> +static bool has_tcg; >> +static bool has_kvm; > > Any special reason for putting these here instead of making them local > variables in the main() function? > Yes, Phillipe was doing work in the same file and I put it here to minimize conflicts. https://lore.kernel.org/r/20230119145838.41835-5-philmd@linaro.org >> static const uint8_t bios_avr[] = { >> 0x88, 0xe0, /* ldi r24, 0x08 */ >> 0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */ >> @@ -285,6 +288,13 @@ int main(int argc, char *argv[]) >> const char *arch = qtest_get_arch(); >> int i; >> >> + has_tcg = qtest_has_accel("tcg"); >> + has_kvm = qtest_has_accel("kvm"); >> + >> + if (!has_tcg && !has_kvm) { >> + return 0; >> + } >> + >> g_test_init(&argc, &argv, NULL); > > Could you please put the new code below the g_test_init() ? > Just to avoid the problem that has been reported here: > > https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg08331.html > I could, but I don't understand why we need this. What does having "code" before g_test_init() causes? Should I move the qtest_get_arch() that's already there as well?
Alex Bennée <alex.bennee@linaro.org> writes: > Fabiano Rosas <farosas@suse.de> writes: > >> Juan Quintela <quintela@redhat.com> writes: >> >>> Fabiano Rosas <farosas@suse.de> wrote: >>>> It is possible to have a build with both TCG and KVM disabled due to >>>> Xen requiring the i386 and x86_64 binaries to be present in an aarch64 >>>> host. >>> >>> Ouch. >>> >>> Just curious: why are they needed? >>> >> >> From https://wiki.xenproject.org/wiki/QEMU_Upstream: >> >> Why is qemu-system-i386 used even on x86_64 and even non-x86? >> >> QEMU in a Xen system only provides device model (DM) emulation and not >> any CPU instruction emulation, so the nominal arch doesn't actually >> matter and Xen builds i386 everywhere as a basically arbitrary choice. >> >> It happens that the Xen DM part of QEMU is quite closely tied to the x86 >> scaffolding for various historical reasons, so we end up using >> qemu-system-i386 even e.g. on ARM! There is no practical difference >> between qemu-system-i386 and qemu-system-x86_64, they should be >> interchangeable. However only qemu-system-i386 is regularly tested by >> Xen Project (via osstest). > > That said with the xenpvh model that was added recently we should be > able to finally build a Xen only qemu-system-aarch64 which while > functionally the same will be less head scratching for users. > It would be nice if we could eventually restrict the x86 build to the x86 host and the aarch64 build to the aarch64 host like we do for the other HW accels.
Fabiano Rosas <farosas@suse.de> writes: > Thomas Huth <thuth@redhat.com> writes: > >> On 28/02/2023 20.26, Fabiano Rosas wrote: >>> It is possible to have a build with both TCG and KVM disabled due to >>> Xen requiring the i386 and x86_64 binaries to be present in an aarch64 >>> host. >>> >>> If we build with --disable-tcg on the aarch64 host, we will end-up >>> with a QEMU binary (x86) that does not support TCG nor KVM. >>> >>> Fix tests that crash or hang in the above scenario. Do not include any >>> test cases if TCG and KVM are missing. >>> >>> Signed-off-by: Fabiano Rosas <farosas@suse.de> >>> --- >> ... >>> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c >>> index 3aef3a97a9..45490f5931 100644 >>> --- a/tests/qtest/boot-serial-test.c >>> +++ b/tests/qtest/boot-serial-test.c >>> @@ -17,6 +17,9 @@ >>> #include "libqtest.h" >>> #include "libqos/libqos-spapr.h" >>> >>> +static bool has_tcg; >>> +static bool has_kvm; >> >> Any special reason for putting these here instead of making them local >> variables in the main() function? >> > > Yes, Phillipe was doing work in the same file and I put it here to > minimize conflicts. > > https://lore.kernel.org/r/20230119145838.41835-5-philmd@linaro.org > >>> static const uint8_t bios_avr[] = { >>> 0x88, 0xe0, /* ldi r24, 0x08 */ >>> 0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */ >>> @@ -285,6 +288,13 @@ int main(int argc, char *argv[]) >>> const char *arch = qtest_get_arch(); >>> int i; >>> >>> + has_tcg = qtest_has_accel("tcg"); >>> + has_kvm = qtest_has_accel("kvm"); >>> + >>> + if (!has_tcg && !has_kvm) { >>> + return 0; >>> + } >>> + >>> g_test_init(&argc, &argv, NULL); >> >> Could you please put the new code below the g_test_init() ? >> Just to avoid the problem that has been reported here: >> >> https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg08331.html >> > > I could, but I don't understand why we need this. What does having > "code" before g_test_init() causes? Should I move the qtest_get_arch() > that's already there as well? Oh, the issue is the early return? I guess it makes sense.
On 01/03/2023 14.43, Fabiano Rosas wrote: > Fabiano Rosas <farosas@suse.de> writes: > >> Thomas Huth <thuth@redhat.com> writes: >> >>> On 28/02/2023 20.26, Fabiano Rosas wrote: >>>> It is possible to have a build with both TCG and KVM disabled due to >>>> Xen requiring the i386 and x86_64 binaries to be present in an aarch64 >>>> host. >>>> >>>> If we build with --disable-tcg on the aarch64 host, we will end-up >>>> with a QEMU binary (x86) that does not support TCG nor KVM. >>>> >>>> Fix tests that crash or hang in the above scenario. Do not include any >>>> test cases if TCG and KVM are missing. >>>> >>>> Signed-off-by: Fabiano Rosas <farosas@suse.de> >>>> --- >>> ... >>>> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c >>>> index 3aef3a97a9..45490f5931 100644 >>>> --- a/tests/qtest/boot-serial-test.c >>>> +++ b/tests/qtest/boot-serial-test.c >>>> @@ -17,6 +17,9 @@ >>>> #include "libqtest.h" >>>> #include "libqos/libqos-spapr.h" >>>> >>>> +static bool has_tcg; >>>> +static bool has_kvm; >>> >>> Any special reason for putting these here instead of making them local >>> variables in the main() function? >>> >> >> Yes, Phillipe was doing work in the same file and I put it here to >> minimize conflicts. >> >> https://lore.kernel.org/r/20230119145838.41835-5-philmd@linaro.org >> >>>> static const uint8_t bios_avr[] = { >>>> 0x88, 0xe0, /* ldi r24, 0x08 */ >>>> 0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */ >>>> @@ -285,6 +288,13 @@ int main(int argc, char *argv[]) >>>> const char *arch = qtest_get_arch(); >>>> int i; >>>> >>>> + has_tcg = qtest_has_accel("tcg"); >>>> + has_kvm = qtest_has_accel("kvm"); >>>> + >>>> + if (!has_tcg && !has_kvm) { >>>> + return 0; >>>> + } >>>> + >>>> g_test_init(&argc, &argv, NULL); >>> >>> Could you please put the new code below the g_test_init() ? >>> Just to avoid the problem that has been reported here: >>> >>> https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg08331.html >>> >> >> I could, but I don't understand why we need this. What does having >> "code" before g_test_init() causes? Should I move the qtest_get_arch() >> that's already there as well? > > Oh, the issue is the early return? I guess it makes sense. Yes, as far as I've undrestood the issue: If we call a function that starts a QEMU subprocess (like qtest_has_device() or qtest_has_accel()), then this could spoil the output since the TAP version from g_test_init() should come first. qtest_get_arch() is not a problem, since it does not start a QEMU subprocess. Thomas
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index d29a4e47af..f6c2a010d2 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -2114,6 +2114,10 @@ int main(int argc, char *argv[]) char *v_env = getenv("V"); int ret; + if (!has_tcg && !has_kvm) { + return 0; + } + if (v_env) { verbosity_level = atoi(v_env); } diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c index 3aef3a97a9..45490f5931 100644 --- a/tests/qtest/boot-serial-test.c +++ b/tests/qtest/boot-serial-test.c @@ -17,6 +17,9 @@ #include "libqtest.h" #include "libqos/libqos-spapr.h" +static bool has_tcg; +static bool has_kvm; + static const uint8_t bios_avr[] = { 0x88, 0xe0, /* ldi r24, 0x08 */ 0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */ @@ -285,6 +288,13 @@ int main(int argc, char *argv[]) const char *arch = qtest_get_arch(); int i; + has_tcg = qtest_has_accel("tcg"); + has_kvm = qtest_has_accel("kvm"); + + if (!has_tcg && !has_kvm) { + return 0; + } + g_test_init(&argc, &argv, NULL); for (i = 0; tests[i].arch != NULL; i++) { diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 109bc8e7b1..a6e3ca9f7d 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -2460,11 +2460,16 @@ static bool kvm_dirty_ring_supported(void) int main(int argc, char **argv) { const bool has_kvm = qtest_has_accel("kvm"); + const bool has_tcg = qtest_has_accel("tcg"); const bool has_uffd = ufd_version_check(); const char *arch = qtest_get_arch(); g_autoptr(GError) err = NULL; int ret; + if (!has_tcg && !has_kvm) { + return 0; + } + g_test_init(&argc, &argv, NULL); /* diff --git a/tests/qtest/pxe-test.c b/tests/qtest/pxe-test.c index 62b6eef464..05575f7687 100644 --- a/tests/qtest/pxe-test.c +++ b/tests/qtest/pxe-test.c @@ -130,6 +130,12 @@ int main(int argc, char *argv[]) { int ret; const char *arch = qtest_get_arch(); + bool has_tcg = qtest_has_accel("tcg"); + bool has_kvm = qtest_has_accel("kvm"); + + if (!has_tcg && !has_kvm) { + return 0; + } ret = boot_sector_init(disk); if(ret) diff --git a/tests/qtest/vmgenid-test.c b/tests/qtest/vmgenid-test.c index efba76e716..8045d3d706 100644 --- a/tests/qtest/vmgenid-test.c +++ b/tests/qtest/vmgenid-test.c @@ -164,6 +164,12 @@ static void vmgenid_query_monitor_test(void) int main(int argc, char **argv) { int ret; + bool has_tcg = qtest_has_accel("tcg"); + bool has_kvm = qtest_has_accel("kvm"); + + if (!has_tcg && !has_kvm) { + return 0; + } ret = boot_sector_init(disk); if (ret) {
It is possible to have a build with both TCG and KVM disabled due to Xen requiring the i386 and x86_64 binaries to be present in an aarch64 host. If we build with --disable-tcg on the aarch64 host, we will end-up with a QEMU binary (x86) that does not support TCG nor KVM. Fix tests that crash or hang in the above scenario. Do not include any test cases if TCG and KVM are missing. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- This currently affects Arm, but will also affect x86 after the xenpvh series gets merged. This patch fixes both scenarios. --- tests/qtest/bios-tables-test.c | 4 ++++ tests/qtest/boot-serial-test.c | 10 ++++++++++ tests/qtest/migration-test.c | 5 +++++ tests/qtest/pxe-test.c | 6 ++++++ tests/qtest/vmgenid-test.c | 6 ++++++ 5 files changed, 31 insertions(+)