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

From: Stefano Stabellini
Date: Thu Sep 23 2021 - 16:00:42 EST

Next message: Leonard Crestez: "[PATCH] kernel-doc: Count enum warnings for -Werror"
Previous message: Vasyl Gomonovych: "[PATCH] ACPI: APEI: Check NULL argument in exported symbol"
In reply to: Oleksandr Andrushchenko: "[PATCH v3 2/2] xen-pciback: allow compiling on other archs than x86"
Next in thread: Jan Beulich: "Re: [PATCH v3 1/2] xen-pciback: prepare for the split for stub and PV"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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

drivers/xen/xen-pciback/conf_space_header.c: In function ‘bar_init’:
drivers/xen/xen-pciback/conf_space_header.c:239:34: warning: right shift count >= width of type [-Wshift-count-overflow]
bar->val = res[pos - 1].start >> 32;
^~
drivers/xen/xen-pciback/conf_space_header.c:240:49: warning: right shift count >= width of type [-Wshift-count-overflow]
bar->len_val = -resource_size(&res[pos - 1]) >> 32;

resource_size_t is defined as phys_addr_t and it can be 32bit on arm32.

One fix is to surround:

if (pos && (res[pos - 1].flags & IORESOURCE_MEM_64)) {
bar->val = res[pos - 1].start >> 32;
bar->len_val = -resource_size(&res[pos - 1]) >> 32;
return bar;
}

with #ifdef PHYS_ADDR_T_64BIT

> ---
> Since v2:
> - swap the patch order
> Since v1:
> - Do not move pci_xen_initial_domain as it is x86 specific
> ---
> arch/x86/include/asm/xen/pci.h | 18 +------
> arch/x86/pci/xen.c | 74 +----------------------------
> drivers/xen/pci.c | 75 ++++++++++++++++++++++++++++++
> drivers/xen/xen-pciback/pci_stub.c | 3 +-
> drivers/xen/xen-pciback/xenbus.c | 2 +-
> include/xen/pci.h | 28 +++++++++++
> 6 files changed, 108 insertions(+), 92 deletions(-)
> create mode 100644 include/xen/pci.h
>
> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
> index 3506d8c598c1..2889f091f459 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -16,26 +16,10 @@ static inline int pci_xen_hvm_init(void)
> #endif
> #if defined(CONFIG_XEN_DOM0)
> int __init pci_xen_initial_domain(void);
> -int xen_find_device_domain_owner(struct pci_dev *dev);
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
> -int xen_unregister_device_domain_owner(struct pci_dev *dev);
> #else
> static inline int __init pci_xen_initial_domain(void)
> {
> - return -1;
> -}
> -static inline int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> - return -1;
> -}
> -static inline int xen_register_device_domain_owner(struct pci_dev *dev,
> - uint16_t domain)
> -{
> - return -1;
> -}
> -static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> - return -1;
> + return -1;
> }
> #endif
>
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 3d41a09c2c14..4a45b0bf9ae4 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -23,6 +23,7 @@
>
> #include <xen/features.h>
> #include <xen/events.h>
> +#include <xen/pci.h>
> #include <asm/xen/pci.h>
> #include <asm/xen/cpuid.h>
> #include <asm/apic.h>
> @@ -583,77 +584,4 @@ int __init pci_xen_initial_domain(void)
> }
> return 0;
> }
> -
> -struct xen_device_domain_owner {
> - domid_t domain;
> - struct pci_dev *dev;
> - struct list_head list;
> -};
> -
> -static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> -static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> -
> -static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
> -{
> - struct xen_device_domain_owner *owner;
> -
> - list_for_each_entry(owner, &dev_domain_list, list) {
> - if (owner->dev == dev)
> - return owner;
> - }
> - return NULL;
> -}
> -
> -int xen_find_device_domain_owner(struct pci_dev *dev)
> -{
> - struct xen_device_domain_owner *owner;
> - int domain = -ENODEV;
> -
> - spin_lock(&dev_domain_list_spinlock);
> - owner = find_device(dev);
> - if (owner)
> - domain = owner->domain;
> - spin_unlock(&dev_domain_list_spinlock);
> - return domain;
> -}
> -EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
> -
> -int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
> -{
> - struct xen_device_domain_owner *owner;
> -
> - owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
> - if (!owner)
> - return -ENODEV;
> -
> - spin_lock(&dev_domain_list_spinlock);
> - if (find_device(dev)) {
> - spin_unlock(&dev_domain_list_spinlock);
> - kfree(owner);
> - return -EEXIST;
> - }
> - owner->domain = domain;
> - owner->dev = dev;
> - list_add_tail(&owner->list, &dev_domain_list);
> - spin_unlock(&dev_domain_list_spinlock);
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
> -
> -int xen_unregister_device_domain_owner(struct pci_dev *dev)
> -{
> - struct xen_device_domain_owner *owner;
> -
> - spin_lock(&dev_domain_list_spinlock);
> - owner = find_device(dev);
> - if (!owner) {
> - spin_unlock(&dev_domain_list_spinlock);
> - return -ENODEV;
> - }
> - list_del(&owner->list);
> - spin_unlock(&dev_domain_list_spinlock);
> - kfree(owner);
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
> #endif
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> index 224df03ce42e..fc8c1249d49f 100644
> --- a/drivers/xen/pci.c
> +++ b/drivers/xen/pci.c
> @@ -254,3 +254,78 @@ static int xen_mcfg_late(void)
> return 0;
> }
> #endif
> +
> +#ifdef CONFIG_XEN_DOM0
> +struct xen_device_domain_owner {
> + domid_t domain;
> + struct pci_dev *dev;
> + struct list_head list;
> +};
> +
> +static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> +static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> +
> +static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
> +{
> + struct xen_device_domain_owner *owner;
> +
> + list_for_each_entry(owner, &dev_domain_list, list) {
> + if (owner->dev == dev)
> + return owner;
> + }
> + return NULL;
> +}
> +
> +int xen_find_device_domain_owner(struct pci_dev *dev)
> +{
> + struct xen_device_domain_owner *owner;
> + int domain = -ENODEV;
> +
> + spin_lock(&dev_domain_list_spinlock);
> + owner = find_device(dev);
> + if (owner)
> + domain = owner->domain;
> + spin_unlock(&dev_domain_list_spinlock);
> + return domain;
> +}
> +EXPORT_SYMBOL_GPL(xen_find_device_domain_owner);
> +
> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)
> +{
> + struct xen_device_domain_owner *owner;
> +
> + owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
> + if (!owner)
> + return -ENODEV;
> +
> + spin_lock(&dev_domain_list_spinlock);
> + if (find_device(dev)) {
> + spin_unlock(&dev_domain_list_spinlock);
> + kfree(owner);
> + return -EEXIST;
> + }
> + owner->domain = domain;
> + owner->dev = dev;
> + list_add_tail(&owner->list, &dev_domain_list);
> + spin_unlock(&dev_domain_list_spinlock);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(xen_register_device_domain_owner);
> +
> +int xen_unregister_device_domain_owner(struct pci_dev *dev)
> +{
> + struct xen_device_domain_owner *owner;
> +
> + spin_lock(&dev_domain_list_spinlock);
> + owner = find_device(dev);
> + if (!owner) {
> + spin_unlock(&dev_domain_list_spinlock);
> + return -ENODEV;
> + }
> + list_del(&owner->list);
> + spin_unlock(&dev_domain_list_spinlock);
> + kfree(owner);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
> +#endif
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index f8e4faa96ad6..bba527620507 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -19,7 +19,8 @@
> #include <linux/sched.h>
> #include <linux/atomic.h>
> #include <xen/events.h>
> -#include <asm/xen/pci.h>
> +#include <xen/pci.h>
> +#include <xen/xen.h>
> #include <asm/xen/hypervisor.h>
> #include <xen/interface/physdev.h>
> #include "pciback.h"
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index f8ba2903a3ff..bde63ef677b8 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -14,7 +14,7 @@
> #include <linux/workqueue.h>
> #include <xen/xenbus.h>
> #include <xen/events.h>
> -#include <asm/xen/pci.h>
> +#include <xen/pci.h>
> #include "pciback.h"
>
> #define INVALID_EVTCHN_IRQ (-1)
> diff --git a/include/xen/pci.h b/include/xen/pci.h
> new file mode 100644
> index 000000000000..b8337cf85fd1
> --- /dev/null
> +++ b/include/xen/pci.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __XEN_PCI_H__
> +#define __XEN_PCI_H__
> +
> +#if defined(CONFIG_XEN_DOM0)
> +int xen_find_device_domain_owner(struct pci_dev *dev);
> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
> +int xen_unregister_device_domain_owner(struct pci_dev *dev);
> +#else
> +static inline int xen_find_device_domain_owner(struct pci_dev *dev)
> +{
> + return -1;
> +}
> +
> +static inline int xen_register_device_domain_owner(struct pci_dev *dev,
> + uint16_t domain)
> +{
> + return -1;
> +}
> +
> +static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
> +{
> + return -1;
> +}
> +#endif
> +
> +#endif
> --
> 2.25.1
>

Next message: Leonard Crestez: "[PATCH] kernel-doc: Count enum warnings for -Werror"
Previous message: Vasyl Gomonovych: "[PATCH] ACPI: APEI: Check NULL argument in exported symbol"
In reply to: Oleksandr Andrushchenko: "[PATCH v3 2/2] xen-pciback: allow compiling on other archs than x86"
Next in thread: Jan Beulich: "Re: [PATCH v3 1/2] xen-pciback: prepare for the split for stub and PV"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]