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 <tomitamo...@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 <tomitamo...@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;
>>>
>>
>
