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

[Tomita Moeko <tomitamoeko@gmail.com>]

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