Xen project Mailing List

Hi Jan,

> On 30 Sep 2021, at 3:51 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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."

Ok.I will add that information in commit msg.
>
>> @@ -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.)

I tried to move the code to driver/pci/ and I also feel it is better than moving code to common/physdev.c
I attached the patch for early feedback please have a look once.

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

Ack.
>
>> +        if ( add.flags & XEN_PCI_DEV_VIRTFN )
>> +        {
>> +            pdev_info.is_virtfn = 1;
>
> ... "true" was used here, and ...

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

Ok.
>
>> +            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.
Ack.
>
>> --- 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.)

NUMA_NO_NODE is defined in "xen/numa.h” but "asm/numa.h" is include in "xen/numa.h”
before defining NUMA_NO_NODE.

I will fix this like we fix for pci. Move the "asm/numa.h" in "xen/numa.h" after defining NUMA_NO_NODE

Regards,
Rahul
> Jan
>

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