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

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

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Currently PCI backend implements multiple functionalities at a time.
To name a few:
1. It is used as a database for assignable PCI devices, e.g. xl
   pci-assignable-{add|remove|list} manipulates that list. So, whenever
   the toolstack needs to know which PCI devices can be passed through
   it reads that from the relevant sysfs entries of the pciback.
2. It is used to hold the unbound PCI devices list, e.g. when passing
   through a PCI device it needs to be unbound from the relevant device
   driver and bound to pciback (strictly speaking it is not required
   that the device is bound to pciback, but pciback is again used as a
   database of the passed through PCI devices, so we can re-bind the
   devices back to their original drivers when guest domain shuts down)
3. Device reset for the devices being passed through
4. Para-virtualized use-cases support

The para-virtualized part of the driver is not always needed as some
architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
model for PCI device passthrough. For such use-cases make the very
first step in splitting the xen-pciback driver into two parts: extended
PCI stub and PCI PV backend drivers. At the moment x86 platform will
continue using CONFIG_XEN_PCIDEV_BACKEND for the fully featured backend
driver and new platforms may build a driver with limited functionality
(no PV) by enabling CONFIG_XEN_PCIDEV_STUB.

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

---
Changes since v2:
 - swap the patch order
New in v2
---
 drivers/xen/Kconfig               | 24 ++++++++++++++++++++++++
 drivers/xen/Makefile              |  2 +-
 drivers/xen/xen-pciback/Makefile  |  1 +
 drivers/xen/xen-pciback/pciback.h |  5 +++++
 drivers/xen/xen-pciback/xenbus.c  |  6 +++++-
 5 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index a37eb52fb401..6e92c6be19f1 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -180,10 +180,34 @@ config SWIOTLB_XEN
 	select DMA_OPS
 	select SWIOTLB
 
+config XEN_PCI_STUB
+	bool
+
+config XEN_PCIDEV_STUB
+	tristate "Xen PCI-device stub driver"
+	depends on PCI && !X86 && XEN
+	depends on XEN_BACKEND
+	select XEN_PCI_STUB
+	default m
+	help
+	  The PCI device stub driver provides limited version of the PCI
+	  device backend driver without para-virtualized support for guests.
+	  If you select this to be a module, you will need to make sure no
+	  other driver has bound to the device(s) you want to make visible to
+	  other guests.
+
+	  The "hide" parameter (only applicable if backend driver is compiled
+	  into the kernel) allows you to bind the PCI devices to this module
+	  from the default device drivers. The argument is the list of PCI BDFs:
+	  xen-pciback.hide=(03:00.0)(04:00.0)
+
+	  If in doubt, say m.
+
 config XEN_PCIDEV_BACKEND
 	tristate "Xen PCI-device backend driver"
 	depends on PCI && X86 && XEN
 	depends on XEN_BACKEND
+	select XEN_PCI_STUB
 	default m
 	help
 	  The PCI device backend driver allows the kernel to export arbitrary
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 3434593455b2..5aae66e638a7 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -24,7 +24,7 @@ obj-$(CONFIG_XEN_SYS_HYPERVISOR)	+= sys-hypervisor.o
 obj-$(CONFIG_XEN_PVHVM_GUEST)		+= platform-pci.o
 obj-$(CONFIG_SWIOTLB_XEN)		+= swiotlb-xen.o
 obj-$(CONFIG_XEN_MCE_LOG)		+= mcelog.o
-obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= xen-pciback/
+obj-$(CONFIG_XEN_PCI_STUB)	        += xen-pciback/
 obj-$(CONFIG_XEN_PRIVCMD)		+= xen-privcmd.o
 obj-$(CONFIG_XEN_ACPI_PROCESSOR)	+= xen-acpi-processor.o
 obj-$(CONFIG_XEN_EFI)			+= efi.o
diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
index e8d981d43235..e2cb376444a6 100644
--- a/drivers/xen/xen-pciback/Makefile
+++ b/drivers/xen/xen-pciback/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
+obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
 
 xen-pciback-y := pci_stub.o pciback_ops.o xenbus.o
 xen-pciback-y += conf_space.o conf_space_header.o \
diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
index 95e28ee48d52..9a64196e831d 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -71,6 +71,11 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
 				    struct pci_dev *dev);
 void pcistub_put_pci_dev(struct pci_dev *dev);
 
+static inline bool xen_pcibk_pv_support(void)
+{
+	return IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND);
+}
+
 /* Ensure a device is turned off or reset */
 void xen_pcibk_reset_device(struct pci_dev *pdev);
 
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index c09c7ebd6968..f8ba2903a3ff 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -743,6 +743,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
 
 int __init xen_pcibk_xenbus_register(void)
 {
+	if (!xen_pcibk_pv_support())
+		return 0;
+
 	xen_pcibk_backend = &xen_pcibk_vpci_backend;
 	if (passthrough)
 		xen_pcibk_backend = &xen_pcibk_passthrough_backend;
@@ -752,5 +755,6 @@ int __init xen_pcibk_xenbus_register(void)
 
 void __exit xen_pcibk_xenbus_unregister(void)
 {
-	xenbus_unregister_driver(&xen_pcibk_driver);
+	if (xen_pcibk_pv_support())
+		xenbus_unregister_driver(&xen_pcibk_driver);
 }
-- 
2.25.1

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

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

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

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

---
Since 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     | 18 +------
 arch/x86/pci/xen.c                 | 74 +----------------------------
 drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
 drivers/xen/xen-pciback/pci_stub.c |  3 +-
 drivers/xen/xen-pciback/xenbus.c   |  2 +-
 include/xen/pci.h                  | 28 +++++++++++
 6 files changed, 108 insertions(+), 92 deletions(-)
 create mode 100644 include/xen/pci.h

diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
index 3506d8c598c1..2889f091f459 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -16,26 +16,10 @@ static inline int pci_xen_hvm_init(void)
 #endif
 #if defined(CONFIG_XEN_DOM0)
 int __init pci_xen_initial_domain(void);
-int xen_find_device_domain_owner(struct pci_dev *dev);
-int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
-int xen_unregister_device_domain_owner(struct pci_dev *dev);
 #else
 static inline int __init pci_xen_initial_domain(void)
 {
-	return -1;
-}
-static inline int xen_find_device_domain_owner(struct pci_dev *dev)
-{
-	return -1;
-}
-static inline int xen_register_device_domain_owner(struct pci_dev *dev,
-						   uint16_t domain)
-{
-	return -1;
-}
-static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
-{
-	return -1;
+       return -1;
 }
 #endif
 
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 3d41a09c2c14..4a45b0bf9ae4 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -23,6 +23,7 @@
 
 #include <xen/features.h>
 #include <xen/events.h>
+#include <xen/pci.h>
 #include <asm/xen/pci.h>
 #include <asm/xen/cpuid.h>
 #include <asm/apic.h>
@@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
 	}
 	return 0;
 }
-
-struct xen_device_domain_owner {
-	domid_t domain;
-	struct pci_dev *dev;
-	struct list_head list;
-};
-
-static DEFINE_SPINLOCK(dev_domain_list_spinlock);
-static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
-
-static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
-{
-	struct xen_device_domain_owner *owner;
-
-	list_for_each_entry(owner, &dev_domain_list, list) {
-		if (owner->dev == dev)
-			return owner;
-	}
-	return NULL;
-}
-
-int xen_find_device_domain_owner(struct pci_dev *dev)
-{
-	struct xen_device_domain_owner *owner;
-	int domain = -ENODEV;
-
-	spin_lock(&dev_domain_list_spinlock);
-	owner = find_device(dev);
-	if (owner)
-		domain = owner->domain;
-	spin_unlock(&dev_domain_list_spinlock);
-	return domain;
-}
-EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
-
-int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
-{
-	struct xen_device_domain_owner *owner;
-
-	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
-	if (!owner)
-		return -ENODEV;
-
-	spin_lock(&dev_domain_list_spinlock);
-	if (find_device(dev)) {
-		spin_unlock(&dev_domain_list_spinlock);
-		kfree(owner);
-		return -EEXIST;
-	}
-	owner->domain = domain;
-	owner->dev = dev;
-	list_add_tail(&owner->list, &dev_domain_list);
-	spin_unlock(&dev_domain_list_spinlock);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
-
-int xen_unregister_device_domain_owner(struct pci_dev *dev)
-{
-	struct xen_device_domain_owner *owner;
-
-	spin_lock(&dev_domain_list_spinlock);
-	owner = find_device(dev);
-	if (!owner) {
-		spin_unlock(&dev_domain_list_spinlock);
-		return -ENODEV;
-	}
-	list_del(&owner->list);
-	spin_unlock(&dev_domain_list_spinlock);
-	kfree(owner);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
 #endif
diff --git a/drivers/xen/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 f8ba2903a3ff..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)
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 v3 1/2] xen-pciback: prepare for the split for stub and PV

[Jan Beulich]

On 23.09.2021 11:53, Oleksandr Andrushchenko wrote:
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -180,10 +180,34 @@ config SWIOTLB_XEN
>  	select DMA_OPS
>  	select SWIOTLB
>  
> +config XEN_PCI_STUB
> +	bool
> +
> +config XEN_PCIDEV_STUB
> +	tristate "Xen PCI-device stub driver"
> +	depends on PCI && !X86 && XEN
> +	depends on XEN_BACKEND
> +	select XEN_PCI_STUB
> +	default m
> +	help
> +	  The PCI device stub driver provides limited version of the PCI
> +	  device backend driver without para-virtualized support for guests.
> +	  If you select this to be a module, you will need to make sure no
> +	  other driver has bound to the device(s) you want to make visible to
> +	  other guests.
> +
> +	  The "hide" parameter (only applicable if backend driver is compiled
> +	  into the kernel) allows you to bind the PCI devices to this module
> +	  from the default device drivers. The argument is the list of PCI BDFs:
> +	  xen-pciback.hide=(03:00.0)(04:00.0)
> +
> +	  If in doubt, say m.
> +
>  config XEN_PCIDEV_BACKEND
>  	tristate "Xen PCI-device backend driver"
>  	depends on PCI && X86 && XEN
>  	depends on XEN_BACKEND
> +	select XEN_PCI_STUB

Does kconfig not at least warn about this? The selected item has a
"depends on !X88" conflicting with the "depends on X86" here.

Jan

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

[Juergen Gross]

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

On 23.09.21 13:10, Jan Beulich wrote:
> On 23.09.2021 11:53, Oleksandr Andrushchenko wrote:
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -180,10 +180,34 @@ config SWIOTLB_XEN
>>   	select DMA_OPS
>>   	select SWIOTLB
>>   
>> +config XEN_PCI_STUB
>> +	bool
>> +
>> +config XEN_PCIDEV_STUB
>> +	tristate "Xen PCI-device stub driver"
>> +	depends on PCI && !X86 && XEN
>> +	depends on XEN_BACKEND
>> +	select XEN_PCI_STUB
>> +	default m
>> +	help
>> +	  The PCI device stub driver provides limited version of the PCI
>> +	  device backend driver without para-virtualized support for guests.
>> +	  If you select this to be a module, you will need to make sure no
>> +	  other driver has bound to the device(s) you want to make visible to
>> +	  other guests.
>> +
>> +	  The "hide" parameter (only applicable if backend driver is compiled
>> +	  into the kernel) allows you to bind the PCI devices to this module
>> +	  from the default device drivers. The argument is the list of PCI BDFs:
>> +	  xen-pciback.hide=(03:00.0)(04:00.0)
>> +
>> +	  If in doubt, say m.
>> +
>>   config XEN_PCIDEV_BACKEND
>>   	tristate "Xen PCI-device backend driver"
>>   	depends on PCI && X86 && XEN
>>   	depends on XEN_BACKEND
>> +	select XEN_PCI_STUB
> 
> Does kconfig not at least warn about this? The selected item has a
> "depends on !X88" conflicting with the "depends on X86" here.

XEN_PCI_STUB != XEN_PCIDEV_STUB


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 v3 1/2] xen-pciback: prepare for the split for stub and PV

[Oleksandr Andrushchenko]

On 23.09.21 14:10, Jan Beulich wrote:
> On 23.09.2021 11:53, Oleksandr Andrushchenko wrote:
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -180,10 +180,34 @@ config SWIOTLB_XEN
>>   	select DMA_OPS
>>   	select SWIOTLB
>>   
>> +config XEN_PCI_STUB
>> +	bool
>> +
>> +config XEN_PCIDEV_STUB
>> +	tristate "Xen PCI-device stub driver"
>> +	depends on PCI && !X86 && XEN
>> +	depends on XEN_BACKEND
>> +	select XEN_PCI_STUB
>> +	default m
>> +	help
>> +	  The PCI device stub driver provides limited version of the PCI
>> +	  device backend driver without para-virtualized support for guests.
>> +	  If you select this to be a module, you will need to make sure no
>> +	  other driver has bound to the device(s) you want to make visible to
>> +	  other guests.
>> +
>> +	  The "hide" parameter (only applicable if backend driver is compiled
>> +	  into the kernel) allows you to bind the PCI devices to this module
>> +	  from the default device drivers. The argument is the list of PCI BDFs:
>> +	  xen-pciback.hide=(03:00.0)(04:00.0)
>> +
>> +	  If in doubt, say m.
>> +
>>   config XEN_PCIDEV_BACKEND
>>   	tristate "Xen PCI-device backend driver"
>>   	depends on PCI && X86 && XEN
>>   	depends on XEN_BACKEND
>> +	select XEN_PCI_STUB
> Does kconfig not at least warn about this? The selected item has a
> "depends on !X88" conflicting with the "depends on X86" here.

Why?

XEN_PCIDEV_STUB depends on PCI && !X86 && XEN

XEN_PCI_STUB has nothing, just bool

XEN_PCIDEV_BACKEND selects XEN_PCI_STUB

>
> Jan
>

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

[Jan Beulich]

On 23.09.2021 13:12, Juergen Gross wrote:
> On 23.09.21 13:10, Jan Beulich wrote:
>> On 23.09.2021 11:53, Oleksandr Andrushchenko wrote:
>>> --- a/drivers/xen/Kconfig
>>> +++ b/drivers/xen/Kconfig
>>> @@ -180,10 +180,34 @@ config SWIOTLB_XEN
>>>   	select DMA_OPS
>>>   	select SWIOTLB
>>>   
>>> +config XEN_PCI_STUB
>>> +	bool
>>> +
>>> +config XEN_PCIDEV_STUB
>>> +	tristate "Xen PCI-device stub driver"
>>> +	depends on PCI && !X86 && XEN
>>> +	depends on XEN_BACKEND
>>> +	select XEN_PCI_STUB
>>> +	default m
>>> +	help
>>> +	  The PCI device stub driver provides limited version of the PCI
>>> +	  device backend driver without para-virtualized support for guests.
>>> +	  If you select this to be a module, you will need to make sure no
>>> +	  other driver has bound to the device(s) you want to make visible to
>>> +	  other guests.
>>> +
>>> +	  The "hide" parameter (only applicable if backend driver is compiled
>>> +	  into the kernel) allows you to bind the PCI devices to this module
>>> +	  from the default device drivers. The argument is the list of PCI BDFs:
>>> +	  xen-pciback.hide=(03:00.0)(04:00.0)
>>> +
>>> +	  If in doubt, say m.
>>> +
>>>   config XEN_PCIDEV_BACKEND
>>>   	tristate "Xen PCI-device backend driver"
>>>   	depends on PCI && X86 && XEN
>>>   	depends on XEN_BACKEND
>>> +	select XEN_PCI_STUB
>>
>> Does kconfig not at least warn about this? The selected item has a
>> "depends on !X88" conflicting with the "depends on X86" here.
> 
> XEN_PCI_STUB != XEN_PCIDEV_STUB

Oh, sorry.

Jan

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

[Stefano Stabellini]

On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
> Currently PCI backend implements multiple functionalities at a time.
> To name a few:
> 1. It is used as a database for assignable PCI devices, e.g. xl
>    pci-assignable-{add|remove|list} manipulates that list. So, whenever
>    the toolstack needs to know which PCI devices can be passed through
>    it reads that from the relevant sysfs entries of the pciback.
> 2. It is used to hold the unbound PCI devices list, e.g. when passing
>    through a PCI device it needs to be unbound from the relevant device
>    driver and bound to pciback (strictly speaking it is not required
>    that the device is bound to pciback, but pciback is again used as a
>    database of the passed through PCI devices, so we can re-bind the
>    devices back to their original drivers when guest domain shuts down)
> 3. Device reset for the devices being passed through
> 4. Para-virtualized use-cases support
> 
> The para-virtualized part of the driver is not always needed as some
> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
> model for PCI device passthrough. For such use-cases make the very
> first step in splitting the xen-pciback driver into two parts: extended
> PCI stub and PCI PV backend drivers. At the moment x86 platform will
> continue using CONFIG_XEN_PCIDEV_BACKEND for the fully featured backend
> driver and new platforms may build a driver with limited functionality
> (no PV) by enabling CONFIG_XEN_PCIDEV_STUB.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> ---
> Changes since v2:
>  - swap the patch order
> New in v2
> ---
>  drivers/xen/Kconfig               | 24 ++++++++++++++++++++++++
>  drivers/xen/Makefile              |  2 +-
>  drivers/xen/xen-pciback/Makefile  |  1 +
>  drivers/xen/xen-pciback/pciback.h |  5 +++++
>  drivers/xen/xen-pciback/xenbus.c  |  6 +++++-
>  5 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index a37eb52fb401..6e92c6be19f1 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -180,10 +180,34 @@ config SWIOTLB_XEN
>  	select DMA_OPS
>  	select SWIOTLB
>  
> +config XEN_PCI_STUB
> +	bool
> +
> +config XEN_PCIDEV_STUB
> +	tristate "Xen PCI-device stub driver"
> +	depends on PCI && !X86 && XEN
> +	depends on XEN_BACKEND
> +	select XEN_PCI_STUB
> +	default m
> +	help
> +	  The PCI device stub driver provides limited version of the PCI
> +	  device backend driver without para-virtualized support for guests.
> +	  If you select this to be a module, you will need to make sure no
> +	  other driver has bound to the device(s) you want to make visible to
> +	  other guests.
> +
> +	  The "hide" parameter (only applicable if backend driver is compiled
> +	  into the kernel) allows you to bind the PCI devices to this module
> +	  from the default device drivers. The argument is the list of PCI BDFs:
> +	  xen-pciback.hide=(03:00.0)(04:00.0)
> +
> +	  If in doubt, say m.
> +

I get this build error on ARM:

drivers/xen/xen-pciback/pci_stub.c:22:10: fatal error: asm/xen/pci.h: No such file or directory
 #include <asm/xen/pci.h>
          ^~~~~~~~~~~~~~~
compilation terminated.
scripts/Makefile.build:277: recipe for target 'drivers/xen/xen-pciback/pci_stub.o' failed


I imagine that the second patch is required for the build on ARM. I
suggest to move the definition of XEN_PCIDEV_STUB to the second patch.
(keep XEN_PCI_STUB and xen_pcibk_pv_support here.)

Alternatively, just remove "tristate "Xen PCI-device stub driver"" from
here so that XEN_PCIDEV_STUB becomes non-selectable. You can add the
tristate property in patch #2.



>  config XEN_PCIDEV_BACKEND
>  	tristate "Xen PCI-device backend driver"
>  	depends on PCI && X86 && XEN
>  	depends on XEN_BACKEND
> +	select XEN_PCI_STUB
>  	default m
>  	help
>  	  The PCI device backend driver allows the kernel to export arbitrary
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 3434593455b2..5aae66e638a7 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -24,7 +24,7 @@ obj-$(CONFIG_XEN_SYS_HYPERVISOR)	+= sys-hypervisor.o
>  obj-$(CONFIG_XEN_PVHVM_GUEST)		+= platform-pci.o
>  obj-$(CONFIG_SWIOTLB_XEN)		+= swiotlb-xen.o
>  obj-$(CONFIG_XEN_MCE_LOG)		+= mcelog.o
> -obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= xen-pciback/
> +obj-$(CONFIG_XEN_PCI_STUB)	        += xen-pciback/
>  obj-$(CONFIG_XEN_PRIVCMD)		+= xen-privcmd.o
>  obj-$(CONFIG_XEN_ACPI_PROCESSOR)	+= xen-acpi-processor.o
>  obj-$(CONFIG_XEN_EFI)			+= efi.o
> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
> index e8d981d43235..e2cb376444a6 100644
> --- a/drivers/xen/xen-pciback/Makefile
> +++ b/drivers/xen/xen-pciback/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
> +obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>  
>  xen-pciback-y := pci_stub.o pciback_ops.o xenbus.o
>  xen-pciback-y += conf_space.o conf_space_header.o \
> diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
> index 95e28ee48d52..9a64196e831d 100644
> --- a/drivers/xen/xen-pciback/pciback.h
> +++ b/drivers/xen/xen-pciback/pciback.h
> @@ -71,6 +71,11 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
>  				    struct pci_dev *dev);
>  void pcistub_put_pci_dev(struct pci_dev *dev);
>  
> +static inline bool xen_pcibk_pv_support(void)
> +{
> +	return IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND);
> +}
> +
>  /* Ensure a device is turned off or reset */
>  void xen_pcibk_reset_device(struct pci_dev *pdev);
>  
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c09c7ebd6968..f8ba2903a3ff 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -743,6 +743,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>  
>  int __init xen_pcibk_xenbus_register(void)
>  {
> +	if (!xen_pcibk_pv_support())
> +		return 0;
> +
>  	xen_pcibk_backend = &xen_pcibk_vpci_backend;
>  	if (passthrough)
>  		xen_pcibk_backend = &xen_pcibk_passthrough_backend;
> @@ -752,5 +755,6 @@ int __init xen_pcibk_xenbus_register(void)
>  
>  void __exit xen_pcibk_xenbus_unregister(void)
>  {
> -	xenbus_unregister_driver(&xen_pcibk_driver);
> +	if (xen_pcibk_pv_support())
> +		xenbus_unregister_driver(&xen_pcibk_driver);
>  }
> -- 
> 2.25.1
> 

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

[Stefano Stabellini]

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

On Thu, 23 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>

The patch looks good to me. Only one thing: on ARM32 I get:

drivers/xen/xen-pciback/conf_space_header.c: In function ‘bar_init’:
drivers/xen/xen-pciback/conf_space_header.c:239:34: warning: right shift count >= width of type [-Wshift-count-overflow]
    bar->val = res[pos - 1].start >> 32;
                                  ^~
drivers/xen/xen-pciback/conf_space_header.c:240:49: warning: right shift count >= width of type [-Wshift-count-overflow]
    bar->len_val = -resource_size(&res[pos - 1]) >> 32;
 
 
resource_size_t is defined as phys_addr_t and it can be 32bit on arm32.


One fix is to surround:

		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;
			return bar;
		}

with #ifdef PHYS_ADDR_T_64BIT



> ---
> 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     | 18 +------
>  arch/x86/pci/xen.c                 | 74 +----------------------------
>  drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>  drivers/xen/xen-pciback/pci_stub.c |  3 +-
>  drivers/xen/xen-pciback/xenbus.c   |  2 +-
>  include/xen/pci.h                  | 28 +++++++++++
>  6 files changed, 108 insertions(+), 92 deletions(-)
>  create mode 100644 include/xen/pci.h
> 
> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
> index 3506d8c598c1..2889f091f459 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -16,26 +16,10 @@ static inline int pci_xen_hvm_init(void)
>  #endif
>  #if defined(CONFIG_XEN_DOM0)
>  int __init pci_xen_initial_domain(void);
> -int xen_find_device_domain_owner(struct pci_dev *dev);
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
> -int xen_unregister_device_domain_owner(struct pci_dev *dev);
>  #else
>  static inline int __init pci_xen_initial_domain(void)
>  {
> -	return -1;
> -}
> -static inline int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> -	return -1;
> -}
> -static inline int xen_register_device_domain_owner(struct pci_dev *dev,
> -						   uint16_t domain)
> -{
> -	return -1;
> -}
> -static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> -	return -1;
> +       return -1;
>  }
>  #endif
>  
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 3d41a09c2c14..4a45b0bf9ae4 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -23,6 +23,7 @@
>  
>  #include <xen/features.h>
>  #include <xen/events.h>
> +#include <xen/pci.h>
>  #include <asm/xen/pci.h>
>  #include <asm/xen/cpuid.h>
>  #include <asm/apic.h>
> @@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
>  	}
>  	return 0;
>  }
> -
> -struct xen_device_domain_owner {
> -	domid_t domain;
> -	struct pci_dev *dev;
> -	struct list_head list;
> -};
> -
> -static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> -static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> -
> -static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
> -{
> -	struct xen_device_domain_owner *owner;
> -
> -	list_for_each_entry(owner, &dev_domain_list, list) {
> -		if (owner->dev == dev)
> -			return owner;
> -	}
> -	return NULL;
> -}
> -
> -int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> -	struct xen_device_domain_owner *owner;
> -	int domain = -ENODEV;
> -
> -	spin_lock(&dev_domain_list_spinlock);
> -	owner = find_device(dev);
> -	if (owner)
> -		domain = owner->domain;
> -	spin_unlock(&dev_domain_list_spinlock);
> -	return domain;
> -}
> -EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
> -
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
> -{
> -	struct xen_device_domain_owner *owner;
> -
> -	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
> -	if (!owner)
> -		return -ENODEV;
> -
> -	spin_lock(&dev_domain_list_spinlock);
> -	if (find_device(dev)) {
> -		spin_unlock(&dev_domain_list_spinlock);
> -		kfree(owner);
> -		return -EEXIST;
> -	}
> -	owner->domain = domain;
> -	owner->dev = dev;
> -	list_add_tail(&owner->list, &dev_domain_list);
> -	spin_unlock(&dev_domain_list_spinlock);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
> -
> -int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> -	struct xen_device_domain_owner *owner;
> -
> -	spin_lock(&dev_domain_list_spinlock);
> -	owner = find_device(dev);
> -	if (!owner) {
> -		spin_unlock(&dev_domain_list_spinlock);
> -		return -ENODEV;
> -	}
> -	list_del(&owner->list);
> -	spin_unlock(&dev_domain_list_spinlock);
> -	kfree(owner);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
>  #endif
> diff --git a/drivers/xen/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 f8ba2903a3ff..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)
> 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 v3 1/2] xen-pciback: prepare for the split for stub and PV

[Oleksandr Andrushchenko]

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

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

[Oleksandr Andrushchenko]

On 23.09.21 23:00, Stefano Stabellini wrote:
> On Thu, 23 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>
> The patch looks good to me. Only one thing: on ARM32 I get:
WE do not yet support Xen PCI passthrough for ARM32
>
> drivers/xen/xen-pciback/conf_space_header.c: In function ‘bar_init’:
> drivers/xen/xen-pciback/conf_space_header.c:239:34: warning: right shift count >= width of type [-Wshift-count-overflow]
>      bar->val = res[pos - 1].start >> 32;
>                                    ^~
> drivers/xen/xen-pciback/conf_space_header.c:240:49: warning: right shift count >= width of type [-Wshift-count-overflow]
>      bar->len_val = -resource_size(&res[pos - 1]) >> 32;
>   
>   
> resource_size_t is defined as phys_addr_t and it can be 32bit on arm32.
>
>
> One fix is to surround:
>
> 		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;
> 			return bar;
> 		}
>
> with #ifdef PHYS_ADDR_T_64BIT
>
This might not be correct. We are dealing here with a 64-bit BAR on a 32-bit OS.

I think that this can still be valid use-case if BAR64.hi == 0. So, not sure

we can just skip it with ifdef.

Instead, to be on the safe side, we can have:

config XEN_PCIDEV_STUB
        tristate "Xen PCI-device stub driver"
        depends on PCI && ARM64 && XEN
e.g. only allow building the "stub" for ARM64 for now.


>
>> ---
>> 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     | 18 +------
>>   arch/x86/pci/xen.c                 | 74 +----------------------------
>>   drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>>   drivers/xen/xen-pciback/pci_stub.c |  3 +-
>>   drivers/xen/xen-pciback/xenbus.c   |  2 +-
>>   include/xen/pci.h                  | 28 +++++++++++
>>   6 files changed, 108 insertions(+), 92 deletions(-)
>>   create mode 100644 include/xen/pci.h
>>
>> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
>> index 3506d8c598c1..2889f091f459 100644
>> --- a/arch/x86/include/asm/xen/pci.h
>> +++ b/arch/x86/include/asm/xen/pci.h
>> @@ -16,26 +16,10 @@ static inline int pci_xen_hvm_init(void)
>>   #endif
>>   #if defined(CONFIG_XEN_DOM0)
>>   int __init pci_xen_initial_domain(void);
>> -int xen_find_device_domain_owner(struct pci_dev *dev);
>> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
>> -int xen_unregister_device_domain_owner(struct pci_dev *dev);
>>   #else
>>   static inline int __init pci_xen_initial_domain(void)
>>   {
>> -	return -1;
>> -}
>> -static inline int xen_find_device_domain_owner(struct pci_dev *dev)
>> -{
>> -	return -1;
>> -}
>> -static inline int xen_register_device_domain_owner(struct pci_dev *dev,
>> -						   uint16_t domain)
>> -{
>> -	return -1;
>> -}
>> -static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> -{
>> -	return -1;
>> +       return -1;
>>   }
>>   #endif
>>   
>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>> index 3d41a09c2c14..4a45b0bf9ae4 100644
>> --- a/arch/x86/pci/xen.c
>> +++ b/arch/x86/pci/xen.c
>> @@ -23,6 +23,7 @@
>>   
>>   #include <xen/features.h>
>>   #include <xen/events.h>
>> +#include <xen/pci.h>
>>   #include <asm/xen/pci.h>
>>   #include <asm/xen/cpuid.h>
>>   #include <asm/apic.h>
>> @@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
>>   	}
>>   	return 0;
>>   }
>> -
>> -struct xen_device_domain_owner {
>> -	domid_t domain;
>> -	struct pci_dev *dev;
>> -	struct list_head list;
>> -};
>> -
>> -static DEFINE_SPINLOCK(dev_domain_list_spinlock);
>> -static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
>> -
>> -static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
>> -{
>> -	struct xen_device_domain_owner *owner;
>> -
>> -	list_for_each_entry(owner, &dev_domain_list, list) {
>> -		if (owner->dev == dev)
>> -			return owner;
>> -	}
>> -	return NULL;
>> -}
>> -
>> -int xen_find_device_domain_owner(struct pci_dev *dev)
>> -{
>> -	struct xen_device_domain_owner *owner;
>> -	int domain = -ENODEV;
>> -
>> -	spin_lock(&dev_domain_list_spinlock);
>> -	owner = find_device(dev);
>> -	if (owner)
>> -		domain = owner->domain;
>> -	spin_unlock(&dev_domain_list_spinlock);
>> -	return domain;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
>> -
>> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
>> -{
>> -	struct xen_device_domain_owner *owner;
>> -
>> -	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
>> -	if (!owner)
>> -		return -ENODEV;
>> -
>> -	spin_lock(&dev_domain_list_spinlock);
>> -	if (find_device(dev)) {
>> -		spin_unlock(&dev_domain_list_spinlock);
>> -		kfree(owner);
>> -		return -EEXIST;
>> -	}
>> -	owner->domain = domain;
>> -	owner->dev = dev;
>> -	list_add_tail(&owner->list, &dev_domain_list);
>> -	spin_unlock(&dev_domain_list_spinlock);
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
>> -
>> -int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> -{
>> -	struct xen_device_domain_owner *owner;
>> -
>> -	spin_lock(&dev_domain_list_spinlock);
>> -	owner = find_device(dev);
>> -	if (!owner) {
>> -		spin_unlock(&dev_domain_list_spinlock);
>> -		return -ENODEV;
>> -	}
>> -	list_del(&owner->list);
>> -	spin_unlock(&dev_domain_list_spinlock);
>> -	kfree(owner);
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
>>   #endif
>> diff --git a/drivers/xen/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 f8ba2903a3ff..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)
>> 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 v3 2/2] xen-pciback: allow compiling on other archs than x86

[Oleksandr Andrushchenko]

On 24.09.21 08:46, Oleksandr Andrushchenko wrote:
> On 23.09.21 23:00, Stefano Stabellini wrote:
>> On Thu, 23 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>
>> The patch looks good to me. Only one thing: on ARM32 I get:
> WE do not yet support Xen PCI passthrough for ARM32
>> drivers/xen/xen-pciback/conf_space_header.c: In function ‘bar_init’:
>> drivers/xen/xen-pciback/conf_space_header.c:239:34: warning: right shift count >= width of type [-Wshift-count-overflow]
>>       bar->val = res[pos - 1].start >> 32;
>>                                     ^~
>> drivers/xen/xen-pciback/conf_space_header.c:240:49: warning: right shift count >= width of type [-Wshift-count-overflow]
>>       bar->len_val = -resource_size(&res[pos - 1]) >> 32;
>>    
>>    
>> resource_size_t is defined as phys_addr_t and it can be 32bit on arm32.
>>
>>
>> One fix is to surround:
>>
>> 		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;
>> 			return bar;
>> 		}
>>
>> with #ifdef PHYS_ADDR_T_64BIT
>>
> This might not be correct. We are dealing here with a 64-bit BAR on a 32-bit OS.
>
> I think that this can still be valid use-case if BAR64.hi == 0. So, not sure
>
> we can just skip it with ifdef.
>
> Instead, to be on the safe side, we can have:
>
> config XEN_PCIDEV_STUB
>          tristate "Xen PCI-device stub driver"
>          depends on PCI && ARM64 && XEN
> e.g. only allow building the "stub" for ARM64 for now.

Or... there are couple of places in the kernel where PCI deals with the 32 bit shift as:

drivers/pci/setup-res.c:108:        new = region.start >> 16 >> 16;
drivers/pci/iov.c:949:        new = region.start >> 16 >> 16;

commit cf7bee5a0bf270a4eace0be39329d6ac0136cc47
Date:   Sun Aug 7 13:49:59 *2005* +0400

[snip]

     Also make sure to write high bits - use "x >> 16 >> 16" (rather than the
     simpler ">> 32") to avoid warnings on 32-bit architectures where we're
     not going to have any high bits.

This might not be(?) immediately correct in case of LPAE though, e.g.

64-bit BAR may tolerate 40-bit address in some use-cases?

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

[Stefano Stabellini]

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

On Fri, 24 Sep 2021, Oleksandr Andrushchenko wrote:
> On 24.09.21 08:46, Oleksandr Andrushchenko wrote:
> > On 23.09.21 23:00, Stefano Stabellini wrote:
> >> On Thu, 23 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>
> >> The patch looks good to me. Only one thing: on ARM32 I get:
> > WE do not yet support Xen PCI passthrough for ARM32

Keep in mind that it is possible to run ARM32 guests on an ARM64
hypervisor.


> >> drivers/xen/xen-pciback/conf_space_header.c: In function ‘bar_init’:
> >> drivers/xen/xen-pciback/conf_space_header.c:239:34: warning: right shift count >= width of type [-Wshift-count-overflow]
> >>       bar->val = res[pos - 1].start >> 32;
> >>                                     ^~
> >> drivers/xen/xen-pciback/conf_space_header.c:240:49: warning: right shift count >= width of type [-Wshift-count-overflow]
> >>       bar->len_val = -resource_size(&res[pos - 1]) >> 32;
> >>    
> >>    
> >> resource_size_t is defined as phys_addr_t and it can be 32bit on arm32.
> >>
> >>
> >> One fix is to surround:
> >>
> >> 		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;
> >> 			return bar;
> >> 		}
> >>
> >> with #ifdef PHYS_ADDR_T_64BIT
> >>
> > This might not be correct. We are dealing here with a 64-bit BAR on a 32-bit OS.
> >
> > I think that this can still be valid use-case if BAR64.hi == 0. So, not sure
> >
> > we can just skip it with ifdef.
> >
> > Instead, to be on the safe side, we can have:
> >
> > config XEN_PCIDEV_STUB
> >          tristate "Xen PCI-device stub driver"
> >          depends on PCI && ARM64 && XEN
> > e.g. only allow building the "stub" for ARM64 for now.

This is a pretty drastic solution. I would be OK with it but I prefer
the solution below >> 16 >> 16.


> Or... there are couple of places in the kernel where PCI deals with the 32 bit shift as:
> 
> drivers/pci/setup-res.c:108:        new = region.start >> 16 >> 16;
> drivers/pci/iov.c:949:        new = region.start >> 16 >> 16;
> 
> commit cf7bee5a0bf270a4eace0be39329d6ac0136cc47
> Date:   Sun Aug 7 13:49:59 *2005* +0400
> 
> [snip]
> 
>      Also make sure to write high bits - use "x >> 16 >> 16" (rather than the
>      simpler ">> 32") to avoid warnings on 32-bit architectures where we're
>      not going to have any high bits.

I think this is the best option


> This might not be(?) immediately correct in case of LPAE though, e.g.
> 
> 64-bit BAR may tolerate 40-bit address in some use-cases?

It is correct for LPAE too, it is just that with LPAE it would be
unnecessary.

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

[Oleksandr Andrushchenko]

On 24.09.21 23:04, Stefano Stabellini wrote:
> On Fri, 24 Sep 2021, Oleksandr Andrushchenko wrote:
>> On 24.09.21 08:46, Oleksandr Andrushchenko wrote:
>>> On 23.09.21 23:00, Stefano Stabellini wrote:
>>>> On Thu, 23 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>
>>>> The patch looks good to me. Only one thing: on ARM32 I get:
>>> WE do not yet support Xen PCI passthrough for ARM32
> Keep in mind that it is possible to run ARM32 guests on an ARM64
> hypervisor.
>
>
>>>> drivers/xen/xen-pciback/conf_space_header.c: In function ‘bar_init’:
>>>> drivers/xen/xen-pciback/conf_space_header.c:239:34: warning: right shift count >= width of type [-Wshift-count-overflow]
>>>>        bar->val = res[pos - 1].start >> 32;
>>>>                                      ^~
>>>> drivers/xen/xen-pciback/conf_space_header.c:240:49: warning: right shift count >= width of type [-Wshift-count-overflow]
>>>>        bar->len_val = -resource_size(&res[pos - 1]) >> 32;
>>>>     
>>>>     
>>>> resource_size_t is defined as phys_addr_t and it can be 32bit on arm32.
>>>>
>>>>
>>>> One fix is to surround:
>>>>
>>>> 		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;
>>>> 			return bar;
>>>> 		}
>>>>
>>>> with #ifdef PHYS_ADDR_T_64BIT
>>>>
>>> This might not be correct. We are dealing here with a 64-bit BAR on a 32-bit OS.
>>>
>>> I think that this can still be valid use-case if BAR64.hi == 0. So, not sure
>>>
>>> we can just skip it with ifdef.
>>>
>>> Instead, to be on the safe side, we can have:
>>>
>>> config XEN_PCIDEV_STUB
>>>           tristate "Xen PCI-device stub driver"
>>>           depends on PCI && ARM64 && XEN
>>> e.g. only allow building the "stub" for ARM64 for now.
> This is a pretty drastic solution. I would be OK with it but I prefer
> the solution below >> 16 >> 16.
>
>
>> Or... there are couple of places in the kernel where PCI deals with the 32 bit shift as:
>>
>> drivers/pci/setup-res.c:108:        new = region.start >> 16 >> 16;
>> drivers/pci/iov.c:949:        new = region.start >> 16 >> 16;
>>
>> commit cf7bee5a0bf270a4eace0be39329d6ac0136cc47
>> Date:   Sun Aug 7 13:49:59 *2005* +0400
>>
>> [snip]
>>
>>       Also make sure to write high bits - use "x >> 16 >> 16" (rather than the
>>       simpler ">> 32") to avoid warnings on 32-bit architectures where we're
>>       not going to have any high bits.
> I think this is the best option

Ok, so for both patches:

1. I'll move CONFIG_XEN_PCIDEV_STUB into the second patch

2. I'll use >> 16 >> 16 to fix 32-bit architectures


>
>
>> This might not be(?) immediately correct in case of LPAE though, e.g.
>>
>> 64-bit BAR may tolerate 40-bit address in some use-cases?
> It is correct for LPAE too, it is just that with LPAE it would be
> unnecessary.