diff mbox series

[RFC] libxl/dm: Stop using "xenfv" machine in the Qemu upstream device model

Message ID c2cb963b24db11a87f1899ce53fe32d0cda91261.1676917444.git.brchuckz@aol.com (mailing list archive)
State New, archived
Headers show
Series [RFC] libxl/dm: Stop using "xenfv" machine in the Qemu upstream device model | expand

Commit Message

Chuck Zmudzinski Feb. 20, 2023, 6:53 p.m. UTC
The "xenfv" machine is almost a clone of the "pc" machine with accel=xen
in Qemu upstream and is mostly redundant, so with this patch libxl
stops using the "xenfv" machine type in favor of the "pc" machine type
with accel=xen.

Looking at the Qemu upstream code in hw/i386/pc_piix.c, the obvious
differences between the "xenfv" machine and the "pc" machine are:
  - The "xenfv" machine unconditionally adds the xen-platform pci device.
  - The "xenfv" machine uses the igd-passthrough-i440FX host pci device
    instead of the i440FX pci device when the guest is configured with
    igd-passthru=on.

This patch adds the xen-platform pci device using a command line option
with the "pc" machine type instead of relying on upstream Qemu to add
it as part of the process of building the "xenfv" machine if the
administrator has not explicitly disabled the xen-platform pci device
in the domain configuration file. Therefore, the current behavior of
adding the xen-platform pci device unless the administrator configures
the guest with the xen_platform_pci option set to 0 is retained.

So this patch does not affect guest behavior except as follows:

  - If the guest is configured for Intel igd passthrough, the guest will
    no longer be configured with the igd-passthrough-i440FX host pci
    device. For this reason, Qemu upstream should be patched so the
    igd-passthrough-i440FX host pci device will be used when the guest
    is configured for igd-passthru.

  - Live migrations of guests configured to use the "xenfv" machine
    when they were created to a host that configures the device model
    to use the "pc" machine instead might be affected. For this reason,
    it might be appropriate to deprecate the "xenfv" machine in upstream
    Qemu and it might be necessary patch upstream Qemu to properly
    handle these live migrations.

  - Testing reveals the display resolution can be affected in some
    guests by the change from the "xenfv" machine to the "pc" machine
    but there is no degredation in performance, just a possible need
    to reset the display resolution to the desired value.

Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
---
Sorry about how long this discussion is, but it is helpful to again
bring together all the considerations about this patch and its
alternatives in one place.

I submit this as an RFC patch because some caveats might make this patch
more trouble than it is worth.

The first caveat is complications that might be involved for existing
guests that currently use the "xenfv" machine and are migrated to a host
with this patch that uses the "pc" machine instead.

The second caveat is the effect of the patch on display resolution in
the guest. Details of tests that reveal the display resolution can be
affected by this patch:

Testing existing guests that previously used the "xenfv" machine reveals
a subtle difference that is not obvious from reading the Qemu upstream
code in hw/i386/ppc_piix.c. The tested guests were configured to use the
Qemu stdvga (bochs) emulated video device with a vnc display. When the
guest is started using the "xenfv" machine, the resolution of the
display manager (either gdm3 or lightdm) is 1024x768, but when using
the "pc" machine with the xen accelerator, the resolution of the display
manager (either gdm3 or lightdm) is 1280x800. Also, the display
resolution of a user's graphical login session might be reset to a
default value when changing from the "xenfv" to "pc" machine type, but
the user can easily correct this by resetting the display resolution to
the desired value in the first graphical login session after changing
from the "xenfv" to the "pc" machine type.

On a more positive note, this patch will not introduce much of a
regression for users of the upstream Qemu igd-passthru feature because
that feature is already currently broken in upstream Qemu because
upstream Qemu currently fails to reserve pci slot 2 for the Intel IGD
and, in fact, with this patch, it is actually easier to patch upstream
Qemu to properly support the igd passthrough feature than it is
without this patch because, in that case, a more complicated alternative
patch to upstream Qemu that reserves pci slot 2 for the igd is required
to properly support the igd passthrough feature if we continue to use
the "xenfv" machine type:

http://patchew.org/QEMU/b1b4a21fe9a600b1322742dda55a40e9961daa57.1674346505.git.brchuckz@aol.com/

This alternative patch, however, has been reviewed and could be merged
into Qemu anytime at the discretion of the Qemu Xen maintainers. Some of
the "pc" machine maintainers might want an additional patch to Qemu
upstream to be applied to change how upstream Qemu's slot_reserved_mask
works if this alternative patch is merged into upstream Qemu.

A much simpler patch to Qemu upstream is available to fix support for
the igd passthrough feature in upstream Qemu for the "pc" machine type:

https://patchew.org/QEMU/a304213d26506b066021f803c39b87f6a262ed86.1675820085.git.brchuckz@aol.com/

This patch has not been fully reviewed yet, but it does have an Acked-by
and this is the patch that would be applied to upstream Qemu in
coordination with this patch to libxl to ensure the guest uses the
igd-passthrough-i440FX pci host device when igd-passthru=on. Also, to
better support the intel igd, additional patches to libxl could be
applied to more easily configure the guest properly for the igd.

So, do we want to completely stop using the "xenfv" machine as suggested
here:

https://lore.kernel.org/xen-devel/Y9EUarVVWr223API@perard.uk.xensource.com/

or, do we want to continue to use the "xenfv" machine and fix the
igd passthrough feature in Qemu with the more complicated patch to
Qemu that has been reviewed and, since it continues to use the "xenfv"
machine type, it also avoids the problem of how the fix might affect
existing guests that currently use the "xenfv" machine type?

And finally, there is a third option, which is to use this previously
proposed alternative patch series to libxl to fix igd passthrough support
by only changing guests configured for igd passthrough from the "xenfv"
to the "pc" machine type and leaving other existing guests unchanged:

https://lore.kernel.org/xen-devel/20230110073201.mdUvSjy1vKtxPriqMQuWAxIjQzf1eAqIlZgal1u3GBI@z/

which would avoid the complications of migrating other existing guests
to the "pc" machine type and also allow use of the simpler patch to Qemu
to fix support for igd passthrough in upstream Qemu.

This alternative patch series also has not been fully reviewed yet but
it might actually be the best approach overall after considering the
complications of stopping use of the "xenfv" machine type in all cases
instead of stopping its use only in the case of guests configured for
igd passthrough.

 tools/libs/light/libxl_dm.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
diff mbox series

Patch

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index fc264a3a13..9a3c27ae9c 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -1373,6 +1373,15 @@  static int libxl__build_device_model_args_new(libxl__gc *gc,
         if (b_info->cmdline)
             flexarray_vappend(dm_args, "-append", b_info->cmdline, NULL);
 
+        /*
+         * Since we are no longer using the "xenfv" machine, we don't
+         * rely on Qemu upstream to add the xen platform pci device
+         * and instead add it here as a command line argument.
+         */
+        if (libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
+            flexarray_append_pair(dm_args, "-device", "xen-platform");
+        }
+
         /* Find out early if one of the disk is on the scsi bus and add a scsi
          * controller. This is done ahead to keep the same behavior as previous
          * version of QEMU (have the controller on the same PCI slot). */
@@ -1809,14 +1818,7 @@  static int libxl__build_device_model_args_new(libxl__gc *gc,
             flexarray_append(dm_args, b_info->extra_pv[i]);
         break;
     case LIBXL_DOMAIN_TYPE_HVM:
-        if (!libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
-            /* Switching here to the machine "pc" which does not add
-             * the xen-platform device instead of the default "xenfv" machine.
-             */
-            machinearg = libxl__strdup(gc, "pc,accel=xen,suppress-vmdesc=on");
-        } else {
-            machinearg = libxl__strdup(gc, "xenfv,suppress-vmdesc=on");
-        }
+        machinearg = libxl__strdup(gc, "pc,accel=xen,suppress-vmdesc=on");
         if (b_info->u.hvm.mmio_hole_memkb) {
             uint64_t max_ram_below_4g = (1ULL << 32) -
                 (b_info->u.hvm.mmio_hole_memkb << 10);