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