[PATCH v3] xen/arm: Add support for Huawei hip04-d01 platform

[PATCH v3] xen/arm: Add support for Huawei hip04-d01 platform

[Frediano Ziglio]

This set of patches add Xen support for hip04-d01 platform (see https://wiki.linaro.org/Boards/D01 for details).

Changes from v2:
- rewrote DTS fix patch (Ian Campbell);
- use is_hip04 macro instead of doing explicit test (Julien Grall);
- do not use quirks to distinguish this platform (Ian Cambell);
- move some GIC constants to C files instead of header (Julien Grall);
- minor changes (Julien Grall).

Changes from v1:
- style (Julien Grall);
- make gicv2_send_SGI faster (Julien Grall);
- cleanup correctly if hip04_smp_init fails (Julien Grall);
- remove quirks using compatibility (Ian Campbell);
- other minor suggestions by Julien Grall.

[PATCH v3 1/8] xen/device_tree: Add new helper to read arrays from a DTB

[Frediano Ziglio]

From: Zoltan Kiss <zoltan.kiss@linaro.org>

Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/device_tree.c      | 13 ++++++++++---
 xen/include/xen/device_tree.h | 11 +++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index f72b2e9..1a886c0 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -160,14 +160,21 @@ const void *dt_get_property(const struct dt_device_node *np,
 bool_t dt_property_read_u32(const struct dt_device_node *np,
                          const char *name, u32 *out_value)
 {
-    u32 len;
+    return dt_property_read_u32_array(np, name, out_value, 1);
+}
+
+bool_t dt_property_read_u32_array(const struct dt_device_node *np,
+                                  const char *name, u32 *out_value, u16 out_len)
+{
+    u32 len, i;
     const __be32 *val;
 
     val = dt_get_property(np, name, &len);
-    if ( !val || len < sizeof(*out_value) )
+    if ( !val || len < sizeof(*out_value) * out_len )
         return 0;
 
-    *out_value = be32_to_cpup(val);
+    for ( i = 0; i < out_len; i++, val++ )
+        out_value[i] = be32_to_cpup(val);
 
     return 1;
 }
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 08db8bc..629bfb2 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -346,6 +346,17 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np,
 bool_t dt_property_read_u32(const struct dt_device_node *np,
                             const char *name, u32 *out_value);
 /**
+ * dt_property_read_u32_array - Helper to read a u32 array property.
+ * @np: node to get the value
+ * @name: name of the property
+ * @out_value: pointer to return value
+ * @out_len: length of the array
+ *
+ * Return true if get the desired value.
+ */
+bool_t dt_property_read_u32_array(const struct dt_device_node *np,
+                                  const char *name, u32 *out_value, u16 out_len);
+/**
  * dt_property_read_u64 - Helper to read a u64 property.
  * @np: node to get the value
  * @name: name of the property
-- 
1.9.1

[PATCH v3 2/8] xen/arm: Implement hip04-d01 platform

[Frediano Ziglio]

Add this new platform to Xen.
This platform requires specific code to initialize CPUs.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/platforms/Makefile |   1 +
 xen/arch/arm/platforms/hip04.c  | 300 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 301 insertions(+)
 create mode 100644 xen/arch/arm/platforms/hip04.c

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 8f47c16..d0b2d99 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_ARM_32) += midway.o
 obj-$(CONFIG_ARM_32) += omap5.o
 obj-$(CONFIG_ARM_32) += sunxi.o
 obj-$(CONFIG_ARM_64) += seattle.o
+obj-$(CONFIG_ARM_32) += hip04.o
 obj-$(CONFIG_ARM_64) += xgene-storm.o
diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
new file mode 100644
index 0000000..f25282c
--- /dev/null
+++ b/xen/arch/arm/platforms/hip04.c
@@ -0,0 +1,300 @@
+/*
+ * xen/arch/arm/platforms/hip04.c
+ *
+ * HiSilicon HIP-04 D01 board
+ *
+ * Copyright (c) 2012-2013 Hisilicon Ltd.
+ * Copyright (c) 2012-2013 Linaro Ltd.
+ * Copyright (c) 2014 Huawei Tech. Co., Ltd.
+ *
+ * Author: Frediano Ziglio <frediano.ziglio@huawei.com>
+ *
+ * Original code from Linux kernel arch/arm/mach-hisi/hisilicon.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/platform.h>
+#include <xen/mm.h>
+#include <xen/vmap.h>
+#include <asm/io.h>
+#include <asm/gic.h>
+#include <xen/delay.h>
+
+#define CORE_RESET_BIT(x)            (1 << x)
+#define NEON_RESET_BIT(x)            (1 << (x + 4))
+#define CORE_DEBUG_RESET_BIT(x)      (1 << (x + 9))
+#define CLUSTER_L2_RESET_BIT         (1 << 8)
+#define CLUSTER_DEBUG_RESET_BIT      (1 << 13)
+
+#define CLUSTER_L2_RESET_STATUS      (1 << 8)
+#define CLUSTER_DEBUG_RESET_STATUS   (1 << 13)
+
+#define SC_CPU_RESET_DREQ(x)         (0x524 + (x << 3))    /* unreset */
+#define SC_CPU_RESET_STATUS(x)       (0x1520 + (x << 3))
+
+#define FAB_SF_MODE                  0x0c
+
+#define HIP04_MAX_CLUSTERS           4
+#define HIP04_MAX_CPUS_PER_CLUSTER   4
+
+struct hip04_secondary_cpu_data
+{
+    u32 bootwrapper_phys;
+    u32 bootwrapper_size;
+    u32 bootwrapper_magic;
+    u32 relocation_entry;
+    u32 relocation_size;
+};
+
+static void __iomem *relocation, *sysctrl, *fabric, *gb2;
+static int hip04_cpu_table[HIP04_MAX_CLUSTERS][HIP04_MAX_CPUS_PER_CLUSTER];
+static struct hip04_secondary_cpu_data hip04_boot;
+
+static void hip04_reset(void)
+{
+    unsigned long data;
+
+    data = readl_relaxed(gb2);
+    writel_relaxed(data & ~0x4000000u, gb2);
+
+    mdelay(10);
+}
+
+static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on)
+{
+    unsigned long data;
+
+    data = readl_relaxed(fabric + FAB_SF_MODE);
+    if ( on )
+        data |= 1 << cluster;
+    else
+        data &= ~(1 << cluster);
+    writel_relaxed(data, fabric + FAB_SF_MODE);
+    while ( 1 )
+    {
+        if ( data == readl_relaxed(fabric + FAB_SF_MODE) )
+            break;
+    }
+}
+
+static bool __init hip04_cpu_table_init(void)
+{
+    unsigned int mpidr, cpu, cluster;
+
+    mpidr = cpu_logical_map(smp_processor_id());
+    cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+    cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+    if ( cluster >= HIP04_MAX_CLUSTERS ||
+        cpu >= HIP04_MAX_CPUS_PER_CLUSTER )
+    {
+        printk(XENLOG_ERR "%s: boot CPU is out of bound!\n", __func__);
+        return false;
+    }
+
+    hip04_set_snoop_filter(cluster, 1);
+    hip04_cpu_table[cluster][cpu] = 1;
+    return true;
+}
+
+static bool hip04_cluster_down(unsigned int cluster)
+{
+    int i;
+
+    for ( i = 0; i < HIP04_MAX_CPUS_PER_CLUSTER; i++ )
+        if ( hip04_cpu_table[cluster][i] )
+            return false;
+    return true;
+}
+
+static void hip04_cluster_up(unsigned int cluster)
+{
+    unsigned long data, mask;
+
+    if ( !hip04_cluster_down(cluster) )
+        return;
+
+    data = CLUSTER_L2_RESET_BIT | CLUSTER_DEBUG_RESET_BIT;
+    writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+    do {
+        mask = CLUSTER_L2_RESET_STATUS | \
+               CLUSTER_DEBUG_RESET_STATUS;
+        data = readl_relaxed(sysctrl + \
+                     SC_CPU_RESET_STATUS(cluster));
+    } while ( data & mask );
+    hip04_set_snoop_filter(cluster, 1);
+}
+
+static void __init hip04_iounmap(void __iomem **p)
+{
+    iounmap(*p);
+    *p = NULL;
+}
+
+static int __init hip04_smp_init(void)
+{
+    const struct dt_device_node *np, *np_fab, *bw;
+    const char *msg;
+    u64 addr, size;
+
+    np = dt_find_compatible_node(NULL, NULL, "hisilicon,sysctrl");
+    msg = "hisilicon,sysctrl missing in DT\n";
+    if ( !np )
+        goto err;
+
+    np_fab = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
+    msg = "hisilicon,hip04-fabric missing in DT\n";
+    if ( !np_fab )
+        goto err;
+
+    if ( !dt_property_read_u32(np, "bootwrapper-phys",
+                               &hip04_boot.bootwrapper_phys) ) {
+        u32 boot_method[4];
+        bw = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-bootwrapper");
+        msg = "hisilicon,hip04-bootwrapper missing in DT\n";
+        if ( !bw )
+            goto err;
+
+        msg = "failed to get boot-method\n";
+        if ( !dt_property_read_u32_array(bw, "boot-method", boot_method, 4) )
+            goto err;
+        hip04_boot.bootwrapper_phys = boot_method[0];
+        hip04_boot.bootwrapper_size = boot_method[1];
+        hip04_boot.bootwrapper_magic = 0xa5a5a5a5;
+        hip04_boot.relocation_entry = boot_method[2];
+        hip04_boot.relocation_size = boot_method[3];
+    }
+    else
+    {
+        msg = "failed to get bootwrapper-size\n";
+        if ( !dt_property_read_u32(np, "bootwrapper-size",
+                                   &hip04_boot.bootwrapper_size) )
+            goto err;
+
+        msg = "failed to get bootwrapper-magic\n";
+        if ( !dt_property_read_u32(np, "bootwrapper-magic",
+                                   &hip04_boot.bootwrapper_magic) )
+            goto err;
+
+        msg = "failed to get relocation-entry\n";
+        if ( !dt_property_read_u32(np, "relocation-entry",
+                                   &hip04_boot.relocation_entry) )
+            goto err;
+
+        msg = "failed to get relocation-size\n";
+        if ( !dt_property_read_u32(np, "relocation-size",
+                                   &hip04_boot.relocation_size) )
+            goto err;
+    }
+
+    relocation = ioremap_nocache(hip04_boot.relocation_entry,
+                                 hip04_boot.relocation_size);
+    msg = "failed to map relocation space\n";
+    if ( !relocation )
+        goto err;
+
+    msg = "Error in \"hisilicon,sysctrl\"\n";
+    if ( dt_device_get_address(np, 0, &addr, &size) )
+        goto err;
+    sysctrl = ioremap_nocache(addr, size);
+    if ( !sysctrl )
+        goto err;
+
+    msg = "Error in \"hisilicon,hip04-fabric\"\n";
+    if ( dt_device_get_address(np_fab, 0, &addr, &size) )
+        goto err;
+    fabric = ioremap_nocache(addr, size);
+    if ( !fabric )
+        goto err;
+
+    msg = "Error mapping GB2\n";
+    gb2 = ioremap_nocache(0xe4002000, 0x1000);
+    if ( !gb2 )
+        goto err;
+
+    msg = "Error initializing SMP table\n";
+    if ( !hip04_cpu_table_init() )
+        goto err;
+
+    writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+    writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+    writel_relaxed(__pa(init_secondary), relocation + 8);
+    writel_relaxed(0, relocation + 12);
+
+    return 0;
+
+err:
+    hip04_iounmap(&relocation);
+    hip04_iounmap(&sysctrl);
+    hip04_iounmap(&fabric);
+    hip04_iounmap(&gb2);
+
+    printk("%s", msg);
+    return -ENXIO;
+}
+
+static int hip04_cpu_up(int cpu)
+{
+    unsigned int cluster;
+    unsigned long data;
+
+    cluster = cpu / HIP04_MAX_CPUS_PER_CLUSTER;
+    cpu %= HIP04_MAX_CPUS_PER_CLUSTER;
+
+    writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+    writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+    writel_relaxed(__pa(init_secondary), relocation + 8);
+    writel_relaxed(0, relocation + 12);
+
+    hip04_cluster_up(cluster);
+
+    hip04_cpu_table[cluster][cpu]++;
+
+    data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+           CORE_DEBUG_RESET_BIT(cpu);
+    writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+
+    return cpu_up_send_sgi(cpu);
+}
+
+
+static const char * const hip04_dt_compat[] __initconst =
+{
+    "hisilicon,hip04-d01",
+    NULL
+};
+
+static const struct dt_device_match hip04_blacklist_dev[] __initconst =
+{
+    /* Hardware power management */
+    DT_MATCH_COMPATIBLE("hisilicon,sysctrl"),
+    DT_MATCH_COMPATIBLE("hisilicon,hip04-fabric"),
+    { /* sentinel */ },
+};
+
+
+PLATFORM_START(hip04, "HISILICON HIP04")
+    .compatible = hip04_dt_compat,
+    .smp_init = hip04_smp_init,
+    .cpu_up = hip04_cpu_up,
+    .reset = hip04_reset,
+    .blacklist_dev = hip04_blacklist_dev,
+PLATFORM_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.9.1

[PATCH v3 3/8] xen/arm: Make gic-v2 code handle hip04-d01 platform

[Frediano Ziglio]

The GIC in this platform is mainly compatible with the standard
GICv2 beside:
- ITARGET is extended to 16 bit to support 16 CPUs;
- SGI mask is extended to support 16 CPUs;
- maximum supported interrupt is 510.

Use nr_lines to check for maximum irq supported. hip04-d01 support less
interrupts due to some field restriction. Any value above this is already
an error.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/gic-v2.c     | 85 ++++++++++++++++++++++++++++++++++++++---------
 xen/arch/arm/gic.c        |  3 +-
 xen/include/asm-arm/gic.h |  4 ++-
 3 files changed, 75 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index faad1ff..3cb59dd 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -79,16 +79,25 @@ static struct gic_info gicv2_info;
  * logical CPU numbering. Let's use mapping as returned by the GIC
  * itself
  */
-static DEFINE_PER_CPU(u8, gic_cpu_id);
+static DEFINE_PER_CPU(u16, gic_cpu_id);
 
 /* Maximum cpu interface per GIC */
-#define NR_GIC_CPU_IF 8
+static unsigned int nr_gic_cpu_if = 8;
+static unsigned int gicd_sgi_target_shift = GICD_SGI_TARGET_SHIFT;
+static unsigned int gic_cpu_mask = 0xff;
+
+#define is_hip04() (nr_gic_cpu_if == 16)
 
 static inline void writeb_gicd(uint8_t val, unsigned int offset)
 {
     writeb_relaxed(val, gicv2.map_dbase + offset);
 }
 
+static inline void writew_gicd(uint16_t val, unsigned int offset)
+{
+    writew_relaxed(val, gicv2.map_dbase + offset);
+}
+
 static inline void writel_gicd(uint32_t val, unsigned int offset)
 {
     writel_relaxed(val, gicv2.map_dbase + offset);
@@ -132,7 +141,7 @@ static unsigned int gicv2_cpu_mask(const cpumask_t *cpumask)
     cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
     for_each_cpu( cpu, &possible_mask )
     {
-        ASSERT(cpu < NR_GIC_CPU_IF);
+        ASSERT(cpu < nr_gic_cpu_if);
         mask |= per_cpu(gic_cpu_id, cpu);
     }
 
@@ -203,6 +212,15 @@ static unsigned int gicv2_read_irq(void)
     return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
 }
 
+/* Set target CPU mask (RAZ/WI on uniprocessor) */
+static void gicv2_set_irq_mask(int irq, unsigned int mask)
+{
+    if ( is_hip04() )
+        writew_gicd(mask, GICD_ITARGETSR + irq * 2);
+    else
+        writeb_gicd(mask, GICD_ITARGETSR + irq);
+}
+
 /*
  * needs to be called with a valid cpu_mask, ie each cpu in the mask has
  * already called gic_cpu_init
@@ -230,7 +248,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
     writel_gicd(cfg, GICD_ICFGR + (irq / 16) * 4);
 
     /* Set target CPU mask (RAZ/WI on uniprocessor) */
-    writeb_gicd(mask, GICD_ITARGETSR + irq);
+    gicv2_set_irq_mask(irq, mask);
     /* Set priority */
     writeb_gicd(priority, GICD_IPRIORITYR + irq);
 
@@ -244,16 +262,21 @@ static void __init gicv2_dist_init(void)
     uint32_t gic_cpus;
     int i;
 
-    cpumask = readl_gicd(GICD_ITARGETSR) & 0xff;
-    cpumask |= cpumask << 8;
-    cpumask |= cpumask << 16;
+    cpumask = readl_gicd(GICD_ITARGETSR) & gic_cpu_mask;
 
     /* Disable the distributor */
     writel_gicd(0, GICD_CTLR);
 
     type = readl_gicd(GICD_TYPER);
     gicv2_info.nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
-    gic_cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5);
+    if ( is_hip04() )
+    {
+        gic_cpus = 16;
+    }
+    else
+    {
+        gic_cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5);
+    }
     printk("GICv2: %d lines, %d cpu%s%s (IID %8.8x).\n",
            gicv2_info.nr_lines, gic_cpus, (gic_cpus == 1) ? "" : "s",
            (type & GICD_TYPE_SEC) ? ", secure" : "",
@@ -264,8 +287,19 @@ static void __init gicv2_dist_init(void)
         writel_gicd(0x0, GICD_ICFGR + (i / 16) * 4);
 
     /* Route all global IRQs to this CPU */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
-        writel_gicd(cpumask, GICD_ITARGETSR + (i / 4) * 4);
+    if ( is_hip04() )
+    {
+        cpumask |= cpumask << 16;
+        for ( i = 32; i < gicv2_info.nr_lines; i += 2 )
+            writel_gicd(cpumask, GICD_ITARGETSR + (i / 2) * 4);
+    }
+    else
+    {
+        cpumask |= cpumask << 8;
+        cpumask |= cpumask << 16;
+        for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
+            writel_gicd(cpumask, GICD_ITARGETSR + (i / 4) * 4);
+    }
 
     /* Default priority for global interrupts */
     for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
@@ -285,7 +319,7 @@ static void __cpuinit gicv2_cpu_init(void)
 {
     int i;
 
-    this_cpu(gic_cpu_id) = readl_gicd(GICD_ITARGETSR) & 0xff;
+    this_cpu(gic_cpu_id) = readl_gicd(GICD_ITARGETSR) & gic_cpu_mask;
 
     /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so
      * even though they are controlled with GICD registers, they must
@@ -366,7 +400,7 @@ static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
         cpumask_and(&online_mask, cpu_mask, &cpu_online_map);
         mask = gicv2_cpu_mask(&online_mask);
         writel_gicd(GICD_SGI_TARGET_LIST |
-                    (mask << GICD_SGI_TARGET_SHIFT) | sgi,
+                    (mask << gicd_sgi_target_shift) | sgi,
                     GICD_SGIR);
         break;
     default:
@@ -581,7 +615,7 @@ static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_m
     mask = gicv2_cpu_mask(cpu_mask);
 
     /* Set target CPU mask (RAZ/WI on uniprocessor) */
-    writeb_gicd(mask, GICD_ITARGETSR + desc->irq);
+    gicv2_set_irq_mask(desc->irq, mask);
 
     spin_unlock(&gicv2.lock);
 }
@@ -684,7 +718,7 @@ const static struct gic_hw_operations gicv2_ops = {
 };
 
 /* Set up the GIC */
-static int __init gicv2_init(struct dt_device_node *node, const void *data)
+static int __init gicv2_init_default(struct dt_device_node *node, const void *data)
 {
     int res;
 
@@ -764,6 +798,15 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
     return 0;
 }
 
+static int __init hip04_gicv2_init(struct dt_device_node *node, const void *data)
+{
+    nr_gic_cpu_if = 16;
+    gicd_sgi_target_shift = 8;
+    gic_cpu_mask = 0xffff;
+
+    return gicv2_init_default(node, data);
+}
+
 static const char * const gicv2_dt_compat[] __initconst =
 {
     DT_COMPAT_GIC_CORTEX_A15,
@@ -774,9 +817,21 @@ static const char * const gicv2_dt_compat[] __initconst =
 
 DT_DEVICE_START(gicv2, "GICv2:", DEVICE_GIC)
         .compatible = gicv2_dt_compat,
-        .init = gicv2_init,
+        .init = gicv2_init_default,
 DT_DEVICE_END
 
+static const char * const hip04_gicv2_dt_compat[] __initconst =
+{
+    DT_COMPAT_GIC_HIP04,
+    NULL
+};
+
+DT_DEVICE_START(hip04_gicv2, "GICv2:", DEVICE_GIC)
+        .compatible = hip04_gicv2_dt_compat,
+        .init = hip04_gicv2_init,
+DT_DEVICE_END
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 70d10d6..8050a65 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -563,12 +563,13 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
 void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
 {
     unsigned int irq;
+    unsigned int max_irq = gic_hw_ops->info->nr_lines;
 
     do  {
         /* Reading IRQ will ACK it */
         irq = gic_hw_ops->read_irq();
 
-        if ( likely(irq >= 16 && irq < 1021) )
+        if ( likely(irq >= 16 && irq < max_irq) )
         {
             local_irq_enable();
             do_IRQ(regs, irq, is_fiq);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 187dc46..5adb628 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -155,10 +155,12 @@
 #define DT_COMPAT_GIC_400            "arm,gic-400"
 #define DT_COMPAT_GIC_CORTEX_A15     "arm,cortex-a15-gic"
 #define DT_COMPAT_GIC_CORTEX_A7      "arm,cortex-a7-gic"
+#define DT_COMPAT_GIC_HIP04          "hisilicon,hip04-gic"
 
 #define DT_MATCH_GIC_V2 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A15), \
                         DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A7), \
-                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400)
+                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400), \
+                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04)
 
 #define DT_COMPAT_GIC_V3             "arm,gic-v3"
 
-- 
1.9.1

[PATCH v3 4/8] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Frediano Ziglio]

This name can appear in some Linux kernel repos. Not very fortunate,
but to avoid others spending an hour to spot that few characters
difference it worth to work around it.

Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/gic-v2.c     | 1 +
 xen/include/asm-arm/gic.h | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 3cb59dd..9ab30ce 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -823,6 +823,7 @@ DT_DEVICE_END
 static const char * const hip04_gicv2_dt_compat[] __initconst =
 {
     DT_COMPAT_GIC_HIP04,
+    DT_COMPAT_GIC_HIP04_2,
     NULL
 };
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 5adb628..3d2b3db 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -156,11 +156,13 @@
 #define DT_COMPAT_GIC_CORTEX_A15     "arm,cortex-a15-gic"
 #define DT_COMPAT_GIC_CORTEX_A7      "arm,cortex-a7-gic"
 #define DT_COMPAT_GIC_HIP04          "hisilicon,hip04-gic"
+#define DT_COMPAT_GIC_HIP04_2        "hisilicon,hip04-intc"
 
 #define DT_MATCH_GIC_V2 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A15), \
                         DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A7), \
                         DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400), \
-                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04)
+                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04), \
+                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04_2)
 
 #define DT_COMPAT_GIC_V3             "arm,gic-v3"
 
-- 
1.9.1

Re: [PATCH v3 4/8] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Julien Grall]

Hi Frediano,

On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
> This name can appear in some Linux kernel repos. Not very fortunate,
> but to avoid others spending an hour to spot that few characters
> difference it worth to work around it.

Linux upstream is using "hisilicon,hip04-intc" to detect the hisilicon
interrupt controller. So it's not a workaround.

Which kernel is using the "*,hip04-gic"?

Regards,

> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  xen/arch/arm/gic-v2.c     | 1 +
>  xen/include/asm-arm/gic.h | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 3cb59dd..9ab30ce 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -823,6 +823,7 @@ DT_DEVICE_END
>  static const char * const hip04_gicv2_dt_compat[] __initconst =
>  {
>      DT_COMPAT_GIC_HIP04,
> +    DT_COMPAT_GIC_HIP04_2,
>      NULL
>  };
>  
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 5adb628..3d2b3db 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -156,11 +156,13 @@
>  #define DT_COMPAT_GIC_CORTEX_A15     "arm,cortex-a15-gic"
>  #define DT_COMPAT_GIC_CORTEX_A7      "arm,cortex-a7-gic"
>  #define DT_COMPAT_GIC_HIP04          "hisilicon,hip04-gic"
> +#define DT_COMPAT_GIC_HIP04_2        "hisilicon,hip04-intc"
>  
>  #define DT_MATCH_GIC_V2 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A15), \
>                          DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A7), \
>                          DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400), \
> -                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04)
> +                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04), \
> +                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04_2)
>  
>  #define DT_COMPAT_GIC_V3             "arm,gic-v3"
>  
> 


-- 
Julien Grall

Re: [PATCH v3 4/8] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Stefano Stabellini]

On Wed, 5 Nov 2014, Julien Grall wrote:
> Hi Frediano,
> 
> On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
> > This name can appear in some Linux kernel repos. Not very fortunate,
> > but to avoid others spending an hour to spot that few characters
> > difference it worth to work around it.
> 
> Linux upstream is using "hisilicon,hip04-intc" to detect the hisilicon
> interrupt controller. So it's not a workaround.
> 
> Which kernel is using the "*,hip04-gic"?

Good question, but what really matters is the string that u-boot (or any
other firmware/bootloader) is going to use, right? So, which one is it?



> > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> > ---
> >  xen/arch/arm/gic-v2.c     | 1 +
> >  xen/include/asm-arm/gic.h | 4 +++-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 3cb59dd..9ab30ce 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -823,6 +823,7 @@ DT_DEVICE_END
> >  static const char * const hip04_gicv2_dt_compat[] __initconst =
> >  {
> >      DT_COMPAT_GIC_HIP04,
> > +    DT_COMPAT_GIC_HIP04_2,
> >      NULL
> >  };
> >  
> > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> > index 5adb628..3d2b3db 100644
> > --- a/xen/include/asm-arm/gic.h
> > +++ b/xen/include/asm-arm/gic.h
> > @@ -156,11 +156,13 @@
> >  #define DT_COMPAT_GIC_CORTEX_A15     "arm,cortex-a15-gic"
> >  #define DT_COMPAT_GIC_CORTEX_A7      "arm,cortex-a7-gic"
> >  #define DT_COMPAT_GIC_HIP04          "hisilicon,hip04-gic"
> > +#define DT_COMPAT_GIC_HIP04_2        "hisilicon,hip04-intc"
> >  
> >  #define DT_MATCH_GIC_V2 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A15), \
> >                          DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A7), \
> >                          DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400), \
> > -                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04)
> > +                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04), \
> > +                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04_2)
> >  
> >  #define DT_COMPAT_GIC_V3             "arm,gic-v3"
> >  
> > 
> 
> 
> -- 
> Julien Grall
> 

Re: [PATCH v3 4/8] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Zoltan Kiss]

On 05/11/14 14:52, Stefano Stabellini wrote:
> On Wed, 5 Nov 2014, Julien Grall wrote:
>> Hi Frediano,
>>
>> On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
>>> This name can appear in some Linux kernel repos. Not very fortunate,
>>> but to avoid others spending an hour to spot that few characters
>>> difference it worth to work around it.
>>
>> Linux upstream is using "hisilicon,hip04-intc" to detect the hisilicon
>> interrupt controller. So it's not a workaround.
>>
>> Which kernel is using the "*,hip04-gic"?
>
> Good question, but what really matters is the string that u-boot (or any
> other firmware/bootloader) is going to use, right? So, which one is it?
We are using the DTB from the kernel source, even when loading a bare 
metal kernel. I've looked around, the *gic version seems to exist only 
in internal repos, as far as I can see. Including the one Frediano 
started to use for porting. Therefore, I don't insist to keep both, but 
as I mentioned in the commit message, it would still provide some 
benefit, and given that it's just a 3 line change which just extend a 
few listings, I think we should keep it.
Of course with a different commit message, which clears that this is the 
official name of it.

Zoli

>
>
>
>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
>>> ---
>>>   xen/arch/arm/gic-v2.c     | 1 +
>>>   xen/include/asm-arm/gic.h | 4 +++-
>>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>>> index 3cb59dd..9ab30ce 100644
>>> --- a/xen/arch/arm/gic-v2.c
>>> +++ b/xen/arch/arm/gic-v2.c
>>> @@ -823,6 +823,7 @@ DT_DEVICE_END
>>>   static const char * const hip04_gicv2_dt_compat[] __initconst =
>>>   {
>>>       DT_COMPAT_GIC_HIP04,
>>> +    DT_COMPAT_GIC_HIP04_2,
>>>       NULL
>>>   };
>>>
>>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>>> index 5adb628..3d2b3db 100644
>>> --- a/xen/include/asm-arm/gic.h
>>> +++ b/xen/include/asm-arm/gic.h
>>> @@ -156,11 +156,13 @@
>>>   #define DT_COMPAT_GIC_CORTEX_A15     "arm,cortex-a15-gic"
>>>   #define DT_COMPAT_GIC_CORTEX_A7      "arm,cortex-a7-gic"
>>>   #define DT_COMPAT_GIC_HIP04          "hisilicon,hip04-gic"
>>> +#define DT_COMPAT_GIC_HIP04_2        "hisilicon,hip04-intc"
>>>
>>>   #define DT_MATCH_GIC_V2 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A15), \
>>>                           DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A7), \
>>>                           DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400), \
>>> -                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04)
>>> +                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04), \
>>> +                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04_2)
>>>
>>>   #define DT_COMPAT_GIC_V3             "arm,gic-v3"
>>>
>>>
>>
>>
>> --
>> Julien Grall
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Re: [PATCH v3 4/8] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Julien Grall]

Hi Zoltan,

On 06/11/2014 09:46, Zoltan Kiss wrote:
>
>
> On 05/11/14 14:52, Stefano Stabellini wrote:
>> On Wed, 5 Nov 2014, Julien Grall wrote:
>>> Hi Frediano,
>>>
>>> On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
>>>> This name can appear in some Linux kernel repos. Not very fortunate,
>>>> but to avoid others spending an hour to spot that few characters
>>>> difference it worth to work around it.
>>>
>>> Linux upstream is using "hisilicon,hip04-intc" to detect the hisilicon
>>> interrupt controller. So it's not a workaround.
>>>
>>> Which kernel is using the "*,hip04-gic"?
>>
>> Good question, but what really matters is the string that u-boot (or any
>> other firmware/bootloader) is going to use, right? So, which one is it?
> We are using the DTB from the kernel source, even when loading a bare
> metal kernel. I've looked around, the *gic version seems to exist only
> in internal repos, as far as I can see. Including the one Frediano
> started to use for porting. Therefore, I don't insist to keep both, but
> as I mentioned in the commit message, it would still provide some
> benefit, and given that it's just a 3 line change which just extend a
> few listings, I think we should keep it.

In general, Xen should respect the binding that has been agreed by the 
device tree team. Anything different should not be upstream, hence it's 
for only internal purpose.

> Of course with a different commit message, which clears that this is the
> official name of it.

If it happens that both compatible are upstream. I would prefer that you 
define the *-intc one in the patch #3 and *-gic in #4.

It's more logical than defining first the non-official and then the 
official.

Regards,

-- 
Julien Grall

Re: [PATCH v3 4/8] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Frediano Ziglio]

> Hi Zoltan,
> 
> On 06/11/2014 09:46, Zoltan Kiss wrote:
> >
> >
> > On 05/11/14 14:52, Stefano Stabellini wrote:
> >> On Wed, 5 Nov 2014, Julien Grall wrote:
> >>> Hi Frediano,
> >>>
> >>> On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
> >>>> This name can appear in some Linux kernel repos. Not very
> >>>> fortunate, but to avoid others spending an hour to spot that few
> >>>> characters difference it worth to work around it.
> >>>
> >>> Linux upstream is using "hisilicon,hip04-intc" to detect the
> >>> hisilicon interrupt controller. So it's not a workaround.
> >>>
> >>> Which kernel is using the "*,hip04-gic"?
> >>
> >> Good question, but what really matters is the string that u-boot (or
> >> any other firmware/bootloader) is going to use, right? So, which one
> is it?
> > We are using the DTB from the kernel source, even when loading a bare
> > metal kernel. I've looked around, the *gic version seems to exist
> only
> > in internal repos, as far as I can see. Including the one Frediano
> > started to use for porting. Therefore, I don't insist to keep both,
> > but as I mentioned in the commit message, it would still provide some
> > benefit, and given that it's just a 3 line change which just extend a
> > few listings, I think we should keep it.
> 
> In general, Xen should respect the binding that has been agreed by the
> device tree team. Anything different should not be upstream, hence it's
> for only internal purpose.
> 
> > Of course with a different commit message, which clears that this is
> > the official name of it.
> 
> If it happens that both compatible are upstream. I would prefer that
> you define the *-intc one in the patch #3 and *-gic in #4.
> 
> It's more logical than defining first the non-official and then the
> official.
> 

Done

> Regards,
> 

Sent version 4.

Frediano

Re: [PATCH v3 4/8] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Julien Grall]

Hi Zoltan,

On 06/11/2014 09:46, Zoltan Kiss wrote:
>
>
> On 05/11/14 14:52, Stefano Stabellini wrote:
>> On Wed, 5 Nov 2014, Julien Grall wrote:
>>> Hi Frediano,
>>>
>>> On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
>>>> This name can appear in some Linux kernel repos. Not very fortunate,
>>>> but to avoid others spending an hour to spot that few characters
>>>> difference it worth to work around it.
>>>
>>> Linux upstream is using "hisilicon,hip04-intc" to detect the hisilicon
>>> interrupt controller. So it's not a workaround.
>>>
>>> Which kernel is using the "*,hip04-gic"?
>>
>> Good question, but what really matters is the string that u-boot (or any
>> other firmware/bootloader) is going to use, right? So, which one is it?
> We are using the DTB from the kernel source, even when loading a bare
> metal kernel. I've looked around, the *gic version seems to exist only
> in internal repos, as far as I can see. Including the one Frediano
> started to use for porting. Therefore, I don't insist to keep both, but
> as I mentioned in the commit message, it would still provide some
> benefit, and given that it's just a 3 line change which just extend a
> few listings, I think we should keep it.
> Of course with a different commit message, which clears that this is the
> official name of it.

If it's only used in your internal repo, we shouldn't support this 
compatible string in Xen.

Nothing prevent someone in the future to use this compatible for a 
completely different purpose (for instance a different GIC driver).

We aim to support only official bindings to avoid a such issue.

Regards,

-- 
Julien Grall

Re: [PATCH v3 4/8] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Frediano Ziglio]

> Hi Zoltan,
> 
> On 06/11/2014 09:46, Zoltan Kiss wrote:
> >
> >
> > On 05/11/14 14:52, Stefano Stabellini wrote:
> >> On Wed, 5 Nov 2014, Julien Grall wrote:
> >>> Hi Frediano,
> >>>
> >>> On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
> >>>> This name can appear in some Linux kernel repos. Not very
> >>>> fortunate, but to avoid others spending an hour to spot that few
> >>>> characters difference it worth to work around it.
> >>>
> >>> Linux upstream is using "hisilicon,hip04-intc" to detect the
> >>> hisilicon interrupt controller. So it's not a workaround.
> >>>
> >>> Which kernel is using the "*,hip04-gic"?
> >>
> >> Good question, but what really matters is the string that u-boot (or
> >> any other firmware/bootloader) is going to use, right? So, which one
> is it?
> > We are using the DTB from the kernel source, even when loading a bare
> > metal kernel. I've looked around, the *gic version seems to exist
> only
> > in internal repos, as far as I can see. Including the one Frediano
> > started to use for porting. Therefore, I don't insist to keep both,
> > but as I mentioned in the commit message, it would still provide some
> > benefit, and given that it's just a 3 line change which just extend a
> > few listings, I think we should keep it.
> > Of course with a different commit message, which clears that this is
> > the official name of it.
> 
> If it's only used in your internal repo, we shouldn't support this
> compatible string in Xen.
> 
> Nothing prevent someone in the future to use this compatible for a
> completely different purpose (for instance a different GIC driver).
> 
> We aim to support only official bindings to avoid a such issue.
> 
> Regards,
> 

Now all changes for old development compatible string are in a single patch so feel free to drop it.

Regards,
  Frediano

[PATCH v3 5/8] xen/arm: handle GICH register changes for hip04-d01 platform

[Frediano Ziglio]

The GICH in this platform is mainly compatible with the standard
GICv2 beside APR and LR register offsets.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/gic-v2.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 9ab30ce..d766438 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -61,6 +61,9 @@
 #define GICH_V2_VMCR_PRIORITY_MASK   0x1f
 #define GICH_V2_VMCR_PRIORITY_SHIFT  27
 
+#define HIP04_GICH_APR  0x70
+#define HIP04_GICH_LR   0x80
+
 /* Global state */
 static struct {
     paddr_t dbase;            /* Address of distributor registers */
@@ -85,6 +88,8 @@ static DEFINE_PER_CPU(u16, gic_cpu_id);
 static unsigned int nr_gic_cpu_if = 8;
 static unsigned int gicd_sgi_target_shift = GICD_SGI_TARGET_SHIFT;
 static unsigned int gic_cpu_mask = 0xff;
+static unsigned int gich_apr = GICH_APR;
+static unsigned int gich_lr = GICH_LR;
 
 #define is_hip04() (nr_gic_cpu_if == 16)
 
@@ -157,9 +162,9 @@ static void gicv2_save_state(struct vcpu *v)
      * accessed simultaneously by another pCPU.
      */
     for ( i = 0; i < gicv2_info.nr_lrs; i++ )
-        v->arch.gic.v2.lr[i] = readl_gich(GICH_LR + i * 4);
+        v->arch.gic.v2.lr[i] = readl_gich(gich_lr + i * 4);
 
-    v->arch.gic.v2.apr = readl_gich(GICH_APR);
+    v->arch.gic.v2.apr = readl_gich(gich_apr);
     v->arch.gic.v2.vmcr = readl_gich(GICH_VMCR);
     /* Disable until next VCPU scheduled */
     writel_gich(0, GICH_HCR);
@@ -170,9 +175,9 @@ static void gicv2_restore_state(const struct vcpu *v)
     int i;
 
     for ( i = 0; i < gicv2_info.nr_lrs; i++ )
-        writel_gich(v->arch.gic.v2.lr[i], GICH_LR + i * 4);
+        writel_gich(v->arch.gic.v2.lr[i], gich_lr + i * 4);
 
-    writel_gich(v->arch.gic.v2.apr, GICH_APR);
+    writel_gich(v->arch.gic.v2.apr, gich_apr);
     writel_gich(v->arch.gic.v2.vmcr, GICH_VMCR);
     writel_gich(GICH_HCR_EN, GICH_HCR);
 }
@@ -185,7 +190,7 @@ static void gicv2_dump_state(const struct vcpu *v)
     {
         for ( i = 0; i < gicv2_info.nr_lrs; i++ )
             printk("   HW_LR[%d]=%x\n", i,
-                   readl_gich(GICH_LR + i * 4));
+                   readl_gich(gich_lr + i * 4));
     }
     else
     {
@@ -439,12 +444,12 @@ static void gicv2_update_lr(int lr, const struct pending_irq *p,
                             << GICH_V2_LR_PHYSICAL_SHIFT);
     }
 
-    writel_gich(lr_reg, GICH_LR + lr * 4);
+    writel_gich(lr_reg, gich_lr + lr * 4);
 }
 
 static void gicv2_clear_lr(int lr)
 {
-    writel_gich(0, GICH_LR + lr * 4);
+    writel_gich(0, gich_lr + lr * 4);
 }
 
 static int gicv2v_setup(struct domain *d)
@@ -494,7 +499,7 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
 {
     uint32_t lrv;
 
-    lrv          = readl_gich(GICH_LR + lr * 4);
+    lrv          = readl_gich(gich_lr + lr * 4);
     lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
     lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
     lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
@@ -517,7 +522,7 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
                                        << GICH_V2_LR_HW_SHIFT)  |
           ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << GICH_V2_LR_GRP_SHIFT) );
 
-    writel_gich(lrv, GICH_LR + lr * 4);
+    writel_gich(lrv, gich_lr + lr * 4);
 }
 
 static void gicv2_hcr_status(uint32_t flag, bool_t status)
@@ -540,7 +545,7 @@ static unsigned int gicv2_read_vmcr_priority(void)
 
 static unsigned int gicv2_read_apr(int apr_reg)
 {
-   return readl_gich(GICH_APR);
+   return readl_gich(gich_apr);
 }
 
 static void gicv2_irq_enable(struct irq_desc *desc)
@@ -803,6 +808,8 @@ static int __init hip04_gicv2_init(struct dt_device_node *node, const void *data
     nr_gic_cpu_if = 16;
     gicd_sgi_target_shift = 8;
     gic_cpu_mask = 0xffff;
+    gich_apr = HIP04_GICH_APR;
+    gich_lr = HIP04_GICH_LR;
 
     return gicv2_init_default(node, data);
 }
-- 
1.9.1

[PATCH v3 6/8] xen/arm: Force dom0 to use normal GICv2 driver on Hip04 platform

[Frediano Ziglio]

Until vGIC support is not implemented and tested, this will prevent
guest kernels to use their Hip04 driver, or crash when they don't
have any.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/gic-v2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index d766438..2f6bbd5 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -641,6 +641,12 @@ static int gicv2_make_dt_node(const struct domain *d,
         return -FDT_ERR_XEN(ENOENT);
     }
 
+    if ( is_hip04() )
+    {
+        compatible = DT_COMPAT_GIC_CORTEX_A15;
+        len = strlen((char*) compatible) + 1;
+    }
+
     res = fdt_begin_node(fdt, "interrupt-controller");
     if ( res )
         return res;
-- 
1.9.1

[PATCH v3 7/8] xen/device_tree: Add dt_device_get_address_raw

[Frediano Ziglio]

Allow to read untranslated address from device node.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/common/device_tree.c      | 34 ++++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h | 11 +++++++++++
 2 files changed, 45 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 1a886c0..4186a24 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -711,6 +711,40 @@ int dt_device_get_address(const struct dt_device_node *dev, int index,
     return 0;
 }
 
+/* dt_device_get_address_raw - Returns address not translated */
+int dt_device_get_address_raw(const struct dt_device_node *dev, int index,
+                          u64 *addr)
+{
+    const __be32 *addrp;
+    const struct dt_device_node *parent;
+    const struct dt_bus *bus;
+    int na, ns;
+
+    if ( !addr )
+        return -EINVAL;
+
+    addrp = dt_get_address(dev, index, NULL, NULL);
+    if ( addrp == NULL )
+        return -EINVAL;
+
+    /* Get parent & match bus type */
+    parent = dt_get_parent(dev);
+    if ( parent == NULL )
+        return -EINVAL;
+    bus = dt_match_bus(parent);
+    if ( !bus )
+        return -EINVAL;
+
+    /* Count address cells & copy address locally */
+    bus->count_cells(dev, &na, &ns);
+    if ( !DT_CHECK_ADDR_COUNT(na) )
+        return -EINVAL;
+
+    *addr = dt_read_number(addrp, na);
+    return 0;
+}
+
+
 /**
  * dt_find_node_by_phandle - Find a node given a phandle
  * @handle: phandle of the node to find
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 629bfb2..3d2a4ae 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -475,6 +475,17 @@ int dt_device_get_address(const struct dt_device_node *dev, int index,
                           u64 *addr, u64 *size);
 
 /**
+ * dt_device_get_address_raw - Get an address for a device
+ * @device: the device whose address is to be resolved
+ * @index: index of the address to resolve
+ * @addr: address filled by this function
+ *
+ * This function get a raw (unresolved) address. It returns 0 on success.
+ */
+int dt_device_get_address_raw(const struct dt_device_node *dev, int index,
+                          u64 *addr);
+
+/**
  * dt_number_of_irq - Get the number of IRQ for a device
  * @device: the device whose number of interrupt is to be retrieved
  *
-- 
1.9.1

Re: [PATCH v3 7/8] xen/device_tree: Add dt_device_get_address_raw

[Julien Grall]

Hi Frediano,

I will made a global comment here for patch #7 and #8.

On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
> Allow to read untranslated address from device node.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> ---
>  xen/common/device_tree.c      | 34 ++++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h | 11 +++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 1a886c0..4186a24 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -711,6 +711,40 @@ int dt_device_get_address(const struct dt_device_node *dev, int index,
>      return 0;
>  }
>  
> +/* dt_device_get_address_raw - Returns address not translated */
> +int dt_device_get_address_raw(const struct dt_device_node *dev, int index,
> +                          u64 *addr)

This is wrong to assume that the untranslated address will fit in a 64
bits value.

Technically an untranslated address could be encoded on up to 4 cells
(though it has been hardcoded).

In any case, I don't think this is the right solution to the problem. As
DOM0 will always have the same layout as the hardware for the GIC, we
should copy "regs" and strip the unecessary regions (such as the GICH
and GICV).

Regards,

-- 
Julien Grall

[PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Frediano Ziglio]

Translated address could have an offset applied to them.
Replicate same value for device node to avoid improper address
computation in the OS.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/gic-v2.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 2f6bbd5..271074d 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -67,8 +67,10 @@
 /* Global state */
 static struct {
     paddr_t dbase;            /* Address of distributor registers */
+    paddr_t dbase_raw;        /* Untranslated address of distributor registers */
     void __iomem * map_dbase; /* IO mapped Address of distributor registers */
     paddr_t cbase;            /* Address of CPU interface registers */
+    paddr_t cbase_raw;        /* Untranslated address of CPU interface registers */
     void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
     paddr_t hbase;            /* Address of virtual interface registers */
     void __iomem * map_hbase; /* IO Address of virtual interface registers */
@@ -671,8 +673,17 @@ static int gicv2_make_dt_node(const struct domain *d,
         return -FDT_ERR_XEN(ENOMEM);
 
     tmp = new_cells;
-    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
-    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
+
+    if ( is_hardware_domain(d) )
+    {
+        dt_set_range(&tmp, node, gicv2.dbase_raw, PAGE_SIZE);
+        dt_set_range(&tmp, node, gicv2.cbase_raw, PAGE_SIZE * 2);
+    }
+    else
+    {
+        dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
+        dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
+    }
 
     res = fdt_property(fdt, "reg", new_cells, len);
     xfree(new_cells);
@@ -739,10 +750,18 @@ static int __init gicv2_init_default(struct dt_device_node *node, const void *da
     if ( res || !gicv2.dbase || (gicv2.dbase & ~PAGE_MASK) )
         panic("GICv2: Cannot find a valid address for the distributor");
 
+    res = dt_device_get_address_raw(node, 0, &gicv2.dbase_raw);
+    if ( res )
+        panic("GICv2: Cannot find a valid address for the distributor");
+
     res = dt_device_get_address(node, 1, &gicv2.cbase, NULL);
     if ( res || !gicv2.cbase || (gicv2.cbase & ~PAGE_MASK) )
         panic("GICv2: Cannot find a valid address for the CPU");
 
+    res = dt_device_get_address_raw(node, 1, &gicv2.cbase_raw);
+    if ( res )
+        panic("GICv2: Cannot find a valid address for the CPU");
+
     res = dt_device_get_address(node, 2, &gicv2.hbase, NULL);
     if ( res || !gicv2.hbase || (gicv2.hbase & ~PAGE_MASK) )
         panic("GICv2: Cannot find a valid address for the hypervisor");
-- 
1.9.1

Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Julien Grall]

Hi Frediano,

On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
> Translated address could have an offset applied to them.
> Replicate same value for device node to avoid improper address
> computation in the OS.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> ---
>  xen/arch/arm/gic-v2.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 2f6bbd5..271074d 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -67,8 +67,10 @@
>  /* Global state */
>  static struct {
>      paddr_t dbase;            /* Address of distributor registers */
> +    paddr_t dbase_raw;        /* Untranslated address of distributor registers */
>      void __iomem * map_dbase; /* IO mapped Address of distributor registers */
>      paddr_t cbase;            /* Address of CPU interface registers */
> +    paddr_t cbase_raw;        /* Untranslated address of CPU interface registers */
>      void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
>      paddr_t hbase;            /* Address of virtual interface registers */
>      void __iomem * map_hbase; /* IO Address of virtual interface registers */
> @@ -671,8 +673,17 @@ static int gicv2_make_dt_node(const struct domain *d,
>          return -FDT_ERR_XEN(ENOMEM);
>  
>      tmp = new_cells;
> -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> +
> +    if ( is_hardware_domain(d) )
> +    {

This check is pointless, as said on an earlier version of this series,
gicv2_make_dt_node is only used to create DOM0 (hardware domain) device
tree.

Regards,

-- 
Julien Grall

Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Zoltan Kiss]

Hi,

On 05/11/14 14:18, Julien Grall wrote:
> This check is pointless, as said on an earlier version of this series,
> gicv2_make_dt_node is only used to create DOM0 (hardware domain) device
> tree.

Btw. where is the guest DTB created? Quickly checking the code I 
couldn't find it.

Thanks,

Zoli

Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Julien Grall]

On 11/05/2014 02:54 PM, Zoltan Kiss wrote:
> Hi,
> 
> On 05/11/14 14:18, Julien Grall wrote:
>> This check is pointless, as said on an earlier version of this series,
>> gicv2_make_dt_node is only used to create DOM0 (hardware domain) device
>> tree.
> 
> Btw. where is the guest DTB created? Quickly checking the code I
> couldn't find it.

The guest DTB is created in by the toolstack:

tools/libxl/libxl_arm.c

Regards,

-- 
Julien Grall

Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Frediano Ziglio]

> Hi Frediano,
> 
> On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
> > Translated address could have an offset applied to them.
> > Replicate same value for device node to avoid improper address
> > computation in the OS.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> > ---
> >  xen/arch/arm/gic-v2.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> > 2f6bbd5..271074d 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -67,8 +67,10 @@
> >  /* Global state */
> >  static struct {
> >      paddr_t dbase;            /* Address of distributor registers */
> > +    paddr_t dbase_raw;        /* Untranslated address of distributor
> registers */
> >      void __iomem * map_dbase; /* IO mapped Address of distributor
> registers */
> >      paddr_t cbase;            /* Address of CPU interface registers
> */
> > +    paddr_t cbase_raw;        /* Untranslated address of CPU
> interface registers */
> >      void __iomem * map_cbase[2]; /* IO mapped Address of CPU
> interface registers */
> >      paddr_t hbase;            /* Address of virtual interface
> registers */
> >      void __iomem * map_hbase; /* IO Address of virtual interface
> > registers */ @@ -671,8 +673,17 @@ static int gicv2_make_dt_node(const
> struct domain *d,
> >          return -FDT_ERR_XEN(ENOMEM);
> >
> >      tmp = new_cells;
> > -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> > -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> > +
> > +    if ( is_hardware_domain(d) )
> > +    {
> 
> This check is pointless, as said on an earlier version of this series,
> gicv2_make_dt_node is only used to create DOM0 (hardware domain) device
> tree.
> 
> Regards,
> 
> --
> Julien Grall

How does sound something like this (already tested, it's working). Perhaps just to be paranoid a test on len after reading reg property would be perfect.

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 2f6bbd5..2c4d097 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -632,7 +632,7 @@ static int gicv2_make_dt_node(const struct domain *d,
     const void *compatible = NULL;
     u32 len;
     __be32 *new_cells, *tmp;
-    int res = 0;
+    int res = 0, na, ns;
 
     compatible = dt_get_property(gic, "compatible", &len);
     if ( !compatible )
@@ -664,15 +664,27 @@ static int gicv2_make_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));
+    /* copy GICC and GICD regions */
+    na = dt_n_addr_cells(node);
+    ns = dt_n_size_cells(node);
+
+    if ( na != dt_n_addr_cells(gic) || ns != dt_n_size_cells(gic) )
+        return -FDT_ERR_XEN(EINVAL);
+
+    tmp = (__be32 *) dt_get_property(gic, "reg", &len);
+    if ( !tmp )
+    {
+        dprintk(XENLOG_ERR, "Can't find reg property for the gic node\n");
+        return -FDT_ERR_XEN(ENOENT);
+    }
+
+    len = dt_cells_to_size(na + ns);
     len *= 2; /* GIC has two memory regions: Distributor + CPU interface */
     new_cells = xzalloc_bytes(len);
     if ( new_cells == NULL )
         return -FDT_ERR_XEN(ENOMEM);
 
-    tmp = new_cells;
-    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
-    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
+    memcpy(new_cells, tmp, len);
 
     res = fdt_property(fdt, "reg", new_cells, len);
     xfree(new_cells);
-- 
1.9.1

Frediano

Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Julien Grall]

On 11/05/2014 03:13 PM, Frediano Ziglio wrote:
> How does sound something like this (already tested, it's working).

The idea looks good to me. Few comments below.

Also, I'm wondering if we could create a generic function for this
purpose. The code of the GICv3 suffers of the same problem.

> Perhaps just to be paranoid a test on len after reading reg property
would be perfect.

The property/node has been validated in the GICv2 initialization.
Checking again here is not necessary.

> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 2f6bbd5..2c4d097 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -632,7 +632,7 @@ static int gicv2_make_dt_node(const struct domain *d,
>      const void *compatible = NULL;
>      u32 len;
>      __be32 *new_cells, *tmp;
> -    int res = 0;
> +    int res = 0, na, ns;
>  
>      compatible = dt_get_property(gic, "compatible", &len);
>      if ( !compatible )
> @@ -664,15 +664,27 @@ static int gicv2_make_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));
> +    /* copy GICC and GICD regions */
> +    na = dt_n_addr_cells(node);
> +    ns = dt_n_size_cells(node);
> +
> +    if ( na != dt_n_addr_cells(gic) || ns != dt_n_size_cells(gic) )
> +        return -FDT_ERR_XEN(EINVAL);

Not necessary, the caller of this function already check that gic (i.e
dt_interrupt_controller) == node.

If you really to be safe, I would add ASSERT(gic == node);

> +
> +    tmp = (__be32 *) dt_get_property(gic, "reg", &len);

The cast is not necessary.

> +    if ( !tmp )
> +    {
> +        dprintk(XENLOG_ERR, "Can't find reg property for the gic node\n");
> +        return -FDT_ERR_XEN(ENOENT);
> +    }
> +
> +    len = dt_cells_to_size(na + ns);
>      len *= 2; /* GIC has two memory regions: Distributor + CPU interface */
>      new_cells = xzalloc_bytes(len);
>      if ( new_cells == NULL )
>          return -FDT_ERR_XEN(ENOMEM);
>  
> -    tmp = new_cells;
> -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> +    memcpy(new_cells, tmp, len);

You don't need to copy the data in the temporary variable. You can
directly use fdt_property with the right len. Smth like:

fdt_property(fdt, "reg", tmp, len);

Regards,

-- 
Julien Grall

Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Frediano Ziglio]

> On 11/05/2014 03:13 PM, Frediano Ziglio wrote:
> > How does sound something like this (already tested, it's working).
> 
> The idea looks good to me. Few comments below.
> 
> Also, I'm wondering if we could create a generic function for this
> purpose. The code of the GICv3 suffers of the same problem.
> 
> > Perhaps just to be paranoid a test on len after reading reg property
> would be perfect.
> 
> The property/node has been validated in the GICv2 initialization.
> Checking again here is not necessary.
> 
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> > 2f6bbd5..2c4d097 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -632,7 +632,7 @@ static int gicv2_make_dt_node(const struct domain
> *d,
> >      const void *compatible = NULL;
> >      u32 len;
> >      __be32 *new_cells, *tmp;
> > -    int res = 0;
> > +    int res = 0, na, ns;
> >
> >      compatible = dt_get_property(gic, "compatible", &len);
> >      if ( !compatible )
> > @@ -664,15 +664,27 @@ static int gicv2_make_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));
> > +    /* copy GICC and GICD regions */
> > +    na = dt_n_addr_cells(node);
> > +    ns = dt_n_size_cells(node);
> > +
> > +    if ( na != dt_n_addr_cells(gic) || ns != dt_n_size_cells(gic) )
> > +        return -FDT_ERR_XEN(EINVAL);
> 
> Not necessary, the caller of this function already check that gic (i.e
> dt_interrupt_controller) == node.
> 
> If you really to be safe, I would add ASSERT(gic == node);
> 
> > +
> > +    tmp = (__be32 *) dt_get_property(gic, "reg", &len);
> 
> The cast is not necessary.
> 
> > +    if ( !tmp )
> > +    {
> > +        dprintk(XENLOG_ERR, "Can't find reg property for the gic
> node\n");
> > +        return -FDT_ERR_XEN(ENOENT);
> > +    }
> > +
> > +    len = dt_cells_to_size(na + ns);
> >      len *= 2; /* GIC has two memory regions: Distributor + CPU
> interface */
> >      new_cells = xzalloc_bytes(len);
> >      if ( new_cells == NULL )
> >          return -FDT_ERR_XEN(ENOMEM);
> >
> > -    tmp = new_cells;
> > -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> > -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> > +    memcpy(new_cells, tmp, len);
> 
> You don't need to copy the data in the temporary variable. You can
> directly use fdt_property with the right len. Smth like:
> 
> fdt_property(fdt, "reg", tmp, len);
> 
> Regards,
> 
> --
> Julien Grall

This then should look much better.

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 2f6bbd5..9b9e696 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -629,9 +629,8 @@ static int gicv2_make_dt_node(const struct domain *d,
                               const struct dt_device_node *node, void *fdt)
 {
     const struct dt_device_node *gic = dt_interrupt_controller;
-    const void *compatible = NULL;
+    const void *compatible = NULL, *tmp;
     u32 len;
-    __be32 *new_cells, *tmp;
     int res = 0;
 
     compatible = dt_get_property(gic, "compatible", &len);
@@ -664,18 +663,18 @@ static int gicv2_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 
+    /* copy GICC and GICD regions */
+    tmp = dt_get_property(gic, "reg", &len);
+    if ( !tmp )
+    {
+        dprintk(XENLOG_ERR, "Can't find reg property for the gic node\n");
+        return -FDT_ERR_XEN(ENOENT);
+    }
+
     len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
     len *= 2; /* GIC has two memory regions: Distributor + CPU interface */
-    new_cells = xzalloc_bytes(len);
-    if ( new_cells == NULL )
-        return -FDT_ERR_XEN(ENOMEM);
-
-    tmp = new_cells;
-    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
-    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
 
-    res = fdt_property(fdt, "reg", new_cells, len);
-    xfree(new_cells);
+    res = fdt_property(fdt, "reg", tmp, len);
 
     return res;
 }
-- 
1.9.1

Frediano

Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Julien Grall]

On 11/05/2014 03:46 PM, Frediano Ziglio wrote:
>> On 11/05/2014 03:13 PM, Frediano Ziglio wrote:
>>> How does sound something like this (already tested, it's working).
>>
>> The idea looks good to me. Few comments below.
>>
>> Also, I'm wondering if we could create a generic function for this
>> purpose. The code of the GICv3 suffers of the same problem.
>>
>>> Perhaps just to be paranoid a test on len after reading reg property
>> would be perfect.
>>
>> The property/node has been validated in the GICv2 initialization.
>> Checking again here is not necessary.
>>
>>>
>>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
>>> 2f6bbd5..2c4d097 100644
>>> --- a/xen/arch/arm/gic-v2.c
>>> +++ b/xen/arch/arm/gic-v2.c
>>> @@ -632,7 +632,7 @@ static int gicv2_make_dt_node(const struct domain
>> *d,
>>>      const void *compatible = NULL;
>>>      u32 len;
>>>      __be32 *new_cells, *tmp;
>>> -    int res = 0;
>>> +    int res = 0, na, ns;
>>>
>>>      compatible = dt_get_property(gic, "compatible", &len);
>>>      if ( !compatible )
>>> @@ -664,15 +664,27 @@ static int gicv2_make_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));
>>> +    /* copy GICC and GICD regions */
>>> +    na = dt_n_addr_cells(node);
>>> +    ns = dt_n_size_cells(node);
>>> +
>>> +    if ( na != dt_n_addr_cells(gic) || ns != dt_n_size_cells(gic) )
>>> +        return -FDT_ERR_XEN(EINVAL);
>>
>> Not necessary, the caller of this function already check that gic (i.e
>> dt_interrupt_controller) == node.
>>
>> If you really to be safe, I would add ASSERT(gic == node);
>>
>>> +
>>> +    tmp = (__be32 *) dt_get_property(gic, "reg", &len);
>>
>> The cast is not necessary.
>>
>>> +    if ( !tmp )
>>> +    {
>>> +        dprintk(XENLOG_ERR, "Can't find reg property for the gic
>> node\n");
>>> +        return -FDT_ERR_XEN(ENOENT);
>>> +    }
>>> +
>>> +    len = dt_cells_to_size(na + ns);
>>>      len *= 2; /* GIC has two memory regions: Distributor + CPU
>> interface */
>>>      new_cells = xzalloc_bytes(len);
>>>      if ( new_cells == NULL )
>>>          return -FDT_ERR_XEN(ENOMEM);
>>>
>>> -    tmp = new_cells;
>>> -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
>>> -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
>>> +    memcpy(new_cells, tmp, len);
>>
>> You don't need to copy the data in the temporary variable. You can
>> directly use fdt_property with the right len. Smth like:
>>
>> fdt_property(fdt, "reg", tmp, len);
>>
>> Regards,
>>
>> --
>> Julien Grall
> 
> This then should look much better.
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 2f6bbd5..9b9e696 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -629,9 +629,8 @@ static int gicv2_make_dt_node(const struct domain *d,
>                                const struct dt_device_node *node, void *fdt)
>  {
>      const struct dt_device_node *gic = dt_interrupt_controller;
> -    const void *compatible = NULL;
> +    const void *compatible = NULL, *tmp;
>      u32 len;
> -    __be32 *new_cells, *tmp;

I would prefer if you keep tmp as __be32 *. It documents the real type
of the data. Also, for clarity, I would rename it to cells.

BTW, the commit title/message wasn't really clear. It make thinks you
are fixing the problem for everyone and not only the GICv2.

Regards,

-- 
Julien Grall

Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Julien Grall]

On 11/05/2014 03:51 PM, Julien Grall wrote:
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index 2f6bbd5..9b9e696 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -629,9 +629,8 @@ static int gicv2_make_dt_node(const struct domain *d,
>>                                const struct dt_device_node *node, void *fdt)
>>  {
>>      const struct dt_device_node *gic = dt_interrupt_controller;
>> -    const void *compatible = NULL;
>> +    const void *compatible = NULL, *tmp;
>>      u32 len;
>> -    __be32 *new_cells, *tmp;
> 
> I would prefer if you keep tmp as __be32 *. It documents the real type
> of the data. Also, for clarity, I would rename it to cells.

s/cells/regs/.  Sorry

-- 
Julien Grall

Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

[Frediano Ziglio]

> On 11/05/2014 03:51 PM, Julien Grall wrote:
> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> >> 2f6bbd5..9b9e696 100644
> >> --- a/xen/arch/arm/gic-v2.c
> >> +++ b/xen/arch/arm/gic-v2.c
> >> @@ -629,9 +629,8 @@ static int gicv2_make_dt_node(const struct
> domain *d,
> >>                                const struct dt_device_node *node,
> >> void *fdt)  {
> >>      const struct dt_device_node *gic = dt_interrupt_controller;
> >> -    const void *compatible = NULL;
> >> +    const void *compatible = NULL, *tmp;
> >>      u32 len;
> >> -    __be32 *new_cells, *tmp;
> >
> > I would prefer if you keep tmp as __be32 *. It documents the real
> type
> > of the data. Also, for clarity, I would rename it to cells.
> 
> s/cells/regs/.  Sorry
> 
> --
> Julien Grall

This version should be perfect then.


Subject: [PATCH 7/7] xen/arm: Handle translated addresses for hardware domains in GICv2

Translated address could have an offset applied to them.
Replicate same value for device node to avoid improper address
computation in the OS.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/gic-v2.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 2f6bbd5..c2666df 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -631,7 +631,7 @@ static int gicv2_make_dt_node(const struct domain *d,
     const struct dt_device_node *gic = dt_interrupt_controller;
     const void *compatible = NULL;
     u32 len;
-    __be32 *new_cells, *tmp;
+    const __be32 *regs;
     int res = 0;
 
     compatible = dt_get_property(gic, "compatible", &len);
@@ -664,18 +664,18 @@ static int gicv2_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 
+    /* copy GICC and GICD regions */
+    regs = dt_get_property(gic, "reg", &len);
+    if ( !regs )
+    {
+        dprintk(XENLOG_ERR, "Can't find reg property for the gic node\n");
+        return -FDT_ERR_XEN(ENOENT);
+    }
+
     len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
     len *= 2; /* GIC has two memory regions: Distributor + CPU interface */
-    new_cells = xzalloc_bytes(len);
-    if ( new_cells == NULL )
-        return -FDT_ERR_XEN(ENOMEM);
-
-    tmp = new_cells;
-    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
-    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
 
-    res = fdt_property(fdt, "reg", new_cells, len);
-    xfree(new_cells);
+    res = fdt_property(fdt, "reg", regs, len);
 
     return res;
 }
-- 
1.9.1

Re: [PATCH v3] xen/arm: Add support for Huawei hip04-d01 platform

[Julien Grall]

Hi Frediano,

Thank you for adding support to this board.

Could you provide the instruction to boot Xen on this board?

It might be interesting to add a wiki page with them and link to:

http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions

Regards,

On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
> This set of patches add Xen support for hip04-d01 platform (see https://wiki.linaro.org/Boards/D01 for details).
> 
> Changes from v2:
> - rewrote DTS fix patch (Ian Campbell);
> - use is_hip04 macro instead of doing explicit test (Julien Grall);
> - do not use quirks to distinguish this platform (Ian Cambell);
> - move some GIC constants to C files instead of header (Julien Grall);
> - minor changes (Julien Grall).
> 
> Changes from v1:
> - style (Julien Grall);
> - make gicv2_send_SGI faster (Julien Grall);
> - cleanup correctly if hip04_smp_init fails (Julien Grall);
> - remove quirks using compatibility (Ian Campbell);
> - other minor suggestions by Julien Grall.
> 
> 
> 


-- 
Julien Grall