Re: [PATCH v1 10/14] xen/arm: Discovering PCI devices and add the PCI devices in XEN.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2021 14:35, Julien Grall wrote:
> On 19/08/2021 13:02, Rahul Singh wrote:
>> Hardware domain is in charge of doing the PCI enumeration and will
>> discover the PCI devices and then will communicate to XEN via hyper
>> call PHYSDEVOP_pci_device_add to add the PCI devices in XEN.
> 
> There are other PHYSDEVOP operations to add PCI devices. I think it is 
> fine to only implement the latest (CC Jan for some opinion and confirm 
> this is the latest). However, this ought to be explained in the commit 
> message.

I don't think "latest" matters much here. Considering there was no
physdevop support at all on Arm, enabling whichever set seems like
a good fit would be okay.

Having written this I realize that by "latest" you may mean whether
the used sub-ops have not been obsoleted by newer ones (rather than
the last ones that were added to the physdevops set). While indeed
PHYSDEVOP_pci_device_add hasn't been superseded so far, I have a
vague recollection of there being some missing part. Without me
remembering details I'm afraid using what is there is the best we
can do for for the moment. However, ...

>> --- a/xen/arch/arm/physdev.c
>> +++ b/xen/arch/arm/physdev.c
>> @@ -9,12 +9,45 @@
>>   #include <xen/errno.h>
>>   #include <xen/sched.h>
>>   #include <asm/hypercall.h>
>> -
>> +#include <xen/guest_access.h>
>> +#include <xsm/xsm.h>
>>   
>>   int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>   {
>> -    gdprintk(XENLOG_DEBUG, "PHYSDEVOP cmd=%d: not implemented\n", cmd);
>> -    return -ENOSYS;
>> +    int ret = 0;
>> +
>> +    switch ( cmd )
>> +    {
>> +#ifdef CONFIG_HAS_PCI
>> +    case PHYSDEVOP_pci_device_add: {
>> +        struct physdev_pci_device_add add;
>> +        struct pci_dev_info pdev_info;
>> +        nodeid_t node = NUMA_NO_NODE;
>> +
>> +        ret = -EFAULT;
>> +        if ( copy_from_guest(&add, arg, 1) != 0 )
>> +            break;
>> +
>> +        pdev_info.is_extfn = !!(add.flags & XEN_PCI_DEV_EXTFN);
>> +        if ( add.flags & XEN_PCI_DEV_VIRTFN )
>> +        {
>> +            pdev_info.is_virtfn = 1;
>> +            pdev_info.physfn.bus = add.physfn.bus;
>> +            pdev_info.physfn.devfn = add.physfn.devfn;
>> +        }
>> +        else
>> +            pdev_info.is_virtfn = 0;
>> +
>> +        ret = pci_add_device(add.seg, add.bus, add.devfn, &pdev_info, node);
>> +        break;
>> +    }

... I don't think it should be only "add" which gets supported. "remove"
exists not just for the purpose of hot-unplug, but also for Dom0 to
remove (and then re-add) devices after e.g. bus re-numbering. (There are
some gaps there iirc, but still ...)

> This is pretty much a copy of the x86 version without the NUMA bit. So I 
> think we want to move the implementation in common code.

+1 (if sensibly possible)

Jan