From: Rahul Singh <Rahul.Singh@arm.com>
To: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
Julien Grall <julien@xen.org>,
"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
Bertrand Marquis <Bertrand.Marquis@arm.com>,
"jbeulich@suse.com" <jbeulich@suse.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
nd <nd@arm.com>, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
"roger.pau@citrix.com" <roger.pau@citrix.com>
Subject: Re: [RFC PATCH v1 1/4] arm/pci: PCI setup and PCI host bridge discovery within XEN on ARM.
Date: Mon, 27 Jul 2020 15:20:06 +0000 [thread overview]
Message-ID: <80133DE4-AFE6-4DCE-AC8F-663C89D4C975@arm.com> (raw)
In-Reply-To: <3ee41590-e8ca-84d6-3010-6e5dffe91df0@epam.com>
> On 24 Jul 2020, at 8:03 am, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> wrote:
>
>
> On 7/24/20 2:38 AM, Stefano Stabellini wrote:
>> + Jan, Andrew, Roger
>>
>> Please have a look at my comment on whether we should share the MMCFG
>> code below, feel free to ignore the rest :-)
>>
>>
>> On Thu, 23 Jul 2020, 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
>> What is this Change-Id property?
> Gerrit ;)
>>
>>
>>> 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
>>> 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.
> I think SPDX will fit better in any new code.
It will be good if community helps us to decide which license is best for new files.
>>> + *
>>> + * 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)
>>> +{
>>> + int rc;
>>> + uint32_t val = GENMASK(0, len * 8);
>>> +
>>> + 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 val;
>>> + }
>>> +
>>> + if ( unlikely(!bridge->ops->read) )
>>> + return val;
>>> +
>>> + rc = bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
>>> + 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)
>> This looks like a subset of xen/arch/x86/x86_64/mmconfig_64.c ?
>>
>> MMCFG is supposed to cover ECAM-compliant host bridges too, if I am not
>> mistaken. Is there any value in sharing the code with x86? It is OK if
>> we don't, but I would like to understand the reasoning.
>>
>>
>>
>>> +/*
>>> + * 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;
>>> + 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);
>>> +}
>
> The two functions above seem to deal with the same resources, e.g. cfg->win
>
> and map/unmap. Would it make sense to align those, something like
>
> s/pci_remap_cfgspace/pci_ecam_alloc and pci_ecam_alloc handles cfg->win?
>
> Or anything which makes them look init/fini style?
Ok will rename the function name.
>
>>> +
>>> +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 ) {
>>> + 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;
>
> So, if bus start is, for example, 0x10 then we'll end up with bus end at (0x10 + 0xff) > 0xff
>
> which doesn't seem to be what we want
This is to fix if in device tree bus-ranges property, bus end corresponds to more than 256 bus.
>
>>> + }
>>> +
>>> + /* Parse our PCI ecam register address*/
>>> + err = dt_device_get_address(dev, 0, &addr, &size);
>>> + if ( err )
>>> + goto err_exit;
>> Shouldn't we handle the possibility of multiple addresses? Is it
>> possible to have more than one range for an ECAM compliant host bridge?
>>
>>
>>> + 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);
>
> I am fine with "win", but can we think of something that tells us that
>
> "win" is actually ECAM base address, so one doesn't need to map "win" to "ECAM"
>
> while reading?
Ack. Will fix.
>
>>> + 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)
>>> +{
>>> + 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 */
> Do you expect multiple windows here as the comment says?
If going forward we want to support 32-bit we have to ioremap the config space for each bus individually, but as of only 64 bit is supported.
>>> + cfg = gen_pci_init(dev, ops)
>>> + if ( !cfg )
>>> + return -ENOMEM;
>> In case of errors the allocated bridge is not freed.
>>
>>
>>> + bridge->dt_node = dev;
>>> + bridge->sysdata = cfg;
>>> + bridge->ops = &ops->pci_ops;
>
> Can we have some sort of dummy ops so we don't have to check for ops != NULL every time
>
> we read/write config? Is it really possible that we have ops set to NULL after we have
>
> the development finished?
We don’t want to support host-bridges with dump ops. Proper ops has to be setup for the host-bridge to access the read/write.
Once we access the config space we are sure that ops is already setup so no need to check for NULL each time.
>
>>> +
>>> + 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;
>>> + }
>>> +
>>> + bridge->segment = (u16)segment;
>> My understanding is that a Linux pci-domain doesn't correspond exactly
>> to a PCI segment. See for instance:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03885.html
>>
>> Do we need to care about the difference? If we mean pci-domain here,
>> should we just call them as such instead of calling them "segments" in
>> Xen (if they are not segments)?
>
>>
>>
>>> + list_add_tail(&bridge->node, &pci_host_bridges);
>> It looks like &pci_host_bridges should be an ordered list, ordered by
>> segment number?
>
> Why? Do you expect bridge access in some specific order so ordered
>
> list will make it faster?
As I mentioned in earlier no need to have ordered list as PCI config space access is random.
>
>>
>>
>>> + 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;
>>> +}
>>> +/*
>>> + * 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)
>> I think the function is misnamed because reading the code below it looks
>> like it is not just returning the base config space address but also the
>> specific address we need to read/write (adding the device offset,
>> "where", and everything).
>>
>> Maybe pci_config_get_address or something like that?
>>
>>
>>> +{
>>> + struct pci_config_window *cfg = bridge->sysdata;
>
> I am a bit confused of the naming ;)
>
> We already have 2 maps: win -> ECAM base and now sysdata -> cfg.
>
> Can we please have that aligned somehow so it is easier to follow?
Ack.
>
>>> + unsigned int devfn_shift = cfg->ops->bus_shift - 8;
>
> We are not checking cfg->ops for NULL, so probably we do not want bridges
>
> with NULL ops as well?
Yes correct we don’t want host-bridge with NULL ops. Do you see any use case to have host-bridge with NULL ops?
>
>>> +
>>> + pci_sbdf_t sbdf_t = (pci_sbdf_t) sbdf ;
>>> +
>>> + unsigned int busn = sbdf_t.bus;
>>> + void __iomem *base;
>>> +
>>> + 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);
>>> + 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
>>
>>
>>> + 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);
>> please use a switch
>>
>>
>>> + 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 },
>> spurious blank line
>>
>>
>>> + { },
>>> +};
>>> +
>>> +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);
>> When adding new command line parameters please also add its
>> documentation (docs/misc/xen-command-line.pandoc) in the same patch,
>> unless this is meant to be just transient and we'll get removed before
>> the final commit of the series.
>>
>>
>>> +void __init pci_init(void)
>>> +{
>>> + /*
>>> + * Enable PCI when has been enabled explicitly (pci=on)
>>> + */
>>> + if ( !param_pci_enable)
>>> + goto disable;
>>> +
>>> + if ( acpi_disabled )
>>> + dt_pci_init();
>>> + else
>>> + acpi_pci_init();
>>> +
>>> +#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();
>> pci_init should probably be an initcall
>>
>>
>>> 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)
> Didn't we have a patch for that recently or talked about?
Not sure need to check.
>>> +#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,
>>> /* 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;
>>> +};
>> Are you actually using struct device in struct arch_pci_dev?
>> struct device is already part of struct dt_device_node and a pointer to
>> it is stored in bridge->dt_node.
>>
>>
>>> +#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;
>>> + 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);
>> I'd prefer if we could use explicitly-sized integers for "where" and
>> "size" too. Also, should they be unsigned rather than signed?
>>
>> Can we use pci_sbdf_t for the sbdf parameter?
>>
>>
>>> +};
>>> +
>>> +/*
>>> + * 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 *);
>>> +};
>> Although I realize that we are only targeting ECAM now, and the
>> implementation is based on ECAM, the interface doesn't seem to have
>> anything ECAM-specific in it. I would rename pci_ecam_ops to something
>> else, maybe simply "pci_ops".
>
> Yes, please, bear in mind that we are about to work with this code on
>
> non-ECAM HW from the very beginning, so this is something that we would
>
> like to see from the ground up
Ok. Will take care of that in next version of the patch.
>
>>
>>
>>> +/*
>>> + * 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;
>>> };
>>>
>>> -#endif /* __X86_PCI_H__ */
>>> +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus);
>>> +
>>> +int pci_host_common_probe(struct dt_device_node *dev,
>>> + struct pci_ecam_ops *ops);
>>> +
>>> +void pci_init(void);
>>> +
>>> +#else /*!CONFIG_ARM_PCI*/
>>> +struct arch_pci_dev { };
>>> +static inline void pci_init(void) { }
>>> +#endif /*!CONFIG_ARM_PCI*/
>>> +#endif /* __ARM_PCI_H__ */
>>> --
>>> 2.17.1
next prev parent reply other threads:[~2020-07-27 15:20 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 [this message]
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
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=80133DE4-AFE6-4DCE-AC8F-663C89D4C975@arm.com \
--to=rahul.singh@arm.com \
--cc=Bertrand.Marquis@arm.com \
--cc=Oleksandr_Andrushchenko@epam.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=jbeulich@suse.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).