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 <[email protected]>
>> ---
>> 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.
>
>
