Re: [PATCH v4 03/14] xen/arm: Add PHYSDEVOP_pci_device_(*add/remove) support for ARM

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stefano,

> On 5 Oct 2021, at 12:46 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Mon, 4 Oct 2021, 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.
>> 
>> Also implement PHYSDEVOP_pci_device_remove(..) to remove the PCI device.
>> 
>> As most of the code for PHYSDEVOP_pci_device_* is the same between x86
>> and ARM, move the code to a common file to avoid duplication.
>> 
>> There are other PHYSDEVOP_pci_device_* operations to add PCI devices.
>> Currently implemented PHYSDEVOP_pci_device_remove(..) and
>> PHYSDEVOP_pci_device_add(..) only as those are minimum required to
>> support PCI passthrough on ARM.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>> Change in v4:
>> - Move file commom/physdev.c to drivers/pci/physdev.c
>> - minor comments.
>> Change in v3: Fixed minor comment.
>> Change in v2:
>> - Add support for PHYSDEVOP_pci_device_remove()
>> - Move code to common code
>> ---
>> ---
>> xen/arch/arm/physdev.c        |  5 +--
>> xen/arch/x86/physdev.c        | 52 +----------------------
>> xen/arch/x86/x86_64/physdev.c |  2 +-
>> xen/drivers/pci/Makefile      |  1 +
>> xen/drivers/pci/physdev.c     | 80 +++++++++++++++++++++++++++++++++++
>> xen/include/public/arch-arm.h |  4 +-
>> xen/include/xen/hypercall.h   | 11 +++++
>> 7 files changed, 100 insertions(+), 55 deletions(-)
>> create mode 100644 xen/drivers/pci/physdev.c
>> 
>> diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
>> index e91355fe22..d766978629 100644
>> --- a/xen/arch/arm/physdev.c
>> +++ b/xen/arch/arm/physdev.c
>> @@ -8,13 +8,12 @@
>> #include <xen/lib.h>
>> #include <xen/errno.h>
>> #include <xen/sched.h>
>> -#include <asm/hypercall.h>
>> +#include <xen/hypercall.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;
>> +    return pci_physdev_op(cmd, arg);
>> }
>> 
>> /*
>> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
>> index 23465bcd00..ea38be8b79 100644
>> --- a/xen/arch/x86/physdev.c
>> +++ b/xen/arch/x86/physdev.c
>> @@ -12,7 +12,7 @@
>> #include <asm/io_apic.h>
>> #include <asm/msi.h>
>> #include <asm/hvm/irq.h>
>> -#include <asm/hypercall.h>
>> +#include <xen/hypercall.h>
>> #include <public/xen.h>
>> #include <public/physdev.h>
>> #include <xsm/xsm.h>
>> @@ -480,54 +480,6 @@ ret_t do_physdev_op(int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>         break;
>>     }
>> 
>> -    case PHYSDEVOP_pci_device_add: {
>> -        struct physdev_pci_device_add add;
>> -        struct pci_dev_info pdev_info;
>> -        nodeid_t 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;
>> -
>> -        if ( add.flags & XEN_PCI_DEV_PXM )
>> -        {
>> -            uint32_t pxm;
>> -            size_t optarr_off = offsetof(struct physdev_pci_device_add, 
>> optarr) /
>> -                                sizeof(add.optarr[0]);
>> -
>> -            if ( copy_from_guest_offset(&pxm, arg, optarr_off, 1) )
>> -                break;
>> -
>> -            node = pxm_to_node(pxm);
>> -        }
>> -        else
>> -            node = NUMA_NO_NODE;
>> -
>> -        ret = pci_add_device(add.seg, add.bus, add.devfn, &pdev_info, node);
>> -        break;
>> -    }
>> -
>> -    case PHYSDEVOP_pci_device_remove: {
>> -        struct physdev_pci_device dev;
>> -
>> -        ret = -EFAULT;
>> -        if ( copy_from_guest(&dev, arg, 1) != 0 )
>> -            break;
>> -
>> -        ret = pci_remove_device(dev.seg, dev.bus, dev.devfn);
>> -        break;
>> -    }
>> -
>>     case PHYSDEVOP_prepare_msix:
>>     case PHYSDEVOP_release_msix: {
>>         struct physdev_pci_device dev;
>> @@ -663,7 +615,7 @@ ret_t do_physdev_op(int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>     }
>> 
>>     default:
>> -        ret = -ENOSYS;
>> +        ret = pci_physdev_op(cmd, arg);
>>         break;
>>     }
>> 
>> diff --git a/xen/arch/x86/x86_64/physdev.c b/xen/arch/x86/x86_64/physdev.c
>> index 0a50cbd4d8..e3cbd5ebcb 100644
>> --- a/xen/arch/x86/x86_64/physdev.c
>> +++ b/xen/arch/x86/x86_64/physdev.c
>> @@ -9,7 +9,7 @@ EMIT_FILE;
>> #include <compat/xen.h>
>> #include <compat/event_channel.h>
>> #include <compat/physdev.h>
>> -#include <asm/hypercall.h>
>> +#include <xen/hypercall.h>
>> 
>> #define do_physdev_op compat_physdev_op
>> 
>> diff --git a/xen/drivers/pci/Makefile b/xen/drivers/pci/Makefile
>> index a98035df4c..972c923db0 100644
>> --- a/xen/drivers/pci/Makefile
>> +++ b/xen/drivers/pci/Makefile
>> @@ -1 +1,2 @@
>> obj-y += pci.o
>> +obj-y += physdev.o
>> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
>> new file mode 100644
>> index 0000000000..4f3e1a96c0
>> --- /dev/null
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -0,0 +1,80 @@
>> +
>> +#include <xen/guest_access.h>
>> +#include <xen/hypercall.h>
>> +#include <xen/init.h>
>> +
>> +#ifndef COMPAT
>> +typedef long ret_t;
>> +#endif
>> +
>> +ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> +{
>> +    ret_t ret;
>> +
>> +    switch ( cmd )
>> +    {
>> +    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 = true;
>> +            pdev_info.physfn.bus = add.physfn.bus;
>> +            pdev_info.physfn.devfn = add.physfn.devfn;
>> +        }
>> +        else
>> +            pdev_info.is_virtfn = false;
>> +
>> +#ifdef CONFIG_NUMA
>> +        if ( add.flags & XEN_PCI_DEV_PXM )
>> +        {
>> +            uint32_t pxm;
>> +            size_t optarr_off = offsetof(struct physdev_pci_device_add, 
>> optarr) /
>> +                                sizeof(add.optarr[0]);
>> +
>> +            if ( copy_from_guest_offset(&pxm, arg, optarr_off, 1) )
>> +                break;
>> +
>> +            node = pxm_to_node(pxm);
>> +        }
>> +#endif
>> +
>> +        ret = pci_add_device(add.seg, add.bus, add.devfn, &pdev_info, node);
>> +        break;
>> +    }
>> +
>> +    case PHYSDEVOP_pci_device_remove: {
>> +        struct physdev_pci_device dev;
>> +
>> +        ret = -EFAULT;
>> +        if ( copy_from_guest(&dev, arg, 1) != 0 )
>> +            break;
>> +
>> +        ret = pci_remove_device(dev.seg, dev.bus, dev.devfn);
>> +        break;
>> +    }
>> +
>> +    default:
>> +        ret = -ENOSYS;
>> +        break;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 6b5a5f818a..d46c61fca9 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -107,7 +107,9 @@
>>  *   All generic sub-operations
>>  *
>>  *  HYPERVISOR_physdev_op
>> - *   No sub-operations are currenty supported
>> + *   Exactly these sub-operations are supported:
>> + *   PHYSDEVOP_pci_device_add
>> + *   PHYSDEVOP_pci_device_remove
>>  *
>>  *  HYPERVISOR_sysctl
>>  *   All generic sub-operations, with the exception of:
>> diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
>> index 3771487a30..7096cc4fe4 100644
>> --- a/xen/include/xen/hypercall.h
>> +++ b/xen/include/xen/hypercall.h
>> @@ -45,6 +45,17 @@ extern long
>> do_platform_op(
>>     XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op);
>> 
>> +#ifdef CONFIG_HAS_PCI
>> +extern long
>> +pci_physdev_op(
>> +    int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
>> +#else
>> +static inline long pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> +{
>> +    gdprintk(XENLOG_DEBUG, "PHYSDEVOP cmd=%d: not implemented\n", cmd);
>> +    return -ENOSYS;
>> +}
>> +#endif
> 
> Everything looks good up until here and you seemed to have addressed
> Jan's comments well.
> 
> However, for this last change to hypercall.h: hypercall.h doesn't seem
> to be the right place to add the static inline stub for the
> !CONFIG_HAS_PCI case. 
> 
> Given that only ARM needs the !CONFIG_HAS_PCI stub, I would add it
> directly to xen/arch/arm/physdev.c. Or just add an #ifdef directly
> within do_physdev_op in xen/arch/arm/physdev.c.

Ack . I will modify the code based on your suggestion.

Regards,
Rahul