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

2021-09-17 21:07:10

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

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

---
Tested on Arm and x86.
---
arch/x86/include/asm/xen/pci.h | 24 ----------
arch/x86/pci/xen.c | 74 +----------------------------
drivers/xen/Kconfig | 2 +-
drivers/xen/events/events_base.c | 1 +
drivers/xen/pci.c | 75 ++++++++++++++++++++++++++++++
drivers/xen/xen-pciback/pci_stub.c | 3 +-
drivers/xen/xen-pciback/xenbus.c | 2 +-
include/xen/pci.h | 34 ++++++++++++++
8 files changed, 115 insertions(+), 100 deletions(-)
create mode 100644 include/xen/pci.h

diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
index 3506d8c598c1..9ff7b49bca08 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -14,30 +14,6 @@ static inline int pci_xen_hvm_init(void)
return -1;
}
#endif
-#if defined(CONFIG_XEN_DOM0)
-int __init pci_xen_initial_domain(void);
-int xen_find_device_domain_owner(struct pci_dev *dev);
-int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
-int xen_unregister_device_domain_owner(struct pci_dev *dev);
-#else
-static inline int __init pci_xen_initial_domain(void)
-{
- return -1;
-}
-static inline int xen_find_device_domain_owner(struct pci_dev *dev)
-{
- return -1;
-}
-static inline int xen_register_device_domain_owner(struct pci_dev *dev,
- uint16_t domain)
-{
- return -1;
-}
-static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
-{
- return -1;
-}
-#endif

#if defined(CONFIG_PCI_MSI)
#if defined(CONFIG_PCI_XEN)
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 3d41a09c2c14..4a45b0bf9ae4 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -23,6 +23,7 @@

#include <xen/features.h>
#include <xen/events.h>
+#include <xen/pci.h>
#include <asm/xen/pci.h>
#include <asm/xen/cpuid.h>
#include <asm/apic.h>
@@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
}
return 0;
}
-
-struct xen_device_domain_owner {
- domid_t domain;
- struct pci_dev *dev;
- struct list_head list;
-};
-
-static DEFINE_SPINLOCK(dev_domain_list_spinlock);
-static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
-
-static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
-{
- struct xen_device_domain_owner *owner;
-
- list_for_each_entry(owner, &dev_domain_list, list) {
- if (owner->dev == dev)
- return owner;
- }
- return NULL;
-}
-
-int xen_find_device_domain_owner(struct pci_dev *dev)
-{
- struct xen_device_domain_owner *owner;
- int domain = -ENODEV;
-
- spin_lock(&dev_domain_list_spinlock);
- owner = find_device(dev);
- if (owner)
- domain = owner->domain;
- spin_unlock(&dev_domain_list_spinlock);
- return domain;
-}
-EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
-
-int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
-{
- struct xen_device_domain_owner *owner;
-
- owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
- if (!owner)
- return -ENODEV;
-
- spin_lock(&dev_domain_list_spinlock);
- if (find_device(dev)) {
- spin_unlock(&dev_domain_list_spinlock);
- kfree(owner);
- return -EEXIST;
- }
- owner->domain = domain;
- owner->dev = dev;
- list_add_tail(&owner->list, &dev_domain_list);
- spin_unlock(&dev_domain_list_spinlock);
- return 0;
-}
-EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
-
-int xen_unregister_device_domain_owner(struct pci_dev *dev)
-{
- struct xen_device_domain_owner *owner;
-
- spin_lock(&dev_domain_list_spinlock);
- owner = find_device(dev);
- if (!owner) {
- spin_unlock(&dev_domain_list_spinlock);
- return -ENODEV;
- }
- list_del(&owner->list);
- spin_unlock(&dev_domain_list_spinlock);
- kfree(owner);
- return 0;
-}
-EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
#endif
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index a37eb52fb401..057ddf61ef61 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -182,7 +182,7 @@ config SWIOTLB_XEN

config XEN_PCIDEV_BACKEND
tristate "Xen PCI-device backend driver"
- depends on PCI && X86 && XEN
+ depends on PCI && XEN
depends on XEN_BACKEND
default m
help
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index a78704ae3618..35493ff0d146 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -65,6 +65,7 @@
#include <xen/interface/vcpu.h>
#include <xen/xenbus.h>
#include <asm/hw_irq.h>
+#include <xen/pci.h>

#include "events_internal.h"

diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 224df03ce42e..fc8c1249d49f 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -254,3 +254,78 @@ static int xen_mcfg_late(void)
return 0;
}
#endif
+
+#ifdef CONFIG_XEN_DOM0
+struct xen_device_domain_owner {
+ domid_t domain;
+ struct pci_dev *dev;
+ struct list_head list;
+};
+
+static DEFINE_SPINLOCK(dev_domain_list_spinlock);
+static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
+
+static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
+{
+ struct xen_device_domain_owner *owner;
+
+ list_for_each_entry(owner, &dev_domain_list, list) {
+ if (owner->dev == dev)
+ return owner;
+ }
+ return NULL;
+}
+
+int xen_find_device_domain_owner(struct pci_dev *dev)
+{
+ struct xen_device_domain_owner *owner;
+ int domain = -ENODEV;
+
+ spin_lock(&dev_domain_list_spinlock);
+ owner = find_device(dev);
+ if (owner)
+ domain = owner->domain;
+ spin_unlock(&dev_domain_list_spinlock);
+ return domain;
+}
+EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
+
+int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
+{
+ struct xen_device_domain_owner *owner;
+
+ owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
+ if (!owner)
+ return -ENODEV;
+
+ spin_lock(&dev_domain_list_spinlock);
+ if (find_device(dev)) {
+ spin_unlock(&dev_domain_list_spinlock);
+ kfree(owner);
+ return -EEXIST;
+ }
+ owner->domain = domain;
+ owner->dev = dev;
+ list_add_tail(&owner->list, &dev_domain_list);
+ spin_unlock(&dev_domain_list_spinlock);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
+
+int xen_unregister_device_domain_owner(struct pci_dev *dev)
+{
+ struct xen_device_domain_owner *owner;
+
+ spin_lock(&dev_domain_list_spinlock);
+ owner = find_device(dev);
+ if (!owner) {
+ spin_unlock(&dev_domain_list_spinlock);
+ return -ENODEV;
+ }
+ list_del(&owner->list);
+ spin_unlock(&dev_domain_list_spinlock);
+ kfree(owner);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
+#endif
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index f8e4faa96ad6..bba527620507 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -19,7 +19,8 @@
#include <linux/sched.h>
#include <linux/atomic.h>
#include <xen/events.h>
-#include <asm/xen/pci.h>
+#include <xen/pci.h>
+#include <xen/xen.h>
#include <asm/xen/hypervisor.h>
#include <xen/interface/physdev.h>
#include "pciback.h"
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index c09c7ebd6968..da34ce85dc88 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -14,7 +14,7 @@
#include <linux/workqueue.h>
#include <xen/xenbus.h>
#include <xen/events.h>
-#include <asm/xen/pci.h>
+#include <xen/pci.h>
#include "pciback.h"

#define INVALID_EVTCHN_IRQ (-1)
diff --git a/include/xen/pci.h b/include/xen/pci.h
new file mode 100644
index 000000000000..5c988d5ff38f
--- /dev/null
+++ b/include/xen/pci.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __XEN_PCI_H__
+#define __XEN_PCI_H__
+
+#if defined(CONFIG_XEN_DOM0)
+int __init pci_xen_initial_domain(void);
+int xen_find_device_domain_owner(struct pci_dev *dev);
+int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
+int xen_unregister_device_domain_owner(struct pci_dev *dev);
+#else
+static inline int __init pci_xen_initial_domain(void)
+{
+ return -1;
+}
+
+static inline int xen_find_device_domain_owner(struct pci_dev *dev)
+{
+ return -1;
+}
+
+static inline int xen_register_device_domain_owner(struct pci_dev *dev,
+ uint16_t domain)
+{
+ return -1;
+}
+
+static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
+{
+ return -1;
+}
+#endif
+
+#endif
--
2.25.1

2021-09-18 09:26:32

by Stefano Stabellini

[permalink] [raw]

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

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

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

> -int xen_find_device_domain_owner(struct pci_dev *dev);
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
> -int xen_unregister_device_domain_owner(struct pci_dev *dev);
> -#else
> -static inline int __init pci_xen_initial_domain(void)
> -{
> - return -1;
> -}
> -static inline int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> - return -1;
> -}
> -static inline int xen_register_device_domain_owner(struct pci_dev *dev,
> - uint16_t domain)
> -{
> - return -1;
> -}
> -static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> - return -1;
> -}
> -#endif
>
> #if defined(CONFIG_PCI_MSI)
> #if defined(CONFIG_PCI_XEN)
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 3d41a09c2c14..4a45b0bf9ae4 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -23,6 +23,7 @@
>
> #include <xen/features.h>
> #include <xen/events.h>
> +#include <xen/pci.h>
> #include <asm/xen/pci.h>
> #include <asm/xen/cpuid.h>
> #include <asm/apic.h>
> @@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
> }
> return 0;
> }
> -
> -struct xen_device_domain_owner {
> - domid_t domain;
> - struct pci_dev *dev;
> - struct list_head list;
> -};
> -
> -static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> -static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> -
> -static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
> -{
> - struct xen_device_domain_owner *owner;
> -
> - list_for_each_entry(owner, &dev_domain_list, list) {
> - if (owner->dev == dev)
> - return owner;
> - }
> - return NULL;
> -}
> -
> -int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> - struct xen_device_domain_owner *owner;
> - int domain = -ENODEV;
> -
> - spin_lock(&dev_domain_list_spinlock);
> - owner = find_device(dev);
> - if (owner)
> - domain = owner->domain;
> - spin_unlock(&dev_domain_list_spinlock);
> - return domain;
> -}
> -EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
> -
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
> -{
> - struct xen_device_domain_owner *owner;
> -
> - owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
> - if (!owner)
> - return -ENODEV;
> -
> - spin_lock(&dev_domain_list_spinlock);
> - if (find_device(dev)) {
> - spin_unlock(&dev_domain_list_spinlock);
> - kfree(owner);
> - return -EEXIST;
> - }
> - owner->domain = domain;
> - owner->dev = dev;
> - list_add_tail(&owner->list, &dev_domain_list);
> - spin_unlock(&dev_domain_list_spinlock);
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
> -
> -int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> - struct xen_device_domain_owner *owner;
> -
> - spin_lock(&dev_domain_list_spinlock);
> - owner = find_device(dev);
> - if (!owner) {
> - spin_unlock(&dev_domain_list_spinlock);
> - return -ENODEV;
> - }
> - list_del(&owner->list);
> - spin_unlock(&dev_domain_list_spinlock);
> - kfree(owner);
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
> #endif
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index a37eb52fb401..057ddf61ef61 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -182,7 +182,7 @@ config SWIOTLB_XEN
>
> config XEN_PCIDEV_BACKEND
> tristate "Xen PCI-device backend driver"
> - depends on PCI && X86 && XEN
> + depends on PCI && XEN
> depends on XEN_BACKEND
> default m
> help
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index a78704ae3618..35493ff0d146 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -65,6 +65,7 @@
> #include <xen/interface/vcpu.h>
> #include <xen/xenbus.h>
> #include <asm/hw_irq.h>
> +#include <xen/pci.h>

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

Juergen

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.06 kB)
OpenPGP public key OpenPGP_signature (505.00 B)
OpenPGP digital signature Download all attachments

2021-09-21 08:03:12

by Oleksandr Andrushchenko

[permalink] [raw]

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

On 21.09.21 10:54, Juergen Gross wrote:
> On 17.09.21 15:01, 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]>
>>
>> ---
>> Tested on Arm and x86.
>> ---
>> arch/x86/include/asm/xen/pci.h     | 24 ----------
>> arch/x86/pci/xen.c                 | 74 +----------------------------
>> drivers/xen/Kconfig                | 2 +-
>> drivers/xen/events/events_base.c   | 1 +
>> drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>> drivers/xen/xen-pciback/pci_stub.c | 3 +-
>> drivers/xen/xen-pciback/xenbus.c   | 2 +-
>> include/xen/pci.h                  | 34 ++++++++++++++
>> 8 files changed, 115 insertions(+), 100 deletions(-)
>> create mode 100644 include/xen/pci.h
>>
>> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
>> index 3506d8c598c1..9ff7b49bca08 100644
>> --- a/arch/x86/include/asm/xen/pci.h
>> +++ b/arch/x86/include/asm/xen/pci.h
>> @@ -14,30 +14,6 @@ static inline int pci_xen_hvm_init(void)
>>       return -1;
>> }
>> #endif
>> -#if defined(CONFIG_XEN_DOM0)
>> -int __init pci_xen_initial_domain(void);
>
> Why are you removing this prototype? It is X86 specific.
Indeed it is. Will not remove it form x86 code
>
>> -int xen_find_device_domain_owner(struct pci_dev *dev);
>> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
>> -int xen_unregister_device_domain_owner(struct pci_dev *dev);
>> -#else
>> -static inline int __init pci_xen_initial_domain(void)
>> -{
>> -    return -1;
>> -}
>> -static inline int xen_find_device_domain_owner(struct pci_dev *dev)
>> -{
>> -    return -1;
>> -}
>> -static inline int xen_register_device_domain_owner(struct pci_dev *dev,
>> -                           uint16_t domain)
>> -{
>> -    return -1;
>> -}
>> -static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> -{
>> -    return -1;
>> -}
>> -#endif
>> #if defined(CONFIG_PCI_MSI)
>> #if defined(CONFIG_PCI_XEN)
>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>> index 3d41a09c2c14..4a45b0bf9ae4 100644
>> --- a/arch/x86/pci/xen.c
>> +++ b/arch/x86/pci/xen.c
>> @@ -23,6 +23,7 @@
>> #include <xen/features.h>
>> #include <xen/events.h>
>> +#include <xen/pci.h>
>> #include <asm/xen/pci.h>
>> #include <asm/xen/cpuid.h>
>> #include <asm/apic.h>
>> @@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
>>       }
>>       return 0;
>> }
>> -
>> -struct xen_device_domain_owner {
>> -    domid_t domain;
>> -    struct pci_dev *dev;
>> -    struct list_head list;
>> -};
>> -
>> -static DEFINE_SPINLOCK(dev_domain_list_spinlock);
>> -static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
>> -
>> -static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
>> -{
>> -    struct xen_device_domain_owner *owner;
>> -
>> -    list_for_each_entry(owner, &dev_domain_list, list) {
>> -        if (owner->dev == dev)
>> -            return owner;
>> -    }
>> -    return NULL;
>> -}
>> -
>> -int xen_find_device_domain_owner(struct pci_dev *dev)
>> -{
>> -    struct xen_device_domain_owner *owner;
>> -    int domain = -ENODEV;
>> -
>> -    spin_lock(&dev_domain_list_spinlock);
>> -    owner = find_device(dev);
>> -    if (owner)
>> -        domain = owner->domain;
>> -    spin_unlock(&dev_domain_list_spinlock);
>> -    return domain;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
>> -
>> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
>> -{
>> -    struct xen_device_domain_owner *owner;
>> -
>> -    owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
>> -    if (!owner)
>> -        return -ENODEV;
>> -
>> -    spin_lock(&dev_domain_list_spinlock);
>> -    if (find_device(dev)) {
>> -        spin_unlock(&dev_domain_list_spinlock);
>> -        kfree(owner);
>> -        return -EEXIST;
>> -    }
>> -    owner->domain = domain;
>> -    owner->dev = dev;
>> -    list_add_tail(&owner->list, &dev_domain_list);
>> -    spin_unlock(&dev_domain_list_spinlock);
>> -    return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
>> -
>> -int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> -{
>> -    struct xen_device_domain_owner *owner;
>> -
>> -    spin_lock(&dev_domain_list_spinlock);
>> -    owner = find_device(dev);
>> -    if (!owner) {
>> -        spin_unlock(&dev_domain_list_spinlock);
>> -        return -ENODEV;
>> -    }
>> -    list_del(&owner->list);
>> -    spin_unlock(&dev_domain_list_spinlock);
>> -    kfree(owner);
>> -    return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
>> #endif
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index a37eb52fb401..057ddf61ef61 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -182,7 +182,7 @@ config SWIOTLB_XEN
>> config XEN_PCIDEV_BACKEND
>>       tristate "Xen PCI-device backend driver"
>> -    depends on PCI && X86 && XEN
>> +    depends on PCI && XEN
>>       depends on XEN_BACKEND
>>       default m
>>       help
>> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
>> index a78704ae3618..35493ff0d146 100644
>> --- a/drivers/xen/events/events_base.c
>> +++ b/drivers/xen/events/events_base.c
>> @@ -65,6 +65,7 @@
>> #include <xen/interface/vcpu.h>
>> #include <xen/xenbus.h>
>> #include <asm/hw_irq.h>
>> +#include <xen/pci.h>
>
> This wouldn't be needed if you'd let the pci_xen_initial_domain()
> prototype where it has been.
Sure, will leave it where it was
>
>
> Juergen

2021-09-21 21:24:35

by Stefano Stabellini

[permalink] [raw]

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

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

This is even better.

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

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

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

2021-09-22 12:52:46

by Oleksandr Andrushchenko

[permalink] [raw]

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

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