Re: [PATCH v2 7/9] vfio/igd: Decouple common quirks from legacy mode

["Corvin Köhne" <corvin.koehne@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10640 bytes --]

On Tue, 2025-03-04 at 01:52 +0800, Tomita Moeko wrote:
> So far, IGD-specific quirks all require enabling legacy mode, which is
> toggled by assigning IGD to 00:02.0. However, some quirks, like the BDSM
> and GGC register quirks, should be applied to all supported IGD devices.
> A new feature bit, VFIO_FEATURE_ENABLE_IGD_LEGACY_MODE, is introduced to
> control the legacy mode only quirks.
> 
> To maintain backward compatibilty, this bit is set by default, but
> legacy mode is only enabled when:
> - Machine type is i440fx
> - IGD device is at guest BDF 00:02.0
> - Not manually disabled by x-igd-legacy-mode=off
> 
> Note that QEMU will now fail immediately if any error occurs when
> setting up legacy mode, instead of simply continues like before. If
> legacy mode is unwanted, it can be explicitly disabled by
> x-igd-legacy-mode=off.
> 
> Additionally, the hotplug check in legacy mode is removed as it will be
> checked when enabling the OpRegion quirk.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 108 +++++++++++++++++++++++++-------------------------
>  hw/vfio/pci.c |   2 +
>  hw/vfio/pci.h |   3 ++
>  3 files changed, 59 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index f5e19f1241..40f5803be9 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -15,6 +15,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> +#include "hw/boards.h"
>  #include "hw/hw.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "pci.h"
> @@ -432,9 +433,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
> nr)
>       * bus address.
>       */
>      if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> -        !vfio_is_vga(vdev) || nr != 0 ||
> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
> -                                       0, PCI_DEVFN(0x2, 0))) {
> +        !vfio_is_vga(vdev) || nr != 0) {
>          return;
>      }
>  
> @@ -482,15 +481,12 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
> nr)
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
>  }
>  
> -bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
> -                                 Error **errp G_GNUC_UNUSED)
> +bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>  {
> -    g_autofree struct vfio_region_info *rom = NULL;
>      int ret, gen;
>      uint64_t gms_size;
>      uint64_t *bdsm_size;
>      uint32_t gmch;
> -    Error *err = NULL;
>  
>      /*
>       * This must be an Intel VGA device at address 00:02.0 for us to even
> @@ -498,9 +494,7 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>       * PCI bus address.
>       */
>      if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> -        !vfio_is_vga(vdev) ||
> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
> -                                       0, PCI_DEVFN(0x2, 0))) {
> +        !vfio_is_vga(vdev)) {
>          return true;
>      }
>  
> @@ -516,56 +510,62 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>          return true;
>      }
>  
> -    /*
> -     * Most of what we're doing here is to enable the ROM to run, so if
> -     * there's no ROM, there's no point in setting up this quirk.
> -     * NB. We only seem to get BIOS ROMs, so a UEFI VM would need CSM
> support.
> -     */
> -    ret = vfio_get_region_info(&vdev->vbasedev,
> -                               VFIO_PCI_ROM_REGION_INDEX, &rom);
> -    if ((ret || !rom->size) && !vdev->pdev.romfile) {
> -        error_report("IGD device %s has no ROM, legacy mode disabled",
> -                     vdev->vbasedev.name);
> -        return true;
> -    }
> -
> -    /*
> -     * Ignore the hotplug corner case, mark the ROM failed, we can't
> -     * create the devices we need for legacy mode in the hotplug scenario.
> -     */
> -    if (vdev->pdev.qdev.hotplugged) {
> -        error_report("IGD device %s hotplugged, ROM disabled, "
> -                     "legacy mode disabled", vdev->vbasedev.name);
> -        vdev->rom_read_failed = true;
> -        return true;
> -    }
> -
>      gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
>  
>      /*
> -     * If IGD VGA Disable is clear (expected) and VGA is not already enabled,
> -     * try to enable it.  Probably shouldn't be using legacy mode without
> VGA,
> -     * but also no point in us enabling VGA if disabled in hardware.
> +     * For backward compatibilty, enable legacy mode when
> +     * - Machine type is i440fx (pc_piix)
> +     * - IGD device is at guest BDF 00:02.0
> +     * - Not manually disabled by x-igd-legacy-mode=off
>       */
> -    if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
> -        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> -        error_report("IGD device %s failed to enable VGA access, "
> -                     "legacy mode disabled", vdev->vbasedev.name);
> -        return true;
> -    }
> +    if ((vdev->features & VFIO_FEATURE_ENABLE_IGD_LEGACY_MODE) &&
> +        !strcmp(MACHINE_GET_CLASS(qdev_get_machine())->family, "pc_piix") &&
> +        (&vdev->pdev == pci_find_device(pci_device_root_bus(&vdev->pdev),
> +        0, PCI_DEVFN(0x2, 0)))) {

Suggestion, refactor is like:

if (x-igd-legacy-mode = "off") {
  /* Legacy mode disabled by user */
  return
} else if (x-igd-legacy-mode = "auto") {
  if (strcmp(MACHINE_GET_CLASS(qdev_get_machine())->family, "pc_piix") || 
         (&vdev->pdev == pci_find_device(pci_device_root_bus(&vdev->pdev), 0,
         PCI_DEVFN(0x2, 0)))) {
    /* No legacy mode detected */
    return;
  }
}

/* Setup legacy mode */

This style would avoid intending this whole code block for setting up legacy
mode.

> +        /*
> +         * IGD legacy mode requires:
> +         * - VBIOS in ROM BAR or file
> +         * - VGA IO/MMIO ranges are claimed by IGD
> +         * - OpRegion
> +         * - Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
> +         */
> +        g_autofree struct vfio_region_info *rom = NULL;
> +
> +        warn_report("IGD legacy mode enabled, "
> +                    "use x-igd-legacy-mode=off to disable it if unwanted.");
> +
> +        /*
> +         * Most of what we're doing here is to enable the ROM to run, so if
> +         * there's no ROM, there's no point in setting up this quirk.
> +         * NB. We only seem to get BIOS ROMs, so UEFI VM would need CSM
> support.
> +         */
> +        ret = vfio_get_region_info(&vdev->vbasedev,
> +                                   VFIO_PCI_ROM_REGION_INDEX, &rom);
> +        if ((ret || !rom->size) && !vdev->pdev.romfile) {
> +            error_setg(errp, "Device has no ROM");
> +            return false;
> +        }
>  
> -    /* Setup OpRegion access */
> -    if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
> -        error_append_hint(&err, "IGD legacy mode disabled\n");
> -        error_report_err(err);
> -        return true;
> -    }
> +        /*
> +         * If IGD VGA Disable is clear (expected) and VGA is not already
> +         * enabled, try to enable it. Probably shouldn't be using legacy mode
> +         * without VGA, but also no point in us enabling VGA if disabled in
> +         * hardware.
> +         */
> +        if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, errp)) {
> +            error_setg(errp, "Unable to enable VGA access");
> +            return false;
> +        }
>  
> -    /* Setup LPC bridge / Host bridge PCI IDs */
> -    if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
> -        error_append_hint(&err, "IGD legacy mode disabled\n");
> -        error_report_err(err);
> -        return true;
> +        /* Setup OpRegion access */
> +        if (!vfio_pci_igd_setup_opregion(vdev, errp)) {
> +            return false;
> +        }
> +
> +        /* Setup LPC bridge / Host bridge PCI IDs */
> +        if (!vfio_pci_igd_setup_lpc_bridge(vdev, errp)) {
> +            return false;
> +        }
>      }
>  
>      /*
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index a58d555934..b0620a0ae8 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3363,6 +3363,8 @@ static const Property vfio_pci_dev_properties[] = {
>                      VFIO_FEATURE_ENABLE_REQ_BIT, true),
>      DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
>                      VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
> +    DEFINE_PROP_BIT("x-igd-legacy-mode", VFIOPCIDevice, features,
> +                    VFIO_FEATURE_ENABLE_IGD_LEGACY_MODE_BIT, true),

This property isn't used in this commit anywhere. What am I missing?

>      DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>                              vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 2e81f9eb19..b7b07644a8 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -154,6 +154,9 @@ struct VFIOPCIDevice {
>  #define VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT 2
>  #define VFIO_FEATURE_ENABLE_IGD_OPREGION \
>                                  (1 << VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT)
> +#define VFIO_FEATURE_ENABLE_IGD_LEGACY_MODE_BIT 4
> +#define VFIO_FEATURE_ENABLE_IGD_LEGACY_MODE \
> +                                (1 <<
> VFIO_FEATURE_ENABLE_IGD_LEGACY_MODE_BIT)
>      OnOffAuto display;
>      uint32_t display_xres;
>      uint32_t display_yres;


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]