xen: arm: Parse PCI DT nodes' ranges and interrupt-map

This is similar to 816f5bb1f074 "xen: arm: propagate gic's should propagate (rather than invent our own value) since this value is used to size fields within other properties within the tree. I'm not sure why I didn't do this as part of 816f5bb1f074. I think probably just because #interrupt-cells must always be 3 for a GIC whereas #address-cells can legitimately differ. Regardless, I think we might as well do this in common code.

Signed-off-by Ian Campbell <[email protected]>

---
 xen/arch/arm/domain_build.c |   18 +++++++++++++-----
 xen/arch/arm/gic-hip04.c    |    4 ----
 xen/arch/arm/gic-v2.c       |    4 ----
 xen/arch/arm/gic-v3.c       |    4 ----
 4 files changed, 13 insertions(+), 17 deletions(-)

xen/arch/arm/domain_build.c +4 -4

index ab4ad65..2a2fc2b 100644
@@ -784,8 +784,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
 {
     const struct dt_device_node *gic = dt_interrupt_controller;
     int res = 0;
-    const void *addrcells;
-    u32 addrcells_len;
+    const void *cells;
+    u32 cells_len;
 
     /*
      * Xen currently supports only a single GIC. Discard any secondary

@@ -815,10 +815,18 @@ static int make_gic_node(const struct domain *d, void *fdt,
             return res;
     }
 
-    addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
-    if ( addrcells )
+    cells = dt_get_property(gic, "#address-cells", &cells_len);
+    if ( cells )
     {

xen/arch/arm/domain_build.c +0 -0

         if ( res )
             return res;
     }

xen/arch/arm/gic-hip04.c +0 -4

index 073ad33..42af10b 100644
@@ -636,10 +636,6 @@ static int hip04gic_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
-    if ( res )
-        return res;
-
     res = fdt_property(fdt, "interrupt-controller", NULL, 0);
 
     if ( res )

xen/arch/arm/gic-v2.c +0 -4

index 20cdbc9..8d1a07d 100644
@@ -622,10 +622,6 @@ static int gicv2_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
-    if ( res )
-        return res;
-
     res = fdt_property(fdt, "interrupt-controller", NULL, 0);
 
     if ( res )

xen/arch/arm/gic-v3.c +0 -0

index ab80670..528500a 100644

Hi Ian,

Quoted 7 lines · Ian Campbell

This is similar to 816f5bb1f074 "xen: arm: propagate gic's should propagate (rather than invent our own value) since this value is used to size fields within other properties within the tree. I'm not sure why I didn't do this as part of 816f5bb1f074. I think probably just because #interrupt-cells must always be 3 for a GIC whereas #address-cells can legitimately differ. Regardless, I think we might as well do this in common code.

Hmmm... We are creating some interrupt ourself assuming the number of interrupt cells is 3. So it makes sense to hard-code (not really invent) the value.

Quoted 39 lines

Signed-off-by Ian Campbell <[email protected]>

---
 xen/arch/arm/domain_build.c |   18 +++++++++++++-----
 xen/arch/arm/gic-hip04.c    |    4 ----
 xen/arch/arm/gic-v2.c       |    4 ----
 xen/arch/arm/gic-v3.c       |    4 ----
 4 files changed, 13 insertions(+), 17 deletions(-)

xen/arch/arm/domain_build.c +9 -5

index ab4ad65..2a2fc2b 100644
@@ -784,8 +784,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
 {
     const struct dt_device_node *gic = dt_interrupt_controller;
     int res = 0;
-    const void *addrcells;
-    u32 addrcells_len;
+    const void *cells;
+    u32 cells_len;
 
     /*
      * Xen currently supports only a single GIC. Discard any secondary

@@ -815,10 +815,18 @@ static int make_gic_node(const struct domain *d, void *fdt,
             return res;
     }
 
-    addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
-    if ( addrcells )
+    cells = dt_get_property(gic, "#address-cells", &cells_len);
+    if ( cells )
     {
-        res = fdt_property(fdt, "#address-cells", addrcells, addrcells_len);
+        res = fdt_property(fdt, "#address-cells", cells, cells_len);
+        if ( res )
+            return res;
+    }
+

xen/arch/arm/domain_build.c static int make_gic_node(const struct domain *d, void *fdt, · line 829

Quoted 4 lines

xen/arch/arm/domain_build.c +4 -0

+    cells = dt_get_property(gic, "#interrupt-cells", &cells_len);
+    if ( cells )
+    {
+        res = fdt_property(fdt, "#interrupt-cells", cells, cells_len);

The #interrupt-cells as to be present at any time for the GIC. So I don't think it's worth to check if it presents. Maybe an ASSERT would be enough?

Also, I would check somewhere that the value is effectively 3 otherwise we are in trouble for the timer/evtchn interrupt creation. Though, it was there before too.

Quoted 8 lines

xen/arch/arm/gic-v3.c +0 -0

index ab80670..528500a 100644
@@ -1102,10 +1102,6 @@ static int gicv3_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 

xen/arch/arm/gic-v3.c static int gicv3_make_dt_node(const struct domain *d, · line 1107

Quoted 8 lines

xen/arch/arm/gic-v3.c +0 -4

-    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
-    if ( res )
-        return res;
-
     res = fdt_property(fdt, "interrupt-controller", NULL, 0);
     if ( res )
         return res;

While you move #interrupt-cells to common code. Could you move interrupt-controller too?

These properties are defined in ePAPR (2.3.8 and 2.4.3.1 respectively) and the OpenFirmware PCI Bus Binding Specification (IEEE Std 1275-1994).

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

TODO: Use a helper iterator (e.g. dt_for_each_range) for the ranges property, like is already done for interrupts using dt_for_each_irq_map.

TODO: Should we stop calling map_device for nodes beneath this one (since we should have mapped everything underneath)? I think this is complex for cases which map interrupt but not ranges or vice versa, perhaps meaning we need to recurse on them separately. Maybe we can continue to descend and the mappings may just be harmlessly instantiated twice.

TODO: Related to the above there are also some buses for which we cannot use dt_bus_default_{map,translate}. We might want to pull in the of_bus_pci stuff from Linux, although I think that would be orthogonal to this fix.

Signed-off-by Ian Campbell <[email protected]>

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

---
 xen/arch/arm/domain_build.c          |  156 ++++++++++++++++++++++++++++++++++
 xen/arch/arm/platforms/xgene-storm.c |  143 -------------------------------
 2 files changed, 156 insertions(+), 143 deletions(-)

xen/arch/arm/domain_build.c +71 -0

index 2a2fc2b..704c2aa 100644
@@ -23,6 +23,21 @@
 #include <xen/irq.h>
 #include "kernel.h"
 
+/*
+ * Definitions for implementing parts of the OpenFirmware PCI Bus Binding
+ * Specification (IEEE Std 1275-1994).
+ */
+
+struct of_pci_unit_address {
+	__be32 hi, mid, lo;
+} __attribute__((packed));
+
+struct of_pci_ranges_entry {
+	struct of_pci_unit_address	pci_addr;
+	__be64				cpu_addr;
+	__be64				length;
+} __attribute__((packed));
+
 static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 

@@ -911,6 +926,139 @@ static int make_timer_node(const struct domain *d, void *fdt,
     return res;
 }
 
+static int map_pci_device_ranges(struct domain *d,
+                                 const struct dt_device_node *dev,
+                                 const struct of_pci_ranges_entry  *ranges,
+                                 const u32 len)
+{
+    int parent_size_cells, parent_addr_cells;
+    int i, nr, res;
+
+    parent_size_cells = dt_n_size_cells(dev);
+    parent_addr_cells = dt_n_addr_cells(dev);
+
+    /*
+     * Range is child address, host address (#address-cells), length
+     * (#size-cells),see ePAPR 2.3.8.
+     *
+     * PCI child address is u32 space + u64 address, see ePAPR 6.2.2.
+     *
+     */
+    nr = len / sizeof(*ranges);
+
+    for ( i = 0; i < nr ; i++ )
+    {
+        const struct of_pci_ranges_entry *range = &ranges[i];
+        u64 addr, len;
+
+        len = fdt64_to_cpu(range->length);
+
+        addr = dt_translate_address(dev, (const __be32 *)&range->cpu_addr);
+        DPRINT("PCI SPACE 0x%08x.%08x.%08x 0x%"PRIx64" size 0x%"PRIx64"\n",
+               fdt32_to_cpu(range->pci_addr.hi),
+               fdt32_to_cpu(range->pci_addr.mid),
+               fdt32_to_cpu(range->pci_addr.lo),
+               addr, len);
+
+        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;
+        }
+    }
+    return 0;
+}
+
+static int map_device_ranges(struct domain *d, const struct dt_device_node *dev)
+{
+    const void *ranges;
+    u32 len;
+
+    ranges = dt_get_property(dev, "ranges", &len);

xen/arch/arm/domain_build.c +44 -0

+    /* Lets not worry for now... */
+    return 0;
+}
+
+static int map_interrupt_to_domain(const struct dt_device_node *dev,
+                                   const struct dt_raw_irq *dt_raw_irq,
+                                   void *data)
+{
+    struct domain *d = data;
+    struct dt_irq dt_irq;
+    int res;
+
+    res = dt_irq_translate(dt_raw_irq, &dt_irq);
+    if ( res < 0 )
+    {
+        printk(XENLOG_ERR "%s: Failed to translate IRQ: %d\n",
+               dt_node_name(dev), res);
+        return res;
+    }
+
+    if ( dt_irq.irq < NR_LOCAL_IRQS )
+    {
+        printk(XENLOG_ERR "%s: IRQ%"PRId32" is not a SPI\n",
+               dt_node_name(dev), dt_irq.irq);
+        return -EINVAL;
+    }
+
+    /* Setup the IRQ type */
+    res = irq_set_spi_type(dt_irq.irq, dt_irq.type);
+    if ( res )
+    {
+        printk(XENLOG_ERR
+               "%s: Unable to setup IRQ%"PRId32" to dom%d\n",
+               dt_node_name(dev), dt_irq.irq, d->domain_id);
+        return res;
+    }
+
+    res = route_irq_to_guest(d, dt_irq.irq, dt_node_name(dev));
+    if ( res < 0 )
+    {
+        printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
+               dt_irq.irq, d->domain_id);
+        return res;
+    }

xen/arch/arm/domain_build.c +13 -0

+        return 0; /* No interrupt map to handle */
+
+    if ( dt_device_type_is_equal(dev, "pci") )
+        return dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
+
+    printk("Cannot handle interrupt-map for non-PCI device %s type %s\n",
+           dt_node_name(dev), dev->type);
+
+    /* Lets not worry for now... */
+    return 0;
+}
+
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, struct dt_device_node *dev)
 {

Hi Ian,

Quoted 10 lines · Ian Campbell

These properties are defined in ePAPR (2.3.8 and 2.4.3.1 respectively) and the OpenFirmware PCI Bus Binding Specification (IEEE Std 1275-1994).

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

TODO: Use a helper iterator (e.g. dt_for_each_range) for the ranges property, like is already done for interrupts using dt_for_each_irq_map.

Quoted 6 lines

TODO: Should we stop calling map_device for nodes beneath this one (since we should have mapped everything underneath)? I think this is complex for cases which map interrupt but not ranges or vice versa, perhaps meaning we need to recurse on them separately. Maybe we can continue to descend and the mappings may just be harmlessly instantiated twice.

There is not harm to route twice the same IRQ. Although it's pointless to continue.

How difficult would it be to introduce a new bus?

xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 994

Quoted 6 lines

xen/arch/arm/domain_build.c +6 -0

+    if ( dt_device_type_is_equal(dev, "pci") )
+        return map_pci_device_ranges(d, dev, ranges, len);
+
+    printk("Cannot handle ranges for non-PCI device %s type %s\n",
+           dt_node_name(dev), dev->type);
+

Is the printk really necessary? It will a spurious log on platform where ranges is not empty (midway, arndale, foundation model...).

In general, we should not handle ranges by default as we may overlap the mapping done during the domain creation (such as the GIC).

Quoted 44 lines

xen/arch/arm/domain_build.c +44 -0

+    /* Lets not worry for now... */
+    return 0;
+}
+
+static int map_interrupt_to_domain(const struct dt_device_node *dev,
+                                   const struct dt_raw_irq *dt_raw_irq,
+                                   void *data)
+{
+    struct domain *d = data;
+    struct dt_irq dt_irq;
+    int res;
+
+    res = dt_irq_translate(dt_raw_irq, &dt_irq);
+    if ( res < 0 )
+    {
+        printk(XENLOG_ERR "%s: Failed to translate IRQ: %d\n",
+               dt_node_name(dev), res);
+        return res;
+    }
+
+    if ( dt_irq.irq < NR_LOCAL_IRQS )
+    {
+        printk(XENLOG_ERR "%s: IRQ%"PRId32" is not a SPI\n",
+               dt_node_name(dev), dt_irq.irq);
+        return -EINVAL;
+    }
+
+    /* Setup the IRQ type */
+    res = irq_set_spi_type(dt_irq.irq, dt_irq.type);
+    if ( res )
+    {
+        printk(XENLOG_ERR
+               "%s: Unable to setup IRQ%"PRId32" to dom%d\n",
+               dt_node_name(dev), dt_irq.irq, d->domain_id);
+        return res;
+    }
+
+    res = route_irq_to_guest(d, dt_irq.irq, dt_node_name(dev));
+    if ( res < 0 )
+    {
+        printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
+               dt_irq.irq, d->domain_id);
+        return res;
+    }

xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 1048

Quoted 10 lines

xen/arch/arm/domain_build.c +10 -0

+
+    DPRINT("PCI IRQ %u mapped to guest", dt_irq.irq);
+
+    return 0;
+}
+
+static int map_device_interrupts(struct domain *d, const struct dt_device_node *dev)
+{
+
+    if ( !dt_property_read_bool(dev, "interrupt-map") )

It's strange to use dt_property_read_bool here and dt_get_property above to check the emptiness.

I prefer the dt_get_property version which is less confusing.

Anyway, why do you need to check interrupt-map. Can't your new helper deal with empty property?

Quoted 21 lines

xen/arch/arm/domain_build.c +13 -0

+        return 0; /* No interrupt map to handle */
+
+    if ( dt_device_type_is_equal(dev, "pci") )
+        return dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
+
+    printk("Cannot handle interrupt-map for non-PCI device %s type %s\n",
+           dt_node_name(dev), dev->type);
+
+    /* Lets not worry for now... */
+    return 0;
+}
+
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, struct dt_device_node *dev)
 {

@@ -1025,6 +1173,14 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
         }
     }
 

xen/arch/arm/domain_build.c static int map_device(struct domain *d, struct dt_device_node *dev) · line 1180

Quoted 5 lines

xen/arch/arm/domain_build.c +5 -0

+    res = map_device_ranges(d, dev);
+    if ( res )
+        return res;
+
+    res = map_device_interrupts(d, dev);

The name of the function doesn't make much sense. We already map the interrupt above (see platform get_irq).

Quoted 9 lines

xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 994

Quoted 6 lines

xen/arch/arm/domain_build.c +6 -0

+    if ( dt_device_type_is_equal(dev, "pci") )
+        return map_pci_device_ranges(d, dev, ranges, len);
+
+    printk("Cannot handle ranges for non-PCI device %s type %s\n",
+           dt_node_name(dev), dev->type);
+

Is the printk really necessary? It will a spurious log on platform where ranges is not empty (midway, arndale, foundation model...).

If the ranges is present and non-empty it's not impossible that we need to be doing something with it. I'd rather try and figure out how to whitelist such nodes, perhaps they lack a dev_type completely?

Quoted 8 lines

xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 1048

Quoted 4 lines

xen/arch/arm/domain_build.c +4 -0

+static int map_device_interrupts(struct domain *d, const struct dt_device_node *dev)
+{
+
+    if ( !dt_property_read_bool(dev, "interrupt-map") )

It's strange to use dt_property_read_bool here and dt_get_property above to check the emptiness.

Quoted 4 lines

I prefer the dt_get_property version which is less confusing.

Anyway, why do you need to check interrupt-map. Can't your new helper deal with empty property?

It can, but the code below would log for any non-pci device, which is undesirable if there is no interrupt-map present.

Quoted 21 lines

Quoted 21 lines

xen/arch/arm/domain_build.c +13 -0

+        return 0; /* No interrupt map to handle */
+
+    if ( dt_device_type_is_equal(dev, "pci") )
+        return dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
+
+    printk("Cannot handle interrupt-map for non-PCI device %s type %s\n",
+           dt_node_name(dev), dev->type);
+
+    /* Lets not worry for now... */
+    return 0;
+}
+
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, struct dt_device_node *dev)
 {

@@ -1025,6 +1173,14 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
         }
     }
 

Quoted 8 lines

xen/arch/arm/domain_build.c static int map_device(struct domain *d, struct dt_device_node *dev) · line 1180

Quoted 5 lines

xen/arch/arm/domain_build.c +5 -0

+    res = map_device_ranges(d, dev);
+    if ( res )
+        return res;
+
+    res = map_device_interrupts(d, dev);

The name of the function doesn't make much sense. We already map the interrupt above (see platform get_irq).

child_interrupt perhaps?

Quoted 10 lines · Ian Campbell

Quoted 9 lines

xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 994

Quoted 6 lines

xen/arch/arm/domain_build.c +6 -0

+    if ( dt_device_type_is_equal(dev, "pci") )
+        return map_pci_device_ranges(d, dev, ranges, len);
+
+    printk("Cannot handle ranges for non-PCI device %s type %s\n",
+           dt_node_name(dev), dev->type);
+

Is the printk really necessary? It will a spurious log on platform where ranges is not empty (midway, arndale, foundation model...).

Quoted 3 lines

If the ranges is present and non-empty it's not impossible that we need to be doing something with it. I'd rather try and figure out how to whitelist such nodes, perhaps they lack a dev_type completely?

Why would we compute the range? Any usable MMIO should be describe the child.

We have to compute the ranges for PCI because we want to map everything at boot time and the PCI devices are not discoverable via the device tree...

Quoted 18 lines · Julien Grall

Quoted 10 lines · Ian Campbell

Quoted 9 lines

xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 994

Quoted 6 lines

xen/arch/arm/domain_build.c +6 -0

+    if ( dt_device_type_is_equal(dev, "pci") )
+        return map_pci_device_ranges(d, dev, ranges, len);
+
+    printk("Cannot handle ranges for non-PCI device %s type %s\n",
+           dt_node_name(dev), dev->type);
+

Is the printk really necessary? It will a spurious log on platform where ranges is not empty (midway, arndale, foundation model...).

Quoted 3 lines

If the ranges is present and non-empty it's not impossible that we need to be doing something with it. I'd rather try and figure out how to whitelist such nodes, perhaps they lack a dev_type completely?

Why would we compute the range? Any usable MMIO should be describe the child.

Quoted 2 lines

We have to compute the ranges for PCI because we want to map everything at boot time and the PCI devices are not discoverable via the device tree...

The same would be true for any similar discoverable bus.

Which I suppose is uncommon on ARM. I think I'll make this a debug level print, or even a #if DT_DEBUG. (Same for the interrupt case)

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.

From d0a024dd49ca6f67b0ec0342fd2d819b750a52a4 Mon Sep 17 00:00:00 2001

Quoted 3 lines · 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.

Somehow I managed to completely miss sending out the first patch here (thanks Chen Baozi!)...

This should be inserted at the head of the series.

From d0a024dd49ca6f67b0ec0342fd2d819b750a52a4 Mon Sep 17 00:00:00 2001
From: Ian Campbell <[email protected]>
Date: Fri, 6 Mar 2015 11:13:30 +0000
Subject: [PATCH] xen: dt: add dt_translate_address to translate a raw address

A future patch is going to want to translate an address which is not part of the reg property so the existing dt_device_get_address is not suitable.

This is the same function as Linux's of_translate_address but with the names changed to fit our context and the dev parameter constified.

Signed-off-by Ian Campbell <[email protected]>

---
 xen/common/device_tree.c      |    5 +++++
 xen/include/xen/device_tree.h |    2 ++
 2 files changed, 7 insertions(+)

xen/common/device_tree.c +5 -0

index d1c716f..89491b2 100644
@@ -682,6 +682,11 @@ bail:
     return result;
 }
 
+u64 dt_translate_address(const struct dt_device_node *dev, const __be32 *in_addr)
+{
+        return __dt_translate_address(dev, in_addr, "ranges");
+}
+
 /* dt_device_address - Translate device tree address and return it */
 int dt_device_get_address(const struct dt_device_node *dev, int index,
                           u64 *addr, u64 *size)

xen/include/xen/device_tree.h +2 -0

index c8a0375..b7455cd 100644
@@ -464,6 +464,8 @@ struct dt_device_node *dt_find_node_by_path(const char *path);
  */
 const struct dt_device_node *dt_get_parent(const struct dt_device_node *node);
 
+u64 dt_translate_address(const struct dt_device_node *np, const __be32 *addr);
+
 /**
  * dt_device_get_address - Resolve an address for a device
  * @device: the device whose address is to be resolved

Signature · 1 line

1.7.10.4