[PATCH 1/2] xen-pciback: allow compiling on other archs than x86

[PATCH 1/2] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Xen-pciback driver was designed to be built for x86 only. But it
can also be used by other architectures, e.g. Arm.
Re-structure the driver in a way that it can be built for other
platforms as well.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>

---
Since v1:
 - Do not move pci_xen_initial_domain as it is x86 specific
---
 arch/x86/include/asm/xen/pci.h     | 18 +------
 arch/x86/pci/xen.c                 | 74 +----------------------------
 drivers/xen/Kconfig                |  2 +-
 drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
 drivers/xen/xen-pciback/pci_stub.c |  3 +-
 drivers/xen/xen-pciback/xenbus.c   |  2 +-
 include/xen/pci.h                  | 28 +++++++++++
 7 files changed, 109 insertions(+), 93 deletions(-)
 create mode 100644 include/xen/pci.h

diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
index 3506d8c598c1..2889f091f459 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -16,26 +16,10 @@ static inline int pci_xen_hvm_init(void)
 #endif
 #if defined(CONFIG_XEN_DOM0)
 int __init pci_xen_initial_domain(void);
-int xen_find_device_domain_owner(struct pci_dev *dev);
-int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
-int xen_unregister_device_domain_owner(struct pci_dev *dev);
 #else
 static inline int __init pci_xen_initial_domain(void)
 {
-	return -1;
-}
-static inline int xen_find_device_domain_owner(struct pci_dev *dev)
-{
-	return -1;
-}
-static inline int xen_register_device_domain_owner(struct pci_dev *dev,
-						   uint16_t domain)
-{
-	return -1;
-}
-static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
-{
-	return -1;
+       return -1;
 }
 #endif
 
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 3d41a09c2c14..4a45b0bf9ae4 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -23,6 +23,7 @@
 
 #include <xen/features.h>
 #include <xen/events.h>
+#include <xen/pci.h>
 #include <asm/xen/pci.h>
 #include <asm/xen/cpuid.h>
 #include <asm/apic.h>
@@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
 	}
 	return 0;
 }
-
-struct xen_device_domain_owner {
-	domid_t domain;
-	struct pci_dev *dev;
-	struct list_head list;
-};
-
-static DEFINE_SPINLOCK(dev_domain_list_spinlock);
-static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
-
-static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
-{
-	struct xen_device_domain_owner *owner;
-
-	list_for_each_entry(owner, &dev_domain_list, list) {
-		if (owner->dev == dev)
-			return owner;
-	}
-	return NULL;
-}
-
-int xen_find_device_domain_owner(struct pci_dev *dev)
-{
-	struct xen_device_domain_owner *owner;
-	int domain = -ENODEV;
-
-	spin_lock(&dev_domain_list_spinlock);
-	owner = find_device(dev);
-	if (owner)
-		domain = owner->domain;
-	spin_unlock(&dev_domain_list_spinlock);
-	return domain;
-}
-EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
-
-int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
-{
-	struct xen_device_domain_owner *owner;
-
-	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
-	if (!owner)
-		return -ENODEV;
-
-	spin_lock(&dev_domain_list_spinlock);
-	if (find_device(dev)) {
-		spin_unlock(&dev_domain_list_spinlock);
-		kfree(owner);
-		return -EEXIST;
-	}
-	owner->domain = domain;
-	owner->dev = dev;
-	list_add_tail(&owner->list, &dev_domain_list);
-	spin_unlock(&dev_domain_list_spinlock);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
-
-int xen_unregister_device_domain_owner(struct pci_dev *dev)
-{
-	struct xen_device_domain_owner *owner;
-
-	spin_lock(&dev_domain_list_spinlock);
-	owner = find_device(dev);
-	if (!owner) {
-		spin_unlock(&dev_domain_list_spinlock);
-		return -ENODEV;
-	}
-	list_del(&owner->list);
-	spin_unlock(&dev_domain_list_spinlock);
-	kfree(owner);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
 #endif
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index a37eb52fb401..057ddf61ef61 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -182,7 +182,7 @@ config SWIOTLB_XEN
 
 config XEN_PCIDEV_BACKEND
 	tristate "Xen PCI-device backend driver"
-	depends on PCI && X86 && XEN
+	depends on PCI && XEN
 	depends on XEN_BACKEND
 	default m
 	help
diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 224df03ce42e..fc8c1249d49f 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -254,3 +254,78 @@ static int xen_mcfg_late(void)
 	return 0;
 }
 #endif
+
+#ifdef CONFIG_XEN_DOM0
+struct xen_device_domain_owner {
+	domid_t domain;
+	struct pci_dev *dev;
+	struct list_head list;
+};
+
+static DEFINE_SPINLOCK(dev_domain_list_spinlock);
+static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
+
+static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
+{
+	struct xen_device_domain_owner *owner;
+
+	list_for_each_entry(owner, &dev_domain_list, list) {
+		if (owner->dev == dev)
+			return owner;
+	}
+	return NULL;
+}
+
+int xen_find_device_domain_owner(struct pci_dev *dev)
+{
+	struct xen_device_domain_owner *owner;
+	int domain = -ENODEV;
+
+	spin_lock(&dev_domain_list_spinlock);
+	owner = find_device(dev);
+	if (owner)
+		domain = owner->domain;
+	spin_unlock(&dev_domain_list_spinlock);
+	return domain;
+}
+EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
+
+int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
+{
+	struct xen_device_domain_owner *owner;
+
+	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
+	if (!owner)
+		return -ENODEV;
+
+	spin_lock(&dev_domain_list_spinlock);
+	if (find_device(dev)) {
+		spin_unlock(&dev_domain_list_spinlock);
+		kfree(owner);
+		return -EEXIST;
+	}
+	owner->domain = domain;
+	owner->dev = dev;
+	list_add_tail(&owner->list, &dev_domain_list);
+	spin_unlock(&dev_domain_list_spinlock);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
+
+int xen_unregister_device_domain_owner(struct pci_dev *dev)
+{
+	struct xen_device_domain_owner *owner;
+
+	spin_lock(&dev_domain_list_spinlock);
+	owner = find_device(dev);
+	if (!owner) {
+		spin_unlock(&dev_domain_list_spinlock);
+		return -ENODEV;
+	}
+	list_del(&owner->list);
+	spin_unlock(&dev_domain_list_spinlock);
+	kfree(owner);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
+#endif
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index f8e4faa96ad6..bba527620507 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -19,7 +19,8 @@
 #include <linux/sched.h>
 #include <linux/atomic.h>
 #include <xen/events.h>
-#include <asm/xen/pci.h>
+#include <xen/pci.h>
+#include <xen/xen.h>
 #include <asm/xen/hypervisor.h>
 #include <xen/interface/physdev.h>
 #include "pciback.h"
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index c09c7ebd6968..da34ce85dc88 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -14,7 +14,7 @@
 #include <linux/workqueue.h>
 #include <xen/xenbus.h>
 #include <xen/events.h>
-#include <asm/xen/pci.h>
+#include <xen/pci.h>
 #include "pciback.h"
 
 #define INVALID_EVTCHN_IRQ  (-1)
diff --git a/include/xen/pci.h b/include/xen/pci.h
new file mode 100644
index 000000000000..b8337cf85fd1
--- /dev/null
+++ b/include/xen/pci.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __XEN_PCI_H__
+#define __XEN_PCI_H__
+
+#if defined(CONFIG_XEN_DOM0)
+int xen_find_device_domain_owner(struct pci_dev *dev);
+int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
+int xen_unregister_device_domain_owner(struct pci_dev *dev);
+#else
+static inline int xen_find_device_domain_owner(struct pci_dev *dev)
+{
+	return -1;
+}
+
+static inline int xen_register_device_domain_owner(struct pci_dev *dev,
+						   uint16_t domain)
+{
+	return -1;
+}
+
+static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
+{
+	return -1;
+}
+#endif
+
+#endif
-- 
2.25.1

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

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

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@epam.com>

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

Re: [PATCH 1/2] xen-pciback: allow compiling on other archs than x86

[Stefano Stabellini]

On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Xen-pciback driver was designed to be built for x86 only. But it
> can also be used by other architectures, e.g. Arm.
> Re-structure the driver in a way that it can be built for other
> platforms as well.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>

This patch looks good to me but I would prefer if it came after the
other patch, not before.


> ---
> Since v1:
>  - Do not move pci_xen_initial_domain as it is x86 specific
> ---
>  arch/x86/include/asm/xen/pci.h     | 18 +------
>  arch/x86/pci/xen.c                 | 74 +----------------------------
>  drivers/xen/Kconfig                |  2 +-
>  drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>  drivers/xen/xen-pciback/pci_stub.c |  3 +-
>  drivers/xen/xen-pciback/xenbus.c   |  2 +-
>  include/xen/pci.h                  | 28 +++++++++++
>  7 files changed, 109 insertions(+), 93 deletions(-)
>  create mode 100644 include/xen/pci.h
> 
> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
> index 3506d8c598c1..2889f091f459 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -16,26 +16,10 @@ static inline int pci_xen_hvm_init(void)
>  #endif
>  #if defined(CONFIG_XEN_DOM0)
>  int __init pci_xen_initial_domain(void);
> -int xen_find_device_domain_owner(struct pci_dev *dev);
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
> -int xen_unregister_device_domain_owner(struct pci_dev *dev);
>  #else
>  static inline int __init pci_xen_initial_domain(void)
>  {
> -	return -1;
> -}
> -static inline int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> -	return -1;
> -}
> -static inline int xen_register_device_domain_owner(struct pci_dev *dev,
> -						   uint16_t domain)
> -{
> -	return -1;
> -}
> -static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> -	return -1;
> +       return -1;
>  }
>  #endif
>  
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 3d41a09c2c14..4a45b0bf9ae4 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -23,6 +23,7 @@
>  
>  #include <xen/features.h>
>  #include <xen/events.h>
> +#include <xen/pci.h>
>  #include <asm/xen/pci.h>
>  #include <asm/xen/cpuid.h>
>  #include <asm/apic.h>
> @@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
>  	}
>  	return 0;
>  }
> -
> -struct xen_device_domain_owner {
> -	domid_t domain;
> -	struct pci_dev *dev;
> -	struct list_head list;
> -};
> -
> -static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> -static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> -
> -static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
> -{
> -	struct xen_device_domain_owner *owner;
> -
> -	list_for_each_entry(owner, &dev_domain_list, list) {
> -		if (owner->dev == dev)
> -			return owner;
> -	}
> -	return NULL;
> -}
> -
> -int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> -	struct xen_device_domain_owner *owner;
> -	int domain = -ENODEV;
> -
> -	spin_lock(&dev_domain_list_spinlock);
> -	owner = find_device(dev);
> -	if (owner)
> -		domain = owner->domain;
> -	spin_unlock(&dev_domain_list_spinlock);
> -	return domain;
> -}
> -EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
> -
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
> -{
> -	struct xen_device_domain_owner *owner;
> -
> -	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
> -	if (!owner)
> -		return -ENODEV;
> -
> -	spin_lock(&dev_domain_list_spinlock);
> -	if (find_device(dev)) {
> -		spin_unlock(&dev_domain_list_spinlock);
> -		kfree(owner);
> -		return -EEXIST;
> -	}
> -	owner->domain = domain;
> -	owner->dev = dev;
> -	list_add_tail(&owner->list, &dev_domain_list);
> -	spin_unlock(&dev_domain_list_spinlock);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
> -
> -int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> -	struct xen_device_domain_owner *owner;
> -
> -	spin_lock(&dev_domain_list_spinlock);
> -	owner = find_device(dev);
> -	if (!owner) {
> -		spin_unlock(&dev_domain_list_spinlock);
> -		return -ENODEV;
> -	}
> -	list_del(&owner->list);
> -	spin_unlock(&dev_domain_list_spinlock);
> -	kfree(owner);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
>  #endif
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index a37eb52fb401..057ddf61ef61 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -182,7 +182,7 @@ config SWIOTLB_XEN
>  
>  config XEN_PCIDEV_BACKEND
>  	tristate "Xen PCI-device backend driver"
> -	depends on PCI && X86 && XEN
> +	depends on PCI && XEN
>  	depends on XEN_BACKEND
>  	default m
>  	help
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> index 224df03ce42e..fc8c1249d49f 100644
> --- a/drivers/xen/pci.c
> +++ b/drivers/xen/pci.c
> @@ -254,3 +254,78 @@ static int xen_mcfg_late(void)
>  	return 0;
>  }
>  #endif
> +
> +#ifdef CONFIG_XEN_DOM0
> +struct xen_device_domain_owner {
> +	domid_t domain;
> +	struct pci_dev *dev;
> +	struct list_head list;
> +};
> +
> +static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> +static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> +
> +static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
> +{
> +	struct xen_device_domain_owner *owner;
> +
> +	list_for_each_entry(owner, &dev_domain_list, list) {
> +		if (owner->dev == dev)
> +			return owner;
> +	}
> +	return NULL;
> +}
> +
> +int xen_find_device_domain_owner(struct pci_dev *dev)
> +{
> +	struct xen_device_domain_owner *owner;
> +	int domain = -ENODEV;
> +
> +	spin_lock(&dev_domain_list_spinlock);
> +	owner = find_device(dev);
> +	if (owner)
> +		domain = owner->domain;
> +	spin_unlock(&dev_domain_list_spinlock);
> +	return domain;
> +}
> +EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
> +
> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
> +{
> +	struct xen_device_domain_owner *owner;
> +
> +	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
> +	if (!owner)
> +		return -ENODEV;
> +
> +	spin_lock(&dev_domain_list_spinlock);
> +	if (find_device(dev)) {
> +		spin_unlock(&dev_domain_list_spinlock);
> +		kfree(owner);
> +		return -EEXIST;
> +	}
> +	owner->domain = domain;
> +	owner->dev = dev;
> +	list_add_tail(&owner->list, &dev_domain_list);
> +	spin_unlock(&dev_domain_list_spinlock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
> +
> +int xen_unregister_device_domain_owner(struct pci_dev *dev)
> +{
> +	struct xen_device_domain_owner *owner;
> +
> +	spin_lock(&dev_domain_list_spinlock);
> +	owner = find_device(dev);
> +	if (!owner) {
> +		spin_unlock(&dev_domain_list_spinlock);
> +		return -ENODEV;
> +	}
> +	list_del(&owner->list);
> +	spin_unlock(&dev_domain_list_spinlock);
> +	kfree(owner);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
> +#endif
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index f8e4faa96ad6..bba527620507 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -19,7 +19,8 @@
>  #include <linux/sched.h>
>  #include <linux/atomic.h>
>  #include <xen/events.h>
> -#include <asm/xen/pci.h>
> +#include <xen/pci.h>
> +#include <xen/xen.h>
>  #include <asm/xen/hypervisor.h>
>  #include <xen/interface/physdev.h>
>  #include "pciback.h"
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c09c7ebd6968..da34ce85dc88 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -14,7 +14,7 @@
>  #include <linux/workqueue.h>
>  #include <xen/xenbus.h>
>  #include <xen/events.h>
> -#include <asm/xen/pci.h>
> +#include <xen/pci.h>
>  #include "pciback.h"
>  
>  #define INVALID_EVTCHN_IRQ  (-1)
> diff --git a/include/xen/pci.h b/include/xen/pci.h
> new file mode 100644
> index 000000000000..b8337cf85fd1
> --- /dev/null
> +++ b/include/xen/pci.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __XEN_PCI_H__
> +#define __XEN_PCI_H__
> +
> +#if defined(CONFIG_XEN_DOM0)
> +int xen_find_device_domain_owner(struct pci_dev *dev);
> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
> +int xen_unregister_device_domain_owner(struct pci_dev *dev);
> +#else
> +static inline int xen_find_device_domain_owner(struct pci_dev *dev)
> +{
> +	return -1;
> +}
> +
> +static inline int xen_register_device_domain_owner(struct pci_dev *dev,
> +						   uint16_t domain)
> +{
> +	return -1;
> +}
> +
> +static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
> +{
> +	return -1;
> +}
> +#endif
> +
> +#endif
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] xen-pciback: allow compiling on other archs than x86

[Stefano Stabellini]

On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Xen-pciback driver was designed to be built for x86 only. But it
> can also be used by other architectures, e.g. Arm.
> Re-structure the driver in a way that it can be built for other
> platforms as well.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>

This patch looks good to me but I would prefer if it came after the
other patch, not before.


> ---
> Since v1:
>  - Do not move pci_xen_initial_domain as it is x86 specific
> ---
>  arch/x86/include/asm/xen/pci.h     | 18 +------
>  arch/x86/pci/xen.c                 | 74 +----------------------------
>  drivers/xen/Kconfig                |  2 +-
>  drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>  drivers/xen/xen-pciback/pci_stub.c |  3 +-
>  drivers/xen/xen-pciback/xenbus.c   |  2 +-
>  include/xen/pci.h                  | 28 +++++++++++
>  7 files changed, 109 insertions(+), 93 deletions(-)
>  create mode 100644 include/xen/pci.h
> 
> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
> index 3506d8c598c1..2889f091f459 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -16,26 +16,10 @@ static inline int pci_xen_hvm_init(void)
>  #endif
>  #if defined(CONFIG_XEN_DOM0)
>  int __init pci_xen_initial_domain(void);
> -int xen_find_device_domain_owner(struct pci_dev *dev);
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
> -int xen_unregister_device_domain_owner(struct pci_dev *dev);
>  #else
>  static inline int __init pci_xen_initial_domain(void)
>  {
> -	return -1;
> -}
> -static inline int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> -	return -1;
> -}
> -static inline int xen_register_device_domain_owner(struct pci_dev *dev,
> -						   uint16_t domain)
> -{
> -	return -1;
> -}
> -static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> -	return -1;
> +       return -1;
>  }
>  #endif
>  
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 3d41a09c2c14..4a45b0bf9ae4 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -23,6 +23,7 @@
>  
>  #include <xen/features.h>
>  #include <xen/events.h>
> +#include <xen/pci.h>
>  #include <asm/xen/pci.h>
>  #include <asm/xen/cpuid.h>
>  #include <asm/apic.h>
> @@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
>  	}
>  	return 0;
>  }
> -
> -struct xen_device_domain_owner {
> -	domid_t domain;
> -	struct pci_dev *dev;
> -	struct list_head list;
> -};
> -
> -static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> -static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> -
> -static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
> -{
> -	struct xen_device_domain_owner *owner;
> -
> -	list_for_each_entry(owner, &dev_domain_list, list) {
> -		if (owner->dev == dev)
> -			return owner;
> -	}
> -	return NULL;
> -}
> -
> -int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> -	struct xen_device_domain_owner *owner;
> -	int domain = -ENODEV;
> -
> -	spin_lock(&dev_domain_list_spinlock);
> -	owner = find_device(dev);
> -	if (owner)
> -		domain = owner->domain;
> -	spin_unlock(&dev_domain_list_spinlock);
> -	return domain;
> -}
> -EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
> -
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
> -{
> -	struct xen_device_domain_owner *owner;
> -
> -	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
> -	if (!owner)
> -		return -ENODEV;
> -
> -	spin_lock(&dev_domain_list_spinlock);
> -	if (find_device(dev)) {
> -		spin_unlock(&dev_domain_list_spinlock);
> -		kfree(owner);
> -		return -EEXIST;
> -	}
> -	owner->domain = domain;
> -	owner->dev = dev;
> -	list_add_tail(&owner->list, &dev_domain_list);
> -	spin_unlock(&dev_domain_list_spinlock);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
> -
> -int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> -	struct xen_device_domain_owner *owner;
> -
> -	spin_lock(&dev_domain_list_spinlock);
> -	owner = find_device(dev);
> -	if (!owner) {
> -		spin_unlock(&dev_domain_list_spinlock);
> -		return -ENODEV;
> -	}
> -	list_del(&owner->list);
> -	spin_unlock(&dev_domain_list_spinlock);
> -	kfree(owner);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
>  #endif
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index a37eb52fb401..057ddf61ef61 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -182,7 +182,7 @@ config SWIOTLB_XEN
>  
>  config XEN_PCIDEV_BACKEND
>  	tristate "Xen PCI-device backend driver"
> -	depends on PCI && X86 && XEN
> +	depends on PCI && XEN
>  	depends on XEN_BACKEND
>  	default m
>  	help
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> index 224df03ce42e..fc8c1249d49f 100644
> --- a/drivers/xen/pci.c
> +++ b/drivers/xen/pci.c
> @@ -254,3 +254,78 @@ static int xen_mcfg_late(void)
>  	return 0;
>  }
>  #endif
> +
> +#ifdef CONFIG_XEN_DOM0
> +struct xen_device_domain_owner {
> +	domid_t domain;
> +	struct pci_dev *dev;
> +	struct list_head list;
> +};
> +
> +static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> +static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> +
> +static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
> +{
> +	struct xen_device_domain_owner *owner;
> +
> +	list_for_each_entry(owner, &dev_domain_list, list) {
> +		if (owner->dev == dev)
> +			return owner;
> +	}
> +	return NULL;
> +}
> +
> +int xen_find_device_domain_owner(struct pci_dev *dev)
> +{
> +	struct xen_device_domain_owner *owner;
> +	int domain = -ENODEV;
> +
> +	spin_lock(&dev_domain_list_spinlock);
> +	owner = find_device(dev);
> +	if (owner)
> +		domain = owner->domain;
> +	spin_unlock(&dev_domain_list_spinlock);
> +	return domain;
> +}
> +EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
> +
> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
> +{
> +	struct xen_device_domain_owner *owner;
> +
> +	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
> +	if (!owner)
> +		return -ENODEV;
> +
> +	spin_lock(&dev_domain_list_spinlock);
> +	if (find_device(dev)) {
> +		spin_unlock(&dev_domain_list_spinlock);
> +		kfree(owner);
> +		return -EEXIST;
> +	}
> +	owner->domain = domain;
> +	owner->dev = dev;
> +	list_add_tail(&owner->list, &dev_domain_list);
> +	spin_unlock(&dev_domain_list_spinlock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
> +
> +int xen_unregister_device_domain_owner(struct pci_dev *dev)
> +{
> +	struct xen_device_domain_owner *owner;
> +
> +	spin_lock(&dev_domain_list_spinlock);
> +	owner = find_device(dev);
> +	if (!owner) {
> +		spin_unlock(&dev_domain_list_spinlock);
> +		return -ENODEV;
> +	}
> +	list_del(&owner->list);
> +	spin_unlock(&dev_domain_list_spinlock);
> +	kfree(owner);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
> +#endif
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index f8e4faa96ad6..bba527620507 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -19,7 +19,8 @@
>  #include <linux/sched.h>
>  #include <linux/atomic.h>
>  #include <xen/events.h>
> -#include <asm/xen/pci.h>
> +#include <xen/pci.h>
> +#include <xen/xen.h>
>  #include <asm/xen/hypervisor.h>
>  #include <xen/interface/physdev.h>
>  #include "pciback.h"
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c09c7ebd6968..da34ce85dc88 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -14,7 +14,7 @@
>  #include <linux/workqueue.h>
>  #include <xen/xenbus.h>
>  #include <xen/events.h>
> -#include <asm/xen/pci.h>
> +#include <xen/pci.h>
>  #include "pciback.h"
>  
>  #define INVALID_EVTCHN_IRQ  (-1)
> diff --git a/include/xen/pci.h b/include/xen/pci.h
> new file mode 100644
> index 000000000000..b8337cf85fd1
> --- /dev/null
> +++ b/include/xen/pci.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __XEN_PCI_H__
> +#define __XEN_PCI_H__
> +
> +#if defined(CONFIG_XEN_DOM0)
> +int xen_find_device_domain_owner(struct pci_dev *dev);
> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
> +int xen_unregister_device_domain_owner(struct pci_dev *dev);
> +#else
> +static inline int xen_find_device_domain_owner(struct pci_dev *dev)
> +{
> +	return -1;
> +}
> +
> +static inline int xen_register_device_domain_owner(struct pci_dev *dev,
> +						   uint16_t domain)
> +{
> +	return -1;
> +}
> +
> +static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
> +{
> +	return -1;
> +}
> +#endif
> +
> +#endif
> -- 
> 2.25.1
> 

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

[Stefano Stabellini]

On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> 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@epam.com>

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


> +	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 :-)




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

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

[Stefano Stabellini]

On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> 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@epam.com>

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


> +	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 :-)




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

Re: [PATCH 1/2] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

On 23.09.21 00:07, Stefano Stabellini wrote:
> On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Xen-pciback driver was designed to be built for x86 only. But it
>> can also be used by other architectures, e.g. Arm.
>> Re-structure the driver in a way that it can be built for other
>> platforms as well.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>
> This patch looks good to me but I would prefer if it came after the
> other patch, not before.
Ok, will swap
>
>
>> ---
>> Since v1:
>>   - Do not move pci_xen_initial_domain as it is x86 specific
>> ---
>>   arch/x86/include/asm/xen/pci.h     | 18 +------
>>   arch/x86/pci/xen.c                 | 74 +----------------------------
>>   drivers/xen/Kconfig                |  2 +-
>>   drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>>   drivers/xen/xen-pciback/pci_stub.c |  3 +-
>>   drivers/xen/xen-pciback/xenbus.c   |  2 +-
>>   include/xen/pci.h                  | 28 +++++++++++
>>   7 files changed, 109 insertions(+), 93 deletions(-)
>>   create mode 100644 include/xen/pci.h
>>
>> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
>> index 3506d8c598c1..2889f091f459 100644
>> --- a/arch/x86/include/asm/xen/pci.h
>> +++ b/arch/x86/include/asm/xen/pci.h
>> @@ -16,26 +16,10 @@ static inline int pci_xen_hvm_init(void)
>>   #endif
>>   #if defined(CONFIG_XEN_DOM0)
>>   int __init pci_xen_initial_domain(void);
>> -int xen_find_device_domain_owner(struct pci_dev *dev);
>> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
>> -int xen_unregister_device_domain_owner(struct pci_dev *dev);
>>   #else
>>   static inline int __init pci_xen_initial_domain(void)
>>   {
>> -	return -1;
>> -}
>> -static inline int xen_find_device_domain_owner(struct pci_dev *dev)
>> -{
>> -	return -1;
>> -}
>> -static inline int xen_register_device_domain_owner(struct pci_dev *dev,
>> -						   uint16_t domain)
>> -{
>> -	return -1;
>> -}
>> -static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> -{
>> -	return -1;
>> +       return -1;
>>   }
>>   #endif
>>   
>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>> index 3d41a09c2c14..4a45b0bf9ae4 100644
>> --- a/arch/x86/pci/xen.c
>> +++ b/arch/x86/pci/xen.c
>> @@ -23,6 +23,7 @@
>>   
>>   #include <xen/features.h>
>>   #include <xen/events.h>
>> +#include <xen/pci.h>
>>   #include <asm/xen/pci.h>
>>   #include <asm/xen/cpuid.h>
>>   #include <asm/apic.h>
>> @@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
>>   	}
>>   	return 0;
>>   }
>> -
>> -struct xen_device_domain_owner {
>> -	domid_t domain;
>> -	struct pci_dev *dev;
>> -	struct list_head list;
>> -};
>> -
>> -static DEFINE_SPINLOCK(dev_domain_list_spinlock);
>> -static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
>> -
>> -static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
>> -{
>> -	struct xen_device_domain_owner *owner;
>> -
>> -	list_for_each_entry(owner, &dev_domain_list, list) {
>> -		if (owner->dev == dev)
>> -			return owner;
>> -	}
>> -	return NULL;
>> -}
>> -
>> -int xen_find_device_domain_owner(struct pci_dev *dev)
>> -{
>> -	struct xen_device_domain_owner *owner;
>> -	int domain = -ENODEV;
>> -
>> -	spin_lock(&dev_domain_list_spinlock);
>> -	owner = find_device(dev);
>> -	if (owner)
>> -		domain = owner->domain;
>> -	spin_unlock(&dev_domain_list_spinlock);
>> -	return domain;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
>> -
>> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
>> -{
>> -	struct xen_device_domain_owner *owner;
>> -
>> -	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
>> -	if (!owner)
>> -		return -ENODEV;
>> -
>> -	spin_lock(&dev_domain_list_spinlock);
>> -	if (find_device(dev)) {
>> -		spin_unlock(&dev_domain_list_spinlock);
>> -		kfree(owner);
>> -		return -EEXIST;
>> -	}
>> -	owner->domain = domain;
>> -	owner->dev = dev;
>> -	list_add_tail(&owner->list, &dev_domain_list);
>> -	spin_unlock(&dev_domain_list_spinlock);
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
>> -
>> -int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> -{
>> -	struct xen_device_domain_owner *owner;
>> -
>> -	spin_lock(&dev_domain_list_spinlock);
>> -	owner = find_device(dev);
>> -	if (!owner) {
>> -		spin_unlock(&dev_domain_list_spinlock);
>> -		return -ENODEV;
>> -	}
>> -	list_del(&owner->list);
>> -	spin_unlock(&dev_domain_list_spinlock);
>> -	kfree(owner);
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
>>   #endif
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index a37eb52fb401..057ddf61ef61 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -182,7 +182,7 @@ config SWIOTLB_XEN
>>   
>>   config XEN_PCIDEV_BACKEND
>>   	tristate "Xen PCI-device backend driver"
>> -	depends on PCI && X86 && XEN
>> +	depends on PCI && XEN
>>   	depends on XEN_BACKEND
>>   	default m
>>   	help
>> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
>> index 224df03ce42e..fc8c1249d49f 100644
>> --- a/drivers/xen/pci.c
>> +++ b/drivers/xen/pci.c
>> @@ -254,3 +254,78 @@ static int xen_mcfg_late(void)
>>   	return 0;
>>   }
>>   #endif
>> +
>> +#ifdef CONFIG_XEN_DOM0
>> +struct xen_device_domain_owner {
>> +	domid_t domain;
>> +	struct pci_dev *dev;
>> +	struct list_head list;
>> +};
>> +
>> +static DEFINE_SPINLOCK(dev_domain_list_spinlock);
>> +static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
>> +
>> +static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
>> +{
>> +	struct xen_device_domain_owner *owner;
>> +
>> +	list_for_each_entry(owner, &dev_domain_list, list) {
>> +		if (owner->dev == dev)
>> +			return owner;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +int xen_find_device_domain_owner(struct pci_dev *dev)
>> +{
>> +	struct xen_device_domain_owner *owner;
>> +	int domain = -ENODEV;
>> +
>> +	spin_lock(&dev_domain_list_spinlock);
>> +	owner = find_device(dev);
>> +	if (owner)
>> +		domain = owner->domain;
>> +	spin_unlock(&dev_domain_list_spinlock);
>> +	return domain;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
>> +
>> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
>> +{
>> +	struct xen_device_domain_owner *owner;
>> +
>> +	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
>> +	if (!owner)
>> +		return -ENODEV;
>> +
>> +	spin_lock(&dev_domain_list_spinlock);
>> +	if (find_device(dev)) {
>> +		spin_unlock(&dev_domain_list_spinlock);
>> +		kfree(owner);
>> +		return -EEXIST;
>> +	}
>> +	owner->domain = domain;
>> +	owner->dev = dev;
>> +	list_add_tail(&owner->list, &dev_domain_list);
>> +	spin_unlock(&dev_domain_list_spinlock);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
>> +
>> +int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> +{
>> +	struct xen_device_domain_owner *owner;
>> +
>> +	spin_lock(&dev_domain_list_spinlock);
>> +	owner = find_device(dev);
>> +	if (!owner) {
>> +		spin_unlock(&dev_domain_list_spinlock);
>> +		return -ENODEV;
>> +	}
>> +	list_del(&owner->list);
>> +	spin_unlock(&dev_domain_list_spinlock);
>> +	kfree(owner);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
>> +#endif
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index f8e4faa96ad6..bba527620507 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -19,7 +19,8 @@
>>   #include <linux/sched.h>
>>   #include <linux/atomic.h>
>>   #include <xen/events.h>
>> -#include <asm/xen/pci.h>
>> +#include <xen/pci.h>
>> +#include <xen/xen.h>
>>   #include <asm/xen/hypervisor.h>
>>   #include <xen/interface/physdev.h>
>>   #include "pciback.h"
>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>> index c09c7ebd6968..da34ce85dc88 100644
>> --- a/drivers/xen/xen-pciback/xenbus.c
>> +++ b/drivers/xen/xen-pciback/xenbus.c
>> @@ -14,7 +14,7 @@
>>   #include <linux/workqueue.h>
>>   #include <xen/xenbus.h>
>>   #include <xen/events.h>
>> -#include <asm/xen/pci.h>
>> +#include <xen/pci.h>
>>   #include "pciback.h"
>>   
>>   #define INVALID_EVTCHN_IRQ  (-1)
>> diff --git a/include/xen/pci.h b/include/xen/pci.h
>> new file mode 100644
>> index 000000000000..b8337cf85fd1
>> --- /dev/null
>> +++ b/include/xen/pci.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __XEN_PCI_H__
>> +#define __XEN_PCI_H__
>> +
>> +#if defined(CONFIG_XEN_DOM0)
>> +int xen_find_device_domain_owner(struct pci_dev *dev);
>> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
>> +int xen_unregister_device_domain_owner(struct pci_dev *dev);
>> +#else
>> +static inline int xen_find_device_domain_owner(struct pci_dev *dev)
>> +{
>> +	return -1;
>> +}
>> +
>> +static inline int xen_register_device_domain_owner(struct pci_dev *dev,
>> +						   uint16_t domain)
>> +{
>> +	return -1;
>> +}
>> +
>> +static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> +{
>> +	return -1;
>> +}
>> +#endif
>> +
>> +#endif
>> -- 
>> 2.25.1
>>

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

[Oleksandr Andrushchenko]

On 23.09.21 00:10, Stefano Stabellini wrote:
> On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> 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@epam.com>
> 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

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2348 bytes --]

On 23.09.21 11:02, Oleksandr Andrushchenko wrote:
> 
> On 23.09.21 00:10, Stefano Stabellini wrote:
>> On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
>>> --- 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?

I don't see how the backend could be running without being registered
with xenbus. It won't receive any watches, so there is no way a
connection with a frontend could be established.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Oleksandr Andrushchenko]

On 23.09.21 12:05, Juergen Gross wrote:
> On 23.09.21 11:02, Oleksandr Andrushchenko wrote:
>>
>> On 23.09.21 00:10, Stefano Stabellini wrote:
>>> On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
>>>> --- 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?
>
> I don't see how the backend could be running without being registered
> with xenbus. It won't receive any watches, so there is no way a
> connection with a frontend could be established.

This is my understanding too, so the only change I've put in patch I removed

register/unregister. It seems this is just enough and the patch should be ok as is

>
>
> Juergen
>
Thank you,

Oleksandr

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
> On 23.09.21 12:05, Juergen Gross wrote:
> > On 23.09.21 11:02, Oleksandr Andrushchenko wrote:
> >>
> >> On 23.09.21 00:10, Stefano Stabellini wrote:
> >>> On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
> >>>> --- 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?
> >
> > I don't see how the backend could be running without being registered
> > with xenbus. It won't receive any watches, so there is no way a
> > connection with a frontend could be established.
> 
> This is my understanding too, so the only change I've put in patch I removed
> 
> register/unregister. It seems this is just enough and the patch should be ok as is

Fantastic! Thanks for checking.

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
> On 23.09.21 12:05, Juergen Gross wrote:
> > On 23.09.21 11:02, Oleksandr Andrushchenko wrote:
> >>
> >> On 23.09.21 00:10, Stefano Stabellini wrote:
> >>> On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
> >>>> --- 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?
> >
> > I don't see how the backend could be running without being registered
> > with xenbus. It won't receive any watches, so there is no way a
> > connection with a frontend could be established.
> 
> This is my understanding too, so the only change I've put in patch I removed
> 
> register/unregister. It seems this is just enough and the patch should be ok as is

Fantastic! Thanks for checking.