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-virtualised use-cases support
The para-virtualised part of the driver is not always needed as some
architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
model for PCI device passthrough. For such use-cases make the very
first step in splitting the xen-pciback driver into two parts: Xen
PCI stub and PCI PV backend drivers.
Signed-off-by: Oleksandr Andrushchenko <[email protected]>
---
Changes since v3:
- Move CONFIG_XEN_PCIDEV_STUB to the second patch
Changes since v2:
- swap the patch order
New in v2
---
drivers/xen/Kconfig | 4 ++++
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, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 22f5aff0c136..4efc95ddda41 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -181,10 +181,14 @@ config SWIOTLB_XEN
select DMA_OPS
select SWIOTLB
+config XEN_PCI_STUB
+ bool
+
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.
Add new configuration option CONFIG_XEN_PCIDEV_STUB, so the driver
can be limited in its functionality, e.g. no support for
para-virtualised scenario when it acts as PCI backend.
x86 platform will continue using CONFIG_XEN_PCIDEV_BACKEND
for the fully featured backend driver.
Signed-off-by: Oleksandr Andrushchenko <[email protected]>
Signed-off-by: Anastasiia Lukianenko <[email protected]>
---
Since v3:
- Fix >>32 shift for 32-bit architectures
- Move CONFIG_XEN_PCIDEV_STUB into this patch
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 | 16 -----
arch/x86/pci/xen.c | 74 +-------------------
drivers/xen/Kconfig | 20 ++++++
drivers/xen/pci.c | 75 +++++++++++++++++++++
drivers/xen/xen-pciback/conf_space_header.c | 8 ++-
drivers/xen/xen-pciback/pci_stub.c | 3 +-
drivers/xen/xen-pciback/xenbus.c | 2 +-
include/xen/pci.h | 28 ++++++++
8 files changed, 133 insertions(+), 93 deletions(-)
create mode 100644 include/xen/pci.h
diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
index 3506d8c598c1..4b08c2b809ea 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -16,27 +16,11 @@ 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;
-}
#endif
#if defined(CONFIG_PCI_MSI)
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 3d41a09c2c14..4a45b0bf9ae4 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -23,6 +23,7 @@
#include <xen/features.h>
#include <xen/events.h>
+#include <xen/pci.h>
#include <asm/xen/pci.h>
#include <asm/xen/cpuid.h>
#include <asm/apic.h>
@@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
}
return 0;
}
-
-struct xen_device_domain_owner {
- domid_t domain;
- struct pci_dev *dev;
- struct list_head list;
-};
-
-static DEFINE_SPINLOCK(dev_domain_list_spinlock);
-static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
-
-static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
-{
- struct xen_device_domain_owner *owner;
-
- list_for_each_entry(owner, &dev_domain_list, list) {
- if (owner->dev == dev)
- return owner;
- }
- return NULL;
-}
-
-int xen_find_device_domain_owner(struct pci_dev *dev)
-{
- struct xen_device_domain_owner *owner;
- int domain = -ENODEV;
-
- spin_lock(&dev_domain_list_spinlock);
- owner = find_device(dev);
- if (owner)
- domain = owner->domain;
- spin_unlock(&dev_domain_list_spinlock);
- return domain;
-}
-EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
-
-int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
-{
- struct xen_device_domain_owner *owner;
-
- owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
- if (!owner)
- return -ENODEV;
-
- spin_lock(&dev_domain_list_spinlock);
- if (find_device(dev)) {
- spin_unlock(&dev_domain_list_spinlock);
- kfree(owner);
- return -EEXIST;
- }
- owner->domain = domain;
- owner->dev = dev;
- list_add_tail(&owner->list, &dev_domain_list);
- spin_unlock(&dev_domain_list_spinlock);
- return 0;
-}
-EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
-
-int xen_unregister_device_domain_owner(struct pci_dev *dev)
-{
- struct xen_device_domain_owner *owner;
-
- spin_lock(&dev_domain_list_spinlock);
- owner = find_device(dev);
- if (!owner) {
- spin_unlock(&dev_domain_list_spinlock);
- return -ENODEV;
- }
- list_del(&owner->list);
- spin_unlock(&dev_domain_list_spinlock);
- kfree(owner);
- return 0;
-}
-EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
#endif
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 4efc95ddda41..75b9330dc6fd 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -184,6 +184,26 @@ config SWIOTLB_XEN
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
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/conf_space_header.c b/drivers/xen/xen-pciback/conf_space_header.c
index ac45cdc38e85..981435103af1 100644
--- a/drivers/xen/xen-pciback/conf_space_header.c
+++ b/drivers/xen/xen-pciback/conf_space_header.c
@@ -236,8 +236,12 @@ static void *bar_init(struct pci_dev *dev, int offset)
else {
pos = (offset - PCI_BASE_ADDRESS_0) / 4;
if (pos && (res[pos - 1].flags & IORESOURCE_MEM_64)) {
- bar->val = res[pos - 1].start >> 32;
- bar->len_val = -resource_size(&res[pos - 1]) >> 32;
+ /*
+ * Use ">> 16 >> 16" instead of direct ">> 32" shift
+ * to avoid warnings on 32-bit architectures.
+ */
+ bar->val = res[pos - 1].start >> 16 >> 16;
+ bar->len_val = -resource_size(&res[pos - 1]) >> 16 >> 16;
return bar;
}
}
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index f8e4faa96ad6..bba527620507 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -19,7 +19,8 @@
#include <linux/sched.h>
#include <linux/atomic.h>
#include <xen/events.h>
-#include <asm/xen/pci.h>
+#include <xen/pci.h>
+#include <xen/xen.h>
#include <asm/xen/hypervisor.h>
#include <xen/interface/physdev.h>
#include "pciback.h"
diff --git a/drivers/xen/xen-pciback/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 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
> 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-virtualised use-cases support
>
> The para-virtualised part of the driver is not always needed as some
> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
> model for PCI device passthrough. For such use-cases make the very
> first step in splitting the xen-pciback driver into two parts: Xen
> PCI stub and PCI PV backend drivers.
>
> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
>
> ---
> Changes since v3:
> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
I'm afraid this wasn't fully done:
> --- 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
While benign when added here, this addition still doesn't seem to
belong here.
Jan
On 27.09.21 10:26, Jan Beulich wrote:
> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>> 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-virtualised use-cases support
>>
>> The para-virtualised part of the driver is not always needed as some
>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>> model for PCI device passthrough. For such use-cases make the very
>> first step in splitting the xen-pciback driver into two parts: Xen
>> PCI stub and PCI PV backend drivers.
>>
>> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
>>
>> ---
>> Changes since v3:
>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
> I'm afraid this wasn't fully done:
>
>> --- 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
> While benign when added here, this addition still doesn't seem to
> belong here.
My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
architectures...
What would be the preference here? Stefano suggested that we still define
CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
in the second patch
Another option is just to squash the two patches.
>
> Jan
>
On 27.09.2021 09:35, Oleksandr Andrushchenko wrote:
>
> On 27.09.21 10:26, Jan Beulich wrote:
>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>> 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-virtualised use-cases support
>>>
>>> The para-virtualised part of the driver is not always needed as some
>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>> model for PCI device passthrough. For such use-cases make the very
>>> first step in splitting the xen-pciback driver into two parts: Xen
>>> PCI stub and PCI PV backend drivers.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
>>>
>>> ---
>>> Changes since v3:
>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>> I'm afraid this wasn't fully done:
>>
>>> --- 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
>> While benign when added here, this addition still doesn't seem to
>> belong here.
>
> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>
> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>
> architectures...
>
> What would be the preference here? Stefano suggested that we still define
>
> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>
> in the second patch
>
> Another option is just to squash the two patches.
I'd suggest to do so, but I'm not the maintainer of this code.
Jan
On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>
> On 27.09.21 10:26, Jan Beulich wrote:
>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>> 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-virtualised use-cases support
>>>
>>> The para-virtualised part of the driver is not always needed as some
>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>> model for PCI device passthrough. For such use-cases make the very
>>> first step in splitting the xen-pciback driver into two parts: Xen
>>> PCI stub and PCI PV backend drivers.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
>>>
>>> ---
>>> Changes since v3:
>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>> I'm afraid this wasn't fully done:
>>
>>> --- 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
>> While benign when added here, this addition still doesn't seem to
>> belong here.
>
> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>
> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>
> architectures...
>
> What would be the preference here? Stefano suggested that we still define
>
> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>
> in the second patch
>
> Another option is just to squash the two patches.
Squashing would be fine for me.
Juergen
On Mon, 27 Sep 2021, Juergen Gross wrote:
> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
> >
> > On 27.09.21 10:26, Jan Beulich wrote:
> > > On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
> > > > 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-virtualised use-cases support
> > > >
> > > > The para-virtualised part of the driver is not always needed as some
> > > > architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
> > > > model for PCI device passthrough. For such use-cases make the very
> > > > first step in splitting the xen-pciback driver into two parts: Xen
> > > > PCI stub and PCI PV backend drivers.
> > > >
> > > > Signed-off-by: Oleksandr Andrushchenko
> > > > <[email protected]>
> > > >
> > > > ---
> > > > Changes since v3:
> > > > - Move CONFIG_XEN_PCIDEV_STUB to the second patch
> > > I'm afraid this wasn't fully done:
> > >
> > > > --- 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
> > > While benign when added here, this addition still doesn't seem to
> > > belong here.
> >
> > My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
> >
> > to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
> >
> > architectures...
> >
> > What would be the preference here? Stefano suggested that we still define
> >
> > CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
> >
> > in the second patch
> >
> > Another option is just to squash the two patches.
>
> Squashing would be fine for me.
It is fine for me to squash the two patches.
But in any case, wouldn't it be better to modify that specific change to:
diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
index e2cb376444a6..e23c758b85ae 100644
--- a/drivers/xen/xen-pciback/Makefile
+++ b/drivers/xen/xen-pciback/Makefile
@@ -1,6 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
-obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
+obj-$(CONFIG_XEN_PCI_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 \
instead?
On 28.09.21 07:18, Stefano Stabellini wrote:
> On Mon, 27 Sep 2021, Juergen Gross wrote:
>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>>> On 27.09.21 10:26, Jan Beulich wrote:
>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>>>> 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-virtualised use-cases support
>>>>>
>>>>> The para-virtualised part of the driver is not always needed as some
>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>>>> model for PCI device passthrough. For such use-cases make the very
>>>>> first step in splitting the xen-pciback driver into two parts: Xen
>>>>> PCI stub and PCI PV backend drivers.
>>>>>
>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>> <[email protected]>
>>>>>
>>>>> ---
>>>>> Changes since v3:
>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>>>> I'm afraid this wasn't fully done:
>>>>
>>>>> --- 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
>>>> While benign when added here, this addition still doesn't seem to
>>>> belong here.
>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>>>
>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>>>
>>> architectures...
>>>
>>> What would be the preference here? Stefano suggested that we still define
>>>
>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>>>
>>> in the second patch
>>>
>>> Another option is just to squash the two patches.
>> Squashing would be fine for me.
>
> It is fine for me to squash the two patches.
>
> But in any case, wouldn't it be better to modify that specific change to:
>
> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
> index e2cb376444a6..e23c758b85ae 100644
> --- a/drivers/xen/xen-pciback/Makefile
> +++ b/drivers/xen/xen-pciback/Makefile
> @@ -1,6 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
> +obj-$(CONFIG_XEN_PCI_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 \
>
>
> instead?
Yes, looks simpler then. I'll squash and add this change
On 28.09.2021 06:18, Stefano Stabellini wrote:
> On Mon, 27 Sep 2021, Juergen Gross wrote:
>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>>>
>>> On 27.09.21 10:26, Jan Beulich wrote:
>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>>>> 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-virtualised use-cases support
>>>>>
>>>>> The para-virtualised part of the driver is not always needed as some
>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>>>> model for PCI device passthrough. For such use-cases make the very
>>>>> first step in splitting the xen-pciback driver into two parts: Xen
>>>>> PCI stub and PCI PV backend drivers.
>>>>>
>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>> <[email protected]>
>>>>>
>>>>> ---
>>>>> Changes since v3:
>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>>>> I'm afraid this wasn't fully done:
>>>>
>>>>> --- 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
>>>> While benign when added here, this addition still doesn't seem to
>>>> belong here.
>>>
>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>>>
>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>>>
>>> architectures...
>>>
>>> What would be the preference here? Stefano suggested that we still define
>>>
>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>>>
>>> in the second patch
>>>
>>> Another option is just to squash the two patches.
>>
>> Squashing would be fine for me.
>
> It is fine for me to squash the two patches.
>
> But in any case, wouldn't it be better to modify that specific change to:
>
> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
> index e2cb376444a6..e23c758b85ae 100644
> --- a/drivers/xen/xen-pciback/Makefile
> +++ b/drivers/xen/xen-pciback/Makefile
> @@ -1,6 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback.o
But that wouldn't allow the driver to be a module anymore, would it?
Jan
On 28.09.21 09:42, Jan Beulich wrote:
> On 28.09.2021 06:18, Stefano Stabellini wrote:
>> On Mon, 27 Sep 2021, Juergen Gross wrote:
>>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>>>> On 27.09.21 10:26, Jan Beulich wrote:
>>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>>>>> 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-virtualised use-cases support
>>>>>>
>>>>>> The para-virtualised part of the driver is not always needed as some
>>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>>>>> model for PCI device passthrough. For such use-cases make the very
>>>>>> first step in splitting the xen-pciback driver into two parts: Xen
>>>>>> PCI stub and PCI PV backend drivers.
>>>>>>
>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>> <[email protected]>
>>>>>>
>>>>>> ---
>>>>>> Changes since v3:
>>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>>>>> I'm afraid this wasn't fully done:
>>>>>
>>>>>> --- 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
>>>>> While benign when added here, this addition still doesn't seem to
>>>>> belong here.
>>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>>>>
>>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>>>>
>>>> architectures...
>>>>
>>>> What would be the preference here? Stefano suggested that we still define
>>>>
>>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>>>>
>>>> in the second patch
>>>>
>>>> Another option is just to squash the two patches.
>>> Squashing would be fine for me.
>>
>> It is fine for me to squash the two patches.
>>
>> But in any case, wouldn't it be better to modify that specific change to:
>>
>> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
>> index e2cb376444a6..e23c758b85ae 100644
>> --- a/drivers/xen/xen-pciback/Makefile
>> +++ b/drivers/xen/xen-pciback/Makefile
>> @@ -1,6 +1,5 @@
>> # SPDX-License-Identifier: GPL-2.0
>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback.o
> But that wouldn't allow the driver to be a module anymore, would it?
Exactly. I forgot that when playing with module/built-in I was not able
to control that anymore because CONFIG_XEN_PCI_STUB will always be
in "y" state, thus even if you have CONFIG_XEN_PCIDEV_BACKEND=m
you won't be able to build it as module. So, I will probably put a comment
about that in the Makefile explaining the need for
obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>
> Jan
>
Thank you,
Oleksandr
On 28.09.21 08:56, Oleksandr Andrushchenko wrote:
>
> On 28.09.21 09:42, Jan Beulich wrote:
>> On 28.09.2021 06:18, Stefano Stabellini wrote:
>>> On Mon, 27 Sep 2021, Juergen Gross wrote:
>>>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>>>>> On 27.09.21 10:26, Jan Beulich wrote:
>>>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>>>>>> 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-virtualised use-cases support
>>>>>>>
>>>>>>> The para-virtualised part of the driver is not always needed as some
>>>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>>>>>> model for PCI device passthrough. For such use-cases make the very
>>>>>>> first step in splitting the xen-pciback driver into two parts: Xen
>>>>>>> PCI stub and PCI PV backend drivers.
>>>>>>>
>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>> <[email protected]>
>>>>>>>
>>>>>>> ---
>>>>>>> Changes since v3:
>>>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>>>>>> I'm afraid this wasn't fully done:
>>>>>>
>>>>>>> --- 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
>>>>>> While benign when added here, this addition still doesn't seem to
>>>>>> belong here.
>>>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>>>>>
>>>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>>>>>
>>>>> architectures...
>>>>>
>>>>> What would be the preference here? Stefano suggested that we still define
>>>>>
>>>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>>>>>
>>>>> in the second patch
>>>>>
>>>>> Another option is just to squash the two patches.
>>>> Squashing would be fine for me.
>>>
>>> It is fine for me to squash the two patches.
>>>
>>> But in any case, wouldn't it be better to modify that specific change to:
>>>
>>> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
>>> index e2cb376444a6..e23c758b85ae 100644
>>> --- a/drivers/xen/xen-pciback/Makefile
>>> +++ b/drivers/xen/xen-pciback/Makefile
>>> @@ -1,6 +1,5 @@
>>> # SPDX-License-Identifier: GPL-2.0
>>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback.o
>> But that wouldn't allow the driver to be a module anymore, would it?
>
> Exactly. I forgot that when playing with module/built-in I was not able
>
> to control that anymore because CONFIG_XEN_PCI_STUB will always be
>
> in "y" state, thus even if you have CONFIG_XEN_PCIDEV_BACKEND=m
>
> you won't be able to build it as module. So, I will probably put a comment
>
> about that in the Makefile explaining the need for
>
> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
In case the real split between both parts of xen-pciback is done this
will be needed anyway.
Juergen
On 28.09.21 09:59, Juergen Gross wrote:
> On 28.09.21 08:56, Oleksandr Andrushchenko wrote:
>>
>> On 28.09.21 09:42, Jan Beulich wrote:
>>> On 28.09.2021 06:18, Stefano Stabellini wrote:
>>>> On Mon, 27 Sep 2021, Juergen Gross wrote:
>>>>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>>>>>> On 27.09.21 10:26, Jan Beulich wrote:
>>>>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>>>>>>> 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-virtualised use-cases support
>>>>>>>>
>>>>>>>> The para-virtualised part of the driver is not always needed as some
>>>>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>>>>>>> model for PCI device passthrough. For such use-cases make the very
>>>>>>>> first step in splitting the xen-pciback driver into two parts: Xen
>>>>>>>> PCI stub and PCI PV backend drivers.
>>>>>>>>
>>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>>> <[email protected]>
>>>>>>>>
>>>>>>>> ---
>>>>>>>> Changes since v3:
>>>>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>>>>>>> I'm afraid this wasn't fully done:
>>>>>>>
>>>>>>>> --- 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
>>>>>>> While benign when added here, this addition still doesn't seem to
>>>>>>> belong here.
>>>>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>>>>>>
>>>>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>>>>>>
>>>>>> architectures...
>>>>>>
>>>>>> What would be the preference here? Stefano suggested that we still define
>>>>>>
>>>>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>>>>>>
>>>>>> in the second patch
>>>>>>
>>>>>> Another option is just to squash the two patches.
>>>>> Squashing would be fine for me.
>>>> It is fine for me to squash the two patches.
>>>>
>>>> But in any case, wouldn't it be better to modify that specific change to:
>>>>
>>>> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
>>>> index e2cb376444a6..e23c758b85ae 100644
>>>> --- a/drivers/xen/xen-pciback/Makefile
>>>> +++ b/drivers/xen/xen-pciback/Makefile
>>>> @@ -1,6 +1,5 @@
>>>> # SPDX-License-Identifier: GPL-2.0
>>>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>>> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>>> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback.o
>>> But that wouldn't allow the driver to be a module anymore, would it?
>>
>> Exactly. I forgot that when playing with module/built-in I was not able
>>
>> to control that anymore because CONFIG_XEN_PCI_STUB will always be
>>
>> in "y" state, thus even if you have CONFIG_XEN_PCIDEV_BACKEND=m
>>
>> you won't be able to build it as module. So, I will probably put a comment
>>
>> about that in the Makefile explaining the need for
>>
>> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>
> In case the real split between both parts of xen-pciback is done this
> will be needed anyway.
Yes, it will
So, I'll put a comment in the Makefile:
# N.B. This cannot be expressed with a single line using CONFIG_XEN_PCI_STUB
# as it always remains in "y" state, thus preventing the driver to be built as
# a module.
obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
Will this be ok or needs some re-wording?
>
>
> Juergen
>
Thank you,
Oleksandr
On 28.09.21 09:17, Oleksandr Andrushchenko wrote:
>
> On 28.09.21 09:59, Juergen Gross wrote:
>> On 28.09.21 08:56, Oleksandr Andrushchenko wrote:
>>>
>>> On 28.09.21 09:42, Jan Beulich wrote:
>>>> On 28.09.2021 06:18, Stefano Stabellini wrote:
>>>>> On Mon, 27 Sep 2021, Juergen Gross wrote:
>>>>>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>>>>>>> On 27.09.21 10:26, Jan Beulich wrote:
>>>>>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>>>>>>>> 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-virtualised use-cases support
>>>>>>>>>
>>>>>>>>> The para-virtualised part of the driver is not always needed as some
>>>>>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>>>>>>>> model for PCI device passthrough. For such use-cases make the very
>>>>>>>>> first step in splitting the xen-pciback driver into two parts: Xen
>>>>>>>>> PCI stub and PCI PV backend drivers.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>>>> <[email protected]>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> Changes since v3:
>>>>>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>>>>>>>> I'm afraid this wasn't fully done:
>>>>>>>>
>>>>>>>>> --- 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
>>>>>>>> While benign when added here, this addition still doesn't seem to
>>>>>>>> belong here.
>>>>>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>>>>>>>
>>>>>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>>>>>>>
>>>>>>> architectures...
>>>>>>>
>>>>>>> What would be the preference here? Stefano suggested that we still define
>>>>>>>
>>>>>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>>>>>>>
>>>>>>> in the second patch
>>>>>>>
>>>>>>> Another option is just to squash the two patches.
>>>>>> Squashing would be fine for me.
>>>>> It is fine for me to squash the two patches.
>>>>>
>>>>> But in any case, wouldn't it be better to modify that specific change to:
>>>>>
>>>>> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
>>>>> index e2cb376444a6..e23c758b85ae 100644
>>>>> --- a/drivers/xen/xen-pciback/Makefile
>>>>> +++ b/drivers/xen/xen-pciback/Makefile
>>>>> @@ -1,6 +1,5 @@
>>>>> # SPDX-License-Identifier: GPL-2.0
>>>>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>>>> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>>>> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback.o
>>>> But that wouldn't allow the driver to be a module anymore, would it?
>>>
>>> Exactly. I forgot that when playing with module/built-in I was not able
>>>
>>> to control that anymore because CONFIG_XEN_PCI_STUB will always be
>>>
>>> in "y" state, thus even if you have CONFIG_XEN_PCIDEV_BACKEND=m
>>>
>>> you won't be able to build it as module. So, I will probably put a comment
>>>
>>> about that in the Makefile explaining the need for
>>>
>>> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>
>> In case the real split between both parts of xen-pciback is done this
>> will be needed anyway.
>
> Yes, it will
>
> So, I'll put a comment in the Makefile:
>
> # N.B. This cannot be expressed with a single line using CONFIG_XEN_PCI_STUB
>
> # as it always remains in "y" state, thus preventing the driver to be built as
>
> # a module.
>
> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>
> Will this be ok or needs some re-wording?
I'd add that CONFIG_XEN_PCIDEV_BACKEND and CONFIG_XEN_PCIDEV_STUB are
mutually exclusive.
Juergen
On Tue, 28 Sep 2021, Oleksandr Andrushchenko wrote:
> On 28.09.21 09:59, Juergen Gross wrote:
> > On 28.09.21 08:56, Oleksandr Andrushchenko wrote:
> >>
> >> On 28.09.21 09:42, Jan Beulich wrote:
> >>> On 28.09.2021 06:18, Stefano Stabellini wrote:
> >>>> On Mon, 27 Sep 2021, Juergen Gross wrote:
> >>>>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
> >>>>>> On 27.09.21 10:26, Jan Beulich wrote:
> >>>>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
> >>>>>>>> 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-virtualised use-cases support
> >>>>>>>>
> >>>>>>>> The para-virtualised part of the driver is not always needed as some
> >>>>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
> >>>>>>>> model for PCI device passthrough. For such use-cases make the very
> >>>>>>>> first step in splitting the xen-pciback driver into two parts: Xen
> >>>>>>>> PCI stub and PCI PV backend drivers.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Oleksandr Andrushchenko
> >>>>>>>> <[email protected]>
> >>>>>>>>
> >>>>>>>> ---
> >>>>>>>> Changes since v3:
> >>>>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
> >>>>>>> I'm afraid this wasn't fully done:
> >>>>>>>
> >>>>>>>> --- 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
> >>>>>>> While benign when added here, this addition still doesn't seem to
> >>>>>>> belong here.
> >>>>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
> >>>>>>
> >>>>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
> >>>>>>
> >>>>>> architectures...
> >>>>>>
> >>>>>> What would be the preference here? Stefano suggested that we still define
> >>>>>>
> >>>>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
> >>>>>>
> >>>>>> in the second patch
> >>>>>>
> >>>>>> Another option is just to squash the two patches.
> >>>>> Squashing would be fine for me.
> >>>> It is fine for me to squash the two patches.
> >>>>
> >>>> But in any case, wouldn't it be better to modify that specific change to:
> >>>>
> >>>> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
> >>>> index e2cb376444a6..e23c758b85ae 100644
> >>>> --- a/drivers/xen/xen-pciback/Makefile
> >>>> +++ b/drivers/xen/xen-pciback/Makefile
> >>>> @@ -1,6 +1,5 @@
> >>>> # SPDX-License-Identifier: GPL-2.0
> >>>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
> >>>> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
> >>>> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback.o
> >>> But that wouldn't allow the driver to be a module anymore, would it?
> >>
> >> Exactly. I forgot that when playing with module/built-in I was not able
> >>
> >> to control that anymore because CONFIG_XEN_PCI_STUB will always be
> >>
> >> in "y" state, thus even if you have CONFIG_XEN_PCIDEV_BACKEND=m
> >>
> >> you won't be able to build it as module. So, I will probably put a comment
> >>
> >> about that in the Makefile explaining the need for
> >>
> >> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
> >> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
> >
> > In case the real split between both parts of xen-pciback is done this
> > will be needed anyway.
>
> Yes, it will
>
> So, I'll put a comment in the Makefile:
>
> # N.B. This cannot be expressed with a single line using CONFIG_XEN_PCI_STUB
>
> # as it always remains in "y" state, thus preventing the driver to be built as
>
> # a module.
>
> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>
> Will this be ok or needs some re-wording?
I am fine with it and honestly that was the only comment I had so you
can add my
Reviewed-by: Stefano Stabellini <[email protected]>
On 28.09.21 10:20, Juergen Gross wrote:
> On 28.09.21 09:17, Oleksandr Andrushchenko wrote:
>>
>> On 28.09.21 09:59, Juergen Gross wrote:
>>> On 28.09.21 08:56, Oleksandr Andrushchenko wrote:
>>>>
>>>> On 28.09.21 09:42, Jan Beulich wrote:
>>>>> On 28.09.2021 06:18, Stefano Stabellini wrote:
>>>>>> On Mon, 27 Sep 2021, Juergen Gross wrote:
>>>>>>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>>>>>>>> On 27.09.21 10:26, Jan Beulich wrote:
>>>>>>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>>>>>>>>> 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-virtualised use-cases support
>>>>>>>>>>
>>>>>>>>>> The para-virtualised part of the driver is not always needed as some
>>>>>>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>>>>>>>>> model for PCI device passthrough. For such use-cases make the very
>>>>>>>>>> first step in splitting the xen-pciback driver into two parts: Xen
>>>>>>>>>> PCI stub and PCI PV backend drivers.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>>>>> <[email protected]>
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> Changes since v3:
>>>>>>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>>>>>>>>> I'm afraid this wasn't fully done:
>>>>>>>>>
>>>>>>>>>> --- 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
>>>>>>>>> While benign when added here, this addition still doesn't seem to
>>>>>>>>> belong here.
>>>>>>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>>>>>>>>
>>>>>>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>>>>>>>>
>>>>>>>> architectures...
>>>>>>>>
>>>>>>>> What would be the preference here? Stefano suggested that we still define
>>>>>>>>
>>>>>>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>>>>>>>>
>>>>>>>> in the second patch
>>>>>>>>
>>>>>>>> Another option is just to squash the two patches.
>>>>>>> Squashing would be fine for me.
>>>>>> It is fine for me to squash the two patches.
>>>>>>
>>>>>> But in any case, wouldn't it be better to modify that specific change to:
>>>>>>
>>>>>> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
>>>>>> index e2cb376444a6..e23c758b85ae 100644
>>>>>> --- a/drivers/xen/xen-pciback/Makefile
>>>>>> +++ b/drivers/xen/xen-pciback/Makefile
>>>>>> @@ -1,6 +1,5 @@
>>>>>> # SPDX-License-Identifier: GPL-2.0
>>>>>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>>>>> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>>>>> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback.o
>>>>> But that wouldn't allow the driver to be a module anymore, would it?
>>>>
>>>> Exactly. I forgot that when playing with module/built-in I was not able
>>>>
>>>> to control that anymore because CONFIG_XEN_PCI_STUB will always be
>>>>
>>>> in "y" state, thus even if you have CONFIG_XEN_PCIDEV_BACKEND=m
>>>>
>>>> you won't be able to build it as module. So, I will probably put a comment
>>>>
>>>> about that in the Makefile explaining the need for
>>>>
>>>> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>>> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>>
>>> In case the real split between both parts of xen-pciback is done this
>>> will be needed anyway.
>>
>> Yes, it will
>>
>> So, I'll put a comment in the Makefile:
>>
>> # N.B. This cannot be expressed with a single line using CONFIG_XEN_PCI_STUB
>>
>> # as it always remains in "y" state, thus preventing the driver to be built as
>>
>> # a module.
>>
>> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>
>> Will this be ok or needs some re-wording?
>
> I'd add that CONFIG_XEN_PCIDEV_BACKEND and CONFIG_XEN_PCIDEV_STUB are
> mutually exclusive.
# N.B. The below cannot be expressed with a single line using
# CONFIG_XEN_PCI_STUB as it always remains in "y" state,
# thus preventing the driver to be built as a module.
# Please note, that CONFIG_XEN_PCIDEV_BACKEND and
# CONFIG_XEN_PCIDEV_STUB are mutually exclusive.
obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>
>
> Juergen
Thank you,
Oleksandr
On 28.09.21 09:24, Oleksandr Andrushchenko wrote:
>
> On 28.09.21 10:20, Juergen Gross wrote:
>> On 28.09.21 09:17, Oleksandr Andrushchenko wrote:
>>>
>>> On 28.09.21 09:59, Juergen Gross wrote:
>>>> On 28.09.21 08:56, Oleksandr Andrushchenko wrote:
>>>>>
>>>>> On 28.09.21 09:42, Jan Beulich wrote:
>>>>>> On 28.09.2021 06:18, Stefano Stabellini wrote:
>>>>>>> On Mon, 27 Sep 2021, Juergen Gross wrote:
>>>>>>>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 27.09.21 10:26, Jan Beulich wrote:
>>>>>>>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>>>>>>>>>> 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-virtualised use-cases support
>>>>>>>>>>>
>>>>>>>>>>> The para-virtualised part of the driver is not always needed as some
>>>>>>>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>>>>>>>>>> model for PCI device passthrough. For such use-cases make the very
>>>>>>>>>>> first step in splitting the xen-pciback driver into two parts: Xen
>>>>>>>>>>> PCI stub and PCI PV backend drivers.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>>>>>> <[email protected]>
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>> Changes since v3:
>>>>>>>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>>>>>>>>>> I'm afraid this wasn't fully done:
>>>>>>>>>>
>>>>>>>>>>> --- 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
>>>>>>>>>> While benign when added here, this addition still doesn't seem to
>>>>>>>>>> belong here.
>>>>>>>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>>>>>>>>>
>>>>>>>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>>>>>>>>>
>>>>>>>>> architectures...
>>>>>>>>>
>>>>>>>>> What would be the preference here? Stefano suggested that we still define
>>>>>>>>>
>>>>>>>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>>>>>>>>>
>>>>>>>>> in the second patch
>>>>>>>>>
>>>>>>>>> Another option is just to squash the two patches.
>>>>>>>> Squashing would be fine for me.
>>>>>>> It is fine for me to squash the two patches.
>>>>>>>
>>>>>>> But in any case, wouldn't it be better to modify that specific change to:
>>>>>>>
>>>>>>> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
>>>>>>> index e2cb376444a6..e23c758b85ae 100644
>>>>>>> --- a/drivers/xen/xen-pciback/Makefile
>>>>>>> +++ b/drivers/xen/xen-pciback/Makefile
>>>>>>> @@ -1,6 +1,5 @@
>>>>>>> # SPDX-License-Identifier: GPL-2.0
>>>>>>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>>>>>> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>>>>>> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback.o
>>>>>> But that wouldn't allow the driver to be a module anymore, would it?
>>>>>
>>>>> Exactly. I forgot that when playing with module/built-in I was not able
>>>>>
>>>>> to control that anymore because CONFIG_XEN_PCI_STUB will always be
>>>>>
>>>>> in "y" state, thus even if you have CONFIG_XEN_PCIDEV_BACKEND=m
>>>>>
>>>>> you won't be able to build it as module. So, I will probably put a comment
>>>>>
>>>>> about that in the Makefile explaining the need for
>>>>>
>>>>> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>>>> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>>>
>>>> In case the real split between both parts of xen-pciback is done this
>>>> will be needed anyway.
>>>
>>> Yes, it will
>>>
>>> So, I'll put a comment in the Makefile:
>>>
>>> # N.B. This cannot be expressed with a single line using CONFIG_XEN_PCI_STUB
>>>
>>> # as it always remains in "y" state, thus preventing the driver to be built as
>>>
>>> # a module.
>>>
>>> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>>
>>> Will this be ok or needs some re-wording?
>>
>> I'd add that CONFIG_XEN_PCIDEV_BACKEND and CONFIG_XEN_PCIDEV_STUB are
>> mutually exclusive.
> # N.B. The below cannot be expressed with a single line using
> # CONFIG_XEN_PCI_STUB as it always remains in "y" state,
> # thus preventing the driver to be built as a module.
> # Please note, that CONFIG_XEN_PCIDEV_BACKEND and
> # CONFIG_XEN_PCIDEV_STUB are mutually exclusive.
> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
Yes, that's fine.
Juergen
On 28.09.21 10:20, Stefano Stabellini wrote:
> On Tue, 28 Sep 2021, Oleksandr Andrushchenko wrote:
>> On 28.09.21 09:59, Juergen Gross wrote:
>>> On 28.09.21 08:56, Oleksandr Andrushchenko wrote:
>>>> On 28.09.21 09:42, Jan Beulich wrote:
>>>>> On 28.09.2021 06:18, Stefano Stabellini wrote:
>>>>>> On Mon, 27 Sep 2021, Juergen Gross wrote:
>>>>>>> On 27.09.21 09:35, Oleksandr Andrushchenko wrote:
>>>>>>>> On 27.09.21 10:26, Jan Beulich wrote:
>>>>>>>>> On 27.09.2021 08:58, Oleksandr Andrushchenko wrote:
>>>>>>>>>> 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-virtualised use-cases support
>>>>>>>>>>
>>>>>>>>>> The para-virtualised part of the driver is not always needed as some
>>>>>>>>>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>>>>>>>>>> model for PCI device passthrough. For such use-cases make the very
>>>>>>>>>> first step in splitting the xen-pciback driver into two parts: Xen
>>>>>>>>>> PCI stub and PCI PV backend drivers.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>>>>> <[email protected]>
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> Changes since v3:
>>>>>>>>>> - Move CONFIG_XEN_PCIDEV_STUB to the second patch
>>>>>>>>> I'm afraid this wasn't fully done:
>>>>>>>>>
>>>>>>>>>> --- 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
>>>>>>>>> While benign when added here, this addition still doesn't seem to
>>>>>>>>> belong here.
>>>>>>>> My bad. So, it seems without CONFIG_XEN_PCIDEV_STUB the change seems
>>>>>>>>
>>>>>>>> to be non-functional. With CONFIG_XEN_PCIDEV_STUB we fail to build on 32-bit
>>>>>>>>
>>>>>>>> architectures...
>>>>>>>>
>>>>>>>> What would be the preference here? Stefano suggested that we still define
>>>>>>>>
>>>>>>>> CONFIG_XEN_PCIDEV_STUB, but in disabled state, e.g. we add tristate to it
>>>>>>>>
>>>>>>>> in the second patch
>>>>>>>>
>>>>>>>> Another option is just to squash the two patches.
>>>>>>> Squashing would be fine for me.
>>>>>> It is fine for me to squash the two patches.
>>>>>>
>>>>>> But in any case, wouldn't it be better to modify that specific change to:
>>>>>>
>>>>>> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
>>>>>> index e2cb376444a6..e23c758b85ae 100644
>>>>>> --- a/drivers/xen/xen-pciback/Makefile
>>>>>> +++ b/drivers/xen/xen-pciback/Makefile
>>>>>> @@ -1,6 +1,5 @@
>>>>>> # SPDX-License-Identifier: GPL-2.0
>>>>>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>>>>> -obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>>>>> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback.o
>>>>> But that wouldn't allow the driver to be a module anymore, would it?
>>>> Exactly. I forgot that when playing with module/built-in I was not able
>>>>
>>>> to control that anymore because CONFIG_XEN_PCI_STUB will always be
>>>>
>>>> in "y" state, thus even if you have CONFIG_XEN_PCIDEV_BACKEND=m
>>>>
>>>> you won't be able to build it as module. So, I will probably put a comment
>>>>
>>>> about that in the Makefile explaining the need for
>>>>
>>>> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>>>> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>> In case the real split between both parts of xen-pciback is done this
>>> will be needed anyway.
>> Yes, it will
>>
>> So, I'll put a comment in the Makefile:
>>
>> # N.B. This cannot be expressed with a single line using CONFIG_XEN_PCI_STUB
>>
>> # as it always remains in "y" state, thus preventing the driver to be built as
>>
>> # a module.
>>
>> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>> obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>
>> Will this be ok or needs some re-wording?
> I am fine with it and honestly that was the only comment I had so you
> can add my
>
> Reviewed-by: Stefano Stabellini <[email protected]>
Ok, thank you