From: Rahul Singh <Rahul.Singh@arm.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
Julien Grall <julien@xen.org>,
Bertrand Marquis <Bertrand.Marquis@arm.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
nd <nd@arm.com>, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [RFC PATCH v1 1/4] arm/pci: PCI setup and PCI host bridge discovery within XEN on ARM.
Date: Tue, 28 Jul 2020 08:06:17 +0000 [thread overview]
Message-ID: <567A8D4D-9D7F-4D53-B740-6095F5512026@arm.com> (raw)
In-Reply-To: <20200724144404.GJ7191@Air-de-Roger>
> On 24 Jul 2020, at 3:44 pm, Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Thu, Jul 23, 2020 at 04:40:21PM +0100, Rahul Singh wrote:
>> XEN during boot will read the PCI device tree node “reg” property
>> and will map the PCI config space to the XEN memory.
>>
>> XEN will read the “linux, pci-domain” property from the device tree
>> node and configure the host bridge segment number accordingly.
>>
>> As of now "pci-host-ecam-generic" compatible board is supported.
>>
>> Change-Id: If32f7748b7dc89dd37114dc502943222a2a36c49
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> xen/arch/arm/Kconfig | 7 +
>> xen/arch/arm/Makefile | 1 +
>> xen/arch/arm/pci/Makefile | 4 +
>> xen/arch/arm/pci/pci-access.c | 101 ++++++++++++++
>> xen/arch/arm/pci/pci-host-common.c | 198 ++++++++++++++++++++++++++++
>> xen/arch/arm/pci/pci-host-generic.c | 131 ++++++++++++++++++
>> xen/arch/arm/pci/pci.c | 112 ++++++++++++++++
>> xen/arch/arm/setup.c | 2 +
>> xen/include/asm-arm/device.h | 7 +-
>> xen/include/asm-arm/pci.h | 97 +++++++++++++-
>> 10 files changed, 654 insertions(+), 6 deletions(-)
>> create mode 100644 xen/arch/arm/pci/Makefile
>> create mode 100644 xen/arch/arm/pci/pci-access.c
>> create mode 100644 xen/arch/arm/pci/pci-host-common.c
>> create mode 100644 xen/arch/arm/pci/pci-host-generic.c
>> create mode 100644 xen/arch/arm/pci/pci.c
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 2777388265..ee13339ae9 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -31,6 +31,13 @@ menu "Architecture Features"
>>
>> source "arch/Kconfig"
>>
>> +config ARM_PCI
>> + bool "PCI Passthrough Support"
>> + depends on ARM_64
>> + ---help---
>> +
>> + PCI passthrough support for Xen on ARM64.
>> +
>> config ACPI
>> bool "ACPI (Advanced Configuration and Power Interface) Support" if EXPERT
>> depends on ARM_64
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 7e82b2178c..345cb83eed 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -6,6 +6,7 @@ ifneq ($(CONFIG_NO_PLAT),y)
>> obj-y += platforms/
>> endif
>> obj-$(CONFIG_TEE) += tee/
>> +obj-$(CONFIG_ARM_PCI) += pci/
>>
>> obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
>> obj-y += bootfdt.init.o
>> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
>> new file mode 100644
>> index 0000000000..358508b787
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/Makefile
>> @@ -0,0 +1,4 @@
>> +obj-y += pci.o
>> +obj-y += pci-host-generic.o
>> +obj-y += pci-host-common.o
>> +obj-y += pci-access.o
>
> The Kconfig option mentions the support being explicitly for ARM64,
> would it be better to place the code in arch/arm/arm64 then?
Ok.As julien also mentioned we tested on ARM64 , We have to test on ARM32 platforms.
>
>> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
>> new file mode 100644
>> index 0000000000..c53ef58336
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-access.c
>> @@ -0,0 +1,101 @@
>> +/*
>> + * Copyright (C) 2020 Arm Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/init.h>
>> +#include <xen/pci.h>
>> +#include <asm/pci.h>
>> +#include <xen/rwlock.h>
>> +
>> +static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>> + unsigned int len)
>
> Please align with the opening parenthesis (here and everywhere in the
> patch series).
Ack.
>
>> +{
>> + int rc;
>> + uint32_t val = GENMASK(0, len * 8);
>
> You can just set val = ~0. The return type of the pci_conf_readXX
> helpers will already truncate the value.
>
Ack.
>> +
>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
>> +
>> + if ( unlikely(!bridge) )
>> + {
>> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
>> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>
> I had a patch to add a custom modifier to out printf format in
> order to handle pci_sbdf_t natively:
>
> https://patchew.org/Xen/20190822065132.48200-1-roger.pau@citrix.com/
>
> It missed maintainers Acks and was never committed. Since you are
> doing a bunch of work here, and likely adding a lot of SBDF related
> prints, feel free to import the modifier (%pp) and use in your code
> (do not attempt to switch existing users, or it's likely to get
> stuck again).
Ok Will integrate that patch once submitted.
>
>> + return val;
>> + }
>> +
>> + if ( unlikely(!bridge->ops->read) )
>> + return val;
>> +
>> + rc = bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
>
> There's no need for the uint32_t cast, the sbdf field is already of
> such type
Ack.
>> + if ( rc )
>> + printk(XENLOG_ERR "Failed to read reg %#x len %u for "PRI_pci"\n",
>> + reg, len, sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>> +
>> + return val;
>> +}
>> +
>> +static void pci_config_write(pci_sbdf_t sbdf, unsigned int reg,
>> + unsigned int len, uint32_t val)
>> +{
>> + int rc;
>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
>> +
>> + if ( unlikely(!bridge) )
>> + {
>> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
>> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>> + return;
>> + }
>> +
>> + if ( unlikely(!bridge->ops->write) )
>> + return;
>> +
>> + rc = bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val);
>> + if ( rc )
>> + printk(XENLOG_ERR "Failed to write reg %#x len %u for "PRI_pci"\n",
>> + reg, len, sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>> +}
>> +
>> +/*
>> + * Wrappers for all PCI configuration access functions.
>> + */
>> +
>> +#define PCI_OP_WRITE(size, type) \
>> + void pci_conf_write##size (pci_sbdf_t sbdf,unsigned int reg, type val) \
>> +{ \
>> + pci_config_write(sbdf, reg, size / 8, val); \
>> +}
>> +
>> +#define PCI_OP_READ(size, type) \
>> + type pci_conf_read##size (pci_sbdf_t sbdf, unsigned int reg) \
>> +{ \
>> + return pci_config_read(sbdf, reg, size / 8); \
>> +}
>> +
>> +PCI_OP_READ(8, u8)
>> +PCI_OP_READ(16, u16)
>> +PCI_OP_READ(32, u32)
>> +PCI_OP_WRITE(8, u8)
>> +PCI_OP_WRITE(16, u16)
>> +PCI_OP_WRITE(32, u32)
>
> Please use uintXX_t.
>
> Also, It's nice to add some kind of signals for cscope and friends so
> they can find the autogenerated functions, ie:
>
> #define pci_conf_read8
> #undef pci_conf_read8
> #define pci_conf_read16
> #undef pci_conf_read16
> ...
>
> It's tedious but helps future users find where the code is generated.
Ack.
>
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> new file mode 100644
>> index 0000000000..c5f98be698
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -0,0 +1,198 @@
>> +/*
>> + * Copyright (C) 2020 Arm Ltd.
>> + *
>> + * Based on Linux drivers/pci/ecam.c
>> + * Copyright 2016 Broadcom.
>> + *
>> + * Based on Linux drivers/pci/controller/pci-host-common.c
>> + * Based on Linux drivers/pci/controller/pci-host-generic.c
>> + * Copyright (C) 2014 ARM Limited Will Deacon <will.deacon@arm.com>
>> + *
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/init.h>
>> +#include <xen/pci.h>
>> +#include <asm/pci.h>
>> +#include <xen/rwlock.h>
>> +#include <xen/vmap.h>
>> +
>> +/*
>> + * List for all the pci host bridges.
>> + */
>> +
>> +static LIST_HEAD(pci_host_bridges);
>> +
>> +static bool __init dt_pci_parse_bus_range(struct dt_device_node *dev,
>> + struct pci_config_window *cfg)
>> +{
>> + const __be32 *cells;
>
> It's my impression that while based on Linux this is not a verbatim
> copy of a Linux file, and tries to adhere with the Xen coding style.
> If so please use uint32_t here.
>
>> + uint32_t len;
>> +
>> + cells = dt_get_property(dev, "bus-range", &len);
>> + /* bus-range should at least be 2 cells */
>> + if ( !cells || (len < (sizeof(*cells) * 2)) )
>> + return false;
>> +
>> + cfg->busn_start = dt_next_cell(1, &cells);
>> + cfg->busn_end = dt_next_cell(1, &cells);
>> +
>> + return true;
>> +}
>> +
>> +static inline void __iomem *pci_remap_cfgspace(paddr_t start, size_t len)
>> +{
>> + return ioremap_nocache(start, len);
>> +}
>> +
>> +static void pci_ecam_free(struct pci_config_window *cfg)
>> +{
>> + if ( cfg->win )
>> + iounmap(cfg->win);
>> +
>> + xfree(cfg);
>> +}
>> +
>> +static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>> + struct pci_ecam_ops *ops)
>> +{
>> + int err;
>> + struct pci_config_window *cfg;
>> + paddr_t addr, size;
>> +
>> + cfg = xzalloc(struct pci_config_window);
>> + if ( !cfg )
>> + return NULL;
>> +
>> + err = dt_pci_parse_bus_range(dev, cfg);
>> + if ( !err ) {
>
> Braces
Ack
>
>> + cfg->busn_start = 0;
>> + cfg->busn_end = 0xff;
>> + printk(XENLOG_ERR "No bus range found for pci controller\n");
>> + } else {
>> + if ( cfg->busn_end > cfg->busn_start + 0xff )
>> + cfg->busn_end = cfg->busn_start + 0xff;
>> + }
>> +
>> + /* Parse our PCI ecam register address*/
>> + err = dt_device_get_address(dev, 0, &addr, &size);
>> + if ( err )
>> + goto err_exit;
>> +
>> + cfg->phys_addr = addr;
>> + cfg->size = size;
>> + cfg->ops = ops;
>> +
>> + /*
>> + * On 64-bit systems, we do a single ioremap for the whole config space
>> + * since we have enough virtual address range available. On 32-bit, we
>> + * ioremap the config space for each bus individually.
>> + *
>> + * As of now only 64-bit is supported 32-bit is not supported.
>> + */
>> + cfg->win = pci_remap_cfgspace(cfg->phys_addr, cfg->size);
>> + if ( !cfg->win )
>> + goto err_exit_remap;
>> +
>> + printk("ECAM at [mem %lx-%lx] for [bus %x-%x] \n",cfg->phys_addr,
>> + cfg->phys_addr + cfg->size - 1,cfg->busn_start,cfg->busn_end);
>> +
>> + if ( ops->init ) {
>> + err = ops->init(cfg);
>> + if (err)
>> + goto err_exit;
>> + }
>> +
>> + return cfg;
>> +
>> +err_exit_remap:
>> + printk(XENLOG_ERR "ECAM ioremap failed\n");
>> +err_exit:
>> + pci_ecam_free(cfg);
>> + return NULL;
>> +}
>> +
>> +static struct pci_host_bridge * pci_alloc_host_bridge(void)
> ^ extra space
Ack.
>> +{
>> + struct pci_host_bridge *bridge = xzalloc(struct pci_host_bridge);
>> +
>> + if ( !bridge )
>> + return NULL;
>> +
>> + INIT_LIST_HEAD(&bridge->node);
>> + return bridge;
>> +}
>> +
>> +int pci_host_common_probe(struct dt_device_node *dev,
>> + struct pci_ecam_ops *ops)
>> +{
>> + struct pci_host_bridge *bridge;
>> + struct pci_config_window *cfg;
>> + u32 segment;
>> +
>> + bridge = pci_alloc_host_bridge();
>> + if ( !bridge )
>> + return -ENOMEM;
>> +
>> + /* Parse and map our Configuration Space windows */
>> + cfg = gen_pci_init(dev, ops);
>> + if ( !cfg )
>> + return -ENOMEM;
>
> You are leaking the allocation of bridge here ...
>
>> +
>> + bridge->dt_node = dev;
>> + bridge->sysdata = cfg;
>> + bridge->ops = &ops->pci_ops;
>> +
>> + if( !dt_property_read_u32(dev, "linux,pci-domain", &segment) )
>> + {
>> + printk(XENLOG_ERR "\"linux,pci-domain\" property in not available in DT\n");
>> + return -ENODEV;
>
> ... and here.
>
>> + }
>> +
>> + bridge->segment = (u16)segment;
>> +
>> + list_add_tail(&bridge->node, &pci_host_bridges);
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * This function will lookup an hostbridge based on the segment and bus
>> + * number.
>> + */
>> +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus)
>> +{
>> + struct pci_host_bridge *bridge;
>> + bool found = false;
>> +
>> + list_for_each_entry( bridge, &pci_host_bridges, node )
>> + {
>> + if ( bridge->segment != segment )
>> + continue;
>> +
>> + found = true;
>> + break;
>> + }
>> +
>> + return (found) ? bridge : NULL;
>
> This can be much shorter:
>
> struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus)
> {
> struct pci_host_bridge *bridge;
>
> list_for_each_entry( bridge, &pci_host_bridges, node )
> if ( bridge->segment == segment )
> return bridge;
>
> return NULL;
> }
>
> Albeit I'm confused by the fact that you pass a bus number that's
> completely unused.
Ok Will fix.
>
>> +}
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
>> new file mode 100644
>> index 0000000000..cd67b3dec6
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-host-generic.c
>> @@ -0,0 +1,131 @@
>> +/*
>> + * Copyright (C) 2020 Arm Ltd.
>> + *
>> + * Based on Linux drivers/pci/controller/pci-host-common.c
>> + * Based on Linux drivers/pci/controller/pci-host-generic.c
>> + * Copyright (C) 2014 ARM Limited Will Deacon <will.deacon@arm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <asm/device.h>
>> +#include <asm/io.h>
>> +#include <xen/pci.h>
>> +#include <asm/pci.h>
>> +
>> +/*
>> + * Function to get the config space base.
>> + */
>> +static void __iomem *pci_config_base(struct pci_host_bridge *bridge,
>> + uint32_t sbdf, int where)
>
> You would be better passing a pci_sbdf_t directly here. Also 'where'
> should be renamed to offset, or reg, and be made unsigned int. AFAICT
> you will never pass a negative value here.
>
> For sanity you should also assert that the offset falls between the
> PCI config space used by the device, in order to easily catch with
> wrong offsets being used.
Ack.
>
>> +{
>> + struct pci_config_window *cfg = bridge->sysdata;
>
> const
Ack.
>
>> + unsigned int devfn_shift = cfg->ops->bus_shift - 8;
>> +
>> + pci_sbdf_t sbdf_t = (pci_sbdf_t) sbdf ;
>> +
>> + unsigned int busn = sbdf_t.bus;
>> + void __iomem *base;
>
> IMO adding newlines between variable definitions is not helpful, but
> that's my taste.
Ack.
>
>> +
>> + if ( busn < cfg->busn_start || busn > cfg->busn_end )
>> + return NULL;
>> +
>> + base = cfg->win + (busn << cfg->ops->bus_shift);
>> +
>> + return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
>> +}
>> +
>> +int pci_ecam_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + int where, int size, u32 val)
>> +{
>> + void __iomem *addr;
>> +
>> + addr = pci_config_base(bridge, sbdf, where);
>
> You can initialize at definition.
Ack.
>
>> + if ( !addr )
>> + return -ENODEV;
>> +
>> + if ( size == 1 )
>> + writeb(val, addr);
>> + else if ( size == 2 )
>> + writew(val, addr);
>> + else
>> + writel(val, addr);
>
> Please use a switch, and check against specific values. The default
> case should be a BUG();. See pci_conf_read from x86 for an example.
Ack.
>
>> +
>> + return 0;
>> +}
>> +
>> +int pci_ecam_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + int where, int size, u32 *val)
>> +{
>> + void __iomem *addr;
>> +
>> + addr = pci_config_base(bridge, sbdf, where);
>> + if ( !addr ) {
>> + *val = ~0;
>> + return -ENODEV;
>> + }
>> +
>> + if ( size == 1 )
>> + *val = readb(addr);
>> + else if ( size == 2 )
>> + *val = readw(addr);
>> + else
>> + *val = readl(addr);
>> +
>> + return 0;
>> +}
>> +
>> +/* ECAM ops */
>> +struct pci_ecam_ops pci_generic_ecam_ops = {
>> + .bus_shift = 20,
>> + .pci_ops = {
>> + .read = pci_ecam_config_read,
>> + .write = pci_ecam_config_write,
>> + }
>> +};
>> +
>> +static const struct dt_device_match gen_pci_dt_match[] = {
>> + { .compatible = "pci-host-ecam-generic",
>> + .data = &pci_generic_ecam_ops },
>> +
>> + { },
>> +};
>> +
>> +static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
>> +{
>> + const struct dt_device_match *of_id;
>> + struct pci_ecam_ops *ops;
>> +
>> + of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
>> + ops = (struct pci_ecam_ops *) of_id->data;
>> +
>> + printk(XENLOG_INFO "Found PCI host bridge %s compatible:%s \n",
>> + dt_node_full_name(dev), of_id->compatible);
>> +
>> + return pci_host_common_probe(dev, ops);
>> +}
>> +
>> +DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
>> +.dt_match = gen_pci_dt_match,
>> +.init = gen_pci_dt_init,
>> +DT_DEVICE_END
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
>> new file mode 100644
>> index 0000000000..f8cbb99591
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci.c
>> @@ -0,0 +1,112 @@
>> +/*
>> + * Copyright (C) 2020 Arm Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/acpi.h>
>> +#include <xen/device_tree.h>
>> +#include <xen/errno.h>
>> +#include <xen/init.h>
>> +#include <xen/pci.h>
>> +#include <xen/param.h>
>> +
>> +static int __init dt_pci_init(void)
>> +{
>> + struct dt_device_node *np;
>> + int rc;
>> +
>> + dt_for_each_device_node(dt_host, np)
>> + {
>> + rc = device_init(np, DEVICE_PCI, NULL);
>> + if( !rc )
>> + continue;
>> + /*
>> + * Ignore the following error codes:
>> + * - EBADF: Indicate the current is not an pci
>> + * - ENODEV: The pci device is not present or cannot be used by
>> + * Xen.
>> + */
>> + else if ( rc != -EBADF && rc != -ENODEV )
>> + {
>> + printk(XENLOG_ERR "No driver found in XEN or driver init error.\n");
>> + return rc;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_ACPI
>> +static void __init acpi_pci_init(void)
>> +{
>> + printk(XENLOG_ERR "ACPI pci init not supported \n");
>> + return;
>> +}
>> +#else
>> +static inline void __init acpi_pci_init(void) { }
>> +#endif
>> +
>> +static bool __initdata param_pci_enable;
>> +static int __init parse_pci_param(const char *arg)
>> +{
>> + if ( !arg )
>> + {
>> + param_pci_enable = false;
>> + return 0;
>> + }
>> +
>> + switch ( parse_bool(arg, NULL) )
>> + {
>> + case 0:
>> + param_pci_enable = false;
>> + return 0;
>> + case 1:
>> + param_pci_enable = true;
>> + return 0;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +custom_param("pci", parse_pci_param);
>
> You need to introduce the documentation for the parameter at
> docs/misc/xen-command-line.pandoc
>
> Albeit I'm not sure I like it, why do you need to enable PCI
> explicitly?
>
> Shouldn't it be discovered automatically and enabled by default?
Ack.
>
>> +void __init pci_init(void)
>> +{
>> + /*
>> + * Enable PCI when has been enabled explicitly (pci=on)
>> + */
>> + if ( !param_pci_enable)
>> + goto disable;
>
> Just return here, there's not point in having a label to perform a
> return.
Ack.
>
>> +
>> + if ( acpi_disabled )
>> + dt_pci_init();
>> + else
>> + acpi_pci_init();
>
> Isn't there an enum or something that tells you whether the system
> description is coming from ACPI or from DT?
>
> This if .. else seems fragile.
>
> Also for ACPI you will get called by acpi_boot_init, and likely need
> to implement a acpi_mmcfg_init or pci_mmcfg_arch_{init,enable}. I'm
> not sure whether the code in acpi_mmcfg_init could be made shared
> between both x86 and Arm.
>
>> +
>> +#ifdef CONFIG_HAS_PCI
>> + pci_segments_init();
>> +#endif
>> +
>> +disable:
>> + return;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 7968cee47d..2d7f1db44f 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -930,6 +930,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>>
>> setup_virt_paging();
>>
>> + pci_init();
>> +
>> do_initcalls();
>>
>> /*
>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>> index ee7cff2d44..28f8049cfd 100644
>> --- a/xen/include/asm-arm/device.h
>> +++ b/xen/include/asm-arm/device.h
>> @@ -4,6 +4,7 @@
>> enum device_type
>> {
>> DEV_DT,
>> + DEV_PCI,
>> };
>>
>> struct dev_archdata {
>> @@ -25,15 +26,15 @@ typedef struct device device_t;
>>
>> #include <xen/device_tree.h>
>>
>> -/* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */
>> -#define dev_is_pci(dev) ((void)(dev), 0)
>> -#define dev_is_dt(dev) ((dev->type == DEV_DT)
>> +#define dev_is_pci(dev) (dev->type == DEV_PCI)
>> +#define dev_is_dt(dev) (dev->type == DEV_DT)
>>
>> enum device_class
>> {
>> DEVICE_SERIAL,
>> DEVICE_IOMMU,
>> DEVICE_GIC,
>> + DEVICE_PCI,
>
> It seems to be like this wants to be DEVICE_PCI_HOST_BRIDGE or some
> such, since this is not used to identify all PCI devices, but just
> bridges?
>
>> /* Use for error */
>> DEVICE_UNKNOWN,
>> };
>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index de13359f65..94fd00360a 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -1,7 +1,98 @@
>> -#ifndef __X86_PCI_H__
>> -#define __X86_PCI_H__
>> +/*
>> + * Copyright (C) 2020 Arm Ltd.
>> + *
>> + * Based on Linux drivers/pci/ecam.c
>> + * Copyright 2016 Broadcom.
>> + *
>> + * Based on Linux drivers/pci/controller/pci-host-common.c
>> + * Based on Linux drivers/pci/controller/pci-host-generic.c
>> + * Copyright (C) 2014 ARM Limited Will Deacon <will.deacon@arm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>>
>> +#ifndef __ARM_PCI_H__
>> +#define __ARM_PCI_H__
>> +
>> +#include <xen/pci.h>
>> +#include <xen/device_tree.h>
>> +#include <asm/device.h>
>> +
>> +#ifdef CONFIG_ARM_PCI
>> +
>> +/* Arch pci dev struct */
>> struct arch_pci_dev {
>> + struct device dev;
>> +};
>
> This seems to be completely unused?
Will be using going forward.
>
>> +
>> +#define PRI_pci "%04x:%02x:%02x.%u"
>> +#define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
>> +
>> +/*
>> + * struct to hold the mappings of a config space window. This
>> + * is expected to be used as sysdata for PCI controllers that
>> + * use ECAM.
>> + */
>> +struct pci_config_window {
>> + paddr_t phys_addr;
>> + paddr_t size;
>> + uint8_t busn_start;
>> + uint8_t busn_end;
>> + struct pci_ecam_ops *ops;
>
> const?
>
>> + void __iomem *win;
>> +};
>> +
>> +/* Forward declaration as pci_host_bridge and pci_ops depend on each other. */
>> +struct pci_host_bridge;
>> +
>> +struct pci_ops {
>> + int (*read)(struct pci_host_bridge *bridge,
>> + uint32_t sbdf, int where, int size, u32 *val);
>> + int (*write)(struct pci_host_bridge *bridge,
>> + uint32_t sbdf, int where, int size, u32 val);
>> +};
>> +
>> +/*
>> + * struct to hold pci ops and bus shift of the config window
>> + * for a PCI controller.
>> + */
>> +struct pci_ecam_ops {
>> + unsigned int bus_shift;
>> + struct pci_ops pci_ops;
>> + int (*init)(struct pci_config_window *);
>> +};
>> +
>> +/*
>> + * struct to hold pci host bridge information
>> + * for a PCI controller.
>> + */
>> +struct pci_host_bridge {
>> + struct dt_device_node *dt_node; /* Pointer to the associated DT node */
>> + struct list_head node; /* Node in list of host bridges */
>> + uint16_t segment; /* Segment number */
>> + void *sysdata; /* Pointer to the config space window*/
>> + const struct pci_ops *ops;
>
> You seem to introduce a lot of ops structs, yet there's only one
> implementation the generic ECAM one, and adding such complexity should
> IMO be done when further implementations are added. Also given this is
> a fully ECAM compliant bridge you could just use most of the existing
> logic for x86?
>
> I understand the discovery needs to be different, but x86 MCFG logic
> should already be capable of handling multiple ECAM regions.
>
> I also agree with Julien that splitting this into separate patch would
> make it easier to review. For example you can start with the discovery
> logic, followed by the initialization and the add the accessors to the
> config space lastly.
Ok . Will spilt the patches in the next version of the patch series.
>
> Thanks, Roger.
next prev parent reply other threads:[~2020-07-28 8:07 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-23 15:40 [RFC PATCH v1 0/4] PCI devices passthrough on Arm Rahul Singh
2020-07-23 15:40 ` [RFC PATCH v1 1/4] arm/pci: PCI setup and PCI host bridge discovery within XEN on ARM Rahul Singh
2020-07-23 23:38 ` Stefano Stabellini
2020-07-24 7:03 ` Oleksandr Andrushchenko
2020-07-24 8:05 ` Julien Grall
2020-07-24 17:47 ` Stefano Stabellini
2020-07-27 15:27 ` Rahul Singh
2020-07-27 15:20 ` Rahul Singh
2020-07-24 8:44 ` Julien Grall
2020-07-24 17:41 ` Stefano Stabellini
2020-07-24 18:21 ` Julien Grall
2020-07-24 18:32 ` Stefano Stabellini
2020-07-24 19:24 ` Julien Grall
2020-07-24 23:46 ` Stefano Stabellini
2020-07-25 9:59 ` Julien Grall
2020-07-27 11:06 ` Roger Pau Monné
2020-07-28 0:06 ` Stefano Stabellini
2020-07-28 8:33 ` Roger Pau Monné
2020-07-28 18:33 ` Stefano Stabellini
2020-07-26 7:01 ` Jan Beulich
2020-07-27 13:27 ` Rahul Singh
2020-07-24 8:23 ` Julien Grall
2020-07-27 15:29 ` Rahul Singh
2020-07-24 14:44 ` Roger Pau Monné
2020-07-24 15:15 ` Julien Grall
2020-07-24 15:29 ` Roger Pau Monné
2020-07-24 15:42 ` Roger Pau Monné
2020-07-24 15:46 ` Julien Grall
2020-07-24 16:01 ` Jan Beulich
2020-07-24 16:54 ` Julien Grall
2020-07-27 10:34 ` Roger Pau Monné
2020-07-28 8:06 ` Rahul Singh [this message]
2020-07-28 8:21 ` Roger Pau Monné
2020-07-23 15:40 ` [RFC PATCH v1 2/4] xen/arm: Discovering PCI devices and add the PCI devices in XEN Rahul Singh
2020-07-23 20:44 ` Stefano Stabellini
2020-07-24 7:14 ` Oleksandr Andrushchenko
2020-07-24 8:19 ` Julien Grall
2020-07-27 16:10 ` Rahul Singh
2020-07-24 14:49 ` Roger Pau Monné
2020-07-27 8:40 ` Rahul Singh
2020-07-23 15:40 ` [RFC PATCH v1 3/4] xen/arm: Enable the existing x86 virtual PCI support for ARM Rahul Singh
2020-07-23 23:39 ` Stefano Stabellini
2020-07-24 15:08 ` Roger Pau Monné
2020-07-23 15:40 ` [RFC PATCH v1 4/4] arm/libxl: Emulated PCI device tree node in libxl Rahul Singh
2020-07-23 23:39 ` Stefano Stabellini
2020-07-24 7:55 ` Oleksandr Andrushchenko
2020-07-24 9:11 ` Julien Grall
2020-07-27 13:40 ` Rahul Singh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=567A8D4D-9D7F-4D53-B740-6095F5512026@arm.com \
--to=rahul.singh@arm.com \
--cc=Bertrand.Marquis@arm.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=julien@xen.org \
--cc=nd@arm.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).