Laszlo Ersek
2022-Jul-20 08:14 UTC
[Libguestfs] [PATCH] always 'max' for the appliance CPU model on all targes except ppc
On 05/27/22 10:21, Laszlo Ersek wrote:> On 05/25/22 15:30, Daniel P. Berrang? wrote: >> The current logic for selecting CPU model to use with the appliance >> selects 'max' for all architectures except for ppc64le and aarch64. >> >> On aarch64, it selects 'host' if KVM is available which is identical >> to 'max'. For TCG it selects 'cortex-a57'. The 'max' model is a >> superset of 'cortex-a57', so it is reasonable to use 'max' for TCG >> too. >> >> On ppc64, it selects no CPU model due to a historical bug where >> using 'host' would break ability to fallback to TCG. Unfortunately >> we can't use 'max' on PPC as this is actually an old G4 vintage >> 32-bit model, rather than a synonym for 'host' / all-TCG-features >> as on other targets. >> >> We can at least simplify the code to use 'max' in all scenarios >> for appliance CPU model, and simply skip a CPU model for PPC. >> >> Signed-off-by: Daniel P. Berrang? <berrange at redhat.com> >> --- >> docs/C_SOURCE_FILES | 1 - >> lib/Makefile.am | 1 - >> lib/appliance-cpu.c | 93 ------------------------------------------ >> lib/guestfs-internal.h | 3 -- >> lib/launch-direct.c | 25 +++++------- >> lib/launch-libvirt.c | 40 ++++++++---------- >> po/POTFILES | 1 - >> 7 files changed, 27 insertions(+), 137 deletions(-) >> delete mode 100644 lib/appliance-cpu.c >> >> diff --git a/docs/C_SOURCE_FILES b/docs/C_SOURCE_FILES >> index a367c93a0..a26487d99 100644 >> --- a/docs/C_SOURCE_FILES >> +++ b/docs/C_SOURCE_FILES >> @@ -283,7 +283,6 @@ lib/actions-6.c >> lib/actions-support.c >> lib/actions-variants.c >> lib/alloc.c >> -lib/appliance-cpu.c >> lib/appliance-kcmdline.c >> lib/appliance-uefi.c >> lib/appliance.c >> diff --git a/lib/Makefile.am b/lib/Makefile.am >> index 212bcb94a..40a4c9ac3 100644 >> --- a/lib/Makefile.am >> +++ b/lib/Makefile.am >> @@ -71,7 +71,6 @@ libguestfs_la_SOURCES = \ >> actions-variants.c \ >> alloc.c \ >> appliance.c \ >> - appliance-cpu.c \ >> appliance-kcmdline.c \ >> appliance-uefi.c \ >> available.c \ >> diff --git a/lib/appliance-cpu.c b/lib/appliance-cpu.c >> deleted file mode 100644 >> index 54ac6e2e3..000000000 >> --- a/lib/appliance-cpu.c >> +++ /dev/null >> @@ -1,93 +0,0 @@ >> -/* libguestfs >> - * Copyright (C) 2009-2020 Red Hat Inc. >> - * >> - * This library is free software; you can redistribute it and/or >> - * modify it under the terms of the GNU Lesser General Public >> - * License as published by the Free Software Foundation; either >> - * version 2 of the License, or (at your option) any later version. >> - * >> - * This library is distributed in the hope that it will be useful, >> - * but WITHOUT ANY WARRANTY; without even the implied warranty of >> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> - * Lesser General Public License for more details. >> - * >> - * You should have received a copy of the GNU Lesser General Public >> - * License along with this library; if not, write to the Free Software >> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA >> - */ >> - >> -/** >> - * The appliance choice of CPU model. >> - */ >> - >> -#include <config.h> >> - >> -#include <stdio.h> >> -#include <stdlib.h> >> - >> -#include "guestfs.h" >> -#include "guestfs-internal.h" >> - >> -/** >> - * Return the right CPU model to use as the qemu C<-cpu> parameter or >> - * its equivalent in libvirt. This returns: >> - * >> - * =over 4 >> - * >> - * =item C<"host"> >> - * >> - * The literal string C<"host"> means use C<-cpu host>. >> - * >> - * =item C<"max"> >> - * >> - * The literal string C<"max"> means use C<-cpu max> (the best >> - * possible). This requires awkward translation for libvirt. >> - * >> - * =item some string >> - * >> - * Some string such as C<"cortex-a57"> means use C<-cpu cortex-a57>. >> - * >> - * =item C<NULL> >> - * >> - * C<NULL> means no C<-cpu> option at all. Note returning C<NULL> >> - * does not indicate an error. >> - * >> - * =back >> - * >> - * This is made unnecessarily hard and fragile because of two stupid >> - * choices in QEMU: >> - * >> - * =over 4 >> - * >> - * =item * >> - * >> - * The default for C<qemu-system-aarch64 -M virt> is to emulate a >> - * C<cortex-a15> (WTF?). >> - * >> - * =item * >> - * >> - * We don't know for sure if KVM will work, but C<-cpu host> is broken >> - * with TCG, so we almost always pass a broken C<-cpu> flag if KVM is >> - * semi-broken in any way. >> - * >> - * =back >> - */ >> -const char * >> -guestfs_int_get_cpu_model (int kvm) >> -{ >> -#if defined(__aarch64__) >> - /* With -M virt, the default -cpu is cortex-a15. Stupid. */ >> - if (kvm) >> - return "host"; >> - else >> - return "cortex-a57"; >> -#elif defined(__powerpc64__) >> - /* See discussion in https://bugzilla.redhat.com/show_bug.cgi?id=1605071 */ >> - return NULL; >> -#else >> - /* On most architectures we can use "max" to get the best possible CPU. >> - * For recent qemu this should work even on TCG. >> - */ >> - return "max"; >> -#endif >> -} >> diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h >> index 16755cfb3..33037a26d 100644 >> --- a/lib/guestfs-internal.h >> +++ b/lib/guestfs-internal.h >> @@ -718,9 +718,6 @@ extern const char *guestfs_int_drive_protocol_to_string (enum drive_protocol pro >> /* appliance.c */ >> extern int guestfs_int_build_appliance (guestfs_h *g, char **kernel, char **initrd, char **appliance); >> >> -/* appliance-cpu.c */ >> -const char *guestfs_int_get_cpu_model (int kvm); >> - >> /* appliance-kcmdline.c */ >> extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance, int flags); >> #define APPLIANCE_COMMAND_LINE_IS_TCG 1 >> diff --git a/lib/launch-direct.c b/lib/launch-direct.c >> index ff0eaeb62..f41711353 100644 >> --- a/lib/launch-direct.c >> +++ b/lib/launch-direct.c >> @@ -354,7 +354,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) >> int force_tcg; >> int force_kvm; >> const char *accel_val = "kvm:tcg"; >> - const char *cpu_model; >> CLEANUP_FREE char *append = NULL; >> CLEANUP_FREE_STRING_LIST char **argv = NULL; >> >> @@ -517,20 +516,18 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) >> #endif >> } end_list (); >> >> - cpu_model = guestfs_int_get_cpu_model (has_kvm && !force_tcg); >> - if (cpu_model) { >> -#if defined(__x86_64__) >> - /* Temporary workaround for RHBZ#2082806 */ >> - if (STREQ (cpu_model, "max")) { >> - start_list ("-cpu") { >> - append_list (cpu_model); >> - append_list ("la57=off"); >> - } end_list (); >> - } >> - else >> + /* >> + * Can't use 'max' on ppc64 since it is an old G4 model >> + * Also can't use 'host' due to TCG fallback issues >> + * https://bugzilla.redhat.com/show_bug.cgi?id=1605071 >> + */ >> +#if !defined(__powerpc64__) >> +# if defined(__x86_64__) >> + arg ("-cpu", "max,la57=off"); >> +# else >> + arg ("-cpu", "max"); >> +# endif >> #endif >> - arg ("-cpu", cpu_model); >> - } >> >> if (g->smp > 1) >> arg_format ("-smp", "%d", g->smp); >> diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c >> index 03d69e027..b97c91566 100644 >> --- a/lib/launch-libvirt.c >> +++ b/lib/launch-libvirt.c >> @@ -1161,8 +1161,6 @@ construct_libvirt_xml_cpu (guestfs_h *g, >> const struct libvirt_xml_params *params, >> xmlTextWriterPtr xo) >> { >> - const char *cpu_model; >> - >> start_element ("memory") { >> attribute ("unit", "MiB"); >> string_format ("%d", g->memsize); >> @@ -1173,30 +1171,24 @@ construct_libvirt_xml_cpu (guestfs_h *g, >> string_format ("%d", g->memsize); >> } end_element (); >> >> - cpu_model = guestfs_int_get_cpu_model (params->data->is_kvm); >> - if (cpu_model) { >> - start_element ("cpu") { >> - if (STREQ (cpu_model, "host")) { >> - attribute ("mode", "host-passthrough"); >> - start_element ("model") { >> - attribute ("fallback", "allow"); >> - } end_element (); >> - } >> - else if (STREQ (cpu_model, "max")) { >> - /* https://bugzilla.redhat.com/show_bug.cgi?id=1935572#c11 */ >> - attribute ("mode", "maximum"); >> + /* >> + * Can't use 'max' on ppc64 since it is an old G4 model >> + * Also can't use 'host' due to TCG fallback issues >> + * https://bugzilla.redhat.com/show_bug.cgi?id=1605071 >> + */ >> +#if !defined(__powerpc64__) >> + start_element ("cpu") { >> + /* https://bugzilla.redhat.com/show_bug.cgi?id=1935572#c11 */ >> + attribute ("mode", "maximum"); >> #if defined(__x86_64__) >> - /* Temporary workaround for RHBZ#2082806 */ >> - start_element ("feature") { >> - attribute ("policy", "disable"); >> - attribute ("name", "la57"); >> - } end_element (); >> -#endif >> - } >> - else >> - single_element ("model", cpu_model); >> + /* Temporary workaround for RHBZ#2082806 */ >> + start_element ("feature") { >> + attribute ("policy", "disable"); >> + attribute ("name", "la57"); >> } end_element (); >> - } >> +#endif >> + } end_element (); >> +#endif >> >> single_element_format ("vcpu", "%d", g->smp); >> >> diff --git a/po/POTFILES b/po/POTFILES >> index 32b975a04..67cdffb29 100644 >> --- a/po/POTFILES >> +++ b/po/POTFILES >> @@ -329,7 +329,6 @@ lib/actions-6.c >> lib/actions-support.c >> lib/actions-variants.c >> lib/alloc.c >> -lib/appliance-cpu.c >> lib/appliance-kcmdline.c >> lib/appliance-uefi.c >> lib/appliance.c >> > > Acked-by: Laszlo Ersek <lersek at redhat.com> >Was this patch forgotten? I don't see it in the commit history (by subject). Thanks Laszlo
Richard W.M. Jones
2022-Jul-20 11:51 UTC
[Libguestfs] [PATCH] always 'max' for the appliance CPU model on all targes except ppc
On Wed, Jul 20, 2022 at 10:14:00AM +0200, Laszlo Ersek wrote:> On 05/27/22 10:21, Laszlo Ersek wrote: > > On 05/25/22 15:30, Daniel P. Berrang? wrote: > >> The current logic for selecting CPU model to use with the appliance > >> selects 'max' for all architectures except for ppc64le and aarch64. > >> > >> On aarch64, it selects 'host' if KVM is available which is identical > >> to 'max'. For TCG it selects 'cortex-a57'. The 'max' model is a > >> superset of 'cortex-a57', so it is reasonable to use 'max' for TCG > >> too. > >> > >> On ppc64, it selects no CPU model due to a historical bug where > >> using 'host' would break ability to fallback to TCG. Unfortunately > >> we can't use 'max' on PPC as this is actually an old G4 vintage > >> 32-bit model, rather than a synonym for 'host' / all-TCG-features > >> as on other targets. > >> > >> We can at least simplify the code to use 'max' in all scenarios > >> for appliance CPU model, and simply skip a CPU model for PPC. > >> > >> Signed-off-by: Daniel P. Berrang? <berrange at redhat.com> > >> --- > >> docs/C_SOURCE_FILES | 1 - > >> lib/Makefile.am | 1 - > >> lib/appliance-cpu.c | 93 ------------------------------------------ > >> lib/guestfs-internal.h | 3 -- > >> lib/launch-direct.c | 25 +++++------- > >> lib/launch-libvirt.c | 40 ++++++++---------- > >> po/POTFILES | 1 - > >> 7 files changed, 27 insertions(+), 137 deletions(-) > >> delete mode 100644 lib/appliance-cpu.c > >> > >> diff --git a/docs/C_SOURCE_FILES b/docs/C_SOURCE_FILES > >> index a367c93a0..a26487d99 100644 > >> --- a/docs/C_SOURCE_FILES > >> +++ b/docs/C_SOURCE_FILES > >> @@ -283,7 +283,6 @@ lib/actions-6.c > >> lib/actions-support.c > >> lib/actions-variants.c > >> lib/alloc.c > >> -lib/appliance-cpu.c > >> lib/appliance-kcmdline.c > >> lib/appliance-uefi.c > >> lib/appliance.c > >> diff --git a/lib/Makefile.am b/lib/Makefile.am > >> index 212bcb94a..40a4c9ac3 100644 > >> --- a/lib/Makefile.am > >> +++ b/lib/Makefile.am > >> @@ -71,7 +71,6 @@ libguestfs_la_SOURCES = \ > >> actions-variants.c \ > >> alloc.c \ > >> appliance.c \ > >> - appliance-cpu.c \ > >> appliance-kcmdline.c \ > >> appliance-uefi.c \ > >> available.c \ > >> diff --git a/lib/appliance-cpu.c b/lib/appliance-cpu.c > >> deleted file mode 100644 > >> index 54ac6e2e3..000000000 > >> --- a/lib/appliance-cpu.c > >> +++ /dev/null > >> @@ -1,93 +0,0 @@ > >> -/* libguestfs > >> - * Copyright (C) 2009-2020 Red Hat Inc. > >> - * > >> - * This library is free software; you can redistribute it and/or > >> - * modify it under the terms of the GNU Lesser General Public > >> - * License as published by the Free Software Foundation; either > >> - * version 2 of the License, or (at your option) any later version. > >> - * > >> - * This library is distributed in the hope that it will be useful, > >> - * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > >> - * Lesser General Public License for more details. > >> - * > >> - * You should have received a copy of the GNU Lesser General Public > >> - * License along with this library; if not, write to the Free Software > >> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > >> - */ > >> - > >> -/** > >> - * The appliance choice of CPU model. > >> - */ > >> - > >> -#include <config.h> > >> - > >> -#include <stdio.h> > >> -#include <stdlib.h> > >> - > >> -#include "guestfs.h" > >> -#include "guestfs-internal.h" > >> - > >> -/** > >> - * Return the right CPU model to use as the qemu C<-cpu> parameter or > >> - * its equivalent in libvirt. This returns: > >> - * > >> - * =over 4 > >> - * > >> - * =item C<"host"> > >> - * > >> - * The literal string C<"host"> means use C<-cpu host>. > >> - * > >> - * =item C<"max"> > >> - * > >> - * The literal string C<"max"> means use C<-cpu max> (the best > >> - * possible). This requires awkward translation for libvirt. > >> - * > >> - * =item some string > >> - * > >> - * Some string such as C<"cortex-a57"> means use C<-cpu cortex-a57>. > >> - * > >> - * =item C<NULL> > >> - * > >> - * C<NULL> means no C<-cpu> option at all. Note returning C<NULL> > >> - * does not indicate an error. > >> - * > >> - * =back > >> - * > >> - * This is made unnecessarily hard and fragile because of two stupid > >> - * choices in QEMU: > >> - * > >> - * =over 4 > >> - * > >> - * =item * > >> - * > >> - * The default for C<qemu-system-aarch64 -M virt> is to emulate a > >> - * C<cortex-a15> (WTF?). > >> - * > >> - * =item * > >> - * > >> - * We don't know for sure if KVM will work, but C<-cpu host> is broken > >> - * with TCG, so we almost always pass a broken C<-cpu> flag if KVM is > >> - * semi-broken in any way. > >> - * > >> - * =back > >> - */ > >> -const char * > >> -guestfs_int_get_cpu_model (int kvm) > >> -{ > >> -#if defined(__aarch64__) > >> - /* With -M virt, the default -cpu is cortex-a15. Stupid. */ > >> - if (kvm) > >> - return "host"; > >> - else > >> - return "cortex-a57"; > >> -#elif defined(__powerpc64__) > >> - /* See discussion in https://bugzilla.redhat.com/show_bug.cgi?id=1605071 */ > >> - return NULL; > >> -#else > >> - /* On most architectures we can use "max" to get the best possible CPU. > >> - * For recent qemu this should work even on TCG. > >> - */ > >> - return "max"; > >> -#endif > >> -} > >> diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h > >> index 16755cfb3..33037a26d 100644 > >> --- a/lib/guestfs-internal.h > >> +++ b/lib/guestfs-internal.h > >> @@ -718,9 +718,6 @@ extern const char *guestfs_int_drive_protocol_to_string (enum drive_protocol pro > >> /* appliance.c */ > >> extern int guestfs_int_build_appliance (guestfs_h *g, char **kernel, char **initrd, char **appliance); > >> > >> -/* appliance-cpu.c */ > >> -const char *guestfs_int_get_cpu_model (int kvm); > >> - > >> /* appliance-kcmdline.c */ > >> extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance, int flags); > >> #define APPLIANCE_COMMAND_LINE_IS_TCG 1 > >> diff --git a/lib/launch-direct.c b/lib/launch-direct.c > >> index ff0eaeb62..f41711353 100644 > >> --- a/lib/launch-direct.c > >> +++ b/lib/launch-direct.c > >> @@ -354,7 +354,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) > >> int force_tcg; > >> int force_kvm; > >> const char *accel_val = "kvm:tcg"; > >> - const char *cpu_model; > >> CLEANUP_FREE char *append = NULL; > >> CLEANUP_FREE_STRING_LIST char **argv = NULL; > >> > >> @@ -517,20 +516,18 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) > >> #endif > >> } end_list (); > >> > >> - cpu_model = guestfs_int_get_cpu_model (has_kvm && !force_tcg); > >> - if (cpu_model) { > >> -#if defined(__x86_64__) > >> - /* Temporary workaround for RHBZ#2082806 */ > >> - if (STREQ (cpu_model, "max")) { > >> - start_list ("-cpu") { > >> - append_list (cpu_model); > >> - append_list ("la57=off"); > >> - } end_list (); > >> - } > >> - else > >> + /* > >> + * Can't use 'max' on ppc64 since it is an old G4 model > >> + * Also can't use 'host' due to TCG fallback issues > >> + * https://bugzilla.redhat.com/show_bug.cgi?id=1605071 > >> + */ > >> +#if !defined(__powerpc64__) > >> +# if defined(__x86_64__) > >> + arg ("-cpu", "max,la57=off"); > >> +# else > >> + arg ("-cpu", "max"); > >> +# endif > >> #endif > >> - arg ("-cpu", cpu_model); > >> - } > >> > >> if (g->smp > 1) > >> arg_format ("-smp", "%d", g->smp); > >> diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c > >> index 03d69e027..b97c91566 100644 > >> --- a/lib/launch-libvirt.c > >> +++ b/lib/launch-libvirt.c > >> @@ -1161,8 +1161,6 @@ construct_libvirt_xml_cpu (guestfs_h *g, > >> const struct libvirt_xml_params *params, > >> xmlTextWriterPtr xo) > >> { > >> - const char *cpu_model; > >> - > >> start_element ("memory") { > >> attribute ("unit", "MiB"); > >> string_format ("%d", g->memsize); > >> @@ -1173,30 +1171,24 @@ construct_libvirt_xml_cpu (guestfs_h *g, > >> string_format ("%d", g->memsize); > >> } end_element (); > >> > >> - cpu_model = guestfs_int_get_cpu_model (params->data->is_kvm); > >> - if (cpu_model) { > >> - start_element ("cpu") { > >> - if (STREQ (cpu_model, "host")) { > >> - attribute ("mode", "host-passthrough"); > >> - start_element ("model") { > >> - attribute ("fallback", "allow"); > >> - } end_element (); > >> - } > >> - else if (STREQ (cpu_model, "max")) { > >> - /* https://bugzilla.redhat.com/show_bug.cgi?id=1935572#c11 */ > >> - attribute ("mode", "maximum"); > >> + /* > >> + * Can't use 'max' on ppc64 since it is an old G4 model > >> + * Also can't use 'host' due to TCG fallback issues > >> + * https://bugzilla.redhat.com/show_bug.cgi?id=1605071 > >> + */ > >> +#if !defined(__powerpc64__) > >> + start_element ("cpu") { > >> + /* https://bugzilla.redhat.com/show_bug.cgi?id=1935572#c11 */ > >> + attribute ("mode", "maximum"); > >> #if defined(__x86_64__) > >> - /* Temporary workaround for RHBZ#2082806 */ > >> - start_element ("feature") { > >> - attribute ("policy", "disable"); > >> - attribute ("name", "la57"); > >> - } end_element (); > >> -#endif > >> - } > >> - else > >> - single_element ("model", cpu_model); > >> + /* Temporary workaround for RHBZ#2082806 */ > >> + start_element ("feature") { > >> + attribute ("policy", "disable"); > >> + attribute ("name", "la57"); > >> } end_element (); > >> - } > >> +#endif > >> + } end_element (); > >> +#endif > >> > >> single_element_format ("vcpu", "%d", g->smp); > >> > >> diff --git a/po/POTFILES b/po/POTFILES > >> index 32b975a04..67cdffb29 100644 > >> --- a/po/POTFILES > >> +++ b/po/POTFILES > >> @@ -329,7 +329,6 @@ lib/actions-6.c > >> lib/actions-support.c > >> lib/actions-variants.c > >> lib/alloc.c > >> -lib/appliance-cpu.c > >> lib/appliance-kcmdline.c > >> lib/appliance-uefi.c > >> lib/appliance.c > >> > > > > Acked-by: Laszlo Ersek <lersek at redhat.com> > > > > Was this patch forgotten? I don't see it in the commit history (by subject).No, but in hindsight I'm lukewarm about deleting lib/appliance-cpu.c. Turns out we needed another exception for RISC-V, so keeping this file around seems like a good idea. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v