[PATCH 0/2] Introducing GICv2m Supports

[PATCH 0/2] Introducing GICv2m Supports

[Suravee Suthikulpanit]

This patch series introduce GICv2m supports in Xen Dom0.

This patch series depend on: 
    [PATCH v3 0/5] xen: arm: Parse PCI DT nodes' ranges and interrupt-map
    http://lists.xen.org/archives/html/xen-devel/2015-04/msg02200.html

This has been tested on AMD Seattle platform with the following kernel:
    git clone https://github.com/ssuthiku/linux.git xen-seattle-revA-pci

Suravee Suthikulpanit (2):
  xen/arm: gic: Refactor the code for creating gic node
  xen/arm: gicv2: Adding support for GICv2m in Dom0

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

-- 
2.1.0

[PATCH 1/2] xen/arm: gic: Refactor the code for creating gic node

[Suravee Suthikulpanit]

Since fdt_begin_node() is called by all gicXX_make_dt_node() to create
the interrupt-controller devicetree node, this patch refactors the call
and moves it inside make_gic_node(). This also matches the fdt_end_node()
call at the end of make_gic_node().

This patch also move the call to gic_make_node() wrapper to be after all
other generic properties are setup. This allows creating sub-node inside
gic_make_node() in order to support v2m sub-node.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 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, 15 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 987ee1e..a2cd471 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -808,8 +808,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 *addrcells, *sizecells;
+    u32 addrcells_len, sizecells_len;
 
     /*
      * Xen currently supports only a single GIC. Discard any secondary
@@ -823,7 +823,7 @@ static int make_gic_node(const struct domain *d, void *fdt,
 
     DPRINT("Create gic node\n");
 
-    res = gic_make_node(d, node, fdt);
+    res = fdt_begin_node(fdt, "interrupt-controller");
     if ( res )
         return res;
 
@@ -847,6 +847,14 @@ static int make_gic_node(const struct domain *d, void *fdt,
             return res;
     }
 
+    sizecells = dt_get_property(gic, "#size-cells", &sizecells_len);
+    if ( sizecells )
+    {
+        res = fdt_property(fdt, "#size-cells", sizecells, sizecells_len);
+        if ( res )
+            return res;
+    }
+
     res = fdt_property_cell(fdt, "#interrupt-cells", 3);
     if ( res )
         return res;
@@ -855,6 +863,10 @@ static int make_gic_node(const struct domain *d, void *fdt,
     if ( res )
         return res;
 
+    res = gic_make_node(d, node, fdt);
+    if ( res )
+        return res;
+
     res = fdt_end_node(fdt);
 
     return res;
diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index 223c414..6d527f1 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -628,10 +628,6 @@ static int hip04gic_make_dt_node(const struct domain *d,
     compatible = DT_COMPAT_GIC_CORTEX_A15;
     len = strlen((char*) compatible) + 1;
 
-    res = fdt_begin_node(fdt, "interrupt-controller");
-    if ( res )
-        return res;
-
     res = fdt_property(fdt, "compatible", compatible, len);
     if ( res )
         return res;
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 073fec2..80acc62 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -616,10 +616,6 @@ static int gicv2_make_dt_node(const struct domain *d,
         return -FDT_ERR_XEN(ENOENT);
     }
 
-    res = fdt_begin_node(fdt, "interrupt-controller");
-    if ( res )
-        return res;
-
     res = fdt_property(fdt, "compatible", compatible, len);
     if ( res )
         return res;
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index e9a8eda..db498ed 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1098,10 +1098,6 @@ static int gicv3_make_dt_node(const struct domain *d,
         return -FDT_ERR_XEN(ENOENT);
     }
 
-    res = fdt_begin_node(fdt, "interrupt-controller");
-    if ( res )
-        return res;
-
     res = fdt_property(fdt, "compatible", compatible, len);
     if ( res )
         return res;
-- 
2.1.0

[PATCH 2/2] xen/arm: gicv2: Adding support for GICv2m in Dom0

[Suravee Suthikulpanit]

This patch detect and propagate the gic-v2m-frame devicetree sub-node.
This allows Dom0 kernel to setup and intialize GICv2m MSI frame.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 xen/arch/arm/gic-v2.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 169 insertions(+)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 80acc62..0c3352e 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -600,6 +600,171 @@ static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_m
     spin_unlock(&gicv2.lock);
 }
 
+/*
+ * Set up gic v2m DT sub-node.
+ *
+ * gic0: interrupt-controller@e1101000 {
+ *          compatible = "arm,gic-400", "arm,cortex-a15-gic";
+ *          interrupt-controller;
+ *          #interrupt-cells = <3>;
+ *          #address-cells = <2>;
+ *          #size-cells = <2>;
+ *          reg = <0x0 0xe1110000 0 0x1000>,
+ *                <0x0 0xe112f000 0 0x2000>,
+ *                <0x0 0xe1140000 0 0x10000>,
+ *                <0x0 0xe1160000 0 0x10000>;
+ *          interrupts = <1 9 0xf04>;
+ *          ranges = <0 0 0 0xe1100000 0 0x100000>;
+ *          v2m0: v2m@e0080000 {
+ *                  compatible = "arm,gic-v2m-frame";
+ *                  msi-controller;
+ *                  arm,msi-base-spi = <64>;
+ *                  arm,msi-num-spis = <256>;
+ *                  reg = <0x0 0x00080000 0 0x1000>;
+ *          };
+ * };
+ */
+static int gicv2m_make_dt_node(const struct domain *d,
+                              const struct dt_device_node *node,
+                              void *fdt)
+{
+    u32 len, base_spi, num_spis;
+    u64 addr, size;
+    int res, i;
+    const void *prop = NULL;
+    struct domain *dom;
+    unsigned long mm_start, mm_nr;
+    const struct dt_device_node *gic = dt_interrupt_controller;
+    const struct dt_device_node *v2m = NULL;
+
+    /* v2m is optional */
+    v2m = dt_find_compatible_node(NULL, NULL, "arm,gic-v2m-frame");
+    if ( !v2m )
+        return 0;
+
+    /* Sub-node requires the ranges property */
+    prop = dt_get_property(gic, "ranges", &len);
+    if ( !prop )
+    {
+        dprintk(XENLOG_ERR, "Can't find ranges property for the gic node\n");
+        return -FDT_ERR_XEN(ENOENT);
+    }
+
+    res = fdt_property(fdt, "ranges", prop, len);
+    if ( res )
+        return res;
+
+    dprintk(XENLOG_DEBUG, "Create v2m node\n");
+
+    res = fdt_begin_node(fdt, "v2m");
+    if ( res )
+        return res;
+
+    res = fdt_property(fdt, "compatible", "arm,gic-v2m-frame", 18);
+    if ( res )
+        goto err_out0;
+
+    res = fdt_property(fdt, "msi-controller", NULL, 0);
+    if ( res )
+        goto err_out0;
+
+    if ( v2m->phandle )
+    {
+        res = fdt_property_cell(fdt, "phandle", v2m->phandle);
+        if ( res )
+            goto err_out0;
+    }
+
+    prop = dt_get_property(v2m, "reg", &len);
+    if ( !prop )
+    {
+        dprintk(XENLOG_ERR, "v2m: Can't find reg property.\n");
+        res = -FDT_ERR_XEN(ENOENT);
+        goto err_out0;
+    }
+
+    len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
+    res = fdt_property(fdt, "reg", prop, len);
+    if ( res )
+        goto err_out0;
+
+    /* Mapping MMIO regions for v2m frame */
+    res = dt_device_get_address(v2m, 0, &addr, &size);
+    if ( res )
+        goto err_out0;
+
+    dom = xzalloc_bytes(sizeof(struct domain));
+    if ( !dom )
+    {
+        res = -ENOMEM;
+        goto err_out0;
+    }
+
+    memcpy(dom, d, sizeof(struct domain));
+    mm_start = paddr_to_pfn(addr & PAGE_MASK);
+    mm_nr = DIV_ROUND_UP(size, PAGE_SIZE);
+    res = map_mmio_regions(dom, mm_start, mm_nr, mm_start);
+    if ( res )
+    {
+        dprintk(XENLOG_ERR, "v2m: map_mmio_regions failed.\n");
+        goto err_out1;
+    }
+
+    /* Set up msi-base-spi dt property */
+    prop = dt_get_property(v2m, "arm,msi-base-spi", &len);
+    if ( !prop )
+    {
+        dprintk(XENLOG_ERR, "v2m: Can't find msi-base-spi.\n");
+        goto err_out2;
+    }
+    base_spi = be32_to_cpup(prop);
+    res = fdt_property(fdt, "arm,msi-base-spi", prop, len);
+    if ( res )
+        goto err_out2;
+
+    /* Set up msi-num-spis dt property */
+    prop = dt_get_property(v2m, "arm,msi-num-spis", &len);
+    if ( !prop )
+    {
+        dprintk(XENLOG_ERR, "v2m: Can't find msi-num-spis.\n");
+        goto err_out2;
+    }
+    num_spis = be32_to_cpup(prop);
+    res = fdt_property(fdt, "arm,msi-num-spis", prop, len);
+    if ( res )
+        goto err_out2;
+
+    /*
+     * Currently, we assign all SPIs for MSI to dom0
+     */
+    for (i = base_spi; i < (base_spi + num_spis); i++)
+    {
+        res = irq_set_spi_type(i, DT_IRQ_TYPE_EDGE_RISING);
+        if ( res )
+        {
+            dprintk(XENLOG_ERR, "v2m: Failed to set MSI interrupt type.\n");
+            goto err_out2;
+        }
+
+        res = route_irq_to_guest(dom, i, i, "v2m");
+        if ( res )
+        {
+            dprintk(XENLOG_ERR, "v2m: Failed to route MSI irqs to guest.\n");
+            goto err_out2;
+        }
+    }
+
+    return fdt_end_node(fdt);
+
+err_out2:
+    unmap_mmio_regions(dom, mm_start, mm_nr, mm_start);
+err_out1:
+    xfree(dom);
+err_out0:
+    fdt_end_node(fdt);
+    return res;
+}
+
 static int gicv2_make_dt_node(const struct domain *d,
                               const struct dt_device_node *node, void *fdt)
 {
@@ -636,6 +801,10 @@ static int gicv2_make_dt_node(const struct domain *d,
     len *= 2;
 
     res = fdt_property(fdt, "reg", regs, len);
+    if ( res )
+        return res;
+
+    res = gicv2m_make_dt_node(d, node, fdt);
 
     return res;
 }
-- 
2.1.0

Re: [PATCH 0/2] Introducing GICv2m Supports

[Julien Grall]

Hi Suravee,

Thanks for adding support of GICv2m.

I left Linaro at the beginning of the month. Can you use my citrix email 
(julien.grall@citrix.com)?

On 23/04/2015 09:51, Suravee Suthikulpanit wrote:
> This patch series introduce GICv2m supports in Xen Dom0.

It looks like that the approach taken by this series won't fit for guest 
(all the "MSI SPIs" are routed to DOM0).

Do you have any plan for guest support? This would be necessary if you 
want to passthrough PCI device on your device.

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen/arm: gic: Refactor the code for creating gic node

[Ian Campbell]

On Wed, 2015-04-22 at 23:51 -0500, Suravee Suthikulpanit wrote:
> Since fdt_begin_node() is called by all gicXX_make_dt_node() to create
> the interrupt-controller devicetree node, this patch refactors the call
> and moves it inside make_gic_node(). This also matches the fdt_end_node()
> call at the end of make_gic_node().
> 
> This patch also move the call to gic_make_node() wrapper to be after all
> other generic properties are setup. This allows creating sub-node inside
> gic_make_node() in order to support v2m sub-node.
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  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, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 987ee1e..a2cd471 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -808,8 +808,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 *addrcells, *sizecells;
> +    u32 addrcells_len, sizecells_len;
>  
>      /*
>       * Xen currently supports only a single GIC. Discard any secondary
> @@ -823,7 +823,7 @@ static int make_gic_node(const struct domain *d, void *fdt,
>  
>      DPRINT("Create gic node\n");
>  
> -    res = gic_make_node(d, node, fdt);
> +    res = fdt_begin_node(fdt, "interrupt-controller");
>      if ( res )
>          return res;
>  
> @@ -847,6 +847,14 @@ static int make_gic_node(const struct domain *d, void *fdt,
>              return res;
>      }
>  
> +    sizecells = dt_get_property(gic, "#size-cells", &sizecells_len);
> +    if ( sizecells )
> +    {
> +        res = fdt_property(fdt, "#size-cells", sizecells, sizecells_len);
> +        if ( res )
> +            return res;
> +    }
> +
>      res = fdt_property_cell(fdt, "#interrupt-cells", 3);
>      if ( res )
>          return res;
> @@ -855,6 +863,10 @@ static int make_gic_node(const struct domain *d, void *fdt,
>      if ( res )
>          return res;
>  
> +    res = gic_make_node(d, node, fdt);
> +    if ( res )
> +        return res;
> +
>      res = fdt_end_node(fdt);
>  
>      return res;
> diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
> index 223c414..6d527f1 100644
> --- a/xen/arch/arm/gic-hip04.c
> +++ b/xen/arch/arm/gic-hip04.c
> @@ -628,10 +628,6 @@ static int hip04gic_make_dt_node(const struct domain *d,
>      compatible = DT_COMPAT_GIC_CORTEX_A15;
>      len = strlen((char*) compatible) + 1;
>  
> -    res = fdt_begin_node(fdt, "interrupt-controller");
> -    if ( res )
> -        return res;
> -
>      res = fdt_property(fdt, "compatible", compatible, len);
>      if ( res )
>          return res;
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 073fec2..80acc62 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -616,10 +616,6 @@ static int gicv2_make_dt_node(const struct domain *d,
>          return -FDT_ERR_XEN(ENOENT);
>      }
>  
> -    res = fdt_begin_node(fdt, "interrupt-controller");
> -    if ( res )
> -        return res;
> -
>      res = fdt_property(fdt, "compatible", compatible, len);
>      if ( res )
>          return res;
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index e9a8eda..db498ed 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1098,10 +1098,6 @@ static int gicv3_make_dt_node(const struct domain *d,
>          return -FDT_ERR_XEN(ENOENT);
>      }
>  
> -    res = fdt_begin_node(fdt, "interrupt-controller");
> -    if ( res )
> -        return res;
> -
>      res = fdt_property(fdt, "compatible", compatible, len);
>      if ( res )
>          return res;

Re: [PATCH 2/2] xen/arm: gicv2: Adding support for GICv2m in Dom0

[Ian Campbell]

On Wed, 2015-04-22 at 23:52 -0500, Suravee Suthikulpanit wrote:
> This patch detect and propagate the gic-v2m-frame devicetree sub-node.

"detects and propagates"

> This allows Dom0 kernel to setup and intialize GICv2m MSI frame.

"initialize"

IIRC the GICv2m is described rather briefly in an appendix to some
document or other, could you reference it here please.

As Julien noted I think this patch is just exposing the GICv2m to dom0
and not doing a more complex thing where Xen owns the GICv2m and
provides a (partially-)emulated vGICv2m to guests (including dom0). It's
worth briefly mentioning that here.

> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  xen/arch/arm/gic-v2.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 169 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 80acc62..0c3352e 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -600,6 +600,171 @@ static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_m
>      spin_unlock(&gicv2.lock);
>  }
>  
> +/*
> + * Set up gic v2m DT sub-node.

Please could you give a reference to the appropriate binding document
here.

> + *
> + * gic0: interrupt-controller@e1101000 {
> + *          compatible = "arm,gic-400", "arm,cortex-a15-gic";
> + *          interrupt-controller;
> + *          #interrupt-cells = <3>;
> + *          #address-cells = <2>;
> + *          #size-cells = <2>;
> + *          reg = <0x0 0xe1110000 0 0x1000>,
> + *                <0x0 0xe112f000 0 0x2000>,
> + *                <0x0 0xe1140000 0 0x10000>,
> + *                <0x0 0xe1160000 0 0x10000>;
> + *          interrupts = <1 9 0xf04>;
> + *          ranges = <0 0 0 0xe1100000 0 0x100000>;
> + *          v2m0: v2m@e0080000 {
> + *                  compatible = "arm,gic-v2m-frame";
> + *                  msi-controller;
> + *                  arm,msi-base-spi = <64>;
> + *                  arm,msi-num-spis = <256>;
> + *                  reg = <0x0 0x00080000 0 0x1000>;
> + *          };
> + * };
> + */
> +static int gicv2m_make_dt_node(const struct domain *d,
> +                              const struct dt_device_node *node,
> +                              void *fdt)
> +{
> +    u32 len, base_spi, num_spis;
> +    u64 addr, size;
> +    int res, i;
> +    const void *prop = NULL;
> +    struct domain *dom;
> +    unsigned long mm_start, mm_nr;
> +    const struct dt_device_node *gic = dt_interrupt_controller;
> +    const struct dt_device_node *v2m = NULL;
> +
> +    /* v2m is optional */
> +    v2m = dt_find_compatible_node(NULL, NULL, "arm,gic-v2m-frame");

Is this required to be a child of the interrupt-controller node,or is
there some other cross link which should be checked?

> +    dom = xzalloc_bytes(sizeof(struct domain));
> +    if ( !dom )
> +    {
> +        res = -ENOMEM;
> +        goto err_out0;
> +    }
> +
> +    memcpy(dom, d, sizeof(struct domain));

This is all veeeerrrry suspicious, what are you trying to do here?

> +    mm_start = paddr_to_pfn(addr & PAGE_MASK);
> +    mm_nr = DIV_ROUND_UP(size, PAGE_SIZE);
> +    res = map_mmio_regions(dom, mm_start, mm_nr, mm_start);

Why not just use d, why make a copy of it?

In any case I don't think calls to map_mmio_regios, or the irq stuff
below belong here, this function should be making the dtb node and
populating it with properties, nothing else.

So the mapping stuff needs to be handled elsewhere, perhaps via a new
gic_hw_operations (or two map_extra_regions + map_extra_irqs perhaps).

This would make much of the complicated error handling here (needed to
unwind the mappings) go away and also let you use dt_read_number instead
of be32_to_* etc.

> +    if ( res )
> +    {
> +        dprintk(XENLOG_ERR, "v2m: map_mmio_regions failed.\n");
> +        goto err_out1;
> +    }
> +
> +    /* Set up msi-base-spi dt property */
> +    prop = dt_get_property(v2m, "arm,msi-base-spi", &len);
> +    if ( !prop )
> +    {
> +        dprintk(XENLOG_ERR, "v2m: Can't find msi-base-spi.\n");
> +        goto err_out2;
> +    }
> +    base_spi = be32_to_cpup(prop);
> +    res = fdt_property(fdt, "arm,msi-base-spi", prop, len);

What about extra properties not hardcoded here? I think you'd be better
of looping over all properties and blacklist any which shouldn't be
passed through. Similar to how write_properties in domain_build.c does
it.

> +    if ( res )
> +        goto err_out2;
> +
> +    /* Set up msi-num-spis dt property */
> +    prop = dt_get_property(v2m, "arm,msi-num-spis", &len);
> +    if ( !prop )
> +    {
> +        dprintk(XENLOG_ERR, "v2m: Can't find msi-num-spis.\n");
> +        goto err_out2;
> +    }
> +    num_spis = be32_to_cpup(prop);
> +    res = fdt_property(fdt, "arm,msi-num-spis", prop, len);
> +    if ( res )
> +        goto err_out2;
> +
> +    /*
> +     * Currently, we assign all SPIs for MSI to dom0
> +     */
> +    for (i = base_spi; i < (base_spi + num_spis); i++)
> +    {
> +        res = irq_set_spi_type(i, DT_IRQ_TYPE_EDGE_RISING);
> +        if ( res )
> +        {
> +            dprintk(XENLOG_ERR, "v2m: Failed to set MSI interrupt type.\n");
> +            goto err_out2;
> +        }

Needs a vgic_reserve_virq call too I think (in its new home, not here).

Ian.