XEN during boot will read the PCI device tree node “reg” property
and will map the PCI config space to the XEN memory.
As of now "pci-host-ecam-generic" compatible board is supported.
"linux,pci-domain" device tree property assigns a fixed PCI domain
number to a host bridge, otherwise an unstable (across boots) unique
number will be assigned by Linux.This property has to be in sync with
XEN to access the PCI devices.
XEN will read the “linux,pci-domain” property from the device tree node
and configure the host bridge segment number accordingly. If this
property is not available XEN will allocate the unique segment number
to the host bridge.
dt_get_pci_domain_nr(..) and dt_pci_bus_find_domain_nr(..) are directly
imported from the Linux source tree.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
xen/arch/arm/pci/Makefile | 2 +
xen/arch/arm/pci/pci-host-common.c | 261 ++++++++++++++++++++++++++++
xen/arch/arm/pci/pci-host-generic.c | 55 ++++++
xen/include/asm-arm/pci.h | 28 +++
4 files changed, 346 insertions(+)
create mode 100644 xen/arch/arm/pci/pci-host-common.c
create mode 100644 xen/arch/arm/pci/pci-host-generic.c
diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
index a9ee0b9b44..f3d97f859e 100644
--- a/xen/arch/arm/pci/Makefile
+++ b/xen/arch/arm/pci/Makefile
@@ -1,2 +1,4 @@
obj-y += pci.o
obj-y += pci-access.o
+obj-y += pci-host-generic.o
+obj-y += pci-host-common.o
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..9dd9b02271
--- /dev/null
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -0,0 +1,261 @@
+/*
+ * Copyright (C) 2021 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/sched.h>
+#include <xen/vmap.h>
+
+/*
+ * List for all the pci host bridges.
+ */
+
+static LIST_HEAD(pci_host_bridges);
+
+static atomic_t domain_nr = ATOMIC_INIT(-1);
+
+bool 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,
+ int ecam_reg_idx)
+{
+ 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 "%s:No bus range found for pci controller\n",
+ dt_node_full_name(dev));
+ } 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, ecam_reg_idx, &addr, &size);
+ if ( err )
+ goto err_exit;
+
+ cfg->phys_addr = addr;
+ cfg->size = size;
+
+ /*
+ * 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);
+
+ return cfg;
+
+err_exit_remap:
+ printk(XENLOG_ERR "ECAM ioremap failed\n");
+err_exit:
+ pci_ecam_free(cfg);
+ return NULL;
+}
+
+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);
+ bridge->bus_start = ~0;
+ bridge->bus_end = ~0;
+ return bridge;
+}
+
+void pci_add_host_bridge(struct pci_host_bridge *bridge)
+{
+ list_add_tail(&bridge->node, &pci_host_bridges);
+}
+
+static int pci_get_new_domain_nr(void)
+{
+ return atomic_inc_return(&domain_nr);
+}
+
+/*
+ * This function will try to obtain the host bridge domain number by
+ * finding a property called "linux,pci-domain" of the given device node.
+ *
+ * @node: device tree node with the domain information
+ *
+ * Returns the associated domain number from DT in the range [0-0xffff], or
+ * a negative value if the required property is not found.
+ */
+static int dt_get_pci_domain_nr(struct dt_device_node *node)
+{
+ u32 domain;
+ int error;
+
+ error = dt_property_read_u32(node, "linux,pci-domain", &domain);
+ if ( !error )
+ return -EINVAL;
+
+ return (u16)domain;
+}
+
+static int pci_bus_find_domain_nr(struct dt_device_node *dev)
+{
+ static int use_dt_domains = -1;
+ int domain;
+
+ domain = dt_get_pci_domain_nr(dev);
+
+ /*
+ * Check DT domain and use_dt_domains values.
+ *
+ * If DT domain property is valid (domain >= 0) and
+ * use_dt_domains != 0, the DT assignment is valid since this means
+ * we have not previously allocated a domain number by using
+ * pci_get_new_domain_nr(); we should also update use_dt_domains to
+ * 1, to indicate that we have just assigned a domain number from
+ * DT.
+ *
+ * If DT domain property value is not valid (ie domain < 0), and we
+ * have not previously assigned a domain number from DT
+ * (use_dt_domains != 1) we should assign a domain number by
+ * using the:
+ *
+ * pci_get_new_domain_nr()
+ *
+ * API and update the use_dt_domains value to keep track of method we
+ * are using to assign domain numbers (use_dt_domains = 0).
+ *
+ * All other combinations imply we have a platform that is trying
+ * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
+ * which is a recipe for domain mishandling and it is prevented by
+ * invalidating the domain value (domain = -1) and printing a
+ * corresponding error.
+ */
+ if ( domain >= 0 && use_dt_domains )
+ {
+ use_dt_domains = 1;
+ }
+ else if ( domain < 0 && use_dt_domains != 1 )
+ {
+ use_dt_domains = 0;
+ domain = pci_get_new_domain_nr();
+ }
+ else
+ {
+ printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" property in DT\n");
+ BUG();
+ }
+
+ return domain;
+}
+
+int pci_host_common_probe(struct dt_device_node *dev,
+ int ecam_reg_idx)
+{
+ struct pci_host_bridge *bridge;
+ struct pci_config_window *cfg;
+ int err;
+
+ bridge = pci_alloc_host_bridge();
+ if ( !bridge )
+ return -ENOMEM;
+
+ /* Parse and map our Configuration Space windows */
+ cfg = gen_pci_init(dev, ecam_reg_idx);
+ if ( !cfg )
+ {
+ err = -ENOMEM;
+ goto err_exit;
+ }
+
+ bridge->dt_node = dev;
+ bridge->sysdata = cfg;
+ bridge->bus_start = cfg->busn_start;
+ bridge->bus_end = cfg->busn_end;
+
+ bridge->segment = pci_bus_find_domain_nr(dev);
+
+ pci_add_host_bridge(bridge);
+
+ return 0;
+
+err_exit:
+ xfree(bridge);
+ return err;
+}
+
+/*
+ * 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..13d0f7f999
--- /dev/null
+++ b/xen/arch/arm/pci/pci-host-generic.c
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2021 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 <xen/pci.h>
+#include <asm/pci.h>
+
+static const struct dt_device_match gen_pci_dt_match[] = {
+ { .compatible = "pci-host-ecam-generic" },
+ { },
+};
+
+static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
+{
+ const struct dt_device_match *of_id;
+
+ of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
+
+ 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, 0);
+}
+
+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/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 61e43da088..58a51e724e 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -26,6 +26,34 @@ struct arch_pci_dev {
struct device 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;
+ void __iomem *win;
+};
+
+/*
+ * 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 */
+ u8 bus_start; /* Bus start of this bridge. */
+ u8 bus_end; /* Bus end of this bridge. */
+ void *sysdata; /* Pointer to the config space window*/
+};
+
+int pci_host_common_probe(struct dt_device_node *dev,
+ int ecam_reg_idx);
#else /*!CONFIG_HAS_PCI*/
struct arch_pci_dev { };
--
2.17.1
Hi Rahul,
On 19/08/2021 13:02, 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.
>
> As of now "pci-host-ecam-generic" compatible board is supported.
I think the word "only" is missing.
>
> "linux,pci-domain" device tree property assigns a fixed PCI domain
> number to a host bridge, otherwise an unstable (across boots) unique
> number will be assigned by Linux.This property has to be in sync with
Typo: missing space after the '.'
> XEN to access the PCI devices.
I would expand a little bit the last sentence to explain why the need to
be sync-ed.
> > XEN will read the “linux,pci-domain” property from the device tree node
> and configure the host bridge segment number accordingly. If this
> property is not available XEN will allocate the unique segment number
> to the host bridge.
>
> dt_get_pci_domain_nr(..) and dt_pci_bus_find_domain_nr(..) are directly
> imported from the Linux source tree.
What was the Linux commit used? I also read "directly imported" as a
verbartim copy but AFAICT the implementation has been slightly reworked.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> xen/arch/arm/pci/Makefile | 2 +
> xen/arch/arm/pci/pci-host-common.c | 261 ++++++++++++++++++++++++++++
> xen/arch/arm/pci/pci-host-generic.c | 55 ++++++
> xen/include/asm-arm/pci.h | 28 +++
> 4 files changed, 346 insertions(+)
> create mode 100644 xen/arch/arm/pci/pci-host-common.c
> create mode 100644 xen/arch/arm/pci/pci-host-generic.c
>
> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
> index a9ee0b9b44..f3d97f859e 100644
> --- a/xen/arch/arm/pci/Makefile
> +++ b/xen/arch/arm/pci/Makefile
> @@ -1,2 +1,4 @@
> obj-y += pci.o
> obj-y += pci-access.o
> +obj-y += pci-host-generic.o
> +obj-y += pci-host-common.o
> 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..9dd9b02271
> --- /dev/null
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -0,0 +1,261 @@
> +/*
> + * Copyright (C) 2021 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>
AFAICT, <xen/pci.h> already includes <asm/pci.h>. So this looks
unneccessary.
> +#include <xen/rwlock.h>
> +#include <xen/sched.h>
> +#include <xen/vmap.h>
> +
> +/*
> + * List for all the pci host bridges.
> + */
> +
> +static LIST_HEAD(pci_host_bridges);
> +
> +static atomic_t domain_nr = ATOMIC_INIT(-1);
> +
> +bool dt_pci_parse_bus_range(struct dt_device_node *dev,
> + struct pci_config_window *cfg)
Aside, "pci_config_window", the function is not Arm specific. Would it
be possible to consider to introduce "struct resource" in Xen so this
function can be moved in common/device_tree.c?
> +{
> + 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;
How about introducing dt_property_read_u32_array()?
> +
> + 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,
> + int ecam_reg_idx)
> +{
> + 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 "%s:No bus range found for pci controller\n",
Typo: Missing space after ':'.
> + dt_node_full_name(dev));
> + } 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, ecam_reg_idx, &addr, &size);
> + if ( err )
> + goto err_exit;
> +
> + cfg->phys_addr = addr;
> + cfg->size = size;
> +
> + /*
> + * 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
In Xen on Arm64, the VMAP is actually only 1GB. So it is not that big
and this will compete with other mapping like ITS, global domain mapping...
So I think the vMAP area will need to increase to cater to increase usage.
> + * 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,
The physical address is a paddr_t. So this needs to use PRIpaddr. Also,
please use preprent hexadecimal with 0x. This makes a lot easier to
differentiate hexa vs decimal in the log.
> + cfg->phys_addr + cfg->size - 1, cfg->busn_start, cfg->busn_end);
> +
> + return cfg;
> +
> +err_exit_remap:
> + printk(XENLOG_ERR "ECAM ioremap failed\n");
> +err_exit:
> + pci_ecam_free(cfg);
Coding style: Please add a new line before return.
> + return NULL;
> +}
> +
> +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);
> + bridge->bus_start = ~0;
> + bridge->bus_end = ~0;
Coding style: Please add a new line before return.
> + return bridge;
> +}
> +
> +void pci_add_host_bridge(struct pci_host_bridge *bridge)
> +{
> + list_add_tail(&bridge->node, &pci_host_bridges);
> +}
> +
> +static int pci_get_new_domain_nr(void)
> +{
> + return atomic_inc_return(&domain_nr);
> +}
> +
> +/*
> + * This function will try to obtain the host bridge domain number by
> + * finding a property called "linux,pci-domain" of the given device node.
> + *
> + * @node: device tree node with the domain information
> + *
> + * Returns the associated domain number from DT in the range [0-0xffff], or
> + * a negative value if the required property is not found.
> + */
> +static int dt_get_pci_domain_nr(struct dt_device_node *node)
Nothing looks Arm specific for this function. Can you move it in
device_tree.c?
> +{
> + u32 domain;
> + int error;
> +
> + error = dt_property_read_u32(node, "linux,pci-domain", &domain);
> + if ( !error )
> + return -EINVAL;
> +
> + return (u16)domain;
> +}
> +
> +static int pci_bus_find_domain_nr(struct dt_device_node *dev)
> +{
> + static int use_dt_domains = -1;
> + int domain;
> +
> + domain = dt_get_pci_domain_nr(dev);
> +
> + /*
> + * Check DT domain and use_dt_domains values.
> + *
> + * If DT domain property is valid (domain >= 0) and
> + * use_dt_domains != 0, the DT assignment is valid since this means
> + * we have not previously allocated a domain number by using
> + * pci_get_new_domain_nr(); we should also update use_dt_domains to
> + * 1, to indicate that we have just assigned a domain number from
> + * DT.
> + *
> + * If DT domain property value is not valid (ie domain < 0), and we
> + * have not previously assigned a domain number from DT
> + * (use_dt_domains != 1) we should assign a domain number by
> + * using the:
> + *
> + * pci_get_new_domain_nr()
> + *
> + * API and update the use_dt_domains value to keep track of method we
> + * are using to assign domain numbers (use_dt_domains = 0).
> + *
> + * All other combinations imply we have a platform that is trying
> + * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> + * which is a recipe for domain mishandling and it is prevented by
> + * invalidating the domain value (domain = -1) and printing a
> + * corresponding error.
> + */
> + if ( domain >= 0 && use_dt_domains )
> + {
> + use_dt_domains = 1;
> + }
> + else if ( domain < 0 && use_dt_domains != 1 )
> + {
> + use_dt_domains = 0;
> + domain = pci_get_new_domain_nr();
> + }
> + else
> + {
> + printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" property in DT\n");
> + BUG();
I don't think pci_bus_find_domain_nr() should be the function that
crashes Xen. Instead, this should be propagated to the highest possible
callers and let it decide what to do.
> + }
> +
> + return domain;
> +}
> +
> +int pci_host_common_probe(struct dt_device_node *dev,
> + int ecam_reg_idx)
> +{
> + struct pci_host_bridge *bridge;
> + struct pci_config_window *cfg;
> + int err;
> +
> + bridge = pci_alloc_host_bridge();
> + if ( !bridge )
> + return -ENOMEM;
> +
> + /* Parse and map our Configuration Space windows */
> + cfg = gen_pci_init(dev, ecam_reg_idx);
> + if ( !cfg )
> + {
> + err = -ENOMEM;
> + goto err_exit;
> + }
> +
> + bridge->dt_node = dev;
> + bridge->sysdata = cfg;
> + bridge->bus_start = cfg->busn_start;
> + bridge->bus_end = cfg->busn_end;
> +
> + bridge->segment = pci_bus_find_domain_nr(dev);
> +
> + pci_add_host_bridge(bridge);
> +
> + return 0;
> +
> +err_exit:
> + xfree(bridge);
> + return err;
> +}
> +
> +/*
> + * 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..13d0f7f999
> --- /dev/null
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (C) 2021 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 <xen/pci.h>
> +#include <asm/pci.h>
> +
> +static const struct dt_device_match gen_pci_dt_match[] = {
> + { .compatible = "pci-host-ecam-generic" },
> + { },
> +};
> +
> +static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
> +{
> + const struct dt_device_match *of_id;
> +
> + of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
This seems to be a bit pointless to me as you already know the
compatible (there is only one possible...).
> +
> + 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, 0);
> +}
> +
> +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/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 61e43da088..58a51e724e 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -26,6 +26,34 @@ struct arch_pci_dev {
> struct device 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;
> + void __iomem *win;
> +};
> +
> +/*
> + * 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 */
> + u8 bus_start; /* Bus start of this bridge. */
> + u8 bus_end; /* Bus end of this bridge. */
Please use uint8_t rather than u8.
> + void *sysdata; /* Pointer to the config space window*/
> +};
> +
> +int pci_host_common_probe(struct dt_device_node *dev,
> + int ecam_reg_idx);
> #else /*!CONFIG_HAS_PCI*/
>
> struct arch_pci_dev { };
>
Cheers,
--
Julien Grall
Hi Julien
> On 7 Sep 2021, at 10:05 am, Julien Grall <julien@xen.org> wrote:
>
> Hi Rahul,
>
> On 19/08/2021 13:02, 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.
>> As of now "pci-host-ecam-generic" compatible board is supported.
>
> I think the word "only" is missing.
Ok.
>
>> "linux,pci-domain" device tree property assigns a fixed PCI domain
>> number to a host bridge, otherwise an unstable (across boots) unique
>> number will be assigned by Linux.This property has to be in sync with
>
> Typo: missing space after the ‘.’
Ack.
>> XEN to access the PCI devices.
>
> I would expand a little bit the last sentence to explain why the need to be sync-ed.
I will explain why segment and domain need to be in sync in next version.
>
>> > XEN will read the “linux,pci-domain” property from the device tree node
>> and configure the host bridge segment number accordingly. If this
>> property is not available XEN will allocate the unique segment number
>> to the host bridge.
>> dt_get_pci_domain_nr(..) and dt_pci_bus_find_domain_nr(..) are directly
>> imported from the Linux source tree.
>
> What was the Linux commit used? I also read "directly imported" as a verbartim copy but AFAICT the implementation has been slightly reworked.
I will add the Linux commit used in commit msg and also will add that code is slightly modified.
>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> xen/arch/arm/pci/Makefile | 2 +
>> xen/arch/arm/pci/pci-host-common.c | 261 ++++++++++++++++++++++++++++
>> xen/arch/arm/pci/pci-host-generic.c | 55 ++++++
>> xen/include/asm-arm/pci.h | 28 +++
>> 4 files changed, 346 insertions(+)
>> create mode 100644 xen/arch/arm/pci/pci-host-common.c
>> create mode 100644 xen/arch/arm/pci/pci-host-generic.c
>> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
>> index a9ee0b9b44..f3d97f859e 100644
>> --- a/xen/arch/arm/pci/Makefile
>> +++ b/xen/arch/arm/pci/Makefile
>> @@ -1,2 +1,4 @@
>> obj-y += pci.o
>> obj-y += pci-access.o
>> +obj-y += pci-host-generic.o
>> +obj-y += pci-host-common.o
>> 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..9dd9b02271
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -0,0 +1,261 @@
>> +/*
>> + * Copyright (C) 2021 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>
>
> AFAICT, <xen/pci.h> already includes <asm/pci.h>. So this looks unneccessary.
Ack.
>
>> +#include <xen/rwlock.h>
>> +#include <xen/sched.h>
>> +#include <xen/vmap.h>
>> +
>> +/*
>> + * List for all the pci host bridges.
>> + */
>> +
>> +static LIST_HEAD(pci_host_bridges);
>> +
>> +static atomic_t domain_nr = ATOMIC_INIT(-1);
>> +
>> +bool dt_pci_parse_bus_range(struct dt_device_node *dev,
>> + struct pci_config_window *cfg)
>
> Aside, "pci_config_window", the function is not Arm specific. Would it be possible to consider to introduce "struct resource" in Xen so this function can be moved in common/device_tree.c?
I can introduce the "struct resource” but I am not sure whether "struct resource” will be
useful later point in time. What I prefer as of now, we can have this function and we can
move this to common/device_tree.c once we have the requirement for "struct resource”.
>
>> +{
>> + 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;
>
> How about introducing dt_property_read_u32_array()?
Ok. I will introduce dt_property_read_u32_array().
>
>> +
>> + 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,
>> + int ecam_reg_idx)
>> +{
>> + 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 "%s:No bus range found for pci controller\n",
>
> Typo: Missing space after ':’.
Ack.
>
>> + dt_node_full_name(dev));
>> + } 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, ecam_reg_idx, &addr, &size);
>> + if ( err )
>> + goto err_exit;
>> +
>> + cfg->phys_addr = addr;
>> + cfg->size = size;
>> +
>> + /*
>> + * 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
>
> In Xen on Arm64, the VMAP is actually only 1GB. So it is not that big and this will compete with other mapping like ITS, global domain mapping...
>
> So I think the vMAP area will need to increase to cater to increase usage.
Let me check on this and come back to you.
>
>> + * 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,
>
> The physical address is a paddr_t. So this needs to use PRIpaddr. Also, please use preprent hexadecimal with 0x. This makes a lot easier to differentiate hexa vs decimal in the log.
Ack.
>
>> + cfg->phys_addr + cfg->size - 1, cfg->busn_start, cfg->busn_end);
>> +
>> + return cfg;
>> +
>> +err_exit_remap:
>> + printk(XENLOG_ERR "ECAM ioremap failed\n");
>> +err_exit:
>> + pci_ecam_free(cfg);
>
> Coding style: Please add a new line before return.
Ack.
>
>> + return NULL;
>> +}
>> +
>> +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);
>> + bridge->bus_start = ~0;
>> + bridge->bus_end = ~0;
>
> Coding style: Please add a new line before return.
Ack.
>
>
>> + return bridge;
>> +}
>> +
>> +void pci_add_host_bridge(struct pci_host_bridge *bridge)
>> +{
>> + list_add_tail(&bridge->node, &pci_host_bridges);
>> +}
>> +
>> +static int pci_get_new_domain_nr(void)
>> +{
>> + return atomic_inc_return(&domain_nr);
>> +}
>> +
>> +/*
>> + * This function will try to obtain the host bridge domain number by
>> + * finding a property called "linux,pci-domain" of the given device node.
>> + *
>> + * @node: device tree node with the domain information
>> + *
>> + * Returns the associated domain number from DT in the range [0-0xffff], or
>> + * a negative value if the required property is not found.
>> + */
>> +static int dt_get_pci_domain_nr(struct dt_device_node *node)
>
> Nothing looks Arm specific for this function. Can you move it in device_tree.c?
Ack.
>
>> +{
>> + u32 domain;
>> + int error;
>> +
>> + error = dt_property_read_u32(node, "linux,pci-domain", &domain);
>> + if ( !error )
>> + return -EINVAL;
>> +
>> + return (u16)domain;
>> +}
>> +
>> +static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>> +{
>> + static int use_dt_domains = -1;
>> + int domain;
>> +
>> + domain = dt_get_pci_domain_nr(dev);
>> +
>> + /*
>> + * Check DT domain and use_dt_domains values.
>> + *
>> + * If DT domain property is valid (domain >= 0) and
>> + * use_dt_domains != 0, the DT assignment is valid since this means
>> + * we have not previously allocated a domain number by using
>> + * pci_get_new_domain_nr(); we should also update use_dt_domains to
>> + * 1, to indicate that we have just assigned a domain number from
>> + * DT.
>> + *
>> + * If DT domain property value is not valid (ie domain < 0), and we
>> + * have not previously assigned a domain number from DT
>> + * (use_dt_domains != 1) we should assign a domain number by
>> + * using the:
>> + *
>> + * pci_get_new_domain_nr()
>> + *
>> + * API and update the use_dt_domains value to keep track of method we
>> + * are using to assign domain numbers (use_dt_domains = 0).
>> + *
>> + * All other combinations imply we have a platform that is trying
>> + * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
>> + * which is a recipe for domain mishandling and it is prevented by
>> + * invalidating the domain value (domain = -1) and printing a
>> + * corresponding error.
>> + */
>> + if ( domain >= 0 && use_dt_domains )
>> + {
>> + use_dt_domains = 1;
>> + }
>> + else if ( domain < 0 && use_dt_domains != 1 )
>> + {
>> + use_dt_domains = 0;
>> + domain = pci_get_new_domain_nr();
>> + }
>> + else
>> + {
>> + printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" property in DT\n");
>> + BUG();
>
> I don't think pci_bus_find_domain_nr() should be the function that crashes Xen. Instead, this should be propagated to the highest possible callers and let it decide what to do.
Ack.
>
>> + }
>> +
>> + return domain;
>> +}
>> +
>> +int pci_host_common_probe(struct dt_device_node *dev,
>> + int ecam_reg_idx)
>> +{
>> + struct pci_host_bridge *bridge;
>> + struct pci_config_window *cfg;
>> + int err;
>> +
>> + bridge = pci_alloc_host_bridge();
>> + if ( !bridge )
>> + return -ENOMEM;
>> +
>> + /* Parse and map our Configuration Space windows */
>> + cfg = gen_pci_init(dev, ecam_reg_idx);
>> + if ( !cfg )
>> + {
>> + err = -ENOMEM;
>> + goto err_exit;
>> + }
>> +
>> + bridge->dt_node = dev;
>> + bridge->sysdata = cfg;
>> + bridge->bus_start = cfg->busn_start;
>> + bridge->bus_end = cfg->busn_end;
>> +
>> + bridge->segment = pci_bus_find_domain_nr(dev);
>> +
>> + pci_add_host_bridge(bridge);
>> +
>> + return 0;
>> +
>> +err_exit:
>> + xfree(bridge);
>> + return err;
>> +}
>> +
>> +/*
>> + * 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..13d0f7f999
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-host-generic.c
>> @@ -0,0 +1,55 @@
>> +/*
>> + * Copyright (C) 2021 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 <xen/pci.h>
>> +#include <asm/pci.h>
>> +
>> +static const struct dt_device_match gen_pci_dt_match[] = {
>> + { .compatible = "pci-host-ecam-generic" },
>> + { },
>> +};
>> +
>> +static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
>> +{
>> + const struct dt_device_match *of_id;
>> +
>> + of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
>
> This seems to be a bit pointless to me as you already know the compatible (there is only one possible...).
As of now we are only implementing the "pci-host-ecam-generic” compatible PCI, but in future we might
need to implement the other compatible like Linux see as below.
https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L59
>
>> +
>> + 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, 0);
>> +}
>> +
>> +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/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index 61e43da088..58a51e724e 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -26,6 +26,34 @@ struct arch_pci_dev {
>> struct device 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;
>> + void __iomem *win;
>> +};
>> +
>> +/*
>> + * 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 */
>> + u8 bus_start; /* Bus start of this bridge. */
>> + u8 bus_end; /* Bus end of this bridge. */
>
> Please use uint8_t rather than u8.
Ack.
Regards,
Rahul
On 10/09/2021 12:22, Rahul Singh wrote:
> Hi Julien
Hi Rahul,
>> On 7 Sep 2021, at 10:05 am, Julien Grall <julien@xen.org> wrote:
>> On 19/08/2021 13:02, Rahul Singh wrote:
>>> +#include <xen/rwlock.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/vmap.h>
>>> +
>>> +/*
>>> + * List for all the pci host bridges.
>>> + */
>>> +
>>> +static LIST_HEAD(pci_host_bridges);
>>> +
>>> +static atomic_t domain_nr = ATOMIC_INIT(-1);
>>> +
>>> +bool dt_pci_parse_bus_range(struct dt_device_node *dev,
>>> + struct pci_config_window *cfg)
>>
>> Aside, "pci_config_window", the function is not Arm specific. Would it be possible to consider to introduce "struct resource" in Xen so this function can be moved in common/device_tree.c?
>
> I can introduce the "struct resource” but I am not sure whether "struct resource” will be
> useful later point in time. What I prefer as of now, we can have this function and we can
> move this to common/device_tree.c once we have the requirement for "struct resource”.
TBH, I wasn't asking about using "struct resource". This was simply a
suggestion how to make the code common.
What I am more interested is moving this function in common code so it
can be re-used by RISC-v in the future. I don't particularly mind
whether this is using "struct resource" or a different structure.
>>> 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..13d0f7f999
>>> --- /dev/null
>>> +++ b/xen/arch/arm/pci/pci-host-generic.c
>>> @@ -0,0 +1,55 @@
>>> +/*
>>> + * Copyright (C) 2021 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 <xen/pci.h>
>>> +#include <asm/pci.h>
>>> +
>>> +static const struct dt_device_match gen_pci_dt_match[] = {
>>> + { .compatible = "pci-host-ecam-generic" },
>>> + { },
>>> +};
>>> +
>>> +static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
>>> +{
>>> + const struct dt_device_match *of_id;
>>> +
>>> + of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
>>
>> This seems to be a bit pointless to me as you already know the compatible (there is only one possible...).
> As of now we are only implementing the "pci-host-ecam-generic” compatible PCI, but in future we might
> need to implement the other compatible like Linux see as below.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L59
Right... You know the node matched and you don't need any data. So this
is a bit pointless to have to walk again the structure for just printing
the compatible that matched.
Cheers,
--
Julien Grall
On Thu, 19 Aug 2021, 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.
>
> As of now "pci-host-ecam-generic" compatible board is supported.
>
> "linux,pci-domain" device tree property assigns a fixed PCI domain
> number to a host bridge, otherwise an unstable (across boots) unique
> number will be assigned by Linux.This property has to be in sync with
> XEN to access the PCI devices.
>
> XEN will read the “linux,pci-domain” property from the device tree node
> and configure the host bridge segment number accordingly. If this
> property is not available XEN will allocate the unique segment number
> to the host bridge.
>
> dt_get_pci_domain_nr(..) and dt_pci_bus_find_domain_nr(..) are directly
> imported from the Linux source tree.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> xen/arch/arm/pci/Makefile | 2 +
> xen/arch/arm/pci/pci-host-common.c | 261 ++++++++++++++++++++++++++++
> xen/arch/arm/pci/pci-host-generic.c | 55 ++++++
> xen/include/asm-arm/pci.h | 28 +++
> 4 files changed, 346 insertions(+)
> create mode 100644 xen/arch/arm/pci/pci-host-common.c
> create mode 100644 xen/arch/arm/pci/pci-host-generic.c
>
> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
> index a9ee0b9b44..f3d97f859e 100644
> --- a/xen/arch/arm/pci/Makefile
> +++ b/xen/arch/arm/pci/Makefile
> @@ -1,2 +1,4 @@
> obj-y += pci.o
> obj-y += pci-access.o
> +obj-y += pci-host-generic.o
> +obj-y += pci-host-common.o
> 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..9dd9b02271
> --- /dev/null
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -0,0 +1,261 @@
> +/*
> + * Copyright (C) 2021 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/sched.h>
> +#include <xen/vmap.h>
> +
> +/*
> + * List for all the pci host bridges.
> + */
> +
> +static LIST_HEAD(pci_host_bridges);
> +
> +static atomic_t domain_nr = ATOMIC_INIT(-1);
> +
> +bool 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,
> + int ecam_reg_idx)
If it is only called at init time, then the function should be __init
> +{
> + 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 "%s:No bus range found for pci controller\n",
> + dt_node_full_name(dev));
> + } else {
> + if ( cfg->busn_end > cfg->busn_start + 0xff )
> + cfg->busn_end = cfg->busn_start + 0xff;
Is this a hard limit in the specification? Or is it a limit in the Xen
implementation?
> + }
> +
> + /* Parse our PCI ecam register address*/
^ space
> + err = dt_device_get_address(dev, ecam_reg_idx, &addr, &size);
> + if ( err )
> + goto err_exit;
> +
> + cfg->phys_addr = addr;
> + cfg->size = size;
> +
> + /*
> + * 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);
> +
> + return cfg;
> +
> +err_exit_remap:
> + printk(XENLOG_ERR "ECAM ioremap failed\n");
> +err_exit:
> + pci_ecam_free(cfg);
> + return NULL;
> +}
> +
> +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);
> + bridge->bus_start = ~0;
> + bridge->bus_end = ~0;
Please use INVALID_PADDR instead of ~0
> + return bridge;
> +}
> +
> +void pci_add_host_bridge(struct pci_host_bridge *bridge)
> +{
> + list_add_tail(&bridge->node, &pci_host_bridges);
> +}
> +
> +static int pci_get_new_domain_nr(void)
> +{
> + return atomic_inc_return(&domain_nr);
> +}
> +
> +/*
> + * This function will try to obtain the host bridge domain number by
> + * finding a property called "linux,pci-domain" of the given device node.
> + *
> + * @node: device tree node with the domain information
> + *
> + * Returns the associated domain number from DT in the range [0-0xffff], or
> + * a negative value if the required property is not found.
> + */
> +static int dt_get_pci_domain_nr(struct dt_device_node *node)
> +{
> + u32 domain;
> + int error;
> +
> + error = dt_property_read_u32(node, "linux,pci-domain", &domain);
> + if ( !error )
> + return -EINVAL;
> +
> + return (u16)domain;
Let's check that domain <= UINT16_MAX
> +}
> +
> +static int pci_bus_find_domain_nr(struct dt_device_node *dev)
> +{
> + static int use_dt_domains = -1;
> + int domain;
> +
> + domain = dt_get_pci_domain_nr(dev);
> +
> + /*
> + * Check DT domain and use_dt_domains values.
> + *
> + * If DT domain property is valid (domain >= 0) and
> + * use_dt_domains != 0, the DT assignment is valid since this means
> + * we have not previously allocated a domain number by using
> + * pci_get_new_domain_nr(); we should also update use_dt_domains to
> + * 1, to indicate that we have just assigned a domain number from
> + * DT.
> + *
> + * If DT domain property value is not valid (ie domain < 0), and we
> + * have not previously assigned a domain number from DT
> + * (use_dt_domains != 1) we should assign a domain number by
> + * using the:
> + *
> + * pci_get_new_domain_nr()
> + *
> + * API and update the use_dt_domains value to keep track of method we
> + * are using to assign domain numbers (use_dt_domains = 0).
> + *
> + * All other combinations imply we have a platform that is trying
> + * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> + * which is a recipe for domain mishandling and it is prevented by
> + * invalidating the domain value (domain = -1) and printing a
> + * corresponding error.
> + */
> + if ( domain >= 0 && use_dt_domains )
> + {
> + use_dt_domains = 1;
> + }
> + else if ( domain < 0 && use_dt_domains != 1 )
> + {
> + use_dt_domains = 0;
> + domain = pci_get_new_domain_nr();
> + }
> + else
> + {
> + printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" property in DT\n");
> + BUG();
> + }
> +
> + return domain;
> +}
> +
> +int pci_host_common_probe(struct dt_device_node *dev,
> + int ecam_reg_idx)
> +{
> + struct pci_host_bridge *bridge;
> + struct pci_config_window *cfg;
> + int err;
> +
> + bridge = pci_alloc_host_bridge();
> + if ( !bridge )
> + return -ENOMEM;
> +
> + /* Parse and map our Configuration Space windows */
> + cfg = gen_pci_init(dev, ecam_reg_idx);
> + if ( !cfg )
> + {
> + err = -ENOMEM;
> + goto err_exit;
> + }
> +
> + bridge->dt_node = dev;
> + bridge->sysdata = cfg;
> + bridge->bus_start = cfg->busn_start;
> + bridge->bus_end = cfg->busn_end;
> +
> + bridge->segment = pci_bus_find_domain_nr(dev);
> +
> + pci_add_host_bridge(bridge);
> +
> + return 0;
> +
> +err_exit:
> + xfree(bridge);
> + return err;
> +}
> +
> +/*
> + * 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..13d0f7f999
> --- /dev/null
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (C) 2021 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 <xen/pci.h>
> +#include <asm/pci.h>
> +
> +static const struct dt_device_match gen_pci_dt_match[] = {
> + { .compatible = "pci-host-ecam-generic" },
> + { },
> +};
> +
> +static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
> +{
> + const struct dt_device_match *of_id;
> +
> + of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
> +
> + 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, 0);
> +}
> +
> +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/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 61e43da088..58a51e724e 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -26,6 +26,34 @@ struct arch_pci_dev {
> struct device 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;
> + void __iomem *win;
> +};
> +
> +/*
> + * 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 */
> + u8 bus_start; /* Bus start of this bridge. */
> + u8 bus_end; /* Bus end of this bridge. */
> + void *sysdata; /* Pointer to the config space window*/
> +};
> +
> +int pci_host_common_probe(struct dt_device_node *dev,
> + int ecam_reg_idx);
> #else /*!CONFIG_HAS_PCI*/
>
> struct arch_pci_dev { };
> --
> 2.17.1
>
Hi Stefano,
> On 9 Sep 2021, at 11:54 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Thu, 19 Aug 2021, 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.
>>
>> As of now "pci-host-ecam-generic" compatible board is supported.
>>
>> "linux,pci-domain" device tree property assigns a fixed PCI domain
>> number to a host bridge, otherwise an unstable (across boots) unique
>> number will be assigned by Linux.This property has to be in sync with
>> XEN to access the PCI devices.
>>
>> XEN will read the “linux,pci-domain” property from the device tree node
>> and configure the host bridge segment number accordingly. If this
>> property is not available XEN will allocate the unique segment number
>> to the host bridge.
>>
>> dt_get_pci_domain_nr(..) and dt_pci_bus_find_domain_nr(..) are directly
>> imported from the Linux source tree.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> xen/arch/arm/pci/Makefile | 2 +
>> xen/arch/arm/pci/pci-host-common.c | 261 ++++++++++++++++++++++++++++
>> xen/arch/arm/pci/pci-host-generic.c | 55 ++++++
>> xen/include/asm-arm/pci.h | 28 +++
>> 4 files changed, 346 insertions(+)
>> create mode 100644 xen/arch/arm/pci/pci-host-common.c
>> create mode 100644 xen/arch/arm/pci/pci-host-generic.c
>>
>> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
>> index a9ee0b9b44..f3d97f859e 100644
>> --- a/xen/arch/arm/pci/Makefile
>> +++ b/xen/arch/arm/pci/Makefile
>> @@ -1,2 +1,4 @@
>> obj-y += pci.o
>> obj-y += pci-access.o
>> +obj-y += pci-host-generic.o
>> +obj-y += pci-host-common.o
>> 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..9dd9b02271
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -0,0 +1,261 @@
>> +/*
>> + * Copyright (C) 2021 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/sched.h>
>> +#include <xen/vmap.h>
>> +
>> +/*
>> + * List for all the pci host bridges.
>> + */
>> +
>> +static LIST_HEAD(pci_host_bridges);
>> +
>> +static atomic_t domain_nr = ATOMIC_INIT(-1);
>> +
>> +bool 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,
>> + int ecam_reg_idx)
>
> If it is only called at init time, then the function should be __init
Ack.
>
>
>> +{
>> + 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 "%s:No bus range found for pci controller\n",
>> + dt_node_full_name(dev));
>> + } else {
>> + if ( cfg->busn_end > cfg->busn_start + 0xff )
>> + cfg->busn_end = cfg->busn_start + 0xff;
>
> Is this a hard limit in the specification? Or is it a limit in the Xen
> implementation?
I just take the reference from Linux code.
https://elixir.bootlin.com/linux/latest/source/drivers/pci/of.c#L306
>
>
>> + }
>> +
>> + /* Parse our PCI ecam register address*/
> ^ space
>
Ack.
>> + err = dt_device_get_address(dev, ecam_reg_idx, &addr, &size);
>> + if ( err )
>> + goto err_exit;
>> +
>> + cfg->phys_addr = addr;
>> + cfg->size = size;
>> +
>> + /*
>> + * 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);
>> +
>> + return cfg;
>> +
>> +err_exit_remap:
>> + printk(XENLOG_ERR "ECAM ioremap failed\n");
>> +err_exit:
>> + pci_ecam_free(cfg);
>> + return NULL;
>> +}
>> +
>> +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);
>> + bridge->bus_start = ~0;
>> + bridge->bus_end = ~0;
>
> Please use INVALID_PADDR instead of ~0
bridge->bus_start is if type uint8_t if I use INVALID_PADDR I will get overflow error.
I will use U8_MAX instead of INVALID_PADDR.
>
>
>> + return bridge;
>> +}
>> +
>> +void pci_add_host_bridge(struct pci_host_bridge *bridge)
>> +{
>> + list_add_tail(&bridge->node, &pci_host_bridges);
>> +}
>> +
>> +static int pci_get_new_domain_nr(void)
>> +{
>> + return atomic_inc_return(&domain_nr);
>> +}
>> +
>> +/*
>> + * This function will try to obtain the host bridge domain number by
>> + * finding a property called "linux,pci-domain" of the given device node.
>> + *
>> + * @node: device tree node with the domain information
>> + *
>> + * Returns the associated domain number from DT in the range [0-0xffff], or
>> + * a negative value if the required property is not found.
>> + */
>> +static int dt_get_pci_domain_nr(struct dt_device_node *node)
>> +{
>> + u32 domain;
>> + int error;
>> +
>> + error = dt_property_read_u32(node, "linux,pci-domain", &domain);
>> + if ( !error )
>> + return -EINVAL;
>> +
>> + return (u16)domain;
>
> Let's check that domain <= UINT16_MAX
>
Ack
Regards,
Rahul
Hi, Rahul!
On 19.08.21 15:02, 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.
[snip]
> +static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
> + int ecam_reg_idx)
> +{
> + 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 "%s:No bus range found for pci controller\n",
> + dt_node_full_name(dev));
> + } 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, ecam_reg_idx, &addr, &size);
I am a bit worried here that we don't get the reg index from the device tree,
but for generic ECAM we use reg[0] and for Xilinx we use reg[2].
For example, for Xilinx we have
reg = <0x00 0xfd0e0000 0x00 0x1000 0x00 0xfd480000 0x00 0x1000 0x80 0x00 0x00 0x1000000>;
reg-names = "breg\0pcireg\0cfg";
so, we can parse the reg-names and understand that the configuration space is the last in the reg property.
The same I think can be done for other device trees probably.
Rahul, do you know if reg-names "cfg" is vendor specific of used widely?
Thank you,
Oleksandr
On Mon, 13 Sep 2021, Oleksandr Andrushchenko wrote:
> Hi, Rahul!
>
> On 19.08.21 15:02, 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.
> [snip]
> > +static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
> > + int ecam_reg_idx)
> > +{
> > + 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 "%s:No bus range found for pci controller\n",
> > + dt_node_full_name(dev));
> > + } 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, ecam_reg_idx, &addr, &size);
>
> I am a bit worried here that we don't get the reg index from the device tree,
>
> but for generic ECAM we use reg[0] and for Xilinx we use reg[2].
>
> For example, for Xilinx we have
>
> reg = <0x00 0xfd0e0000 0x00 0x1000 0x00 0xfd480000 0x00 0x1000 0x80 0x00 0x00 0x1000000>;
> reg-names = "breg\0pcireg\0cfg";
>
> so, we can parse the reg-names and understand that the configuration space is the last in the reg property.
>
> The same I think can be done for other device trees probably.
Well spotted!
> Rahul, do you know if reg-names "cfg" is vendor specific of used widely?
Unfortunately it seems to be vendor specific :-(
Which means that "dt_device_get_address" or similar should be moved to a
vendor specific function.
On 13.09.21 23:23, Stefano Stabellini wrote:
> On Mon, 13 Sep 2021, Oleksandr Andrushchenko wrote:
>> Hi, Rahul!
>>
>> On 19.08.21 15:02, 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.
>> [snip]
>>> +static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>>> + int ecam_reg_idx)
>>> +{
>>> + 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 "%s:No bus range found for pci controller\n",
>>> + dt_node_full_name(dev));
>>> + } 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, ecam_reg_idx, &addr, &size);
>> I am a bit worried here that we don't get the reg index from the device tree,
>>
>> but for generic ECAM we use reg[0] and for Xilinx we use reg[2].
>>
>> For example, for Xilinx we have
>>
>> reg = <0x00 0xfd0e0000 0x00 0x1000 0x00 0xfd480000 0x00 0x1000 0x80 0x00 0x00 0x1000000>;
>> reg-names = "breg\0pcireg\0cfg";
>>
>> so, we can parse the reg-names and understand that the configuration space is the last in the reg property.
>>
>> The same I think can be done for other device trees probably.
> Well spotted!
>
>
>> Rahul, do you know if reg-names "cfg" is vendor specific of used widely?
> Unfortunately it seems to be vendor specific :-(
> Which means that "dt_device_get_address" or similar should be moved to a
> vendor specific function.
I just stepped on this while working on the patch for not mapping
MMIOs and config to Dom0 and need to find a way to tell them from
device registers. So, yes, it seems we will need something more generic
than just an index. We can still have the callback for this optional,
so the bridges with the same reg names can use the default callback
and don't need to implement the same
Hi, Rahul!
On 19.08.21 15:02, 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.
[snip]
> +int pci_host_common_probe(struct dt_device_node *dev,
> + int ecam_reg_idx)
> +{
> + struct pci_host_bridge *bridge;
> + struct pci_config_window *cfg;
> + int err;
> +
Should we check if the bridge is to be passed through? E.g.
if ( dt_device_for_passthrough(dev) )
return 0;
Thanks,
Oleksandr
© 2016 - 2024 Red Hat, Inc.