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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 23.09.21 00:10, Stefano Stabellini wrote:
> On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> 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>
> Please make this the first patch of the series so that...
>
>> ---
>> New in v2
>> ---
>>   drivers/xen/Kconfig               | 26 +++++++++++++++++++++++++-
>>   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, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index 057ddf61ef61..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.
>> +
>>   config XEN_PCIDEV_BACKEND
>>      tristate "Xen PCI-device backend driver"
>> -    depends on PCI && XEN
>> +    depends on PCI && X86 && XEN
>>      depends on XEN_BACKEND
> ...we don't need this
Yes, I didn't like this change too
>
>
>> +    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 da34ce85dc88..bde63ef677b8 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;
> Is this truly enough to stop the PV backend from initializing? Have you
> actually tested it to make sure? If it works, amazing! I am quite happy
> about this approach :-)

Well, I put some logs into the driver and saw nothing obvious pointing

to any backend activities (probably this is also because I don't have any

frontend). I see that the xenbus driver is not registered. In XenStore I see:

root@dom0:~# xenstore-ls -f | grep pci
/local/domain/0/backend/pci = ""
/local/domain/0/backend/pci/2 = ""
/local/domain/0/backend/pci/2/0 = ""
/local/domain/0/backend/pci/2/0/frontend = "/local/domain/2/device/pci/0"
/local/domain/0/backend/pci/2/0/frontend-id = "2"
/local/domain/0/backend/pci/2/0/online = "1"
/local/domain/0/backend/pci/2/0/state = "1"
/local/domain/0/backend/pci/2/0/domain = "DomU"
/local/domain/0/backend/pci/2/0/key-0 = "0000:03:00.0"
/local/domain/0/backend/pci/2/0/dev-0 = "0000:03:00.0"
/local/domain/0/backend/pci/2/0/opts-0 = 
"msitranslate=0,power_mgmt=0,permissive=0,rdm_policy=strict"
/local/domain/0/backend/pci/2/0/state-0 = "1"
/local/domain/0/backend/pci/2/0/num_devs = "1"
/local/domain/2/device/pci = ""
/local/domain/2/device/pci/0 = ""
/local/domain/2/device/pci/0/backend = "/local/domain/0/backend/pci/2/0"
/local/domain/2/device/pci/0/backend-id = "0"
/local/domain/2/device/pci/0/state = "1"
/libxl/pci = ""
/libxl/pci/0000-03-00-0 = ""
/libxl/pci/0000-03-00-0/domid = "2"

But IIUIC these come from the toolstack

@Juergen, do you know how to check if the backend is indeed not running

or the above should be enough to prove?


>
>
>
>
>>      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
>>
Thank you,

Oleksandr