| Message ID | 64ebd4ef614b36a5844c52426a4a6a4a23b1f087.1595511416.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI devices passthrough on Arm | expand |
+ 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? > 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. > + * > + * 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); > +} > + > +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; > + } > + > + /* 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); > + 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 */ > + 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; > + > + 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? > + 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; > + 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; > + > + 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) > +#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". > +/* > + * 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 >
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. >> + * >> + * 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? >> + >> +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 >> + } >> + >> + /* 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? >> + 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? >> + 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? >> + >> + 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? > > >> + 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? >> + 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? >> + >> + 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? >> +#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 > > >> +/* >> + * 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
Hi, On 24/07/2020 08:03, Oleksandr Andrushchenko wrote: >>> 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. While I would love to use SPDX in Xen, there was some push back in the past to use it. So the new code should use the full-blown copyright until there is an agreement to use it. >> >>> + 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? Access to the configure space will be pretty random. So I don't think ordering the list will make anything better. However, looking up for the bridge for every config spec access is pretty slow. When I was working on the PCI passthrough, I wanted to look whether it would be possible to have a pointer to the PCI host bridge passed in argument to the helpers (rather than the segment). Cheers,
Hi Rahul, On 23/07/2020 16:40, 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(-) As a general comment, I would suggest to split the patch in smaller chunk. This would help the review and also allow to provide more explanation on what is done. For instance, I think it is possible to a split looking like: - Add framework to access an hostbridge - Add support for ECAM - Add code to initialize the PCI subsystem There is also some small fixes in this code that probably can move in there own patches. Cheers,
Hi, On 24/07/2020 00:38, Stefano Stabellini wrote: >> + 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; >> + } >> + >> + 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)? So we definitely need a segment number in hand because this is what the admin will use to assign a PCI device to a guest. The segment number is just a value defined by the software. So as long as Linux and Xen agrees with the number, then we should be ok. Looking at the code, I think Linux is using 'segment' as 'domain'. So they should be interchangeably. I am not sure what other OS does though. Cheers,
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? > 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). > +{ > + 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. > + > + 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). > + 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. > + 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. > + > +/* > + * 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 > + 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 > +{ > + 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. > +} > +/* > + * 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. > +{ > + struct pci_config_window *cfg = bridge->sysdata; const > + 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. > + > + 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. > + 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. > + > + 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? > +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. > + > + 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? > + > +#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. Thanks, Roger.
On 24/07/2020 15:44, Roger Pau Monné wrote: >> 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? I don't believe any of the code in this series is very arm64 specific. I guess it was just only tested on arm64. So I would rather keep that under arm/pci. >> + >> + 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). I forgot about this patch :/. It would be good to revive it. Which acks are you missing? [...] >> +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 would be incorrect because this is a 32-bit value always in big endian. I don't think we have other typedef to show it is a 32-bit BE value, so __be32 is the best choice. [...] >> + >> + 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. > This is the common way we do it on Arm.... I would welcome any improvement, but I don't think this should be part of this work. Cheers,
On Fri, Jul 24, 2020 at 04:15:47PM +0100, Julien Grall wrote: > > > On 24/07/2020 15:44, Roger Pau Monné wrote: > > > 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? > I don't believe any of the code in this series is very arm64 specific. I > guess it was just only tested on arm64. So I would rather keep that under > arm/pci. Ack. Could the Kconfg be adjusted to not depend on ARM_64? Just stating it's only been tested on Arm64 would be enough IMO. > > > + > > > + 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). > > I forgot about this patch :/. It would be good to revive it. Which acks are > you missing? I only had an Ack from Jan, so I was missing Intel and AMD Acks, which would now only be Intel since AMD has been absorbed by the x86 maintainers. > [...] > > > > +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 would be incorrect because this is a 32-bit value always in big > endian. I don't think we have other typedef to show it is a 32-bit BE value, > so __be32 is the best choice. Oh, OK, so this is done to explicitly denote the endianness of a value on the type itself. > [...] > > > > + > > > + 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. > > > > This is the common way we do it on Arm.... I would welcome any improvement, > but I don't think this should be part of this work. Ack. In any case I think for ACPI PCI init will get called by acpi_mmcfg_init as part of acpi_boot_init, so I'm not sure there's much point in having something about ACPI added here, as it seems this will be DT only? Roger.
On Fri, Jul 24, 2020 at 05:29:05PM +0200, Roger Pau Monné wrote: > On Fri, Jul 24, 2020 at 04:15:47PM +0100, Julien Grall wrote: > > > > > > On 24/07/2020 15:44, Roger Pau Monné wrote: > > > > + > > > > + 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. > > > > > > > This is the common way we do it on Arm.... I would welcome any improvement, > > but I don't think this should be part of this work. > > Ack. In any case I think for ACPI PCI init will get called by > acpi_mmcfg_init as part of acpi_boot_init, so I'm not sure there's > much point in having something about ACPI added here, as it seems this > will be DT only? Sorry I got confused and acpi_boot_init is a x86 specific function, wrong context. If Arm is not using that path then maybe it makes sense to init PCI here for ACPI also. Roger.
On 24/07/2020 16:29, Roger Pau Monné wrote: > On Fri, Jul 24, 2020 at 04:15:47PM +0100, Julien Grall wrote: >> >> >> On 24/07/2020 15:44, Roger Pau Monné wrote: >>>> 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? >> I don't believe any of the code in this series is very arm64 specific. I >> guess it was just only tested on arm64. So I would rather keep that under >> arm/pci. > > Ack. Could the Kconfg be adjusted to not depend on ARM_64? Just > stating it's only been tested on Arm64 would be enough IMO. We already have an option to select PCI (see CONFIG_HAS_PCI). So I would prefer if we reuse it (possible renamed to CONFIG_PCI) rather than inventing one for Arm specific. Regarding the dependency, it will depend if it is possible to make it build easily on Arm32. If not, then we will need to keep the ARM_64. > >>>> + >>>> + 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). >> >> I forgot about this patch :/. It would be good to revive it. Which acks are >> you missing? > > I only had an Ack from Jan, so I was missing Intel and AMD Acks, which > would now only be Intel since AMD has been absorbed by the x86 > maintainers. Ok. So, it should be easier to get it acked now :). > >> [...] >> >>>> +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 would be incorrect because this is a 32-bit value always in big >> endian. I don't think we have other typedef to show it is a 32-bit BE value, >> so __be32 is the best choice. > > Oh, OK, so this is done to explicitly denote the endianness of a value > on the type itself. That's correct. On Linux, they use sparse to then check the BE and LE fields are not mixed together. We don't have that on Xen, but at least this makes more obvious what we are using. > >> [...] >> >>>> + >>>> + 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. >>> >> >> This is the common way we do it on Arm.... I would welcome any improvement, >> but I don't think this should be part of this work. > > Ack. In any case I think for ACPI PCI init will get called by > acpi_mmcfg_init as part of acpi_boot_init, so I'm not sure there's > much point in having something about ACPI added here, as it seems this > will be DT only? acpi_boot_init() does not exist on Arm. Looking at x86, acpi_mmcfg_init() is not event called from that function. In general, I would prefer if each subsystem takes care of initialization itself. This makes easier to figure out the difference between ACPI and DT. FWIW, this is inline with majority of the Arm code. Cheers,
On 24.07.2020 17:15, Julien Grall wrote: > On 24/07/2020 15:44, Roger Pau Monné wrote: >>> + >>> + 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). > > I forgot about this patch :/. It would be good to revive it. Which acks > are you missing? It wasn't so much missing acks, but a controversy. And that not so much about switching existing users, but whether to indeed derive this from %p (which I continue to consider inefficient). Jan
Hi Jan, On 24/07/2020 17:01, Jan Beulich wrote: > On 24.07.2020 17:15, Julien Grall wrote: >> On 24/07/2020 15:44, Roger Pau Monné wrote: >>>> + >>>> + 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). >> >> I forgot about this patch :/. It would be good to revive it. Which acks >> are you missing? > > It wasn't so much missing acks, but a controversy. And that not so much > about switching existing users, but whether to indeed derive this from > %p (which I continue to consider inefficient). Looking at the thread, I can see you (relunctantly) acked any components that you are the sole maintainers. Kevin gave his acked for the vtd code and I gave it mine for the common code. I would suggest to not rehash the argument unless another maintainer agree with your position. It loosk like to me the next step is for Roger (or someone else) to resend the patch so we could collect the missing ack (I think there is only one missing from Andrew). Cheers,
On Fri, 24 Jul 2020, Julien Grall wrote: > On 24/07/2020 00:38, Stefano Stabellini wrote: > > > + 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; > > > + } > > > + > > > + 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)? > > So we definitely need a segment number in hand because this is what the admin > will use to assign a PCI device to a guest. Yeah > The segment number is just a value defined by the software. So as long as > Linux and Xen agrees with the number, then we should be ok. As far as I understand a Linux "domain" (linux,pci-domain in device tree) is a value defined by the software. The PCI segment has a definition in the PCI spec and it is returned by _SEG on ACPI systems. The link above suggests that a Linux domain corresponds to (_SEG, _BBN) where _SEG is the segment and _BBN is the "Bus Number". I just would like to be precise with the terminology: if we are talking about domains in the linux sense of the word, as it looks like we are doing, then let's call them domain instead of segments which seem to have a different definition.
On Fri, 24 Jul 2020, Julien Grall wrote: > > > > + 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? > > Access to the configure space will be pretty random. So I don't think ordering > the list will make anything better. > > However, looking up for the bridge for every config spec access is pretty > slow. When I was working on the PCI passthrough, I wanted to look whether it > would be possible to have a pointer to the PCI host bridge passed in argument > to the helpers (rather than the segment). I was suggesting ordering the list because it is a rather cheap optimization: it is easy to implement and it typically leads to decent improvements (although it varies on a case by case basis). Something more sophisticated as you mention here would be even better.
On 24/07/2020 18:41, Stefano Stabellini wrote: > On Fri, 24 Jul 2020, Julien Grall wrote: >> On 24/07/2020 00:38, Stefano Stabellini wrote: >> The segment number is just a value defined by the software. So as long as >> Linux and Xen agrees with the number, then we should be ok. > > As far as I understand a Linux "domain" (linux,pci-domain in device > tree) is a value defined by the software. The PCI segment has a > definition in the PCI spec and it is returned by _SEG on ACPI systems. > > The link above suggests that a Linux domain corresponds to (_SEG, > _BBN) where _SEG is the segment and _BBN is the "Bus Number". > > I just would like to be precise with the terminology: if we are talking > about domains in the linux sense of the word, as it looks like we are > doing, then let's call them domain instead of segments which seem to > have a different definition. You seem to argue on the name but this doesn't resolve the underlying problem. Indeed, all our external interfaces are expecting a segment number. If they are not equal, then I fail to see why it would be useful to have this value in Xen. In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so Dom0 and Xen can synchronize on the segment number. Cheers,
On Fri, 24 Jul 2020, Julien Grall wrote: > On 24/07/2020 18:41, Stefano Stabellini wrote: > > On Fri, 24 Jul 2020, Julien Grall wrote: > > > On 24/07/2020 00:38, Stefano Stabellini wrote: > > > The segment number is just a value defined by the software. So as long as > > > Linux and Xen agrees with the number, then we should be ok. > > > > As far as I understand a Linux "domain" (linux,pci-domain in device > > tree) is a value defined by the software. The PCI segment has a > > definition in the PCI spec and it is returned by _SEG on ACPI systems. > > > > The link above suggests that a Linux domain corresponds to (_SEG, > > _BBN) where _SEG is the segment and _BBN is the "Bus Number". > > > > I just would like to be precise with the terminology: if we are talking > > about domains in the linux sense of the word, as it looks like we are > > doing, then let's call them domain instead of segments which seem to > > have a different definition. > > You seem to argue on the name but this doesn't resolve the underlying problem. > Indeed, all our external interfaces are expecting a segment number. Yes, you are right, that is a bigger problem. > If they are not equal, then I fail to see why it would be useful to have this > value in Xen. I think that's because the domain is actually more convenient to use because a segment can span multiple PCI host bridges. So my understanding is that a segment alone is not sufficient to identify a host bridge. From a software implementation point of view it would be better to use domains. > In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so > Dom0 and Xen can synchronize on the segment number. I was hoping we could write down the assumption somewhere that for the cases we care about domain == segment, and error out if it is not the case.
On Fri, 24 Jul 2020 at 19:32, Stefano Stabellini <sstabellini@kernel.org> wrote: > > If they are not equal, then I fail to see why it would be useful to have this > > value in Xen. > > I think that's because the domain is actually more convenient to use > because a segment can span multiple PCI host bridges. So my > understanding is that a segment alone is not sufficient to identify a > host bridge. From a software implementation point of view it would be > better to use domains. AFAICT, this would be a matter of one check vs two checks in Xen :). But... looking at Linux, they will also use domain == segment for ACPI (see [1]). So, I think, they still have to use (domain, bus) to do the lookup. > > > In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so > > Dom0 and Xen can synchronize on the segment number. > > I was hoping we could write down the assumption somewhere that for the > cases we care about domain == segment, and error out if it is not the > case. Given that we have only the domain in hand, how would you enforce that? From this discussion, it also looks like there is a mismatch between the implementation and the understanding on QEMU devel. So I am a bit concerned that this is not stable and may change in future Linux version. IOW, we are know tying Xen to Linux. So could we implement PHYSDEVOP_pci_mmcfg_reserved *or* introduce a new property that really represent the segment? Cheers, [1] https://elixir.bootlin.com/linux/latest/source/arch/arm64/kernel/pci.c#L74 Cheers,
On Fri, 24 Jul 2020, Julien Grall wrote: > On Fri, 24 Jul 2020 at 19:32, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > If they are not equal, then I fail to see why it would be useful to have this > > > value in Xen. > > > > I think that's because the domain is actually more convenient to use > > because a segment can span multiple PCI host bridges. So my > > understanding is that a segment alone is not sufficient to identify a > > host bridge. From a software implementation point of view it would be > > better to use domains. > > AFAICT, this would be a matter of one check vs two checks in Xen :). > But... looking at Linux, they will also use domain == segment for ACPI > (see [1]). So, I think, they still have to use (domain, bus) to do the lookup. > > > > In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so > > > Dom0 and Xen can synchronize on the segment number. > > > > I was hoping we could write down the assumption somewhere that for the > > cases we care about domain == segment, and error out if it is not the > > case. > > Given that we have only the domain in hand, how would you enforce that? > > >From this discussion, it also looks like there is a mismatch between the > implementation and the understanding on QEMU devel. So I am a bit > concerned that this is not stable and may change in future Linux version. > > IOW, we are know tying Xen to Linux. So could we implement > PHYSDEVOP_pci_mmcfg_reserved *or* introduce a new property that > really represent the segment? I don't think we are tying Xen to Linux. Rob has already said that linux,pci-domain is basically a generic device tree property. And if we look at https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf "PCI domain" is described and seems to match the Linux definition. I do think we need to understand the definitions and the differences. Reading online [1][2] it looks like a Linux PCI domain matches a "PCI Segment Group Number" in PCI Express which is probably why Linux is making the assumption that it is making. So maybe it is OK to use domains == segments, but we need to verify this in the specs and also clarify the terminology we use in a doc for our own sanity -- I am hoping that Rahul can come up with a good explanation on the topic :-) [1] https://stackoverflow.com/questions/49050847/how-is-pci-segmentdomain-related-to-multiple-host-bridgesor-root-bridges [2] https://wiki.osdev.org/PCI_Express
On Sat, 25 Jul 2020 at 00:46, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Fri, 24 Jul 2020, Julien Grall wrote: > > On Fri, 24 Jul 2020 at 19:32, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > If they are not equal, then I fail to see why it would be useful to have this > > > > value in Xen. > > > > > > I think that's because the domain is actually more convenient to use > > > because a segment can span multiple PCI host bridges. So my > > > understanding is that a segment alone is not sufficient to identify a > > > host bridge. From a software implementation point of view it would be > > > better to use domains. > > > > AFAICT, this would be a matter of one check vs two checks in Xen :). > > But... looking at Linux, they will also use domain == segment for ACPI > > (see [1]). So, I think, they still have to use (domain, bus) to do the lookup. > > > > > > In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so > > > > Dom0 and Xen can synchronize on the segment number. > > > > > > I was hoping we could write down the assumption somewhere that for the > > > cases we care about domain == segment, and error out if it is not the > > > case. > > > > Given that we have only the domain in hand, how would you enforce that? > > > > >From this discussion, it also looks like there is a mismatch between the > > implementation and the understanding on QEMU devel. So I am a bit > > concerned that this is not stable and may change in future Linux version. > > > > IOW, we are know tying Xen to Linux. So could we implement > > PHYSDEVOP_pci_mmcfg_reserved *or* introduce a new property that > > really represent the segment? > > I don't think we are tying Xen to Linux. Rob has already said that > linux,pci-domain is basically a generic device tree property. My concern is not so much the name of the property, but the definition of it. AFAICT, from this thread there can be two interpretation: - domain == segment - domain == (segment, bus) > And if we > look at https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf > "PCI domain" is described and seems to match the Linux definition. > > I do think we need to understand the definitions and the differences. +1 > Reading online [1][2] it looks like a Linux PCI domain matches a "PCI > Segment Group Number" in PCI Express which is probably why Linux is > making the assumption that it is making. > > So maybe it is OK to use domains == segments, but we need to verify this > in the specs and also clarify the terminology we use in a doc for our > own sanity -- I am hoping that Rahul can come up with a good > explanation on the topic :-) I am a bit confused.... You were the one arguing that domain == (segment, bus) in this thread. So may I ask why the interpretation wouldn't be valid anymore? Cheers, > [1] https://stackoverflow.com/questions/49050847/how-is-pci-segmentdomain-related-to-multiple-host-bridgesor-root-bridges > [2] https://wiki.osdev.org/PCI_Express
On 25.07.2020 01:46, Stefano Stabellini wrote: > On Fri, 24 Jul 2020, Julien Grall wrote: >> On Fri, 24 Jul 2020 at 19:32, Stefano Stabellini <sstabellini@kernel.org> wrote: >>>> If they are not equal, then I fail to see why it would be useful to have this >>>> value in Xen. >>> >>> I think that's because the domain is actually more convenient to use >>> because a segment can span multiple PCI host bridges. So my >>> understanding is that a segment alone is not sufficient to identify a >>> host bridge. From a software implementation point of view it would be >>> better to use domains. >> >> AFAICT, this would be a matter of one check vs two checks in Xen :). >> But... looking at Linux, they will also use domain == segment for ACPI >> (see [1]). So, I think, they still have to use (domain, bus) to do the lookup. >> >>>> In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so >>>> Dom0 and Xen can synchronize on the segment number. >>> >>> I was hoping we could write down the assumption somewhere that for the >>> cases we care about domain == segment, and error out if it is not the >>> case. >> >> Given that we have only the domain in hand, how would you enforce that? >> >> >From this discussion, it also looks like there is a mismatch between the >> implementation and the understanding on QEMU devel. So I am a bit >> concerned that this is not stable and may change in future Linux version. >> >> IOW, we are know tying Xen to Linux. So could we implement >> PHYSDEVOP_pci_mmcfg_reserved *or* introduce a new property that >> really represent the segment? > > I don't think we are tying Xen to Linux. Rob has already said that > linux,pci-domain is basically a generic device tree property. And if we > look at https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf > "PCI domain" is described and seems to match the Linux definition. > > I do think we need to understand the definitions and the differences. > Reading online [1][2] it looks like a Linux PCI domain matches a "PCI > Segment Group Number" in PCI Express which is probably why Linux is > making the assumption that it is making. If I may, I'd like to put the question a little differently, in the hope for me to understand the actual issue here: On the x86 side, by way of using ACPI, Linux and Xen "naturally" agree on segment numbering (as far as normal devices go; Intel's Volume Management Device concept still needs accommodating so that it would work with Xen). This includes the multiple host bridges case then naturally. How is the Device Tree model different from ACPI? Jan
On Fri, Jul 24, 2020 at 05:54:20PM +0100, Julien Grall wrote: > Hi Jan, > > On 24/07/2020 17:01, Jan Beulich wrote: > > On 24.07.2020 17:15, Julien Grall wrote: > > > On 24/07/2020 15:44, Roger Pau Monné wrote: > > > > > + > > > > > + 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). > > > > > > I forgot about this patch :/. It would be good to revive it. Which acks > > > are you missing? > > > > It wasn't so much missing acks, but a controversy. And that not so much > > about switching existing users, but whether to indeed derive this from > > %p (which I continue to consider inefficient). > > Looking at the thread, I can see you (relunctantly) acked any components > that you are the sole maintainers. Kevin gave his acked for the vtd code and > I gave it mine for the common code. > > I would suggest to not rehash the argument unless another maintainer agree > with your position. It loosk like to me the next step is for Roger (or > someone else) to resend the patch so we could collect the missing ack (I > think there is only one missing from Andrew). I've rebased and sent the updated patch with the collected Acks. Roger.
On Sat, Jul 25, 2020 at 10:59:50AM +0100, Julien Grall wrote: > On Sat, 25 Jul 2020 at 00:46, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > On Fri, 24 Jul 2020, Julien Grall wrote: > > > On Fri, 24 Jul 2020 at 19:32, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > > If they are not equal, then I fail to see why it would be useful to have this > > > > > value in Xen. > > > > > > > > I think that's because the domain is actually more convenient to use > > > > because a segment can span multiple PCI host bridges. So my > > > > understanding is that a segment alone is not sufficient to identify a > > > > host bridge. From a software implementation point of view it would be > > > > better to use domains. > > > > > > AFAICT, this would be a matter of one check vs two checks in Xen :). > > > But... looking at Linux, they will also use domain == segment for ACPI > > > (see [1]). So, I think, they still have to use (domain, bus) to do the lookup. You have to use the (segment, bus) tuple when doing a lookup because MMCFG regions on ACPI are defined for a segment and a bus range, you can have a MMCFG region that covers segment 0 bus [0, 20) and another MMCFG region that covers segment 0 bus [20, 255], and those will use different addresses in the MMIO space. > > > > > In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so > > > > > Dom0 and Xen can synchronize on the segment number. > > > > > > > > I was hoping we could write down the assumption somewhere that for the > > > > cases we care about domain == segment, and error out if it is not the > > > > case. > > > > > > Given that we have only the domain in hand, how would you enforce that? > > > > > > >From this discussion, it also looks like there is a mismatch between the > > > implementation and the understanding on QEMU devel. So I am a bit > > > concerned that this is not stable and may change in future Linux version. > > > > > > IOW, we are know tying Xen to Linux. So could we implement > > > PHYSDEVOP_pci_mmcfg_reserved *or* introduce a new property that > > > really represent the segment? > > > > I don't think we are tying Xen to Linux. Rob has already said that > > linux,pci-domain is basically a generic device tree property. > > My concern is not so much the name of the property, but the definition of it. > > AFAICT, from this thread there can be two interpretation: > - domain == segment > - domain == (segment, bus) I think domain is just an alias for segment, the difference seems to be that when using DT all bridges get a different segment (or domain) number, and thus you will always end up starting numbering at bus 0 for each bridge? Ideally you would need a way to specify the segment and start/end bus numbers of each bridge, if not you cannot match what ACPI does. Albeit it might be fine as long as the OS and Xen agree on the segments and bus numbers that belong to each bridge (and thus each ECAM region). Roger.
> On 24 Jul 2020, at 12:38 am, Stefano Stabellini <sstabellini@kernel.org> 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? Will remove this in next patch series. > > >> 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. >> + * >> + * 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. We have just started to investigate the ACPI support for PCI within XEN on ARM. Once we have understanding how we can support the ACPI we will have a look x86 MMCFG access and will check if we can have a common interface to access the configuration space. > > > >> +/* >> + * 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); >> +} >> + >> +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; >> + } >> + >> + /* 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? As of now we are supporting the PCI controllers that have "pci-host-ecam-generic” compatible node in device tree binding.As per my understanding controller that have "pci-host-ecam-generic” compatible property have only one reg property value at index 0. Yes there are PCI controllers that support ECAM and have more than one "reg" property values.As per my understanding once we support other controller in XEN at that time we have to handle the possibility of multiple addresses and that code is specific to that controller . > > >> + 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) >> +{ >> + 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; > > In case of errors the allocated bridge is not freed. > Will fix this in next version. > >> + 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; >> + } >> + >> + 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)? > As per my understanding linux domain number is an alias to segment number.Device tree binding uses the property domain number and ACPI uses the property segment number. We have just started the investigation on ACPI, once we have more information about MMCFG ACPI table how it works will share the information and will update the patches and design doc accordingly. > >> + list_add_tail(&bridge->node, &pci_host_bridges); > > It looks like &pci_host_bridges should be an ordered list, ordered by > segment number? > As Julien also mentioned access to the PCI config space is random there is no need to have an ordered list. > >> + 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? OK yes will rename the function name. > > >> +{ >> + struct pci_config_window *cfg = bridge->sysdata; >> + 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; >> + >> + 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 Ok. > > >> + 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 Ok will fix this. > > >> + { }, >> +}; >> + >> +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. > Ok yes will add it to the documentation. > >> +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. OK. > > >> 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, >> /* 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. Will be using this going forward once we have full PCI passthrough support. > > >> +#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? Ok will fix in next version. > > >> +}; >> + >> +/* >> + * 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". Ok will have a look and modify accordingly. > > >> +/* >> + * 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
> 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
> On 24 Jul 2020, at 9:05 am, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 24/07/2020 08:03, Oleksandr Andrushchenko wrote: >>>> 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. > > While I would love to use SPDX in Xen, there was some push back in the past to use it. So the new code should use the full-blown copyright until there is an agreement to use it. Ack. We will use the copyright until we will get the confirmation from the community to use the SPDX in XEN. > >>> >>>> + 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? > > Access to the configure space will be pretty random. So I don't think ordering the list will make anything better. > > However, looking up for the bridge for every config spec access is pretty slow. When I was working on the PCI passthrough, I wanted to look whether it would be possible to have a pointer to the PCI host bridge passed in argument to the helpers (rather than the segment). Yes true every config space access has to scan the host-bridges list to find the corresponding host-bridge. I will try to find better solution so that no need to scan the host-bridge every time for read/write access. > Cheers, > > -- > Julien Grall
> On 24 Jul 2020, at 9:23 am, Julien Grall <julien@xen.org> wrote: > > Hi Rahul, > > On 23/07/2020 16:40, 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(-) > > As a general comment, I would suggest to split the patch in smaller chunk. This would help the review and also allow to provide more explanation on what is done. Ok I will split the patches in next version of the patch series. > > For instance, I think it is possible to a split looking like: > - Add framework to access an hostbridge > - Add support for ECAM > - Add code to initialize the PCI subsystem > > There is also some small fixes in this code that probably can move in there own patches. Ack. > > Cheers, > > -- > Julien Grall
On Mon, 27 Jul 2020, Roger Pau Monné wrote: > On Sat, Jul 25, 2020 at 10:59:50AM +0100, Julien Grall wrote: > > On Sat, 25 Jul 2020 at 00:46, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > > > On Fri, 24 Jul 2020, Julien Grall wrote: > > > > On Fri, 24 Jul 2020 at 19:32, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > > > If they are not equal, then I fail to see why it would be useful to have this > > > > > > value in Xen. > > > > > > > > > > I think that's because the domain is actually more convenient to use > > > > > because a segment can span multiple PCI host bridges. So my > > > > > understanding is that a segment alone is not sufficient to identify a > > > > > host bridge. From a software implementation point of view it would be > > > > > better to use domains. > > > > > > > > AFAICT, this would be a matter of one check vs two checks in Xen :). > > > > But... looking at Linux, they will also use domain == segment for ACPI > > > > (see [1]). So, I think, they still have to use (domain, bus) to do the lookup. > > You have to use the (segment, bus) tuple when doing a lookup because > MMCFG regions on ACPI are defined for a segment and a bus range, you > can have a MMCFG region that covers segment 0 bus [0, 20) and another > MMCFG region that covers segment 0 bus [20, 255], and those will use > different addresses in the MMIO space. Thanks for the clarification! > > > > > > In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so > > > > > > Dom0 and Xen can synchronize on the segment number. > > > > > > > > > > I was hoping we could write down the assumption somewhere that for the > > > > > cases we care about domain == segment, and error out if it is not the > > > > > case. > > > > > > > > Given that we have only the domain in hand, how would you enforce that? > > > > > > > > >From this discussion, it also looks like there is a mismatch between the > > > > implementation and the understanding on QEMU devel. So I am a bit > > > > concerned that this is not stable and may change in future Linux version. > > > > > > > > IOW, we are know tying Xen to Linux. So could we implement > > > > PHYSDEVOP_pci_mmcfg_reserved *or* introduce a new property that > > > > really represent the segment? > > > > > > I don't think we are tying Xen to Linux. Rob has already said that > > > linux,pci-domain is basically a generic device tree property. > > > > My concern is not so much the name of the property, but the definition of it. > > > > AFAICT, from this thread there can be two interpretation: > > - domain == segment > > - domain == (segment, bus) > > I think domain is just an alias for segment, the difference seems to > be that when using DT all bridges get a different segment (or domain) > number, and thus you will always end up starting numbering at bus 0 > for each bridge? > > Ideally you would need a way to specify the segment and start/end bus > numbers of each bridge, if not you cannot match what ACPI does. Albeit > it might be fine as long as the OS and Xen agree on the segments and > bus numbers that belong to each bridge (and thus each ECAM region). That is what I thought and it is why I was asking to clarify the naming and/or writing a document to explain the assumptions, if any. Then after Julien's email I followed up in the Linux codebase and clearly there is a different assumption baked in the Linux kernel for architectures that have CONFIG_PCI_DOMAINS enabled (including ARM64). The assumption is that segment == domain == unique host bridge. It looks like it is coming from IEEE Std 1275-1994 but I am not certain. In fact, it seems that ACPI MCFG and IEEE Std 1275-1994 don't exactly match. So I am starting to think that domain == segment for IEEE Std 1275-1994 compliant device tree based systems.
> 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.
On Tue, Jul 28, 2020 at 08:06:17AM +0000, Rahul Singh wrote: > > > > 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: > >> + > >> + 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. I've posted an updated version to the list yesterday: https://lore.kernel.org/xen-devel/20200727103136.53343-1-roger.pau@citrix.com/ > >> 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://secure-web.cisco.com/1YIShdE6aU0WIMsRjhIYYGdRo_H_wyOVeLqk0ITHI7i_FMIXuMEmV9Y6lR76qCBB4XHss0ba_WMfQa0mLGD37bM1S3AFMBVC1WUEF0LmbURPWld2kU2zuyOamUxqnYwy4ZAv4N2EUJy7HCgDYn2s183YCQ-5YBc98vH8TO-tfBgEShkKVzAm23AgzIqikuKN_BIhcQhAbsjypr9ffeNZDsSX2jC8ClqegEPXFsCWJldrASGgWoR16rMjI-INfPNy55m9nGP5UmHIYWBmTpnHLjXtqCkruENixh20vPWIUROhcjjtzVBrR8d-Q5HnJy0hSR97WdlUSAryfxeH-8VxcPw/http%3A%2F%2Fwww.gnu.org%2Flicenses%2F>. > >> + */ > >> > >> +#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. Please introduce it when it's required then. Thanks, Roger.
On Mon, Jul 27, 2020 at 05:06:25PM -0700, Stefano Stabellini wrote: > On Mon, 27 Jul 2020, Roger Pau Monné wrote: > > On Sat, Jul 25, 2020 at 10:59:50AM +0100, Julien Grall wrote: > > > On Sat, 25 Jul 2020 at 00:46, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > > > > > On Fri, 24 Jul 2020, Julien Grall wrote: > > > > > On Fri, 24 Jul 2020 at 19:32, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > > > > If they are not equal, then I fail to see why it would be useful to have this > > > > > > > value in Xen. > > > > > > > > > > > > I think that's because the domain is actually more convenient to use > > > > > > because a segment can span multiple PCI host bridges. So my > > > > > > understanding is that a segment alone is not sufficient to identify a > > > > > > host bridge. From a software implementation point of view it would be > > > > > > better to use domains. > > > > > > > > > > AFAICT, this would be a matter of one check vs two checks in Xen :). > > > > > But... looking at Linux, they will also use domain == segment for ACPI > > > > > (see [1]). So, I think, they still have to use (domain, bus) to do the lookup. > > > > You have to use the (segment, bus) tuple when doing a lookup because > > MMCFG regions on ACPI are defined for a segment and a bus range, you > > can have a MMCFG region that covers segment 0 bus [0, 20) and another > > MMCFG region that covers segment 0 bus [20, 255], and those will use > > different addresses in the MMIO space. > > Thanks for the clarification! > > > > > > > > > In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so > > > > > > > Dom0 and Xen can synchronize on the segment number. > > > > > > > > > > > > I was hoping we could write down the assumption somewhere that for the > > > > > > cases we care about domain == segment, and error out if it is not the > > > > > > case. > > > > > > > > > > Given that we have only the domain in hand, how would you enforce that? > > > > > > > > > > >From this discussion, it also looks like there is a mismatch between the > > > > > implementation and the understanding on QEMU devel. So I am a bit > > > > > concerned that this is not stable and may change in future Linux version. > > > > > > > > > > IOW, we are know tying Xen to Linux. So could we implement > > > > > PHYSDEVOP_pci_mmcfg_reserved *or* introduce a new property that > > > > > really represent the segment? > > > > > > > > I don't think we are tying Xen to Linux. Rob has already said that > > > > linux,pci-domain is basically a generic device tree property. > > > > > > My concern is not so much the name of the property, but the definition of it. > > > > > > AFAICT, from this thread there can be two interpretation: > > > - domain == segment > > > - domain == (segment, bus) > > > > I think domain is just an alias for segment, the difference seems to > > be that when using DT all bridges get a different segment (or domain) > > number, and thus you will always end up starting numbering at bus 0 > > for each bridge? > > > > Ideally you would need a way to specify the segment and start/end bus > > numbers of each bridge, if not you cannot match what ACPI does. Albeit > > it might be fine as long as the OS and Xen agree on the segments and > > bus numbers that belong to each bridge (and thus each ECAM region). > > That is what I thought and it is why I was asking to clarify the naming > and/or writing a document to explain the assumptions, if any. > > Then after Julien's email I followed up in the Linux codebase and > clearly there is a different assumption baked in the Linux kernel for > architectures that have CONFIG_PCI_DOMAINS enabled (including ARM64). > > The assumption is that segment == domain == unique host bridge. It > looks like it is coming from IEEE Std 1275-1994 but I am not certain. > In fact, it seems that ACPI MCFG and IEEE Std 1275-1994 don't exactly > match. So I am starting to think that domain == segment for IEEE Std > 1275-1994 compliant device tree based systems. I don't think the ACPI MCFG spec contains the notion of bridges, it just describes ECAM (or MMCFG) regions, but those could be made up by concatenating different bridge ECAM regions by the firmware itself, so you could AFAICT end up with multiple bridges being aggregated into a single ECAM region, and thus using the same segment number, which seems not possible with the DT spec, where each bridge must get a different segment number? If you could assign both a segment number and a bus start and end values to a bridge then I think it would be kind of equivalent to ACPI MCFG. I assume we would never support a system where Xen is getting the hardware description from a DT and the hardware domain is using ACPI (or the other way around)? If so, I don't think we care that enumeration when using DT is different than when using ACPI, as we can only guarantee consistency when both Xen and the hardware domain use the same source for the hardware description. If when using DT each bridge has a unique segment number that's fine as long as Xen and the OS agree to not change such values. Roger.
On Tue, 28 Jul 2020, Roger Pau Monné wrote: > On Mon, Jul 27, 2020 at 05:06:25PM -0700, Stefano Stabellini wrote: > > On Mon, 27 Jul 2020, Roger Pau Monné wrote: > > > On Sat, Jul 25, 2020 at 10:59:50AM +0100, Julien Grall wrote: > > > > On Sat, 25 Jul 2020 at 00:46, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > > > > > > > On Fri, 24 Jul 2020, Julien Grall wrote: > > > > > > On Fri, 24 Jul 2020 at 19:32, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > > > > > If they are not equal, then I fail to see why it would be useful to have this > > > > > > > > value in Xen. > > > > > > > > > > > > > > I think that's because the domain is actually more convenient to use > > > > > > > because a segment can span multiple PCI host bridges. So my > > > > > > > understanding is that a segment alone is not sufficient to identify a > > > > > > > host bridge. From a software implementation point of view it would be > > > > > > > better to use domains. > > > > > > > > > > > > AFAICT, this would be a matter of one check vs two checks in Xen :). > > > > > > But... looking at Linux, they will also use domain == segment for ACPI > > > > > > (see [1]). So, I think, they still have to use (domain, bus) to do the lookup. > > > > > > You have to use the (segment, bus) tuple when doing a lookup because > > > MMCFG regions on ACPI are defined for a segment and a bus range, you > > > can have a MMCFG region that covers segment 0 bus [0, 20) and another > > > MMCFG region that covers segment 0 bus [20, 255], and those will use > > > different addresses in the MMIO space. > > > > Thanks for the clarification! > > > > > > > > > > > > In which case, we need to use PHYSDEVOP_pci_mmcfg_reserved so > > > > > > > > Dom0 and Xen can synchronize on the segment number. > > > > > > > > > > > > > > I was hoping we could write down the assumption somewhere that for the > > > > > > > cases we care about domain == segment, and error out if it is not the > > > > > > > case. > > > > > > > > > > > > Given that we have only the domain in hand, how would you enforce that? > > > > > > > > > > > > >From this discussion, it also looks like there is a mismatch between the > > > > > > implementation and the understanding on QEMU devel. So I am a bit > > > > > > concerned that this is not stable and may change in future Linux version. > > > > > > > > > > > > IOW, we are know tying Xen to Linux. So could we implement > > > > > > PHYSDEVOP_pci_mmcfg_reserved *or* introduce a new property that > > > > > > really represent the segment? > > > > > > > > > > I don't think we are tying Xen to Linux. Rob has already said that > > > > > linux,pci-domain is basically a generic device tree property. > > > > > > > > My concern is not so much the name of the property, but the definition of it. > > > > > > > > AFAICT, from this thread there can be two interpretation: > > > > - domain == segment > > > > - domain == (segment, bus) > > > > > > I think domain is just an alias for segment, the difference seems to > > > be that when using DT all bridges get a different segment (or domain) > > > number, and thus you will always end up starting numbering at bus 0 > > > for each bridge? > > > > > > Ideally you would need a way to specify the segment and start/end bus > > > numbers of each bridge, if not you cannot match what ACPI does. Albeit > > > it might be fine as long as the OS and Xen agree on the segments and > > > bus numbers that belong to each bridge (and thus each ECAM region). > > > > That is what I thought and it is why I was asking to clarify the naming > > and/or writing a document to explain the assumptions, if any. > > > > Then after Julien's email I followed up in the Linux codebase and > > clearly there is a different assumption baked in the Linux kernel for > > architectures that have CONFIG_PCI_DOMAINS enabled (including ARM64). > > > > The assumption is that segment == domain == unique host bridge. It > > looks like it is coming from IEEE Std 1275-1994 but I am not certain. > > In fact, it seems that ACPI MCFG and IEEE Std 1275-1994 don't exactly > > match. So I am starting to think that domain == segment for IEEE Std > > 1275-1994 compliant device tree based systems. > > I don't think the ACPI MCFG spec contains the notion of bridges, it > just describes ECAM (or MMCFG) regions, but those could be made up by > concatenating different bridge ECAM regions by the firmware itself, so > you could AFAICT end up with multiple bridges being aggregated into a > single ECAM region, and thus using the same segment number, which > seems not possible with the DT spec, where each bridge must get a > different segment number? Yes, that's my understanding too > If you could assign both a segment number and a bus start and end > values to a bridge then I think it would be kind of equivalent to ACPI > MCFG. > > I assume we would never support a system where Xen is getting the > hardware description from a DT and the hardware domain is using ACPI > (or the other way around)? Yeah, I think it is a good assumption > If so, I don't think we care that enumeration when using DT is > different than when using ACPI, as we can only guarantee consistency > when both Xen and the hardware domain use the same source for the > hardware description. > > If when using DT each bridge has a unique segment number that's fine > as long as Xen and the OS agree to not change such values. I agree
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. + * + * 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) + +/* + * 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); +} + +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; + } + + /* 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) +{ + 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; + + 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; + } + + 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; +} +/* + * 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) +{ + struct pci_config_window *cfg = bridge->sysdata; + 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; + + 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); + + 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); + +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(); + 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, /* 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; +}; + +#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); +}; + +/* + * 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; }; -#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__ */
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