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.
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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
