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: 11324 bytes --]

On Tue, 2025-03-04 at 09:07 +0100, Corvin Köhne wrote:
> 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?
> 

Sry, missed that this sets VFIO_FEATURE_ENABLE_IGD_LEGACY_MODE_BIT which is
indeed used.

> >      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 --]