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.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/arch/arm/physdev.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
index e91355fe22..ccce8f0eba 100644
--- 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;
+    }
+#endif
+    default:
+        gdprintk(XENLOG_DEBUG, "PHYSDEVOP cmd=%d: not implemented\n", cmd);
+        ret = -ENOSYS;
+    }
+
+    return ret;
 }
 
 /*
-- 
2.17.1

(+ Jan)

Hi Rahul,

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.

Also, public/arch-arm.h will need to be updated as we now support the 
PHYSDEVOP hypercall.

> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>   xen/arch/arm/physdev.c | 39 ++++++++++++++++++++++++++++++++++++---
>   1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
> index e91355fe22..ccce8f0eba 100644
> --- 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;
> +    }

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.

> +#endif
> +    default:
> +        gdprintk(XENLOG_DEBUG, "PHYSDEVOP cmd=%d: not implemented\n", cmd);
> +        ret = -ENOSYS;
> +    }
> +
> +    return ret;
>   }
>   
>   /*
> 

Cheers,

-- 
Julien Grall

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

Hi Julien,

> On 19 Aug 2021, at 1:35 pm, Julien Grall <julien@xen.org> wrote:
> 
> (+ Jan)
> 
> Hi Rahul,
> 
> 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.

As per Jan comments I will add the PHYSDEVOP_pci_device_remove() in the next version.
> 
> Also, public/arch-arm.h will need to be updated as we now support the PHYSDEVOP hypercall.

Ok.
> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>>  xen/arch/arm/physdev.c | 39 ++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 36 insertions(+), 3 deletions(-)
>> diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
>> index e91355fe22..ccce8f0eba 100644
>> --- 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;
>> +    }
> 
> 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.

Ok. Let me move the PHYSDEVOP_pci_device_* to common code.

Regards,
Rahul