Re: [PATCH v2 3/5] vfio/igd: refactor vfio_probe_igd_bar4_quirk() into pci config quirk

[Tomita Moeko <tomitamoeko@gmail.com>]

On 1/31/25 17:14, Cédric Le Goater wrote:
> Hello Tomita,
> 
> On 1/24/25 20:12, Tomita Moeko wrote:
>> The actual IO BAR4 write quirk in vfio_probe_igd_bar4_quirk() was
>> removed in previous change, leaving the function not matching its name,
>> so move it into the newly introduced vfio_config_quirk_setup(). There
>> is no functional change in this commit. If any failure occurs, the
>> function simply returns and proceeds.
>>
>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>> ---
>>   hw/vfio/igd.c        | 31 +++++++++++++++++--------------
>>   hw/vfio/pci-quirks.c |  6 +++++-
>>   hw/vfio/pci.h        |  2 +-
>>   3 files changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index ca3a32f4f2..376a26fbae 100644
>> --- a/hw/vfio/igd.c
>> +++ b/hw/vfio/igd.c
>> @@ -359,7 +359,8 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>>       QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
>>   }
>>   -void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>> +bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>> +                                 Error **errp G_GNUC_UNUSED)
> 
> Adding an 'Error **' parameter is in improvement indeed. All the error_report
> of this routine need to be converted too.

Sorry I missed this mail previously.

Originally this function simply return and continue on error, I prefer
keeping current behavior until legacy mode was finally removed, as I
described in my reply to Alex. At that point, this function will
terminate and report on error.
 
>>   {
>>       g_autofree struct vfio_region_info *rom = NULL;
>>       g_autofree struct vfio_region_info *opregion = NULL;
>> @@ -378,10 +379,10 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>        * PCI bus address.
>>        */
>>       if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>> -        !vfio_is_vga(vdev) || nr != 4 ||
>> +        !vfio_is_vga(vdev) ||
>>           &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
>>                                          0, PCI_DEVFN(0x2, 0))) {
>> -        return;
>> +        return true;
>>       }
>>         /*
>> @@ -395,7 +396,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>                                              "vfio-pci-igd-lpc-bridge")) {
>>           error_report("IGD device %s cannot support legacy mode due to existing "
>>                        "devices at address 1f.0", vdev->vbasedev.name);
>> -        return;
>> +        return true;
> 
> if there is an error_report, why is this returning true ? It should be false.

Please see my comment above
  
>>       }
>>         /*
>> @@ -407,7 +408,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>       if (gen == -1) {
>>           error_report("IGD device %s is unsupported in legacy mode, "
>>                        "try SandyBridge or newer", vdev->vbasedev.name);
>> -        return;
>> +        return true;
> 
> same here and else where.
> 
>>       }
>>         /*
>> @@ -420,7 +421,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>       if ((ret || !rom->size) && !vdev->pdev.romfile) {
>>           error_report("IGD device %s has no ROM, legacy mode disabled",
>>                        vdev->vbasedev.name);
>> -        return;
>> +        return true;
>>       }
>>         /*
>> @@ -431,7 +432,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>           error_report("IGD device %s hotplugged, ROM disabled, "
>>                        "legacy mode disabled", vdev->vbasedev.name);
>>           vdev->rom_read_failed = true;
>> -        return;
>> +        return true;
>>       }
>>         /*
>> @@ -444,7 +445,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>       if (ret) {
>>           error_report("IGD device %s does not support OpRegion access,"
>>                        "legacy mode disabled", vdev->vbasedev.name);
>> -        return;
>> +        return true;
>>       }
>>         ret = vfio_get_dev_region_info(&vdev->vbasedev,
>> @@ -453,7 +454,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>       if (ret) {
>>           error_report("IGD device %s does not support host bridge access,"
>>                        "legacy mode disabled", vdev->vbasedev.name);
>> -        return;
>> +        return true;
>>       }
>>         ret = vfio_get_dev_region_info(&vdev->vbasedev,
>> @@ -462,7 +463,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>       if (ret) {
>>           error_report("IGD device %s does not support LPC bridge access,"
>>                        "legacy mode disabled", vdev->vbasedev.name);
>> -        return;
>> +        return true;
>>       }
>>         gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
>> @@ -476,7 +477,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>           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;
>> +        return true;
>>       }
>>         /* Create our LPC/ISA bridge */
>> @@ -484,7 +485,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>       if (ret) {
>>           error_report("IGD device %s failed to create LPC bridge, "
>>                        "legacy mode disabled", vdev->vbasedev.name);
>> -        return;
>> +        return true;
>>       }
>>         /* Stuff some host values into the VM PCI host bridge */
>> @@ -492,14 +493,14 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>       if (ret) {
>>           error_report("IGD device %s failed to modify host bridge, "
>>                        "legacy mode disabled", vdev->vbasedev.name);
>> -        return;
>> +        return true;
>>       }
>>         /* Setup OpRegion access */
>>       if (!vfio_pci_igd_opregion_init(vdev, opregion, &err)) {
>>           error_append_hint(&err, "IGD legacy mode disabled\n");
>>           error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> -        return;
>> +        return true;
>>       }
>>         /*
>> @@ -561,4 +562,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>         trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name,
>>                                       (ggms_size + gms_size) / MiB);
>> +
>> +    return true;
>>   }
>> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
>> index c40e3ca88f..b8379cb512 100644
>> --- a/hw/vfio/pci-quirks.c
>> +++ b/hw/vfio/pci-quirks.c
>> @@ -1169,6 +1169,11 @@ bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
>>    */
>>   bool vfio_config_quirk_setup(VFIOPCIDevice *vdev, Error **errp)
>>   {
>> +#ifdef CONFIG_VFIO_IGD
> 
> oh. We try to avoid such ifdef in QEMU. Only very specific and high level
> configs are discarded at compile time (KVM, LINUX, etc).

This ifdef has been in QEMU code, for only building igd-related code
into x86 target (VFIO_IGD is only seleted when PC_PCI=on, and I440FX/
Q35 selests PC_PCI). IMO using the ifdef here is reasonable, as IGD
quirks is a part of vfio-pci, but using QOM in furure would be a
good direction.

> One way to adress this case, would be to use QOM. Example below :
> 
>  
> Declare a base class :
>         #define TYPE_VFIO_PCI_QUIRK_PROVIDER "vfio-pci-quirk-provider"
>         OBJECT_DECLARE_TYPE(VFIOPCIQuirkProvider, VFIOPCIQuirkProviderClass,
>                         VFIO_PCI_QUIRK_PROVIDER)
>         struct VFIOPCIQuirkProviderClass {
>         ObjectClass parent;
>             bool (*probe)(VFIOPCIDevice *vdev, Error **errp);
>         bool (*setup)(VFIOPCIDevice *vdev, Error **errp);
>     };
>         static const TypeInfo vfio_pci_quirk_info = {
>         .name = TYPE_VFIO_PCI_QUIRK_PROVIDER,
>         .parent = TYPE_OBJECT,
>         .class_size = sizeof(VFIOPCIQuirkClass),
>         .abstract = true,
>     };
>         static void register_vfio_pci_quirk_type(void)
>     {
>         type_register_static(&vfio_pci_quirk_info);
>     }
>         type_init(register_vfio_pci_quirk_type)
> 
> 
> Declare one for IGD
>         static void vfio_pci_quirk_igd_class_init(ObjectClass *klass, void *data)
>     {
>         VFIOPCIQuirkClass* vpqc = VFIO_PCI_QUIRK_PROVIDER_CLASS(klass);
>             vpqc->setup = vfio_probe_igd_quirk_probe;
>         vpqc->probe = vfio_probe_igd_quirk_probe;
>     }
>         static const TypeInfo vfio_pci_quirk_igd_info = {
>         .name = TYPE_VFIO_PCI_QUIRK_PROVIDER "-igd",
>         .parent = TYPE_VFIO_PCI_QUIRK_PROVIDER,
>         .class_init = vfio_pci_quirk_igd_class_init,
>         .class_size = sizeof(VFIOPCIQuirkClass),
>     };
>         static void vfio_pci_quirk_igd_register_types(void)
>     {
>         type_register_static(&vfio_pci_quirk_igd_info);
>     }
>         type_init(vfio_pci_quirk_igd_register_types)
> 
> 
> and in the common part, loop on all the classes to probe and setup :
> 
> 
>     static void vfio_pci_quirk_class_foreach(ObjectClass *klass, void *opaque)
>     {
>         VFIOPCIQuirkProviderClass* vpqc = VFIO_PCI_QUIRK_PROVIDER_CLASS(klass);
>         Error *local_err = NULL;
>             vpqc->setup(opaque, &local_err);
>     }
>         bool vfio_config_quirk_setup(VFIOPCIDevice *vdev, Error **errp)
>     {
>         object_class_foreach(vfio_pci_quirk_class_foreach,
>                              TYPE_VFIO_PCI_QUIRK_PROVIDER, false, vdev);
>        ...
> 
> 
> 
> 
> Thanks,
> 
> C.
> 
>