On 07/20/22 13:51, Richard W.M. Jones wrote: > 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é <berra...@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 <ler...@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.
OK! Just wanted to reach closure on this thread, as I happened to look at CPU models for a different reason. Laszlo _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs