Re: [PATCH v3 1/2] xen-pciback: prepare for the split for stub and PV

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 23.09.21 22:47, Stefano Stabellini wrote:
> On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
>> Currently PCI backend implements multiple functionalities at a time.
>> To name a few:
>> 1. It is used as a database for assignable PCI devices, e.g. xl
>>     pci-assignable-{add|remove|list} manipulates that list. So, whenever
>>     the toolstack needs to know which PCI devices can be passed through
>>     it reads that from the relevant sysfs entries of the pciback.
>> 2. It is used to hold the unbound PCI devices list, e.g. when passing
>>     through a PCI device it needs to be unbound from the relevant device
>>     driver and bound to pciback (strictly speaking it is not required
>>     that the device is bound to pciback, but pciback is again used as a
>>     database of the passed through PCI devices, so we can re-bind the
>>     devices back to their original drivers when guest domain shuts down)
>> 3. Device reset for the devices being passed through
>> 4. Para-virtualized use-cases support
>>
>> The para-virtualized part of the driver is not always needed as some
>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>> model for PCI device passthrough. For such use-cases make the very
>> first step in splitting the xen-pciback driver into two parts: extended
>> PCI stub and PCI PV backend drivers. At the moment x86 platform will
>> continue using CONFIG_XEN_PCIDEV_BACKEND for the fully featured backend
>> driver and new platforms may build a driver with limited functionality
>> (no PV) by enabling CONFIG_XEN_PCIDEV_STUB.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> ---
>> Changes since v2:
>>   - swap the patch order
>> New in v2
>> ---
>>   drivers/xen/Kconfig               | 24 ++++++++++++++++++++++++
>>   drivers/xen/Makefile              |  2 +-
>>   drivers/xen/xen-pciback/Makefile  |  1 +
>>   drivers/xen/xen-pciback/pciback.h |  5 +++++
>>   drivers/xen/xen-pciback/xenbus.c  |  6 +++++-
>>   5 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index a37eb52fb401..6e92c6be19f1 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -180,10 +180,34 @@ config SWIOTLB_XEN
>>      select DMA_OPS
>>      select SWIOTLB
>>   
>> +config XEN_PCI_STUB
>> +    bool
>> +
>> +config XEN_PCIDEV_STUB
>> +    tristate "Xen PCI-device stub driver"
>> +    depends on PCI && !X86 && XEN
>> +    depends on XEN_BACKEND
>> +    select XEN_PCI_STUB
>> +    default m
>> +    help
>> +      The PCI device stub driver provides limited version of the PCI
>> +      device backend driver without para-virtualized support for guests.
>> +      If you select this to be a module, you will need to make sure no
>> +      other driver has bound to the device(s) you want to make visible to
>> +      other guests.
>> +
>> +      The "hide" parameter (only applicable if backend driver is compiled
>> +      into the kernel) allows you to bind the PCI devices to this module
>> +      from the default device drivers. The argument is the list of PCI BDFs:
>> +      xen-pciback.hide=(03:00.0)(04:00.0)
>> +
>> +      If in doubt, say m.
>> +
> I get this build error on ARM:
>
> drivers/xen/xen-pciback/pci_stub.c:22:10: fatal error: asm/xen/pci.h: No such 
> file or directory
>   #include <asm/xen/pci.h>
>            ^~~~~~~~~~~~~~~
> compilation terminated.
> scripts/Makefile.build:277: recipe for target 
> 'drivers/xen/xen-pciback/pci_stub.o' failed
>
>
> I imagine that the second patch is required for the build on ARM.
Ah, yes. That is why it was the first
>   I
> suggest to move the definition of XEN_PCIDEV_STUB to the second patch.
> (keep XEN_PCI_STUB and xen_pcibk_pv_support here.)
I'll go with this
>
> Alternatively, just remove "tristate "Xen PCI-device stub driver"" from
> here so that XEN_PCIDEV_STUB becomes non-selectable. You can add the
> tristate property in patch #2.
It looks like half-baked...
>
>
>
>>   config XEN_PCIDEV_BACKEND
>>      tristate "Xen PCI-device backend driver"
>>      depends on PCI && X86 && XEN
>>      depends on XEN_BACKEND
>> +    select XEN_PCI_STUB
>>      default m
>>      help
>>        The PCI device backend driver allows the kernel to export arbitrary
>> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
>> index 3434593455b2..5aae66e638a7 100644
>> --- a/drivers/xen/Makefile
>> +++ b/drivers/xen/Makefile
>> @@ -24,7 +24,7 @@ obj-$(CONFIG_XEN_SYS_HYPERVISOR)   += sys-hypervisor.o
>>   obj-$(CONFIG_XEN_PVHVM_GUEST)              += platform-pci.o
>>   obj-$(CONFIG_SWIOTLB_XEN)          += swiotlb-xen.o
>>   obj-$(CONFIG_XEN_MCE_LOG)          += mcelog.o
>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND)    += xen-pciback/
>> +obj-$(CONFIG_XEN_PCI_STUB)          += xen-pciback/
>>   obj-$(CONFIG_XEN_PRIVCMD)          += xen-privcmd.o
>>   obj-$(CONFIG_XEN_ACPI_PROCESSOR)   += xen-acpi-processor.o
>>   obj-$(CONFIG_XEN_EFI)                      += efi.o
>> diff --git a/drivers/xen/xen-pciback/Makefile 
>> b/drivers/xen/xen-pciback/Makefile
>> index e8d981d43235..e2cb376444a6 100644
>> --- a/drivers/xen/xen-pciback/Makefile
>> +++ b/drivers/xen/xen-pciback/Makefile
>> @@ -1,5 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0
>>   obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>> +obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>   
>>   xen-pciback-y := pci_stub.o pciback_ops.o xenbus.o
>>   xen-pciback-y += conf_space.o conf_space_header.o \
>> diff --git a/drivers/xen/xen-pciback/pciback.h 
>> b/drivers/xen/xen-pciback/pciback.h
>> index 95e28ee48d52..9a64196e831d 100644
>> --- a/drivers/xen/xen-pciback/pciback.h
>> +++ b/drivers/xen/xen-pciback/pciback.h
>> @@ -71,6 +71,11 @@ struct pci_dev *pcistub_get_pci_dev(struct 
>> xen_pcibk_device *pdev,
>>                                  struct pci_dev *dev);
>>   void pcistub_put_pci_dev(struct pci_dev *dev);
>>   
>> +static inline bool xen_pcibk_pv_support(void)
>> +{
>> +    return IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND);
>> +}
>> +
>>   /* Ensure a device is turned off or reset */
>>   void xen_pcibk_reset_device(struct pci_dev *pdev);
>>   
>> diff --git a/drivers/xen/xen-pciback/xenbus.c 
>> b/drivers/xen/xen-pciback/xenbus.c
>> index c09c7ebd6968..f8ba2903a3ff 100644
>> --- a/drivers/xen/xen-pciback/xenbus.c
>> +++ b/drivers/xen/xen-pciback/xenbus.c
>> @@ -743,6 +743,9 @@ const struct xen_pcibk_backend *__read_mostly 
>> xen_pcibk_backend;
>>   
>>   int __init xen_pcibk_xenbus_register(void)
>>   {
>> +    if (!xen_pcibk_pv_support())
>> +            return 0;
>> +
>>      xen_pcibk_backend = &xen_pcibk_vpci_backend;
>>      if (passthrough)
>>              xen_pcibk_backend = &xen_pcibk_passthrough_backend;
>> @@ -752,5 +755,6 @@ int __init xen_pcibk_xenbus_register(void)
>>   
>>   void __exit xen_pcibk_xenbus_unregister(void)
>>   {
>> -    xenbus_unregister_driver(&xen_pcibk_driver);
>> +    if (xen_pcibk_pv_support())
>> +            xenbus_unregister_driver(&xen_pcibk_driver);
>>   }
>> -- 
>> 2.25.1
>>