Hi Jan, > On 30 Sep 2021, at 3:51 pm, Jan Beulich <jbeul...@suse.com> 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.si...@arm.com> > > 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
>
0001-xen-arm-Add-PHYSDEVOP_pci_device_-add-remove-support.patch
Description: 0001-xen-arm-Add-PHYSDEVOP_pci_device_-add-remove-support.patch
