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

[PATCH] 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.

Currently PCI backend implements multiple functionalities at a time,
such as:
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-virtualised use-cases support

The para-virtualised 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: Xen PCI stub and PCI PV backend
drivers.

For that add new configuration options CONFIG_XEN_PCI_STUB and
CONFIG_XEN_PCIDEV_STUB, so the driver can be limited in its
functionality, e.g. no support for para-virtualised scenario.
x86 platform will continue using CONFIG_XEN_PCIDEV_BACKEND for the
fully featured backend driver.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Juergen Gross <jgross@suse.com>
---
Changes since v5:
- rebased
Changes since v4:
- squashed two patches
- simplified Makefile
Changes since v3:
 - Fix >>32 shift for 32-bit architectures
Changes since v2:
 - swap the patch order
Since v1:
 - Do not move pci_xen_initial_domain as it is x86 specific
---
 arch/x86/include/asm/xen/pci.h              | 19 ------
 arch/x86/pci/xen.c                          | 76 +--------------------
 drivers/xen/Kconfig                         | 24 +++++++
 drivers/xen/Makefile                        |  2 +-
 drivers/xen/pci.c                           | 75 ++++++++++++++++++++
 drivers/xen/xen-pciback/Makefile            |  7 ++
 drivers/xen/xen-pciback/conf_space_header.c |  8 ++-
 drivers/xen/xen-pciback/pci_stub.c          |  3 +-
 drivers/xen/xen-pciback/pciback.h           |  5 ++
 drivers/xen/xen-pciback/xenbus.c            |  8 ++-
 include/xen/pci.h                           | 28 ++++++++
 11 files changed, 155 insertions(+), 100 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 4557f7cb0fa6..9015b888edd6 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -22,25 +22,6 @@ static inline int __init pci_xen_initial_domain(void)
 	return -1;
 }
 #endif
-#ifdef 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
 
 #if defined(CONFIG_PCI_MSI)
 #if defined(CONFIG_PCI_XEN)
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 5debe4ac6f81..12da00558631 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>
@@ -585,78 +586,3 @@ int __init pci_xen_initial_domain(void)
 }
 #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 /* CONFIG_XEN_DOM0 */
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 1b2c3aca6887..a1b11c62da9e 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -181,10 +181,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 && 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/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/Makefile b/drivers/xen/xen-pciback/Makefile
index e8d981d43235..d63df09de81c 100644
--- a/drivers/xen/xen-pciback/Makefile
+++ b/drivers/xen/xen-pciback/Makefile
@@ -1,5 +1,12 @@
 # SPDX-License-Identifier: GPL-2.0
+
+# N.B. The below cannot be expressed with a single line using
+# CONFIG_XEN_PCI_STUB as it always remains in "y" state,
+# thus preventing the driver to be built as a module.
+# Please note, that CONFIG_XEN_PCIDEV_BACKEND and
+# CONFIG_XEN_PCIDEV_STUB are mutually exclusive.
 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/conf_space_header.c b/drivers/xen/xen-pciback/conf_space_header.c
index ac45cdc38e85..981435103af1 100644
--- a/drivers/xen/xen-pciback/conf_space_header.c
+++ b/drivers/xen/xen-pciback/conf_space_header.c
@@ -236,8 +236,12 @@ static void *bar_init(struct pci_dev *dev, int offset)
 	else {
 		pos = (offset - PCI_BASE_ADDRESS_0) / 4;
 		if (pos && (res[pos - 1].flags & IORESOURCE_MEM_64)) {
-			bar->val = res[pos - 1].start >> 32;
-			bar->len_val = -resource_size(&res[pos - 1]) >> 32;
+			/*
+			 * Use ">> 16 >> 16" instead of direct ">> 32" shift
+			 * to avoid warnings on 32-bit architectures.
+			 */
+			bar->val = res[pos - 1].start >> 16 >> 16;
+			bar->len_val = -resource_size(&res[pos - 1]) >> 16 >> 16;
 			return bar;
 		}
 	}
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/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..bde63ef677b8 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)
@@ -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);
 }
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] xen-pciback: allow compiling on other archs than x86

[kernel test robot]

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

Hi Oleksandr,

I love your patch! Perhaps something to improve:

[auto build test WARNING on xen-tip/linux-next]
[also build test WARNING on linux/master linus/master v5.15-rc7 next-20211028]
[cannot apply to helgaas-pci/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Oleksandr-Andrushchenko/xen-pciback-allow-compiling-on-other-archs-than-x86/20211028-141523
base:   https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git linux-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/085a7279d0d85497688bc5be9a1f00ab2cce02ae
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Oleksandr-Andrushchenko/xen-pciback-allow-compiling-on-other-archs-than-x86/20211028-141523
        git checkout 085a7279d0d85497688bc5be9a1f00ab2cce02ae
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/xen/pci.c:279:5: warning: no previous prototype for 'xen_find_device_domain_owner' [-Wmissing-prototypes]
     279 | int xen_find_device_domain_owner(struct pci_dev *dev)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/xen/pci.c:293:5: warning: no previous prototype for 'xen_register_device_domain_owner' [-Wmissing-prototypes]
     293 | int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/xen/pci.c:315:5: warning: no previous prototype for 'xen_unregister_device_domain_owner' [-Wmissing-prototypes]
     315 | int xen_unregister_device_domain_owner(struct pci_dev *dev)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/xen_find_device_domain_owner +279 drivers/xen/pci.c

   278	
 > 279	int xen_find_device_domain_owner(struct pci_dev *dev)
   280	{
   281		struct xen_device_domain_owner *owner;
   282		int domain = -ENODEV;
   283	
   284		spin_lock(&dev_domain_list_spinlock);
   285		owner = find_device(dev);
   286		if (owner)
   287			domain = owner->domain;
   288		spin_unlock(&dev_domain_list_spinlock);
   289		return domain;
   290	}
   291	EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
   292	
 > 293	int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
   294	{
   295		struct xen_device_domain_owner *owner;
   296	
   297		owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
   298		if (!owner)
   299			return -ENODEV;
   300	
   301		spin_lock(&dev_domain_list_spinlock);
   302		if (find_device(dev)) {
   303			spin_unlock(&dev_domain_list_spinlock);
   304			kfree(owner);
   305			return -EEXIST;
   306		}
   307		owner->domain = domain;
   308		owner->dev = dev;
   309		list_add_tail(&owner->list, &dev_domain_list);
   310		spin_unlock(&dev_domain_list_spinlock);
   311		return 0;
   312	}
   313	EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
   314	
 > 315	int xen_unregister_device_domain_owner(struct pci_dev *dev)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 56219 bytes --]

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

[Oleksandr Andrushchenko]

Please ignore this patch in favor of "RESEND PATCH v6" due to
a warning reported by kernel test robot <lkp@intel.com>:

I love your patch! Perhaps something to improve:

[auto build test WARNING on xen-tip/linux-next]
[also build test WARNING on linux/master linus/master v5.15-rc7 next-20211028]
[cannot apply to helgaas-pci/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:https://github.com/0day-ci/linux/commits/Oleksandr-Andrushchenko/xen-pciback-allow-compiling-on-other-archs-than-x86/20211028-141523
base:https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git  linux-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
         wgethttps://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
         chmod +x ~/bin/make.cross
         #https://github.com/0day-ci/linux/commit/085a7279d0d85497688bc5be9a1f00ab2cce02ae
         git remote add linux-reviewhttps://github.com/0day-ci/linux
         git fetch --no-tags linux-review Oleksandr-Andrushchenko/xen-pciback-allow-compiling-on-other-archs-than-x86/20211028-141523
         git checkout 085a7279d0d85497688bc5be9a1f00ab2cce02ae
         # save the attached .config to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arm64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot<lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/xen/pci.c:279:5: warning: no previous prototype for 'xen_find_device_domain_owner' [-Wmissing-prototypes]

      279 | int xen_find_device_domain_owner(struct pci_dev *dev)
          |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

>> drivers/xen/pci.c:293:5: warning: no previous prototype for 'xen_register_device_domain_owner' [-Wmissing-prototypes]

      293 | int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
          |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

>> drivers/xen/pci.c:315:5: warning: no previous prototype for 'xen_unregister_device_domain_owner' [-Wmissing-prototypes]

      315 | int xen_unregister_device_domain_owner(struct pci_dev *dev)
          |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/xen_find_device_domain_owner +279 drivers/xen/pci.c

    278	
  > 279	int xen_find_device_domain_owner(struct pci_dev *dev)
    280	{
    281		struct xen_device_domain_owner *owner;
    282		int domain = -ENODEV;
    283	
    284		spin_lock(&dev_domain_list_spinlock);
    285		owner = find_device(dev);
    286		if (owner)
    287			domain = owner->domain;
    288		spin_unlock(&dev_domain_list_spinlock);
    289		return domain;
    290	}
    291	EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
    292	
  > 293	int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
    294	{
    295		struct xen_device_domain_owner *owner;
    296	
    297		owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
    298		if (!owner)
    299			return -ENODEV;
    300	
    301		spin_lock(&dev_domain_list_spinlock);
    302		if (find_device(dev)) {
    303			spin_unlock(&dev_domain_list_spinlock);
    304			kfree(owner);
    305			return -EEXIST;
    306		}
    307		owner->domain = domain;
    308		owner->dev = dev;
    309		list_add_tail(&owner->list, &dev_domain_list);
    310		spin_unlock(&dev_domain_list_spinlock);
    311		return 0;
    312	}
    313	EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
    314	
  > 315	int xen_unregister_device_domain_owner(struct pci_dev *dev)

On 28.10.21 09:13, 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.
>
> Currently PCI backend implements multiple functionalities at a time,
> such as:
> 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-virtualised use-cases support
>
> The para-virtualised 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: Xen PCI stub and PCI PV backend
> drivers.
>
> For that add new configuration options CONFIG_XEN_PCI_STUB and
> CONFIG_XEN_PCIDEV_STUB, so the driver can be limited in its
> functionality, e.g. no support for para-virtualised scenario.
> x86 platform will continue using CONFIG_XEN_PCIDEV_BACKEND for the
> fully featured backend driver.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Reviewed-by: Juergen Gross <jgross@suse.com>
> ---
> Changes since v5:
> - rebased
> Changes since v4:
> - squashed two patches
> - simplified Makefile
> Changes since v3:
>   - Fix >>32 shift for 32-bit architectures
> Changes since v2:
>   - swap the patch order
> Since v1:
>   - Do not move pci_xen_initial_domain as it is x86 specific
> ---
>   arch/x86/include/asm/xen/pci.h              | 19 ------
>   arch/x86/pci/xen.c                          | 76 +--------------------
>   drivers/xen/Kconfig                         | 24 +++++++
>   drivers/xen/Makefile                        |  2 +-
>   drivers/xen/pci.c                           | 75 ++++++++++++++++++++
>   drivers/xen/xen-pciback/Makefile            |  7 ++
>   drivers/xen/xen-pciback/conf_space_header.c |  8 ++-
>   drivers/xen/xen-pciback/pci_stub.c          |  3 +-
>   drivers/xen/xen-pciback/pciback.h           |  5 ++
>   drivers/xen/xen-pciback/xenbus.c            |  8 ++-
>   include/xen/pci.h                           | 28 ++++++++
>   11 files changed, 155 insertions(+), 100 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 4557f7cb0fa6..9015b888edd6 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -22,25 +22,6 @@ static inline int __init pci_xen_initial_domain(void)
>   	return -1;
>   }
>   #endif
> -#ifdef 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
>   
>   #if defined(CONFIG_PCI_MSI)
>   #if defined(CONFIG_PCI_XEN)
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 5debe4ac6f81..12da00558631 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>
> @@ -585,78 +586,3 @@ int __init pci_xen_initial_domain(void)
>   }
>   #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 /* CONFIG_XEN_DOM0 */
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index 1b2c3aca6887..a1b11c62da9e 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -181,10 +181,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 && 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/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/Makefile b/drivers/xen/xen-pciback/Makefile
> index e8d981d43235..d63df09de81c 100644
> --- a/drivers/xen/xen-pciback/Makefile
> +++ b/drivers/xen/xen-pciback/Makefile
> @@ -1,5 +1,12 @@
>   # SPDX-License-Identifier: GPL-2.0
> +
> +# N.B. The below cannot be expressed with a single line using
> +# CONFIG_XEN_PCI_STUB as it always remains in "y" state,
> +# thus preventing the driver to be built as a module.
> +# Please note, that CONFIG_XEN_PCIDEV_BACKEND and
> +# CONFIG_XEN_PCIDEV_STUB are mutually exclusive.
>   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/conf_space_header.c b/drivers/xen/xen-pciback/conf_space_header.c
> index ac45cdc38e85..981435103af1 100644
> --- a/drivers/xen/xen-pciback/conf_space_header.c
> +++ b/drivers/xen/xen-pciback/conf_space_header.c
> @@ -236,8 +236,12 @@ static void *bar_init(struct pci_dev *dev, int offset)
>   	else {
>   		pos = (offset - PCI_BASE_ADDRESS_0) / 4;
>   		if (pos && (res[pos - 1].flags & IORESOURCE_MEM_64)) {
> -			bar->val = res[pos - 1].start >> 32;
> -			bar->len_val = -resource_size(&res[pos - 1]) >> 32;
> +			/*
> +			 * Use ">> 16 >> 16" instead of direct ">> 32" shift
> +			 * to avoid warnings on 32-bit architectures.
> +			 */
> +			bar->val = res[pos - 1].start >> 16 >> 16;
> +			bar->len_val = -resource_size(&res[pos - 1]) >> 16 >> 16;
>   			return bar;
>   		}
>   	}
> 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/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..bde63ef677b8 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)
> @@ -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);
>   }
> 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

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

[Geert Uytterhoeven]

Hi Oleksandr,

On Thu, Oct 28, 2021 at 8:15 AM Oleksandr Andrushchenko
<andr2000@gmail.com> 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.
>
> Currently PCI backend implements multiple functionalities at a time,
> such as:
> 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-virtualised use-cases support
>
> The para-virtualised 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: Xen PCI stub and PCI PV backend
> drivers.
>
> For that add new configuration options CONFIG_XEN_PCI_STUB and
> CONFIG_XEN_PCIDEV_STUB, so the driver can be limited in its
> functionality, e.g. no support for para-virtualised scenario.
> x86 platform will continue using CONFIG_XEN_PCIDEV_BACKEND for the
> fully featured backend driver.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks for your patch, which is now commit a67efff28832a597
("xen-pciback: allow compiling on other archs than x86")
in v5.16-rc1.

> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -181,10 +181,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

Please note that this means "default y" if CONFIG_MODULES=n.
Perhaps this should be "default m if MODULES" instead?

> +       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 && X86 && XEN
>         depends on XEN_BACKEND
> +       select XEN_PCI_STUB
>         default m
>         help
>           The PCI device backend driver allows the kernel to export arbitrary

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Oleksandr Andrushchenko]

Hi, Geert!

On 16.11.21 11:36, Geert Uytterhoeven wrote:
> Hi Oleksandr,
>
> On Thu, Oct 28, 2021 at 8:15 AM Oleksandr Andrushchenko
> <andr2000@gmail.com> 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.
>>
>> Currently PCI backend implements multiple functionalities at a time,
>> such as:
>> 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-virtualised use-cases support
>>
>> The para-virtualised 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: Xen PCI stub and PCI PV backend
>> drivers.
>>
>> For that add new configuration options CONFIG_XEN_PCI_STUB and
>> CONFIG_XEN_PCIDEV_STUB, so the driver can be limited in its
>> functionality, e.g. no support for para-virtualised scenario.
>> x86 platform will continue using CONFIG_XEN_PCIDEV_BACKEND for the
>> fully featured backend driver.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> Thanks for your patch, which is now commit a67efff28832a597
> ("xen-pciback: allow compiling on other archs than x86")
> in v5.16-rc1.
>
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -181,10 +181,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
> Please note that this means "default y" if CONFIG_MODULES=n.
> Perhaps this should be "default m if MODULES" instead?
I don't really have strong opinion on this and will let Xen maintainers
speak: @Boris, @Juergen what's your preference here?
>
>> +       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 && X86 && XEN
>>          depends on XEN_BACKEND
>> +       select XEN_PCI_STUB
>>          default m
>>          help
>>            The PCI device backend driver allows the kernel to export arbitrary
> Gr{oetje,eeting}s,
>
>                          Geert
Thank you,
Oleksandr
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

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

[Juergen Gross]

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

On 16.11.21 11:45, Oleksandr Andrushchenko wrote:
> Hi, Geert!
> 
> On 16.11.21 11:36, Geert Uytterhoeven wrote:
>> Hi Oleksandr,
>>
>> On Thu, Oct 28, 2021 at 8:15 AM Oleksandr Andrushchenko
>> <andr2000@gmail.com> 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.
>>>
>>> Currently PCI backend implements multiple functionalities at a time,
>>> such as:
>>> 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-virtualised use-cases support
>>>
>>> The para-virtualised 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: Xen PCI stub and PCI PV backend
>>> drivers.
>>>
>>> For that add new configuration options CONFIG_XEN_PCI_STUB and
>>> CONFIG_XEN_PCIDEV_STUB, so the driver can be limited in its
>>> functionality, e.g. no support for para-virtualised scenario.
>>> x86 platform will continue using CONFIG_XEN_PCIDEV_BACKEND for the
>>> fully featured backend driver.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>> Thanks for your patch, which is now commit a67efff28832a597
>> ("xen-pciback: allow compiling on other archs than x86")
>> in v5.16-rc1.
>>
>>> --- a/drivers/xen/Kconfig
>>> +++ b/drivers/xen/Kconfig
>>> @@ -181,10 +181,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
>> Please note that this means "default y" if CONFIG_MODULES=n.
>> Perhaps this should be "default m if MODULES" instead?
> I don't really have strong opinion on this and will let Xen maintainers
> speak: @Boris, @Juergen what's your preference here?

TBH, I don't have any.

All other backends have no "default" line, so maybe the cleanest
solution would be to remove the "default" lines for XEN_PCIDEV_STUB
and XEN_PCIDEV_BACKEND, too?

>>
>>> +       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 && X86 && XEN
>>>           depends on XEN_BACKEND
>>> +       select XEN_PCI_STUB
>>>           default m
>>>           help
>>>             The PCI device backend driver allows the kernel to export arbitrary


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] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

On 21.09.21 23:44, Stefano Stabellini wrote:
> On Tue, 21 Sep 2021, Oleksandr Andrushchenko wrote:
>> On 21.09.21 10:09, Juergen Gross wrote:
>>> On 21.09.21 09:00, Oleksandr Andrushchenko wrote:
>>>> On 21.09.21 09:49, Juergen Gross wrote:
>>>>> On 21.09.21 08:38, Oleksandr Andrushchenko wrote:
>>>>>> On 21.09.21 09:07, Juergen Gross wrote:
>>>>>>> On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
>>>>>>>> On 21.09.21 08:20, Juergen Gross wrote:
>>>>>>>>> On 21.09.21 01:16, Stefano Stabellini wrote:
>>>>>>>>>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>>>>>>>>>>> On 20.09.21 14:30, Juergen Gross wrote:
>>>>>>>>>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>>>>>>>>>>> Hello, Stefano!
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>>>>>>>>>>> Hi Oleksandr,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>>>>>>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>>>>>>>>>>> Not only that
>>>>>>>>>>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>>>>>>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>>>>>>>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>>>>>>>>>>> the same time.
>>>>>>>>>>>>> Correct, it is not used
>>>>>>>>>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>>>>>>>>>>> possible and better to use pci-stub instead?
>>>>>>>>>>>>> Not only that, so pci-stub is not enough
>>>>>>>>>>>>>
>>>>>>>>>>>>> The functionality which is implemented by the pciback and the toolstack
>>>>>>>>>>>>> and which is relevant/missing/needed for ARM:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>>>>>>>>>>
>>>>>>>>>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>>>>>>>>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Please see [1] and [2]:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>>>>>>>>>>> the rest like vPCI etc.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 3. pcifront is not used on Arm
>>>>>>>>>>>> It is neither in x86 PVH/HVM guests.
>>>>>>>>>>> Didn't know that, thank you for pointing
>>>>>>>>>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>>>>>>>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>>>>>>>>>>> all the goals above.
>>>>>>>>>>>>>
>>>>>>>>>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>>>>>>>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>>>>>>>>>>> Doing this split should be done, as the pcifront specific part could be
>>>>>>>>>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>>>>>>>>>>> be supported.
>>>>>>>>>>> Agree, that the final solution should have the driver split
>>>>>>>>>>>>> So, I think this patch is still going to be needed besides which direction we take.
>>>>>>>>>>>> Some kind of this patch, yes. It might look different in case the split
>>>>>>>>>>>> is done first.
>>>>>>>>>>>>
>>>>>>>>>>>> I don't mind doing it in either sequence.
>>>>>>>>>>>>
>>>>>>>>>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>>>>>>>>>>
>>>>>>>>>>> e.g. the driver has unused code, but yet allows Arm to function now.
>>>>>>>>>>>
>>>>>>>>>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>>>>>>>>>>
>>>>>>>>>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>>>>>>>>>>
>>>>>>>>>>> have enough bandwidth for that piece of work at the moment.
>>>>>>>>>> That's fair and I don't want to scope-creep this simple patch asking for
>>>>>>>>>> an enormous rework. At the same time I don't think we should enable the
>>>>>>>>>> whole of pciback on ARM because it would be erroneous and confusing.
>>>>>>>> As the first stage before the driver is split or ifdef's used - can we take the patch
>>>>>>>> as is now? In either way we chose this needs to be done, e.g. enable compiling
>>>>>>>> for other architectures and common code move.
>>>>>>> Fine with me in principle. I need to take a more thorough look
>>>>>>> at the patch, though.
>>>>>> Of course
>>>>>>>>>> I am wonder if there is a simple:
>>>>>>>>>>
>>>>>>>>>> if (!xen_pv_domain())
>>>>>>>>>>          return;
>>>>>>>>>>
>>>>>>>>>> That we could add in a couple of places in pciback to stop it from
>>>>>>>>>> initializing the parts we don't care about. Something along these lines
>>>>>>>>>> (untested and probably incomplete).
>>>>>>>>>>
>>>>>>>>>> What do you guys think?
>>>>>>>>> Uh no, not in this way, please. This will kill pci passthrough on x86
>>>>>>>>> with dom0 running as PVH. I don't think this is working right now, but
>>>>>>>>> adding more code making it even harder to work should be avoided.
>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>>>>>>>>> index da34ce85dc88..991ba0a9b359 100644
>>>>>>>>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>>>>>>>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>>>>>>>>> @@ -15,6 +15,7 @@
>>>>>>>>>>       #include <xen/xenbus.h>
>>>>>>>>>>       #include <xen/events.h>
>>>>>>>>>>       #include <xen/pci.h>
>>>>>>>>>> +#include <xen/xen.h>
>>>>>>>>>>       #include "pciback.h"
>>>>>>>>>>         #define INVALID_EVTCHN_IRQ  (-1)
>>>>>>>>>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>>>>>>>>>>                       const struct xenbus_device_id *id)
>>>>>>>>>>       {
>>>>>>>>>>           int err = 0;
>>>>>>>>>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
>>>>>>>>>> +    struct xen_pcibk_device *pdev;
>>>>>>>>>> +
>>>>>>>>>> +    if (!xen_pv_domain())
>>>>>>>>>> +        return 0;
>>>>>>>>>>       +    pdev = alloc_pdev(dev);
>>>>>>>>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
>>>>>>>>> early will result in xen_pcibk_xenbus_probe never being called.
>>>>>>>>>
>>>>>>>>>>           if (pdev == NULL) {
>>>>>>>>>>               err = -ENOMEM;
>>>>>>>>>>               xenbus_dev_fatal(dev, err,
>>>>>>>>>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>>>>>>>>>>         int __init xen_pcibk_xenbus_register(void)
>>>>>>>>>>       {
>>>>>>>>>> +    if (!xen_pv_domain())
>>>>>>>>>> +        return 0;
>>>>>>>>>> +
>>>>>>>>> Use #ifdef CONFIG_X86 instead.
>>>>>>>> The title of this patch says that we want to allow this driver for other archs
>>>>>>>> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
>>>>>>>> right with that respect. Instead, we may want having something like a
>>>>>>>> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
>>>>>>>> or something which is architecture agnostic.
>>>>>>> Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
>>>>>>> acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
>>>>>>> functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
>>>>>>> select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
>>>>>>> wanted. The splitting of the driver can still be done later.
>>>>>> Hm, pciback is now compiled when CONFIG_XEN_PCIDEV_BACKEND is enabled
>>>>>> and we want to skip some parts of its code when CONFIG_XEN_PCI_STUB is set.
>>>>>> So, I imagine that for x86 we just enable CONFIG_XEN_PCIDEV_BACKEND and the
>>>>>> driver compiles in its current state. For Arm we enable both CONFIG_XEN_PCIDEV_BACKEND
>>>>>> and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.
>>>>> No, I'd rather switch to compiling xen-pciback when CONFIG_XEN_PCI_STUB
>>>>> is set and compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is
>>>>> not set (this will be the case on Arm).
>>>> But this will require that the existing kernel configurations out there have to additionally define CONFIG_XEN_PCI_STUB to get what they had before with simply enabling CONFIG_XEN_PCIDEV_BACKEND. My point was that it is probably desirable not to break
>>>> the things while doing the split/re-work.
>>> By letting XEN_PCIDEV_BACKEND select XEN_PCI_STUB this won't happen.
>> Indeed
>>>> I also thought that "compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is not set"
>>>> may have more code gated rather than with gating unwanted code with CONFIG_XEN_PCI_STUB.
>>>> I am not quite sure about this though.
>>> This would be a very weird semantics of XEN_PCI_STUB, as the stub part
>>> is needed on X86 and on Arm.
>>>
>>> Gating could even be done with Stefano's patch just by replacing his
>>> "!xen_pv_domain()" tests with "!IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND)".
>> Makes sense.
>>
>> Another question if we do not want the code to be compiled or not executed?
>>
>> If the later then we can define something like:
>>
>> bool need_pv_part(void)
>>
>> {
>>
>>       return IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND);
>>
>> }
>>
>> and then just use need_pv_part() for the checks where it is needed.
>>
>> This allows avoiding multiple ifdef's through the code
> This is even better.
>
> For my clarity, Oleksandr, are you OK with adding a few need_pv_part()
> checks through the code as part of this series so that the PV PCI
> backend is not initialized?
Yes
>
> I don't have a good test environment ready for this, so I cannot really
> volunteer myself.
>
> I would prefer if we made this change as part of this series so that the
> PV PCI backend features doesn't get enabled on ARM, not even temporarily.
Ok, I will push v2 today with the additional patch for PV

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

[Stefano Stabellini]

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

On Tue, 21 Sep 2021, Oleksandr Andrushchenko wrote:
> On 21.09.21 10:09, Juergen Gross wrote:
> > On 21.09.21 09:00, Oleksandr Andrushchenko wrote:
> >>
> >> On 21.09.21 09:49, Juergen Gross wrote:
> >>> On 21.09.21 08:38, Oleksandr Andrushchenko wrote:
> >>>>
> >>>> On 21.09.21 09:07, Juergen Gross wrote:
> >>>>> On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
> >>>>>>
> >>>>>> On 21.09.21 08:20, Juergen Gross wrote:
> >>>>>>> On 21.09.21 01:16, Stefano Stabellini wrote:
> >>>>>>>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
> >>>>>>>>> On 20.09.21 14:30, Juergen Gross wrote:
> >>>>>>>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
> >>>>>>>>>>> Hello, Stefano!
> >>>>>>>>>>>
> >>>>>>>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
> >>>>>>>>>>>> Hi Oleksandr,
> >>>>>>>>>>>>
> >>>>>>>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
> >>>>>>>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
> >>>>>>>>>>> Not only that
> >>>>>>>>>>>>
> >>>>>>>>>>>> I am asking because actually I don't think we want to enable the PV PCI
> >>>>>>>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
> >>>>>>>>>>>> assignment work you have been doing in Xen. They couldn't both work at
> >>>>>>>>>>>> the same time.
> >>>>>>>>>>> Correct, it is not used
> >>>>>>>>>>>>
> >>>>>>>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
> >>>>>>>>>>>> possible and better to use pci-stub instead?
> >>>>>>>>>>>
> >>>>>>>>>>> Not only that, so pci-stub is not enough
> >>>>>>>>>>>
> >>>>>>>>>>> The functionality which is implemented by the pciback and the toolstack
> >>>>>>>>>>> and which is relevant/missing/needed for ARM:
> >>>>>>>>>>>
> >>>>>>>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
> >>>>>>>>>>>
> >>>>>>>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
> >>>>>>>>>>> above we see that pciback functionality is going to be only partially used on Arm.
> >>>>>>>>>>>
> >>>>>>>>>>> Please see [1] and [2]:
> >>>>>>>>>>>
> >>>>>>>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
> >>>>>>>>>>>
> >>>>>>>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
> >>>>>>>>>>> the rest like vPCI etc.
> >>>>>>>>>>>
> >>>>>>>>>>> 3. pcifront is not used on Arm
> >>>>>>>>>>
> >>>>>>>>>> It is neither in x86 PVH/HVM guests.
> >>>>>>>>> Didn't know that, thank you for pointing
> >>>>>>>>>>
> >>>>>>>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
> >>>>>>>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
> >>>>>>>>>>> all the goals above.
> >>>>>>>>>>>
> >>>>>>>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
> >>>>>>>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
> >>>>>>>>>>
> >>>>>>>>>> Doing this split should be done, as the pcifront specific part could be
> >>>>>>>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
> >>>>>>>>>> be supported.
> >>>>>>>>> Agree, that the final solution should have the driver split
> >>>>>>>>>>
> >>>>>>>>>>> So, I think this patch is still going to be needed besides which direction we take.
> >>>>>>>>>>
> >>>>>>>>>> Some kind of this patch, yes. It might look different in case the split
> >>>>>>>>>> is done first.
> >>>>>>>>>>
> >>>>>>>>>> I don't mind doing it in either sequence.
> >>>>>>>>>>
> >>>>>>>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
> >>>>>>>>>
> >>>>>>>>> e.g. the driver has unused code, but yet allows Arm to function now.
> >>>>>>>>>
> >>>>>>>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
> >>>>>>>>>
> >>>>>>>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
> >>>>>>>>>
> >>>>>>>>> have enough bandwidth for that piece of work at the moment.
> >>>>>>>>
> >>>>>>>> That's fair and I don't want to scope-creep this simple patch asking for
> >>>>>>>> an enormous rework. At the same time I don't think we should enable the
> >>>>>>>> whole of pciback on ARM because it would be erroneous and confusing.
> >>>>>>
> >>>>>> As the first stage before the driver is split or ifdef's used - can we take the patch
> >>>>>> as is now? In either way we chose this needs to be done, e.g. enable compiling
> >>>>>> for other architectures and common code move.
> >>>>>
> >>>>> Fine with me in principle. I need to take a more thorough look
> >>>>> at the patch, though.
> >>>> Of course
> >>>>>
> >>>>>>
> >>>>>>>>
> >>>>>>>> I am wonder if there is a simple:
> >>>>>>>>
> >>>>>>>> if (!xen_pv_domain())
> >>>>>>>>         return;
> >>>>>>>>
> >>>>>>>> That we could add in a couple of places in pciback to stop it from
> >>>>>>>> initializing the parts we don't care about. Something along these lines
> >>>>>>>> (untested and probably incomplete).
> >>>>>>>>
> >>>>>>>> What do you guys think?
> >>>>>>>
> >>>>>>> Uh no, not in this way, please. This will kill pci passthrough on x86
> >>>>>>> with dom0 running as PVH. I don't think this is working right now, but
> >>>>>>> adding more code making it even harder to work should be avoided.
> >>>>>>>
> >>>>>>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> >>>>>>>> index da34ce85dc88..991ba0a9b359 100644
> >>>>>>>> --- a/drivers/xen/xen-pciback/xenbus.c
> >>>>>>>> +++ b/drivers/xen/xen-pciback/xenbus.c
> >>>>>>>> @@ -15,6 +15,7 @@
> >>>>>>>>      #include <xen/xenbus.h>
> >>>>>>>>      #include <xen/events.h>
> >>>>>>>>      #include <xen/pci.h>
> >>>>>>>> +#include <xen/xen.h>
> >>>>>>>>      #include "pciback.h"
> >>>>>>>>        #define INVALID_EVTCHN_IRQ  (-1)
> >>>>>>>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
> >>>>>>>>                      const struct xenbus_device_id *id)
> >>>>>>>>      {
> >>>>>>>>          int err = 0;
> >>>>>>>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
> >>>>>>>> +    struct xen_pcibk_device *pdev;
> >>>>>>>> +
> >>>>>>>> +    if (!xen_pv_domain())
> >>>>>>>> +        return 0;
> >>>>>>>>      +    pdev = alloc_pdev(dev);
> >>>>>>>
> >>>>>>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
> >>>>>>> early will result in xen_pcibk_xenbus_probe never being called.
> >>>>>>>
> >>>>>>>>          if (pdev == NULL) {
> >>>>>>>>              err = -ENOMEM;
> >>>>>>>>              xenbus_dev_fatal(dev, err,
> >>>>>>>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
> >>>>>>>>        int __init xen_pcibk_xenbus_register(void)
> >>>>>>>>      {
> >>>>>>>> +    if (!xen_pv_domain())
> >>>>>>>> +        return 0;
> >>>>>>>> +
> >>>>>>>
> >>>>>>> Use #ifdef CONFIG_X86 instead.
> >>>>>>
> >>>>>> The title of this patch says that we want to allow this driver for other archs
> >>>>>> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
> >>>>>> right with that respect. Instead, we may want having something like a
> >>>>>> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
> >>>>>> or something which is architecture agnostic.
> >>>>>
> >>>>> Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
> >>>>> acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
> >>>>> functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
> >>>>> select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
> >>>>> wanted. The splitting of the driver can still be done later.
> >>>>
> >>>> Hm, pciback is now compiled when CONFIG_XEN_PCIDEV_BACKEND is enabled
> >>>> and we want to skip some parts of its code when CONFIG_XEN_PCI_STUB is set.
> >>>> So, I imagine that for x86 we just enable CONFIG_XEN_PCIDEV_BACKEND and the
> >>>> driver compiles in its current state. For Arm we enable both CONFIG_XEN_PCIDEV_BACKEND
> >>>> and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.
> >>>
> >>> No, I'd rather switch to compiling xen-pciback when CONFIG_XEN_PCI_STUB
> >>> is set and compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is
> >>> not set (this will be the case on Arm).
> >>
> >> But this will require that the existing kernel configurations out there have to additionally define CONFIG_XEN_PCI_STUB to get what they had before with simply enabling CONFIG_XEN_PCIDEV_BACKEND. My point was that it is probably desirable not to break
> >> the things while doing the split/re-work.
> >
> > By letting XEN_PCIDEV_BACKEND select XEN_PCI_STUB this won't happen.
> Indeed
> >
> >> I also thought that "compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is not set"
> >> may have more code gated rather than with gating unwanted code with CONFIG_XEN_PCI_STUB.
> >> I am not quite sure about this though.
> >
> > This would be a very weird semantics of XEN_PCI_STUB, as the stub part
> > is needed on X86 and on Arm.
> >
> > Gating could even be done with Stefano's patch just by replacing his
> > "!xen_pv_domain()" tests with "!IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND)".
> 
> Makes sense.
> 
> Another question if we do not want the code to be compiled or not executed?
> 
> If the later then we can define something like:
> 
> bool need_pv_part(void)
> 
> {
> 
>      return IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND);
> 
> }
> 
> and then just use need_pv_part() for the checks where it is needed.
> 
> This allows avoiding multiple ifdef's through the code

This is even better.

For my clarity, Oleksandr, are you OK with adding a few need_pv_part()
checks through the code as part of this series so that the PV PCI
backend is not initialized?

I don't have a good test environment ready for this, so I cannot really
volunteer myself.

I would prefer if we made this change as part of this series so that the
PV PCI backend features doesn't get enabled on ARM, not even temporarily.

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

[Stefano Stabellini]

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

On Tue, 21 Sep 2021, Oleksandr Andrushchenko wrote:
> On 21.09.21 10:09, Juergen Gross wrote:
> > On 21.09.21 09:00, Oleksandr Andrushchenko wrote:
> >>
> >> On 21.09.21 09:49, Juergen Gross wrote:
> >>> On 21.09.21 08:38, Oleksandr Andrushchenko wrote:
> >>>>
> >>>> On 21.09.21 09:07, Juergen Gross wrote:
> >>>>> On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
> >>>>>>
> >>>>>> On 21.09.21 08:20, Juergen Gross wrote:
> >>>>>>> On 21.09.21 01:16, Stefano Stabellini wrote:
> >>>>>>>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
> >>>>>>>>> On 20.09.21 14:30, Juergen Gross wrote:
> >>>>>>>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
> >>>>>>>>>>> Hello, Stefano!
> >>>>>>>>>>>
> >>>>>>>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
> >>>>>>>>>>>> Hi Oleksandr,
> >>>>>>>>>>>>
> >>>>>>>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
> >>>>>>>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
> >>>>>>>>>>> Not only that
> >>>>>>>>>>>>
> >>>>>>>>>>>> I am asking because actually I don't think we want to enable the PV PCI
> >>>>>>>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
> >>>>>>>>>>>> assignment work you have been doing in Xen. They couldn't both work at
> >>>>>>>>>>>> the same time.
> >>>>>>>>>>> Correct, it is not used
> >>>>>>>>>>>>
> >>>>>>>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
> >>>>>>>>>>>> possible and better to use pci-stub instead?
> >>>>>>>>>>>
> >>>>>>>>>>> Not only that, so pci-stub is not enough
> >>>>>>>>>>>
> >>>>>>>>>>> The functionality which is implemented by the pciback and the toolstack
> >>>>>>>>>>> and which is relevant/missing/needed for ARM:
> >>>>>>>>>>>
> >>>>>>>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
> >>>>>>>>>>>
> >>>>>>>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
> >>>>>>>>>>> above we see that pciback functionality is going to be only partially used on Arm.
> >>>>>>>>>>>
> >>>>>>>>>>> Please see [1] and [2]:
> >>>>>>>>>>>
> >>>>>>>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
> >>>>>>>>>>>
> >>>>>>>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
> >>>>>>>>>>> the rest like vPCI etc.
> >>>>>>>>>>>
> >>>>>>>>>>> 3. pcifront is not used on Arm
> >>>>>>>>>>
> >>>>>>>>>> It is neither in x86 PVH/HVM guests.
> >>>>>>>>> Didn't know that, thank you for pointing
> >>>>>>>>>>
> >>>>>>>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
> >>>>>>>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
> >>>>>>>>>>> all the goals above.
> >>>>>>>>>>>
> >>>>>>>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
> >>>>>>>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
> >>>>>>>>>>
> >>>>>>>>>> Doing this split should be done, as the pcifront specific part could be
> >>>>>>>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
> >>>>>>>>>> be supported.
> >>>>>>>>> Agree, that the final solution should have the driver split
> >>>>>>>>>>
> >>>>>>>>>>> So, I think this patch is still going to be needed besides which direction we take.
> >>>>>>>>>>
> >>>>>>>>>> Some kind of this patch, yes. It might look different in case the split
> >>>>>>>>>> is done first.
> >>>>>>>>>>
> >>>>>>>>>> I don't mind doing it in either sequence.
> >>>>>>>>>>
> >>>>>>>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
> >>>>>>>>>
> >>>>>>>>> e.g. the driver has unused code, but yet allows Arm to function now.
> >>>>>>>>>
> >>>>>>>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
> >>>>>>>>>
> >>>>>>>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
> >>>>>>>>>
> >>>>>>>>> have enough bandwidth for that piece of work at the moment.
> >>>>>>>>
> >>>>>>>> That's fair and I don't want to scope-creep this simple patch asking for
> >>>>>>>> an enormous rework. At the same time I don't think we should enable the
> >>>>>>>> whole of pciback on ARM because it would be erroneous and confusing.
> >>>>>>
> >>>>>> As the first stage before the driver is split or ifdef's used - can we take the patch
> >>>>>> as is now? In either way we chose this needs to be done, e.g. enable compiling
> >>>>>> for other architectures and common code move.
> >>>>>
> >>>>> Fine with me in principle. I need to take a more thorough look
> >>>>> at the patch, though.
> >>>> Of course
> >>>>>
> >>>>>>
> >>>>>>>>
> >>>>>>>> I am wonder if there is a simple:
> >>>>>>>>
> >>>>>>>> if (!xen_pv_domain())
> >>>>>>>>         return;
> >>>>>>>>
> >>>>>>>> That we could add in a couple of places in pciback to stop it from
> >>>>>>>> initializing the parts we don't care about. Something along these lines
> >>>>>>>> (untested and probably incomplete).
> >>>>>>>>
> >>>>>>>> What do you guys think?
> >>>>>>>
> >>>>>>> Uh no, not in this way, please. This will kill pci passthrough on x86
> >>>>>>> with dom0 running as PVH. I don't think this is working right now, but
> >>>>>>> adding more code making it even harder to work should be avoided.
> >>>>>>>
> >>>>>>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> >>>>>>>> index da34ce85dc88..991ba0a9b359 100644
> >>>>>>>> --- a/drivers/xen/xen-pciback/xenbus.c
> >>>>>>>> +++ b/drivers/xen/xen-pciback/xenbus.c
> >>>>>>>> @@ -15,6 +15,7 @@
> >>>>>>>>      #include <xen/xenbus.h>
> >>>>>>>>      #include <xen/events.h>
> >>>>>>>>      #include <xen/pci.h>
> >>>>>>>> +#include <xen/xen.h>
> >>>>>>>>      #include "pciback.h"
> >>>>>>>>        #define INVALID_EVTCHN_IRQ  (-1)
> >>>>>>>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
> >>>>>>>>                      const struct xenbus_device_id *id)
> >>>>>>>>      {
> >>>>>>>>          int err = 0;
> >>>>>>>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
> >>>>>>>> +    struct xen_pcibk_device *pdev;
> >>>>>>>> +
> >>>>>>>> +    if (!xen_pv_domain())
> >>>>>>>> +        return 0;
> >>>>>>>>      +    pdev = alloc_pdev(dev);
> >>>>>>>
> >>>>>>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
> >>>>>>> early will result in xen_pcibk_xenbus_probe never being called.
> >>>>>>>
> >>>>>>>>          if (pdev == NULL) {
> >>>>>>>>              err = -ENOMEM;
> >>>>>>>>              xenbus_dev_fatal(dev, err,
> >>>>>>>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
> >>>>>>>>        int __init xen_pcibk_xenbus_register(void)
> >>>>>>>>      {
> >>>>>>>> +    if (!xen_pv_domain())
> >>>>>>>> +        return 0;
> >>>>>>>> +
> >>>>>>>
> >>>>>>> Use #ifdef CONFIG_X86 instead.
> >>>>>>
> >>>>>> The title of this patch says that we want to allow this driver for other archs
> >>>>>> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
> >>>>>> right with that respect. Instead, we may want having something like a
> >>>>>> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
> >>>>>> or something which is architecture agnostic.
> >>>>>
> >>>>> Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
> >>>>> acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
> >>>>> functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
> >>>>> select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
> >>>>> wanted. The splitting of the driver can still be done later.
> >>>>
> >>>> Hm, pciback is now compiled when CONFIG_XEN_PCIDEV_BACKEND is enabled
> >>>> and we want to skip some parts of its code when CONFIG_XEN_PCI_STUB is set.
> >>>> So, I imagine that for x86 we just enable CONFIG_XEN_PCIDEV_BACKEND and the
> >>>> driver compiles in its current state. For Arm we enable both CONFIG_XEN_PCIDEV_BACKEND
> >>>> and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.
> >>>
> >>> No, I'd rather switch to compiling xen-pciback when CONFIG_XEN_PCI_STUB
> >>> is set and compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is
> >>> not set (this will be the case on Arm).
> >>
> >> But this will require that the existing kernel configurations out there have to additionally define CONFIG_XEN_PCI_STUB to get what they had before with simply enabling CONFIG_XEN_PCIDEV_BACKEND. My point was that it is probably desirable not to break
> >> the things while doing the split/re-work.
> >
> > By letting XEN_PCIDEV_BACKEND select XEN_PCI_STUB this won't happen.
> Indeed
> >
> >> I also thought that "compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is not set"
> >> may have more code gated rather than with gating unwanted code with CONFIG_XEN_PCI_STUB.
> >> I am not quite sure about this though.
> >
> > This would be a very weird semantics of XEN_PCI_STUB, as the stub part
> > is needed on X86 and on Arm.
> >
> > Gating could even be done with Stefano's patch just by replacing his
> > "!xen_pv_domain()" tests with "!IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND)".
> 
> Makes sense.
> 
> Another question if we do not want the code to be compiled or not executed?
> 
> If the later then we can define something like:
> 
> bool need_pv_part(void)
> 
> {
> 
>      return IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND);
> 
> }
> 
> and then just use need_pv_part() for the checks where it is needed.
> 
> This allows avoiding multiple ifdef's through the code

This is even better.

For my clarity, Oleksandr, are you OK with adding a few need_pv_part()
checks through the code as part of this series so that the PV PCI
backend is not initialized?

I don't have a good test environment ready for this, so I cannot really
volunteer myself.

I would prefer if we made this change as part of this series so that the
PV PCI backend features doesn't get enabled on ARM, not even temporarily.

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

[Oleksandr Andrushchenko]

On 21.09.21 10:54, Juergen Gross wrote:
> On 17.09.21 15:01, 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>
>>
>> ---
>> Tested on Arm and x86.
>> ---
>>   arch/x86/include/asm/xen/pci.h     | 24 ----------
>>   arch/x86/pci/xen.c                 | 74 +----------------------------
>>   drivers/xen/Kconfig                |  2 +-
>>   drivers/xen/events/events_base.c   |  1 +
>>   drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>>   drivers/xen/xen-pciback/pci_stub.c |  3 +-
>>   drivers/xen/xen-pciback/xenbus.c   |  2 +-
>>   include/xen/pci.h                  | 34 ++++++++++++++
>>   8 files changed, 115 insertions(+), 100 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..9ff7b49bca08 100644
>> --- a/arch/x86/include/asm/xen/pci.h
>> +++ b/arch/x86/include/asm/xen/pci.h
>> @@ -14,30 +14,6 @@ static inline int pci_xen_hvm_init(void)
>>       return -1;
>>   }
>>   #endif
>> -#if defined(CONFIG_XEN_DOM0)
>> -int __init pci_xen_initial_domain(void);
>
> Why are you removing this prototype? It is X86 specific.
Indeed it is. Will not remove it form x86 code
>
>> -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;
>> -}
>> -#endif
>>     #if defined(CONFIG_PCI_MSI)
>>   #if defined(CONFIG_PCI_XEN)
>> 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/events/events_base.c b/drivers/xen/events/events_base.c
>> index a78704ae3618..35493ff0d146 100644
>> --- a/drivers/xen/events/events_base.c
>> +++ b/drivers/xen/events/events_base.c
>> @@ -65,6 +65,7 @@
>>   #include <xen/interface/vcpu.h>
>>   #include <xen/xenbus.h>
>>   #include <asm/hw_irq.h>
>> +#include <xen/pci.h>
>
> This wouldn't be needed if you'd let the pci_xen_initial_domain()
> prototype where it has been.
Sure, will leave it where it was
>
>
> Juergen

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

[Juergen Gross]

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

On 17.09.21 15:01, 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>
> 
> ---
> Tested on Arm and x86.
> ---
>   arch/x86/include/asm/xen/pci.h     | 24 ----------
>   arch/x86/pci/xen.c                 | 74 +----------------------------
>   drivers/xen/Kconfig                |  2 +-
>   drivers/xen/events/events_base.c   |  1 +
>   drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>   drivers/xen/xen-pciback/pci_stub.c |  3 +-
>   drivers/xen/xen-pciback/xenbus.c   |  2 +-
>   include/xen/pci.h                  | 34 ++++++++++++++
>   8 files changed, 115 insertions(+), 100 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..9ff7b49bca08 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -14,30 +14,6 @@ static inline int pci_xen_hvm_init(void)
>   	return -1;
>   }
>   #endif
> -#if defined(CONFIG_XEN_DOM0)
> -int __init pci_xen_initial_domain(void);

Why are you removing this prototype? It is X86 specific.

> -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;
> -}
> -#endif
>   
>   #if defined(CONFIG_PCI_MSI)
>   #if defined(CONFIG_PCI_XEN)
> 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/events/events_base.c b/drivers/xen/events/events_base.c
> index a78704ae3618..35493ff0d146 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -65,6 +65,7 @@
>   #include <xen/interface/vcpu.h>
>   #include <xen/xenbus.h>
>   #include <asm/hw_irq.h>
> +#include <xen/pci.h>

This wouldn't be needed if you'd let the pci_xen_initial_domain()
prototype where it has been.


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] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

On 21.09.21 10:09, Juergen Gross wrote:
> On 21.09.21 09:00, Oleksandr Andrushchenko wrote:
>>
>> On 21.09.21 09:49, Juergen Gross wrote:
>>> On 21.09.21 08:38, Oleksandr Andrushchenko wrote:
>>>>
>>>> On 21.09.21 09:07, Juergen Gross wrote:
>>>>> On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
>>>>>>
>>>>>> On 21.09.21 08:20, Juergen Gross wrote:
>>>>>>> On 21.09.21 01:16, Stefano Stabellini wrote:
>>>>>>>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 20.09.21 14:30, Juergen Gross wrote:
>>>>>>>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>>>>>>>>> Hello, Stefano!
>>>>>>>>>>>
>>>>>>>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>>>>>>>>> Hi Oleksandr,
>>>>>>>>>>>>
>>>>>>>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>>>>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>>>>>>>>> Not only that
>>>>>>>>>>>>
>>>>>>>>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>>>>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>>>>>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>>>>>>>>> the same time.
>>>>>>>>>>> Correct, it is not used
>>>>>>>>>>>>
>>>>>>>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>>>>>>>>> possible and better to use pci-stub instead?
>>>>>>>>>>>
>>>>>>>>>>> Not only that, so pci-stub is not enough
>>>>>>>>>>>
>>>>>>>>>>> The functionality which is implemented by the pciback and the toolstack
>>>>>>>>>>> and which is relevant/missing/needed for ARM:
>>>>>>>>>>>
>>>>>>>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>>>>>>>>
>>>>>>>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>>>>>>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>>>>>>>>
>>>>>>>>>>> Please see [1] and [2]:
>>>>>>>>>>>
>>>>>>>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>>>>>>>>
>>>>>>>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>>>>>>>>> the rest like vPCI etc.
>>>>>>>>>>>
>>>>>>>>>>> 3. pcifront is not used on Arm
>>>>>>>>>>
>>>>>>>>>> It is neither in x86 PVH/HVM guests.
>>>>>>>>> Didn't know that, thank you for pointing
>>>>>>>>>>
>>>>>>>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>>>>>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>>>>>>>>> all the goals above.
>>>>>>>>>>>
>>>>>>>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>>>>>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>>>>>>>>>
>>>>>>>>>> Doing this split should be done, as the pcifront specific part could be
>>>>>>>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>>>>>>>>> be supported.
>>>>>>>>> Agree, that the final solution should have the driver split
>>>>>>>>>>
>>>>>>>>>>> So, I think this patch is still going to be needed besides which direction we take.
>>>>>>>>>>
>>>>>>>>>> Some kind of this patch, yes. It might look different in case the split
>>>>>>>>>> is done first.
>>>>>>>>>>
>>>>>>>>>> I don't mind doing it in either sequence.
>>>>>>>>>>
>>>>>>>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>>>>>>>>
>>>>>>>>> e.g. the driver has unused code, but yet allows Arm to function now.
>>>>>>>>>
>>>>>>>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>>>>>>>>
>>>>>>>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>>>>>>>>
>>>>>>>>> have enough bandwidth for that piece of work at the moment.
>>>>>>>>
>>>>>>>> That's fair and I don't want to scope-creep this simple patch asking for
>>>>>>>> an enormous rework. At the same time I don't think we should enable the
>>>>>>>> whole of pciback on ARM because it would be erroneous and confusing.
>>>>>>
>>>>>> As the first stage before the driver is split or ifdef's used - can we take the patch
>>>>>> as is now? In either way we chose this needs to be done, e.g. enable compiling
>>>>>> for other architectures and common code move.
>>>>>
>>>>> Fine with me in principle. I need to take a more thorough look
>>>>> at the patch, though.
>>>> Of course
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> I am wonder if there is a simple:
>>>>>>>>
>>>>>>>> if (!xen_pv_domain())
>>>>>>>>         return;
>>>>>>>>
>>>>>>>> That we could add in a couple of places in pciback to stop it from
>>>>>>>> initializing the parts we don't care about. Something along these lines
>>>>>>>> (untested and probably incomplete).
>>>>>>>>
>>>>>>>> What do you guys think?
>>>>>>>
>>>>>>> Uh no, not in this way, please. This will kill pci passthrough on x86
>>>>>>> with dom0 running as PVH. I don't think this is working right now, but
>>>>>>> adding more code making it even harder to work should be avoided.
>>>>>>>
>>>>>>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>>>>>>> index da34ce85dc88..991ba0a9b359 100644
>>>>>>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>>>>>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>>>>>>> @@ -15,6 +15,7 @@
>>>>>>>>      #include <xen/xenbus.h>
>>>>>>>>      #include <xen/events.h>
>>>>>>>>      #include <xen/pci.h>
>>>>>>>> +#include <xen/xen.h>
>>>>>>>>      #include "pciback.h"
>>>>>>>>        #define INVALID_EVTCHN_IRQ  (-1)
>>>>>>>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>>>>>>>>                      const struct xenbus_device_id *id)
>>>>>>>>      {
>>>>>>>>          int err = 0;
>>>>>>>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
>>>>>>>> +    struct xen_pcibk_device *pdev;
>>>>>>>> +
>>>>>>>> +    if (!xen_pv_domain())
>>>>>>>> +        return 0;
>>>>>>>>      +    pdev = alloc_pdev(dev);
>>>>>>>
>>>>>>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
>>>>>>> early will result in xen_pcibk_xenbus_probe never being called.
>>>>>>>
>>>>>>>>          if (pdev == NULL) {
>>>>>>>>              err = -ENOMEM;
>>>>>>>>              xenbus_dev_fatal(dev, err,
>>>>>>>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>>>>>>>>        int __init xen_pcibk_xenbus_register(void)
>>>>>>>>      {
>>>>>>>> +    if (!xen_pv_domain())
>>>>>>>> +        return 0;
>>>>>>>> +
>>>>>>>
>>>>>>> Use #ifdef CONFIG_X86 instead.
>>>>>>
>>>>>> The title of this patch says that we want to allow this driver for other archs
>>>>>> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
>>>>>> right with that respect. Instead, we may want having something like a
>>>>>> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
>>>>>> or something which is architecture agnostic.
>>>>>
>>>>> Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
>>>>> acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
>>>>> functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
>>>>> select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
>>>>> wanted. The splitting of the driver can still be done later.
>>>>
>>>> Hm, pciback is now compiled when CONFIG_XEN_PCIDEV_BACKEND is enabled
>>>> and we want to skip some parts of its code when CONFIG_XEN_PCI_STUB is set.
>>>> So, I imagine that for x86 we just enable CONFIG_XEN_PCIDEV_BACKEND and the
>>>> driver compiles in its current state. For Arm we enable both CONFIG_XEN_PCIDEV_BACKEND
>>>> and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.
>>>
>>> No, I'd rather switch to compiling xen-pciback when CONFIG_XEN_PCI_STUB
>>> is set and compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is
>>> not set (this will be the case on Arm).
>>
>> But this will require that the existing kernel configurations out there have to additionally define CONFIG_XEN_PCI_STUB to get what they had before with simply enabling CONFIG_XEN_PCIDEV_BACKEND. My point was that it is probably desirable not to break
>> the things while doing the split/re-work.
>
> By letting XEN_PCIDEV_BACKEND select XEN_PCI_STUB this won't happen.
Indeed
>
>> I also thought that "compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is not set"
>> may have more code gated rather than with gating unwanted code with CONFIG_XEN_PCI_STUB.
>> I am not quite sure about this though.
>
> This would be a very weird semantics of XEN_PCI_STUB, as the stub part
> is needed on X86 and on Arm.
>
> Gating could even be done with Stefano's patch just by replacing his
> "!xen_pv_domain()" tests with "!IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND)".

Makes sense.

Another question if we do not want the code to be compiled or not executed?

If the later then we can define something like:

bool need_pv_part(void)

{

     return IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND);

}

and then just use need_pv_part() for the checks where it is needed.

This allows avoiding multiple ifdef's through the code

>
>
> Juergen

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

[Juergen Gross]

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

On 21.09.21 09:00, Oleksandr Andrushchenko wrote:
> 
> On 21.09.21 09:49, Juergen Gross wrote:
>> On 21.09.21 08:38, Oleksandr Andrushchenko wrote:
>>>
>>> On 21.09.21 09:07, Juergen Gross wrote:
>>>> On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
>>>>>
>>>>> On 21.09.21 08:20, Juergen Gross wrote:
>>>>>> On 21.09.21 01:16, Stefano Stabellini wrote:
>>>>>>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>>>>>>>> On 20.09.21 14:30, Juergen Gross wrote:
>>>>>>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>>>>>>>> Hello, Stefano!
>>>>>>>>>>
>>>>>>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>>>>>>>> Hi Oleksandr,
>>>>>>>>>>>
>>>>>>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>>>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>>>>>>>> Not only that
>>>>>>>>>>>
>>>>>>>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>>>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>>>>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>>>>>>>> the same time.
>>>>>>>>>> Correct, it is not used
>>>>>>>>>>>
>>>>>>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>>>>>>>> possible and better to use pci-stub instead?
>>>>>>>>>>
>>>>>>>>>> Not only that, so pci-stub is not enough
>>>>>>>>>>
>>>>>>>>>> The functionality which is implemented by the pciback and the toolstack
>>>>>>>>>> and which is relevant/missing/needed for ARM:
>>>>>>>>>>
>>>>>>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>>>>>>>
>>>>>>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>>>>>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>>>>>>>
>>>>>>>>>> Please see [1] and [2]:
>>>>>>>>>>
>>>>>>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>>>>>>>
>>>>>>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>>>>>>>> the rest like vPCI etc.
>>>>>>>>>>
>>>>>>>>>> 3. pcifront is not used on Arm
>>>>>>>>>
>>>>>>>>> It is neither in x86 PVH/HVM guests.
>>>>>>>> Didn't know that, thank you for pointing
>>>>>>>>>
>>>>>>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>>>>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>>>>>>>> all the goals above.
>>>>>>>>>>
>>>>>>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>>>>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>>>>>>>>
>>>>>>>>> Doing this split should be done, as the pcifront specific part could be
>>>>>>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>>>>>>>> be supported.
>>>>>>>> Agree, that the final solution should have the driver split
>>>>>>>>>
>>>>>>>>>> So, I think this patch is still going to be needed besides which direction we take.
>>>>>>>>>
>>>>>>>>> Some kind of this patch, yes. It might look different in case the split
>>>>>>>>> is done first.
>>>>>>>>>
>>>>>>>>> I don't mind doing it in either sequence.
>>>>>>>>>
>>>>>>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>>>>>>>
>>>>>>>> e.g. the driver has unused code, but yet allows Arm to function now.
>>>>>>>>
>>>>>>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>>>>>>>
>>>>>>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>>>>>>>
>>>>>>>> have enough bandwidth for that piece of work at the moment.
>>>>>>>
>>>>>>> That's fair and I don't want to scope-creep this simple patch asking for
>>>>>>> an enormous rework. At the same time I don't think we should enable the
>>>>>>> whole of pciback on ARM because it would be erroneous and confusing.
>>>>>
>>>>> As the first stage before the driver is split or ifdef's used - can we take the patch
>>>>> as is now? In either way we chose this needs to be done, e.g. enable compiling
>>>>> for other architectures and common code move.
>>>>
>>>> Fine with me in principle. I need to take a more thorough look
>>>> at the patch, though.
>>> Of course
>>>>
>>>>>
>>>>>>>
>>>>>>> I am wonder if there is a simple:
>>>>>>>
>>>>>>> if (!xen_pv_domain())
>>>>>>>         return;
>>>>>>>
>>>>>>> That we could add in a couple of places in pciback to stop it from
>>>>>>> initializing the parts we don't care about. Something along these lines
>>>>>>> (untested and probably incomplete).
>>>>>>>
>>>>>>> What do you guys think?
>>>>>>
>>>>>> Uh no, not in this way, please. This will kill pci passthrough on x86
>>>>>> with dom0 running as PVH. I don't think this is working right now, but
>>>>>> adding more code making it even harder to work should be avoided.
>>>>>>
>>>>>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>>>>>> index da34ce85dc88..991ba0a9b359 100644
>>>>>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>>>>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>>>>>> @@ -15,6 +15,7 @@
>>>>>>>      #include <xen/xenbus.h>
>>>>>>>      #include <xen/events.h>
>>>>>>>      #include <xen/pci.h>
>>>>>>> +#include <xen/xen.h>
>>>>>>>      #include "pciback.h"
>>>>>>>        #define INVALID_EVTCHN_IRQ  (-1)
>>>>>>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>>>>>>>                      const struct xenbus_device_id *id)
>>>>>>>      {
>>>>>>>          int err = 0;
>>>>>>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
>>>>>>> +    struct xen_pcibk_device *pdev;
>>>>>>> +
>>>>>>> +    if (!xen_pv_domain())
>>>>>>> +        return 0;
>>>>>>>      +    pdev = alloc_pdev(dev);
>>>>>>
>>>>>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
>>>>>> early will result in xen_pcibk_xenbus_probe never being called.
>>>>>>
>>>>>>>          if (pdev == NULL) {
>>>>>>>              err = -ENOMEM;
>>>>>>>              xenbus_dev_fatal(dev, err,
>>>>>>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>>>>>>>        int __init xen_pcibk_xenbus_register(void)
>>>>>>>      {
>>>>>>> +    if (!xen_pv_domain())
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>
>>>>>> Use #ifdef CONFIG_X86 instead.
>>>>>
>>>>> The title of this patch says that we want to allow this driver for other archs
>>>>> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
>>>>> right with that respect. Instead, we may want having something like a
>>>>> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
>>>>> or something which is architecture agnostic.
>>>>
>>>> Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
>>>> acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
>>>> functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
>>>> select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
>>>> wanted. The splitting of the driver can still be done later.
>>>
>>> Hm, pciback is now compiled when CONFIG_XEN_PCIDEV_BACKEND  is enabled
>>> and we want to skip some parts of its code when CONFIG_XEN_PCI_STUB is set.
>>> So, I imagine that for x86 we just enable CONFIG_XEN_PCIDEV_BACKEND and the
>>> driver compiles in its current state. For Arm we enable both CONFIG_XEN_PCIDEV_BACKEND
>>> and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.
>>
>> No, I'd rather switch to compiling xen-pciback when CONFIG_XEN_PCI_STUB
>> is set and compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is
>> not set (this will be the case on Arm).
> 
> But this will require that the existing kernel configurations out there have to additionally 
> define CONFIG_XEN_PCI_STUB to get what they had before with simply enabling 
> CONFIG_XEN_PCIDEV_BACKEND. My point was that it is probably desirable not to break
> the things while doing the split/re-work.

By letting XEN_PCIDEV_BACKEND select XEN_PCI_STUB this won't happen.

> I also thought that "compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is not set"
> may have more code gated rather than with gating unwanted code with CONFIG_XEN_PCI_STUB.
> I am not quite sure about this though.

This would be a very weird semantics of XEN_PCI_STUB, as the stub part
is needed on X86 and on Arm.

Gating could even be done with Stefano's patch just by replacing his
"!xen_pv_domain()" tests with "!IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND)".


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] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

On 21.09.21 09:49, Juergen Gross wrote:
> On 21.09.21 08:38, Oleksandr Andrushchenko wrote:
>>
>> On 21.09.21 09:07, Juergen Gross wrote:
>>> On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
>>>>
>>>> On 21.09.21 08:20, Juergen Gross wrote:
>>>>> On 21.09.21 01:16, Stefano Stabellini wrote:
>>>>>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>>>>>>> On 20.09.21 14:30, Juergen Gross wrote:
>>>>>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>>>>>>> Hello, Stefano!
>>>>>>>>>
>>>>>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>>>>>>> Hi Oleksandr,
>>>>>>>>>>
>>>>>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>>>>>>> Not only that
>>>>>>>>>>
>>>>>>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>>>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>>>>>>> the same time.
>>>>>>>>> Correct, it is not used
>>>>>>>>>>
>>>>>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>>>>>>> possible and better to use pci-stub instead?
>>>>>>>>>
>>>>>>>>> Not only that, so pci-stub is not enough
>>>>>>>>>
>>>>>>>>> The functionality which is implemented by the pciback and the toolstack
>>>>>>>>> and which is relevant/missing/needed for ARM:
>>>>>>>>>
>>>>>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>>>>>>
>>>>>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>>>>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>>>>>>
>>>>>>>>> Please see [1] and [2]:
>>>>>>>>>
>>>>>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>>>>>>
>>>>>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>>>>>>> the rest like vPCI etc.
>>>>>>>>>
>>>>>>>>> 3. pcifront is not used on Arm
>>>>>>>>
>>>>>>>> It is neither in x86 PVH/HVM guests.
>>>>>>> Didn't know that, thank you for pointing
>>>>>>>>
>>>>>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>>>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>>>>>>> all the goals above.
>>>>>>>>>
>>>>>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>>>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>>>>>>>
>>>>>>>> Doing this split should be done, as the pcifront specific part could be
>>>>>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>>>>>>> be supported.
>>>>>>> Agree, that the final solution should have the driver split
>>>>>>>>
>>>>>>>>> So, I think this patch is still going to be needed besides which direction we take.
>>>>>>>>
>>>>>>>> Some kind of this patch, yes. It might look different in case the split
>>>>>>>> is done first.
>>>>>>>>
>>>>>>>> I don't mind doing it in either sequence.
>>>>>>>>
>>>>>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>>>>>>
>>>>>>> e.g. the driver has unused code, but yet allows Arm to function now.
>>>>>>>
>>>>>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>>>>>>
>>>>>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>>>>>>
>>>>>>> have enough bandwidth for that piece of work at the moment.
>>>>>>
>>>>>> That's fair and I don't want to scope-creep this simple patch asking for
>>>>>> an enormous rework. At the same time I don't think we should enable the
>>>>>> whole of pciback on ARM because it would be erroneous and confusing.
>>>>
>>>> As the first stage before the driver is split or ifdef's used - can we take the patch
>>>> as is now? In either way we chose this needs to be done, e.g. enable compiling
>>>> for other architectures and common code move.
>>>
>>> Fine with me in principle. I need to take a more thorough look
>>> at the patch, though.
>> Of course
>>>
>>>>
>>>>>>
>>>>>> I am wonder if there is a simple:
>>>>>>
>>>>>> if (!xen_pv_domain())
>>>>>>        return;
>>>>>>
>>>>>> That we could add in a couple of places in pciback to stop it from
>>>>>> initializing the parts we don't care about. Something along these lines
>>>>>> (untested and probably incomplete).
>>>>>>
>>>>>> What do you guys think?
>>>>>
>>>>> Uh no, not in this way, please. This will kill pci passthrough on x86
>>>>> with dom0 running as PVH. I don't think this is working right now, but
>>>>> adding more code making it even harder to work should be avoided.
>>>>>
>>>>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>>>>> index da34ce85dc88..991ba0a9b359 100644
>>>>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>>>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>>     #include <xen/xenbus.h>
>>>>>>     #include <xen/events.h>
>>>>>>     #include <xen/pci.h>
>>>>>> +#include <xen/xen.h>
>>>>>>     #include "pciback.h"
>>>>>>       #define INVALID_EVTCHN_IRQ  (-1)
>>>>>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>>>>>>                     const struct xenbus_device_id *id)
>>>>>>     {
>>>>>>         int err = 0;
>>>>>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
>>>>>> +    struct xen_pcibk_device *pdev;
>>>>>> +
>>>>>> +    if (!xen_pv_domain())
>>>>>> +        return 0;
>>>>>>     +    pdev = alloc_pdev(dev);
>>>>>
>>>>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
>>>>> early will result in xen_pcibk_xenbus_probe never being called.
>>>>>
>>>>>>         if (pdev == NULL) {
>>>>>>             err = -ENOMEM;
>>>>>>             xenbus_dev_fatal(dev, err,
>>>>>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>>>>>>       int __init xen_pcibk_xenbus_register(void)
>>>>>>     {
>>>>>> +    if (!xen_pv_domain())
>>>>>> +        return 0;
>>>>>> +
>>>>>
>>>>> Use #ifdef CONFIG_X86 instead.
>>>>
>>>> The title of this patch says that we want to allow this driver for other archs
>>>> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
>>>> right with that respect. Instead, we may want having something like a
>>>> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
>>>> or something which is architecture agnostic.
>>>
>>> Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
>>> acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
>>> functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
>>> select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
>>> wanted. The splitting of the driver can still be done later.
>>
>> Hm, pciback is now compiled when CONFIG_XEN_PCIDEV_BACKEND  is enabled
>> and we want to skip some parts of its code when CONFIG_XEN_PCI_STUB is set.
>> So, I imagine that for x86 we just enable CONFIG_XEN_PCIDEV_BACKEND and the
>> driver compiles in its current state. For Arm we enable both CONFIG_XEN_PCIDEV_BACKEND
>> and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.
>
> No, I'd rather switch to compiling xen-pciback when CONFIG_XEN_PCI_STUB
> is set and compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is
> not set (this will be the case on Arm).

But this will require that the existing kernel configurations out there have to additionally

define CONFIG_XEN_PCI_STUB to get what they had before with simply enabling

CONFIG_XEN_PCIDEV_BACKEND. My point was that it is probably desirable not to break

the things while doing the split/re-work.

I also thought that "compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is not set"

may have more code gated rather than with gating unwanted code with CONFIG_XEN_PCI_STUB.

I am not quite sure about this though.

>
> This is another step in the right direction preparing the split.
>
> But as said before, this is not a requirement by me to take your patch.
Thank you
>
>
> Juergen

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

[Juergen Gross]

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

On 21.09.21 08:38, Oleksandr Andrushchenko wrote:
> 
> On 21.09.21 09:07, Juergen Gross wrote:
>> On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
>>>
>>> On 21.09.21 08:20, Juergen Gross wrote:
>>>> On 21.09.21 01:16, Stefano Stabellini wrote:
>>>>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>>>>>> On 20.09.21 14:30, Juergen Gross wrote:
>>>>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>>>>>> Hello, Stefano!
>>>>>>>>
>>>>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>>>>>> Hi Oleksandr,
>>>>>>>>>
>>>>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>>>>>> Not only that
>>>>>>>>>
>>>>>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>>>>>> the same time.
>>>>>>>> Correct, it is not used
>>>>>>>>>
>>>>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>>>>>> possible and better to use pci-stub instead?
>>>>>>>>
>>>>>>>> Not only that, so pci-stub is not enough
>>>>>>>>
>>>>>>>> The functionality which is implemented by the pciback and the toolstack
>>>>>>>> and which is relevant/missing/needed for ARM:
>>>>>>>>
>>>>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>>>>>
>>>>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>>>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>>>>>
>>>>>>>> Please see [1] and [2]:
>>>>>>>>
>>>>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>>>>>
>>>>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>>>>>> the rest like vPCI etc.
>>>>>>>>
>>>>>>>> 3. pcifront is not used on Arm
>>>>>>>
>>>>>>> It is neither in x86 PVH/HVM guests.
>>>>>> Didn't know that, thank you for pointing
>>>>>>>
>>>>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>>>>>> all the goals above.
>>>>>>>>
>>>>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>>>>>>
>>>>>>> Doing this split should be done, as the pcifront specific part could be
>>>>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>>>>>> be supported.
>>>>>> Agree, that the final solution should have the driver split
>>>>>>>
>>>>>>>> So, I think this patch is still going to be needed besides which direction we take.
>>>>>>>
>>>>>>> Some kind of this patch, yes. It might look different in case the split
>>>>>>> is done first.
>>>>>>>
>>>>>>> I don't mind doing it in either sequence.
>>>>>>>
>>>>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>>>>>
>>>>>> e.g. the driver has unused code, but yet allows Arm to function now.
>>>>>>
>>>>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>>>>>
>>>>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>>>>>
>>>>>> have enough bandwidth for that piece of work at the moment.
>>>>>
>>>>> That's fair and I don't want to scope-creep this simple patch asking for
>>>>> an enormous rework. At the same time I don't think we should enable the
>>>>> whole of pciback on ARM because it would be erroneous and confusing.
>>>
>>> As the first stage before the driver is split or ifdef's used - can we take the patch
>>> as is now? In either way we chose this needs to be done, e.g. enable compiling
>>> for other architectures and common code move.
>>
>> Fine with me in principle. I need to take a more thorough look
>> at the patch, though.
> Of course
>>
>>>
>>>>>
>>>>> I am wonder if there is a simple:
>>>>>
>>>>> if (!xen_pv_domain())
>>>>>        return;
>>>>>
>>>>> That we could add in a couple of places in pciback to stop it from
>>>>> initializing the parts we don't care about. Something along these lines
>>>>> (untested and probably incomplete).
>>>>>
>>>>> What do you guys think?
>>>>
>>>> Uh no, not in this way, please. This will kill pci passthrough on x86
>>>> with dom0 running as PVH. I don't think this is working right now, but
>>>> adding more code making it even harder to work should be avoided.
>>>>
>>>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>>>> index da34ce85dc88..991ba0a9b359 100644
>>>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>>>> @@ -15,6 +15,7 @@
>>>>>     #include <xen/xenbus.h>
>>>>>     #include <xen/events.h>
>>>>>     #include <xen/pci.h>
>>>>> +#include <xen/xen.h>
>>>>>     #include "pciback.h"
>>>>>       #define INVALID_EVTCHN_IRQ  (-1)
>>>>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>>>>>                     const struct xenbus_device_id *id)
>>>>>     {
>>>>>         int err = 0;
>>>>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
>>>>> +    struct xen_pcibk_device *pdev;
>>>>> +
>>>>> +    if (!xen_pv_domain())
>>>>> +        return 0;
>>>>>     +    pdev = alloc_pdev(dev);
>>>>
>>>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
>>>> early will result in xen_pcibk_xenbus_probe never being called.
>>>>
>>>>>         if (pdev == NULL) {
>>>>>             err = -ENOMEM;
>>>>>             xenbus_dev_fatal(dev, err,
>>>>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>>>>>       int __init xen_pcibk_xenbus_register(void)
>>>>>     {
>>>>> +    if (!xen_pv_domain())
>>>>> +        return 0;
>>>>> +
>>>>
>>>> Use #ifdef CONFIG_X86 instead.
>>>
>>> The title of this patch says that we want to allow this driver for other archs
>>> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
>>> right with that respect. Instead, we may want having something like a
>>> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
>>> or something which is architecture agnostic.
>>
>> Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
>> acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
>> functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
>> select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
>> wanted. The splitting of the driver can still be done later.
> 
> Hm, pciback is now compiled when CONFIG_XEN_PCIDEV_BACKEND  is enabled
> and we want to skip some parts of its code when CONFIG_XEN_PCI_STUB is set.
> So, I imagine that for x86 we just enable CONFIG_XEN_PCIDEV_BACKEND and the
> driver compiles in its current state. For Arm we enable both CONFIG_XEN_PCIDEV_BACKEND
> and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.

No, I'd rather switch to compiling xen-pciback when CONFIG_XEN_PCI_STUB
is set and compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is
not set (this will be the case on Arm).

This is another step in the right direction preparing the split.

But as said before, this is not a requirement by me to take your patch.


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] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

On 21.09.21 09:07, Juergen Gross wrote:
> On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
>>
>> On 21.09.21 08:20, Juergen Gross wrote:
>>> On 21.09.21 01:16, Stefano Stabellini wrote:
>>>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>>>>> On 20.09.21 14:30, Juergen Gross wrote:
>>>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>>>>> Hello, Stefano!
>>>>>>>
>>>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>>>>> Hi Oleksandr,
>>>>>>>>
>>>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>>>>> Not only that
>>>>>>>>
>>>>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>>>>> the same time.
>>>>>>> Correct, it is not used
>>>>>>>>
>>>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>>>>> possible and better to use pci-stub instead?
>>>>>>>
>>>>>>> Not only that, so pci-stub is not enough
>>>>>>>
>>>>>>> The functionality which is implemented by the pciback and the toolstack
>>>>>>> and which is relevant/missing/needed for ARM:
>>>>>>>
>>>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>>>>
>>>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>>>>
>>>>>>> Please see [1] and [2]:
>>>>>>>
>>>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>>>>
>>>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>>>>> the rest like vPCI etc.
>>>>>>>
>>>>>>> 3. pcifront is not used on Arm
>>>>>>
>>>>>> It is neither in x86 PVH/HVM guests.
>>>>> Didn't know that, thank you for pointing
>>>>>>
>>>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>>>>> all the goals above.
>>>>>>>
>>>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>>>>>
>>>>>> Doing this split should be done, as the pcifront specific part could be
>>>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>>>>> be supported.
>>>>> Agree, that the final solution should have the driver split
>>>>>>
>>>>>>> So, I think this patch is still going to be needed besides which direction we take.
>>>>>>
>>>>>> Some kind of this patch, yes. It might look different in case the split
>>>>>> is done first.
>>>>>>
>>>>>> I don't mind doing it in either sequence.
>>>>>>
>>>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>>>>
>>>>> e.g. the driver has unused code, but yet allows Arm to function now.
>>>>>
>>>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>>>>
>>>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>>>>
>>>>> have enough bandwidth for that piece of work at the moment.
>>>>
>>>> That's fair and I don't want to scope-creep this simple patch asking for
>>>> an enormous rework. At the same time I don't think we should enable the
>>>> whole of pciback on ARM because it would be erroneous and confusing.
>>
>> As the first stage before the driver is split or ifdef's used - can we take the patch
>> as is now? In either way we chose this needs to be done, e.g. enable compiling
>> for other architectures and common code move.
>
> Fine with me in principle. I need to take a more thorough look
> at the patch, though.
Of course
>
>>
>>>>
>>>> I am wonder if there is a simple:
>>>>
>>>> if (!xen_pv_domain())
>>>>       return;
>>>>
>>>> That we could add in a couple of places in pciback to stop it from
>>>> initializing the parts we don't care about. Something along these lines
>>>> (untested and probably incomplete).
>>>>
>>>> What do you guys think?
>>>
>>> Uh no, not in this way, please. This will kill pci passthrough on x86
>>> with dom0 running as PVH. I don't think this is working right now, but
>>> adding more code making it even harder to work should be avoided.
>>>
>>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>>> index da34ce85dc88..991ba0a9b359 100644
>>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <xen/xenbus.h>
>>>>    #include <xen/events.h>
>>>>    #include <xen/pci.h>
>>>> +#include <xen/xen.h>
>>>>    #include "pciback.h"
>>>>      #define INVALID_EVTCHN_IRQ  (-1)
>>>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>>>>                    const struct xenbus_device_id *id)
>>>>    {
>>>>        int err = 0;
>>>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
>>>> +    struct xen_pcibk_device *pdev;
>>>> +
>>>> +    if (!xen_pv_domain())
>>>> +        return 0;
>>>>    +    pdev = alloc_pdev(dev);
>>>
>>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
>>> early will result in xen_pcibk_xenbus_probe never being called.
>>>
>>>>        if (pdev == NULL) {
>>>>            err = -ENOMEM;
>>>>            xenbus_dev_fatal(dev, err,
>>>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>>>>      int __init xen_pcibk_xenbus_register(void)
>>>>    {
>>>> +    if (!xen_pv_domain())
>>>> +        return 0;
>>>> +
>>>
>>> Use #ifdef CONFIG_X86 instead.
>>
>> The title of this patch says that we want to allow this driver for other archs
>> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
>> right with that respect. Instead, we may want having something like a
>> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
>> or something which is architecture agnostic.
>
> Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
> acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
> functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
> select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
> wanted. The splitting of the driver can still be done later.

Hm, pciback is now compiled when CONFIG_XEN_PCIDEV_BACKEND  is enabled

and we want to skip some parts of its code when CONFIG_XEN_PCI_STUB is set.

So, I imagine that for x86 we just enable CONFIG_XEN_PCIDEV_BACKEND and the

driver compiles in its current state. For Arm we enable both CONFIG_XEN_PCIDEV_BACKEND

and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.


>
>> Gating also means that we are not thinking about splitting the backend driver into
>> two different ones, e.g. one for "common" code and one for PV stuff.
>> Otherwise this ifdefery won't be needed.
>
> I just wanted to avoid the xen_pv_domain() tests creeping in, as
> they are wrong IMO.
>
I understand that
>
> Juergen

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

[Juergen Gross]

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

On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
> 
> On 21.09.21 08:20, Juergen Gross wrote:
>> On 21.09.21 01:16, Stefano Stabellini wrote:
>>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>>>> On 20.09.21 14:30, Juergen Gross wrote:
>>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>>>> Hello, Stefano!
>>>>>>
>>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>>>> Hi Oleksandr,
>>>>>>>
>>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>>>> Not only that
>>>>>>>
>>>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>>>> the same time.
>>>>>> Correct, it is not used
>>>>>>>
>>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>>>> possible and better to use pci-stub instead?
>>>>>>
>>>>>> Not only that, so pci-stub is not enough
>>>>>>
>>>>>> The functionality which is implemented by the pciback and the toolstack
>>>>>> and which is relevant/missing/needed for ARM:
>>>>>>
>>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>>>
>>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>>>
>>>>>> Please see [1] and [2]:
>>>>>>
>>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>>>
>>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>>>> the rest like vPCI etc.
>>>>>>
>>>>>> 3. pcifront is not used on Arm
>>>>>
>>>>> It is neither in x86 PVH/HVM guests.
>>>> Didn't know that, thank you for pointing
>>>>>
>>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>>>> all the goals above.
>>>>>>
>>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>>>>
>>>>> Doing this split should be done, as the pcifront specific part could be
>>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>>>> be supported.
>>>> Agree, that the final solution should have the driver split
>>>>>
>>>>>> So, I think this patch is still going to be needed besides which direction we take.
>>>>>
>>>>> Some kind of this patch, yes. It might look different in case the split
>>>>> is done first.
>>>>>
>>>>> I don't mind doing it in either sequence.
>>>>>
>>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>>>
>>>> e.g. the driver has unused code, but yet allows Arm to function now.
>>>>
>>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>>>
>>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>>>
>>>> have enough bandwidth for that piece of work at the moment.
>>>
>>> That's fair and I don't want to scope-creep this simple patch asking for
>>> an enormous rework. At the same time I don't think we should enable the
>>> whole of pciback on ARM because it would be erroneous and confusing.
> 
> As the first stage before the driver is split or ifdef's used - can we take the patch
> as is now? In either way we chose this needs to be done, e.g. enable compiling
> for other architectures and common code move.

Fine with me in principle. I need to take a more thorough look
at the patch, though.

> 
>>>
>>> I am wonder if there is a simple:
>>>
>>> if (!xen_pv_domain())
>>>       return;
>>>
>>> That we could add in a couple of places in pciback to stop it from
>>> initializing the parts we don't care about. Something along these lines
>>> (untested and probably incomplete).
>>>
>>> What do you guys think?
>>
>> Uh no, not in this way, please. This will kill pci passthrough on x86
>> with dom0 running as PVH. I don't think this is working right now, but
>> adding more code making it even harder to work should be avoided.
>>
>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>> index da34ce85dc88..991ba0a9b359 100644
>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>> @@ -15,6 +15,7 @@
>>>    #include <xen/xenbus.h>
>>>    #include <xen/events.h>
>>>    #include <xen/pci.h>
>>> +#include <xen/xen.h>
>>>    #include "pciback.h"
>>>      #define INVALID_EVTCHN_IRQ  (-1)
>>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>>>                    const struct xenbus_device_id *id)
>>>    {
>>>        int err = 0;
>>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
>>> +    struct xen_pcibk_device *pdev;
>>> +
>>> +    if (!xen_pv_domain())
>>> +        return 0;
>>>    +    pdev = alloc_pdev(dev);
>>
>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
>> early will result in xen_pcibk_xenbus_probe never being called.
>>
>>>        if (pdev == NULL) {
>>>            err = -ENOMEM;
>>>            xenbus_dev_fatal(dev, err,
>>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>>>      int __init xen_pcibk_xenbus_register(void)
>>>    {
>>> +    if (!xen_pv_domain())
>>> +        return 0;
>>> +
>>
>> Use #ifdef CONFIG_X86 instead.
> 
> The title of this patch says that we want to allow this driver for other archs
> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
> right with that respect. Instead, we may want having something like a
> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
> or something which is architecture agnostic.

Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
wanted. The splitting of the driver can still be done later.

> Gating also means that we are not thinking about splitting the backend driver into
> two different ones, e.g. one for "common" code and one for PV stuff.
> Otherwise this ifdefery won't be needed.

I just wanted to avoid the xen_pv_domain() tests creeping in, as
they are wrong IMO.


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] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

On 21.09.21 08:20, Juergen Gross wrote:
> On 21.09.21 01:16, Stefano Stabellini wrote:
>> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>>> On 20.09.21 14:30, Juergen Gross wrote:
>>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>>> Hello, Stefano!
>>>>>
>>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>>> Hi Oleksandr,
>>>>>>
>>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>>> Not only that
>>>>>>
>>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>>> the same time.
>>>>> Correct, it is not used
>>>>>>
>>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>>> possible and better to use pci-stub instead?
>>>>>
>>>>> Not only that, so pci-stub is not enough
>>>>>
>>>>> The functionality which is implemented by the pciback and the toolstack
>>>>> and which is relevant/missing/needed for ARM:
>>>>>
>>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>>
>>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>>
>>>>> Please see [1] and [2]:
>>>>>
>>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>>
>>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>>> the rest like vPCI etc.
>>>>>
>>>>> 3. pcifront is not used on Arm
>>>>
>>>> It is neither in x86 PVH/HVM guests.
>>> Didn't know that, thank you for pointing
>>>>
>>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>>> all the goals above.
>>>>>
>>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>>>
>>>> Doing this split should be done, as the pcifront specific part could be
>>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>>> be supported.
>>> Agree, that the final solution should have the driver split
>>>>
>>>>> So, I think this patch is still going to be needed besides which direction we take.
>>>>
>>>> Some kind of this patch, yes. It might look different in case the split
>>>> is done first.
>>>>
>>>> I don't mind doing it in either sequence.
>>>>
>>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>>
>>> e.g. the driver has unused code, but yet allows Arm to function now.
>>>
>>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>>
>>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>>
>>> have enough bandwidth for that piece of work at the moment.
>>
>> That's fair and I don't want to scope-creep this simple patch asking for
>> an enormous rework. At the same time I don't think we should enable the
>> whole of pciback on ARM because it would be erroneous and confusing.

As the first stage before the driver is split or ifdef's used - can we take the patch

as is now? In either way we chose this needs to be done, e.g. enable compiling

for other architectures and common code move.

>>
>> I am wonder if there is a simple:
>>
>> if (!xen_pv_domain())
>>      return;
>>
>> That we could add in a couple of places in pciback to stop it from
>> initializing the parts we don't care about. Something along these lines
>> (untested and probably incomplete).
>>
>> What do you guys think?
>
> Uh no, not in this way, please. This will kill pci passthrough on x86
> with dom0 running as PVH. I don't think this is working right now, but
> adding more code making it even harder to work should be avoided.
>
>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>> index da34ce85dc88..991ba0a9b359 100644
>> --- a/drivers/xen/xen-pciback/xenbus.c
>> +++ b/drivers/xen/xen-pciback/xenbus.c
>> @@ -15,6 +15,7 @@
>>   #include <xen/xenbus.h>
>>   #include <xen/events.h>
>>   #include <xen/pci.h>
>> +#include <xen/xen.h>
>>   #include "pciback.h"
>>     #define INVALID_EVTCHN_IRQ  (-1)
>> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>>                   const struct xenbus_device_id *id)
>>   {
>>       int err = 0;
>> -    struct xen_pcibk_device *pdev = alloc_pdev(dev);
>> +    struct xen_pcibk_device *pdev;
>> +
>> +    if (!xen_pv_domain())
>> +        return 0;
>>   +    pdev = alloc_pdev(dev);
>
> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
> early will result in xen_pcibk_xenbus_probe never being called.
>
>>       if (pdev == NULL) {
>>           err = -ENOMEM;
>>           xenbus_dev_fatal(dev, err,
>> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>>     int __init xen_pcibk_xenbus_register(void)
>>   {
>> +    if (!xen_pv_domain())
>> +        return 0;
>> +
>
> Use #ifdef CONFIG_X86 instead.

The title of this patch says that we want to allow this driver for other archs

and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound

right with that respect. Instead, we may want having something like a

dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"

or something which is architecture agnostic.

Gating also means that we are not thinking about splitting the backend driver into

two different ones, e.g. one for "common" code and one for PV stuff.

Otherwise this ifdefery won't be needed.

>
>>       xen_pcibk_backend = &xen_pcibk_vpci_backend;
>>       if (passthrough)
>>           xen_pcibk_backend = &xen_pcibk_passthrough_backend;
>> @@ -752,5 +760,7 @@ int __init xen_pcibk_xenbus_register(void)
>>     void __exit xen_pcibk_xenbus_unregister(void)
>>   {
>> +    if (!xen_pv_domain())
>> +        return;
>
> #ifdef again.
>
>> xenbus_unregister_driver(&xen_pcibk_driver);
>>   }
>>
>
>
> Juergen

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

[Juergen Gross]

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

On 21.09.21 01:16, Stefano Stabellini wrote:
> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>> On 20.09.21 14:30, Juergen Gross wrote:
>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>> Hello, Stefano!
>>>>
>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>> Hi Oleksandr,
>>>>>
>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>> Not only that
>>>>>
>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>> the same time.
>>>> Correct, it is not used
>>>>>
>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>> possible and better to use pci-stub instead?
>>>>
>>>> Not only that, so pci-stub is not enough
>>>>
>>>> The functionality which is implemented by the pciback and the toolstack
>>>> and which is relevant/missing/needed for ARM:
>>>>
>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>
>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>
>>>> Please see [1] and [2]:
>>>>
>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>
>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>> the rest like vPCI etc.
>>>>
>>>> 3. pcifront is not used on Arm
>>>
>>> It is neither in x86 PVH/HVM guests.
>> Didn't know that, thank you for pointing
>>>
>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>> all the goals above.
>>>>
>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>>
>>> Doing this split should be done, as the pcifront specific part could be
>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>> be supported.
>> Agree, that the final solution should have the driver split
>>>
>>>> So, I think this patch is still going to be needed besides which direction we take.
>>>
>>> Some kind of this patch, yes. It might look different in case the split
>>> is done first.
>>>
>>> I don't mind doing it in either sequence.
>>>
>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>
>> e.g. the driver has unused code, but yet allows Arm to function now.
>>
>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>
>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>
>> have enough bandwidth for that piece of work at the moment.
> 
> That's fair and I don't want to scope-creep this simple patch asking for
> an enormous rework. At the same time I don't think we should enable the
> whole of pciback on ARM because it would be erroneous and confusing.
> 
> I am wonder if there is a simple:
> 
> if (!xen_pv_domain())
>      return;
> 
> That we could add in a couple of places in pciback to stop it from
> initializing the parts we don't care about. Something along these lines
> (untested and probably incomplete).
> 
> What do you guys think?

Uh no, not in this way, please. This will kill pci passthrough on x86
with dom0 running as PVH. I don't think this is working right now, but
adding more code making it even harder to work should be avoided.

> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index da34ce85dc88..991ba0a9b359 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -15,6 +15,7 @@
>   #include <xen/xenbus.h>
>   #include <xen/events.h>
>   #include <xen/pci.h>
> +#include <xen/xen.h>
>   #include "pciback.h"
>   
>   #define INVALID_EVTCHN_IRQ  (-1)
> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>   				const struct xenbus_device_id *id)
>   {
>   	int err = 0;
> -	struct xen_pcibk_device *pdev = alloc_pdev(dev);
> +	struct xen_pcibk_device *pdev;
> +
> +	if (!xen_pv_domain())
> +		return 0;
>   
> +	pdev = alloc_pdev(dev);

This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
early will result in xen_pcibk_xenbus_probe never being called.

>   	if (pdev == NULL) {
>   		err = -ENOMEM;
>   		xenbus_dev_fatal(dev, err,
> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>   
>   int __init xen_pcibk_xenbus_register(void)
>   {
> +	if (!xen_pv_domain())
> +		return 0;
> +

Use #ifdef CONFIG_X86 instead.

>   	xen_pcibk_backend = &xen_pcibk_vpci_backend;
>   	if (passthrough)
>   		xen_pcibk_backend = &xen_pcibk_passthrough_backend;
> @@ -752,5 +760,7 @@ int __init xen_pcibk_xenbus_register(void)
>   
>   void __exit xen_pcibk_xenbus_unregister(void)
>   {
> +	if (!xen_pv_domain())
> +		return;

#ifdef again.

>   	xenbus_unregister_driver(&xen_pcibk_driver);
>   }
> 


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] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

Hi, Stefano!

On 21.09.21 02:16, Stefano Stabellini wrote:
> On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
>> On 20.09.21 14:30, Juergen Gross wrote:
>>> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>>>> Hello, Stefano!
>>>>
>>>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>>>> Hi Oleksandr,
>>>>>
>>>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>>>> device in Dom0 so that it can be safely assigned to a DomU?
>>>> Not only that
>>>>> I am asking because actually I don't think we want to enable the PV PCI
>>>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>>>> assignment work you have been doing in Xen. They couldn't both work at
>>>>> the same time.
>>>> Correct, it is not used
>>>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>>>> possible and better to use pci-stub instead?
>>>> Not only that, so pci-stub is not enough
>>>>
>>>> The functionality which is implemented by the pciback and the toolstack
>>>> and which is relevant/missing/needed for ARM:
>>>>
>>>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>>>
>>>> We have previously discussed on xen-devel ML possible solutions to that as from the
>>>> above we see that pciback functionality is going to be only partially used on Arm.
>>>>
>>>> Please see [1] and [2]:
>>>>
>>>> 1. It is not acceptable to manage the assignable list in Xen itself
>>>>
>>>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>>>> the rest like vPCI etc.
>>>>
>>>> 3. pcifront is not used on Arm
>>> It is neither in x86 PVH/HVM guests.
>> Didn't know that, thank you for pointing
>>>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>>>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>>>> all the goals above.
>>>>
>>>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>>>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>>> Doing this split should be done, as the pcifront specific part could be
>>> omitted on x86, too, in case no PV guests using PCI passthrough have to
>>> be supported.
>> Agree, that the final solution should have the driver split
>>>> So, I think this patch is still going to be needed besides which direction we take.
>>> Some kind of this patch, yes. It might look different in case the split
>>> is done first.
>>>
>>> I don't mind doing it in either sequence.
>>>
>> With this patch we have Arm on the same page as the above mentioned x86 guests,
>>
>> e.g. the driver has unused code, but yet allows Arm to function now.
>>
>> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
>>
>> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
>>
>> have enough bandwidth for that piece of work at the moment.
> That's fair and I don't want to scope-creep this simple patch asking for
> an enormous rework. At the same time I don't think we should enable the
> whole of pciback on ARM because it would be erroneous and confusing.
>
> I am wonder if there is a simple:
>
> if (!xen_pv_domain())
>      return;
>
> That we could add in a couple of places in pciback to stop it from
> initializing the parts we don't care about. Something along these lines
> (untested and probably incomplete).
>
> What do you guys think?

I think that it needs to be an additional patch and the PV check seems

reasonable to me. We need to check if gating only part of the initialization

with xen_pv_domain is just enough, e.g. if the rest of the code is ok that

something was not initialized and won't be touched at run-time.

Let's see what other think about the approach

>
>
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index da34ce85dc88..991ba0a9b359 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -15,6 +15,7 @@
>   #include <xen/xenbus.h>
>   #include <xen/events.h>
>   #include <xen/pci.h>
> +#include <xen/xen.h>
>   #include "pciback.h"
>   
>   #define INVALID_EVTCHN_IRQ  (-1)
> @@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>   				const struct xenbus_device_id *id)
>   {
>   	int err = 0;
> -	struct xen_pcibk_device *pdev = alloc_pdev(dev);
> +	struct xen_pcibk_device *pdev;
> +
> +	if (!xen_pv_domain())
> +		return 0;
>   
> +	pdev = alloc_pdev(dev);
>   	if (pdev == NULL) {
>   		err = -ENOMEM;
>   		xenbus_dev_fatal(dev, err,
> @@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>   
>   int __init xen_pcibk_xenbus_register(void)
>   {
> +	if (!xen_pv_domain())
> +		return 0;
> +
>   	xen_pcibk_backend = &xen_pcibk_vpci_backend;
>   	if (passthrough)
>   		xen_pcibk_backend = &xen_pcibk_passthrough_backend;
> @@ -752,5 +760,7 @@ int __init xen_pcibk_xenbus_register(void)
>   
>   void __exit xen_pcibk_xenbus_unregister(void)
>   {
> +	if (!xen_pv_domain())
> +		return;
>   	xenbus_unregister_driver(&xen_pcibk_driver);
>   }

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

[Stefano Stabellini]

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

On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
> On 20.09.21 14:30, Juergen Gross wrote:
> > On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
> >> Hello, Stefano!
> >>
> >> On 18.09.21 00:45, Stefano Stabellini wrote:
> >>> Hi Oleksandr,
> >>>
> >>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
> >>> device in Dom0 so that it can be safely assigned to a DomU?
> >> Not only that
> >>>
> >>> I am asking because actually I don't think we want to enable the PV PCI
> >>> backend feature of pciback on ARM, right? That would clash with the PCI
> >>> assignment work you have been doing in Xen. They couldn't both work at
> >>> the same time.
> >> Correct, it is not used
> >>>
> >>> If we only need pciback to "park" a device in Dom0, wouldn't it be
> >>> possible and better to use pci-stub instead?
> >>
> >> Not only that, so pci-stub is not enough
> >>
> >> The functionality which is implemented by the pciback and the toolstack
> >> and which is relevant/missing/needed for ARM:
> >>
> >> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
> >>
> >> We have previously discussed on xen-devel ML possible solutions to that as from the
> >> above we see that pciback functionality is going to be only partially used on Arm.
> >>
> >> Please see [1] and [2]:
> >>
> >> 1. It is not acceptable to manage the assignable list in Xen itself
> >>
> >> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
> >> the rest like vPCI etc.
> >>
> >> 3. pcifront is not used on Arm
> >
> > It is neither in x86 PVH/HVM guests.
> Didn't know that, thank you for pointing
> >
> >> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
> >> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
> >> all the goals above.
> >>
> >> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
> >> parts then it can be done, yet the patch is going to be the very first brick in that building.
> >
> > Doing this split should be done, as the pcifront specific part could be
> > omitted on x86, too, in case no PV guests using PCI passthrough have to
> > be supported.
> Agree, that the final solution should have the driver split
> >
> >> So, I think this patch is still going to be needed besides which direction we take.
> >
> > Some kind of this patch, yes. It might look different in case the split
> > is done first.
> >
> > I don't mind doing it in either sequence.
> >
> With this patch we have Arm on the same page as the above mentioned x86 guests,
> 
> e.g. the driver has unused code, but yet allows Arm to function now.
> 
> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
> 
> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
> 
> have enough bandwidth for that piece of work at the moment.

That's fair and I don't want to scope-creep this simple patch asking for
an enormous rework. At the same time I don't think we should enable the
whole of pciback on ARM because it would be erroneous and confusing.

I am wonder if there is a simple:

if (!xen_pv_domain())
    return;

That we could add in a couple of places in pciback to stop it from
initializing the parts we don't care about. Something along these lines
(untested and probably incomplete).

What do you guys think?


diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index da34ce85dc88..991ba0a9b359 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -15,6 +15,7 @@
 #include <xen/xenbus.h>
 #include <xen/events.h>
 #include <xen/pci.h>
+#include <xen/xen.h>
 #include "pciback.h"
 
 #define INVALID_EVTCHN_IRQ  (-1)
@@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
 				const struct xenbus_device_id *id)
 {
 	int err = 0;
-	struct xen_pcibk_device *pdev = alloc_pdev(dev);
+	struct xen_pcibk_device *pdev;
+
+	if (!xen_pv_domain())
+		return 0;
 
+	pdev = alloc_pdev(dev);
 	if (pdev == NULL) {
 		err = -ENOMEM;
 		xenbus_dev_fatal(dev, err,
@@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
 
 int __init xen_pcibk_xenbus_register(void)
 {
+	if (!xen_pv_domain())
+		return 0;
+
 	xen_pcibk_backend = &xen_pcibk_vpci_backend;
 	if (passthrough)
 		xen_pcibk_backend = &xen_pcibk_passthrough_backend;
@@ -752,5 +760,7 @@ int __init xen_pcibk_xenbus_register(void)
 
 void __exit xen_pcibk_xenbus_unregister(void)
 {
+	if (!xen_pv_domain())
+		return;
 	xenbus_unregister_driver(&xen_pcibk_driver);
 }

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

[Stefano Stabellini]

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

On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
> On 20.09.21 14:30, Juergen Gross wrote:
> > On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
> >> Hello, Stefano!
> >>
> >> On 18.09.21 00:45, Stefano Stabellini wrote:
> >>> Hi Oleksandr,
> >>>
> >>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
> >>> device in Dom0 so that it can be safely assigned to a DomU?
> >> Not only that
> >>>
> >>> I am asking because actually I don't think we want to enable the PV PCI
> >>> backend feature of pciback on ARM, right? That would clash with the PCI
> >>> assignment work you have been doing in Xen. They couldn't both work at
> >>> the same time.
> >> Correct, it is not used
> >>>
> >>> If we only need pciback to "park" a device in Dom0, wouldn't it be
> >>> possible and better to use pci-stub instead?
> >>
> >> Not only that, so pci-stub is not enough
> >>
> >> The functionality which is implemented by the pciback and the toolstack
> >> and which is relevant/missing/needed for ARM:
> >>
> >> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
> >>
> >> We have previously discussed on xen-devel ML possible solutions to that as from the
> >> above we see that pciback functionality is going to be only partially used on Arm.
> >>
> >> Please see [1] and [2]:
> >>
> >> 1. It is not acceptable to manage the assignable list in Xen itself
> >>
> >> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
> >> the rest like vPCI etc.
> >>
> >> 3. pcifront is not used on Arm
> >
> > It is neither in x86 PVH/HVM guests.
> Didn't know that, thank you for pointing
> >
> >> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
> >> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
> >> all the goals above.
> >>
> >> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
> >> parts then it can be done, yet the patch is going to be the very first brick in that building.
> >
> > Doing this split should be done, as the pcifront specific part could be
> > omitted on x86, too, in case no PV guests using PCI passthrough have to
> > be supported.
> Agree, that the final solution should have the driver split
> >
> >> So, I think this patch is still going to be needed besides which direction we take.
> >
> > Some kind of this patch, yes. It might look different in case the split
> > is done first.
> >
> > I don't mind doing it in either sequence.
> >
> With this patch we have Arm on the same page as the above mentioned x86 guests,
> 
> e.g. the driver has unused code, but yet allows Arm to function now.
> 
> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
> 
> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
> 
> have enough bandwidth for that piece of work at the moment.

That's fair and I don't want to scope-creep this simple patch asking for
an enormous rework. At the same time I don't think we should enable the
whole of pciback on ARM because it would be erroneous and confusing.

I am wonder if there is a simple:

if (!xen_pv_domain())
    return;

That we could add in a couple of places in pciback to stop it from
initializing the parts we don't care about. Something along these lines
(untested and probably incomplete).

What do you guys think?


diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index da34ce85dc88..991ba0a9b359 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -15,6 +15,7 @@
 #include <xen/xenbus.h>
 #include <xen/events.h>
 #include <xen/pci.h>
+#include <xen/xen.h>
 #include "pciback.h"
 
 #define INVALID_EVTCHN_IRQ  (-1)
@@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
 				const struct xenbus_device_id *id)
 {
 	int err = 0;
-	struct xen_pcibk_device *pdev = alloc_pdev(dev);
+	struct xen_pcibk_device *pdev;
+
+	if (!xen_pv_domain())
+		return 0;
 
+	pdev = alloc_pdev(dev);
 	if (pdev == NULL) {
 		err = -ENOMEM;
 		xenbus_dev_fatal(dev, err,
@@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
 
 int __init xen_pcibk_xenbus_register(void)
 {
+	if (!xen_pv_domain())
+		return 0;
+
 	xen_pcibk_backend = &xen_pcibk_vpci_backend;
 	if (passthrough)
 		xen_pcibk_backend = &xen_pcibk_passthrough_backend;
@@ -752,5 +760,7 @@ int __init xen_pcibk_xenbus_register(void)
 
 void __exit xen_pcibk_xenbus_unregister(void)
 {
+	if (!xen_pv_domain())
+		return;
 	xenbus_unregister_driver(&xen_pcibk_driver);
 }

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

[Oleksandr Andrushchenko]

Hello, Juergen!

On 20.09.21 14:30, Juergen Gross wrote:
> On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
>> Hello, Stefano!
>>
>> On 18.09.21 00:45, Stefano Stabellini wrote:
>>> Hi Oleksandr,
>>>
>>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>>> device in Dom0 so that it can be safely assigned to a DomU?
>> Not only that
>>>
>>> I am asking because actually I don't think we want to enable the PV PCI
>>> backend feature of pciback on ARM, right? That would clash with the PCI
>>> assignment work you have been doing in Xen. They couldn't both work at
>>> the same time.
>> Correct, it is not used
>>>
>>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>>> possible and better to use pci-stub instead?
>>
>> Not only that, so pci-stub is not enough
>>
>> The functionality which is implemented by the pciback and the toolstack
>> and which is relevant/missing/needed for ARM:
>>
>> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
>>
>> We have previously discussed on xen-devel ML possible solutions to that as from the
>> above we see that pciback functionality is going to be only partially used on Arm.
>>
>> Please see [1] and [2]:
>>
>> 1. It is not acceptable to manage the assignable list in Xen itself
>>
>> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
>> the rest like vPCI etc.
>>
>> 3. pcifront is not used on Arm
>
> It is neither in x86 PVH/HVM guests.
Didn't know that, thank you for pointing
>
>> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
>> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
>> all the goals above.
>>
>> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
>> parts then it can be done, yet the patch is going to be the very first brick in that building.
>
> Doing this split should be done, as the pcifront specific part could be
> omitted on x86, too, in case no PV guests using PCI passthrough have to
> be supported.
Agree, that the final solution should have the driver split
>
>> So, I think this patch is still going to be needed besides which direction we take.
>
> Some kind of this patch, yes. It might look different in case the split
> is done first.
>
> I don't mind doing it in either sequence.
>
With this patch we have Arm on the same page as the above mentioned x86 guests,

e.g. the driver has unused code, but yet allows Arm to function now.

At this stage of PCI passthrough on Arm it is yet enough. Long term, when

the driver gets split, Arm will benefit from that split too, but unfortunately I do not

have enough bandwidth for that piece of work at the moment.

>
> Juergen

Thank you,

Oleksandr

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

[Juergen Gross]

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

On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
> Hello, Stefano!
> 
> On 18.09.21 00:45, Stefano Stabellini wrote:
>> Hi Oleksandr,
>>
>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
>> device in Dom0 so that it can be safely assigned to a DomU?
> Not only that
>>
>> I am asking because actually I don't think we want to enable the PV PCI
>> backend feature of pciback on ARM, right? That would clash with the PCI
>> assignment work you have been doing in Xen. They couldn't both work at
>> the same time.
> Correct, it is not used
>>
>> If we only need pciback to "park" a device in Dom0, wouldn't it be
>> possible and better to use pci-stub instead?
> 
> Not only that, so pci-stub is not enough
> 
> The functionality which is implemented by the pciback and the toolstack
> and which is relevant/missing/needed for ARM:
> 
> 1. pciback 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. pciback is used to hold the unbound PCI devices, 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
> 
> We have previously discussed on xen-devel ML possible solutions to that as from the
> above we see that pciback functionality is going to be only partially used on Arm.
> 
> Please see [1] and [2]:
> 
> 1. It is not acceptable to manage the assignable list in Xen itself
> 
> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
> the rest like vPCI etc.
> 
> 3. pcifront is not used on Arm

It is neither in x86 PVH/HVM guests.

> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
> all the goals above.
> 
> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
> parts then it can be done, yet the patch is going to be the very first brick in that building.

Doing this split should be done, as the pcifront specific part could be
omitted on x86, too, in case no PV guests using PCI passthrough have to
be supported.

> So, I think this patch is still going to be needed besides which direction we take.

Some kind of this patch, yes. It might look different in case the split
is done first.

I don't mind doing it in either sequence.


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] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

Hello, Stefano!

On 18.09.21 00:45, Stefano Stabellini wrote:
> Hi Oleksandr,
>
> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
> device in Dom0 so that it can be safely assigned to a DomU?
Not only that
>
> I am asking because actually I don't think we want to enable the PV PCI
> backend feature of pciback on ARM, right? That would clash with the PCI
> assignment work you have been doing in Xen. They couldn't both work at
> the same time.
Correct, it is not used
>
> If we only need pciback to "park" a device in Dom0, wouldn't it be
> possible and better to use pci-stub instead?

Not only that, so pci-stub is not enough

The functionality which is implemented by the pciback and the toolstack
and which is relevant/missing/needed for ARM:

1. pciback 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. pciback is used to hold the unbound PCI devices, 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

We have previously discussed on xen-devel ML possible solutions to that as from the

above we see that pciback functionality is going to be only partially used on Arm.

Please see [1] and [2]:

1. It is not acceptable to manage the assignable list in Xen itself

2. pciback can be split into two parts: PCI assignable/bind/reset handling and

the rest like vPCI etc.

3. pcifront is not used on Arm

So, limited use of the pciback is one of the bricks used to enable PCI passthrough

on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve

all the goals above.

If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"

parts then it can be done, yet the patch is going to be the very first brick in that building.

So, I think this patch is still going to be needed besides which direction we take.

Hope this sheds some light,

Oleksandr

>
>
> On Fri, 17 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>
>>
>> ---
>> Tested on Arm and x86.
>> ---
>>   arch/x86/include/asm/xen/pci.h     | 24 ----------
>>   arch/x86/pci/xen.c                 | 74 +----------------------------
>>   drivers/xen/Kconfig                |  2 +-
>>   drivers/xen/events/events_base.c   |  1 +
>>   drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>>   drivers/xen/xen-pciback/pci_stub.c |  3 +-
>>   drivers/xen/xen-pciback/xenbus.c   |  2 +-
>>   include/xen/pci.h                  | 34 ++++++++++++++
>>   8 files changed, 115 insertions(+), 100 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..9ff7b49bca08 100644
>> --- a/arch/x86/include/asm/xen/pci.h
>> +++ b/arch/x86/include/asm/xen/pci.h
>> @@ -14,30 +14,6 @@ static inline int pci_xen_hvm_init(void)
>>   	return -1;
>>   }
>>   #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;
>> -}
>> -#endif
>>   
>>   #if defined(CONFIG_PCI_MSI)
>>   #if defined(CONFIG_PCI_XEN)
>> 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/events/events_base.c b/drivers/xen/events/events_base.c
>> index a78704ae3618..35493ff0d146 100644
>> --- a/drivers/xen/events/events_base.c
>> +++ b/drivers/xen/events/events_base.c
>> @@ -65,6 +65,7 @@
>>   #include <xen/interface/vcpu.h>
>>   #include <xen/xenbus.h>
>>   #include <asm/hw_irq.h>
>> +#include <xen/pci.h>
>>   
>>   #include "events_internal.h"
>>   
>> 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..5c988d5ff38f
>> --- /dev/null
>> +++ b/include/xen/pci.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __XEN_PCI_H__
>> +#define __XEN_PCI_H__
>> +
>> +#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;
>> +}
>> +#endif
>> +
>> +#endif
>> -- 
>> 2.25.1
>>
[1] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg84452.html

[2] https://patchwork.kernel.org/project/xen-devel/patch/20201109125031.26409-3-andr2000@gmail.com/

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

[Stefano Stabellini]

Hi Oleksandr,

Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
device in Dom0 so that it can be safely assigned to a DomU?

I am asking because actually I don't think we want to enable the PV PCI
backend feature of pciback on ARM, right? That would clash with the PCI
assignment work you have been doing in Xen. They couldn't both work at
the same time.

If we only need pciback to "park" a device in Dom0, wouldn't it be
possible and better to use pci-stub instead?


On Fri, 17 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>
> 
> ---
> Tested on Arm and x86.
> ---
>  arch/x86/include/asm/xen/pci.h     | 24 ----------
>  arch/x86/pci/xen.c                 | 74 +----------------------------
>  drivers/xen/Kconfig                |  2 +-
>  drivers/xen/events/events_base.c   |  1 +
>  drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>  drivers/xen/xen-pciback/pci_stub.c |  3 +-
>  drivers/xen/xen-pciback/xenbus.c   |  2 +-
>  include/xen/pci.h                  | 34 ++++++++++++++
>  8 files changed, 115 insertions(+), 100 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..9ff7b49bca08 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -14,30 +14,6 @@ static inline int pci_xen_hvm_init(void)
>  	return -1;
>  }
>  #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;
> -}
> -#endif
>  
>  #if defined(CONFIG_PCI_MSI)
>  #if defined(CONFIG_PCI_XEN)
> 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/events/events_base.c b/drivers/xen/events/events_base.c
> index a78704ae3618..35493ff0d146 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -65,6 +65,7 @@
>  #include <xen/interface/vcpu.h>
>  #include <xen/xenbus.h>
>  #include <asm/hw_irq.h>
> +#include <xen/pci.h>
>  
>  #include "events_internal.h"
>  
> 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..5c988d5ff38f
> --- /dev/null
> +++ b/include/xen/pci.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __XEN_PCI_H__
> +#define __XEN_PCI_H__
> +
> +#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;
> +}
> +#endif
> +
> +#endif
> -- 
> 2.25.1
> 

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

[Stefano Stabellini]

Hi Oleksandr,

Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
device in Dom0 so that it can be safely assigned to a DomU?

I am asking because actually I don't think we want to enable the PV PCI
backend feature of pciback on ARM, right? That would clash with the PCI
assignment work you have been doing in Xen. They couldn't both work at
the same time.

If we only need pciback to "park" a device in Dom0, wouldn't it be
possible and better to use pci-stub instead?


On Fri, 17 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>
> 
> ---
> Tested on Arm and x86.
> ---
>  arch/x86/include/asm/xen/pci.h     | 24 ----------
>  arch/x86/pci/xen.c                 | 74 +----------------------------
>  drivers/xen/Kconfig                |  2 +-
>  drivers/xen/events/events_base.c   |  1 +
>  drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>  drivers/xen/xen-pciback/pci_stub.c |  3 +-
>  drivers/xen/xen-pciback/xenbus.c   |  2 +-
>  include/xen/pci.h                  | 34 ++++++++++++++
>  8 files changed, 115 insertions(+), 100 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..9ff7b49bca08 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -14,30 +14,6 @@ static inline int pci_xen_hvm_init(void)
>  	return -1;
>  }
>  #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;
> -}
> -#endif
>  
>  #if defined(CONFIG_PCI_MSI)
>  #if defined(CONFIG_PCI_XEN)
> 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/events/events_base.c b/drivers/xen/events/events_base.c
> index a78704ae3618..35493ff0d146 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -65,6 +65,7 @@
>  #include <xen/interface/vcpu.h>
>  #include <xen/xenbus.h>
>  #include <asm/hw_irq.h>
> +#include <xen/pci.h>
>  
>  #include "events_internal.h"
>  
> 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..5c988d5ff38f
> --- /dev/null
> +++ b/include/xen/pci.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __XEN_PCI_H__
> +#define __XEN_PCI_H__
> +
> +#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;
> +}
> +#endif
> +
> +#endif
> -- 
> 2.25.1
> 

[PATCH] 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>

---
Tested on Arm and x86.
---
 arch/x86/include/asm/xen/pci.h     | 24 ----------
 arch/x86/pci/xen.c                 | 74 +----------------------------
 drivers/xen/Kconfig                |  2 +-
 drivers/xen/events/events_base.c   |  1 +
 drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
 drivers/xen/xen-pciback/pci_stub.c |  3 +-
 drivers/xen/xen-pciback/xenbus.c   |  2 +-
 include/xen/pci.h                  | 34 ++++++++++++++
 8 files changed, 115 insertions(+), 100 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..9ff7b49bca08 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -14,30 +14,6 @@ static inline int pci_xen_hvm_init(void)
 	return -1;
 }
 #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;
-}
-#endif
 
 #if defined(CONFIG_PCI_MSI)
 #if defined(CONFIG_PCI_XEN)
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/events/events_base.c b/drivers/xen/events/events_base.c
index a78704ae3618..35493ff0d146 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -65,6 +65,7 @@
 #include <xen/interface/vcpu.h>
 #include <xen/xenbus.h>
 #include <asm/hw_irq.h>
+#include <xen/pci.h>
 
 #include "events_internal.h"
 
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..5c988d5ff38f
--- /dev/null
+++ b/include/xen/pci.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __XEN_PCI_H__
+#define __XEN_PCI_H__
+
+#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;
+}
+#endif
+
+#endif
-- 
2.25.1