[PATCH v2 0/7] xen/arm: gic-v2: Detect automatically aliased GIC 400

[PATCH v2 0/7] xen/arm: gic-v2: Detect automatically aliased GIC 400

[Julien Grall]

Hi all,

Only patch #7 is related to the subject of the cover letter. The rest is
clean up of code I looked while I was working on this patch series.

For all the changes, see in each patch.

Sincerely yours,

Julien Grall (7):
  xen/arm: gic: Make it clear the GIC node is passed to
    make_hwdom_dt_node
  xen/arm: Retrieve the correct number of cells when building dom0 DT
  xen/arm: Warn when a device tree path will be re-used by Xen
  xen/arm: vgic-v2: Drop cbase from arch_domain
  xen/arm: gic: Check the size of the CPU and vCPU interface retrieved
    from DT
  xen/arm: gic-v2: Automatically detect aliased GIC400
  xen/arm: platform: Drop the quirks callback

 xen/arch/arm/domain_build.c          | 26 ++++++++--
 xen/arch/arm/gic-hip04.c             | 17 +++----
 xen/arch/arm/gic-v2.c                | 97 +++++++++++++++++++++++++++---------
 xen/arch/arm/gic-v3.c                | 32 ++++++++----
 xen/arch/arm/gic.c                   |  6 ++-
 xen/arch/arm/platform.c              | 10 ----
 xen/arch/arm/platforms/xgene-storm.c |  6 ---
 xen/arch/arm/vgic-v2.c               | 51 ++++++++++++-------
 xen/common/device_tree.c             | 39 +++++++++++++--
 xen/include/asm-arm/domain.h         |  1 -
 xen/include/asm-arm/gic.h            |  6 ++-
 xen/include/asm-arm/platform.h       | 14 ------
 xen/include/asm-arm/vgic.h           |  3 +-
 xen/include/xen/device_tree.h        | 33 ++++++++++++
 14 files changed, 234 insertions(+), 107 deletions(-)

-- 
2.1.4

[PATCH v2 1/7] xen/arm: gic: Make it clear the GIC node is passed to make_hwdom_dt_node

[Julien Grall]

The callback make_hwdom_dt_node already has the GIC node in parameter.

Rather than using a weird mix between "dt_interrupt_controller" (aliased
to "gic") and "node", rename the callback parameter "node" to "gic" and
remove local GIC definitions in terms of the global
dt_interrupt_interrupt_controller.

Also, add an assert to gic_make_hwdom_dt_node to check that the GIC
really is the global dt_interrupt_controller.

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

---
    Changes in v2:
        - Fix typoes
        - Improved commit message based on Ian suggestion
        - Add Ian's acked-by.
---
 xen/arch/arm/gic-hip04.c  | 5 ++---
 xen/arch/arm/gic-v2.c     | 5 ++---
 xen/arch/arm/gic-v3.c     | 9 ++++-----
 xen/arch/arm/gic.c        | 6 ++++--
 xen/include/asm-arm/gic.h | 5 +++--
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index c5ed545..e8cdcd4 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -562,10 +562,9 @@ static void hip04gic_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cp
 }
 
 static int hip04gic_make_hwdom_dt_node(const struct domain *d,
-                                       const struct dt_device_node *node,
+                                       const struct dt_device_node *gic,
                                        void *fdt)
 {
-    const struct dt_device_node *gic = dt_interrupt_controller;
     const void *compatible;
     u32 len;
     const __be32 *regs;
@@ -598,7 +597,7 @@ static int hip04gic_make_hwdom_dt_node(const struct domain *d,
         return -FDT_ERR_XEN(ENOENT);
     }
 
-    len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
+    len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
     len *= 2;
 
     res = fdt_property(fdt, "reg", regs, len);
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 596126d..5841e59 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -552,10 +552,9 @@ static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_m
 }
 
 static int gicv2_make_hwdom_dt_node(const struct domain *d,
-                                    const struct dt_device_node *node,
+                                    const struct dt_device_node *gic,
                                     void *fdt)
 {
-    const struct dt_device_node *gic = dt_interrupt_controller;
     const void *compatible = NULL;
     u32 len;
     const __be32 *regs;
@@ -584,7 +583,7 @@ static int gicv2_make_hwdom_dt_node(const struct domain *d,
         return -FDT_ERR_XEN(ENOENT);
     }
 
-    len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
+    len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
     len *= 2;
 
     res = fdt_property(fdt, "reg", regs, len);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 1e3c19b..957c6e0 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1054,10 +1054,9 @@ static void gicv3_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
 }
 
 static int gicv3_make_hwdom_dt_node(const struct domain *d,
-                                    const struct dt_device_node *node,
+                                    const struct dt_device_node *gic,
                                     void *fdt)
 {
-    const struct dt_device_node *gic = dt_interrupt_controller;
     const void *compatible = NULL;
     uint32_t len;
     __be32 *new_cells, *tmp;
@@ -1084,7 +1083,7 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
+    len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
     /*
      * GIC has two memory regions: Distributor + rdist regions
      * CPU interface and virtual cpu interfaces accessesed as System registers
@@ -1097,10 +1096,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
 
     tmp = new_cells;
 
-    dt_set_range(&tmp, node, d->arch.vgic.dbase, SZ_64K);
+    dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
 
     for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
-        dt_set_range(&tmp, node, d->arch.vgic.rdist_regions[i].base,
+        dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
                      d->arch.vgic.rdist_regions[i].size);
 
     res = fdt_property(fdt, "reg", new_cells, len);
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 1757193..1e1e5ba 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -702,10 +702,12 @@ void __cpuinit init_maintenance_interrupt(void)
 }
 
 int gic_make_hwdom_dt_node(const struct domain *d,
-                           const struct dt_device_node *node,
+                           const struct dt_device_node *gic,
                            void *fdt)
 {
-    return gic_hw_ops->make_hwdom_dt_node(d, node, fdt);
+    ASSERT(gic == dt_interrupt_controller);
+
+    return gic_hw_ops->make_hwdom_dt_node(d, gic, fdt);
 }
 
 /*
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index d343abf..6d53f97 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -350,13 +350,14 @@ struct gic_hw_operations {
     unsigned int (*read_apr)(int apr_reg);
     /* Secondary CPU init */
     int (*secondary_init)(void);
+    /* Create GIC node for the hardware domain */
     int (*make_hwdom_dt_node)(const struct domain *d,
-                              const struct dt_device_node *node, void *fdt);
+                              const struct dt_device_node *gic, void *fdt);
 };
 
 void register_gic_ops(const struct gic_hw_operations *ops);
 int gic_make_hwdom_dt_node(const struct domain *d,
-                           const struct dt_device_node *node,
+                           const struct dt_device_node *gic,
                            void *fdt);
 
 #endif /* __ASSEMBLY__ */
-- 
2.1.4

[PATCH v2 2/7] xen/arm: Retrieve the correct number of cells when building dom0 DT

[Julien Grall]

The functions dt_n_*_cells return the number of cells for a "reg"
property of a given node. So those numbers won't be correct if the
parent of a given node is passed.

This is fine today because the parent is always the root node which
means there is no upper parent.

Introduce new helpers dt_child_n_*_cells to retrieve the number of
cells for the address and size that can be used to create the "reg"
property of the immediate child of a given parent. Also introduce
dt_child_set_range to pair up with dt_child_n_*_cells.

Use the new helpers when creating the hypervisor and memory node where
we only have the parent in hand. This is because those nodes are created
from scratch by Xen and therefore we don't have a dt_device_node for
them. The only thing we have is a pointer to their future parent.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---

    Changes in v2:
        - Keep same behavior as before for dt_set_range and introduce
        dt_child_set_range.
        - Typoes and update in the commit message
---
 xen/arch/arm/domain_build.c   | 10 +++++-----
 xen/common/device_tree.c      | 39 +++++++++++++++++++++++++++++++++++----
 xen/include/xen/device_tree.h | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 72b1845..e8f024f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -569,7 +569,7 @@ static int make_memory_node(const struct domain *d,
                             const struct kernel_info *kinfo)
 {
     int res, i;
-    int reg_size = dt_n_addr_cells(parent) + dt_n_size_cells(parent);
+    int reg_size = dt_child_n_addr_cells(parent) + dt_child_n_size_cells(parent);
     int nr_cells = reg_size*kinfo->mem.nr_banks;
     __be32 reg[nr_cells];
     __be32 *cells;
@@ -594,7 +594,7 @@ static int make_memory_node(const struct domain *d,
         DPRINT("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
                 i, start, start + size);
 
-        dt_set_range(&cells, parent, start, size);
+        dt_child_set_range(&cells, parent, start, size);
     }
 
     res = fdt_property(fdt, "reg", reg, sizeof(reg));
@@ -617,8 +617,8 @@ static int make_hypervisor_node(const struct kernel_info *kinfo,
     __be32 *cells;
     int res;
     /* Convenience alias */
-    int addrcells = dt_n_addr_cells(parent);
-    int sizecells = dt_n_size_cells(parent);
+    int addrcells = dt_child_n_addr_cells(parent);
+    int sizecells = dt_child_n_size_cells(parent);
     void *fdt = kinfo->fdt;
 
     DPRINT("Create hypervisor node\n");
@@ -643,7 +643,7 @@ static int make_hypervisor_node(const struct kernel_info *kinfo,
 
     /* reg 0 is grant table space */
     cells = &reg[0];
-    dt_set_range(&cells, parent, kinfo->gnttab_start, kinfo->gnttab_size);
+    dt_child_set_range(&cells, parent, kinfo->gnttab_start, kinfo->gnttab_size);
     res = fdt_property(fdt, "reg", reg,
                        dt_cells_to_size(addrcells + sizecells));
     if ( res )
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 18cdb6f..87c3f71 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -117,6 +117,13 @@ void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
     dt_set_cell(cellp, dt_n_size_cells(np), size);
 }
 
+void dt_child_set_range(__be32 **cellp, const struct dt_device_node *parent,
+                        u64 address, u64 size)
+{
+    dt_set_cell(cellp, dt_child_n_addr_cells(parent), address);
+    dt_set_cell(cellp, dt_child_n_size_cells(parent), size);
+}
+
 static void __init *unflatten_dt_alloc(unsigned long *mem, unsigned long size,
                                        unsigned long align)
 {
@@ -386,13 +393,15 @@ dt_find_matching_node(struct dt_device_node *from,
     return NULL;
 }
 
-int dt_n_addr_cells(const struct dt_device_node *np)
+static int __dt_n_addr_cells(const struct dt_device_node *np, bool_t parent)
 {
     const __be32 *ip;
 
     do {
-        if ( np->parent )
+        if ( np->parent && !parent )
             np = np->parent;
+        parent = false;
+
         ip = dt_get_property(np, "#address-cells", NULL);
         if ( ip )
             return be32_to_cpup(ip);
@@ -401,13 +410,15 @@ int dt_n_addr_cells(const struct dt_device_node *np)
     return DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
 }
 
-int dt_n_size_cells(const struct dt_device_node *np)
+int __dt_n_size_cells(const struct dt_device_node *np, bool_t parent)
 {
     const __be32 *ip;
 
     do {
-        if ( np->parent )
+        if ( np->parent && !parent )
             np = np->parent;
+        parent = false;
+
         ip = dt_get_property(np, "#size-cells", NULL);
         if ( ip )
             return be32_to_cpup(ip);
@@ -416,6 +427,26 @@ int dt_n_size_cells(const struct dt_device_node *np)
     return DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
 }
 
+int dt_n_addr_cells(const struct dt_device_node *np)
+{
+    return __dt_n_addr_cells(np, false);
+}
+
+int dt_n_size_cells(const struct dt_device_node *np)
+{
+    return __dt_n_size_cells(np, false);
+}
+
+int dt_child_n_addr_cells(const struct dt_device_node *parent)
+{
+    return __dt_n_addr_cells(parent, true);
+}
+
+int dt_child_n_size_cells(const struct dt_device_node *parent)
+{
+    return __dt_n_size_cells(parent, true);
+}
+
 /*
  * These are defined in Linux where much of this code comes from, but
  * are currently unused outside this file in the context of Xen.
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 46c5ba8..5c03f40 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -584,6 +584,25 @@ int dt_n_size_cells(const struct dt_device_node *np);
 int dt_n_addr_cells(const struct dt_device_node *np);
 
 /**
+ * dt_child_n_size_cells - Helper to retrieve the number of cell for the size
+ * @parent: parent of the child to get the value
+ *
+ * This function retrieves for a given device-tree node the number of
+ * cell for the size field of there child
+ */
+int dt_child_n_size_cells(const struct dt_device_node *parent);
+
+/**
+ * dt_child_n_addr_cells - Helper to retrieve the number of cell for the
+ * address
+ * @parent: parent of the child to get the value
+ *
+ * This function retrieves for a given device-tree node the number of
+ * cell for the address field of there child
+ */
+int dt_child_n_addr_cells(const struct dt_device_node *parent);
+
+/**
  * dt_device_is_available - Check if a device is available for use
  *
  * @device: Node to check for availability
@@ -655,6 +674,20 @@ void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
                   u64 address, u64 size);
 
 /**
+ * dt_child_set_range - Write range into a series of cells
+ *
+ * @cellp: Pointer to cells
+ * @parent: Parent node which contains the encode for the address and the size
+ * @address: Start of range
+ * @size: Size of the range
+ *
+ * Write a range into a series of cells and update cellp to point to the
+ * cell just after.
+ */
+void dt_child_set_range(__be32 **cellp, const struct dt_device_node *parent,
+                        u64 address, u64 size);
+
+/**
  * dt_get_range - Read a range (address/size) from a series of cells
  *
  * @cellp: Pointer to cells
-- 
2.1.4

[PATCH v2 3/7] xen/arm: Warn when a device tree path will be re-used by Xen

[Julien Grall]

Xen is unconditionally using certain device tree paths to create DOM0
specific node (for instance /psci, /memory and /hypervisor).

Print a warning message on the console to let the user know if we
re-use one of these nodes.

Note that the content of most of those is very common and they
should have already been skipped via the compatible string or typer
string. This warning is here to catch unusual device-tree and
compatible string that we may not yet support in Xen.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
    Changes in v2:
        - Update commit message
        - Update the warning message
---
 xen/arch/arm/domain_build.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e8f024f..eb93a3a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1205,6 +1205,13 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         DT_MATCH_TIMER,
         { /* sentinel */ },
     };
+    static const struct dt_device_match reserved_matches[] __initconst =
+    {
+        DT_MATCH_PATH("/psci"),
+        DT_MATCH_PATH("/memory"),
+        DT_MATCH_PATH("/hypervisor"),
+        { /* sentinel */ },
+    };
     struct dt_device_node *child;
     int res;
     const char *name;
@@ -1252,6 +1259,15 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         return 0;
     }
 
+    /*
+     * Xen is using some path for its own purpose. Warn if a node
+     * already exists with the same path.
+     */
+    if ( dt_match_node(reserved_matches, node) )
+        printk(XENLOG_WARNING
+               "WARNING: Path %s is reserved, skip the node as we may re-use the path.\n",
+               path);
+
     res = handle_device(d, node);
     if ( res)
         return res;
-- 
2.1.4

[PATCH v2 4/7] xen/arm: vgic-v2: Drop cbase from arch_domain

[Julien Grall]

The field value is only used within a single function in the vgic-v2
emulation. So it's not necessary to store the value in the domain
structure.

This is also saving 8 bytes on a structure which begin to be constrained
(the maximum size of struct domain is 4KB).

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

---
    Changes in v2:
        - Add Ian's acked-by
---
 xen/arch/arm/vgic-v2.c       | 11 ++++++-----
 xen/include/asm-arm/domain.h |  1 -
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index fa71598..ecd6bf3 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -546,6 +546,7 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
 static int vgic_v2_domain_init(struct domain *d)
 {
     int i, ret;
+    paddr_t cbase;
 
     /*
      * The hardware domain gets the hardware address.
@@ -554,12 +555,12 @@ static int vgic_v2_domain_init(struct domain *d)
     if ( is_hardware_domain(d) )
     {
         d->arch.vgic.dbase = vgic_v2_hw.dbase;
-        d->arch.vgic.cbase = vgic_v2_hw.cbase;
+        cbase = vgic_v2_hw.cbase;
     }
     else
     {
         d->arch.vgic.dbase = GUEST_GICD_BASE;
-        d->arch.vgic.cbase = GUEST_GICC_BASE;
+        cbase = GUEST_GICC_BASE;
     }
 
     /*
@@ -569,16 +570,16 @@ static int vgic_v2_domain_init(struct domain *d)
      * The second page is always mapped at +4K irrespective of the
      * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
      */
-    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
+    ret = map_mmio_regions(d, paddr_to_pfn(cbase), 1,
                            paddr_to_pfn(vgic_v2_hw.vbase));
     if ( ret )
         return ret;
 
     if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
+        ret = map_mmio_regions(d, paddr_to_pfn(cbase + PAGE_SIZE),
                                1, paddr_to_pfn(vgic_v2_hw.vbase + PAGE_SIZE));
     else
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
+        ret = map_mmio_regions(d, paddr_to_pfn(cbase + PAGE_SIZE),
                                1, paddr_to_pfn(vgic_v2_hw.vbase + SZ_64K));
 
     if ( ret )
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c3f5a95..ba430a7 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -101,7 +101,6 @@ struct arch_domain
         struct pending_irq *pending_irqs;
         /* Base address for guest GIC */
         paddr_t dbase; /* Distributor base address */
-        paddr_t cbase; /* CPU base address */
 #ifdef HAS_GICV3
         /* GIC V3 addressing */
         /* List of contiguous occupied by the redistributors */
-- 
2.1.4

[PATCH v2 5/7] xen/arm: gic: Check the size of the CPU and vCPU interface retrieved from DT

[Julien Grall]

The size of the CPU interface will used in a follow-up patch to map the
region in Xen memory.

Based on GICv2 spec, the CPU interface should at least be 8KB, although
most of the platform we are supporting use incorrectly the GICv1 size
(i.e 4KB) in their DT. Only warn and update the size to avoid any
breakage on these platforms.

Furthermore, Xen is relying on the fact that the Virtual CPU interface
been at least 8KB. As in reality the Virtual CPU interface matches the CPU
interface, check that the 2 interfaces have the same size. Also only warn,
to avoid any breakage with buggy DT.

For GICv3, only allow GICv2 compatibility when the Virtual CPU interface
and CPU interface are 8KB.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>

I haven't done any change in the gic-hip04 driver. I will let the
maintainers doing it if they feel it's necessary.

The check on the CPU interface size has not been modified because if we
really want to check the validity of it, we would have to check at least
SZ_4K, SZ_8K and SZ_128K. Although I'm not sure if there is other
possible values.

    Changes in v2:
        - Fix typoes in the commit message
        - Fix typoes in warning messages
        - Avoid to split string literally in the middle of the sence, so
        grep still works.
        - csize should be checked against vsize and not vbase!
---
 xen/arch/arm/gic-v2.c | 33 +++++++++++++++++++++++++++++----
 xen/arch/arm/gic-v3.c | 21 ++++++++++++++++++---
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 5841e59..49f5960 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -617,14 +617,16 @@ static hw_irq_controller gicv2_guest_irq_type = {
 static int __init gicv2_init(void)
 {
     int res;
-    paddr_t hbase, dbase, cbase, vbase;
+    paddr_t hbase, dbase;
+    paddr_t cbase, csize;
+    paddr_t vbase, vsize;
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &dbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the distributor");
 
-    res = dt_device_get_address(node, 1, &cbase, NULL);
+    res = dt_device_get_address(node, 1, &cbase, &csize);
     if ( res )
         panic("GICv2: Cannot find a valid address for the CPU");
 
@@ -632,7 +634,7 @@ static int __init gicv2_init(void)
     if ( res )
         panic("GICv2: Cannot find a valid address for the hypervisor");
 
-    res = dt_device_get_address(node, 3, &vbase, NULL);
+    res = dt_device_get_address(node, 3, &vbase, &vsize);
     if ( res )
         panic("GICv2: Cannot find a valid address for the virtual CPU");
 
@@ -641,7 +643,30 @@ static int __init gicv2_init(void)
         panic("GICv2: Cannot find the maintenance IRQ");
     gicv2_info.maintenance_irq = res;
 
-    /* TODO: Add check on distributor, cpu size */
+    /* TODO: Add check on distributor */
+
+    /*
+     * The GICv2 CPU interface should at least be 8KB. Although, most of the DT
+     * doesn't correctly set it and use the GICv1 CPU interface size (i.e 4KB).
+     * Warn and then fixup.
+     */
+    if ( csize < SZ_8K )
+    {
+        printk(XENLOG_WARNING "GICv2: WARNING: "
+               "The CPU interface size is wrong: %#"PRIx64
+               " expected %#x\n",
+               csize, SZ_8K);
+        csize = SZ_8K;
+    }
+
+    /*
+     * Check if the CPU interface and virtual CPU interface have the
+     * same size.
+     */
+    if ( csize != vsize )
+        printk(XENLOG_WARNING "GICv2: WARNING: "
+               "The size of the CPU interface (%#"PRIpaddr") and the vCPU interface (%#"PRIpaddr") don't match\n",
+               csize, vsize);
 
     printk("GICv2 initialization:\n"
               "        gic_dist_addr=%"PRIpaddr"\n"
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 957c6e0..2b6d923 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1143,22 +1143,37 @@ static void __init gicv3_init_v2(const struct dt_device_node *node,
                                  paddr_t dbase)
 {
     int res;
-    paddr_t cbase, vbase;
+    paddr_t cbase, csize;
+    paddr_t vbase, vsize;
 
     /*
      * For GICv3 supporting GICv2, GICC and GICV base address will be
      * provided.
      */
     res = dt_device_get_address(node, 1 + gicv3.rdist_count,
-                                &cbase, NULL);
+                                &cbase, &csize);
     if ( res )
         return;
 
     res = dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
-                                &vbase, NULL);
+                                &vbase, &vsize);
     if ( res )
         return;
 
+    /*
+     * Only allow support of GICv2 compatible when the CPU interface
+     * and virtual CPU interface are 8KB
+     * XXX: Handle other size?
+     */
+    if ( csize != SZ_8K && vsize != SZ_8K )
+    {
+        printk(XENLOG_WARNING
+               "GICv3: WARNING: Not enabling support of GICv2 compat mode.\n"
+               "The size of the CPU interface (%#"PRIpaddr") and the vCPU interface (%#"PRIpaddr") should both be 8KB.\n",
+               csize, vsize);
+        return;
+    }
+
     printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
            cbase, vbase);
 
-- 
2.1.4

[PATCH v2 6/7] xen/arm: gic-v2: Automatically detect aliased GIC400

[Julien Grall]

We are currently using a per-platform quirk to know if the 2 4KB region of
the GIC CPU interface are each aligned to 64KB. Although, it may be
possible to have different layout on a same platform (depending on the
firmware version).

Rather than having a quirk it's possible to detect by reading the GIC
memory. This patch is based from the Linux commit "irqchip/GIC: Add workaround
for aliased GIC400" [1].

Take the opportunity to clean up the GICv2 of code which was only
required because of the quirk.

Note that none of the platform using the gic-hip04 were actually using
the quirk, so the code has been dropped. I will let the maintainers
decide whether it's relevant or not to add proper detection for aliased
GIC for this hardware.

[1] commit 12e14066f4835f5ee1ca795f0309415b54c067a9
    Author: Marc Zyngier <marc.zyngier@arm.com>
    Date:   Sun Sep 13 12:14:31 2015 +0100

    irqchip/GIC: Add workaround for aliased GIC400

    The GICv2 architecture mandates that the two 4kB GIC regions are
    contiguous, and on two separate physical pages (so that access to
    the second page can be trapped by a hypervisor). This doesn't work
    very well when PAGE_SIZE is 64kB.

    A relatively common hack^Wway to work around this is to alias each
    4kB region over its own 64kB page. Of course in this case, the base
    address you want to use is not really the begining of the region,
    but base + 60kB (so that you get a contiguous 8kB region over two
    distinct pages).

    Normally, this would be described in DT with a new property, but
    some HW is already out there, and the firmware makes sure that
    it will override whatever you put in the GIC node. Duh. And of course,
    said firmware source code is not available, despite being based
    on u-boot.

    The workaround is to detect the case where the CPU interface size
    is set to 128kB, and verify the aliasing by checking that the ID
    register for GIC400 (which is the only GIC wired this way so far)
    is the same at base and base + 0xF000. In this case, we update
    the GIC base address and let it roll.

    And if you feel slightly sick by looking at this, rest assured that
    I do too...

    Reported-by: Julien Grall <julien.grall@citrix.com>
    Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
    Cc: linux-arm-kernel@lists.infradead.org
    Cc: Stuart Yoder <stuart.yoder@freescale.com>
    Cc: Pavel Fedin <p.fedin@samsung.com>
    Cc: Jason Cooper <jason@lakedaemon.net>
    Link: http://lkml.kernel.org/r/1442142873-20213-2-git-send-email-marc.zyngier@arm.com
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>

    Changes in v2:
        - Drop XXX for gic-v3 and gic-hip04 as won't support aliased GIC
        - Fix typoes
---
 xen/arch/arm/gic-hip04.c             | 12 +++-----
 xen/arch/arm/gic-v2.c                | 59 ++++++++++++++++++++++++++----------
 xen/arch/arm/gic-v3.c                |  2 +-
 xen/arch/arm/platforms/xgene-storm.c |  6 ----
 xen/arch/arm/vgic-v2.c               | 48 ++++++++++++++++++-----------
 xen/include/asm-arm/gic.h            |  1 +
 xen/include/asm-arm/platform.h       |  6 ----
 xen/include/asm-arm/vgic.h           |  3 +-
 8 files changed, 81 insertions(+), 56 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index e8cdcd4..310f35a 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -631,14 +631,14 @@ static hw_irq_controller hip04gic_guest_irq_type = {
 static int __init hip04gic_init(void)
 {
     int res;
-    paddr_t hbase, dbase, cbase, vbase;
+    paddr_t hbase, dbase, cbase, csize, vbase;
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &dbase, NULL);
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the distributor");
 
-    res = dt_device_get_address(node, 1, &cbase, NULL);
+    res = dt_device_get_address(node, 1, &cbase, &csize);
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the CPU");
 
@@ -675,11 +675,7 @@ static int __init hip04gic_init(void)
         panic("GIC-HIP04: Failed to ioremap for GIC distributor\n");
 
     gicv2.map_cbase[0] = ioremap_nocache(cbase, PAGE_SIZE);
-
-    if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(cbase + SZ_64K, PAGE_SIZE);
-    else
-        gicv2.map_cbase[1] = ioremap_nocache(cbase + PAGE_SIZE, PAGE_SIZE);
+    gicv2.map_cbase[1] = ioremap_nocache(cbase + PAGE_SIZE, PAGE_SIZE);
 
     if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
         panic("GIC-HIP04: Failed to ioremap for GIC CPU interface\n");
@@ -688,7 +684,7 @@ static int __init hip04gic_init(void)
     if ( !gicv2.map_hbase )
         panic("GIC-HIP04: Failed to ioremap for GIC Virtual interface\n");
 
-    vgic_v2_setup_hw(dbase, cbase, vbase);
+    vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0);
 
     /* Global settings: interrupt distributor */
     spin_lock_init(&gicv2.lock);
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 49f5960..7a26dff 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -65,7 +65,7 @@
 /* Global state */
 static struct {
     void __iomem * map_dbase; /* IO mapped Address of distributor registers */
-    void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
+    void __iomem * map_cbase; /* IO mapped Address of CPU interface registers */
     void __iomem * map_hbase; /* IO Address of virtual interface registers */
     spinlock_t lock;
 } gicv2;
@@ -98,16 +98,12 @@ static inline uint32_t readl_gicd(unsigned int offset)
 
 static inline void writel_gicc(uint32_t val, unsigned int offset)
 {
-    unsigned int page = offset >> PAGE_SHIFT;
-    offset &= ~PAGE_MASK;
-    writel_relaxed(val, gicv2.map_cbase[page] + offset);
+    writel_relaxed(val, gicv2.map_cbase + offset);
 }
 
 static inline uint32_t readl_gicc(unsigned int offset)
 {
-    unsigned int page = offset >> PAGE_SHIFT;
-    offset &= ~PAGE_MASK;
-    return readl_relaxed(gicv2.map_cbase[page] + offset);
+    return readl_relaxed(gicv2.map_cbase + offset);
 }
 
 static inline void writel_gich(uint32_t val, unsigned int offset)
@@ -614,12 +610,31 @@ static hw_irq_controller gicv2_guest_irq_type = {
     .set_affinity = gicv2_irq_set_affinity,
 };
 
+static bool_t gicv2_is_aliased(paddr_t cbase, paddr_t csize)
+{
+    uint32_t val_low, val_high;
+
+    if ( csize != SZ_128K )
+        return false;
+
+    /*
+     * Verify that we have the first 4kB of a GIC400
+     * aliased over the first 64kB by checking the
+     * GICC_IIDR register on both ends.
+     */
+    val_low = readl_gicc(GICC_IIDR);
+    val_high = readl_gicc(GICC_IIDR + 0xf000);
+
+    return ((val_low & 0xfff0fff) == 0x0202043B && val_low == val_high);
+}
+
 static int __init gicv2_init(void)
 {
     int res;
     paddr_t hbase, dbase;
     paddr_t cbase, csize;
     paddr_t vbase, vsize;
+    uint32_t aliased_offset;
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &dbase, NULL);
@@ -685,21 +700,33 @@ static int __init gicv2_init(void)
     if ( !gicv2.map_dbase )
         panic("GICv2: Failed to ioremap for GIC distributor\n");
 
-    gicv2.map_cbase[0] = ioremap_nocache(cbase, PAGE_SIZE);
-
-    if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(cbase + SZ_64K, PAGE_SIZE);
-    else
-        gicv2.map_cbase[1] = ioremap_nocache(cbase + PAGE_SIZE, PAGE_SIZE);
-
-    if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
+    gicv2.map_cbase = ioremap_nocache(cbase, csize);
+    if ( !gicv2.map_cbase )
         panic("GICv2: Failed to ioremap for GIC CPU interface\n");
 
+    if ( gicv2_is_aliased(cbase, csize) )
+    {
+        /*
+         * Move the base up by 60kB, so that we have a 8kB contiguous
+         * region, which allows us to use GICC_DIR at its
+         * normal offset.
+         * Note the variable cbase is not updated as we need the original
+         * value for the vGICv2 emulation.
+         */
+        aliased_offset = 0xf000;
+
+        gicv2.map_cbase += aliased_offset;
+
+        printk(XENLOG_WARNING
+               "GICv2: Adjusting CPU interface base to %#"PRIx64"\n",
+               cbase + aliased_offset);
+    }
+
     gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE);
     if ( !gicv2.map_hbase )
         panic("GICv2: Failed to ioremap for GIC Virtual interface\n");
 
-    vgic_v2_setup_hw(dbase, cbase, vbase);
+    vgic_v2_setup_hw(dbase, cbase, csize, vbase, aliased_offset);
 
     /* Global settings: interrupt distributor */
     spin_lock_init(&gicv2.lock);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 2b6d923..df965dd 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1177,7 +1177,7 @@ static void __init gicv3_init_v2(const struct dt_device_node *node,
     printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
            cbase, vbase);
 
-    vgic_v2_setup_hw(dbase, cbase, vbase);
+    vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0);
 }
 
 /* Set up the GIC */
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 8b05ed5..70cb655 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -68,11 +68,6 @@ static void __init xgene_check_pirq_eoi(void)
               "Please upgrade your firmware to the latest version");
 }
 
-static uint32_t xgene_storm_quirks(void)
-{
-    return PLATFORM_QUIRK_GIC_64K_STRIDE;
-}
-
 static void xgene_storm_reset(void)
 {
     void __iomem *addr;
@@ -122,7 +117,6 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
     .compatible = xgene_storm_dt_compat,
     .init = xgene_storm_init,
     .reset = xgene_storm_reset,
-    .quirks = xgene_storm_quirks,
 PLATFORM_END
 
 /*
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index ecd6bf3..734384e 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -36,18 +36,25 @@ static struct {
     bool_t enabled;
     /* Distributor interface address */
     paddr_t dbase;
-    /* CPU interface address */
+    /* CPU interface address & size */
     paddr_t cbase;
+    paddr_t csize;
     /* Virtual CPU interface address */
     paddr_t vbase;
+
+    /* Offset to add to get an 8kB contiguous region if GIC is aliased */
+    uint32_t aliased_offset;
 } vgic_v2_hw;
 
-void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t vbase)
+void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
+                      paddr_t vbase, uint32_t aliased_offset)
 {
     vgic_v2_hw.enabled = 1;
     vgic_v2_hw.dbase = dbase;
     vgic_v2_hw.cbase = cbase;
+    vgic_v2_hw.csize = csize;
     vgic_v2_hw.vbase = vbase;
+    vgic_v2_hw.aliased_offset = aliased_offset;
 }
 
 static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
@@ -546,7 +553,8 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
 static int vgic_v2_domain_init(struct domain *d)
 {
     int i, ret;
-    paddr_t cbase;
+    paddr_t cbase, csize;
+    paddr_t vbase;
 
     /*
      * The hardware domain gets the hardware address.
@@ -555,33 +563,37 @@ static int vgic_v2_domain_init(struct domain *d)
     if ( is_hardware_domain(d) )
     {
         d->arch.vgic.dbase = vgic_v2_hw.dbase;
+        /*
+         * For the hardware domain, we always map the whole HW CPU
+         * interface region in order to match the device tree (the "reg"
+         * properties is copied as it is).
+         * Note that we assume the size of the CPU interface is always
+         * aligned to PAGE_SIZE.
+         */
         cbase = vgic_v2_hw.cbase;
+        csize = vgic_v2_hw.csize;
+        vbase = vgic_v2_hw.vbase;
     }
     else
     {
         d->arch.vgic.dbase = GUEST_GICD_BASE;
+        /*
+         * The CPU interface exposed to the guest is always 8kB. We may
+         * need to add an offset to the virtual CPU interface base
+         * address when in the GIC is aliased to get a 8kB contiguous
+         * region.
+         */
         cbase = GUEST_GICC_BASE;
+        csize = SZ_8K;
+        vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
     }
 
     /*
      * Map the gic virtual cpu interface in the gic cpu interface
      * region of the guest.
-     *
-     * The second page is always mapped at +4K irrespective of the
-     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
      */
-    ret = map_mmio_regions(d, paddr_to_pfn(cbase), 1,
-                           paddr_to_pfn(vgic_v2_hw.vbase));
-    if ( ret )
-        return ret;
-
-    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        ret = map_mmio_regions(d, paddr_to_pfn(cbase + PAGE_SIZE),
-                               1, paddr_to_pfn(vgic_v2_hw.vbase + PAGE_SIZE));
-    else
-        ret = map_mmio_regions(d, paddr_to_pfn(cbase + PAGE_SIZE),
-                               1, paddr_to_pfn(vgic_v2_hw.vbase + SZ_64K));
-
+    ret = map_mmio_regions(d, paddr_to_pfn(cbase), csize / PAGE_SIZE,
+                           paddr_to_pfn(vbase));
     if ( ret )
         return ret;
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 6d53f97..0116481 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -76,6 +76,7 @@
 #define GICC_HPPIR      (0x0018)
 #define GICC_APR        (0x00D0)
 #define GICC_NSAPR      (0x00E0)
+#define GICC_IIDR       (0x00FC)
 #define GICC_DIR        (0x1000)
 
 #define GICH_HCR        (0x00)
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index b8fc5ac..5e462ac 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -39,12 +39,6 @@ struct platform_desc {
     const struct dt_device_match *blacklist_dev;
 };
 
-/*
- * Quirk for platforms where the 4K GIC register ranges are placed at
- * 64K stride.
- */
-#define PLATFORM_QUIRK_GIC_64K_STRIDE (1 << 0)
-
 void __init platform_init(void);
 int __init platform_init_time(void);
 int __init platform_specific_mapping(struct domain *d);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 96839f0..c7cc247 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -236,7 +236,8 @@ static inline int vgic_allocate_spi(struct domain *d)
 
 extern void vgic_free_virq(struct domain *d, unsigned int virq);
 
-void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t vbase);
+void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
+                      paddr_t vbase, uint32_t aliased_offset);
 
 #ifdef HAS_GICV3
 struct rdist_region;
-- 
2.1.4

[PATCH v2 7/7] xen/arm: platform: Drop the quirks callback

[Julien Grall]

All the quirks has been replaced by proper detection. Lets drop the
callback and hope that no one will need new quirks.

At the same time, remove the definition platform_dom0_evtchn_ppi with is
not used any more.

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

---
    Changes in v2:
        - Add Ian's acked-by
---
 xen/arch/arm/platform.c        | 10 ----------
 xen/include/asm-arm/platform.h |  8 --------
 2 files changed, 18 deletions(-)

diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 0af6d57..b0bfaa9 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -127,16 +127,6 @@ void platform_poweroff(void)
         platform->poweroff();
 }
 
-bool_t platform_has_quirk(uint32_t quirk)
-{
-    uint32_t quirks = 0;
-
-    if ( platform && platform->quirks )
-        quirks = platform->quirks();
-
-    return !!(quirks & quirk);
-}
-
 bool_t platform_device_is_blacklisted(const struct dt_device_node *node)
 {
     const struct dt_device_match *blacklist = NULL;
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index 5e462ac..f97315d 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -27,12 +27,6 @@ struct platform_desc {
     /* Platform power-off */
     void (*poweroff)(void);
     /*
-     * Platform quirks
-     * Defined has a function because a platform can support multiple
-     * board with different quirk on each
-     */
-    uint32_t (*quirks)(void);
-    /*
      * Platform blacklist devices
      * List of devices which must not pass-through to a guest
      */
@@ -48,9 +42,7 @@ int platform_cpu_up(int cpu);
 #endif
 void platform_reset(void);
 void platform_poweroff(void);
-bool_t platform_has_quirk(uint32_t quirk);
 bool_t platform_device_is_blacklisted(const struct dt_device_node *node);
-unsigned int platform_dom0_evtchn_ppi(void);
 
 #define PLATFORM_START(_name, _namestr)                         \
 static const struct platform_desc  __plat_desc_##_name __used   \
-- 
2.1.4

Re: [PATCH v2 7/7] xen/arm: platform: Drop the quirks callback

[Julien Grall]

On 29/09/15 17:21, Julien Grall wrote:
> All the quirks has been replaced by proper detection. Lets drop the
> callback and hope that no one will need new quirks.
> 
> At the same time, remove the definition platform_dom0_evtchn_ppi with is
> not used any more.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Acked-by: Ian Campbell <ian.campebll@citrix.com>

I made a typo while writing the acked-by. It should campbell and not
campebll.

I will resend a new version if necessary.

Regards,

-- 
Julien Grall

Re: [PATCH v2 2/7] xen/arm: Retrieve the correct number of cells when building dom0 DT

[Ian Campbell]

On Tue, 2015-09-29 at 17:21 +0100, Julien Grall wrote:
> The functions dt_n_*_cells return the number of cells for a "reg"
> property of a given node. So those numbers won't be correct if the
> parent of a given node is passed.
> 
> This is fine today because the parent is always the root node which
> means there is no upper parent.
> 
> Introduce new helpers dt_child_n_*_cells to retrieve the number of
> cells for the address and size that can be used to create the "reg"
> property of the immediate child of a given parent. Also introduce
> dt_child_set_range to pair up with dt_child_n_*_cells.
> 
> Use the new helpers when creating the hypervisor and memory node where
> we only have the parent in hand. This is because those nodes are created
> from scratch by Xen and therefore we don't have a dt_device_node for
> them. The only thing we have is a pointer to their future parent.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

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

Re: [PATCH v2 3/7] xen/arm: Warn when a device tree path will be re-used by Xen

[Ian Campbell]

On Tue, 2015-09-29 at 17:21 +0100, Julien Grall wrote:
> Xen is unconditionally using certain device tree paths to create DOM0
> specific node (for instance /psci, /memory and /hypervisor).
> 
> Print a warning message on the console to let the user know if we
> re-use one of these nodes.
> 
> Note that the content of most of those is very common and they
> should have already been skipped via the compatible string or typer

Stray "r" at the end (fixable on commit)

> string. This warning is here to catch unusual device-tree and
> compatible string that we may not yet support in Xen.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

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

Should we consider using /xen-psci or something to avoid this conflict? Or
(worse IMHO) adding a -N until we find an unused path?

Re: [PATCH v2 5/7] xen/arm: gic: Check the size of the CPU and vCPU interface retrieved from DT

[Ian Campbell]

On Tue, 2015-09-29 at 17:21 +0100, Julien Grall wrote:
> @@ -641,7 +643,30 @@ static int __init gicv2_init(void)
>          panic("GICv2: Cannot find the maintenance IRQ");
>      gicv2_info.maintenance_irq = res;
>  
> -    /* TODO: Add check on distributor, cpu size */
> +    /* TODO: Add check on distributor */
> +
> +    /*
> +     * The GICv2 CPU interface should at least be 8KB. Although, most of the DT
> +     * doesn't correctly set it and use the GICv1 CPU interface size (i.e 4KB).
> +     * Warn and then fixup.
> +     */
> +    if ( csize < SZ_8K )
> +    {
> +        printk(XENLOG_WARNING "GICv2: WARNING: "
> +               "The CPU interface size is wrong: %#"PRIx64
> +               " expected %#x\n",

You missed fixing a split string constant here.

> +               csize, SZ_8K);
> +        csize = SZ_8K;
> +    }
> +
> +    /*
> +     * Check if the CPU interface and virtual CPU interface have the
> +     * same size.
> +     */
> +    if ( csize != vsize )
> +        printk(XENLOG_WARNING "GICv2: WARNING: "
> +               "The size of the CPU interface (%#"PRIpaddr") and the vCPU interface (%#"PRIpaddr") don't match\n",

Apart from the wrapping this is also just quite a long line in its own
right (100+ characters with the prefix on the preceeding line) e.g. for
reading a serial log.

How about s/the CPU interface/GICC/ and s/the vCPU interface/GICV/ ?

And maybe s/The size/Sizes/?

>  
> +    /*
> +     * Only allow support of GICv2 compatible when the CPU interface
> +     * and virtual CPU interface are 8KB
> +     * XXX: Handle other size?
> +     */
> +    if ( csize != SZ_8K && vsize != SZ_8K )
> +    {
> +        printk(XENLOG_WARNING
> +               "GICv3: WARNING: Not enabling support of GICv2 compat mode.\n"

s/of/for/

> +               "The size of the CPU interface (%#"PRIpaddr") and the vCPU interface (%#"PRIpaddr") should both be 8KB.\n",

Similarly here. Or just "GICC ("%#...") and GICV ("%#...") must both be 8KB"?

Re: [PATCH v2 6/7] xen/arm: gic-v2: Automatically detect aliased GIC400

[Ian Campbell]

On Tue, 2015-09-29 at 17:21 +0100, Julien Grall wrote:
> We are currently using a per-platform quirk to know if the 2 4KB region
> of
> the GIC CPU interface are each aligned to 64KB. Although, it may be
> possible to have different layout on a same platform (depending on the
> firmware version).
> 
> Rather than having a quirk it's possible to detect by reading the GIC
> memory. This patch is based from the Linux commit "irqchip/GIC: Add
> workaround
> for aliased GIC400" [1].
> 
> Take the opportunity to clean up the GICv2 of code which was only
> required because of the quirk.
> 
> Note that none of the platform using the gic-hip04 were actually using
> the quirk, so the code has been dropped. I will let the maintainers
> decide whether it's relevant or not to add proper detection for aliased
> GIC for this hardware.
> 
> [1] commit 12e14066f4835f5ee1ca795f0309415b54c067a9
>     Author: Marc Zyngier <marc.zyngier@arm.com>
>     Date:   Sun Sep 13 12:14:31 2015 +0100
> 
>     irqchip/GIC: Add workaround for aliased GIC400
> 
>     The GICv2 architecture mandates that the two 4kB GIC regions are
>     contiguous, and on two separate physical pages (so that access to
>     the second page can be trapped by a hypervisor). This doesn't work
>     very well when PAGE_SIZE is 64kB.
> 
>     A relatively common hack^Wway to work around this is to alias each
>     4kB region over its own 64kB page. Of course in this case, the base
>     address you want to use is not really the begining of the region,
>     but base + 60kB (so that you get a contiguous 8kB region over two
>     distinct pages).
> 
>     Normally, this would be described in DT with a new property, but
>     some HW is already out there, and the firmware makes sure that
>     it will override whatever you put in the GIC node. Duh. And of
> course,
>     said firmware source code is not available, despite being based
>     on u-boot.
> 
>     The workaround is to detect the case where the CPU interface size
>     is set to 128kB, and verify the aliasing by checking that the ID
>     register for GIC400 (which is the only GIC wired this way so far)
>     is the same at base and base + 0xF000. In this case, we update
>     the GIC base address and let it roll.
> 
>     And if you feel slightly sick by looking at this, rest assured that
>     I do too...
> 
>     Reported-by: Julien Grall <julien.grall@citrix.com>
>     Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>     Cc: linux-arm-kernel@lists.infradead.org
>     Cc: Stuart Yoder <stuart.yoder@freescale.com>
>     Cc: Pavel Fedin <p.fedin@samsung.com>
>     Cc: Jason Cooper <jason@lakedaemon.net>
>     Link: http://lkml.kernel.org/r/1442142873-20213-2-git-send-email-marc
> .zyngier@arm.com
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

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

Re: [PATCH v2 7/7] xen/arm: platform: Drop the quirks callback

[Ian Campbell]

On Tue, 2015-09-29 at 17:55 +0100, Julien Grall wrote:
> On 29/09/15 17:21, Julien Grall wrote:
> > All the quirks has been replaced by proper detection. Lets drop the
> > callback and hope that no one will need new quirks.
> > 
> > At the same time, remove the definition platform_dom0_evtchn_ppi with
> > is
> > not used any more.
> > 
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > Acked-by: Ian Campbell <ian.campebll@citrix.com>
> 
> I made a typo while writing the acked-by. It should campbell and not
> campebll.

I'm terribly offended! Not really...

> I will resend a new version if necessary.

No need.

Re: [PATCH v2 3/7] xen/arm: Warn when a device tree path will be re-used by Xen

[Julien Grall]

On 01/10/15 13:01, Ian Campbell wrote:
> On Tue, 2015-09-29 at 17:21 +0100, Julien Grall wrote:
>> Xen is unconditionally using certain device tree paths to create DOM0
>> specific node (for instance /psci, /memory and /hypervisor).
>>
>> Print a warning message on the console to let the user know if we
>> re-use one of these nodes.
>>
>> Note that the content of most of those is very common and they
>> should have already been skipped via the compatible string or typer
> 
> Stray "r" at the end (fixable on commit)
> 
>> string. This warning is here to catch unusual device-tree and
>> compatible string that we may not yet support in Xen.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Should we consider using /xen-psci or something to avoid this conflict? Or
> (worse IMHO) adding a -N until we find an unused path?

If the /psci node is not removed because it uses a compatible string
which we don't support (i.e arm,psci-1.0 or else), we would end up
having 2 PSCI nodes in the DT. This would be harder to catch the problem
in the DT.

So I think we should stay with the current name.

Regards,

-- 
Julien Grall

Re: [PATCH v2 5/7] xen/arm: gic: Check the size of the CPU and vCPU interface retrieved from DT

[Julien Grall]

On 01/10/15 13:08, Ian Campbell wrote:
> On Tue, 2015-09-29 at 17:21 +0100, Julien Grall wrote:
>> @@ -641,7 +643,30 @@ static int __init gicv2_init(void)
>>          panic("GICv2: Cannot find the maintenance IRQ");
>>      gicv2_info.maintenance_irq = res;
>>  
>> -    /* TODO: Add check on distributor, cpu size */
>> +    /* TODO: Add check on distributor */
>> +
>> +    /*
>> +     * The GICv2 CPU interface should at least be 8KB. Although, most of the DT
>> +     * doesn't correctly set it and use the GICv1 CPU interface size (i.e 4KB).
>> +     * Warn and then fixup.
>> +     */
>> +    if ( csize < SZ_8K )
>> +    {
>> +        printk(XENLOG_WARNING "GICv2: WARNING: "
>> +               "The CPU interface size is wrong: %#"PRIx64
>> +               " expected %#x\n",
> 
> You missed fixing a split string constant here.

Hmmm right. Although, I think the sentence is small so I don't need to
rewrite it.

>> +               csize, SZ_8K);
>> +        csize = SZ_8K;
>> +    }
>> +
>> +    /*
>> +     * Check if the CPU interface and virtual CPU interface have the
>> +     * same size.
>> +     */
>> +    if ( csize != vsize )
>> +        printk(XENLOG_WARNING "GICv2: WARNING: "
>> +               "The size of the CPU interface (%#"PRIpaddr") and the vCPU interface (%#"PRIpaddr") don't match\n",
> 
> Apart from the wrapping this is also just quite a long line in its own
> right (100+ characters with the prefix on the preceeding line) e.g. for
> reading a serial log.
> 
> How about s/the CPU interface/GICC/ and s/the vCPU interface/GICV/ ?
> 
> And maybe s/The size/Sizes/?

The two suggestions are fine for me.

There is a lot small things to fix in this patch series. Shall I resend it?

>>  
>> +    /*
>> +     * Only allow support of GICv2 compatible when the CPU interface
>> +     * and virtual CPU interface are 8KB
>> +     * XXX: Handle other size?
>> +     */
>> +    if ( csize != SZ_8K && vsize != SZ_8K )
>> +    {
>> +        printk(XENLOG_WARNING
>> +               "GICv3: WARNING: Not enabling support of GICv2 compat mode.\n"
> 
> s/of/for/
> 
>> +               "The size of the CPU interface (%#"PRIpaddr") and the vCPU interface (%#"PRIpaddr") should both be 8KB.\n",
> 
> Similarly here. Or just "GICC ("%#...") and GICV ("%#...") must both be 8KB"?

I'm fine with that.

Regards,

-- 
Julien Grall

Re: [PATCH v2 5/7] xen/arm: gic: Check the size of the CPU and vCPU interface retrieved from DT

[Ian Campbell]

On Thu, 2015-10-01 at 13:43 +0100, Julien Grall wrote:
> On 01/10/15 13:08, Ian Campbell wrote:
> > On Tue, 2015-09-29 at 17:21 +0100, Julien Grall wrote:
> > > @@ -641,7 +643,30 @@ static int __init gicv2_init(void)
> > >          panic("GICv2: Cannot find the maintenance IRQ");
> > >      gicv2_info.maintenance_irq = res;
> > >  
> > > -    /* TODO: Add check on distributor, cpu size */
> > > +    /* TODO: Add check on distributor */
> > > +
> > > +    /*
> > > +     * The GICv2 CPU interface should at least be 8KB. Although,
> > > most of the DT
> > > +     * doesn't correctly set it and use the GICv1 CPU interface size
> > > (i.e 4KB).
> > > +     * Warn and then fixup.
> > > +     */
> > > +    if ( csize < SZ_8K )
> > > +    {
> > > +        printk(XENLOG_WARNING "GICv2: WARNING: "
> > > +               "The CPU interface size is wrong: %#"PRIx64
> > > +               " expected %#x\n",
> > 
> > You missed fixing a split string constant here.
> 
> Hmmm right. Although, I think the sentence is small so I don't need to
> rewrite it.

Correct. (If it weren't for the other comments I'd probably have just
unwrapped it when I committed)

> There is a lot small things to fix in this patch series. Shall I resend
> it?

Yes, please. I'm in the process of applying #1..#4 so you might want to
wait until they show up in staging and then rebase.

Re: [PATCH v2 0/7] xen/arm: gic-v2: Detect automatically aliased GIC 400

[Ian Campbell]

On Tue, 2015-09-29 at 17:21 +0100, Julien Grall wrote:
> Hi all,
> 
> Only patch #7 is related to the subject of the cover letter. The rest is
> clean up of code I looked while I was working on this patch series.
> 
> For all the changes, see in each patch.
> 
> Sincerely yours,
> 
> Julien Grall (7):
>   xen/arm: gic: Make it clear the GIC node is passed to
>     make_hwdom_dt_node
>   xen/arm: Retrieve the correct number of cells when building dom0 DT
>   xen/arm: Warn when a device tree path will be re-used by Xen
>   xen/arm: vgic-v2: Drop cbase from arch_domain

Applied up to here.

There were comments on the rest.

>   xen/arm: gic: Check the size of the CPU and vCPU interface retrieved
>     from DT
>   xen/arm: gic-v2: Automatically detect aliased GIC400
>   xen/arm: platform: Drop the quirks callback
> 
>  xen/arch/arm/domain_build.c          | 26 ++++++++--
>  xen/arch/arm/gic-hip04.c             | 17 +++----
>  xen/arch/arm/gic-v2.c                | 97 +++++++++++++++++++++++++++---
> ------
>  xen/arch/arm/gic-v3.c                | 32 ++++++++----
>  xen/arch/arm/gic.c                   |  6 ++-
>  xen/arch/arm/platform.c              | 10 ----
>  xen/arch/arm/platforms/xgene-storm.c |  6 ---
>  xen/arch/arm/vgic-v2.c               | 51 ++++++++++++-------
>  xen/common/device_tree.c             | 39 +++++++++++++--
>  xen/include/asm-arm/domain.h         |  1 -
>  xen/include/asm-arm/gic.h            |  6 ++-
>  xen/include/asm-arm/platform.h       | 14 ------
>  xen/include/asm-arm/vgic.h           |  3 +-
>  xen/include/xen/device_tree.h        | 33 ++++++++++++
>  14 files changed, 234 insertions(+), 107 deletions(-)
>