Re: [PATCH v3 05/17] xen/arm: Add PHYSDEVOP_pci_device_* support for ARM

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.09.2021 20:18, 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.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>

On v1 Julien said:

"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."

> @@ -82,6 +83,7 @@ CHECK_physdev_pci_device
>  typedef int ret_t;
>  
>  #include "../physdev.c"
> +#include "../../../common/physdev.c"

Please don't unless entirely unavoidable: common/ has its own
common/compat/.

> --- /dev/null
> +++ b/xen/common/physdev.c
> @@ -0,0 +1,81 @@
> +
> +#include <xen/guest_access.h>
> +#include <xen/hypercall.h>
> +#include <xen/init.h>
> +
> +#ifndef COMPAT
> +typedef long ret_t;
> +#endif
> +
> +ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    ret_t ret;
> +
> +    switch ( cmd )
> +    {
> +#ifdef CONFIG_HAS_PCI

All of the enclosed code should really be under drivers/pci/ or in
drivers/passthrough/pci.c, e.g. in a pci_physdev_op() function
called from both Arm and x86. Unless, I will admit, this would pose
undue problems for x86'es compat handling. But I'd like to know
whether that route was at least explored. (I.e. I'm afraid Julien's
request to move this code to "common" was understood too much to the
word, sorry.)

> +    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);

While I'm not going to insist (as you're merely moving this code), it
would be nice if the !!() was dropped here, ...

> +        if ( add.flags & XEN_PCI_DEV_VIRTFN )
> +        {
> +            pdev_info.is_virtfn = 1;

... "true" was used here, and ...

> +            pdev_info.physfn.bus = add.physfn.bus;
> +            pdev_info.physfn.devfn = add.physfn.devfn;
> +        }
> +        else
> +            pdev_info.is_virtfn = 0;

... "false" here while moving, as both fields are bool.

> +        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

I think this code should become CONFIG_NUMA dependent, now that it
gets moved to common code. This would save you from (oddly; see
below) implementing pxm_to_node() on Arm.

> +            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;
> +    }
> +#endif
> +    default:

Blank line between non-fall-through case blocks please.

> --- a/xen/include/asm-arm/numa.h
> +++ b/xen/include/asm-arm/numa.h
> @@ -25,6 +25,11 @@ extern mfn_t first_valid_mfn;
>  #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
>  #define __node_distance(a, b) (20)
>  
> +static inline nodeid_t pxm_to_node(unsigned pxm)
> +{
> +    return 0xff;

If you can use NUMA_NO_NODE in do_physdev_op(), why not also here?
(Assuming this function is going to survive in this series in the
first place, as per the earlier comment.)

Jan