[PATCH 0/8] xen/arm: gic-v2: Detect automatically aliased GIC400

[PATCH 0/8] xen/arm: gic-v2: Detect automatically aliased GIC400

[Julien Grall]

Hi all,

Only patch #7 is related to subject. The others are clean up of code I looked
while I was working on this series.

Regards,

Julien Grall (8):
  xen/arm: gic: Make 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: Fix comment coding style in handle_node in domain_build.c
  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: Detect automatically aliased GIC400
  xen/arm: platform: Drop the quirks callback

 xen/arch/arm/domain_build.c          | 30 ++++++++---
 xen/arch/arm/gic-hip04.c             | 18 +++----
 xen/arch/arm/gic-v2.c                | 98 +++++++++++++++++++++++++++---------
 xen/arch/arm/gic-v3.c                | 34 +++++++++----
 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             | 38 +++++++++++---
 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        | 23 ++++++++-
 14 files changed, 225 insertions(+), 113 deletions(-)

-- 
2.1.4

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

[Julien Grall]

The callback make_hwdom_dt_node already have 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".

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 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 d1db1ce..0df4baf 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 2/8] xen/arm: Retrieve the correct number of cells when building dom0 DT

[Julien Grall]

The function dt_n_*_cells will retrieve the number of cells for a given
node. Those numbers may not be correct to use for the child "reg"
property if the parent 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 a child of a given parent.

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
recreated 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 there
future parent.

We also have to modify dt_set_range to take a parent in parameter rather
than an HW DT node that we will replicated in the DOM0 DT.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/domain_build.c   |  6 +++---
 xen/arch/arm/gic-v3.c         |  4 ++--
 xen/common/device_tree.c      | 38 +++++++++++++++++++++++++++++++-------
 xen/include/xen/device_tree.h | 23 +++++++++++++++++++++--
 4 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a059de6..f7ea240 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;
@@ -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");
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0df4baf..957e491 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1096,10 +1096,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
 
     tmp = new_cells;
 
-    dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
+    dt_set_range(&tmp, gic->parent, d->arch.vgic.dbase, SZ_64K);
 
     for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
-        dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
+        dt_set_range(&tmp, gic->parent, 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/common/device_tree.c b/xen/common/device_tree.c
index 18cdb6f..845cc77 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -110,11 +110,11 @@ void dt_set_cell(__be32 **cellp, int size, u64 val)
     (*cellp) += cells;
 }
 
-void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
+void dt_set_range(__be32 **cellp, const struct dt_device_node *parent,
                   u64 address, u64 size)
 {
-    dt_set_cell(cellp, dt_n_addr_cells(np), address);
-    dt_set_cell(cellp, dt_n_size_cells(np), 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,
@@ -386,13 +386,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 +403,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 +420,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..8618ca7 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
@@ -644,14 +663,14 @@ void dt_set_cell(__be32 **cellp, int size, u64 val);
  * dt_set_range - Write range into a series of cells
  *
  * @cellp: Pointer to cells
- * @np: Node which contains the encoding for the address and the size
+ * @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_set_range(__be32 **cellp, const struct dt_device_node *np,
+void dt_set_range(__be32 **cellp, const struct dt_device_node *parent,
                   u64 address, u64 size);
 
 /**
-- 
2.1.4

[PATCH 3/8] xen/arm: Fix comment coding style in handle_node in domain_build.c

[Julien Grall]

Only coding style changes. No functional changes.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/domain_build.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f7ea240..651d75e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1226,8 +1226,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         return 0;
     }
 
-    /* Replace these nodes with our own. Note that the original may be
-     * used_by DOMID_XEN so this check comes first. */
+    /*
+     * Replace these nodes with our own. Note that the original may be
+     * used_by DOMID_XEN so this check comes first.
+     */
     if ( device_get_class(node) == DEVICE_GIC )
         return make_gic_node(d, kinfo->fdt, node);
     if ( dt_match_node(timer_matches, node) )
@@ -1240,7 +1242,8 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         return 0;
     }
 
-    /* Even if the IOMMU device is not used by Xen, it should not be
+    /*
+     * Even if the IOMMU device is not used by Xen, it should not be
      * passthrough to DOM0
      */
     if ( device_get_class(node) == DEVICE_IOMMU )
-- 
2.1.4

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

[Julien Grall]

Xen is using unconditionnally some device tree path to create DOM0
specific node (for instance /psci, /memory and /hypervisor).

Rather than blindly add new nodes with the same, print a warning message
on the console to let know the user that something may go wrong.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/domain_build.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 651d75e..2670431 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,14 @@ 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\n",
+               path);
+
     res = handle_device(d, node);
     if ( res)
         return res;
-- 
2.1.4

[PATCH 5/8] 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>
---
 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 6/8] 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 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 Virtual CPU interface been at least
8KB. As in reality the Virtual CPU interface match 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.
---
 xen/arch/arm/gic-v2.c | 34 ++++++++++++++++++++++++++++++----
 xen/arch/arm/gic-v3.c | 22 +++++++++++++++++++---
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 5841e59..62583e7 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,31 @@ 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 != vbase )
+        printk(XENLOG_WARNING "GICv2: WARNING: "
+               "The size of the CPU interface (%#"PRIpaddr") and the vCPU"
+               " interface (%#"PRIpaddr") doesn'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 957e491..4c58baf 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1143,22 +1143,38 @@ 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: Don't enable support of GICv2.\n"
+               "The size of the CPU interface (%#"PRIpaddr") and the vCPU"
+               " interface (%#"PRIpaddr") should be 8KB.\n",
+               csize, vsize);
+        return;
+    }
+
     printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
            cbase, vbase);
 
-- 
2.1.4

[PATCH 7/8] xen/arm: gic-v2: Detect automatically 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 where 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>
---
 xen/arch/arm/gic-hip04.c             | 13 +++-----
 xen/arch/arm/gic-v2.c                | 59 ++++++++++++++++++++++++++----------
 xen/arch/arm/gic-v3.c                |  3 +-
 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, 83 insertions(+), 56 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index e8cdcd4..ac2912a 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,8 @@ 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);
+    /* XXX: Support aliased HIP04 GIC? */
+    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 62583e7..b60bd9b 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);
@@ -686,21 +701,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 has 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 4c58baf..49b58c5 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1178,7 +1178,8 @@ 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);
+    /* XXX: Support aliased GICv2 on GICv3? */
+    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 41cadb1..bb89efd 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 8/8] 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>
---
 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 1/8] xen/arm: gic: Make clear the GIC node is passed to make_hwdom_dt_node

[Ian Campbell]

On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:

"Make it clear..." in the subject.

> The callback make_hwdom_dt_node already have the gic node in parameter.

"...already has the..." or "...already takes the..."

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

Also given the hunk below I'd recommend adding:
"Add an assert to gic_make_hwdom_dt_node to check that the gic really is
the global dt_interrupt_controller"

> @@ -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);
>  }

With those commit message changes:

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

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

[Ian Campbell]

On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> The function dt_n_*_cells will retrieve the number of cells for a given
> node. Those numbers may not be correct to use for the child "reg"
> property if the parent is passed.

I think a clearer way to express this is that the functions return the
number of cells to use for a reg property in the given node, and so they
will naturally not necessarily return the right number if you give them
some other node (whether that's the parent or some other node entirely).

> 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 a child of a given parent.

"of the immediate child of a given"

maybe?

> 
> 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
> recreated from scratch by Xen and therefore we don't have a

s/re//

> dt_device_node for them. The only thing we have is a pointer to there

"their"

> future parent.
> 
> We also have to modify dt_set_range to take a parent in parameter rather
> than an HW DT node that we will replicated in the DOM0 DT.

"replicate".

But here I'd rather leave dt_set_range alone to remain analogous to
dt_n_*_cells. Can you instead do as you did for dt_n_*_cells please and
create a new dt_set_child_range or dt_child_set_range or whatever?

Re: [PATCH 3/8] xen/arm: Fix comment coding style in handle_node in domain_build.c

[Ian Campbell]

On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> Only coding style changes. No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked + applied since it was so trivial and standalone.

> ---
>  xen/arch/arm/domain_build.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f7ea240..651d75e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1226,8 +1226,10 @@ static int handle_node(struct domain *d, struct
> kernel_info *kinfo,
>          return 0;
>      }
>  
> -    /* Replace these nodes with our own. Note that the original may be
> -     * used_by DOMID_XEN so this check comes first. */
> +    /*
> +     * Replace these nodes with our own. Note that the original may be
> +     * used_by DOMID_XEN so this check comes first.
> +     */
>      if ( device_get_class(node) == DEVICE_GIC )
>          return make_gic_node(d, kinfo->fdt, node);
>      if ( dt_match_node(timer_matches, node) )
> @@ -1240,7 +1242,8 @@ static int handle_node(struct domain *d, struct
> kernel_info *kinfo,
>          return 0;
>      }
>  
> -    /* Even if the IOMMU device is not used by Xen, it should not be
> +    /*
> +     * Even if the IOMMU device is not used by Xen, it should not be
>       * passthrough to DOM0
>       */
>      if ( device_get_class(node) == DEVICE_IOMMU )

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

[Ian Campbell]

On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> Xen is using unconditionnally some device tree path to create DOM0

"unconditionally"

> specific node (for instance /psci, /memory and /hypervisor).
> 
> Rather than blindly add new nodes with the same, print a warning message
> on the console to let know the user that something may go wrong.

So it seems to me that /psci and /memory should be pretty common and that
warning when we deliberately replace them with our own contents seems
wrong.

I think the bit which the commit message misses out is that we already
filter out psci and such by compatible string before this point, so this
warning is only for the case where there is a /psci which is not compatible
"arm,psci" or "arm,psci-0.2" etc, i.e. it is something strange and special.

And that case does indeed seem worth warning about but the commit message
needs to be clearer about the circumstances of the logging.

I wonder if it would be worth printing the compatible strings of the node
we are clobbering? There's a good chance they will be e.g. "arm,psci-0.3"
(with no backwards compat) or something and knowing that might be useful.
But maybe it's too much code to worry about.

BTW, how did you trip over this?

> +    /*
> +     * Xen is using some path for its own purpose. Warn if a node

"paths" and "purposes".

> +     * already exists with the same path.
> +     */
> +    if ( dt_match_node(reserved_matches, node) )
> +        printk(XENLOG_WARNING "WARNING: Path %s is reserved, skip the
> node\n",

Rather than skipping we are actually replacing it with our version. Maybe
say that?

Re: [PATCH 5/8] xen/arm: vgic-v2: Drop cbase from arch_domain

[Ian Campbell]

On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> 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>

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

[Ian Campbell]

On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> 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 the GICv1 size (i.e 4KB) in

                                        ^incorrectly

(to avoid implying they actually have a GICv1)

> their DT. Only warn and update the size to avoid any breakage on these
> platforms.
> 
> Furthermore, Xen is relying on the Virtual CPU interface been at least

"relying on the fact that the..."

> 8KB. As in reality the Virtual CPU interface match the CPU interface,

"matches"

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

Speaking of which, wasn't their going to be a new comaintainer at one point
(mid.gname.org/<554B5C71.3020007@linaro.org>)

> @@ -641,7 +643,31 @@ 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 )

What do you think of checking for exactly SZ_4K (the only actually used
incorrect value) and still being picky about other sizes <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 != vbase )
> +        printk(XENLOG_WARNING "GICv2: WARNING: "
> +               "The size of the CPU interface (%#"PRIpaddr") and the
> vCPU"
> +               " interface (%#"PRIpaddr") doesn't match\n",

"don't match"  or "do not match".

In general we should prefer not to split string literally in the middle of
sentences, so grep still works. I think we can tolerate the long lines in
such cases.

> +               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 957e491..4c58baf 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1143,22 +1143,38 @@ 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?

Any size >= 8KB ought to be ok, no?

We could clamp to 8KB if we were worried about what is in the rest.

> +     */
> +    if ( csize != SZ_8K && vsize != SZ_8K )
> +    {
> +        printk(XENLOG_WARNING
> +               "GICv3: WARNING: Don't enable support of GICv2.\n"

"Not enabling support for GICv2 compat mode"

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

"should both be" ?

> +               csize, vsize);
> +        return;
> +    }
> +
>      printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase
> %#"PRIpaddr"\n",
>             cbase, vbase);
>  

Re: [PATCH 7/8] xen/arm: gic-v2: Detect automatically aliased GIC400

[Ian Campbell]

On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:

Subject: "automatically detect..."

> 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 where actually using

s/where/were/

> @@ -688,7 +684,8 @@ 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);
> +    /* XXX: Support aliased HIP04 GIC? */

I assume it doesn't need this, so lets drop the comment and assume that's
the lack of the quirk on this platform is correct.


> +    gicv2.map_cbase = ioremap_nocache(cbase, csize);
> +    if ( !gicv2.map_cbase )
>          panic("GICv2: Failed to ioremap for GIC CPU interface\n");

s/for/the/ or just s/for //.

>  
> +    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 has we need the original

s/has/as/

>      printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
>             cbase, vbase);
>  
> -    vgic_v2_setup_hw(dbase, cbase, vbase);
> +    /* XXX: Support aliased GICv2 on GICv3? */

Nah.

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

[Ian Campbell]

On Tue, 2015-09-22 at 18:47 +0100, 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.

Hah!

> 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.campbell@citrix.com>

Re: [PATCH 3/8] xen/arm: Fix comment coding style in handle_node in domain_build.c

[Ian Campbell]

On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> Only coding style changes. No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked + applied since it was obvious.

> ---
>  xen/arch/arm/domain_build.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f7ea240..651d75e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1226,8 +1226,10 @@ static int handle_node(struct domain *d, struct
> kernel_info *kinfo,
>          return 0;
>      }
>  
> -    /* Replace these nodes with our own. Note that the original may be
> -     * used_by DOMID_XEN so this check comes first. */
> +    /*
> +     * Replace these nodes with our own. Note that the original may be
> +     * used_by DOMID_XEN so this check comes first.
> +     */
>      if ( device_get_class(node) == DEVICE_GIC )
>          return make_gic_node(d, kinfo->fdt, node);
>      if ( dt_match_node(timer_matches, node) )
> @@ -1240,7 +1242,8 @@ static int handle_node(struct domain *d, struct
> kernel_info *kinfo,
>          return 0;
>      }
>  
> -    /* Even if the IOMMU device is not used by Xen, it should not be
> +    /*
> +     * Even if the IOMMU device is not used by Xen, it should not be
>       * passthrough to DOM0
>       */
>      if ( device_get_class(node) == DEVICE_IOMMU )

Re: [PATCH 1/8] xen/arm: gic: Make clear the GIC node is passed to make_hwdom_dt_node

[Julien Grall]

Hi Ian,

On 25/09/15 16:48, Ian Campbell wrote:
> On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> 
> "Make it clear..." in the subject.
> 
>> The callback make_hwdom_dt_node already have the gic node in parameter.
> 
> "...already has the..." or "...already takes the..."
> 
>> 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_controller".
> 
> Also given the hunk below I'd recommend adding:
> "Add an assert to gic_make_hwdom_dt_node to check that the gic really is
> the global dt_interrupt_controller"

Your suggestions looks good. Although, I replaced gic by GIC because
it's an acronym.

>> @@ -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);
>>  }
> 
> With those commit message changes:
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thank you!


Regards,

-- 
Julien Grall

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

[Julien Grall]

Hi Ian,

On 25/09/15 17:01, Ian Campbell wrote:
> On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
>> The function dt_n_*_cells will retrieve the number of cells for a given
>> node. Those numbers may not be correct to use for the child "reg"
>> property if the parent is passed.
> 
> I think a clearer way to express this is that the functions return the
> number of cells to use for a reg property in the given node, and so they
> will naturally not necessarily return the right number if you give them
> some other node (whether that's the parent or some other node entirely).

I will rework this part into:

"The function dt_n_*_cells return the number of cells to use for a reg
property in a given node. So those numbers won't be correct if the
parent of a give 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 a child of a given parent.
> 
> "of the immediate child of a given"
> 
> maybe?

I'm not sure to see why "immediate" is necessary here. A child is always
immediate, otherwise it would be grandchild or else.

> 
>>
>> 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
>> recreated from scratch by Xen and therefore we don't have a
> 
> s/re//
> 
>> dt_device_node for them. The only thing we have is a pointer to there
> 
> "their"
> 
>> future parent.
>>
>> We also have to modify dt_set_range to take a parent in parameter rather
>> than an HW DT node that we will replicated in the DOM0 DT.
> 
> "replicate".
> 
> But here I'd rather leave dt_set_range alone to remain analogous to
> dt_n_*_cells. Can you instead do as you did for dt_n_*_cells please and
> create a new dt_set_child_range or dt_child_set_range or whatever?

I will introduce dt_child_set_range.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-09-28 at 15:59 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 25/09/15 17:01, Ian Campbell wrote:
> > On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> > > The function dt_n_*_cells will retrieve the number of cells for a
> > > given
> > > node. Those numbers may not be correct to use for the child "reg"
> > > property if the parent is passed.
> > 
> > I think a clearer way to express this is that the functions return the
> > number of cells to use for a reg property in the given node, and so
> > they
> > will naturally not necessarily return the right number if you give them
> > some other node (whether that's the parent or some other node
> > entirely).
> 
> I will rework this part into:
> 
> "The function dt_n_*_cells return the number of cells to use for a reg
> property in a given node. So those numbers won't be correct if the
> parent of a give node is passed."

                  ^n

> 
> > 
> > > 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 a child of a given parent.
> > 
> > "of the immediate child of a given"
> > 
> > maybe?
> 
> I'm not sure to see why "immediate" is necessary here. A child is always
> immediate, otherwise it would be grandchild or else.

Sadly it's not as unambiguous as that in this kind of context, child could
be taken to mean "somewhere below".


Ian.

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

[Julien Grall]

On 28/09/15 16:19, Ian Campbell wrote:
> On Mon, 2015-09-28 at 15:59 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 25/09/15 17:01, Ian Campbell wrote:
>>> On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
>>>> The function dt_n_*_cells will retrieve the number of cells for a
>>>> given
>>>> node. Those numbers may not be correct to use for the child "reg"
>>>> property if the parent is passed.
>>>
>>> I think a clearer way to express this is that the functions return the
>>> number of cells to use for a reg property in the given node, and so
>>> they
>>> will naturally not necessarily return the right number if you give them
>>> some other node (whether that's the parent or some other node
>>> entirely).
>>
>> I will rework this part into:
>>
>> "The function dt_n_*_cells return the number of cells to use for a reg
>> property in a given node. So those numbers won't be correct if the
>> parent of a give node is passed."
> 
>                   ^n
> 
>>
>>>
>>>> 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 a child of a given parent.
>>>
>>> "of the immediate child of a given"
>>>
>>> maybe?
>>
>> I'm not sure to see why "immediate" is necessary here. A child is always
>> immediate, otherwise it would be grandchild or else.
> 
> Sadly it's not as unambiguous as that in this kind of context, child could
> be taken to mean "somewhere below".

I will add the "immediate" to remove the ambiguity.

Regards,

-- 
Julien Grall

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

[Julien Grall]

Hi Ian,

On 25/09/15 17:10, Ian Campbell wrote:
> On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
>> Xen is using unconditionnally some device tree path to create DOM0
> 
> "unconditionally"
> 
>> specific node (for instance /psci, /memory and /hypervisor).
>>
>> Rather than blindly add new nodes with the same, print a warning message
>> on the console to let know the user that something may go wrong.
> 
> So it seems to me that /psci and /memory should be pretty common and that
> warning when we deliberately replace them with our own contents seems
> wrong.
>
> I think the bit which the commit message misses out is that we already
> filter out psci and such by compatible string before this point, so this
> warning is only for the case where there is a /psci which is not compatible
> "arm,psci" or "arm,psci-0.2" etc, i.e. it is something strange and special.

Correct. And it's the same for the node /memory. We are removing the
node only when the type is "memory".


> And that case does indeed seem worth warning about but the commit message
> needs to be clearer about the circumstances of the logging.

What about:

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

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

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

> I wonder if it would be worth printing the compatible strings of the node
> we are clobbering? There's a good chance they will be e.g. "arm,psci-0.3"
> (with no backwards compat) or something and knowing that might be useful.
> But maybe it's too much code to worry about.
> 
> BTW, how did you trip over this?

No specific issue. I wrote this patch because I was thinking to move the
interrupt-controller node in /. Although, I gave up on this idea and I
though this patch would still be useful to catch early buggy DT.

> 
>> +    /*
>> +     * Xen is using some path for its own purpose. Warn if a node
> 
> "paths" and "purposes".
> 
>> +     * already exists with the same path.
>> +     */
>> +    if ( dt_match_node(reserved_matches, node) )
>> +        printk(XENLOG_WARNING "WARNING: Path %s is reserved, skip the
>> node\n",
> 
> Rather than skipping we are actually replacing it with our version. Maybe
> say that?

I would rather keep "skipping" because we may decide to not use this
path based on DOM0 configuration.

Though I can rework the message with:

"WARNING: Patch %s is reserved, skip the node as we may re-use the path.\n"

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-09-28 at 16:44 +0100, Julien Grall wrote:
> > And that case does indeed seem worth warning about but the commit
> > message
> > needs to be clearer about the circumstances of the logging.
> 
> What about:
> 
> "Xen is using unconditionally some device tree path to create DOM0

"unconditionally using" and "paths", and probably s/some/certain/.


> specific node (for instance /psci, /memory and /hypervisor).
> 
> Print a warning message on the console to let know user the user if we

"to let the user know"

> re-use one of this node.

"these nodes"

> Note that the content of most those nodes are very common and they

"most of those is"

> should have already been skipped via the compatible string or type
> string. This warning is here to catch buggy device-tree and compatible
> string that we may not yet support in Xen."

I don't think they are "buggy", perhaps more like "unusual".

Ian

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

[Julien Grall]

On 28/09/15 16:55, Ian Campbell wrote:
> On Mon, 2015-09-28 at 16:44 +0100, Julien Grall wrote:
>>> And that case does indeed seem worth warning about but the commit
>>> message
>>> needs to be clearer about the circumstances of the logging.
>>
>> What about:
>>
>> "Xen is using unconditionally some device tree path to create DOM0
> 
> "unconditionally using" and "paths", and probably s/some/certain/.
> 
> 
>> specific node (for instance /psci, /memory and /hypervisor).
>>
>> Print a warning message on the console to let know user the user if we
> 
> "to let the user know"
> 
>> re-use one of this node.
> 
> "these nodes"
> 
>> Note that the content of most those nodes are very common and they
> 
> "most of those is"
> 
>> should have already been skipped via the compatible string or type
>> string. This warning is here to catch buggy device-tree and compatible
>> string that we may not yet support in Xen."
> 
> I don't think they are "buggy", perhaps more like "unusual".

I will use "unusual".

Regards,

-- 
Julien Grall

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

[Julien Grall]

Hi Ian,

On 25/09/15 17:19, Ian Campbell wrote:
> On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
>> 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 the GICv1 size (i.e 4KB) in
> 
>                                         ^incorrectly
> 
> (to avoid implying they actually have a GICv1)
> 
>> their DT. Only warn and update the size to avoid any breakage on these
>> platforms.
>>
>> Furthermore, Xen is relying on the Virtual CPU interface been at least
> 
> "relying on the fact that the..."
> 
>> 8KB. As in reality the Virtual CPU interface match the CPU interface,
> 
> "matches"
> 
>> 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.
> 
> Speaking of which, wasn't their going to be a new comaintainer at one point
> (mid.gname.org/<554B5C71.3020007@linaro.org>)

I think so. I've CCed Shameerali who seems to be more responsive and
tested my previous changes about GIC-hip04.

> 
>> @@ -641,7 +643,31 @@ 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 )
> 
> What do you think of checking for exactly SZ_4K (the only actually used
> incorrect value) and still being picky about other sizes <8K?

If so, we should have to be picky for any value other than SZ_4K, SZ_8K,
SZ_128K.

Although I didn't do it because I wasn't not sure if there is DT out
with other possible value.

I've added this check because any size below 8K would result to a
non-obvious crash when Xen is trying to access GIC_DIR.

> 
>> +    {
>> +        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 != vbase )
>> +        printk(XENLOG_WARNING "GICv2: WARNING: "
>> +               "The size of the CPU interface (%#"PRIpaddr") and the
>> vCPU"
>> +               " interface (%#"PRIpaddr") doesn't match\n",
> 
> "don't match"  or "do not match".
> 
> In general we should prefer not to split string literally in the middle of
> sentences, so grep still works. I think we can tolerate the long lines in
> such cases.

Good point. I will put the message one a single line.

>> +               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 957e491..4c58baf 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1143,22 +1143,38 @@ 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?
> 
> Any size >= 8KB ought to be ok, no?
> We could clamp to 8KB if we were worried about what is in the rest.

The GICv2 CPU interface is always 8KB. Having an higher value may mean
that the GIC is aliased.

GICv2 on GICv3 is only used for guest. I prefer to restrict the usage to
known and safe value until we have someone using different size.

This will avoid to expose unwanted data/value to a guest.

> 
>> +     */
>> +    if ( csize != SZ_8K && vsize != SZ_8K )
>> +    {
>> +        printk(XENLOG_WARNING
>> +               "GICv3: WARNING: Don't enable support of GICv2.\n"
> 
> "Not enabling support for GICv2 compat mode"

Will fix it.

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

Yes.

Regards,

-- 
Julien Grall

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

[Julien Grall]

On 25/09/15 17:10, Ian Campbell wrote:
> I wonder if it would be worth printing the compatible strings of the node
> we are clobbering? There's a good chance they will be e.g. "arm,psci-0.3"
> (with no backwards compat) or something and knowing that might be useful.
> But maybe it's too much code to worry about.

I forgot to answer to this bit. Printing the first compatible node will
be very easy, the others will require a bit more work (maybe we could
re-use a bit of dt_device_is_compatible?).

I think a generic function to dump DT node in Xen would be very useful.
I can give a look when I have time as a follow-up.

Regards,


-- 
Julien Grall

Re: [PATCH 7/8] xen/arm: gic-v2: Detect automatically aliased GIC400

[Julien Grall]

On 25/09/15 17:26, Ian Campbell wrote:
> On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
>> +    gicv2.map_cbase = ioremap_nocache(cbase, csize);
>> +    if ( !gicv2.map_cbase )
>>          panic("GICv2: Failed to ioremap for GIC CPU interface\n");
> 
> s/for/the/ or just s/for //.

That's not part of my changes and the error is the same on every error
message after ioremap within the GIC* drivers.

I won't change this one here, but if you really want I can send a
follow-up to fix it.


Regards,

-- 
Julien Grall

Re: [PATCH 7/8] xen/arm: gic-v2: Detect automatically aliased GIC400

[Ian Campbell]

On Mon, 2015-09-28 at 19:07 +0100, Julien Grall wrote:
> On 25/09/15 17:26, Ian Campbell wrote:
> > On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> > > +    gicv2.map_cbase = ioremap_nocache(cbase, csize);
> > > +    if ( !gicv2.map_cbase )
> > >          panic("GICv2: Failed to ioremap for GIC CPU interface\n");
> > 
> > s/for/the/ or just s/for //.
> 
> That's not part of my changes and the error is the same on every error
> message after ioremap within the GIC* drivers.

Oh sorry, my eye must have skipped a line as I scanned from the +

> I won't change this one here, but if you really want I can send a
> follow-up to fix it.

No pressing need.