[PATCH v5 0/6] xen: arm: Parse PCI DT nodes' ranges and interrupt-map

[PATCH v5 0/6] xen: arm: Parse PCI DT nodes' ranges and interrupt-map

[Ian Campbell]

This series adds parsing of the DT ranges and interrupt-map properties
for PCI devices, these contain the MMIOs and IRQs used by children on
the bus. This replaces the specific mapping stuff on xgene.

Since last time I've added the needed call to iomem_permit_access and
added a patch to consolidate some now duplicated code. (Another patch
was committed last time around, so the # of patches is the same).

I only tested on xgene this time around since the changes were pretty
minor (and I'm lazy on Fridays...)

Ian.

[PATCH v5 1/6] xen: dt: add dt_for_each_irq_map helper

[Ian Campbell]

This function iterates over a nodes interrupt-map property and calls a
callback for each interrupt. For now it only supplies the translated
IRQ since my use case has no need of e.g. child unit address. These
can be added as needed by any future users.

This follows much the same logic as dt_irq_map_raw when parsing the
interrupt-map, but doesn't walk up the tree doing the actual
translation and it iterates over all entries instead of just looking
for the first match.

I looked into refactoring dt_irq_map_raw but I couldn't find a way
which I was confident in, plus I was reluctant to diverge from the
Linux roots of this function any further.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>

---
v4: Pass a dt_irq not a dt_irq_raw to the callback.
---
 xen/common/device_tree.c      |  148 +++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h |   12 ++++
 2 files changed, 160 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 31f169b..67b1758 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -829,6 +829,154 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
     return (psize / onesize);
 }
 
+int dt_for_each_irq_map(const struct dt_device_node *dev,
+                        int (*cb)(const struct dt_device_node *,
+                                  const struct dt_irq *,
+                                  void *),
+                        void *data)
+{
+    const struct dt_device_node *ipar, *tnode, *old = NULL;
+    const __be32 *tmp, *imap;
+    u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0;
+    u32 imaplen;
+    int i, ret;
+
+    struct dt_raw_irq dt_raw_irq;
+    struct dt_irq dt_irq;
+
+    dt_dprintk("%s: par=%s cb=%p data=%p\n", __func__,
+               dev->full_name, cb, data);
+
+    ipar = dev;
+
+    /* First get the #interrupt-cells property of the current cursor
+     * that tells us how to interpret the passed-in intspec. If there
+     * is none, we are nice and just walk up the tree
+     */
+    do {
+        tmp = dt_get_property(ipar, "#interrupt-cells", NULL);
+        if ( tmp != NULL )
+        {
+            intsize = be32_to_cpu(*tmp);
+            break;
+        }
+        tnode = ipar;
+        ipar = dt_irq_find_parent(ipar);
+    } while ( ipar );
+    if ( ipar == NULL )
+    {
+        dt_dprintk(" -> no parent found !\n");
+        goto fail;
+    }
+
+    dt_dprintk("%s: ipar=%s, size=%d\n", __func__, ipar->full_name, intsize);
+
+    if ( intsize > DT_MAX_IRQ_SPEC )
+    {
+        dt_dprintk(" -> too many irq specifier cells\n");
+        goto fail;
+    }
+
+    /* Look for this #address-cells. We have to implement the old linux
+     * trick of looking for the parent here as some device-trees rely on it
+     */
+    old = ipar;
+    do {
+        tmp = dt_get_property(old, "#address-cells", NULL);
+        tnode = dt_get_parent(old);
+        old = tnode;
+    } while ( old && tmp == NULL );
+
+    old = NULL;
+    addrsize = (tmp == NULL) ? 2 : be32_to_cpu(*tmp);
+
+    dt_dprintk(" -> addrsize=%d\n", addrsize);
+
+    /* Now look for an interrupt-map */
+    imap = dt_get_property(dev, "interrupt-map", &imaplen);
+    /* No interrupt map, check for an interrupt parent */
+    if ( imap == NULL )
+    {
+        dt_dprintk(" -> no map, ignoring\n");
+        goto fail;
+    }
+    imaplen /= sizeof(u32);
+
+    /* Parse interrupt-map */
+    while ( imaplen > (addrsize + intsize + 1) )
+    {
+        /* skip child unit address and child interrupt specifier */
+        imap += addrsize + intsize;
+        imaplen -= addrsize + intsize;
+
+        /* Get the interrupt parent */
+        ipar = dt_find_node_by_phandle(be32_to_cpup(imap));
+        imap++;
+        --imaplen;
+
+        /* Check if not found */
+        if ( ipar == NULL )
+        {
+            dt_dprintk(" -> imap parent not found !\n");
+            goto fail;
+        }
+
+        dt_dprintk(" -> ipar %s\n", dt_node_name(ipar));
+
+        /* Get #interrupt-cells and #address-cells of new
+         * parent
+         */
+        tmp = dt_get_property(ipar, "#interrupt-cells", NULL);
+        if ( tmp == NULL )
+        {
+            dt_dprintk(" -> parent lacks #interrupt-cells!\n");
+            goto fail;
+        }
+        pintsize = be32_to_cpu(*tmp);
+        tmp = dt_get_property(ipar, "#address-cells", NULL);
+        paddrsize = (tmp == NULL) ? 0 : be32_to_cpu(*tmp);
+
+        dt_dprintk(" -> pintsize=%d, paddrsize=%d\n",
+                   pintsize, paddrsize);
+
+        /* Check for malformed properties */
+        if ( imaplen < (paddrsize + pintsize) )
+            goto fail;
+
+        imap += paddrsize;
+        imaplen -= paddrsize;
+
+        dt_raw_irq.controller = ipar;
+        dt_raw_irq.size = pintsize;
+        for ( i = 0; i < pintsize; i++ )
+            dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1);
+
+        ret = dt_irq_translate(&dt_raw_irq, &dt_irq);
+        if ( ret < 0 )
+        {
+            dt_dprintk(" -> failed to translate IRQ: %d\n", ret);
+            return ret;
+        }
+
+        ret = cb(dev, &dt_irq, data);
+        if ( ret < 0 )
+        {
+            dt_dprintk(" -> callback failed=%d\n", ret);
+            return ret;
+        }
+
+        imap += pintsize;
+        imaplen -= pintsize;
+
+        dt_dprintk(" -> imaplen=%d\n", imaplen);
+    }
+
+    return 0;
+
+fail:
+    return -EINVAL;
+}
+
 /**
  * dt_irq_map_raw - Low level interrupt tree parsing
  * @parent:     the device interrupt parent
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index e187780..957f96e 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -542,6 +542,18 @@ int dt_device_get_raw_irq(const struct dt_device_node *device,
 int dt_irq_translate(const struct dt_raw_irq *raw, struct dt_irq *out_irq);
 
 /**
+ * dt_for_each_irq_map - Iterate over a nodes interrupt-map property
+ * @dev: The node whose interrupt-map property should be iterated over
+ * @cb: Call back to call for each entry
+ * @data: Caller data passed to callback
+ */
+int dt_for_each_irq_map(const struct dt_device_node *dev,
+                        int (*cb)(const struct dt_device_node *,
+                                  const struct dt_irq *,
+                                  void *),
+                        void *data);
+
+/**
  * dt_n_size_cells - Helper to retrieve the number of cell for the size
  * @np: node to get the value
  *
-- 
1.7.10.4

[PATCH v5 2/6] xen: dt: add dt_for_each_range helper

[Ian Campbell]

This function iterates over a node's ranges property and calls a
callback for each region. For now it only supplies the MMIO range (in
terms of CPU addresses, i.e. already translated).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>
---
 xen/common/device_tree.c      |   85 +++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h |   12 ++++++
 2 files changed, 97 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 67b1758..1942372 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -722,6 +722,91 @@ int dt_device_get_address(const struct dt_device_node *dev, unsigned int index,
     return 0;
 }
 
+
+int dt_for_each_range(const struct dt_device_node *dev,
+                      int (*cb)(const struct dt_device_node *,
+                                u64 addr, u64 length,
+                                void *),
+                      void *data)
+{
+    const struct dt_device_node *parent = NULL;
+    const struct dt_bus *bus, *pbus;
+    const __be32 *ranges;
+    __be32 addr[DT_MAX_ADDR_CELLS];
+    unsigned int rlen;
+    int na, ns, pna, pns, rone;
+
+    bus = dt_match_bus(dev);
+    if ( !bus )
+        return 0; /* device is not a bus */
+
+    parent = dt_get_parent(dev);
+    if ( parent == NULL )
+        return -EINVAL;
+
+    ranges = dt_get_property(dev, "ranges", &rlen);
+    if ( ranges == NULL )
+    {
+        printk(XENLOG_ERR "DT: no ranges; cannot enumerate\n");
+        return -EINVAL;
+    }
+    if ( rlen == 0 ) /* Nothing to do */
+        return 0;
+
+    bus->count_cells(dev, &na, &ns);
+    if ( !DT_CHECK_COUNTS(na, ns) )
+    {
+        printk(XENLOG_ERR "dt_parse: Bad cell count for device %s\n",
+                  dev->full_name);
+        return -EINVAL;
+    }
+
+    pbus = dt_match_bus(parent);
+    if ( pbus == NULL )
+    {
+        printk("DT: %s is not a valid bus\n", parent->full_name);
+        return -EINVAL;
+    }
+
+    pbus->count_cells(dev, &pna, &pns);
+    if ( !DT_CHECK_COUNTS(pna, pns) )
+    {
+        printk(XENLOG_ERR "dt_parse: Bad cell count for parent %s\n",
+               dev->full_name);
+        return -EINVAL;
+    }
+
+    /* Now walk through the ranges */
+    rlen /= 4;
+    rone = na + pna + ns;
+
+    dt_dprintk("%s: dev=%s, bus=%s, parent=%s, rlen=%d, rone=%d\n",
+               __func__,
+               dt_node_name(dev), bus->name,
+               dt_node_name(parent), rlen, rone);
+
+    for ( ; rlen >= rone; rlen -= rone, ranges += rone )
+    {
+        u64 a, s;
+        int ret;
+
+        memcpy(addr, ranges + na, 4 * pna);
+
+        a = __dt_translate_address(dev, addr, "ranges");
+        s = dt_read_number(ranges + na + pna, ns);
+
+        ret = cb(dev, a, s, data);
+        if ( ret < 0 )
+        {
+            dt_dprintk(" -> callback failed=%d\n", ret);
+            return ret;
+        }
+
+    }
+
+    return 0;
+}
+
 /**
  * dt_find_node_by_phandle - Find a node given a phandle
  * @handle: phandle of the node to find
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 957f96e..46c5ba8 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -554,6 +554,18 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
                         void *data);
 
 /**
+ * dt_for_each_range - Iterate over a nodes ranges property
+ * @dev: The node whose interrupt-map property should be iterated over
+ * @cb: Call back to call for each entry
+ * @data: Caller data passed to callback
+ */
+int dt_for_each_range(const struct dt_device_node *dev,
+                      int (*cb)(const struct dt_device_node *,
+                                u64 addr, u64 length,
+                                void *),
+                      void *data);
+
+/**
  * dt_n_size_cells - Helper to retrieve the number of cell for the size
  * @np: node to get the value
  *
-- 
1.7.10.4

[PATCH v5 3/6] xen: arm: drop redundant extra call to vgic_reserve_virq

[Ian Campbell]

This is only needed if we are giving the IRQ to dom0 (as opposed to
setting it up for passthrough due to xen,passthrough property). There
is already a call to vgic_reserve_virq inside the if ( need_mapping ),
so drop this one.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>

---
v4: New patch
---
 xen/arch/arm/domain_build.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 04460b0..980a2a3 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1024,12 +1024,6 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
         irq = res;
 
         DPRINT("irq %u = %u\n", i, irq);
-        /*
-         * Checking the return of vgic_reserve_virq is not
-         * necessary. It should not fail except when we try to map
-         * the IRQ twice. This can legitimately happen if the IRQ is shared
-         */
-        vgic_reserve_virq(d, irq);
 
         res = irq_permit_access(d, irq);
         if ( res )
-- 
1.7.10.4

[PATCH v5 4/6] xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.

[Ian Campbell]

This uses the dt_for_each_{irq_map,range} helpers to map the interrupt
and child MMIO regions to dom0. Since PCI busses are enumerable these
resources may not be otherwise described in the DT (although they can
be).

Although PCI is the only bus we handle this way the code should be
generic enough to apply to similar buses in the future.

This replaces the xgene specific mapping. Tested on Mustang and on a
model with a PCI virtio controller.

This patch doesn't stop recursing when it finds such a node, since
double mapping these resources if they do happen to be described is
(or should be) harmless

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: This is essentially a complete reworking, which actually parses
things properly (obeying #{address,size,interrupt}-cells on the
appriopriate nodes) and includes handling of interrupt-map too.

v3: Use dt_for_each_ranges and refactor handling into
    handle_device_children.

    Retitled from "xen: arm: handle PCI DT node ranges and
    interrupt-map properties" and rewrote much of the commit message.

    This change also essentially obsoleted any discussion about
    logging unhandled buses since it no longer makes sense (or at
    least there is nothing convenient to hang it off)

v4: dt_for_each_irq_map now provides the translated IRQ, so adjust
    accordingly.

    Call irq_permit_access to allow dom0 to passthrough the device to
    another domain.

    Only call route_irq_to_guest, map_mmio_regions and (newly added
    call to) vgic_reserve_virq if the device is not marked as reserved
    for passthrough.

v5: Call iomem_permit_access too
---
 xen/arch/arm/domain_build.c          |  127 ++++++++++++++++++++++++++++++
 xen/arch/arm/platforms/xgene-storm.c |  143 ----------------------------------
 2 files changed, 127 insertions(+), 143 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 980a2a3..22b4c04 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -954,6 +954,129 @@ static int make_timer_node(const struct domain *d, void *fdt,
     return res;
 }
 
+static int map_interrupt_to_domain(const struct dt_device_node *dev,
+                                   const struct dt_irq *dt_irq,
+                                   void *data)
+{
+    struct domain *d = data;
+    bool_t need_mapping = !dt_device_for_passthrough(dev);
+    unsigned int irq = dt_irq->irq;
+    int res;
+
+    if ( irq < NR_LOCAL_IRQS )
+    {
+        printk(XENLOG_ERR "%s: IRQ%"PRId32" is not a SPI\n",
+               dt_node_name(dev), irq);
+        return -EINVAL;
+    }
+
+    /* Setup the IRQ type */
+    res = irq_set_spi_type(irq, dt_irq->type);
+    if ( res )
+    {
+        printk(XENLOG_ERR
+               "%s: Unable to setup IRQ%"PRId32" to dom%d\n",
+               dt_node_name(dev), irq, d->domain_id);
+        return res;
+    }
+
+    res = irq_permit_access(d, irq);
+    if ( res )
+    {
+        printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
+               d->domain_id, irq);
+        return res;
+    }
+
+    if ( need_mapping )
+    {
+        /*
+         * Checking the return of vgic_reserve_virq is not
+         * necessary. It should not fail except when we try to map
+         * the IRQ twice. This can legitimately happen if the IRQ is shared
+         */
+        vgic_reserve_virq(d, irq);
+
+        res = route_irq_to_guest(d, irq, irq, dt_node_name(dev));
+        if ( res < 0 )
+        {
+            printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
+                   irq, d->domain_id);
+            return res;
+        }
+    }
+
+    DPRINT("  - IRQ: %u\n", irq);
+
+    return 0;
+}
+
+static int map_range_to_domain(const struct dt_device_node *dev,
+                               u64 addr, u64 len,
+                               void *data)
+{
+    struct domain *d = data;
+    bool_t need_mapping = !dt_device_for_passthrough(dev);
+    int res;
+
+    res = iomem_permit_access(d, paddr_to_pfn(addr & PAGE_MASK),
+                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
+    if ( res )
+    {
+        printk(XENLOG_ERR "Unable to permit to dom%d access to"
+               " 0x%"PRIx64" - 0x%"PRIx64"\n",
+               d->domain_id,
+               addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
+        return res;
+    }
+
+    if ( need_mapping )
+    {
+        res = map_mmio_regions(d,
+                               paddr_to_pfn(addr & PAGE_MASK),
+                               DIV_ROUND_UP(len, PAGE_SIZE),
+                               paddr_to_pfn(addr & PAGE_MASK));
+        if ( res < 0 )
+        {
+            printk(XENLOG_ERR "Unable to map 0x%"PRIx64
+                   " - 0x%"PRIx64" in domain %d\n",
+                   addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
+                   d->domain_id);
+            return res;
+        }
+    }
+
+    DPRINT("  - MMIO: %010"PRIx64" - %010"PRIx64"\n", addr, addr + len);
+
+    return 0;
+}
+
+/*
+ * For a node which describes a discoverable bus (such as a PCI bus)
+ * then we may need to perform additional mappings in order to make
+ * the child resources available to domain 0.
+ */
+static int map_device_children(struct domain *d,
+                               const struct dt_device_node *dev)
+{
+    int ret;
+
+    if ( dt_device_type_is_equal(dev, "pci") )
+    {
+        DPRINT("Mapping children of %s to guest\n", dt_node_full_name(dev));
+
+        ret = dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
+        if ( ret < 0 )
+            return ret;
+
+        ret = dt_for_each_range(dev, &map_range_to_domain, d);
+        if ( ret < 0 )
+            return ret;
+    }
+
+    return 0;
+}
+
 /*
  * For a given device node:
  *  - Give permission to the guest to manage IRQ and MMIO range
@@ -1093,6 +1216,10 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
         }
     }
 
+    res = map_device_children(d, dev);
+    if ( res )
+        return res;
+
     return 0;
 }
 
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 26754b5..8b05ed5 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -73,148 +73,6 @@ static uint32_t xgene_storm_quirks(void)
     return PLATFORM_QUIRK_GIC_64K_STRIDE;
 }
 
-static int map_one_mmio(struct domain *d, const char *what,
-                         unsigned long start, unsigned long end)
-{
-    int ret;
-
-    printk("Additional MMIO %lx-%lx (%s)\n",
-           start, end, what);
-    ret = map_mmio_regions(d, start, end - start, start);
-    if ( ret )
-        printk("Failed to map %s @ %lx to dom%d\n",
-               what, start, d->domain_id);
-    return ret;
-}
-
-static int map_one_spi(struct domain *d, const char *what,
-                       unsigned int spi, unsigned int type)
-{
-    unsigned int irq;
-    int ret;
-
-    irq = spi + 32; /* SPIs start at IRQ 32 */
-
-    ret = irq_set_spi_type(irq, type);
-    if ( ret )
-    {
-        printk("Failed to set the type for IRQ%u\n", irq);
-        return ret;
-    }
-
-    printk("Additional IRQ %u (%s)\n", irq, what);
-
-    if ( !vgic_reserve_virq(d, irq) )
-        printk("Failed to reserve vIRQ %u on dom%d\n",
-               irq, d->domain_id);
-
-    ret = route_irq_to_guest(d, irq, irq, what);
-    if ( ret )
-        printk("Failed to route %s to dom%d\n", what, d->domain_id);
-
-    return ret;
-}
-
-/* Creates MMIO mappings base..end as well as 4 SPIs from the given base. */
-static int xgene_storm_pcie_specific_mapping(struct domain *d,
-                                             const struct dt_device_node *node,
-                                             paddr_t base, paddr_t end,
-                                             int base_spi)
-{
-    int ret;
-
-    printk("Mapping additional regions for PCIe device %s\n",
-           dt_node_full_name(node));
-
-    /* Map the PCIe bus resources */
-    ret = map_one_mmio(d, "PCI MEMORY", paddr_to_pfn(base), paddr_to_pfn(end));
-    if ( ret )
-        goto err;
-
-    ret = map_one_spi(d, "PCI#INTA", base_spi+0, DT_IRQ_TYPE_LEVEL_HIGH);
-    if ( ret )
-        goto err;
-
-    ret = map_one_spi(d, "PCI#INTB", base_spi+1, DT_IRQ_TYPE_LEVEL_HIGH);
-    if ( ret )
-        goto err;
-
-    ret = map_one_spi(d, "PCI#INTC", base_spi+2, DT_IRQ_TYPE_LEVEL_HIGH);
-    if ( ret )
-        goto err;
-
-    ret = map_one_spi(d, "PCI#INTD", base_spi+3, DT_IRQ_TYPE_LEVEL_HIGH);
-    if ( ret )
-        goto err;
-
-    ret = 0;
-err:
-    return ret;
-}
-
-/*
- * Xen does not currently support mapping MMIO regions and interrupt
- * for bus child devices (referenced via the "ranges" and
- * "interrupt-map" properties to domain 0). Instead for now map the
- * necessary resources manually.
- */
-static int xgene_storm_specific_mapping(struct domain *d)
-{
-    struct dt_device_node *node = NULL;
-    int ret;
-
-    while ( (node = dt_find_compatible_node(node, "pci", "apm,xgene-pcie")) )
-    {
-        u64 addr;
-
-        /* Identify the bus via it's control register address */
-        ret = dt_device_get_address(node, 0, &addr, NULL);
-        if ( ret < 0 )
-            return ret;
-
-        if ( !dt_device_is_available(node) )
-            continue;
-
-       switch ( addr )
-        {
-        case 0x1f2b0000: /* PCIe0 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x0e000000000UL, 0x10000000000UL, 0xc2);
-            break;
-        case 0x1f2c0000: /* PCIe1 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x0d000000000UL, 0x0e000000000UL, 0xc8);
-            break;
-        case 0x1f2d0000: /* PCIe2 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x09000000000UL, 0x0a000000000UL, 0xce);
-            break;
-        case 0x1f500000: /* PCIe3 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x0a000000000UL, 0x0c000000000UL, 0xd4);
-            break;
-        case 0x1f510000: /* PCIe4 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x0c000000000UL, 0x0d000000000UL, 0xda);
-            break;
-
-        default:
-            printk("Ignoring unknown PCI bus %s\n", dt_node_full_name(node));
-            continue;
-        }
-
-        if ( ret < 0 )
-            return ret;
-    }
-
-    return 0;
-}
-
 static void xgene_storm_reset(void)
 {
     void __iomem *addr;
@@ -265,7 +123,6 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
     .init = xgene_storm_init,
     .reset = xgene_storm_reset,
     .quirks = xgene_storm_quirks,
-    .specific_mapping = xgene_storm_specific_mapping,
 PLATFORM_END
 
 /*
-- 
1.7.10.4

[PATCH v5 5/6] xen: arm: Import of_bus PCI entry from Linux (as a dt_bus entry)

[Ian Campbell]

This provides specific handlers for the PCI bus relating to matching
and translating. It's mostly similar to the defaults but includes some
additional error checks and other PCI specific bits.

There are some subtle differences in how the generic code vs. the pci
specific code here will handle buggy DTs (i.e. #*-cells which are not
as required by the pci bindings). This will mean we tolerate such
device trees better.

I say "buggy", but actually it's not clear to me from reading "PCI Bus
Binding to Open Firmware" that when the device_type is "pci" that
e.g. the text says "The value of "#address-cells" for PCI Bus Nodes is
3." and not "A PCI Bus Node must contain a #address-cells property
containing 3", iow the #address-cells might validly be implicit rather
than an actual property. Maybe that interpretation is bogus, but with
this patch we are are able to cope with DTs written by people who do
read it like that.

It also gets us the ability to parse the flags (cacheability),
although at the moment we only check them for validity rather than use
them.

Functions/types renamed and reindented (because apparently we do
that for these).

Needs a selection of IORESOURCE_* defines, which I've taken from Linux
and have included locally for now until we figure out where else they
might be needed.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v5: Expanded commit message.
v3: New patch
---
 xen/common/device_tree.c |  108 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 105 insertions(+), 3 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 1942372..bc53b1a 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -417,6 +417,26 @@ int dt_n_size_cells(const struct dt_device_node *np)
 }
 
 /*
+ * These are defined in Linux where much of this code comes from, but
+ * are currently unused outside this file in the context of Xen.
+ */
+#define IORESOURCE_BITS         0x000000ff      /* Bus-specific bits */
+
+#define IORESOURCE_TYPE_BITS    0x00001f00      /* Resource type */
+#define IORESOURCE_IO           0x00000100      /* PCI/ISA I/O ports */
+#define IORESOURCE_MEM          0x00000200
+#define IORESOURCE_REG          0x00000300      /* Register offsets */
+#define IORESOURCE_IRQ          0x00000400
+#define IORESOURCE_DMA          0x00000800
+#define IORESOURCE_BUS          0x00001000
+
+#define IORESOURCE_PREFETCH     0x00002000      /* No side effects */
+#define IORESOURCE_READONLY     0x00004000
+#define IORESOURCE_CACHEABLE    0x00008000
+#define IORESOURCE_RANGELENGTH  0x00010000
+#define IORESOURCE_SHADOWABLE   0x00020000
+
+/*
  * Default translator (generic bus)
  */
 static bool_t dt_bus_default_match(const struct dt_device_node *node)
@@ -480,9 +500,81 @@ static int dt_bus_default_translate(__be32 *addr, u64 offset, int na)
 }
 static unsigned int dt_bus_default_get_flags(const __be32 *addr)
 {
-    /* TODO: Return the type of memory (device, ...) for caching
-     * attribute during mapping */
-    return 0;
+    return IORESOURCE_MEM;
+}
+
+/*
+ * PCI bus specific translator
+ */
+
+static bool_t dt_bus_pci_match(const struct dt_device_node *np)
+{
+    /*
+     * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI
+     * powermacs "ht" is hypertransport
+     */
+    return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") ||
+        !strcmp(np->type, "vci") || !strcmp(np->type, "ht");
+}
+
+static void dt_bus_pci_count_cells(const struct dt_device_node *np,
+				   int *addrc, int *sizec)
+{
+    if (addrc)
+        *addrc = 3;
+    if (sizec)
+        *sizec = 2;
+}
+
+static unsigned int dt_bus_pci_get_flags(const __be32 *addr)
+{
+    unsigned int flags = 0;
+    u32 w = be32_to_cpup(addr);
+
+    switch((w >> 24) & 0x03) {
+    case 0x01:
+        flags |= IORESOURCE_IO;
+        break;
+    case 0x02: /* 32 bits */
+    case 0x03: /* 64 bits */
+        flags |= IORESOURCE_MEM;
+        break;
+    }
+    if (w & 0x40000000)
+        flags |= IORESOURCE_PREFETCH;
+    return flags;
+}
+
+static u64 dt_bus_pci_map(__be32 *addr, const __be32 *range, int na, int ns,
+		int pna)
+{
+    u64 cp, s, da;
+    unsigned int af, rf;
+
+    af = dt_bus_pci_get_flags(addr);
+    rf = dt_bus_pci_get_flags(range);
+
+    /* Check address type match */
+    if ((af ^ rf) & (IORESOURCE_MEM | IORESOURCE_IO))
+        return DT_BAD_ADDR;
+
+    /* Read address values, skipping high cell */
+    cp = dt_read_number(range + 1, na - 1);
+    s  = dt_read_number(range + na + pna, ns);
+    da = dt_read_number(addr + 1, na - 1);
+
+    dt_dprintk("DT: PCI map, cp=%llx, s=%llx, da=%llx\n",
+               (unsigned long long)cp, (unsigned long long)s,
+               (unsigned long long)da);
+
+    if (da < cp || da >= (cp + s))
+        return DT_BAD_ADDR;
+    return da - cp;
+}
+
+static int dt_bus_pci_translate(__be32 *addr, u64 offset, int na)
+{
+    return dt_bus_default_translate(addr + 1, offset, na - 1);
 }
 
 /*
@@ -490,6 +582,16 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr)
  */
 static const struct dt_bus dt_busses[] =
 {
+    /* PCI */
+    {
+        .name = "pci",
+        .addresses = "assigned-addresses",
+        .match = dt_bus_pci_match,
+        .count_cells = dt_bus_pci_count_cells,
+        .map = dt_bus_pci_map,
+        .translate = dt_bus_pci_translate,
+        .get_flags = dt_bus_pci_get_flags,
+    },
     /* Default */
     {
         .name = "default",
-- 
1.7.10.4

[PATCH v5 6/6] xen: arm: consolidate mmio and irq mapping to dom0

[Ian Campbell]

The code in the callbacks for dt_for_each_irq_map and
dt_for_each_range is very similar to the code in handle_device for
each non-pci device.

In fact the only major difference is that the irq callback needs to
call irq_set_spi_type in the PCI case. Refactor into a
map_dt_irq_to_domain callback which does the irq_set_spi_type and then
calls map_irq_to_domain which is also used from handle_device.

For mmio map_range_to_domain can already be used directly from
handle_device too.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v5: New patch
---
 xen/arch/arm/domain_build.c |  112 ++++++++++++++-----------------------------
 1 file changed, 35 insertions(+), 77 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 22b4c04..8495bf1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -954,32 +954,13 @@ static int make_timer_node(const struct domain *d, void *fdt,
     return res;
 }
 
-static int map_interrupt_to_domain(const struct dt_device_node *dev,
-                                   const struct dt_irq *dt_irq,
-                                   void *data)
+static int map_irq_to_domain(const struct dt_device_node *dev,
+                             struct domain *d, unsigned int irq)
+
 {
-    struct domain *d = data;
     bool_t need_mapping = !dt_device_for_passthrough(dev);
-    unsigned int irq = dt_irq->irq;
     int res;
 
-    if ( irq < NR_LOCAL_IRQS )
-    {
-        printk(XENLOG_ERR "%s: IRQ%"PRId32" is not a SPI\n",
-               dt_node_name(dev), irq);
-        return -EINVAL;
-    }
-
-    /* Setup the IRQ type */
-    res = irq_set_spi_type(irq, dt_irq->type);
-    if ( res )
-    {
-        printk(XENLOG_ERR
-               "%s: Unable to setup IRQ%"PRId32" to dom%d\n",
-               dt_node_name(dev), irq, d->domain_id);
-        return res;
-    }
-
     res = irq_permit_access(d, irq);
     if ( res )
     {
@@ -1007,6 +988,35 @@ static int map_interrupt_to_domain(const struct dt_device_node *dev,
     }
 
     DPRINT("  - IRQ: %u\n", irq);
+    return 0;
+}
+
+static int map_dt_irq_to_domain(const struct dt_device_node *dev,
+                                const struct dt_irq *dt_irq,
+                                void *data)
+{
+    struct domain *d = data;
+    unsigned int irq = dt_irq->irq;
+    int res;
+
+    if ( irq < NR_LOCAL_IRQS )
+    {
+        printk(XENLOG_ERR "%s: IRQ%"PRId32" is not a SPI\n",
+               dt_node_name(dev), irq);
+        return -EINVAL;
+    }
+
+    /* Setup the IRQ type */
+    res = irq_set_spi_type(irq, dt_irq->type);
+    if ( res )
+    {
+        printk(XENLOG_ERR
+               "%s: Unable to setup IRQ%"PRId32" to dom%d\n",
+               dt_node_name(dev), irq, d->domain_id);
+        return res;
+    }
+
+    res = map_irq_to_domain(dev, d, irq);
 
     return 0;
 }
@@ -1065,7 +1075,7 @@ static int map_device_children(struct domain *d,
     {
         DPRINT("Mapping children of %s to guest\n", dt_node_full_name(dev));
 
-        ret = dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
+        ret = dt_for_each_irq_map(dev, &map_dt_irq_to_domain, d);
         if ( ret < 0 )
             return ret;
 
@@ -1091,7 +1101,6 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
     unsigned int naddr;
     unsigned int i;
     int res;
-    unsigned int irq;
     struct dt_raw_irq rirq;
     u64 addr, size;
     bool_t need_mapping = !dt_device_for_passthrough(dev);
@@ -1144,34 +1153,9 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
             return res;
         }
 
-        irq = res;
-
-        DPRINT("irq %u = %u\n", i, irq);
-
-        res = irq_permit_access(d, irq);
+        res = map_irq_to_domain(dev, d, res);
         if ( res )
-        {
-            printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
-                   d->domain_id, irq);
             return res;
-        }
-
-        if ( need_mapping )
-        {
-            /*
-             * Checking the return of vgic_reserve_virq is not
-             * necessary. It should not fail except when we try to map
-             * twice the IRQ. This can happen if the IRQ is shared
-             */
-            vgic_reserve_virq(d, irq);
-            res = route_irq_to_guest(d, irq, irq, dt_node_name(dev));
-            if ( res )
-            {
-                printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
-                       irq, d->domain_id);
-                return res;
-            }
-        }
     }
 
     /* Give permission and map MMIOs */
@@ -1185,35 +1169,9 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
             return res;
         }
 
-        DPRINT("addr %u = 0x%"PRIx64" - 0x%"PRIx64"\n",
-               i, addr, addr + size - 1);
-
-        res = iomem_permit_access(d, paddr_to_pfn(addr & PAGE_MASK),
-                                  paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
+        res = map_range_to_domain(dev, addr, size, d);
         if ( res )
-        {
-            printk(XENLOG_ERR "Unable to permit to dom%d access to"
-                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
-                   d->domain_id,
-                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
             return res;
-        }
-
-        if ( need_mapping )
-        {
-            res = map_mmio_regions(d,
-                                   paddr_to_pfn(addr & PAGE_MASK),
-                                   DIV_ROUND_UP(size, PAGE_SIZE),
-                                   paddr_to_pfn(addr & PAGE_MASK));
-            if ( res )
-            {
-                printk(XENLOG_ERR "Unable to map 0x%"PRIx64
-                       " - 0x%"PRIx64" in domain %d\n",
-                       addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1,
-                       d->domain_id);
-                return res;
-            }
-        }
     }
 
     res = map_device_children(d, dev);
-- 
1.7.10.4

Re: [PATCH v5 1/6] xen: dt: add dt_for_each_irq_map helper

[Ian Campbell]

On Fri, 2015-07-03 at 14:59 +0100, Ian Campbell wrote:
> This function iterates over a nodes interrupt-map property and calls a
> callback for each interrupt. For now it only supplies the translated
> IRQ since my use case has no need of e.g. child unit address. These
> can be added as needed by any future users.
> 
> This follows much the same logic as dt_irq_map_raw when parsing the
> interrupt-map, but doesn't walk up the tree doing the actual
> translation and it iterates over all entries instead of just looking
> for the first match.
> 
> I looked into refactoring dt_irq_map_raw but I couldn't find a way
> which I was confident in, plus I was reluctant to diverge from the
> Linux roots of this function any further.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Julien Grall <julien.grall@citrix.com>

Julien pointed out that after this R-by response on v4 there was also a
bunch of comments which I had missed (i.e. the R-by was a typo). I've
replied to those on v4.

(Mostly responding here for my own benefit so I don't go and commit it
at some point).