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 config option, x-igd-legacy-mode=[on|off|auto], is introduced to
control the legacy mode only quirks. The default value is "auto", which
keeps current behavior that enables legacy mode implicitly and continues
on error when all following conditions are met.
* Machine type is i440fx
* IGD device is at guest BDF 00:02.0
If any one of the conditions above is not met, the default behavior is
equivalent to "off", QEMU will fail immediately if any error occurs.
Users can also use "on" to force enabling legacy mode. It checks if all
the conditions above are met and set up legacy mode. QEMU will also fail
immediately on error in this case.
Additionally, the hotplug check in legacy mode is removed as hotplugging
IGD device is never supported, and it will be checked when enabling the
OpRegion quirk.
Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
hw/vfio/pci.c | 2 +
hw/vfio/pci.h | 1 +
3 files changed, 77 insertions(+), 53 deletions(-)
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index f5e19f1241..ac096e2eb5 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,14 +481,13 @@ 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;
+ bool legacy_mode_enabled = false;
Error *err = NULL;
/*
@@ -498,9 +496,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 +512,67 @@ 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->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
+ !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)))) {
+ /*
+ * 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;
+
+ legacy_mode_enabled = true;
+ 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(&err, "Device has no ROM");
+ goto error;
+ }
- /* 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, &err)) {
+ error_setg(&err, "Unable to enable VGA access");
+ goto error;
+ }
- /* 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, &err)) {
+ goto error;
+ }
+
+ /* Setup LPC bridge / Host bridge PCI IDs */
+ if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
+ goto error;
+ }
+ } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
+ error_setg(&err,
+ "Machine is not i440fx or assigned BDF is not 00:02.0");
+ goto error;
}
/*
@@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
return true;
+
+error:
+ /*
+ * When legacy mode is implicity enabled, continue on error,
+ * to keep compatibility
+ */
+ if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
+ error_report_err(err);
+ error_report("IGD legacy mode disabled");
+ return true;
+ }
+
+ error_propagate(errp, err);
+ return false;
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a58d555934..d5ff8c1ea8 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_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
+ igd_legacy_mode, ON_OFF_AUTO_AUTO),
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..aa67ceb346 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -158,6 +158,7 @@ struct VFIOPCIDevice {
uint32_t display_xres;
uint32_t display_yres;
int32_t bootindex;
+ OnOffAuto igd_legacy_mode;
uint32_t igd_gms;
OffAutoPCIBAR msix_relo;
uint8_t pm_cap;
--
2.47.2On Fri, 7 Mar 2025 02:01:27 +0800
Tomita Moeko <tomitamoeko@gmail.com> 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 config option, x-igd-legacy-mode=[on|off|auto], is introduced to
> control the legacy mode only quirks. The default value is "auto", which
> keeps current behavior that enables legacy mode implicitly and continues
> on error when all following conditions are met.
> * Machine type is i440fx
> * IGD device is at guest BDF 00:02.0
>
> If any one of the conditions above is not met, the default behavior is
> equivalent to "off", QEMU will fail immediately if any error occurs.
>
> Users can also use "on" to force enabling legacy mode. It checks if all
> the conditions above are met and set up legacy mode. QEMU will also fail
> immediately on error in this case.
>
> Additionally, the hotplug check in legacy mode is removed as hotplugging
> IGD device is never supported, and it will be checked when enabling the
> OpRegion quirk.
>
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
> hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
> hw/vfio/pci.c | 2 +
> hw/vfio/pci.h | 1 +
> 3 files changed, 77 insertions(+), 53 deletions(-)
>
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index f5e19f1241..ac096e2eb5 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,14 +481,13 @@ 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;
> + bool legacy_mode_enabled = false;
> Error *err = NULL;
>
> /*
> @@ -498,9 +496,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 +512,67 @@ 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->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
> + !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)))) {
> + /*
> + * 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;
> +
> + legacy_mode_enabled = true;
> + warn_report("IGD legacy mode enabled, "
> + "use x-igd-legacy-mode=off to disable it if unwanted.");
info_report() maybe? These aren't great choices for a user, get a
warning for using the intended mode or silence that warning by
requiring experimental options that taint the VM relative to libvirt.
Also, why do we list all the requirements then only test a few of them
before declaring we're using legacy mode? It seems like the above
should be moved to the end of this branch.
Given the pending release deadline, maybe these aren't blockers, but
let's follow-up with something that assumes the user wants something
other than what they're doing. Thanks,
Alex
> +
> + /*
> + * 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(&err, "Device has no ROM");
> + goto error;
> + }
>
> - /* 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, &err)) {
> + error_setg(&err, "Unable to enable VGA access");
> + goto error;
> + }
>
> - /* 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, &err)) {
> + goto error;
> + }
> +
> + /* Setup LPC bridge / Host bridge PCI IDs */
> + if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
> + goto error;
> + }
> + } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
> + error_setg(&err,
> + "Machine is not i440fx or assigned BDF is not 00:02.0");
> + goto error;
> }
>
> /*
> @@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
> trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
>
> return true;
> +
> +error:
> + /*
> + * When legacy mode is implicity enabled, continue on error,
> + * to keep compatibility
> + */
> + if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
> + error_report_err(err);
> + error_report("IGD legacy mode disabled");
> + return true;
> + }
> +
> + error_propagate(errp, err);
> + return false;
> }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index a58d555934..d5ff8c1ea8 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_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
> + igd_legacy_mode, ON_OFF_AUTO_AUTO),
> 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..aa67ceb346 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -158,6 +158,7 @@ struct VFIOPCIDevice {
> uint32_t display_xres;
> uint32_t display_yres;
> int32_t bootindex;
> + OnOffAuto igd_legacy_mode;
> uint32_t igd_gms;
> OffAutoPCIBAR msix_relo;
> uint8_t pm_cap;
On 2025/3/7 6:49, Alex Williamson wrote:
> On Fri, 7 Mar 2025 02:01:27 +0800
> Tomita Moeko <tomitamoeko@gmail.com> 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 config option, x-igd-legacy-mode=[on|off|auto], is introduced to
>> control the legacy mode only quirks. The default value is "auto", which
>> keeps current behavior that enables legacy mode implicitly and continues
>> on error when all following conditions are met.
>> * Machine type is i440fx
>> * IGD device is at guest BDF 00:02.0
>>
>> If any one of the conditions above is not met, the default behavior is
>> equivalent to "off", QEMU will fail immediately if any error occurs.
>>
>> Users can also use "on" to force enabling legacy mode. It checks if all
>> the conditions above are met and set up legacy mode. QEMU will also fail
>> immediately on error in this case.
>>
>> Additionally, the hotplug check in legacy mode is removed as hotplugging
>> IGD device is never supported, and it will be checked when enabling the
>> OpRegion quirk.
>>
>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>> ---
>> hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
>> hw/vfio/pci.c | 2 +
>> hw/vfio/pci.h | 1 +
>> 3 files changed, 77 insertions(+), 53 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index f5e19f1241..ac096e2eb5 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,14 +481,13 @@ 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;
>> + bool legacy_mode_enabled = false;
>> Error *err = NULL;
>>
>> /*
>> @@ -498,9 +496,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 +512,67 @@ 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->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
>> + !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)))) {
>> + /*
>> + * 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;
>> +
>> + legacy_mode_enabled = true;
>> + warn_report("IGD legacy mode enabled, "
>> + "use x-igd-legacy-mode=off to disable it if unwanted.");
>
> info_report() maybe? These aren't great choices for a user, get a
> warning for using the intended mode or silence that warning by
> requiring experimental options that taint the VM relative to libvirt.
Got it. If possible, please help change this to info_report when
applying.
> Also, why do we list all the requirements then only test a few of them
> before declaring we're using legacy mode? It seems like the above
> should be moved to the end of this branch.
Having i440fx machine type and BDF 00:02.0 are the must-have condition
for considering enabling legacy mode, while others are the requirements
for legacy mode itself. In this verison, legacy mode is equivalent to
romfile=oprom.bin,x-vga=on,x-igd-opregion=on,x-igd-lpc=on
BDF being 00:02.0 is a requirement of VBIOS, and hacking the LPC DID
currently only works on i440fx. Though for Q35, we can overwrite the
existing ICH9 LPC DID to make IGD driver happy, it won't break ACPI PM
in recent guest kernel, and maybe later making it an option for Q35,
its still too risky to make it an implicit default. That's why I prefer
to check the must-have condtions first, then check other requirements
when setting up them.
Setting the `legacy_mode_enabled` flag here is for error handling, as
we have to keep the old "continue on error" behavior for legacy mode.
> Given the pending release deadline, maybe these aren't blockers, but
> let's follow-up with something that assumes the user wants something
> other than what they're doing. Thanks,
>
> Alex
>
Sure.
Moeko
>> +
>> + /*
>> + * 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(&err, "Device has no ROM");
>> + goto error;
>> + }
>>
>> - /* 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, &err)) {
>> + error_setg(&err, "Unable to enable VGA access");
>> + goto error;
>> + }
>>
>> - /* 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, &err)) {
>> + goto error;
>> + }
>> +
>> + /* Setup LPC bridge / Host bridge PCI IDs */
>> + if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
>> + goto error;
>> + }
>> + } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
>> + error_setg(&err,
>> + "Machine is not i440fx or assigned BDF is not 00:02.0");
>> + goto error;
>> }
>>
>> /*
>> @@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>> trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
>>
>> return true;
>> +
>> +error:
>> + /*
>> + * When legacy mode is implicity enabled, continue on error,
>> + * to keep compatibility
>> + */
>> + if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
>> + error_report_err(err);
>> + error_report("IGD legacy mode disabled");
>> + return true;
>> + }
>> +
>> + error_propagate(errp, err);
>> + return false;
>> }
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index a58d555934..d5ff8c1ea8 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_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
>> + igd_legacy_mode, ON_OFF_AUTO_AUTO),
>> 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..aa67ceb346 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -158,6 +158,7 @@ struct VFIOPCIDevice {
>> uint32_t display_xres;
>> uint32_t display_yres;
>> int32_t bootindex;
>> + OnOffAuto igd_legacy_mode;
>> uint32_t igd_gms;
>> OffAutoPCIBAR msix_relo;
>> uint8_t pm_cap;
>
Tomita,
On 3/7/25 19:37, Tomita Moeko wrote:
> On 2025/3/7 6:49, Alex Williamson wrote:
>> On Fri, 7 Mar 2025 02:01:27 +0800
>> Tomita Moeko <tomitamoeko@gmail.com> 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 config option, x-igd-legacy-mode=[on|off|auto], is introduced to
>>> control the legacy mode only quirks. The default value is "auto", which
>>> keeps current behavior that enables legacy mode implicitly and continues
>>> on error when all following conditions are met.
>>> * Machine type is i440fx
>>> * IGD device is at guest BDF 00:02.0
>>>
>>> If any one of the conditions above is not met, the default behavior is
>>> equivalent to "off", QEMU will fail immediately if any error occurs.
>>>
>>> Users can also use "on" to force enabling legacy mode. It checks if all
>>> the conditions above are met and set up legacy mode. QEMU will also fail
>>> immediately on error in this case.
>>>
>>> Additionally, the hotplug check in legacy mode is removed as hotplugging
>>> IGD device is never supported, and it will be checked when enabling the
>>> OpRegion quirk.
>>>
>>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>>> ---
>>> hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
>>> hw/vfio/pci.c | 2 +
>>> hw/vfio/pci.h | 1 +
>>> 3 files changed, 77 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>>> index f5e19f1241..ac096e2eb5 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,14 +481,13 @@ 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;
>>> + bool legacy_mode_enabled = false;
>>> Error *err = NULL;
>>>
>>> /*
>>> @@ -498,9 +496,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 +512,67 @@ 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->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
>>> + !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)))) {
>>> + /*
>>> + * 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;
>>> +
>>> + legacy_mode_enabled = true;
>>> + warn_report("IGD legacy mode enabled, "
>>> + "use x-igd-legacy-mode=off to disable it if unwanted.");
>>
>> info_report() maybe? These aren't great choices for a user, get a
>> warning for using the intended mode or silence that warning by
>> requiring experimental options that taint the VM relative to libvirt.
>
> Got it. If possible, please help change this to info_report when
> applying.
done.
>
>> Also, why do we list all the requirements then only test a few of them
>> before declaring we're using legacy mode? It seems like the above
>> should be moved to the end of this branch.
>
> Having i440fx machine type and BDF 00:02.0 are the must-have condition
> for considering enabling legacy mode, while others are the requirements
> for legacy mode itself. In this verison, legacy mode is equivalent to
> romfile=oprom.bin,x-vga=on,x-igd-opregion=on,x-igd-lpc=on
>
> BDF being 00:02.0 is a requirement of VBIOS, and hacking the LPC DID
> currently only works on i440fx. Though for Q35, we can overwrite the
> existing ICH9 LPC DID to make IGD driver happy, it won't break ACPI PM
> in recent guest kernel, and maybe later making it an option for Q35,
> its still too risky to make it an implicit default. That's why I prefer
> to check the must-have condtions first, then check other requirements
> when setting up them.
>
> Setting the `legacy_mode_enabled` flag here is for error handling, as
> we have to keep the old "continue on error" behavior for legacy mode.
A ducomentation update is welcome ! Could you do that before hard-freeze ?
Thanks,
C.
>> Given the pending release deadline, maybe these aren't blockers, but
>> let's follow-up with something that assumes the user wants something
>> other than what they're doing. Thanks,
>>
>> Alex
>>
>
> Sure.
>
> Moeko
>
>>> +
>>> + /*
>>> + * 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(&err, "Device has no ROM");
>>> + goto error;
>>> + }
>>>
>>> - /* 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, &err)) {
>>> + error_setg(&err, "Unable to enable VGA access");
>>> + goto error;
>>> + }
>>>
>>> - /* 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, &err)) {
>>> + goto error;
>>> + }
>>> +
>>> + /* Setup LPC bridge / Host bridge PCI IDs */
>>> + if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
>>> + goto error;
>>> + }
>>> + } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
>>> + error_setg(&err,
>>> + "Machine is not i440fx or assigned BDF is not 00:02.0");
>>> + goto error;
>>> }
>>>
>>> /*
>>> @@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>> trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
>>>
>>> return true;
>>> +
>>> +error:
>>> + /*
>>> + * When legacy mode is implicity enabled, continue on error,
>>> + * to keep compatibility
>>> + */
>>> + if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
>>> + error_report_err(err);
>>> + error_report("IGD legacy mode disabled");
>>> + return true;
>>> + }
>>> +
>>> + error_propagate(errp, err);
>>> + return false;
>>> }
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index a58d555934..d5ff8c1ea8 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_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
>>> + igd_legacy_mode, ON_OFF_AUTO_AUTO),
>>> 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..aa67ceb346 100644
>>> --- a/hw/vfio/pci.h
>>> +++ b/hw/vfio/pci.h
>>> @@ -158,6 +158,7 @@ struct VFIOPCIDevice {
>>> uint32_t display_xres;
>>> uint32_t display_yres;
>>> int32_t bootindex;
>>> + OnOffAuto igd_legacy_mode;
>>> uint32_t igd_gms;
>>> OffAutoPCIBAR msix_relo;
>>> uint8_t pm_cap;
>>
>
On 2025/3/10 15:13, Cédric Le Goater wrote:
> Tomita,
>
> On 3/7/25 19:37, Tomita Moeko wrote:
>> On 2025/3/7 6:49, Alex Williamson wrote:
>>> On Fri, 7 Mar 2025 02:01:27 +0800
>>> Tomita Moeko <tomitamoeko@gmail.com> 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 config option, x-igd-legacy-mode=[on|off|auto], is introduced to
>>>> control the legacy mode only quirks. The default value is "auto", which
>>>> keeps current behavior that enables legacy mode implicitly and continues
>>>> on error when all following conditions are met.
>>>> * Machine type is i440fx
>>>> * IGD device is at guest BDF 00:02.0
>>>>
>>>> If any one of the conditions above is not met, the default behavior is
>>>> equivalent to "off", QEMU will fail immediately if any error occurs.
>>>>
>>>> Users can also use "on" to force enabling legacy mode. It checks if all
>>>> the conditions above are met and set up legacy mode. QEMU will also fail
>>>> immediately on error in this case.
>>>>
>>>> Additionally, the hotplug check in legacy mode is removed as hotplugging
>>>> IGD device is never supported, and it will be checked when enabling the
>>>> OpRegion quirk.
>>>>
>>>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>>>> ---
>>>> hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
>>>> hw/vfio/pci.c | 2 +
>>>> hw/vfio/pci.h | 1 +
>>>> 3 files changed, 77 insertions(+), 53 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>>>> index f5e19f1241..ac096e2eb5 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,14 +481,13 @@ 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;
>>>> + bool legacy_mode_enabled = false;
>>>> Error *err = NULL;
>>>> /*
>>>> @@ -498,9 +496,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 +512,67 @@ 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->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
>>>> + !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)))) {
>>>> + /*
>>>> + * 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;
>>>> +
>>>> + legacy_mode_enabled = true;
>>>> + warn_report("IGD legacy mode enabled, "
>>>> + "use x-igd-legacy-mode=off to disable it if unwanted.");
>>>
>>> info_report() maybe? These aren't great choices for a user, get a
>>> warning for using the intended mode or silence that warning by
>>> requiring experimental options that taint the VM relative to libvirt.
>>
>> Got it. If possible, please help change this to info_report when
>> applying.
>
> done.
>
>
>>
>>> Also, why do we list all the requirements then only test a few of them
>>> before declaring we're using legacy mode? It seems like the above
>>> should be moved to the end of this branch.
>>
>> Having i440fx machine type and BDF 00:02.0 are the must-have condition
>> for considering enabling legacy mode, while others are the requirements
>> for legacy mode itself. In this verison, legacy mode is equivalent to
>> romfile=oprom.bin,x-vga=on,x-igd-opregion=on,x-igd-lpc=on
>>
>> BDF being 00:02.0 is a requirement of VBIOS, and hacking the LPC DID
>> currently only works on i440fx. Though for Q35, we can overwrite the
>> existing ICH9 LPC DID to make IGD driver happy, it won't break ACPI PM
>> in recent guest kernel, and maybe later making it an option for Q35,
>> its still too risky to make it an implicit default. That's why I prefer
>> to check the must-have condtions first, then check other requirements
>> when setting up them.
>>
>> Setting the `legacy_mode_enabled` flag here is for error handling, as
>> we have to keep the old "continue on error" behavior for legacy mode.
>
> A ducomentation update is welcome ! Could you do that before hard-freeze ?
>
> Thanks,
>
> C.
>
Let me try to make it before Mar 18.
Thanks,
Moeko
>>> Given the pending release deadline, maybe these aren't blockers, but
>>> let's follow-up with something that assumes the user wants something
>>> other than what they're doing. Thanks,
>>>
>>> Alex
>>>
>>
>> Sure.
>>
>> Moeko
>>
>>>> +
>>>> + /*
>>>> + * 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(&err, "Device has no ROM");
>>>> + goto error;
>>>> + }
>>>> - /* 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, &err)) {
>>>> + error_setg(&err, "Unable to enable VGA access");
>>>> + goto error;
>>>> + }
>>>> - /* 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, &err)) {
>>>> + goto error;
>>>> + }
>>>> +
>>>> + /* Setup LPC bridge / Host bridge PCI IDs */
>>>> + if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
>>>> + goto error;
>>>> + }
>>>> + } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
>>>> + error_setg(&err,
>>>> + "Machine is not i440fx or assigned BDF is not 00:02.0");
>>>> + goto error;
>>>> }
>>>> /*
>>>> @@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>>> trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
>>>> return true;
>>>> +
>>>> +error:
>>>> + /*
>>>> + * When legacy mode is implicity enabled, continue on error,
>>>> + * to keep compatibility
>>>> + */
>>>> + if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
>>>> + error_report_err(err);
>>>> + error_report("IGD legacy mode disabled");
>>>> + return true;
>>>> + }
>>>> +
>>>> + error_propagate(errp, err);
>>>> + return false;
>>>> }
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index a58d555934..d5ff8c1ea8 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_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
>>>> + igd_legacy_mode, ON_OFF_AUTO_AUTO),
>>>> 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..aa67ceb346 100644
>>>> --- a/hw/vfio/pci.h
>>>> +++ b/hw/vfio/pci.h
>>>> @@ -158,6 +158,7 @@ struct VFIOPCIDevice {
>>>> uint32_t display_xres;
>>>> uint32_t display_yres;
>>>> int32_t bootindex;
>>>> + OnOffAuto igd_legacy_mode;
>>>> uint32_t igd_gms;
>>>> OffAutoPCIBAR msix_relo;
>>>> uint8_t pm_cap;
>>>
>>
>
© 2016 - 2025 Red Hat, Inc.