Re: [PATCH v3 07/10] vfio/igd: Decouple common quirks from legacy mode

["Cédric Le Goater" <clg@redhat.com>]

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