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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt;

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(a)redhat.com&gt; >>> --- >>> 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(a)redhat.com&gt; >> > > 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.