Re: [PATCH v1 3/3] xen/pci: add discovered PCI device at boot

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.08.2025 13:09, Mykyta Poturai wrote:
> On 04.08.25 11:35, Jan Beulich wrote:
>> On 01.08.2025 11:22, Mykyta Poturai wrote:
>>> Create add_discovered_pci_devices function that calls pci_device_add
>>> on every PCI device discovered.
>>> The devices will be added to dom_io so that they can be assigned
>>> later to other domains.
>>
>> And why's the intermediate step necessary? IOW can't they be assigned to 
>> their
>> target domains right away, and only whatever's left would go to DOM_IO?
> 
> For Dom0less case, guest domains are not yet created at this point.

I understand that, but this doesn't answer my question. Once the guest domains
are there, can't you directly assign their devices to them?

>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1180,6 +1180,34 @@ int __init scan_pci_devices(void)
>>>       return ret;
>>>   }
>>>   
>>> +static int __init _add_discovered_pci_devices(struct pci_seg *pseg, void 
>>> *arg)
>>> +{
>>> +    struct pci_dev *pdev;
>>> +    int ret = 0;
>>> +
>>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>> +    {
>>> +        ret = pci_add_device(dom_io, pdev->seg, pdev->bus, pdev->devfn, 
>>> NULL,
>>> +                             NUMA_NO_NODE);
>>> +        if ( ret < 0 )
>>> +        {
>>> +            printk(XENLOG_ERR
>>> +                   "%pp: Failure adding the discovered pci device (Error 
>>> %d)\n",
>>> +                   &pdev->sbdf, ret);
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +void __init add_discovered_pci_devices(void)
>>> +{
>>> +    pcidevs_lock();
>>> +    pci_segments_iterate(_add_discovered_pci_devices, NULL);
>>> +    pcidevs_unlock();
>>> +}
>>
>> This looks to merely be a specialized form of what ...
>>
>>>   struct setup_hwdom {
>>>       struct domain *d;
>>>       int (*handler)(uint8_t devfn, struct pci_dev *pdev);
>>
>> ... follows below here. By generalizing what we have (perhaps from the top, 
>> i.e.
>> iommu_hwdom_init()), you'd also avoid violating Misra rule 2.1 on x86, as 
>> you add
>> unreachable code there.
> 
> Can you please elaborate a little further on how you see this 
> generalization? With routines you mentioned being specifically for 
> hwdom, which may not exist,

That's precisely the generalization I'm having in mind. What's hwdom-only
now ought to be possible to be made usable in wider manner.

> and the different approaches to PCI 
> initialization on Arm/x86 (as far as I understand on x86 all of the PCI 
> related stuff is initialized from iommu helpers, while Arm has a 
> dedicated init step)

This, too, may want harmonizing.

> I am afraid I can't find a nice point of contact to 
> generalize this, but I can be missing somthing due to unfamiliarity with 
> x86 code.
> 
> Another way of addressing possible MISRA violation I can see is moving 
> the add_discovered_pci_devices to xen/arch/arm/pci/pci.c so it will only 
> be compiled when used.

And pose a problem when e.g. RISC-V also wants the same.

Jan