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 <tomitamo...@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;
>
signature.asc
Description: This is a digitally signed message part
