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

[Oleksandr Andrushchenko]

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