Add support for PCI ecam operations to access the PCI
configuration space.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/arch/arm/pci/Makefile           |  1 +
 xen/arch/arm/pci/ecam.c             | 63 +++++++++++++++++++++++++++++
 xen/arch/arm/pci/pci-access.c       | 53 ++++++++++++++++++++++++
 xen/arch/arm/pci/pci-host-common.c  | 13 +++++-
 xen/arch/arm/pci/pci-host-generic.c |  8 +++-
 xen/include/asm-arm/pci.h           | 32 +++++++++++++++
 6 files changed, 167 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/arm/pci/ecam.c

diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
index f3d97f859e..6f32fbbe67 100644
--- a/xen/arch/arm/pci/Makefile
+++ b/xen/arch/arm/pci/Makefile
@@ -2,3 +2,4 @@ obj-y += pci.o
 obj-y += pci-access.o
 obj-y += pci-host-generic.o
 obj-y += pci-host-common.o
+obj-y += ecam.o
diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
new file mode 100644
index 0000000000..91c691b41f
--- /dev/null
+++ b/xen/arch/arm/pci/ecam.c
@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) 2021 Arm Ltd.
+ *
+ * Based on Linux drivers/pci/ecam.c
+ * Copyright 2016 Broadcom
+ *
+ * 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/pci.h>
+#include <xen/sched.h>
+
+/*
+ * Function to implement the pci_ops ->map_bus method.
+ */
+void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
+                                      uint32_t sbdf, uint32_t where)
+{
+    const struct pci_config_window *cfg = bridge->sysdata;
+    unsigned int devfn_shift = cfg->ops->bus_shift - 8;
+    void __iomem *base;
+
+    pci_sbdf_t sbdf_t = (pci_sbdf_t) sbdf ;
+    unsigned int busn = sbdf_t.bus;
+
+    if ( busn < cfg->busn_start || busn > cfg->busn_end )
+        return NULL;
+
+    busn -= cfg->busn_start;
+    base = cfg->win + (busn << cfg->ops->bus_shift);
+
+    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
+}
+
+/* ECAM ops */
+const struct pci_ecam_ops pci_generic_ecam_ops = {
+    .bus_shift  = 20,
+    .pci_ops    = {
+        .map_bus                = pci_ecam_map_bus,
+        .read                   = pci_generic_config_read,
+        .write                  = pci_generic_config_write,
+    }
+};
+
+/*
+ * 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-access.c b/xen/arch/arm/pci/pci-access.c
index b938047c03..f39f6a3a38 100644
--- a/xen/arch/arm/pci/pci-access.c
+++ b/xen/arch/arm/pci/pci-access.c
@@ -15,6 +15,59 @@
  */
 
 #include <xen/pci.h>
+#include <asm/io.h>
+
+int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
+                            uint32_t reg, uint32_t len, uint32_t *value)
+{
+    void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
+    if (!addr) {
+        *value = ~0;
+        return -ENODEV;
+    }
+
+    switch (len)
+    {
+    case 1:
+        *value = readb(addr);
+        break;
+    case 2:
+        *value = readw(addr);
+        break;
+    case 4:
+        *value = readl(addr);
+        break;
+    default:
+        BUG();
+    }
+
+    return 0;
+}
+
+int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
+                            uint32_t reg, uint32_t len, uint32_t value)
+{
+    void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
+    if (!addr)
+        return -ENODEV;
+
+    switch (len)
+    {
+    case 1:
+        writeb(value, addr);
+        break;
+    case 2:
+        writew(value, addr);
+        break;
+    case 4:
+        writel(value, addr);
+        break;
+    default:
+        BUG();
+    }
+
+    return 0;
+}
 
 static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
                                 unsigned int len)
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 9dd9b02271..c582527e92 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -68,6 +68,7 @@ static void pci_ecam_free(struct pci_config_window *cfg)
 }
 
 static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
+                                              const struct pci_ecam_ops *ops,
                                               int ecam_reg_idx)
 {
     int err;
@@ -96,6 +97,7 @@ static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
 
     cfg->phys_addr = addr;
     cfg->size = size;
+    cfg->ops = ops;
 
     /*
      * On 64-bit systems, we do a single ioremap for the whole config space
@@ -111,6 +113,13 @@ static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
     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:
@@ -216,6 +225,7 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
 }
 
 int pci_host_common_probe(struct dt_device_node *dev,
+                          const struct pci_ecam_ops *ops,
                           int ecam_reg_idx)
 {
     struct pci_host_bridge *bridge;
@@ -227,7 +237,7 @@ int pci_host_common_probe(struct dt_device_node *dev,
         return -ENOMEM;
 
     /* Parse and map our Configuration Space windows */
-    cfg = gen_pci_init(dev, ecam_reg_idx);
+    cfg = gen_pci_init(dev, ops, ecam_reg_idx);
     if ( !cfg )
     {
         err = -ENOMEM;
@@ -236,6 +246,7 @@ int pci_host_common_probe(struct dt_device_node *dev,
 
     bridge->dt_node = dev;
     bridge->sysdata = cfg;
+    bridge->ops = &ops->pci_ops;
     bridge->bus_start = cfg->busn_start;
     bridge->bus_end = cfg->busn_end;
 
diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
index 13d0f7f999..2d652e8910 100644
--- a/xen/arch/arm/pci/pci-host-generic.c
+++ b/xen/arch/arm/pci/pci-host-generic.c
@@ -23,20 +23,24 @@
 #include <asm/pci.h>
 
 static const struct dt_device_match gen_pci_dt_match[] = {
-    { .compatible = "pci-host-ecam-generic" },
+    { .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;
+    const 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, 0);
+    return pci_host_common_probe(dev, ops, 0);
 }
 
 DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 58a51e724e..22866244d2 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -37,6 +37,7 @@ struct pci_config_window {
     uint8_t         busn_start;
     uint8_t         busn_end;
     void __iomem    *win;
+    const struct    pci_ecam_ops *ops;
 };
 
 /*
@@ -50,10 +51,41 @@ struct pci_host_bridge {
     u8 bus_start;                    /* Bus start of this bridge. */
     u8 bus_end;                      /* Bus end of this bridge. */
     void *sysdata;                   /* Pointer to the config space window*/
+    const struct pci_ops *ops;
 };
 
+struct pci_ops {
+    void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
+                             uint32_t offset);
+    int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
+                uint32_t reg, uint32_t len, uint32_t *value);
+    int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
+                 uint32_t reg, uint32_t len, uint32_t value);
+};
+
+/*
+ * 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 *);
+};
+
+/* Default ECAM ops */
+extern const struct pci_ecam_ops pci_generic_ecam_ops;
+
 int pci_host_common_probe(struct dt_device_node *dev,
+                          const struct pci_ecam_ops *ops,
                           int ecam_reg_idx);
+int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
+                            uint32_t reg, uint32_t len, uint32_t *value);
+int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
+                            uint32_t reg, uint32_t len, uint32_t value);
+void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
+                               uint32_t sbdf, uint32_t where);
+
 #else   /*!CONFIG_HAS_PCI*/
 
 struct arch_pci_dev { };
-- 
2.17.1

On Thu, 19 Aug 2021, Rahul Singh wrote:
> Add support for PCI ecam operations to access the PCI
> configuration space.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>  xen/arch/arm/pci/Makefile           |  1 +
>  xen/arch/arm/pci/ecam.c             | 63 +++++++++++++++++++++++++++++
>  xen/arch/arm/pci/pci-access.c       | 53 ++++++++++++++++++++++++
>  xen/arch/arm/pci/pci-host-common.c  | 13 +++++-
>  xen/arch/arm/pci/pci-host-generic.c |  8 +++-
>  xen/include/asm-arm/pci.h           | 32 +++++++++++++++
>  6 files changed, 167 insertions(+), 3 deletions(-)
>  create mode 100644 xen/arch/arm/pci/ecam.c
> 
> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
> index f3d97f859e..6f32fbbe67 100644
> --- a/xen/arch/arm/pci/Makefile
> +++ b/xen/arch/arm/pci/Makefile
> @@ -2,3 +2,4 @@ obj-y += pci.o
>  obj-y += pci-access.o
>  obj-y += pci-host-generic.o
>  obj-y += pci-host-common.o
> +obj-y += ecam.o
> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
> new file mode 100644
> index 0000000000..91c691b41f
> --- /dev/null
> +++ b/xen/arch/arm/pci/ecam.c
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (C) 2021 Arm Ltd.
> + *
> + * Based on Linux drivers/pci/ecam.c
> + * Copyright 2016 Broadcom
> + *
> + * 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/pci.h>
> +#include <xen/sched.h>
> +
> +/*
> + * Function to implement the pci_ops ->map_bus method.
> + */
> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
> +                                      uint32_t sbdf, uint32_t where)

Code style: alignment


> +{
> +    const struct pci_config_window *cfg = bridge->sysdata;
> +    unsigned int devfn_shift = cfg->ops->bus_shift - 8;

Is it a guarantee that devfn_shift == bus_shift - 8, or is it just so
for ECAM?


> +    void __iomem *base;
> +
> +    pci_sbdf_t sbdf_t = (pci_sbdf_t) sbdf ;
> +    unsigned int busn = sbdf_t.bus;
> +
> +    if ( busn < cfg->busn_start || busn > cfg->busn_end )

Genuine question: should it be busn >= cfg->busn_end ?  I don't know if
the range includes busn_end or not.


> +        return NULL;
> +
> +    busn -= cfg->busn_start;
> +    base = cfg->win + (busn << cfg->ops->bus_shift);
> +
> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
> +}

I understand that the arm32 part is not implemented and not part of this
series, that's fine. However if the plan is that arm32 will dynamically
map each bus individually, then I imagine this function will have an
ioremap in the arm32 version. Which means that we also need an
unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus
would be a NOP today for arm64, but I think it makes sense to have it if
we want the API to be generic.


> +/* ECAM ops */
> +const struct pci_ecam_ops pci_generic_ecam_ops = {
> +    .bus_shift  = 20,
> +    .pci_ops    = {
> +        .map_bus                = pci_ecam_map_bus,
> +        .read                   = pci_generic_config_read,
> +        .write                  = pci_generic_config_write,
> +    }
> +};
> +
> +/*
> + * 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-access.c b/xen/arch/arm/pci/pci-access.c
> index b938047c03..f39f6a3a38 100644
> --- a/xen/arch/arm/pci/pci-access.c
> +++ b/xen/arch/arm/pci/pci-access.c
> @@ -15,6 +15,59 @@
>   */
>  
>  #include <xen/pci.h>
> +#include <asm/io.h>
> +
> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                            uint32_t reg, uint32_t len, uint32_t *value)
> +{
> +    void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
> +    if (!addr) {
> +        *value = ~0;

Is this a standard error? If so, I think we should define it with a
macro (e.g. INVALID_PADDR).


> +        return -ENODEV;
> +    }
> +
> +    switch (len)
> +    {
> +    case 1:
> +        *value = readb(addr);
> +        break;
> +    case 2:
> +        *value = readw(addr);
> +        break;
> +    case 4:
> +        *value = readl(addr);
> +        break;
> +    default:
> +        BUG();

A BUG here is harsh because it could be potentially guest-triggered. An
ASSERT would be better.


> +    }
> +
> +    return 0;
> +}
> +
> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                            uint32_t reg, uint32_t len, uint32_t value)
> +{
> +    void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
> +    if (!addr)
> +        return -ENODEV;
> +
> +    switch (len)
> +    {
> +    case 1:
> +        writeb(value, addr);
> +        break;
> +    case 2:
> +        writew(value, addr);
> +        break;
> +    case 4:
> +        writel(value, addr);
> +        break;
> +    default:
> +        BUG();

Same here


> +    }
> +
> +    return 0;
> +}
>  
>  static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>                                  unsigned int len)
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 9dd9b02271..c582527e92 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -68,6 +68,7 @@ static void pci_ecam_free(struct pci_config_window *cfg)
>  }
>  
>  static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
> +                                              const struct pci_ecam_ops *ops,
>                                                int ecam_reg_idx)
>  {
>      int err;
> @@ -96,6 +97,7 @@ static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>  
>      cfg->phys_addr = addr;
>      cfg->size = size;
> +    cfg->ops = ops;
>  
>      /*
>       * On 64-bit systems, we do a single ioremap for the whole config space
> @@ -111,6 +113,13 @@ static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>      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:
> @@ -216,6 +225,7 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>  }
>  
>  int pci_host_common_probe(struct dt_device_node *dev,
> +                          const struct pci_ecam_ops *ops,
>                            int ecam_reg_idx)
>  {
>      struct pci_host_bridge *bridge;
> @@ -227,7 +237,7 @@ int pci_host_common_probe(struct dt_device_node *dev,
>          return -ENOMEM;
>  
>      /* Parse and map our Configuration Space windows */
> -    cfg = gen_pci_init(dev, ecam_reg_idx);
> +    cfg = gen_pci_init(dev, ops, ecam_reg_idx);
>      if ( !cfg )
>      {
>          err = -ENOMEM;
> @@ -236,6 +246,7 @@ int pci_host_common_probe(struct dt_device_node *dev,
>  
>      bridge->dt_node = dev;
>      bridge->sysdata = cfg;
> +    bridge->ops = &ops->pci_ops;
>      bridge->bus_start = cfg->busn_start;
>      bridge->bus_end = cfg->busn_end;
>  
> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
> index 13d0f7f999..2d652e8910 100644
> --- a/xen/arch/arm/pci/pci-host-generic.c
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -23,20 +23,24 @@
>  #include <asm/pci.h>
>  
>  static const struct dt_device_match gen_pci_dt_match[] = {
> -    { .compatible = "pci-host-ecam-generic" },
> +    { .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;
> +    const 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, 0);
> +    return pci_host_common_probe(dev, ops, 0);
>  }
>  
>  DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 58a51e724e..22866244d2 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -37,6 +37,7 @@ struct pci_config_window {
>      uint8_t         busn_start;
>      uint8_t         busn_end;
>      void __iomem    *win;
> +    const struct    pci_ecam_ops *ops;
>  };
>  
>  /*
> @@ -50,10 +51,41 @@ struct pci_host_bridge {
>      u8 bus_start;                    /* Bus start of this bridge. */
>      u8 bus_end;                      /* Bus end of this bridge. */
>      void *sysdata;                   /* Pointer to the config space window*/
> +    const struct pci_ops *ops;
>  };
>  
> +struct pci_ops {
> +    void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                             uint32_t offset);
> +    int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                uint32_t reg, uint32_t len, uint32_t *value);
> +    int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                 uint32_t reg, uint32_t len, uint32_t value);
> +};
> +
> +/*
> + * 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 *);

Is this last member of the struct needed/used?


> +};
> +
> +/* Default ECAM ops */
> +extern const struct pci_ecam_ops pci_generic_ecam_ops;
> +
>  int pci_host_common_probe(struct dt_device_node *dev,
> +                          const struct pci_ecam_ops *ops,
>                            int ecam_reg_idx);
> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                            uint32_t reg, uint32_t len, uint32_t *value);
> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                            uint32_t reg, uint32_t len, uint32_t value);
> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
> +                               uint32_t sbdf, uint32_t where);
> +
>  #else   /*!CONFIG_HAS_PCI*/
>  
>  struct arch_pci_dev { };
> -- 
> 2.17.1
> 

Hi Stefano,

> On 10 Sep 2021, at 12:21 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 19 Aug 2021, Rahul Singh wrote:
>> Add support for PCI ecam operations to access the PCI
>> configuration space.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> xen/arch/arm/pci/Makefile           |  1 +
>> xen/arch/arm/pci/ecam.c             | 63 +++++++++++++++++++++++++++++
>> xen/arch/arm/pci/pci-access.c       | 53 ++++++++++++++++++++++++
>> xen/arch/arm/pci/pci-host-common.c  | 13 +++++-
>> xen/arch/arm/pci/pci-host-generic.c |  8 +++-
>> xen/include/asm-arm/pci.h           | 32 +++++++++++++++
>> 6 files changed, 167 insertions(+), 3 deletions(-)
>> create mode 100644 xen/arch/arm/pci/ecam.c
>> 
>> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
>> index f3d97f859e..6f32fbbe67 100644
>> --- a/xen/arch/arm/pci/Makefile
>> +++ b/xen/arch/arm/pci/Makefile
>> @@ -2,3 +2,4 @@ obj-y += pci.o
>> obj-y += pci-access.o
>> obj-y += pci-host-generic.o
>> obj-y += pci-host-common.o
>> +obj-y += ecam.o
>> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
>> new file mode 100644
>> index 0000000000..91c691b41f
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/ecam.c
>> @@ -0,0 +1,63 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
>> + *
>> + * Based on Linux drivers/pci/ecam.c
>> + * Copyright 2016 Broadcom
>> + *
>> + * 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/pci.h>
>> +#include <xen/sched.h>
>> +
>> +/*
>> + * Function to implement the pci_ops ->map_bus method.
>> + */
>> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>> +                                      uint32_t sbdf, uint32_t where)
> 
> Code style: alignment
Ack.
> 
> 
>> +{
>> +    const struct pci_config_window *cfg = bridge->sysdata;
>> +    unsigned int devfn_shift = cfg->ops->bus_shift - 8;
> 
> Is it a guarantee that devfn_shift == bus_shift - 8, or is it just so
> for ECAM?

Yes it is guarantee that "devfn_shift == bus_shift - 8” as bus number will be 8 bit always.
bus_shift can be different based on vendor.

https://elixir.bootlin.com/linux/latest/source/drivers/pci/ecam.c#L172
https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-thunder-pem.c#L30

> 
> 
>> +    void __iomem *base;
>> +
>> +    pci_sbdf_t sbdf_t = (pci_sbdf_t) sbdf ;
>> +    unsigned int busn = sbdf_t.bus;
>> +
>> +    if ( busn < cfg->busn_start || busn > cfg->busn_end )
> 
> Genuine question: should it be busn >= cfg->busn_end ?  I don't know if
> the range includes busn_end or not.

cfg->bus_end includes the valid last bus number.  "busn > cfg->busn_end" is valid check 
to confirm if we are not accessing pci device outside the valid bus number.

> 
> 
>> +        return NULL;
>> +
>> +    busn -= cfg->busn_start;
>> +    base = cfg->win + (busn << cfg->ops->bus_shift);
>> +
>> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
>> +}
> 
> I understand that the arm32 part is not implemented and not part of this
> series, that's fine. However if the plan is that arm32 will dynamically
> map each bus individually, then I imagine this function will have an
> ioremap in the arm32 version. Which means that we also need an
> unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus
> would be a NOP today for arm64, but I think it makes sense to have it if
> we want the API to be generic.

As per my understanding we don’t need pci_ecam_unmap_bus(..) as I don’t see any use case to unmap the 
bus dynamically. We can add the support for per_bus_mapping for ARM32 once we implement arm32 part.
Let me know your view on this.

> 
> 
>> +/* ECAM ops */
>> +const struct pci_ecam_ops pci_generic_ecam_ops = {
>> +    .bus_shift  = 20,
>> +    .pci_ops    = {
>> +        .map_bus                = pci_ecam_map_bus,
>> +        .read                   = pci_generic_config_read,
>> +        .write                  = pci_generic_config_write,
>> +    }
>> +};
>> +
>> +/*
>> + * 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-access.c b/xen/arch/arm/pci/pci-access.c
>> index b938047c03..f39f6a3a38 100644
>> --- a/xen/arch/arm/pci/pci-access.c
>> +++ b/xen/arch/arm/pci/pci-access.c
>> @@ -15,6 +15,59 @@
>>  */
>> 
>> #include <xen/pci.h>
>> +#include <asm/io.h>
>> +
>> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
>> +                            uint32_t reg, uint32_t len, uint32_t *value)
>> +{
>> +    void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
>> +    if (!addr) {
>> +        *value = ~0;
> 
> Is this a standard error? If so, I think we should define it with a
> macro (e.g. INVALID_PADDR).

Ok.
> 
> 
>> +        return -ENODEV;
>> +    }
>> +
>> +    switch (len)
>> +    {
>> +    case 1:
>> +        *value = readb(addr);
>> +        break;
>> +    case 2:
>> +        *value = readw(addr);
>> +        break;
>> +    case 4:
>> +        *value = readl(addr);
>> +        break;
>> +    default:
>> +        BUG();
> 
> A BUG here is harsh because it could be potentially guest-triggered. An
> ASSERT would be better.

Ok.
> 
> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>> +                            uint32_t reg, uint32_t len, uint32_t value)
>> +{
>> +    void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
>> +    if (!addr)
>> +        return -ENODEV;
>> +
>> +    switch (len)
>> +    {
>> +    case 1:
>> +        writeb(value, addr);
>> +        break;
>> +    case 2:
>> +        writew(value, addr);
>> +        break;
>> +    case 4:
>> +        writel(value, addr);
>> +        break;
>> +    default:
>> +        BUG();
> 
> Same here
Ok.
> 
> 
>> +    }
>> +
>> +    return 0;
>> +}
>> 
>> static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>>                                 unsigned int len)
>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> index 9dd9b02271..c582527e92 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -68,6 +68,7 @@ static void pci_ecam_free(struct pci_config_window *cfg)
>> }
>> 
>> static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>> +                                              const struct pci_ecam_ops *ops,
>>                                               int ecam_reg_idx)
>> {
>>     int err;
>> @@ -96,6 +97,7 @@ static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>> 
>>     cfg->phys_addr = addr;
>>     cfg->size = size;
>> +    cfg->ops = ops;
>> 
>>     /*
>>      * On 64-bit systems, we do a single ioremap for the whole config space
>> @@ -111,6 +113,13 @@ static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>>     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:
>> @@ -216,6 +225,7 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>> }
>> 
>> int pci_host_common_probe(struct dt_device_node *dev,
>> +                          const struct pci_ecam_ops *ops,
>>                           int ecam_reg_idx)
>> {
>>     struct pci_host_bridge *bridge;
>> @@ -227,7 +237,7 @@ int pci_host_common_probe(struct dt_device_node *dev,
>>         return -ENOMEM;
>> 
>>     /* Parse and map our Configuration Space windows */
>> -    cfg = gen_pci_init(dev, ecam_reg_idx);
>> +    cfg = gen_pci_init(dev, ops, ecam_reg_idx);
>>     if ( !cfg )
>>     {
>>         err = -ENOMEM;
>> @@ -236,6 +246,7 @@ int pci_host_common_probe(struct dt_device_node *dev,
>> 
>>     bridge->dt_node = dev;
>>     bridge->sysdata = cfg;
>> +    bridge->ops = &ops->pci_ops;
>>     bridge->bus_start = cfg->busn_start;
>>     bridge->bus_end = cfg->busn_end;
>> 
>> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
>> index 13d0f7f999..2d652e8910 100644
>> --- a/xen/arch/arm/pci/pci-host-generic.c
>> +++ b/xen/arch/arm/pci/pci-host-generic.c
>> @@ -23,20 +23,24 @@
>> #include <asm/pci.h>
>> 
>> static const struct dt_device_match gen_pci_dt_match[] = {
>> -    { .compatible = "pci-host-ecam-generic" },
>> +    { .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;
>> +    const 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, 0);
>> +    return pci_host_common_probe(dev, ops, 0);
>> }
>> 
>> DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index 58a51e724e..22866244d2 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -37,6 +37,7 @@ struct pci_config_window {
>>     uint8_t         busn_start;
>>     uint8_t         busn_end;
>>     void __iomem    *win;
>> +    const struct    pci_ecam_ops *ops;
>> };
>> 
>> /*
>> @@ -50,10 +51,41 @@ struct pci_host_bridge {
>>     u8 bus_start;                    /* Bus start of this bridge. */
>>     u8 bus_end;                      /* Bus end of this bridge. */
>>     void *sysdata;                   /* Pointer to the config space window*/
>> +    const struct pci_ops *ops;
>> };
>> 
>> +struct pci_ops {
>> +    void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
>> +                             uint32_t offset);
>> +    int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
>> +                uint32_t reg, uint32_t len, uint32_t *value);
>> +    int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
>> +                 uint32_t reg, uint32_t len, uint32_t value);
>> +};
>> +
>> +/*
>> + * 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 *);
> 
> Is this last member of the struct needed/used?

Yes It will be used by some vendor specific board( N1SDP ). Please check below.
https://git.linaro.org/landing-teams/working/arm/n1sdp-pcie-quirk.git/commit/?id=04b7e76d0fe6481a803f58e54e008a1489d713a5
 
Regards,
Rahul

On Tue, 14 Sep 2021, Rahul Singh wrote:
> >> +        return NULL;
> >> +
> >> +    busn -= cfg->busn_start;
> >> +    base = cfg->win + (busn << cfg->ops->bus_shift);
> >> +
> >> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
> >> +}
> > 
> > I understand that the arm32 part is not implemented and not part of this
> > series, that's fine. However if the plan is that arm32 will dynamically
> > map each bus individually, then I imagine this function will have an
> > ioremap in the arm32 version. Which means that we also need an
> > unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus
> > would be a NOP today for arm64, but I think it makes sense to have it if
> > we want the API to be generic.
> 
> As per my understanding we don’t need pci_ecam_unmap_bus(..) as I don’t see any use case to unmap the 
> bus dynamically. We can add the support for per_bus_mapping for ARM32 once we implement arm32 part.
> Let me know your view on this.

In the patch titled "xen/arm: PCI host bridge discovery within XEN on
ARM" there is the following in-code comment:

* 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.


So I take it that on arm32 we don't have enough virtual address range
available, therefore we cannot ioremap the whole range. Instead, we'll
have to ioremap the config space of each bus individually.

I assumed that the idea was to call ioremap and iounmap dynamically,
otherwise the total amount of virtual address range required would still
be the same. I also thought that the dynamic mapping function, the one
which would end up calling ioremap on arm32, would be pci_ecam_map_bus.
If so, then we are missing the corresponding unmapping function, the one
that would call iounmap on arm32 and do nothing on arm64, called before
returning from pci_generic_config_read and pci_generic_config_write.

As always, I am not asking for the arm32 implementation, but if we are
introducing internal APIs, it would be good that they are consistent.



> >> @@ -50,10 +51,41 @@ struct pci_host_bridge {
> >>     u8 bus_start;                    /* Bus start of this bridge. */
> >>     u8 bus_end;                      /* Bus end of this bridge. */
> >>     void *sysdata;                   /* Pointer to the config space window*/
> >> +    const struct pci_ops *ops;
> >> };
> >> 
> >> +struct pci_ops {
> >> +    void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
> >> +                             uint32_t offset);
> >> +    int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
> >> +                uint32_t reg, uint32_t len, uint32_t *value);
> >> +    int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
> >> +                 uint32_t reg, uint32_t len, uint32_t value);
> >> +};
> >> +
> >> +/*
> >> + * 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 *);
> > 
> > Is this last member of the struct needed/used?
> 
> Yes It will be used by some vendor specific board( N1SDP ). Please check below.
> https://git.linaro.org/landing-teams/working/arm/n1sdp-pcie-quirk.git/commit/?id=04b7e76d0fe6481a803f58e54e008a1489d713a5

OK. I don't doubt that there might be bridge-specific initializations,
but we already have things like:

DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
.dt_match = gen_pci_dt_match,
.init = gen_pci_dt_init,
DT_DEVICE_END

The question is do we actually need two different vendor-specific init
functions? One is driven by device tree, e.g. ZynqMP is calling
gen_pci_dt_init. The other one is pci_ecam_ops->init, which is called
from the following chain:

DT_DEVICE_START -> gen_pci_dt_init -> pci_host_common_probe ->
gen_pci_init -> pci_generic_ecam_ops.init

What's the difference between gen_pci_dt_init and pci_ecam_ops.init in
terms of purpose?

I am happy to have pci_ecam_ops.init if it serves a different purpose
from DT_DEVICE_START.init. In that case we might want to add an in-code
comment so that an engineer would know where to add vendor-specific code
(whether to DT_DEVICE_START.init or to pci_ecam_ops.init).

Hi Stefano,

> On 15 Sep 2021, at 12:06 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 14 Sep 2021, Rahul Singh wrote:
>>>> +        return NULL;
>>>> +
>>>> +    busn -= cfg->busn_start;
>>>> +    base = cfg->win + (busn << cfg->ops->bus_shift);
>>>> +
>>>> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
>>>> +}
>>> 
>>> I understand that the arm32 part is not implemented and not part of this
>>> series, that's fine. However if the plan is that arm32 will dynamically
>>> map each bus individually, then I imagine this function will have an
>>> ioremap in the arm32 version. Which means that we also need an
>>> unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus
>>> would be a NOP today for arm64, but I think it makes sense to have it if
>>> we want the API to be generic.
>> 
>> As per my understanding we don’t need pci_ecam_unmap_bus(..) as I don’t see any use case to unmap the 
>> bus dynamically. We can add the support for per_bus_mapping for ARM32 once we implement arm32 part.
>> Let me know your view on this.
> 
> In the patch titled "xen/arm: PCI host bridge discovery within XEN on
> ARM" there is the following in-code comment:
> 
> * 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.
> 
> 
> So I take it that on arm32 we don't have enough virtual address range
> available, therefore we cannot ioremap the whole range. Instead, we'll
> have to ioremap the config space of each bus individually.

Yes you are right my understand is also same.
> 
> I assumed that the idea was to call ioremap and iounmap dynamically,
> otherwise the total amount of virtual address range required would still
> be the same.

As per my understanding for 32-bit we need per_bus mapping as we don’t have enough virtual address space in one chunk
but we can have virtual address space in different chunk.

I am not sure if we need to map/unmap the virtual address space for each read/write call. 
I just checked the Linux code[1]  and there also mapping is done once not for each read/write call.

[1]  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/ecam.c?id=ea4aae05974334e9837d86ff1cb716bad36b3ca8#n76

> I also thought that the dynamic mapping function, the one
> which would end up calling ioremap on arm32, would be pci_ecam_map_bus.
> If so, then we are missing the corresponding unmapping function, the one
> that would call iounmap on arm32 and do nothing on arm64, called before
> returning from pci_generic_config_read and pci_generic_config_write.
> 
> As always, I am not asking for the arm32 implementation, but if we are
> introducing internal APIs, it would be good that they are consistent.
> 
> 
> 
>>>> @@ -50,10 +51,41 @@ struct pci_host_bridge {
>>>>    u8 bus_start;                    /* Bus start of this bridge. */
>>>>    u8 bus_end;                      /* Bus end of this bridge. */
>>>>    void *sysdata;                   /* Pointer to the config space window*/
>>>> +    const struct pci_ops *ops;
>>>> };
>>>> 
>>>> +struct pci_ops {
>>>> +    void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
>>>> +                             uint32_t offset);
>>>> +    int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
>>>> +                uint32_t reg, uint32_t len, uint32_t *value);
>>>> +    int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
>>>> +                 uint32_t reg, uint32_t len, uint32_t value);
>>>> +};
>>>> +
>>>> +/*
>>>> + * 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 *);
>>> 
>>> Is this last member of the struct needed/used?
>> 
>> Yes It will be used by some vendor specific board( N1SDP ). Please check below.
>> https://git.linaro.org/landing-teams/working/arm/n1sdp-pcie-quirk.git/commit/?id=04b7e76d0fe6481a803f58e54e008a1489d713a5
> 
> OK. I don't doubt that there might be bridge-specific initializations,
> but we already have things like:
> 
> DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
> .dt_match = gen_pci_dt_match,
> .init = gen_pci_dt_init,
> DT_DEVICE_END
> 
> The question is do we actually need two different vendor-specific init
> functions? One is driven by device tree, e.g. ZynqMP is calling
> gen_pci_dt_init. The other one is pci_ecam_ops->init, which is called
> from the following chain:
> 
> DT_DEVICE_START -> gen_pci_dt_init -> pci_host_common_probe ->
> gen_pci_init -> pci_generic_ecam_ops.init
> 
> What's the difference between gen_pci_dt_init and pci_ecam_ops.init in
> terms of purpose?

Yes I also agree with you we don’t need two init function. I will modify the code like below.
DT_DEVICE_START ->pci_host_common_probe -> gen_pci_init -> pci_generic_ecam_ops.init

Regards,
Rahul
> 
> I am happy to have pci_ecam_ops.init if it serves a different purpose
> from DT_DEVICE_START.init. In that case we might want to add an in-code
> comment so that an engineer would know where to add vendor-specific code
> (whether to DT_DEVICE_START.init or to pci_ecam_ops.init).

On Wed, 15 Sep 2021, Rahul Singh wrote:
> > On 15 Sep 2021, at 12:06 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > On Tue, 14 Sep 2021, Rahul Singh wrote:
> >>>> +        return NULL;
> >>>> +
> >>>> +    busn -= cfg->busn_start;
> >>>> +    base = cfg->win + (busn << cfg->ops->bus_shift);
> >>>> +
> >>>> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
> >>>> +}
> >>> 
> >>> I understand that the arm32 part is not implemented and not part of this
> >>> series, that's fine. However if the plan is that arm32 will dynamically
> >>> map each bus individually, then I imagine this function will have an
> >>> ioremap in the arm32 version. Which means that we also need an
> >>> unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus
> >>> would be a NOP today for arm64, but I think it makes sense to have it if
> >>> we want the API to be generic.
> >> 
> >> As per my understanding we don’t need pci_ecam_unmap_bus(..) as I don’t see any use case to unmap the 
> >> bus dynamically. We can add the support for per_bus_mapping for ARM32 once we implement arm32 part.
> >> Let me know your view on this.
> > 
> > In the patch titled "xen/arm: PCI host bridge discovery within XEN on
> > ARM" there is the following in-code comment:
> > 
> > * 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.
> > 
> > 
> > So I take it that on arm32 we don't have enough virtual address range
> > available, therefore we cannot ioremap the whole range. Instead, we'll
> > have to ioremap the config space of each bus individually.
> 
> Yes you are right my understand is also same.
> > 
> > I assumed that the idea was to call ioremap and iounmap dynamically,
> > otherwise the total amount of virtual address range required would still
> > be the same.
> 
> As per my understanding for 32-bit we need per_bus mapping as we don’t have enough virtual address space in one chunk
> but we can have virtual address space in different chunk.

Interesting. I would have assumed that the sum of all the individual
smaller ioremaps would still be equal to one big ioremap. Maybe for
Linux is different, but I don't think that many smaller ioremaps would
buy us very much in Xen because it is the total ioremap virtual space
that is too small. Or am I missing something?


> I am not sure if we need to map/unmap the virtual address space for each read/write call. 
> I just checked the Linux code[1]  and there also mapping is done once not for each read/write call.

So my guess is that for arm32 we would have to resort to dynamic
map/unmap for each read/write call, unless there is a trick with the
individual smaller ioremaps that I haven't spotted (e.g. maybe something
doesn't get mapped that way?)

That said, given that we are uncertain about this and the arm32
implementation is nowhere close, I think that we are OK to continue like
this for this series. Maybe you could add a couple of sentences to the
in-code comment so that if somebody wants to jump in and implement
arm32 support they would know where to start.

Hi Stefano,

> On 15 Sep 2021, at 9:45 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 15 Sep 2021, Rahul Singh wrote:
>>> On 15 Sep 2021, at 12:06 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> On Tue, 14 Sep 2021, Rahul Singh wrote:
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    busn -= cfg->busn_start;
>>>>>> +    base = cfg->win + (busn << cfg->ops->bus_shift);
>>>>>> +
>>>>>> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
>>>>>> +}
>>>>> 
>>>>> I understand that the arm32 part is not implemented and not part of this
>>>>> series, that's fine. However if the plan is that arm32 will dynamically
>>>>> map each bus individually, then I imagine this function will have an
>>>>> ioremap in the arm32 version. Which means that we also need an
>>>>> unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus
>>>>> would be a NOP today for arm64, but I think it makes sense to have it if
>>>>> we want the API to be generic.
>>>> 
>>>> As per my understanding we don’t need pci_ecam_unmap_bus(..) as I don’t see any use case to unmap the 
>>>> bus dynamically. We can add the support for per_bus_mapping for ARM32 once we implement arm32 part.
>>>> Let me know your view on this.
>>> 
>>> In the patch titled "xen/arm: PCI host bridge discovery within XEN on
>>> ARM" there is the following in-code comment:
>>> 
>>> * 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.
>>> 
>>> 
>>> So I take it that on arm32 we don't have enough virtual address range
>>> available, therefore we cannot ioremap the whole range. Instead, we'll
>>> have to ioremap the config space of each bus individually.
>> 
>> Yes you are right my understand is also same.
>>> 
>>> I assumed that the idea was to call ioremap and iounmap dynamically,
>>> otherwise the total amount of virtual address range required would still
>>> be the same.
>> 
>> As per my understanding for 32-bit we need per_bus mapping as we don’t have enough virtual address space in one chunk
>> but we can have virtual address space in different chunk.
> 
> Interesting. I would have assumed that the sum of all the individual
> smaller ioremaps would still be equal to one big ioremap. Maybe for
> Linux is different, but I don't think that many smaller ioremaps would
> buy us very much in Xen because it is the total ioremap virtual space
> that is too small. Or am I missing something?
> 
> 
>> I am not sure if we need to map/unmap the virtual address space for each read/write call. 
>> I just checked the Linux code[1]  and there also mapping is done once not for each read/write call.
> 
> So my guess is that for arm32 we would have to resort to dynamic
> map/unmap for each read/write call, unless there is a trick with the
> individual smaller ioremaps that I haven't spotted (e.g. maybe something
> doesn't get mapped that way?)
> 
> That said, given that we are uncertain about this and the arm32
> implementation is nowhere close, I think that we are OK to continue like
> this for this series. Maybe you could add a couple of sentences to the
> in-code comment so that if somebody wants to jump in and implement
> arm32 support they would know where to start.

I am ok with both ways adding comment in code to explain or implement the pci_ecam_add_bus(..) and pci_ecam_remove_bus() like Linux [1] and 
we can call those function in pci_read()/pci_write.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/ecam.c#n126


Regards,
Rahul

Hi Rahul,

On 19/08/2021 13:02, Rahul Singh wrote:
> Add support for PCI ecam operations to access the PCI
> configuration space.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>   xen/arch/arm/pci/Makefile           |  1 +
>   xen/arch/arm/pci/ecam.c             | 63 +++++++++++++++++++++++++++++
>   xen/arch/arm/pci/pci-access.c       | 53 ++++++++++++++++++++++++
>   xen/arch/arm/pci/pci-host-common.c  | 13 +++++-
>   xen/arch/arm/pci/pci-host-generic.c |  8 +++-
>   xen/include/asm-arm/pci.h           | 32 +++++++++++++++
>   6 files changed, 167 insertions(+), 3 deletions(-)
>   create mode 100644 xen/arch/arm/pci/ecam.c
> 
> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
> index f3d97f859e..6f32fbbe67 100644
> --- a/xen/arch/arm/pci/Makefile
> +++ b/xen/arch/arm/pci/Makefile
> @@ -2,3 +2,4 @@ obj-y += pci.o
>   obj-y += pci-access.o
>   obj-y += pci-host-generic.o
>   obj-y += pci-host-common.o
> +obj-y += ecam.o
> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
> new file mode 100644
> index 0000000000..91c691b41f
> --- /dev/null
> +++ b/xen/arch/arm/pci/ecam.c
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (C) 2021 Arm Ltd.
> + *
> + * Based on Linux drivers/pci/ecam.c
> + * Copyright 2016 Broadcom
> + *
> + * 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/pci.h>
> +#include <xen/sched.h>
> +
> +/*
> + * Function to implement the pci_ops ->map_bus method.
> + */
> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
> +                                      uint32_t sbdf, uint32_t where)
> +{
> +    const struct pci_config_window *cfg = bridge->sysdata;
> +    unsigned int devfn_shift = cfg->ops->bus_shift - 8;
> +    void __iomem *base;
> +
> +    pci_sbdf_t sbdf_t = (pci_sbdf_t) sbdf ;

AFAICT, pci_sbdf is an union between a 32-bit and a structure. So please 
don't use the cast and use the 32-bit field to assign the value.

Also, there is an extra space before ';'.

> +    unsigned int busn = sbdf_t.bus;
> +
> +    if ( busn < cfg->busn_start || busn > cfg->busn_end )
> +        return NULL;
> +
> +    busn -= cfg->busn_start;
> +    base = cfg->win + (busn << cfg->ops->bus_shift);
> +
> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;

How about using PCI_DEVFN2(sbdf)? This would allow you to drop the use 
of sbdf_t completely (sbdf_t.bus could be replaced with PCI_BUS(sdbf)).

> +}
> +
> +/* ECAM ops */
> +const struct pci_ecam_ops pci_generic_ecam_ops = {
> +    .bus_shift  = 20,
> +    .pci_ops    = {
> +        .map_bus                = pci_ecam_map_bus,
> +        .read                   = pci_generic_config_read,
> +        .write                  = pci_generic_config_write,
> +    }
> +};
> +
> +/*
> + * 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-access.c b/xen/arch/arm/pci/pci-access.c
> index b938047c03..f39f6a3a38 100644
> --- a/xen/arch/arm/pci/pci-access.c
> +++ b/xen/arch/arm/pci/pci-access.c
> @@ -15,6 +15,59 @@
>    */
>   
>   #include <xen/pci.h>
> +#include <asm/io.h>
> +
> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                            uint32_t reg, uint32_t len, uint32_t *value)
> +{
> +    void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);

Please add a newline here.

> +    if (!addr) {

You seem to use a mix of Xen and Linux coding style. If the file 
contains mostly Xen code, then we should use the former.

> +        *value = ~0;
> +        return -ENODEV;
> +    }
> +
> +    switch (len)
> +    {
> +    case 1:
> +        *value = readb(addr);
> +        break;
> +    case 2:
> +        *value = readw(addr);
> +        break;
> +    case 4:
> +        *value = readl(addr);
> +        break;
> +    default:
> +        BUG();
> +    }
> +
> +    return 0;
> +}
> +
> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                            uint32_t reg, uint32_t len, uint32_t value)
> +{
> +    void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
> +    if (!addr)
> +        return -ENODEV;
> +
> +    switch (len)
> +    {
> +    case 1:
> +        writeb(value, addr);
> +        break;
> +    case 2:
> +        writew(value, addr);
> +        break;
> +    case 4:
> +        writel(value, addr);
> +        break;
> +    default:
> +        BUG();
> +    }
> +
> +    return 0;
> +}
>   
>   static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>                                   unsigned int len)
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 9dd9b02271..c582527e92 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -68,6 +68,7 @@ static void pci_ecam_free(struct pci_config_window *cfg)
>   }
>   
>   static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
> +                                              const struct pci_ecam_ops *ops,
>                                                 int ecam_reg_idx)
>   {
>       int err;
> @@ -96,6 +97,7 @@ static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>   
>       cfg->phys_addr = addr;
>       cfg->size = size;
> +    cfg->ops = ops;
>   
>       /*
>        * On 64-bit systems, we do a single ioremap for the whole config space
> @@ -111,6 +113,13 @@ static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>       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:
> @@ -216,6 +225,7 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>   }
>   
>   int pci_host_common_probe(struct dt_device_node *dev,
> +                          const struct pci_ecam_ops *ops,
>                             int ecam_reg_idx)
>   {
>       struct pci_host_bridge *bridge;
> @@ -227,7 +237,7 @@ int pci_host_common_probe(struct dt_device_node *dev,
>           return -ENOMEM;
>   
>       /* Parse and map our Configuration Space windows */
> -    cfg = gen_pci_init(dev, ecam_reg_idx);
> +    cfg = gen_pci_init(dev, ops, ecam_reg_idx);
>       if ( !cfg )
>       {
>           err = -ENOMEM;
> @@ -236,6 +246,7 @@ int pci_host_common_probe(struct dt_device_node *dev,
>   
>       bridge->dt_node = dev;
>       bridge->sysdata = cfg;
> +    bridge->ops = &ops->pci_ops;
>       bridge->bus_start = cfg->busn_start;
>       bridge->bus_end = cfg->busn_end;
>   
> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
> index 13d0f7f999..2d652e8910 100644
> --- a/xen/arch/arm/pci/pci-host-generic.c
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -23,20 +23,24 @@
>   #include <asm/pci.h>
>   
>   static const struct dt_device_match gen_pci_dt_match[] = {
> -    { .compatible = "pci-host-ecam-generic" },
> +    { .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;
> +    const 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, 0);
> +    return pci_host_common_probe(dev, ops, 0);
>   }
>   
>   DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 58a51e724e..22866244d2 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -37,6 +37,7 @@ struct pci_config_window {
>       uint8_t         busn_start;
>       uint8_t         busn_end;
>       void __iomem    *win;
> +    const struct    pci_ecam_ops *ops;
>   };
>   
>   /*
> @@ -50,10 +51,41 @@ struct pci_host_bridge {
>       u8 bus_start;                    /* Bus start of this bridge. */
>       u8 bus_end;                      /* Bus end of this bridge. */
>       void *sysdata;                   /* Pointer to the config space window*/
> +    const struct pci_ops *ops;
>   };
>   
> +struct pci_ops {
> +    void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                             uint32_t offset);
> +    int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                uint32_t reg, uint32_t len, uint32_t *value);
> +    int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                 uint32_t reg, uint32_t len, uint32_t value);
> +};
> +
> +/*
> + * 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 *);
> +};
> +
> +/* Default ECAM ops */
> +extern const struct pci_ecam_ops pci_generic_ecam_ops;
> +
>   int pci_host_common_probe(struct dt_device_node *dev,
> +                          const struct pci_ecam_ops *ops,
>                             int ecam_reg_idx);
> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                            uint32_t reg, uint32_t len, uint32_t *value);
> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> +                            uint32_t reg, uint32_t len, uint32_t value);
> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
> +                               uint32_t sbdf, uint32_t where);
> +
>   #else   /*!CONFIG_HAS_PCI*/
>   
>   struct arch_pci_dev { };
> 

Cheers,

-- 
Julien Grall

Hi Julien,

> On 9 Sep 2021, at 12:32 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 19/08/2021 13:02, Rahul Singh wrote:
>> Add support for PCI ecam operations to access the PCI
>> configuration space.
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>>  xen/arch/arm/pci/Makefile           |  1 +
>>  xen/arch/arm/pci/ecam.c             | 63 +++++++++++++++++++++++++++++
>>  xen/arch/arm/pci/pci-access.c       | 53 ++++++++++++++++++++++++
>>  xen/arch/arm/pci/pci-host-common.c  | 13 +++++-
>>  xen/arch/arm/pci/pci-host-generic.c |  8 +++-
>>  xen/include/asm-arm/pci.h           | 32 +++++++++++++++
>>  6 files changed, 167 insertions(+), 3 deletions(-)
>>  create mode 100644 xen/arch/arm/pci/ecam.c
>> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
>> index f3d97f859e..6f32fbbe67 100644
>> --- a/xen/arch/arm/pci/Makefile
>> +++ b/xen/arch/arm/pci/Makefile
>> @@ -2,3 +2,4 @@ obj-y += pci.o
>>  obj-y += pci-access.o
>>  obj-y += pci-host-generic.o
>>  obj-y += pci-host-common.o
>> +obj-y += ecam.o
>> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
>> new file mode 100644
>> index 0000000000..91c691b41f
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/ecam.c
>> @@ -0,0 +1,63 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
>> + *
>> + * Based on Linux drivers/pci/ecam.c
>> + * Copyright 2016 Broadcom
>> + *
>> + * 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/pci.h>
>> +#include <xen/sched.h>
>> +
>> +/*
>> + * Function to implement the pci_ops ->map_bus method.
>> + */
>> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>> +                                      uint32_t sbdf, uint32_t where)
>> +{
>> +    const struct pci_config_window *cfg = bridge->sysdata;
>> +    unsigned int devfn_shift = cfg->ops->bus_shift - 8;
>> +    void __iomem *base;
>> +
>> +    pci_sbdf_t sbdf_t = (pci_sbdf_t) sbdf ;
> 
> AFAICT, pci_sbdf is an union between a 32-bit and a structure. So please don't use the cast and use the 32-bit field to assign the value.
> 
> Also, there is an extra space before ';'.

Ok. As per your below comment I will remove the sbdf_t completely.
> 
> 
>> +    unsigned int busn = sbdf_t.bus;
>> +
>> +    if ( busn < cfg->busn_start || busn > cfg->busn_end )
>> +        return NULL;
>> +
>> +    busn -= cfg->busn_start;
>> +    base = cfg->win + (busn << cfg->ops->bus_shift);
>> +
>> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
> 
> How about using PCI_DEVFN2(sbdf)? This would allow you to drop the use of sbdf_t completely (sbdf_t.bus could be replaced with PCI_BUS(sdbf)).
Make sense. I will modify as per your request.
> 
>> +}
>> +
>> +/* ECAM ops */
>> +const struct pci_ecam_ops pci_generic_ecam_ops = {
>> +    .bus_shift  = 20,
>> +    .pci_ops    = {
>> +        .map_bus                = pci_ecam_map_bus,
>> +        .read                   = pci_generic_config_read,
>> +        .write                  = pci_generic_config_write,
>> +    }
>> +};
>> +
>> +/*
>> + * 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-access.c b/xen/arch/arm/pci/pci-access.c
>> index b938047c03..f39f6a3a38 100644
>> --- a/xen/arch/arm/pci/pci-access.c
>> +++ b/xen/arch/arm/pci/pci-access.c
>> @@ -15,6 +15,59 @@
>>   */
>>    #include <xen/pci.h>
>> +#include <asm/io.h>
>> +
>> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
>> +                            uint32_t reg, uint32_t len, uint32_t *value)
>> +{
>> +    void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
> 
> Please add a newline here.

Ack. 
> 
>> +    if (!addr) {
> 
> You seem to use a mix of Xen and Linux coding style. If the file contains mostly Xen code, then we should use the former.

Ok. I will fix the coding style in next version.
 
Regards,
Rahul