From: Oleksandr Andrushchenko <[email protected]>
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 <[email protected]>
---
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
From: Oleksandr Andrushchenko <[email protected]>
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 <[email protected]>
Signed-off-by: Anastasiia Lukianenko <[email protected]>
---
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
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
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
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
>
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
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 <[email protected]>
>
> ---
> 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
>
On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Anastasiia Lukianenko <[email protected]>
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
>
On 23.09.21 23:00, Stefano Stabellini wrote:
> On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Anastasiia Lukianenko <[email protected]>
> 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
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 <[email protected]>
>>
>> ---
>> 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
>>
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Anastasiia Lukianenko <[email protected]>
>> 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?
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 <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> Signed-off-by: Anastasiia Lukianenko <[email protected]>
> >> 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.
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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Anastasiia Lukianenko <[email protected]>
>>>> 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.