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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hello, Stefano!

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

Not only that, so pci-stub is not enough

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

1. pciback is used as a database for assignable PCI devices, e.g. xl
    pci-assignable-{add|remove|list} manipulates that list. So, whenever the
    toolstack needs to know which PCI devices can be passed through it reads
    that from the relevant sysfs entries of the pciback.

2. pciback is used to hold the unbound PCI devices, e.g. when passing through
    a PCI device it needs to be unbound from the relevant device driver and 
bound
    to pciback (strictly speaking it is not required that the device is bound to
    pciback, but pciback is again used as a database of the passed through PCI
    devices, so we can re-bind the devices back to their original drivers when
    guest domain shuts down)

3. Device reset

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

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

Please see [1] and [2]:

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

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

the rest like vPCI etc.

3. pcifront is not used on Arm

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

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

all the goals above.

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

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

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

Hope this sheds some light,

Oleksandr

>
>
> On Fri, 17 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> Xen-pciback driver was designed to be built for x86 only. But it
>> can also be used by other architectures, e.g. Arm.
>> Re-structure the driver in a way that it can be built for other
>> platforms as well.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@xxxxxxxx>
>>
>> ---
>> Tested on Arm and x86.
>> ---
>>   arch/x86/include/asm/xen/pci.h     | 24 ----------
>>   arch/x86/pci/xen.c                 | 74 +----------------------------
>>   drivers/xen/Kconfig                |  2 +-
>>   drivers/xen/events/events_base.c   |  1 +
>>   drivers/xen/pci.c                  | 75 ++++++++++++++++++++++++++++++
>>   drivers/xen/xen-pciback/pci_stub.c |  3 +-
>>   drivers/xen/xen-pciback/xenbus.c   |  2 +-
>>   include/xen/pci.h                  | 34 ++++++++++++++
>>   8 files changed, 115 insertions(+), 100 deletions(-)
>>   create mode 100644 include/xen/pci.h
>>
>> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
>> index 3506d8c598c1..9ff7b49bca08 100644
>> --- a/arch/x86/include/asm/xen/pci.h
>> +++ b/arch/x86/include/asm/xen/pci.h
>> @@ -14,30 +14,6 @@ static inline int pci_xen_hvm_init(void)
>>      return -1;
>>   }
>>   #endif
>> -#if defined(CONFIG_XEN_DOM0)
>> -int __init pci_xen_initial_domain(void);
>> -int xen_find_device_domain_owner(struct pci_dev *dev);
>> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
>> -int xen_unregister_device_domain_owner(struct pci_dev *dev);
>> -#else
>> -static inline int __init pci_xen_initial_domain(void)
>> -{
>> -    return -1;
>> -}
>> -static inline int xen_find_device_domain_owner(struct pci_dev *dev)
>> -{
>> -    return -1;
>> -}
>> -static inline int xen_register_device_domain_owner(struct pci_dev *dev,
>> -                                               uint16_t domain)
>> -{
>> -    return -1;
>> -}
>> -static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> -{
>> -    return -1;
>> -}
>> -#endif
>>   
>>   #if defined(CONFIG_PCI_MSI)
>>   #if defined(CONFIG_PCI_XEN)
>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>> index 3d41a09c2c14..4a45b0bf9ae4 100644
>> --- a/arch/x86/pci/xen.c
>> +++ b/arch/x86/pci/xen.c
>> @@ -23,6 +23,7 @@
>>   
>>   #include <xen/features.h>
>>   #include <xen/events.h>
>> +#include <xen/pci.h>
>>   #include <asm/xen/pci.h>
>>   #include <asm/xen/cpuid.h>
>>   #include <asm/apic.h>
>> @@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
>>      }
>>      return 0;
>>   }
>> -
>> -struct xen_device_domain_owner {
>> -    domid_t domain;
>> -    struct pci_dev *dev;
>> -    struct list_head list;
>> -};
>> -
>> -static DEFINE_SPINLOCK(dev_domain_list_spinlock);
>> -static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
>> -
>> -static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
>> -{
>> -    struct xen_device_domain_owner *owner;
>> -
>> -    list_for_each_entry(owner, &dev_domain_list, list) {
>> -            if (owner->dev == dev)
>> -                    return owner;
>> -    }
>> -    return NULL;
>> -}
>> -
>> -int xen_find_device_domain_owner(struct pci_dev *dev)
>> -{
>> -    struct xen_device_domain_owner *owner;
>> -    int domain = -ENODEV;
>> -
>> -    spin_lock(&dev_domain_list_spinlock);
>> -    owner = find_device(dev);
>> -    if (owner)
>> -            domain = owner->domain;
>> -    spin_unlock(&dev_domain_list_spinlock);
>> -    return domain;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
>> -
>> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
>> -{
>> -    struct xen_device_domain_owner *owner;
>> -
>> -    owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
>> -    if (!owner)
>> -            return -ENODEV;
>> -
>> -    spin_lock(&dev_domain_list_spinlock);
>> -    if (find_device(dev)) {
>> -            spin_unlock(&dev_domain_list_spinlock);
>> -            kfree(owner);
>> -            return -EEXIST;
>> -    }
>> -    owner->domain = domain;
>> -    owner->dev = dev;
>> -    list_add_tail(&owner->list, &dev_domain_list);
>> -    spin_unlock(&dev_domain_list_spinlock);
>> -    return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
>> -
>> -int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> -{
>> -    struct xen_device_domain_owner *owner;
>> -
>> -    spin_lock(&dev_domain_list_spinlock);
>> -    owner = find_device(dev);
>> -    if (!owner) {
>> -            spin_unlock(&dev_domain_list_spinlock);
>> -            return -ENODEV;
>> -    }
>> -    list_del(&owner->list);
>> -    spin_unlock(&dev_domain_list_spinlock);
>> -    kfree(owner);
>> -    return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
>>   #endif
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index a37eb52fb401..057ddf61ef61 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -182,7 +182,7 @@ config SWIOTLB_XEN
>>   
>>   config XEN_PCIDEV_BACKEND
>>      tristate "Xen PCI-device backend driver"
>> -    depends on PCI && X86 && XEN
>> +    depends on PCI && XEN
>>      depends on XEN_BACKEND
>>      default m
>>      help
>> diff --git a/drivers/xen/events/events_base.c 
>> b/drivers/xen/events/events_base.c
>> index a78704ae3618..35493ff0d146 100644
>> --- a/drivers/xen/events/events_base.c
>> +++ b/drivers/xen/events/events_base.c
>> @@ -65,6 +65,7 @@
>>   #include <xen/interface/vcpu.h>
>>   #include <xen/xenbus.h>
>>   #include <asm/hw_irq.h>
>> +#include <xen/pci.h>
>>   
>>   #include "events_internal.h"
>>   
>> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
>> index 224df03ce42e..fc8c1249d49f 100644
>> --- a/drivers/xen/pci.c
>> +++ b/drivers/xen/pci.c
>> @@ -254,3 +254,78 @@ static int xen_mcfg_late(void)
>>      return 0;
>>   }
>>   #endif
>> +
>> +#ifdef CONFIG_XEN_DOM0
>> +struct xen_device_domain_owner {
>> +    domid_t domain;
>> +    struct pci_dev *dev;
>> +    struct list_head list;
>> +};
>> +
>> +static DEFINE_SPINLOCK(dev_domain_list_spinlock);
>> +static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
>> +
>> +static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
>> +{
>> +    struct xen_device_domain_owner *owner;
>> +
>> +    list_for_each_entry(owner, &dev_domain_list, list) {
>> +            if (owner->dev == dev)
>> +                    return owner;
>> +    }
>> +    return NULL;
>> +}
>> +
>> +int xen_find_device_domain_owner(struct pci_dev *dev)
>> +{
>> +    struct xen_device_domain_owner *owner;
>> +    int domain = -ENODEV;
>> +
>> +    spin_lock(&dev_domain_list_spinlock);
>> +    owner = find_device(dev);
>> +    if (owner)
>> +            domain = owner->domain;
>> +    spin_unlock(&dev_domain_list_spinlock);
>> +    return domain;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
>> +
>> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
>> +{
>> +    struct xen_device_domain_owner *owner;
>> +
>> +    owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
>> +    if (!owner)
>> +            return -ENODEV;
>> +
>> +    spin_lock(&dev_domain_list_spinlock);
>> +    if (find_device(dev)) {
>> +            spin_unlock(&dev_domain_list_spinlock);
>> +            kfree(owner);
>> +            return -EEXIST;
>> +    }
>> +    owner->domain = domain;
>> +    owner->dev = dev;
>> +    list_add_tail(&owner->list, &dev_domain_list);
>> +    spin_unlock(&dev_domain_list_spinlock);
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
>> +
>> +int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> +{
>> +    struct xen_device_domain_owner *owner;
>> +
>> +    spin_lock(&dev_domain_list_spinlock);
>> +    owner = find_device(dev);
>> +    if (!owner) {
>> +            spin_unlock(&dev_domain_list_spinlock);
>> +            return -ENODEV;
>> +    }
>> +    list_del(&owner->list);
>> +    spin_unlock(&dev_domain_list_spinlock);
>> +    kfree(owner);
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
>> +#endif
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c 
>> b/drivers/xen/xen-pciback/pci_stub.c
>> index f8e4faa96ad6..bba527620507 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -19,7 +19,8 @@
>>   #include <linux/sched.h>
>>   #include <linux/atomic.h>
>>   #include <xen/events.h>
>> -#include <asm/xen/pci.h>
>> +#include <xen/pci.h>
>> +#include <xen/xen.h>
>>   #include <asm/xen/hypervisor.h>
>>   #include <xen/interface/physdev.h>
>>   #include "pciback.h"
>> diff --git a/drivers/xen/xen-pciback/xenbus.c 
>> b/drivers/xen/xen-pciback/xenbus.c
>> index c09c7ebd6968..da34ce85dc88 100644
>> --- a/drivers/xen/xen-pciback/xenbus.c
>> +++ b/drivers/xen/xen-pciback/xenbus.c
>> @@ -14,7 +14,7 @@
>>   #include <linux/workqueue.h>
>>   #include <xen/xenbus.h>
>>   #include <xen/events.h>
>> -#include <asm/xen/pci.h>
>> +#include <xen/pci.h>
>>   #include "pciback.h"
>>   
>>   #define INVALID_EVTCHN_IRQ  (-1)
>> diff --git a/include/xen/pci.h b/include/xen/pci.h
>> new file mode 100644
>> index 000000000000..5c988d5ff38f
>> --- /dev/null
>> +++ b/include/xen/pci.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __XEN_PCI_H__
>> +#define __XEN_PCI_H__
>> +
>> +#if defined(CONFIG_XEN_DOM0)
>> +int __init pci_xen_initial_domain(void);
>> +int xen_find_device_domain_owner(struct pci_dev *dev);
>> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
>> +int xen_unregister_device_domain_owner(struct pci_dev *dev);
>> +#else
>> +static inline int __init pci_xen_initial_domain(void)
>> +{
>> +    return -1;
>> +}
>> +
>> +static inline int xen_find_device_domain_owner(struct pci_dev *dev)
>> +{
>> +    return -1;
>> +}
>> +
>> +static inline int xen_register_device_domain_owner(struct pci_dev *dev,
>> +                                               uint16_t domain)
>> +{
>> +    return -1;
>> +}
>> +
>> +static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
>> +{
>> +    return -1;
>> +}
>> +#endif
>> +
>> +#endif
>> -- 
>> 2.25.1
>>
[1] https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxxxxxxxxx/msg84452.html

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