Xen devel - Nov 2013 - [PATCH+RFC+HACK 00/16] xen: arm initial support for xgene arm64 platform

I''m afraid this series is rather a grab bag and it is distressingly
large at this stage. With this series I can boot an Xgene board until it
fails to find its SATA controller. This is a dom0 issue for which
patches are pending from APM (/me nudges Anup).

As well as the APM specific platform stuff there are also some generic
improvements which were either necessary or useful during this work.
Towards the tail end are some hacks and rfcs which need more work and/or
discussion. I mostly posting now because I''m aware that I''ve
been
negligent in not sending these out sooner.

WRT the freeze I think that the baseline stuff is all plausible for 4.4.
Firstly because I''m inclined to give new platform enablement a fairly
loose reign at least for the time being (and much of it was posted ages
ago by Anup/Pranavkumar). Secondly the non-platform related bits (other
than the aforementioned hacks/rfcs) fall mostly either into two buckets:
Either they are bugfixes or they are mostly obviously safe (additional
debug prints etc).

Blow by blow analysis:

        xen: arm64: Add 8250 earlyprintk support
        
                New early uart driver. It is enabled as a build time
                debug option and is totally harmless to platforms which
                don''t use it.
                
        xen: arm64: Add Basic Platform support for APM X-Gene Storm.
        xen: arm64: Add APM implementor id to processor implementers.
        xen: arm: include ns16550 driver on arm64 too
        xen: arm: Enable 1:1 workaround for APM X-Gene Storm.
        
                Support for the new platform. Enable an existing driver
                used by that platform (already on for arm32).
                
        xen: arm: early logging of command line
        
                Pretty safe & very useful IMNSVHO.
                
        xen: arm: Handle cpus nodes with #address-calls > 1
        xen: arm: Make register bit definitions unsigned.
        xen: arm: explicitly map 64 bit release address
        
                Bug fixes.
        
        xen: arm: enable synchronous console while starting secondary CPUs
        
                Improves logging in a useful way. Pretty safe.

        xen: arm: Add debug keyhandler to dump the physical GIC state.
        
                Useful debug functionality. Harmless unless you
                deliberately trigger the particular debug key.
                
        xen: arm: improve early memory map readability
        
                Cosmetic, but safe.
        
        RFC: xen: arm: handle 40-bit addresses in the p2m
        RFC: xen: arm: allow platform code to select dom0 event channel irq
        
                These should be considered for cleanup review and
                eventual commit for 4.4. The rest of the platform
                enablement is pretty pointless without these.
        
        HACK: xen: arm: GICC_DIR register at offset 0x10000 instead of 0x1000
        
                Should be properly implemented with a view to being
                accepted for 4.4. Again things are rather pointless
                without.
                
                Could plausibly be reimplemented as a platform quirk,
                which might be safer for 4.4.
                
        HACK: xen: arm: map PCI controller ranges region MMIOs to dom0.
        
                I think this one is likely to be a step too far for 4.4.
                Even if it worked (it doesn''t) it is quite a big and
                potentially complex change. I''m considering the option
                of implementing the hardcoded list (which is here as a
                HACK, see the commit message for more) via the
                platform->specific_mapping callback for 4.4. In that
                case it would only impact Xgene if it were broken.

Ian.

From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>

Extracted from "Basic Platform support for APM X-Gene Storm."

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>

Reworked into generic 8250 driver, use EARLY_UART_REG_SHIFT.

While there observe a missing shift in the arm32 version (UART_THR is zero so
it doesn''t really matter). Changed for consistency.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/Rules.mk             |    6 +++++
 xen/arch/arm/arm64/debug-8250.inc |   52 +++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
 create mode 100644 xen/arch/arm/arm64/debug-8250.inc

diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index c27c2eb..aaa203e 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -82,6 +82,12 @@ EARLY_PRINTK_INC := 8250
 EARLY_UART_BASE_ADDRESS := 0xF0406B00
 EARLY_UART_REG_SHIFT := 2
 endif
+ifeq ($(CONFIG_EARLY_PRINTK), xgene-storm)
+EARLY_PRINTK_INC := 8250
+EARLY_PRINTK_BAUD := 115200
+EARLY_UART_BASE_ADDRESS := 0x1c020000
+EARLY_UART_REG_SHIFT := 2
+endif
 
 ifneq ($(EARLY_PRINTK_INC),)
 EARLY_PRINTK := y
diff --git a/xen/arch/arm/arm64/debug-8250.inc
b/xen/arch/arm/arm64/debug-8250.inc
new file mode 100644
index 0000000..5858354
--- /dev/null
+++ b/xen/arch/arm/arm64/debug-8250.inc
@@ -0,0 +1,52 @@
+/*
+ * xen/arch/arm/arm64/debug-8250.inc
+ *
+ * 8250 specific debug code
+ *
+ * Copyright (c) 2013 Applied Micro.
+ *
+ * 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 <xen/8250-uart.h>
+
+/* UART initialization
+ * rb: register which contains the UART base address
+ * rc: scratch register 1
+ * rd: scratch register 2 */
+.macro early_uart_init rb rc rd
+.endm
+
+/* UART wait UART to be ready to transmit
+ * xb: register which contains the UART base address
+ * c: scratch register */
+.macro early_uart_ready xb c
+1:
+       ldrb  w\c, [\xb, #UART_LSR << EARLY_UART_REG_SHIFT]
+       and w\c, w\c, #UART_LSR_THRE
+       cmp w\c, #UART_LSR_THRE
+       b.ne 1b
+.endm
+
+/* UART transmit character
+ * xb: register which contains the UART base address
+ * wt: register which contains the character to transmit */
+.macro early_uart_transmit xb wt
+        /* UART_THR  transmit holding */
+        strb   \wt, [\xb, #UART_THR << EARLY_UART_REG_SHIFT]
+.endm
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.10.4

From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>

This patch adds initial platform stubs for APM X-Gene.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>

Drop earlyprintk (split into earlier patch). Only build on ARM64.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/platforms/Makefile      |    1 +
 xen/arch/arm/platforms/xgene-storm.c |   57 ++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
 create mode 100644 xen/arch/arm/platforms/xgene-storm.c

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index f0dd72c..680364f 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_ARM_32) += exynos5.o
 obj-$(CONFIG_ARM_32) += midway.o
 obj-$(CONFIG_ARM_32) += omap5.o
 obj-$(CONFIG_ARM_32) += sunxi.o
+obj-$(CONFIG_ARM_64) += xgene-storm.o
diff --git a/xen/arch/arm/platforms/xgene-storm.c
b/xen/arch/arm/platforms/xgene-storm.c
new file mode 100644
index 0000000..8e2b3b6
--- /dev/null
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -0,0 +1,57 @@
+/*
+ * xen/arch/arm/platforms/xgene-storm.c
+ *
+ * Applied Micro''s X-Gene specific settings
+ *
+ * Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
+ * Anup Patel <apatel@apm.com>
+ * Copyright (c) 2013 Applied Micro.
+ *
+ * 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 <xen/config.h>
+#include <xen/device_tree.h>
+#include <xen/domain_page.h>
+#include <xen/mm.h>
+#include <xen/vmap.h>
+#include <asm/platform.h>
+#include <asm/early_printk.h>
+
+static void xgene_storm_reset(void)
+{
+}
+
+static int xgene_storm_init(void)
+{
+        return 0;
+}
+
+static const char const *xgene_storm_dt_compat[] __initdata +{
+    "apm,xgene-storm",
+    NULL
+};
+
+PLATFORM_START(xgene_storm, "APM X-GENE STORM")
+    .compatible = xgene_storm_dt_compat,
+    .init = xgene_storm_init,
+    .reset = xgene_storm_reset,
+PLATFORM_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.10.4

From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>

This patch updates the list of processor implementers with APM implementor id.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 xen/arch/arm/setup.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index cdcc2e7..49e1b5c 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -64,6 +64,7 @@ static const char * __initdata processor_implementers[] = {
     [''B''] = "Broadcom Corporation",
     [''D''] = "Digital Equipment Corp",
     [''M''] = "Motorola, Freescale Semiconductor
Inc.",
+    [''P''] = "Applied Micro",
     [''Q''] = "Qualcomm Inc.",
     [''V''] = "Marvell Semiconductor Inc.",
     [''i''] = "Intel Corporation",
-- 
1.7.10.4

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 config/arm64.mk |    1 +
 1 file changed, 1 insertion(+)

diff --git a/config/arm64.mk b/config/arm64.mk
index 49055fa..15b57a4 100644
--- a/config/arm64.mk
+++ b/config/arm64.mk
@@ -7,6 +7,7 @@ CONFIG_XEN_INSTALL_SUFFIX : CFLAGS += #-marm -march= -mcpu= etc
 
 HAS_PL011 := y
+HAS_NS16550 := y
 
 # Use only if calling $(LD) directly.
 LDFLAGS_DIRECT += -EL
-- 
1.7.10.4

There is no SMMU on this platform.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/platforms/xgene-storm.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/arm/platforms/xgene-storm.c
b/xen/arch/arm/platforms/xgene-storm.c
index 8e2b3b6..23986a9 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -35,6 +35,12 @@ static int xgene_storm_init(void)
         return 0;
 }
 
+static uint32_t xgene_storm_quirks(void)
+{
+    return PLATFORM_QUIRK_DOM0_MAPPING_11;
+}
+
+
 static const char const *xgene_storm_dt_compat[] __initdata  {
     "apm,xgene-storm",
@@ -45,6 +51,7 @@ 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
 
 /*
-- 
1.7.10.4

Helpful for diagnosis of bad console= parameters.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/setup.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 49e1b5c..d252131 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -597,6 +597,7 @@ void __init start_xen(unsigned long boot_phys_offset,
 {
     size_t fdt_size;
     int cpus, i;
+    const char *cmdline;
 
     setup_cache();
 
@@ -610,7 +611,9 @@ void __init start_xen(unsigned long boot_phys_offset,
         + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
     fdt_size = device_tree_early_init(device_tree_flattened, fdt_paddr);
 
-    cmdline_parse(device_tree_bootargs(device_tree_flattened));
+    cmdline = device_tree_bootargs(device_tree_flattened);
+    early_printk("Command line: %s\n", cmdline);
+    cmdline_parse(cmdline);
 
     setup_pagetables(boot_phys_offset, get_xen_paddr());
     setup_mm(fdt_paddr, fdt_size);
-- 
1.7.10.4

The APM X-Gene Mustang board DTS has #address-cells = 2.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/smpboot.c |   25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 6c90fa6..ce832ae 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -125,18 +125,35 @@ void __init smp_init_cpus(void)
 
     dt_for_each_child_node( cpus, cpu )
     {
+        const __be32 *prop;
+        u64 addr, size;
         u32 hwid;
 
         if ( !dt_device_type_is_equal(cpu, "cpu") )
             continue;
 
-        if ( !dt_property_read_u32(cpu, "reg", &hwid) )
+        if ( dt_n_size_cells(cpu) != 0 )
+            printk(XENLOG_WARNING "cpu node `%s`: #size-cells %d\n",
+                   dt_node_full_name(cpu), dt_n_size_cells(cpu));
+
+        prop = dt_get_property(cpu, "reg", NULL);
+        if ( !prop )
         {
-            printk(XENLOG_WARNING "cpu node `%s`: missing reg
property\n",
+            printk(XENLOG_WARNING "cpu node `%s`: has no reg
property\n",
                    dt_node_full_name(cpu));
             continue;
         }
 
+        dt_get_range(&prop, cpu, &addr, &size);
+
+        hwid = addr;
+        if ( hwid != addr )
+        {
+            printk(XENLOG_WARNING "cpu node `%s`: hwid overflow
%"PRIx64"\n",
+                   dt_node_full_name(cpu), addr);
+            continue;
+        }
+
         /*
          * 8 MSBs must be set to 0 in the DT since the reg property
          * defines the MPIDR[23:0]
@@ -159,8 +176,8 @@ void __init smp_init_cpus(void)
             if ( tmp_map[j] == hwid )
             {
                 printk(XENLOG_WARNING
-                       "cpu node `%s`: duplicate /cpu reg properties in
the DT\n",
-                       dt_node_full_name(cpu));
+                       "cpu node `%s`: duplicate /cpu reg properties
%"PRIx32" in the DT\n",
+                       dt_node_full_name(cpu), hwid);
                 break;
             }
         }
-- 
1.7.10.4

Otherwise the results of the shifting can be undefined and/or sign extended.

Pointed out in the context of HCR_* by Pranavkumar Sawargaonkar.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
---
 xen/include/asm-arm/processor.h |  144 +++++++++++++++++++--------------------
 1 file changed, 72 insertions(+), 72 deletions(-)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 3da3a3d..7cd2d3e 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -18,71 +18,71 @@
 #define MPIDR_INVALID       (~MPIDR_HWID_MASK)
 
 /* TTBCR Translation Table Base Control Register */
-#define TTBCR_EAE    0x80000000
-#define TTBCR_N_MASK 0x07
-#define TTBCR_N_16KB 0x00
-#define TTBCR_N_8KB  0x01
-#define TTBCR_N_4KB  0x02
-#define TTBCR_N_2KB  0x03
-#define TTBCR_N_1KB  0x04
+#define TTBCR_EAE    _AC(0x80000000,U)
+#define TTBCR_N_MASK _AC(0x07,U)
+#define TTBCR_N_16KB _AC(0x00,U)
+#define TTBCR_N_8KB  _AC(0x01,U)
+#define TTBCR_N_4KB  _AC(0x02,U)
+#define TTBCR_N_2KB  _AC(0x03,U)
+#define TTBCR_N_1KB  _AC(0x04,U)
 
 /* SCTLR System Control Register. */
 /* HSCTLR is a subset of this. */
-#define SCTLR_TE        (1<<30)
-#define SCTLR_AFE       (1<<29)
-#define SCTLR_TRE       (1<<28)
-#define SCTLR_NMFI      (1<<27)
-#define SCTLR_EE        (1<<25)
-#define SCTLR_VE        (1<<24)
-#define SCTLR_U         (1<<22)
-#define SCTLR_FI        (1<<21)
-#define SCTLR_WXN       (1<<19)
-#define SCTLR_HA        (1<<17)
-#define SCTLR_RR        (1<<14)
-#define SCTLR_V         (1<<13)
-#define SCTLR_I         (1<<12)
-#define SCTLR_Z         (1<<11)
-#define SCTLR_SW        (1<<10)
-#define SCTLR_B         (1<<7)
-#define SCTLR_C         (1<<2)
-#define SCTLR_A         (1<<1)
-#define SCTLR_M         (1<<0)
-
-#define HSCTLR_BASE       0x30c51878
+#define SCTLR_TE        (_AC(1,U)<<30)
+#define SCTLR_AFE       (_AC(1,U)<<29)
+#define SCTLR_TRE       (_AC(1,U)<<28)
+#define SCTLR_NMFI      (_AC(1,U)<<27)
+#define SCTLR_EE        (_AC(1,U)<<25)
+#define SCTLR_VE        (_AC(1,U)<<24)
+#define SCTLR_U         (_AC(1,U)<<22)
+#define SCTLR_FI        (_AC(1,U)<<21)
+#define SCTLR_WXN       (_AC(1,U)<<19)
+#define SCTLR_HA        (_AC(1,U)<<17)
+#define SCTLR_RR        (_AC(1,U)<<14)
+#define SCTLR_V         (_AC(1,U)<<13)
+#define SCTLR_I         (_AC(1,U)<<12)
+#define SCTLR_Z         (_AC(1,U)<<11)
+#define SCTLR_SW        (_AC(1,U)<<10)
+#define SCTLR_B         (_AC(1,U)<<7)
+#define SCTLR_C         (_AC(1,U)<<2)
+#define SCTLR_A         (_AC(1,U)<<1)
+#define SCTLR_M         (_AC(1,U)<<0)
+
+#define HSCTLR_BASE     _AC(0x30c51878,U)
 
 /* HCR Hyp Configuration Register */
-#define HCR_RW          (1<<31) /* Register Width, ARM64 only */
-#define HCR_TGE         (1<<27) /* Trap General Exceptions */
-#define HCR_TVM         (1<<26) /* Trap Virtual Memory Controls */
-#define HCR_TTLB        (1<<25) /* Trap TLB Maintenance Operations */
-#define HCR_TPU         (1<<24) /* Trap Cache Maintenance Operations to
PoU */
-#define HCR_TPC         (1<<23) /* Trap Cache Maintenance Operations to
PoC */
-#define HCR_TSW         (1<<22) /* Trap Set/Way Cache Maintenance
Operations */
-#define HCR_TAC         (1<<21) /* Trap ACTLR Accesses */
-#define HCR_TIDCP       (1<<20) /* Trap lockdown */
-#define HCR_TSC         (1<<19) /* Trap SMC instruction */
-#define HCR_TID3        (1<<18) /* Trap ID Register Group 3 */
-#define HCR_TID2        (1<<17) /* Trap ID Register Group 2 */
-#define HCR_TID1        (1<<16) /* Trap ID Register Group 1 */
-#define HCR_TID0        (1<<15) /* Trap ID Register Group 0 */
-#define HCR_TWE         (1<<14) /* Trap WFE instruction */
-#define HCR_TWI         (1<<13) /* Trap WFI instruction */
-#define HCR_DC          (1<<12) /* Default cacheable */
-#define HCR_BSU_MASK    (3<<10) /* Barrier Shareability Upgrade */
-#define HCR_BSU_NONE     (0<<10)
-#define HCR_BSU_INNER    (1<<10)
-#define HCR_BSU_OUTER    (2<<10)
-#define HCR_BSU_FULL     (3<<10)
-#define HCR_FB          (1<<9) /* Force Broadcast of Cache/BP/TLB
operations */
-#define HCR_VA          (1<<8) /* Virtual Asynchronous Abort */
-#define HCR_VI          (1<<7) /* Virtual IRQ */
-#define HCR_VF          (1<<6) /* Virtual FIQ */
-#define HCR_AMO         (1<<5) /* Override CPSR.A */
-#define HCR_IMO         (1<<4) /* Override CPSR.I */
-#define HCR_FMO         (1<<3) /* Override CPSR.F */
-#define HCR_PTW         (1<<2) /* Protected Walk */
-#define HCR_SWIO        (1<<1) /* Set/Way Invalidation Override */
-#define HCR_VM          (1<<0) /* Virtual MMU Enable */
+#define HCR_RW          (_AC(1,U)<<31) /* Register Width, ARM64 only */
+#define HCR_TGE         (_AC(1,U)<<27) /* Trap General Exceptions */
+#define HCR_TVM         (_AC(1,U)<<26) /* Trap Virtual Memory Controls */
+#define HCR_TTLB        (_AC(1,U)<<25) /* Trap TLB Maintenance Operations
*/
+#define HCR_TPU         (_AC(1,U)<<24) /* Trap Cache Maintenance
Operations to PoU */
+#define HCR_TPC         (_AC(1,U)<<23) /* Trap Cache Maintenance
Operations to PoC */
+#define HCR_TSW         (_AC(1,U)<<22) /* Trap Set/Way Cache Maintenance
Operations */
+#define HCR_TAC         (_AC(1,U)<<21) /* Trap ACTLR Accesses */
+#define HCR_TIDCP       (_AC(1,U)<<20) /* Trap lockdown */
+#define HCR_TSC         (_AC(1,U)<<19) /* Trap SMC instruction */
+#define HCR_TID3        (_AC(1,U)<<18) /* Trap ID Register Group 3 */
+#define HCR_TID2        (_AC(1,U)<<17) /* Trap ID Register Group 2 */
+#define HCR_TID1        (_AC(1,U)<<16) /* Trap ID Register Group 1 */
+#define HCR_TID0        (_AC(1,U)<<15) /* Trap ID Register Group 0 */
+#define HCR_TWE         (_AC(1,U)<<14) /* Trap WFE instruction */
+#define HCR_TWI         (_AC(1,U)<<13) /* Trap WFI instruction */
+#define HCR_DC          (_AC(1,U)<<12) /* Default cacheable */
+#define HCR_BSU_MASK    (_AC(3,U)<<10) /* Barrier Shareability Upgrade */
+#define HCR_BSU_NONE     (_AC(0,U)<<10)
+#define HCR_BSU_INNER    (_AC(1,U)<<10)
+#define HCR_BSU_OUTER    (_AC(2,U)<<10)
+#define HCR_BSU_FULL     (_AC(3,U)<<10)
+#define HCR_FB          (_AC(1,U)<<9) /* Force Broadcast of Cache/BP/TLB
operations */
+#define HCR_VA          (_AC(1,U)<<8) /* Virtual Asynchronous Abort */
+#define HCR_VI          (_AC(1,U)<<7) /* Virtual IRQ */
+#define HCR_VF          (_AC(1,U)<<6) /* Virtual FIQ */
+#define HCR_AMO         (_AC(1,U)<<5) /* Override CPSR.A */
+#define HCR_IMO         (_AC(1,U)<<4) /* Override CPSR.I */
+#define HCR_FMO         (_AC(1,U)<<3) /* Override CPSR.F */
+#define HCR_PTW         (_AC(1,U)<<2) /* Protected Walk */
+#define HCR_SWIO        (_AC(1,U)<<1) /* Set/Way Invalidation Override */
+#define HCR_VM          (_AC(1,U)<<0) /* Virtual MMU Enable */
 
 #define HSR_EC_UNKNOWN              0x00
 #define HSR_EC_WFI_WFE              0x01
@@ -346,20 +346,20 @@ union hsr {
                               HSR_SYSREG_OP2_MASK)
 
 /* Physical Address Register */
-#define PAR_F           (1<<0)
+#define PAR_F           (_AC(1,U)<<0)
 
 /* .... If F == 1 */
 #define PAR_FSC_SHIFT   (1)
-#define PAR_FSC_MASK    (0x3f<<PAR_FSC_SHIFT)
-#define PAR_STAGE21     (1<<8)     /* Stage 2 Fault During Stage 1 Walk
*/
-#define PAR_STAGE2      (1<<9)     /* Stage 2 Fault */
+#define PAR_FSC_MASK    (_AC(0x3f,U)<<PAR_FSC_SHIFT)
+#define PAR_STAGE21     (_AC(1,U)<<8)     /* Stage 2 Fault During Stage 1
Walk */
+#define PAR_STAGE2      (_AC(1,U)<<9)     /* Stage 2 Fault */
 
 /* If F == 0 */
 #define PAR_MAIR_SHIFT  56                       /* Memory Attributes */
 #define PAR_MAIR_MASK   (0xffLL<<PAR_MAIR_SHIFT)
-#define PAR_NS          (1<<9)                   /* Non-Secure */
+#define PAR_NS          (_AC(1,U)<<9)                   /* Non-Secure */
 #define PAR_SH_SHIFT    7                        /* Shareability */
-#define PAR_SH_MASK     (3<<PAR_SH_SHIFT)
+#define PAR_SH_MASK     (_AC(3,U)<<PAR_SH_SHIFT)
 
 /* Fault Status Register */
 /*
@@ -372,11 +372,11 @@ union hsr {
  * 10xxxx -- Other
  * 11xxxx -- Implementation Defined
  */
-#define FSC_TYPE_MASK (0x3<<4)
-#define FSC_TYPE_FAULT (0x00<<4)
-#define FSC_TYPE_ABT   (0x01<<4)
-#define FSC_TYPE_OTH   (0x02<<4)
-#define FSC_TYPE_IMPL  (0x03<<4)
+#define FSC_TYPE_MASK (_AC(0x3,U)<<4)
+#define FSC_TYPE_FAULT (_AC(0x00,U)<<4)
+#define FSC_TYPE_ABT   (_AC(0x01,U)<<4)
+#define FSC_TYPE_OTH   (_AC(0x02,U)<<4)
+#define FSC_TYPE_IMPL  (_AC(0x03,U)<<4)
 
 #define FSC_FLT_TRANS  (0x04)
 #define FSC_FLT_ACCESS (0x08)
@@ -391,7 +391,7 @@ union hsr {
 #define FSC_LKD        (0x34) /* Lockdown Abort */
 #define FSC_CPR        (0x3a) /* Coprocossor Abort */
 
-#define FSC_LL_MASK    (0x03<<0)
+#define FSC_LL_MASK    (_AC(0x03,U)<<0)
 
 /* Time counter hypervisor control register */
 #define CNTHCTL_PA      (1u<<0)  /* Kernel/user access to physical
counter */
-- 
1.7.10.4

In case it is outside visible ram.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/arm64/smpboot.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 8239590..8696ed6 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -4,6 +4,8 @@
 #include <xen/errno.h>
 #include <xen/mm.h>
 #include <xen/smp.h>
+#include <xen/vmap.h>
+#include <asm/io.h>
 
 struct smp_enable_ops {
         int             (*prepare_cpu)(int);
@@ -14,7 +16,7 @@ static struct smp_enable_ops smp_enable_ops[NR_CPUS];
 
 static int __init smp_spin_table_cpu_up(int cpu)
 {
-    paddr_t *release;
+    paddr_t __iomem *release;
 
     if (!cpu_release_addr[cpu])
     {
@@ -22,12 +24,20 @@ static int __init smp_spin_table_cpu_up(int cpu)
         return -ENODEV;
     }
 
-    release = __va(cpu_release_addr[cpu]);
+    release = ioremap_nocache(cpu_release_addr[cpu], 8);
+    if ( !release )
+    {
+        dprintk(XENLOG_ERR, "CPU%d: Unable to map release address\n",
cpu);
+        return -EFAULT;
+    }
 
     release[0] = __pa(init_secondary);
     flush_xen_data_tlb_range_va((vaddr_t)release, sizeof(*release));
 
+    iounmap(release);
+
     sev();
+
     return 0;
 }
 
-- 
1.7.10.4

If CONFIG_EARLY_PRINTK is in use then this gets all interleaved with the
standard logging otherwise.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/smpboot.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index ce832ae..9b58345 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -28,6 +28,7 @@
 #include <xen/softirq.h>
 #include <xen/timer.h>
 #include <xen/irq.h>
+#include <xen/console.h>
 #include <asm/gic.h>
 
 cpumask_t cpu_online_map;
@@ -349,6 +350,8 @@ int __cpu_up(unsigned int cpu)
     if ( rc < 0 )
         return rc;
 
+    console_start_sync(); /* Secondary may use early_printk */
+
     /* Tell the remote CPU which stack to boot on. */
     init_data.stack = idle_vcpu[cpu]->arch.stack;
 
@@ -361,6 +364,8 @@ int __cpu_up(unsigned int cpu)
 
     rc = arch_cpu_up(cpu);
 
+    console_end_sync();
+
     if ( rc < 0 )
     {
         printk("Failed to bring up CPU%d\n", cpu);
-- 
1.7.10.4

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/gic.c |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ab49106..185a6b8 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -28,6 +28,7 @@
 #include <xen/softirq.h>
 #include <xen/list.h>
 #include <xen/device_tree.h>
+#include <xen/keyhandler.h>
 #include <asm/p2m.h>
 #include <asm/domain.h>
 
@@ -385,6 +386,77 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
     return 0;
 }
 
+
+static void do_dump_gic(unsigned char key)
+{
+    int irq;
+    printk("''%c'' pressed -> dumping GIC
state\n", key);
+
+    for ( irq = 0; irq < gic.lines; irq++ )
+    {
+        const char *type;
+        int type_nr, enable, pend, active, priority, target;
+        struct irq_desc *desc = irq_to_desc(irq);
+        uint8_t *bytereg;
+        uint32_t wordreg;
+
+        bytereg = (uint8_t *) (GICD + GICD_ITARGETSR);
+        target = bytereg[irq];
+
+        bytereg = (uint8_t *) (GICD + GICD_IPRIORITYR);
+        priority = bytereg[irq];
+
+        switch ( irq )
+        {
+        case 0 ... 15:
+            type = "SGI";
+            type_nr = irq;
+            target = 0x00; /* these are per-CPU */
+            break;
+        case 16 ... 31:
+            type = "PPI";
+            type_nr = irq - 16;
+            break;
+        default:
+            type = "SPI";
+            type_nr = irq - 32;
+            break;
+        }
+
+        wordreg = GICD[GICD_ISENABLER + irq / 32];
+        enable = !!(wordreg & (1u << (irq % 32)));
+        wordreg = GICD[GICD_ISPENDR + irq / 32];
+        pend = !!(wordreg & (1u << (irq % 32)));
+        wordreg = GICD[GICD_ISACTIVER + irq / 32];
+        active = !!(wordreg & (1u << (irq % 32)));
+
+        printk("IRQ%d %s%d: %c%c%c pri:%02x tgt:%02x ",
+               irq, type, type_nr,
+               enable ? ''e'' : ''-'',
+               pend   ? ''p'' : ''-'',
+               active ? ''a'' : ''-'',
+               priority, target);
+
+        if ( desc->status & IRQ_GUEST )
+        {
+            struct domain *d = desc->action->dev_id;
+            printk("dom%d %s", d->domain_id,
desc->action->name);
+        }
+        else
+        {
+            printk("Xen");
+        }
+        printk("\n");
+    }
+
+}
+
+static struct keyhandler dump_gic_keyhandler = {
+    .irq_callback = 0,
+    .u.fn = do_dump_gic,
+    .desc = "dump GIC state"
+};
+
 /* Set up the GIC */
 void __init gic_init(void)
 {
@@ -456,6 +528,9 @@ void __init gic_init(void)
     gic_hyp_init();
 
     spin_unlock(&gic.lock);
+
+    register_keyhandler(''G'', &dump_gic_keyhandler);
+
 }
 
 void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
-- 
1.7.10.4

Purely cosmetic, put a blank line after the early memory map to separate it
from the subsequent information. This looks better since hte memory map is
preceded by a blank line too.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/common/device_tree.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 44253da..943b181 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -452,6 +452,7 @@ static void __init early_print_info(void)
         early_printk(" RESVD[%d]: %"PRIpaddr" -
%"PRIpaddr"\n",
                      i, s, e);
     }
+    early_printk("\n");
 }
 
 /**
-- 
1.7.10.4

On the X-gene platform there are resources up this high which must be mapped
to dom0.

I''m becoming more convinced that p2m first level pages should be mapped
from
the xenheap so we can avoid all this faff with figuring out which page is
needed.

Remove the first level page from the p2m->pages list since it is actually two
pages and must be freed as such. Do so in p2m_teardown.

I''ve also punted on the implementation of dump_p2m_lookup for high
addresses...

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
This is a little bit RFC...
---
 xen/arch/arm/p2m.c |   60 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 82dda65..af32511 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -7,6 +7,10 @@
 #include <asm/flushtlb.h>
 #include <asm/gic.h>
 
+/* First level P2M is 2 consecutive pages */
+#define P2M_FIRST_ORDER 1
+#define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER)
+
 void dump_p2m_lookup(struct domain *d, paddr_t addr)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
@@ -14,6 +18,12 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
 
     printk("dom%d IPA 0x%"PRIpaddr"\n", d->domain_id,
addr);
 
+    if ( first_linear_offset(addr) > LPAE_ENTRIES )
+    {
+        printk("Cannot dump addresses in second of first level
pages...\n");
+        return;
+    }
+
     printk("P2M @ %p mfn:0x%lx\n",
            p2m->first_level, page_to_mfn(p2m->first_level));
 
@@ -31,6 +41,30 @@ void p2m_load_VTTBR(struct domain *d)
     isb(); /* Ensure update is visible */
 }
 
+static int p2m_first_level_index(paddr_t addr)
+{
+    /*
+     * 1st pages are concatenated so zeroeth offset gives us the
+     * index of the 1st page
+     */
+    return zeroeth_table_offset(addr);
+}
+
+/*
+ * Map whichever of the first pages contain addr. The caller should
+ * then use first_table_offset as an index.
+ */
+static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
+{
+    struct page_info *page;
+
+    BUG_ON(first_linear_offset(addr) > P2M_FIRST_ENTRIES);
+
+    page = p2m->first_level + p2m_first_level_index(addr);
+
+    return __map_domain_page(page);
+}
+
 /*
  * Lookup the MFN corresponding to a domain''s PFN.
  *
@@ -45,7 +79,7 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr)
 
     spin_lock(&p2m->lock);
 
-    first = __map_domain_page(p2m->first_level);
+    first = p2m_map_first(p2m, paddr);
 
     pte = first[first_table_offset(paddr)];
     if ( !pte.p2m.valid || !pte.p2m.table )
@@ -135,18 +169,21 @@ static int create_p2m_entries(struct domain *d,
     struct p2m_domain *p2m = &d->arch.p2m;
     lpae_t *first = NULL, *second = NULL, *third = NULL;
     paddr_t addr;
-    unsigned long cur_first_offset = ~0, cur_second_offset = ~0;
+    unsigned long cur_first_page = ~0,
+                  cur_first_offset = ~0,
+                  cur_second_offset = ~0;
 
     spin_lock(&p2m->lock);
 
-    /* XXX Don''t actually handle 40 bit guest physical addresses */
-    BUG_ON(start_gpaddr & 0x8000000000ULL);
-    BUG_ON(end_gpaddr   & 0x8000000000ULL);
-
-    first = __map_domain_page(p2m->first_level);
-
     for(addr = start_gpaddr; addr < end_gpaddr; addr += PAGE_SIZE)
     {
+        if ( cur_first_page != p2m_first_level_index(addr) )
+        {
+            if ( first ) unmap_domain_page(first);
+            first = p2m_map_first(p2m, addr);
+            cur_first_page = p2m_first_level_index(addr);
+        }
+
         if ( !first[first_table_offset(addr)].p2m.valid )
         {
             rc = p2m_create_table(d, &first[first_table_offset(addr)]);
@@ -279,15 +316,12 @@ int p2m_alloc_table(struct domain *d)
     struct page_info *page;
     void *p;
 
-    /* First level P2M is 2 consecutive pages */
-    page = alloc_domheap_pages(NULL, 1, 0);
+    page = alloc_domheap_pages(NULL, P2M_FIRST_ORDER, 0);
     if ( page == NULL )
         return -ENOMEM;
 
     spin_lock(&p2m->lock);
 
-    page_list_add(page, &p2m->pages);
-
     /* Clear both first level pages */
     p = __map_domain_page(page);
     clear_page(p);
@@ -380,6 +414,8 @@ void p2m_teardown(struct domain *d)
     while ( (pg = page_list_remove_head(&p2m->pages)) )
         free_domheap_page(pg);
 
+    free_domheap_pages(p2m->first_level, P2M_FIRST_ORDER);
+
     p2m->first_level = NULL;
 
     p2m_free_vmid(d);
-- 
1.7.10.4

Currently the hardcoded use of GUEST_EVTCHN_PPI is problematic if that is a
real PPI on the platform.

We really need to be smarter about selecting an unused PPI but in the meantime
we can at least give the platform code the option of hardcoding a number which
works for the platform.

Hardcode a suitable PPI on the Xgene platform.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
RFC: I''m not all that happy with this, but it seems better than nothing
for
4.4...

If other existing platforms also have this issue I can extend the patch if
told what a suitable PPI is.
---
 xen/arch/arm/domain.c                |    7 +++++--
 xen/arch/arm/platform.c              |    7 +++++++
 xen/arch/arm/platforms/xgene-storm.c |    1 +
 xen/include/asm-arm/platform.h       |    5 +++++
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2f57d01..52d2403 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -31,6 +31,7 @@
 #include <asm/processor-ca15.h>
 
 #include <asm/gic.h>
+#include <asm/platform.h>
 #include "vtimer.h"
 #include "vuart.h"
 
@@ -526,8 +527,10 @@ int arch_domain_create(struct domain *d, unsigned int
domcr_flags)
     if ( (rc = vcpu_domain_init(d)) != 0 )
         goto fail;
 
-    /* XXX dom0 needs more intelligent selection of PPI */
-    d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
+    if ( d->domain_id )
+        d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
+    else
+        d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
 
     /*
      * Virtual UART is only used by linux early printk and decompress code.
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 0fbbdc7..a7f9ee4 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -156,6 +156,13 @@ bool_t platform_device_is_blacklisted(const struct
dt_device_node *node)
     return dt_match_node(blacklist, node);
 }
 
+unsigned int platform_dom0_evtchn_ppi(void)
+{
+    if ( platform && platform->dom0_evtchn_ppi )
+        return platform->dom0_evtchn_ppi;
+    return GUEST_EVTCHN_PPI;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/platforms/xgene-storm.c
b/xen/arch/arm/platforms/xgene-storm.c
index 23986a9..b1b850b 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -52,6 +52,7 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
     .init = xgene_storm_init,
     .reset = xgene_storm_reset,
     .quirks = xgene_storm_quirks,
+    .dom0_evtchn_ppi = 24,
 PLATFORM_END
 
 /*
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index c282b30..92b954d 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -37,6 +37,10 @@ struct platform_desc {
      * List of devices which must not pass-through to a guest
      */
     const struct dt_device_match *blacklist_dev;
+    /*
+     * The IRQ (PPI) to use to inject event channels to dom0.
+     */
+    unsigned int dom0_evtchn_ppi;
 };
 
 /*
@@ -56,6 +60,7 @@ 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   \
-- 
1.7.10.4

FIXME: Should implement device tree extensions from
http://www.spinics.net/lists/devicetree/msg10473.html
especially
http://www.spinics.net/lists/devicetree/msg10478.html
---
 xen/arch/arm/gic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 185a6b8..0a8cf09 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -516,7 +516,7 @@ void __init gic_init(void)
     BUILD_BUG_ON(FIXMAP_ADDR(FIXMAP_GICC1) !                 
FIXMAP_ADDR(FIXMAP_GICC2)-PAGE_SIZE);
     set_fixmap(FIXMAP_GICC1, gic.cbase >> PAGE_SHIFT, DEV_SHARED);
-    set_fixmap(FIXMAP_GICC2, (gic.cbase >> PAGE_SHIFT) + 1, DEV_SHARED);
+    set_fixmap(FIXMAP_GICC2, (gic.cbase >> PAGE_SHIFT) + 0x10,
DEV_SHARED);
     set_fixmap(FIXMAP_GICH, gic.hbase >> PAGE_SHIFT, DEV_SHARED);
 
     /* Global settings: interrupt distributor */
-- 
1.7.10.4

The ranges property of a node with device_type = "pci" is defined in
ePAPR
2.3.8. Map the appropriate MMIO regions through to dom0.

This is a hack/PoC since it actually crashes for some reason. Hence it
contains a hacked in hardcoded list suitable for Xgene while I figure this
out.

This should also eventually handle the interrupt-map and (ePAPR 2.4.3.1) and
possibly dma-ranges (ePAPR 2.3.9) and msi-ranges (unspeciifed?) too.
---
 xen/arch/arm/domain_build.c |  103 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index aa7e3d2..e778c06 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -702,6 +702,69 @@ static int make_xen_node(const struct domain *d, void *fdt,
     return res;
 }
 
+static int map_pci_device_ranges(struct domain *d,
+                                 const struct dt_device_node *dev,
+                                 const struct dt_property *ranges)
+{
+    const __be32 *cells;
+
+    int size_cells, addr_cells, i, nr;
+
+    u32 pci_space;
+    u64 child_addr;
+    u64 host_addr;
+    u64 length;
+
+    printk("%s(%p, %p, %p)\n", __func__, d, dev, ranges);
+    printk("%s device %s\n", __func__, dt_node_full_name(dev));
+    return 0;
+
+    cells = ranges->value;
+    printk("%s ranges at %p, length %d\n", __func__, cells,
ranges->length);
+    size_cells = dt_n_size_cells(dev);
+    addr_cells = dt_n_addr_cells(dev);
+
+    /*
+     * Range is child address, host address (#address-cells), length
+     * (#size-cells),see ePAPR 2.3.8.
+     *
+     * PCI child address is u32 space + u64 address, see ePAPR 6.2.2.
+     *
+     */
+    nr = ranges->length / ( 3 + size_cells + addr_cells );
+    printk("PCI device %s: #address-cells %d, #size-cells %d. len %d,
entries %d\n",
+           dt_node_name(dev), addr_cells, size_cells, ranges->length, nr);
+
+    for ( i = 0; i < nr ; i++ )
+    {
+        pci_space = (u32)dt_next_cell(1, &cells);
+        child_addr = dt_next_cell(2, &cells);
+        host_addr = dt_next_cell(addr_cells, &cells);
+        length = dt_next_cell(size_cells, &cells);
+        printk("PCI SPACE 0x%08x, 0x%"PRIx64" maps to
0x%"PRIx64" size 0x%"PRIx64"\n",
+               pci_space, child_addr, host_addr, length);
+    }
+    return 0;
+}
+
+static int map_device_ranges(struct domain *d, const struct dt_device_node
*dev)
+{
+    const struct dt_property *ranges;
+    u32 len;
+
+    ranges = dt_get_property(dev, "ranges", &len);
+    /* No ranges, nothing to do */
+    if ( !ranges )
+        return 0;
+
+    if ( dt_device_type_is_equal(dev, "pci") )
+        return map_pci_device_ranges(d, dev, ranges);
+
+    printk("Cannot handle ranges for non-PCI device type %s\n",
dev->type);
+    /* Lets not worry for now... */
+    return 0;
+}
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, const struct dt_device_node *dev)
 {
@@ -767,6 +830,9 @@ static int map_device(struct domain *d, const struct
dt_device_node *dev)
         DPRINT("addr %u = 0x%"PRIx64" -
0x%"PRIx64"\n",
                i, addr, addr + size - 1);
 
+        if ( size == 0 )
+            continue;
+
         res = map_mmio_regions(d, addr & PAGE_MASK,
                                PAGE_ALIGN(addr + size) - 1,
                                addr & PAGE_MASK);
@@ -779,6 +845,8 @@ static int map_device(struct domain *d, const struct
dt_device_node *dev)
         }
     }
 
+    res = map_device_ranges(d, dev);
+
     return 0;
 }
 
@@ -903,6 +971,41 @@ static int prepare_dtb(struct domain *d, struct kernel_info
*kinfo)
     if ( ret )
         goto err;
 
+    {
+        struct dt_irq irq;
+
+        ret = map_mmio_regions(d,
+                               0xe000000000UL,
+                               0xe00fffffffUL,
+                               0xe000000000UL);
+        if (ret) printk("PCI REGION 0 failed\n");
+        ret = map_mmio_regions(d,
+                               0xe080000000UL,
+                               0xe08fffffffUL,
+                               0xe080000000UL);
+        if (ret) printk("PCI REGION 1 failed\n");
+        ret = map_mmio_regions(d,
+                               0xe010000000UL,
+                               0xe010000000UL,
+                               0xe01000ffffUL);
+        if (ret) printk("PCI REGION 2 failed\n");
+
+        irq.type = 0x4;
+
+        irq.irq = 0xc2 + 32;
+        ret = gic_route_irq_to_guest(d, &irq, "PCI#INTA");
+        if (ret) printk("PCI INTA failed\n");
+        irq.irq = 0xc3 + 32;
+        ret = gic_route_irq_to_guest(d, &irq, "PCI#INTB");
+        if (ret) printk("PCI INTB failed\n");
+        irq.irq = 0xc4 + 32;
+        ret = gic_route_irq_to_guest(d, &irq, "PCI#INTC");
+        if (ret) printk("PCI INTC failed\n");
+        irq.irq = 0xc5 + 32;
+        ret = gic_route_irq_to_guest(d, &irq, "PCI#INTD");
+        if (ret) printk("PCI INTD failed\n");
+    }
+
     ret = fdt_finish(kinfo->fdt);
     if ( ret < 0 )
         goto err;
-- 
1.7.10.4

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> 
> Extracted from "Basic Platform support for APM X-Gene Storm."
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> 
> Reworked into generic 8250 driver, use EARLY_UART_REG_SHIFT.
> 
> While there observe a missing shift in the arm32 version (UART_THR is zero
so
> it doesn''t really matter). Changed for consistency.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/Rules.mk             |    6 +++++
>  xen/arch/arm/arm64/debug-8250.inc |   52
+++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
>  create mode 100644 xen/arch/arm/arm64/debug-8250.inc
> 
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index c27c2eb..aaa203e 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -82,6 +82,12 @@ EARLY_PRINTK_INC := 8250
>  EARLY_UART_BASE_ADDRESS := 0xF0406B00
>  EARLY_UART_REG_SHIFT := 2
>  endif
> +ifeq ($(CONFIG_EARLY_PRINTK), xgene-storm)
> +EARLY_PRINTK_INC := 8250
> +EARLY_PRINTK_BAUD := 115200
> +EARLY_UART_BASE_ADDRESS := 0x1c020000
> +EARLY_UART_REG_SHIFT := 2
> +endif
>  
>  ifneq ($(EARLY_PRINTK_INC),)
>  EARLY_PRINTK := y
> diff --git a/xen/arch/arm/arm64/debug-8250.inc
b/xen/arch/arm/arm64/debug-8250.inc
> new file mode 100644
> index 0000000..5858354
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-8250.inc
> @@ -0,0 +1,52 @@
> +/*
> + * xen/arch/arm/arm64/debug-8250.inc
> + *
> + * 8250 specific debug code
> + *
> + * Copyright (c) 2013 Applied Micro.
> + *
> + * 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 <xen/8250-uart.h>
> +
> +/* UART initialization
> + * rb: register which contains the UART base address
> + * rc: scratch register 1
> + * rd: scratch register 2 */
> +.macro early_uart_init rb rc rd
> +.endm
You don''t need to define early_uart_init. This macro is only used if
EARLY_PRINTK_INIT_UART is defined, which it''s not the case here.

-- 
Julien Grall

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> 
> This patch adds initial platform stubs for APM X-Gene.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> 
> Drop earlyprintk (split into earlier patch). Only build on ARM64.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/platforms/Makefile      |    1 +
>  xen/arch/arm/platforms/xgene-storm.c |   57
++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
>  create mode 100644 xen/arch/arm/platforms/xgene-storm.c
> 
> diff --git a/xen/arch/arm/platforms/Makefile
b/xen/arch/arm/platforms/Makefile
> index f0dd72c..680364f 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_ARM_32) += exynos5.o
>  obj-$(CONFIG_ARM_32) += midway.o
>  obj-$(CONFIG_ARM_32) += omap5.o
>  obj-$(CONFIG_ARM_32) += sunxi.o
> +obj-$(CONFIG_ARM_64) += xgene-storm.o
> diff --git a/xen/arch/arm/platforms/xgene-storm.c
b/xen/arch/arm/platforms/xgene-storm.c
> new file mode 100644
> index 0000000..8e2b3b6
> --- /dev/null
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -0,0 +1,57 @@
> +/*
> + * xen/arch/arm/platforms/xgene-storm.c
> + *
> + * Applied Micro''s X-Gene specific settings
> + *
> + * Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> + * Anup Patel <apatel@apm.com>
> + * Copyright (c) 2013 Applied Micro.
> + *
> + * 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 <xen/config.h>
> +#include <xen/device_tree.h>
> +#include <xen/domain_page.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +#include <asm/platform.h>
> +#include <asm/early_printk.h>
> +
> +static void xgene_storm_reset(void)
> +{
> +}
> +
> +static int xgene_storm_init(void)
> +{
> +        return 0;
> +}
There is no need to create callbacks if they are empty.
Perhaps, this patch should be merge with #5 "Enable 1:1 Workaround
.."?

-- 
Julien Grall

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> 
> This patch updates the list of processor implementers with APM implementor
id.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
Acked-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/setup.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index cdcc2e7..49e1b5c 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -64,6 +64,7 @@ static const char * __initdata processor_implementers[] =
{
>      [''B''] = "Broadcom Corporation",
>      [''D''] = "Digital Equipment Corp",
>      [''M''] = "Motorola, Freescale Semiconductor
Inc.",
> +    [''P''] = "Applied Micro",
>      [''Q''] = "Qualcomm Inc.",
>      [''V''] = "Marvell Semiconductor Inc.",
>      [''i''] = "Intel Corporation",
> 

-- 
Julien Grall

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
> ---
>  config/arm64.mk |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/config/arm64.mk b/config/arm64.mk
> index 49055fa..15b57a4 100644
> --- a/config/arm64.mk
> +++ b/config/arm64.mk
> @@ -7,6 +7,7 @@ CONFIG_XEN_INSTALL_SUFFIX :>  CFLAGS += #-marm -march=
-mcpu= etc
>  
>  HAS_PL011 := y
> +HAS_NS16550 := y
>  
>  # Use only if calling $(LD) directly.
>  LDFLAGS_DIRECT += -EL
> 

-- 
Julien Grall

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> Helpful for diagnosis of bad console= parameters.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Thanks, I wanted to create a similar patch ! :)

I think it can be apply now without the other patches.

Acked-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/setup.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 49e1b5c..d252131 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -597,6 +597,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>  {
>      size_t fdt_size;
>      int cpus, i;
> +    const char *cmdline;
>  
>      setup_cache();
>  
> @@ -610,7 +611,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>          + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
>      fdt_size = device_tree_early_init(device_tree_flattened, fdt_paddr);
>  
> -    cmdline_parse(device_tree_bootargs(device_tree_flattened));
> +    cmdline = device_tree_bootargs(device_tree_flattened);
> +    early_printk("Command line: %s\n", cmdline);
> +    cmdline_parse(cmdline);
>  
>      setup_pagetables(boot_phys_offset, get_xen_paddr());
>      setup_mm(fdt_paddr, fdt_size);
> 

-- 
Julien Grall

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> The APM X-Gene Mustang board DTS has #address-cells = 2.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/smpboot.c |   25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 6c90fa6..ce832ae 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -125,18 +125,35 @@ void __init smp_init_cpus(void)
>  
>      dt_for_each_child_node( cpus, cpu )
>      {
> +        const __be32 *prop;
> +        u64 addr, size;
>          u32 hwid;
>  
>          if ( !dt_device_type_is_equal(cpu, "cpu") )
>              continue;
>  
> -        if ( !dt_property_read_u32(cpu, "reg", &hwid) )
> +        if ( dt_n_size_cells(cpu) != 0 )
> +            printk(XENLOG_WARNING "cpu node `%s`: #size-cells
%d\n",
> +                   dt_node_full_name(cpu), dt_n_size_cells(cpu));
> +
> +        prop = dt_get_property(cpu, "reg", NULL);
> +        if ( !prop )
>          {
> -            printk(XENLOG_WARNING "cpu node `%s`: missing reg
property\n",
> +            printk(XENLOG_WARNING "cpu node `%s`: has no reg
property\n",
>                     dt_node_full_name(cpu));
>              continue;
>          }
>  
> +        dt_get_range(&prop, cpu, &addr, &size);
You can use dt_read_number here.
> +
> +        hwid = addr;
> +        if ( hwid != addr )
> +        {
> +            printk(XENLOG_WARNING "cpu node `%s`: hwid overflow
%"PRIx64"\n",
> +                   dt_node_full_name(cpu), addr);
> +            continue;
> +        }
> +
>          /*
>           * 8 MSBs must be set to 0 in the DT since the reg property
>           * defines the MPIDR[23:0]
> @@ -159,8 +176,8 @@ void __init smp_init_cpus(void)
>              if ( tmp_map[j] == hwid )
>              {
>                  printk(XENLOG_WARNING
> -                       "cpu node `%s`: duplicate /cpu reg properties
in the DT\n",
> -                       dt_node_full_name(cpu));
> +                       "cpu node `%s`: duplicate /cpu reg properties
%"PRIx32" in the DT\n",
> +                       dt_node_full_name(cpu), hwid);
>                  break;
>              }
>          }
> 

-- 
Julien Grall

On Wed, 2013-11-20 at 16:31 +0000, Julien Grall wrote:
> On 11/20/2013 02:48 PM, Ian Campbell wrote:
> > The APM X-Gene Mustang board DTS has #address-cells = 2.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/smpboot.c |   25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index 6c90fa6..ce832ae 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -125,18 +125,35 @@ void __init smp_init_cpus(void)
> >  
> >      dt_for_each_child_node( cpus, cpu )
> >      {
> > +        const __be32 *prop;
> > +        u64 addr, size;
> >          u32 hwid;
> >  
> >          if ( !dt_device_type_is_equal(cpu, "cpu") )
> >              continue;
> >  
> > -        if ( !dt_property_read_u32(cpu, "reg", &hwid) )
> > +        if ( dt_n_size_cells(cpu) != 0 )
> > +            printk(XENLOG_WARNING "cpu node `%s`: #size-cells
%d\n",
> > +                   dt_node_full_name(cpu), dt_n_size_cells(cpu));
> > +
> > +        prop = dt_get_property(cpu, "reg", NULL);
> > +        if ( !prop )
> >          {
> > -            printk(XENLOG_WARNING "cpu node `%s`: missing reg
property\n",
> > +            printk(XENLOG_WARNING "cpu node `%s`: has no reg
property\n",
> >                     dt_node_full_name(cpu));
> >              continue;
> >          }
> >  
> > +        dt_get_range(&prop, cpu, &addr, &size);
> 
> You can use dt_read_number here.
As in
	dt_read_number(&prop, dt_n_addr_cells(cpu)
? Is there a helper which will take (&prop, cpu, &addr) and figure out
the size etc internally, that''s really what I was looking for.

Ian.

On 11/20/2013 04:37 PM, Ian Campbell wrote:
> On Wed, 2013-11-20 at 16:31 +0000, Julien Grall wrote:
>> On 11/20/2013 02:48 PM, Ian Campbell wrote:
>>> The APM X-Gene Mustang board DTS has #address-cells = 2.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>  xen/arch/arm/smpboot.c |   25 +++++++++++++++++++++----
>>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>>> index 6c90fa6..ce832ae 100644
>>> --- a/xen/arch/arm/smpboot.c
>>> +++ b/xen/arch/arm/smpboot.c
>>> @@ -125,18 +125,35 @@ void __init smp_init_cpus(void)
>>>  
>>>      dt_for_each_child_node( cpus, cpu )
>>>      {
>>> +        const __be32 *prop;
>>> +        u64 addr, size;
>>>          u32 hwid;
>>>  
>>>          if ( !dt_device_type_is_equal(cpu, "cpu") )
>>>              continue;
>>>  
>>> -        if ( !dt_property_read_u32(cpu, "reg",
&hwid) )
>>> +        if ( dt_n_size_cells(cpu) != 0 )
>>> +            printk(XENLOG_WARNING "cpu node `%s`: #size-cells
%d\n",
>>> +                   dt_node_full_name(cpu), dt_n_size_cells(cpu));
>>> +
>>> +        prop = dt_get_property(cpu, "reg", NULL);
>>> +        if ( !prop )
>>>          {
>>> -            printk(XENLOG_WARNING "cpu node `%s`: missing reg
property\n",
>>> +            printk(XENLOG_WARNING "cpu node `%s`: has no reg
property\n",
>>>                     dt_node_full_name(cpu));
>>>              continue;
>>>          }
>>>  
>>> +        dt_get_range(&prop, cpu, &addr, &size);
>>
>> You can use dt_read_number here.
> 
> As in
> 	dt_read_number(&prop, dt_n_addr_cells(cpu)
> ? Is there a helper which will take (&prop, cpu, &addr) and figure
out
> the size etc internally, that''s really what I was looking for.
Except dt_get_range, no. I would prefer the solution with dt_read_number
because it more clear that we want only a number. A cpu is not defined
by a range.

In the both case, you will also need to check the size of the property.
A bad crafted device tree can have reg smaller than #address-cells +
#size-cells.

-- 
Julien Grall

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> Purely cosmetic, put a blank line after the early memory map to separate it
> from the subsequent information. This looks better since hte memory map is
s/hte/the/

Except this minor typo:

Acked-by: Julien Grall <julien.grall@linaro.org>
> preceded by a blank line too.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/common/device_tree.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 44253da..943b181 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -452,6 +452,7 @@ static void __init early_print_info(void)
>          early_printk(" RESVD[%d]: %"PRIpaddr" -
%"PRIpaddr"\n",
>                       i, s, e);
>      }
> +    early_printk("\n");
>  }
>  
>  /**
> 

-- 
Julien Grall

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> If CONFIG_EARLY_PRINTK is in use then this gets all interleaved with the
> standard logging otherwise.
> 
Before the discussion IRL, it was not clear to me that you want to
directly "flush" the console buffer when prinkt is called. Can you
improve your commit message?
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/smpboot.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index ce832ae..9b58345 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -28,6 +28,7 @@
>  #include <xen/softirq.h>
>  #include <xen/timer.h>
>  #include <xen/irq.h>
> +#include <xen/console.h>
>  #include <asm/gic.h>
>  
>  cpumask_t cpu_online_map;
> @@ -349,6 +350,8 @@ int __cpu_up(unsigned int cpu)
>      if ( rc < 0 )
>          return rc;
>  
> +    console_start_sync(); /* Secondary may use early_printk */
> +
If it''s only for early printk, can you surround console_*_sync by
#ifdef
CONFIG_EARLY_PRINTK?
>      /* Tell the remote CPU which stack to boot on. */
>      init_data.stack = idle_vcpu[cpu]->arch.stack;
>  
> @@ -361,6 +364,8 @@ int __cpu_up(unsigned int cpu)
>  
>      rc = arch_cpu_up(cpu);
>  
> +    console_end_sync();
> +
>      if ( rc < 0 )
>      {
>          printk("Failed to bring up CPU%d\n", cpu);
> 
-- 
Julien Grall

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/gic.c |   75
++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ab49106..185a6b8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -28,6 +28,7 @@
>  #include <xen/softirq.h>
>  #include <xen/list.h>
>  #include <xen/device_tree.h>
> +#include <xen/keyhandler.h>
>  #include <asm/p2m.h>
>  #include <asm/domain.h>
>  
> @@ -385,6 +386,77 @@ int gic_irq_xlate(const u32 *intspec, unsigned int
intsize,
>      return 0;
>  }
>  
> +
> +static void do_dump_gic(unsigned char key)
> +{
> +    int irq;
> +    printk("''%c'' pressed -> dumping GIC
state\n", key);
> +
> +    for ( irq = 0; irq < gic.lines; irq++ )
> +    {
> +        const char *type;
> +        int type_nr, enable, pend, active, priority, target;
> +        struct irq_desc *desc = irq_to_desc(irq);
> +        uint8_t *bytereg;
> +        uint32_t wordreg;
> +
> +        bytereg = (uint8_t *) (GICD + GICD_ITARGETSR);
> +        target = bytereg[irq];
> +
> +        bytereg = (uint8_t *) (GICD + GICD_IPRIORITYR);
> +        priority = bytereg[irq];
> +
> +        switch ( irq )
> +        {
> +        case 0 ... 15:
> +            type = "SGI";
> +            type_nr = irq;
> +            target = 0x00; /* these are per-CPU */
> +            break;
> +        case 16 ... 31:
> +            type = "PPI";
> +            type_nr = irq - 16;
> +            break;
I think it''s a bit stupid to print SGI and PPI as it''s per-CPU
interrupt. With your solution, you don''t know which CPU call the
keyhandler.

Perhaps, you need to an SGI to each CPU?

-- 
Julien Grall

On Wed, 2013-11-20 at 17:31 +0000, Julien Grall wrote:
> On 11/20/2013 02:48 PM, Ian Campbell wrote:
> > If CONFIG_EARLY_PRINTK is in use then this gets all interleaved with
the
> > standard logging otherwise.
> > 
> 
> Before the discussion IRL, it was not clear to me that you want to
> directly "flush" the console buffer when prinkt is called. Can
you
> improve your commit message?
Sure. How about:

        xen: arm: enable synchronous console while starting secondary
        CPUs
        
        Setting synchronous console ensures that any printk hits the
        buffer immediately and that any outstanding queued log messages
        are flushed. This ensures that such log messages are not being
        printed while the secondary CPU may be using early_printk during
        early bringup.
        
        Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

> > ---
> >  xen/arch/arm/smpboot.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index ce832ae..9b58345 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -28,6 +28,7 @@
> >  #include <xen/softirq.h>
> >  #include <xen/timer.h>
> >  #include <xen/irq.h>
> > +#include <xen/console.h>
> >  #include <asm/gic.h>
> >  
> >  cpumask_t cpu_online_map;
> > @@ -349,6 +350,8 @@ int __cpu_up(unsigned int cpu)
> >      if ( rc < 0 )
> >          return rc;
> >  
> > +    console_start_sync(); /* Secondary may use early_printk */
> > +
> 
> If it''s only for early printk, can you surround console_*_sync by
#ifdef
> CONFIG_EARLY_PRINTK?
> 
> >      /* Tell the remote CPU which stack to boot on. */
> >      init_data.stack = idle_vcpu[cpu]->arch.stack;
> >  
> > @@ -361,6 +364,8 @@ int __cpu_up(unsigned int cpu)
> >  
> >      rc = arch_cpu_up(cpu);
> >  
> > +    console_end_sync();
> > +
> >      if ( rc < 0 )
> >      {
> >          printk("Failed to bring up CPU%d\n", cpu);
> > 
>

On Wed, 2013-11-20 at 17:36 +0000, Julien Grall wrote:
> On 11/20/2013 02:48 PM, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/gic.c |   75
++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index ab49106..185a6b8 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -28,6 +28,7 @@
> >  #include <xen/softirq.h>
> >  #include <xen/list.h>
> >  #include <xen/device_tree.h>
> > +#include <xen/keyhandler.h>
> >  #include <asm/p2m.h>
> >  #include <asm/domain.h>
> >  
> > @@ -385,6 +386,77 @@ int gic_irq_xlate(const u32 *intspec, unsigned
int intsize,
> >      return 0;
> >  }
> >  
> > +
> > +static void do_dump_gic(unsigned char key)
> > +{
> > +    int irq;
> > +    printk("''%c'' pressed -> dumping GIC
state\n", key);
> > +
> > +    for ( irq = 0; irq < gic.lines; irq++ )
> > +    {
> > +        const char *type;
> > +        int type_nr, enable, pend, active, priority, target;
> > +        struct irq_desc *desc = irq_to_desc(irq);
> > +        uint8_t *bytereg;
> > +        uint32_t wordreg;
> > +
> > +        bytereg = (uint8_t *) (GICD + GICD_ITARGETSR);
> > +        target = bytereg[irq];
> > +
> > +        bytereg = (uint8_t *) (GICD + GICD_IPRIORITYR);
> > +        priority = bytereg[irq];
> > +
> > +        switch ( irq )
> > +        {
> > +        case 0 ... 15:
> > +            type = "SGI";
> > +            type_nr = irq;
> > +            target = 0x00; /* these are per-CPU */
> > +            break;
> > +        case 16 ... 31:
> > +            type = "PPI";
> > +            type_nr = irq - 16;
> > +            break;
> 
> I think it''s a bit stupid to print SGI and PPI as it''s
per-CPU
> interrupt. With your solution, you don''t know which CPU call the
keyhandler.
Most of the things I was interested don''t vary across the CPUs,
specifically who is receiving the interrupt (dom0 or Xen) and the
priority (which is either static or consistent across the CPUs by
design). The enable/pend/act flags are a bit bogus but that didn''t
really matter to me at the time.
> Perhaps, you need to an SGI to each CPU?
I suppose I could but I didn''t need this for my debugging so I
didn''t
implement it. I''d rather leave this until there is a need for that
functionality.

I don''t much care if this patch goes in as is or not but I
don''t want to
spend lots more time on it.

Ian.

On Wed, 20 Nov 2013, Ian Campbell wrote:
> Helpful for diagnosis of bad console= parameters.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  xen/arch/arm/setup.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 49e1b5c..d252131 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -597,6 +597,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>  {
>      size_t fdt_size;
>      int cpus, i;
> +    const char *cmdline;
>  
>      setup_cache();
>  
> @@ -610,7 +611,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>          + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
>      fdt_size = device_tree_early_init(device_tree_flattened, fdt_paddr);
>  
> -    cmdline_parse(device_tree_bootargs(device_tree_flattened));
> +    cmdline = device_tree_bootargs(device_tree_flattened);
> +    early_printk("Command line: %s\n", cmdline);
> +    cmdline_parse(cmdline);
>  
>      setup_pagetables(boot_phys_offset, get_xen_paddr());
>      setup_mm(fdt_paddr, fdt_size);
> -- 
> 1.7.10.4
>

On Wed, 20 Nov 2013, Ian Campbell wrote:
> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> 
> This patch adds initial platform stubs for APM X-Gene.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> 
> Drop earlyprintk (split into earlier patch). Only build on ARM64.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  xen/arch/arm/platforms/Makefile      |    1 +
>  xen/arch/arm/platforms/xgene-storm.c |   57
++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
>  create mode 100644 xen/arch/arm/platforms/xgene-storm.c
> 
> diff --git a/xen/arch/arm/platforms/Makefile
b/xen/arch/arm/platforms/Makefile
> index f0dd72c..680364f 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_ARM_32) += exynos5.o
>  obj-$(CONFIG_ARM_32) += midway.o
>  obj-$(CONFIG_ARM_32) += omap5.o
>  obj-$(CONFIG_ARM_32) += sunxi.o
> +obj-$(CONFIG_ARM_64) += xgene-storm.o
> diff --git a/xen/arch/arm/platforms/xgene-storm.c
b/xen/arch/arm/platforms/xgene-storm.c
> new file mode 100644
> index 0000000..8e2b3b6
> --- /dev/null
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -0,0 +1,57 @@
> +/*
> + * xen/arch/arm/platforms/xgene-storm.c
> + *
> + * Applied Micro''s X-Gene specific settings
> + *
> + * Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> + * Anup Patel <apatel@apm.com>
> + * Copyright (c) 2013 Applied Micro.
> + *
> + * 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 <xen/config.h>
> +#include <xen/device_tree.h>
> +#include <xen/domain_page.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +#include <asm/platform.h>
> +#include <asm/early_printk.h>
> +
> +static void xgene_storm_reset(void)
> +{
> +}
> +
> +static int xgene_storm_init(void)
> +{
> +        return 0;
> +}
> +
> +static const char const *xgene_storm_dt_compat[] __initdata > +{
> +    "apm,xgene-storm",
> +    NULL
> +};
> +
> +PLATFORM_START(xgene_storm, "APM X-GENE STORM")
> +    .compatible = xgene_storm_dt_compat,
> +    .init = xgene_storm_init,
> +    .reset = xgene_storm_reset,
> +PLATFORM_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 1.7.10.4
>

On Wed, 20 Nov 2013, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  config/arm64.mk |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/config/arm64.mk b/config/arm64.mk
> index 49055fa..15b57a4 100644
> --- a/config/arm64.mk
> +++ b/config/arm64.mk
> @@ -7,6 +7,7 @@ CONFIG_XEN_INSTALL_SUFFIX :>  CFLAGS += #-marm -march=
-mcpu= etc
>  
>  HAS_PL011 := y
> +HAS_NS16550 := y
>  
>  # Use only if calling $(LD) directly.
>  LDFLAGS_DIRECT += -EL
> -- 
> 1.7.10.4
>

On Wed, 20 Nov 2013, Ian Campbell wrote:
> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> 
> This patch updates the list of processor implementers with APM implementor
id.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  xen/arch/arm/setup.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index cdcc2e7..49e1b5c 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -64,6 +64,7 @@ static const char * __initdata processor_implementers[] =
{
>      [''B''] = "Broadcom Corporation",
>      [''D''] = "Digital Equipment Corp",
>      [''M''] = "Motorola, Freescale Semiconductor
Inc.",
> +    [''P''] = "Applied Micro",
>      [''Q''] = "Qualcomm Inc.",
>      [''V''] = "Marvell Semiconductor Inc.",
>      [''i''] = "Intel Corporation",
> -- 
> 1.7.10.4
>

On Wed, 20 Nov 2013, Ian Campbell wrote:
> There is no SMMU on this platform.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  xen/arch/arm/platforms/xgene-storm.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/platforms/xgene-storm.c
b/xen/arch/arm/platforms/xgene-storm.c
> index 8e2b3b6..23986a9 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -35,6 +35,12 @@ static int xgene_storm_init(void)
>          return 0;
>  }
>  
> +static uint32_t xgene_storm_quirks(void)
> +{
> +    return PLATFORM_QUIRK_DOM0_MAPPING_11;
> +}
> +
> +
>  static const char const *xgene_storm_dt_compat[] __initdata >  {
>      "apm,xgene-storm",
> @@ -45,6 +51,7 @@ 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
>  
>  /*
> -- 
> 1.7.10.4
>

On Wed, 20 Nov 2013, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/gic.c |   75
++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ab49106..185a6b8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -28,6 +28,7 @@
>  #include <xen/softirq.h>
>  #include <xen/list.h>
>  #include <xen/device_tree.h>
> +#include <xen/keyhandler.h>
>  #include <asm/p2m.h>
>  #include <asm/domain.h>
>  
> @@ -385,6 +386,77 @@ int gic_irq_xlate(const u32 *intspec, unsigned int
intsize,
>      return 0;
>  }
>  
> +
> +static void do_dump_gic(unsigned char key)
It might be worth renaming the current gic_dump_info to
gic_dump_info_guest to avoid confusions.

> +    int irq;
> +    printk("''%c'' pressed -> dumping GIC
state\n", key);
> +
> +    for ( irq = 0; irq < gic.lines; irq++ )
> +    {
> +        const char *type;
> +        int type_nr, enable, pend, active, priority, target;
> +        struct irq_desc *desc = irq_to_desc(irq);
> +        uint8_t *bytereg;
> +        uint32_t wordreg;
> +
> +        bytereg = (uint8_t *) (GICD + GICD_ITARGETSR);
> +        target = bytereg[irq];
> +
> +        bytereg = (uint8_t *) (GICD + GICD_IPRIORITYR);
> +        priority = bytereg[irq];
> +
> +        switch ( irq )
> +        {
> +        case 0 ... 15:
> +            type = "SGI";
> +            type_nr = irq;
> +            target = 0x00; /* these are per-CPU */
> +            break;
> +        case 16 ... 31:
> +            type = "PPI";
> +            type_nr = irq - 16;
> +            break;
> +        default:
> +            type = "SPI";
> +            type_nr = irq - 32;
> +            break;
> +        }
> +
> +        wordreg = GICD[GICD_ISENABLER + irq / 32];
> +        enable = !!(wordreg & (1u << (irq % 32)));
> +        wordreg = GICD[GICD_ISPENDR + irq / 32];
> +        pend = !!(wordreg & (1u << (irq % 32)));
> +        wordreg = GICD[GICD_ISACTIVER + irq / 32];
> +        active = !!(wordreg & (1u << (irq % 32)));
> +
> +        printk("IRQ%d %s%d: %c%c%c pri:%02x tgt:%02x ",
> +               irq, type, type_nr,
> +               enable ? ''e'' : ''-'',
> +               pend   ? ''p'' : ''-'',
> +               active ? ''a'' : ''-'',
> +               priority, target);
> +
> +        if ( desc->status & IRQ_GUEST )
> +        {
> +            struct domain *d = desc->action->dev_id;
> +            printk("dom%d %s", d->domain_id,
desc->action->name);
> +        }
> +        else
> +        {
> +            printk("Xen");
> +        }
> +        printk("\n");
> +    }
> +
> +}
> +
> +static struct keyhandler dump_gic_keyhandler = {
> +    .irq_callback = 0,
> +    .u.fn = do_dump_gic,
> +    .desc = "dump GIC state"
> +};
> +
>  /* Set up the GIC */
>  void __init gic_init(void)
>  {
> @@ -456,6 +528,9 @@ void __init gic_init(void)
>      gic_hyp_init();
>  
>      spin_unlock(&gic.lock);
> +
> +    register_keyhandler(''G'', &dump_gic_keyhandler);
> +
>  }
>  
>  void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
> -- 
> 1.7.10.4
>

On Wed, 20 Nov 2013, Ian Campbell wrote:
> If CONFIG_EARLY_PRINTK is in use then this gets all interleaved with the
> standard logging otherwise.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
I am concerned because in this case console_start_sync and
console_end_sync would be called before console_endboot and I can''t
tell
whether it is a supported.

>  xen/arch/arm/smpboot.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index ce832ae..9b58345 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -28,6 +28,7 @@
>  #include <xen/softirq.h>
>  #include <xen/timer.h>
>  #include <xen/irq.h>
> +#include <xen/console.h>
>  #include <asm/gic.h>
>  
>  cpumask_t cpu_online_map;
> @@ -349,6 +350,8 @@ int __cpu_up(unsigned int cpu)
>      if ( rc < 0 )
>          return rc;
>  
> +    console_start_sync(); /* Secondary may use early_printk */
> +
>      /* Tell the remote CPU which stack to boot on. */
>      init_data.stack = idle_vcpu[cpu]->arch.stack;
>  
> @@ -361,6 +364,8 @@ int __cpu_up(unsigned int cpu)
>  
>      rc = arch_cpu_up(cpu);
>  
> +    console_end_sync();
> +
>      if ( rc < 0 )
>      {
>          printk("Failed to bring up CPU%d\n", cpu);
> -- 
> 1.7.10.4
>

On Wed, 20 Nov 2013, Ian Campbell wrote:
> Otherwise the results of the shifting can be undefined and/or sign
extended.
> 
> Pointed out in the context of HCR_* by Pranavkumar Sawargaonkar.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
Do you realize that by using U instead of UL, it would be 4 bytes even
on ARMv8? Is that expected? If so maybe adding a line to the commit
message would be good.

>  xen/include/asm-arm/processor.h |  144
+++++++++++++++++++--------------------
>  1 file changed, 72 insertions(+), 72 deletions(-)
> 
> diff --git a/xen/include/asm-arm/processor.h
b/xen/include/asm-arm/processor.h
> index 3da3a3d..7cd2d3e 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -18,71 +18,71 @@
>  #define MPIDR_INVALID       (~MPIDR_HWID_MASK)
>  
>  /* TTBCR Translation Table Base Control Register */
> -#define TTBCR_EAE    0x80000000
> -#define TTBCR_N_MASK 0x07
> -#define TTBCR_N_16KB 0x00
> -#define TTBCR_N_8KB  0x01
> -#define TTBCR_N_4KB  0x02
> -#define TTBCR_N_2KB  0x03
> -#define TTBCR_N_1KB  0x04
> +#define TTBCR_EAE    _AC(0x80000000,U)
> +#define TTBCR_N_MASK _AC(0x07,U)
> +#define TTBCR_N_16KB _AC(0x00,U)
> +#define TTBCR_N_8KB  _AC(0x01,U)
> +#define TTBCR_N_4KB  _AC(0x02,U)
> +#define TTBCR_N_2KB  _AC(0x03,U)
> +#define TTBCR_N_1KB  _AC(0x04,U)
>  
>  /* SCTLR System Control Register. */
>  /* HSCTLR is a subset of this. */
> -#define SCTLR_TE        (1<<30)
> -#define SCTLR_AFE       (1<<29)
> -#define SCTLR_TRE       (1<<28)
> -#define SCTLR_NMFI      (1<<27)
> -#define SCTLR_EE        (1<<25)
> -#define SCTLR_VE        (1<<24)
> -#define SCTLR_U         (1<<22)
> -#define SCTLR_FI        (1<<21)
> -#define SCTLR_WXN       (1<<19)
> -#define SCTLR_HA        (1<<17)
> -#define SCTLR_RR        (1<<14)
> -#define SCTLR_V         (1<<13)
> -#define SCTLR_I         (1<<12)
> -#define SCTLR_Z         (1<<11)
> -#define SCTLR_SW        (1<<10)
> -#define SCTLR_B         (1<<7)
> -#define SCTLR_C         (1<<2)
> -#define SCTLR_A         (1<<1)
> -#define SCTLR_M         (1<<0)
> -
> -#define HSCTLR_BASE       0x30c51878
> +#define SCTLR_TE        (_AC(1,U)<<30)
> +#define SCTLR_AFE       (_AC(1,U)<<29)
> +#define SCTLR_TRE       (_AC(1,U)<<28)
> +#define SCTLR_NMFI      (_AC(1,U)<<27)
> +#define SCTLR_EE        (_AC(1,U)<<25)
> +#define SCTLR_VE        (_AC(1,U)<<24)
> +#define SCTLR_U         (_AC(1,U)<<22)
> +#define SCTLR_FI        (_AC(1,U)<<21)
> +#define SCTLR_WXN       (_AC(1,U)<<19)
> +#define SCTLR_HA        (_AC(1,U)<<17)
> +#define SCTLR_RR        (_AC(1,U)<<14)
> +#define SCTLR_V         (_AC(1,U)<<13)
> +#define SCTLR_I         (_AC(1,U)<<12)
> +#define SCTLR_Z         (_AC(1,U)<<11)
> +#define SCTLR_SW        (_AC(1,U)<<10)
> +#define SCTLR_B         (_AC(1,U)<<7)
> +#define SCTLR_C         (_AC(1,U)<<2)
> +#define SCTLR_A         (_AC(1,U)<<1)
> +#define SCTLR_M         (_AC(1,U)<<0)
> +
> +#define HSCTLR_BASE     _AC(0x30c51878,U)
>  
>  /* HCR Hyp Configuration Register */
> -#define HCR_RW          (1<<31) /* Register Width, ARM64 only */
> -#define HCR_TGE         (1<<27) /* Trap General Exceptions */
> -#define HCR_TVM         (1<<26) /* Trap Virtual Memory Controls */
> -#define HCR_TTLB        (1<<25) /* Trap TLB Maintenance Operations
*/
> -#define HCR_TPU         (1<<24) /* Trap Cache Maintenance Operations
to PoU */
> -#define HCR_TPC         (1<<23) /* Trap Cache Maintenance Operations
to PoC */
> -#define HCR_TSW         (1<<22) /* Trap Set/Way Cache Maintenance
Operations */
> -#define HCR_TAC         (1<<21) /* Trap ACTLR Accesses */
> -#define HCR_TIDCP       (1<<20) /* Trap lockdown */
> -#define HCR_TSC         (1<<19) /* Trap SMC instruction */
> -#define HCR_TID3        (1<<18) /* Trap ID Register Group 3 */
> -#define HCR_TID2        (1<<17) /* Trap ID Register Group 2 */
> -#define HCR_TID1        (1<<16) /* Trap ID Register Group 1 */
> -#define HCR_TID0        (1<<15) /* Trap ID Register Group 0 */
> -#define HCR_TWE         (1<<14) /* Trap WFE instruction */
> -#define HCR_TWI         (1<<13) /* Trap WFI instruction */
> -#define HCR_DC          (1<<12) /* Default cacheable */
> -#define HCR_BSU_MASK    (3<<10) /* Barrier Shareability Upgrade */
> -#define HCR_BSU_NONE     (0<<10)
> -#define HCR_BSU_INNER    (1<<10)
> -#define HCR_BSU_OUTER    (2<<10)
> -#define HCR_BSU_FULL     (3<<10)
> -#define HCR_FB          (1<<9) /* Force Broadcast of Cache/BP/TLB
operations */
> -#define HCR_VA          (1<<8) /* Virtual Asynchronous Abort */
> -#define HCR_VI          (1<<7) /* Virtual IRQ */
> -#define HCR_VF          (1<<6) /* Virtual FIQ */
> -#define HCR_AMO         (1<<5) /* Override CPSR.A */
> -#define HCR_IMO         (1<<4) /* Override CPSR.I */
> -#define HCR_FMO         (1<<3) /* Override CPSR.F */
> -#define HCR_PTW         (1<<2) /* Protected Walk */
> -#define HCR_SWIO        (1<<1) /* Set/Way Invalidation Override */
> -#define HCR_VM          (1<<0) /* Virtual MMU Enable */
> +#define HCR_RW          (_AC(1,U)<<31) /* Register Width, ARM64 only
*/
> +#define HCR_TGE         (_AC(1,U)<<27) /* Trap General Exceptions */
> +#define HCR_TVM         (_AC(1,U)<<26) /* Trap Virtual Memory
Controls */
> +#define HCR_TTLB        (_AC(1,U)<<25) /* Trap TLB Maintenance
Operations */
> +#define HCR_TPU         (_AC(1,U)<<24) /* Trap Cache Maintenance
Operations to PoU */
> +#define HCR_TPC         (_AC(1,U)<<23) /* Trap Cache Maintenance
Operations to PoC */
> +#define HCR_TSW         (_AC(1,U)<<22) /* Trap Set/Way Cache
Maintenance Operations */
> +#define HCR_TAC         (_AC(1,U)<<21) /* Trap ACTLR Accesses */
> +#define HCR_TIDCP       (_AC(1,U)<<20) /* Trap lockdown */
> +#define HCR_TSC         (_AC(1,U)<<19) /* Trap SMC instruction */
> +#define HCR_TID3        (_AC(1,U)<<18) /* Trap ID Register Group 3
*/
> +#define HCR_TID2        (_AC(1,U)<<17) /* Trap ID Register Group 2
*/
> +#define HCR_TID1        (_AC(1,U)<<16) /* Trap ID Register Group 1
*/
> +#define HCR_TID0        (_AC(1,U)<<15) /* Trap ID Register Group 0
*/
> +#define HCR_TWE         (_AC(1,U)<<14) /* Trap WFE instruction */
> +#define HCR_TWI         (_AC(1,U)<<13) /* Trap WFI instruction */
> +#define HCR_DC          (_AC(1,U)<<12) /* Default cacheable */
> +#define HCR_BSU_MASK    (_AC(3,U)<<10) /* Barrier Shareability
Upgrade */
> +#define HCR_BSU_NONE     (_AC(0,U)<<10)
> +#define HCR_BSU_INNER    (_AC(1,U)<<10)
> +#define HCR_BSU_OUTER    (_AC(2,U)<<10)
> +#define HCR_BSU_FULL     (_AC(3,U)<<10)
> +#define HCR_FB          (_AC(1,U)<<9) /* Force Broadcast of
Cache/BP/TLB operations */
> +#define HCR_VA          (_AC(1,U)<<8) /* Virtual Asynchronous Abort
*/
> +#define HCR_VI          (_AC(1,U)<<7) /* Virtual IRQ */
> +#define HCR_VF          (_AC(1,U)<<6) /* Virtual FIQ */
> +#define HCR_AMO         (_AC(1,U)<<5) /* Override CPSR.A */
> +#define HCR_IMO         (_AC(1,U)<<4) /* Override CPSR.I */
> +#define HCR_FMO         (_AC(1,U)<<3) /* Override CPSR.F */
> +#define HCR_PTW         (_AC(1,U)<<2) /* Protected Walk */
> +#define HCR_SWIO        (_AC(1,U)<<1) /* Set/Way Invalidation
Override */
> +#define HCR_VM          (_AC(1,U)<<0) /* Virtual MMU Enable */
>  
>  #define HSR_EC_UNKNOWN              0x00
>  #define HSR_EC_WFI_WFE              0x01
> @@ -346,20 +346,20 @@ union hsr {
>                                HSR_SYSREG_OP2_MASK)
>  
>  /* Physical Address Register */
> -#define PAR_F           (1<<0)
> +#define PAR_F           (_AC(1,U)<<0)
>  
>  /* .... If F == 1 */
>  #define PAR_FSC_SHIFT   (1)
> -#define PAR_FSC_MASK    (0x3f<<PAR_FSC_SHIFT)
> -#define PAR_STAGE21     (1<<8)     /* Stage 2 Fault During Stage 1
Walk */
> -#define PAR_STAGE2      (1<<9)     /* Stage 2 Fault */
> +#define PAR_FSC_MASK    (_AC(0x3f,U)<<PAR_FSC_SHIFT)
> +#define PAR_STAGE21     (_AC(1,U)<<8)     /* Stage 2 Fault During
Stage 1 Walk */
> +#define PAR_STAGE2      (_AC(1,U)<<9)     /* Stage 2 Fault */
>  
>  /* If F == 0 */
>  #define PAR_MAIR_SHIFT  56                       /* Memory Attributes */
>  #define PAR_MAIR_MASK   (0xffLL<<PAR_MAIR_SHIFT)
> -#define PAR_NS          (1<<9)                   /* Non-Secure */
> +#define PAR_NS          (_AC(1,U)<<9)                   /*
Non-Secure */
>  #define PAR_SH_SHIFT    7                        /* Shareability */
> -#define PAR_SH_MASK     (3<<PAR_SH_SHIFT)
> +#define PAR_SH_MASK     (_AC(3,U)<<PAR_SH_SHIFT)
>  
>  /* Fault Status Register */
>  /*
> @@ -372,11 +372,11 @@ union hsr {
>   * 10xxxx -- Other
>   * 11xxxx -- Implementation Defined
>   */
> -#define FSC_TYPE_MASK (0x3<<4)
> -#define FSC_TYPE_FAULT (0x00<<4)
> -#define FSC_TYPE_ABT   (0x01<<4)
> -#define FSC_TYPE_OTH   (0x02<<4)
> -#define FSC_TYPE_IMPL  (0x03<<4)
> +#define FSC_TYPE_MASK (_AC(0x3,U)<<4)
> +#define FSC_TYPE_FAULT (_AC(0x00,U)<<4)
> +#define FSC_TYPE_ABT   (_AC(0x01,U)<<4)
> +#define FSC_TYPE_OTH   (_AC(0x02,U)<<4)
> +#define FSC_TYPE_IMPL  (_AC(0x03,U)<<4)
>  
>  #define FSC_FLT_TRANS  (0x04)
>  #define FSC_FLT_ACCESS (0x08)
> @@ -391,7 +391,7 @@ union hsr {
>  #define FSC_LKD        (0x34) /* Lockdown Abort */
>  #define FSC_CPR        (0x3a) /* Coprocossor Abort */
>  
> -#define FSC_LL_MASK    (0x03<<0)
> +#define FSC_LL_MASK    (_AC(0x03,U)<<0)
>  
>  /* Time counter hypervisor control register */
>  #define CNTHCTL_PA      (1u<<0)  /* Kernel/user access to physical
counter */
> -- 
> 1.7.10.4
>

On Wed, 20 Nov 2013, Ian Campbell wrote:
> In case it is outside visible ram.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  xen/arch/arm/arm64/smpboot.c |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index 8239590..8696ed6 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c
> @@ -4,6 +4,8 @@
>  #include <xen/errno.h>
>  #include <xen/mm.h>
>  #include <xen/smp.h>
> +#include <xen/vmap.h>
> +#include <asm/io.h>
>  
>  struct smp_enable_ops {
>          int             (*prepare_cpu)(int);
> @@ -14,7 +16,7 @@ static struct smp_enable_ops smp_enable_ops[NR_CPUS];
>  
>  static int __init smp_spin_table_cpu_up(int cpu)
>  {
> -    paddr_t *release;
> +    paddr_t __iomem *release;
>  
>      if (!cpu_release_addr[cpu])
>      {
> @@ -22,12 +24,20 @@ static int __init smp_spin_table_cpu_up(int cpu)
>          return -ENODEV;
>      }
>  
> -    release = __va(cpu_release_addr[cpu]);
> +    release = ioremap_nocache(cpu_release_addr[cpu], 8);
> +    if ( !release )
> +    {
> +        dprintk(XENLOG_ERR, "CPU%d: Unable to map release
address\n", cpu);
> +        return -EFAULT;
> +    }
>  
>      release[0] = __pa(init_secondary);
>      flush_xen_data_tlb_range_va((vaddr_t)release, sizeof(*release));
>  
> +    iounmap(release);
> +
>      sev();
> +
>      return 0;
>  }
>  
> -- 
> 1.7.10.4
>

On Wed, 2013-11-20 at 19:10 +0000, Stefano Stabellini
wrote:
> On Wed, 20 Nov 2013, Ian Campbell wrote:
> > There is no SMMU on this platform.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Thanks. I ended up merging this into "Add Basic Platform support for APM
X-Gene Storm." for the (not yet posted) v2. Since you acked both I have
added your ack to the result, hope that''s ok.

Ian.

On Wed, 2013-11-20 at 19:29 +0000, Stefano Stabellini
wrote:
> On Wed, 20 Nov 2013, Ian Campbell wrote:
> > Otherwise the results of the shifting can be undefined and/or sign
extended.
> > 
> > Pointed out in the context of HCR_* by Pranavkumar Sawargaonkar.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> 
> Do you realize that by using U instead of UL, it would be 4 bytes even
> on ARMv8? Is that expected? If so maybe adding a line to the commit
> message would be good.
Hrm yes. For most of the registers that is expected/desired since the
actual register is 32-bits, but for HCR in particular it should be 32
bits on 32-bit and 64 bits on 64-bits, which might be tricky to arrange
with these macros. I''ll see what I can do. We only actually use bits
0..31 since bits 32+33 aren''t useful to us (at least not now) so I
might
end up fudging it somehow...

On Wed, 2013-11-20 at 19:22 +0000, Stefano Stabellini
wrote:
> On Wed, 20 Nov 2013, Ian Campbell wrote:
> > If CONFIG_EARLY_PRINTK is in use then this gets all interleaved with
the
> > standard logging otherwise.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I am concerned because in this case console_start_sync and
> console_end_sync would be called before console_endboot and I
can''t tell
> whether it is a supported.
I believe it is. Or at least I can''t see anything which would prevent
it
and it works in practice. The synchronousness of the console is
reference counted so it doesn''t interfere with e..g the sync_console
option etc.

Keir CCd in case he has an opinion...
> 
> 
> >  xen/arch/arm/smpboot.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index ce832ae..9b58345 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -28,6 +28,7 @@
> >  #include <xen/softirq.h>
> >  #include <xen/timer.h>
> >  #include <xen/irq.h>
> > +#include <xen/console.h>
> >  #include <asm/gic.h>
> >  
> >  cpumask_t cpu_online_map;
> > @@ -349,6 +350,8 @@ int __cpu_up(unsigned int cpu)
> >      if ( rc < 0 )
> >          return rc;
> >  
> > +    console_start_sync(); /* Secondary may use early_printk */
> > +
> >      /* Tell the remote CPU which stack to boot on. */
> >      init_data.stack = idle_vcpu[cpu]->arch.stack;
> >  
> > @@ -361,6 +364,8 @@ int __cpu_up(unsigned int cpu)
> >  
> >      rc = arch_cpu_up(cpu);
> >  
> > +    console_end_sync();
> > +
> >      if ( rc < 0 )
> >      {
> >          printk("Failed to bring up CPU%d\n", cpu);
> > -- 
> > 1.7.10.4
> >

On Wed, 2013-11-20 at 19:17 +0000, Stefano Stabellini
wrote:
> On Wed, 20 Nov 2013, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/gic.c |   75
++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index ab49106..185a6b8 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -28,6 +28,7 @@
> >  #include <xen/softirq.h>
> >  #include <xen/list.h>
> >  #include <xen/device_tree.h>
> > +#include <xen/keyhandler.h>
> >  #include <asm/p2m.h>
> >  #include <asm/domain.h>
> >  
> > @@ -385,6 +386,77 @@ int gic_irq_xlate(const u32 *intspec, unsigned
int intsize,
> >      return 0;
> >  }
> >  
> > +
> > +static void do_dump_gic(unsigned char key)
> 
> It might be worth renaming the current gic_dump_info to
> gic_dump_info_guest to avoid confusions.
Done although based on Julien''s comments about SGI/PPI printing
I''m
considering putting this patch to one side for the time being.

On 11/20/2013 05:37 PM, Ian Campbell wrote:
> On Wed, 2013-11-20 at 17:31 +0000, Julien Grall wrote:
>> On 11/20/2013 02:48 PM, Ian Campbell wrote:
>>> If CONFIG_EARLY_PRINTK is in use then this gets all interleaved
with the
>>> standard logging otherwise.
>>>
>>
>> Before the discussion IRL, it was not clear to me that you want to
>> directly "flush" the console buffer when prinkt is called.
Can you
>> improve your commit message?
>
> Sure. How about:
>
>          xen: arm: enable synchronous console while starting secondary
>          CPUs
>
>          Setting synchronous console ensures that any printk hits the
>          buffer immediately and that any outstanding queued log messages
>          are flushed. This ensures that such log messages are not being
>          printed while the secondary CPU may be using early_printk during
>          early bringup.
>
>          Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

This commit message looks better for me.

-- 
Julien Grall

On 11/20/2013 02:48 PM, Ian Campbell wrote:
> The ranges property of a node with device_type = "pci" is defined
in ePAPR
> 2.3.8. Map the appropriate MMIO regions through to dom0.
>
> This is a hack/PoC since it actually crashes for some reason. Hence it
> contains a hacked in hardcoded list suitable for Xgene while I figure this
> out.
>
> This should also eventually handle the interrupt-map and (ePAPR 2.4.3.1)
and
> possibly dma-ranges (ePAPR 2.3.9) and msi-ranges (unspeciifed?) too.

For pci ranges, you can add a new bus in dt_bues. So you won''t need 
specific code in domain_build.c and we will be able to use it later.
You can take a look to linux/drivers/of/address.c

Mapping interrupt should be OK. I don''t see any reason that will fail 
with the current code because IRQ are all under the GIC controller.

-- 
Julien Grall

On Thu, 2013-11-21 at 14:32 +0000, Julien Grall wrote:
> 
> On 11/20/2013 02:48 PM, Ian Campbell wrote:
> > The ranges property of a node with device_type = "pci" is
defined in ePAPR
> > 2.3.8. Map the appropriate MMIO regions through to dom0.
> >
> > This is a hack/PoC since it actually crashes for some reason. Hence it
> > contains a hacked in hardcoded list suitable for Xgene while I figure
this
> > out.
> >
> > This should also eventually handle the interrupt-map and (ePAPR
2.4.3.1) and
> > possibly dma-ranges (ePAPR 2.3.9) and msi-ranges (unspeciifed?) too.
> 
> 
> For pci ranges, you can add a new bus in dt_bues. So you won''t
need
> specific code in domain_build.c and we will be able to use it later.
> You can take a look to linux/drivers/of/address.c
Won''t we still need to handle the resulting extra MMIO addresses which
are not part of the reg region?
> Mapping interrupt should be OK. I don''t see any reason that will
fail
> with the current code because IRQ are all under the GIC controller.
We need to parse interrupt-map because the child interrupts are not
listed in the regular interrupts property.

Ian.

On Wed, Nov 20, 2013 at 2:45 PM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> I''m afraid this series is rather a grab bag and it is
distressingly
> large at this stage. With this series I can boot an Xgene board until it
> fails to find its SATA controller. This is a dom0 issue for which
> patches are pending from APM (/me nudges Anup).
>
> As well as the APM specific platform stuff there are also some generic
> improvements which were either necessary or useful during this work.
> Towards the tail end are some hacks and rfcs which need more work and/or
> discussion. I mostly posting now because I''m aware that
I''ve been
> negligent in not sending these out sooner.
>
> WRT the freeze I think that the baseline stuff is all plausible for 4.4.
> Firstly because I''m inclined to give new platform enablement a
fairly
> loose reign at least for the time being (and much of it was posted ages
> ago by Anup/Pranavkumar). Secondly the non-platform related bits (other
> than the aforementioned hacks/rfcs) fall mostly either into two buckets:
> Either they are bugfixes or they are mostly obviously safe (additional
> debug prints etc).
>
> Blow by blow analysis:
>
>         xen: arm64: Add 8250 earlyprintk support
>
>                 New early uart driver. It is enabled as a build time
>                 debug option and is totally harmless to platforms which
>                 don''t use it.
>
>         xen: arm64: Add Basic Platform support for APM X-Gene Storm.
>         xen: arm64: Add APM implementor id to processor implementers.
>         xen: arm: include ns16550 driver on arm64 too
>         xen: arm: Enable 1:1 workaround for APM X-Gene Storm.
>
>                 Support for the new platform. Enable an existing driver
>                 used by that platform (already on for arm32).
>
>         xen: arm: early logging of command line
>
>                 Pretty safe & very useful IMNSVHO.
All of these look fine from a release perspective.
>
>         xen: arm: Handle cpus nodes with #address-calls > 1
So we need to make a distinction here with "bug fixes": from a release
perspective, bugs are errors in the code that affect working features.
 What is the likelihood that any currently-supported architectures
might problems without this patch?  It''s not clear from looking at the
patch.  If it''s low-to-none, then this wouldn''t really qualify
for a
bug fix exemption to the code freeze.
>         xen: arm: Make register bit definitions unsigned.
>         xen: arm: explicitly map 64 bit release address
>
>                 Bug fixes.
These two look fine.
>
>         xen: arm: enable synchronous console while starting secondary CPUs
>
>                 Improves logging in a useful way. Pretty safe.
>
>         xen: arm: Add debug keyhandler to dump the physical GIC state.
>
>                 Useful debug functionality. Harmless unless you
>                 deliberately trigger the particular debug key.
>
>         xen: arm: improve early memory map readability
>
>                 Cosmetic, but safe.
These all look fine as well.
>
>         RFC: xen: arm: handle 40-bit addresses in the p2m
>         RFC: xen: arm: allow platform code to select dom0 event channel irq
>
>                 These should be considered for cleanup review and
>                 eventual commit for 4.4. The rest of the platform
>                 enablement is pretty pointless without these.
Hmm... it seems a bit late for RFC stuff in fairly core code.  These
look like they might possibly be extending functionality for
currently-supported architectures as well; but without concrete
examples, this would come under "new feature" rather than "bug
fix".

On the other hand, both of these look pretty obvious and low-risk improvements.
>
>         HACK: xen: arm: GICC_DIR register at offset 0x10000 instead of
0x1000
>
>                 Should be properly implemented with a view to being
>                 accepted for 4.4. Again things are rather pointless
>                 without.
>
>                 Could plausibly be reimplemented as a platform quirk,
>                 which might be safer for 4.4.
>
>         HACK: xen: arm: map PCI controller ranges region MMIOs to dom0.
>
>                 I think this one is likely to be a step too far for 4.4.
>                 Even if it worked (it doesn''t) it is quite a big
and
>                 potentially complex change. I''m considering the
option
>                 of implementing the hardcoded list (which is here as a
>                 HACK, see the commit message for more) via the
>                 platform->specific_mapping callback for 4.4. In that
>                 case it would only impact Xgene if it were broken.
This is starting to look like an awful lot of "to be sorted out".
(Although it looks like Julien has a simple solution that makes this
last patch unnecessary?)

You address risks, but you don''t address the fundamental benefit of
including it now, rather than waiting to check it in for 4.5.  At the
moment, unless there is some compelling strategic reason for including
this in 4.4, I''m inclined to say it should just wait.

 -George

On Thu, 21 Nov 2013, George Dunlap wrote:
> >         HACK: xen: arm: GICC_DIR register at offset 0x10000 instead of
0x1000
> >
> >                 Should be properly implemented with a view to being
> >                 accepted for 4.4. Again things are rather pointless
> >                 without.
> >
> >                 Could plausibly be reimplemented as a platform quirk,
> >                 which might be safer for 4.4.
> >
> >         HACK: xen: arm: map PCI controller ranges region MMIOs to
dom0.
> >
> >                 I think this one is likely to be a step too far for
4.4.
> >                 Even if it worked (it doesn''t) it is quite a
big and
> >                 potentially complex change. I''m considering
the option
> >                 of implementing the hardcoded list (which is here as a
> >                 HACK, see the commit message for more) via the
> >                 platform->specific_mapping callback for 4.4. In
that
> >                 case it would only impact Xgene if it were broken.
> 
> This is starting to look like an awful lot of "to be sorted out".
> (Although it looks like Julien has a simple solution that makes this
> last patch unnecessary?)
> 
> You address risks, but you don''t address the fundamental benefit
of
> including it now, rather than waiting to check it in for 4.5.  At the
> moment, unless there is some compelling strategic reason for including
> this in 4.4, I''m inclined to say it should just wait.
Would you be OK with accepting an alternative implementation of these
two patches via platform specific hooks, as Ian suggested?

On Thu, 2013-11-21 at 15:05 +0000, George Dunlap wrote:
> On Wed, Nov 20, 2013 at 2:45 PM, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > I''m afraid this series is rather a grab bag and it is
distressingly
> > large at this stage. With this series I can boot an Xgene board until
it
> > fails to find its SATA controller. This is a dom0 issue for which
> > patches are pending from APM (/me nudges Anup).
> >
> > As well as the APM specific platform stuff there are also some generic
> > improvements which were either necessary or useful during this work.
> > Towards the tail end are some hacks and rfcs which need more work
and/or
> > discussion. I mostly posting now because I''m aware that
I''ve been
> > negligent in not sending these out sooner.
> >
> > WRT the freeze I think that the baseline stuff is all plausible for
4.4.
> > Firstly because I''m inclined to give new platform enablement
a fairly
> > loose reign at least for the time being (and much of it was posted
ages
> > ago by Anup/Pranavkumar). Secondly the non-platform related bits
(other
> > than the aforementioned hacks/rfcs) fall mostly either into two
buckets:
> > Either they are bugfixes or they are mostly obviously safe (additional
> > debug prints etc).
> >
> > Blow by blow analysis:
Pulling your last comment first, since it informs many of my other
answers:
> You address risks, but you don''t address the fundamental benefit
of
> including it now, rather than waiting to check it in for 4.5.  At the
> moment, unless there is some compelling strategic reason for including
> this in 4.4, I''m inclined to say it should just wait.
The primary benefit is that this is the first real (i.e. non emulated)
64-bit ARM server platform on the market. Having Xen running on that
early in the new year would be awesome.

As well as the currently supported platforms and this one new one we
should also account for people who want to port Xen 4.4 to their own
platform. The closer we can make this to "drop a file in
xen/arch/arm/platforms/ and add it to the Makefile" the better IMHO.
Most of the patches below (i.e. the ones which haven''t already been
okayed) are relevant in this light.
> >         xen: arm: Handle cpus nodes with #address-cells > 1
> 
> So we need to make a distinction here with "bug fixes": from a
release
> perspective, bugs are errors in the code that affect working features.
>  What is the likelihood that any currently-supported architectures
> might problems without this patch?  It''s not clear from looking at
the
> patch.  If it''s low-to-none, then this wouldn''t really
qualify for a
> bug fix exemption to the code freeze.
In principal it''s entirely possible that someone might rewrite the dts
of such a platform this way. It''s a bit unlikely but the main reason
would because as well as the cpu number #address-cells also influences
things like the cpu spin address property (where it is present), which
could conceivably be about 4G (albeit unlikely on 32-bit).

But ultimately this is a Xen bug because it does not correctly parse a
valid device tree file.
> >
> >         RFC: xen: arm: handle 40-bit addresses in the p2m
> >         RFC: xen: arm: allow platform code to select dom0 event
channel irq
> >
> >                 These should be considered for cleanup review and
> >                 eventual commit for 4.4. The rest of the platform
> >                 enablement is pretty pointless without these.
> 
> Hmm... it seems a bit late for RFC stuff in fairly core code.  These
> look like they might possibly be extending functionality for
> currently-supported architectures as well; but without concrete
> examples, this would come under "new feature" rather than
"bug fix".
The 40-bit address thing is an issue on 32-bit too, it''s just that the
platforms don''t typically have anything mapped up that high (MMIO tends
to be lower to support non-LPAE kernels and they don''t typically have
TBs of RAM).

On the plus side the new case would never hit on the existing platforms.
If nothing else there is currently a BUG_ON checking for that.

The dom0 event channel thing is an issue for all platforms, I think
we''ve just been lucky that they mostly don''t use the current
hardcoded
number for something, although I had it in mind that one of them did and
was being hacked around.
> On the other hand, both of these look pretty obvious and low-risk
improvements.
> 
> >
> >         HACK: xen: arm: GICC_DIR register at offset 0x10000 instead of
0x1000
> >
> >                 Should be properly implemented with a view to being
> >                 accepted for 4.4. Again things are rather pointless
> >                 without.
> >
> >                 Could plausibly be reimplemented as a platform quirk,
> >                 which might be safer for 4.4.
> >
> >         HACK: xen: arm: map PCI controller ranges region MMIOs to
dom0.
> >
> >                 I think this one is likely to be a step too far for
4.4.
> >                 Even if it worked (it doesn''t) it is quite a
big and
> >                 potentially complex change. I''m considering
the option
> >                 of implementing the hardcoded list (which is here as a
> >                 HACK, see the commit message for more) via the
> >                 platform->specific_mapping callback for 4.4. In
that
> >                 case it would only impact Xgene if it were broken.
> 
> This is starting to look like an awful lot of "to be sorted out".
Yes. I was a bit unhappy to find I had these in my tree -- I thought
what was outstanding was less intrusive.
> (Although it looks like Julien has a simple solution that makes this
> last patch unnecessary?)
I don''t think Julien is right about that simpler solution being
workable, but regardless I think it would be better to err on the side
of caution and reimplement both of these as platform hooks for 4.4. The
first would be a new quirk (only implemented by this platform) and the
second would be using an existing per-platform hook.

Unless that sounds obviously bogus to you I''ll put something together
for you to pass judgement on.

Ian.

On 11/21/2013 02:57 PM, Ian Campbell wrote:
> On Thu, 2013-11-21 at 14:32 +0000, Julien Grall wrote:
>>
>> On 11/20/2013 02:48 PM, Ian Campbell wrote:
>>> The ranges property of a node with device_type = "pci" is
defined in ePAPR
>>> 2.3.8. Map the appropriate MMIO regions through to dom0.
>>>
>>> This is a hack/PoC since it actually crashes for some reason. Hence
it
>>> contains a hacked in hardcoded list suitable for Xgene while I
figure this
>>> out.
>>>
>>> This should also eventually handle the interrupt-map and (ePAPR
2.4.3.1) and
>>> possibly dma-ranges (ePAPR 2.3.9) and msi-ranges (unspeciifed?)
too.
>>
>>
>> For pci ranges, you can add a new bus in dt_bues. So you won''t
need
>> specific code in domain_build.c and we will be able to use it later.
>> You can take a look to linux/drivers/of/address.c
>
> Won''t we still need to handle the resulting extra MMIO addresses
which
> are not part of the reg region?
Which MMIO addresses are you talking about? I though all the ranges are 
described by "reg"?

If the modification is too hard to implement for Xen 4.4, you can 
implement specific_mapping callback for APM.
>
>> Mapping interrupt should be OK. I don''t see any reason that
will fail
>> with the current code because IRQ are all under the GIC controller.
>
> We need to parse interrupt-map because the child interrupts are not
> listed in the regular interrupts property.
Right, I didn''t notice that the interrupt-map is difference for pci.

-- 
Julien Grall

On Thu, 2013-11-21 at 15:42 +0000, Julien Grall wrote:
> 
> On 11/21/2013 02:57 PM, Ian Campbell wrote:
> > On Thu, 2013-11-21 at 14:32 +0000, Julien Grall wrote:
> >>
> >> On 11/20/2013 02:48 PM, Ian Campbell wrote:
> >>> The ranges property of a node with device_type =
"pci" is defined in ePAPR
> >>> 2.3.8. Map the appropriate MMIO regions through to dom0.
> >>>
> >>> This is a hack/PoC since it actually crashes for some reason.
Hence it
> >>> contains a hacked in hardcoded list suitable for Xgene while I
figure this
> >>> out.
> >>>
> >>> This should also eventually handle the interrupt-map and
(ePAPR 2.4.3.1) and
> >>> possibly dma-ranges (ePAPR 2.3.9) and msi-ranges
(unspeciifed?) too.
> >>
> >>
> >> For pci ranges, you can add a new bus in dt_bues. So you
won''t need
> >> specific code in domain_build.c and we will be able to use it
later.
> >> You can take a look to linux/drivers/of/address.c
> >
> > Won''t we still need to handle the resulting extra MMIO
addresses which
> > are not part of the reg region?
> 
> Which MMIO addresses are you talking about? I though all the ranges are 
> described by "reg"?
Please check ePAPR 2.3.8 -- the ranges property is used in bus nodes to
define the MMIO regions used by the child devices on that bus. The reg
property only covers the MMIO used by the bus controller itself.

The interrupts-map is similar.
> If the modification is too hard to implement for Xen 4.4, you can 
> implement specific_mapping callback for APM.
Yes, this is what I proposed already and am looking into.

Ian.

On 21/11/13 15:38, Ian Campbell wrote:
> On Thu, 2013-11-21 at 15:05 +0000, George Dunlap wrote:
>> On Wed, Nov 20, 2013 at 2:45 PM, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>>> I''m afraid this series is rather a grab bag and it is
distressingly
>>> large at this stage. With this series I can boot an Xgene board
until it
>>> fails to find its SATA controller. This is a dom0 issue for which
>>> patches are pending from APM (/me nudges Anup).
>>>
>>> As well as the APM specific platform stuff there are also some
generic
>>> improvements which were either necessary or useful during this
work.
>>> Towards the tail end are some hacks and rfcs which need more work
and/or
>>> discussion. I mostly posting now because I''m aware that
I''ve been
>>> negligent in not sending these out sooner.
>>>
>>> WRT the freeze I think that the baseline stuff is all plausible for
4.4.
>>> Firstly because I''m inclined to give new platform
enablement a fairly
>>> loose reign at least for the time being (and much of it was posted
ages
>>> ago by Anup/Pranavkumar). Secondly the non-platform related bits
(other
>>> than the aforementioned hacks/rfcs) fall mostly either into two
buckets:
>>> Either they are bugfixes or they are mostly obviously safe
(additional
>>> debug prints etc).
>>>
>>> Blow by blow analysis:
> Pulling your last comment first, since it informs many of my other
> answers:
>> You address risks, but you don''t address the fundamental
benefit of
>> including it now, rather than waiting to check it in for 4.5.  At the
>> moment, unless there is some compelling strategic reason for including
>> this in 4.4, I''m inclined to say it should just wait.
> The primary benefit is that this is the first real (i.e. non emulated)
> 64-bit ARM server platform on the market. Having Xen running on that
> early in the new year would be awesome.
>
> As well as the currently supported platforms and this one new one we
> should also account for people who want to port Xen 4.4 to their own
> platform. The closer we can make this to "drop a file in
> xen/arch/arm/platforms/ and add it to the Makefile" the better IMHO.
> Most of the patches below (i.e. the ones which haven''t already
been
> okayed) are relevant in this light.
Right, so this would be for people shipping "4.4+vendor patches". If
we
didn''t accept these, they''d have to fix or backport all these
things
themselves.

That sounds like a pretty compelling strategic reason. :-)  And it 
justifies a number of the more potentially risky patches as improvements 
in their own right (i.e., not just for xgene, but for other bigger 
platforms).
>>>          xen: arm: Handle cpus nodes with #address-cells > 1
>> So we need to make a distinction here with "bug fixes": from
a release
>> perspective, bugs are errors in the code that affect working features.
>>   What is the likelihood that any currently-supported architectures
>> might problems without this patch?  It''s not clear from
looking at the
>> patch.  If it''s low-to-none, then this wouldn''t
really qualify for a
>> bug fix exemption to the code freeze.
> In principal it''s entirely possible that someone might rewrite the
dts
> of such a platform this way. It''s a bit unlikely but the main
reason
> would because as well as the cpu number #address-cells also influences
> things like the cpu spin address property (where it is present), which
> could conceivably be about 4G (albeit unlikely on 32-bit).
>
> But ultimately this is a Xen bug because it does not correctly parse a
> valid device tree file.
So just to step back and talk about principles here:  Sure, and I
didn''t
say it wasn''t a bug.  But I don''t think that the freeze
exemption is to
just fix bugs per se; it''s to fix broken functionality in supported 
features.  So just the fact that something is in theory wrong with Xen 
isn''t enough; it has to impact features that are actually supported.

Now in this case I think this distinction is unnecessary, since I buy 
your argument that one of the things we want to support is the 
"4.4+vendor patches" model; so it does impact features that we
actually
want to support.  But if it weren''t for that, I wouldn''t
accept it just
on the basis that it''s a bug in Xen, if it didn''t actually
affect any of
the supported functionality.
>
>>>          RFC: xen: arm: handle 40-bit addresses in the p2m
>>>          RFC: xen: arm: allow platform code to select dom0 event
channel irq
>>>
>>>                  These should be considered for cleanup review and
>>>                  eventual commit for 4.4. The rest of the platform
>>>                  enablement is pretty pointless without these.
>> Hmm... it seems a bit late for RFC stuff in fairly core code.  These
>> look like they might possibly be extending functionality for
>> currently-supported architectures as well; but without concrete
>> examples, this would come under "new feature" rather than
"bug fix".
> The 40-bit address thing is an issue on 32-bit too, it''s just that
the
> platforms don''t typically have anything mapped up that high (MMIO
tends
> to be lower to support non-LPAE kernels and they don''t typically
have
> TBs of RAM).
>
> On the plus side the new case would never hit on the existing platforms.
> If nothing else there is currently a BUG_ON checking for that.
Oh, right -- it looked like you might be increasing the number of pages 
allocated, but now I see that you''ve just replaced a
''1'' with
P2M_FIRST_ORDER (which is different to P2M_FIRST_ENTRIES).  That makes 
more sense then.
>> (Although it looks like Julien has a simple solution that makes this
>> last patch unnecessary?)
> I don''t think Julien is right about that simpler solution being
> workable, but regardless I think it would be better to err on the side
> of caution and reimplement both of these as platform hooks for 4.4. The
> first would be a new quirk (only implemented by this platform) and the
> second would be using an existing per-platform hook.
>
> Unless that sounds obviously bogus to you I''ll put something
together
> for you to pass judgement on.
Sounds good.

  -George

On Wed, 20 Nov 2013, Ian Campbell wrote:
> Currently the hardcoded use of GUEST_EVTCHN_PPI is problematic if that is a
> real PPI on the platform.
> 
> We really need to be smarter about selecting an unused PPI but in the
meantime
> we can at least give the platform code the option of hardcoding a number
which
> works for the platform.
> 
> Hardcode a suitable PPI on the Xgene platform.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

I think this is a reasonable approach for 4.4.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

> RFC: I''m not all that happy with this, but it seems better than
nothing for
> 4.4...
> 
> If other existing platforms also have this issue I can extend the patch if
> told what a suitable PPI is.
> ---
>  xen/arch/arm/domain.c                |    7 +++++--
>  xen/arch/arm/platform.c              |    7 +++++++
>  xen/arch/arm/platforms/xgene-storm.c |    1 +
>  xen/include/asm-arm/platform.h       |    5 +++++
>  4 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 2f57d01..52d2403 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -31,6 +31,7 @@
>  #include <asm/processor-ca15.h>
>  
>  #include <asm/gic.h>
> +#include <asm/platform.h>
>  #include "vtimer.h"
>  #include "vuart.h"
>  
> @@ -526,8 +527,10 @@ int arch_domain_create(struct domain *d, unsigned int
domcr_flags)
>      if ( (rc = vcpu_domain_init(d)) != 0 )
>          goto fail;
>  
> -    /* XXX dom0 needs more intelligent selection of PPI */
> -    d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
> +    if ( d->domain_id )
> +        d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
> +    else
> +        d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
>  
>      /*
>       * Virtual UART is only used by linux early printk and decompress
code.
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index 0fbbdc7..a7f9ee4 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -156,6 +156,13 @@ bool_t platform_device_is_blacklisted(const struct
dt_device_node *node)
>      return dt_match_node(blacklist, node);
>  }
>  
> +unsigned int platform_dom0_evtchn_ppi(void)
> +{
> +    if ( platform && platform->dom0_evtchn_ppi )
> +        return platform->dom0_evtchn_ppi;
> +    return GUEST_EVTCHN_PPI;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/platforms/xgene-storm.c
b/xen/arch/arm/platforms/xgene-storm.c
> index 23986a9..b1b850b 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -52,6 +52,7 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
>      .init = xgene_storm_init,
>      .reset = xgene_storm_reset,
>      .quirks = xgene_storm_quirks,
> +    .dom0_evtchn_ppi = 24,
>  PLATFORM_END
>  
>  /*
> diff --git a/xen/include/asm-arm/platform.h
b/xen/include/asm-arm/platform.h
> index c282b30..92b954d 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -37,6 +37,10 @@ struct platform_desc {
>       * List of devices which must not pass-through to a guest
>       */
>      const struct dt_device_match *blacklist_dev;
> +    /*
> +     * The IRQ (PPI) to use to inject event channels to dom0.
> +     */
> +    unsigned int dom0_evtchn_ppi;
>  };
>  
>  /*
> @@ -56,6 +60,7 @@ 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   \
> -- 
> 1.7.10.4
>

On Wed, 20 Nov 2013, Ian Campbell wrote:
> On the X-gene platform there are resources up this high which must be
mapped
> to dom0.
> 
> I''m becoming more convinced that p2m first level pages should be
mapped from
> the xenheap so we can avoid all this faff with figuring out which page is
> needed.
> 
> Remove the first level page from the p2m->pages list since it is
actually two
> pages and must be freed as such. Do so in p2m_teardown.
> 
> I''ve also punted on the implementation of dump_p2m_lookup for high
> addresses...
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> This is a little bit RFC...
The patch looks good but the commit message is very unclear. It is
best to clarify what this patch intends to do and then separately what
could be a future development.

>  xen/arch/arm/p2m.c |   60
+++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 48 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 82dda65..af32511 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -7,6 +7,10 @@
>  #include <asm/flushtlb.h>
>  #include <asm/gic.h>
>  
> +/* First level P2M is 2 consecutive pages */
> +#define P2M_FIRST_ORDER 1
> +#define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER)
> +
>  void dump_p2m_lookup(struct domain *d, paddr_t addr)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> @@ -14,6 +18,12 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
>  
>      printk("dom%d IPA 0x%"PRIpaddr"\n",
d->domain_id, addr);
>  
> +    if ( first_linear_offset(addr) > LPAE_ENTRIES )
> +    {
> +        printk("Cannot dump addresses in second of first level
pages...\n");
> +        return;
> +    }
> +
>      printk("P2M @ %p mfn:0x%lx\n",
>             p2m->first_level, page_to_mfn(p2m->first_level));
>  
> @@ -31,6 +41,30 @@ void p2m_load_VTTBR(struct domain *d)
>      isb(); /* Ensure update is visible */
>  }
>  
> +static int p2m_first_level_index(paddr_t addr)
> +{
> +    /*
> +     * 1st pages are concatenated so zeroeth offset gives us the
> +     * index of the 1st page
> +     */
> +    return zeroeth_table_offset(addr);
> +}
> +
> +/*
> + * Map whichever of the first pages contain addr. The caller should
> + * then use first_table_offset as an index.
> + */
> +static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
> +{
> +    struct page_info *page;
> +
> +    BUG_ON(first_linear_offset(addr) > P2M_FIRST_ENTRIES);
> +
> +    page = p2m->first_level + p2m_first_level_index(addr);
> +
> +    return __map_domain_page(page);
> +}
> +
>  /*
>   * Lookup the MFN corresponding to a domain''s PFN.
>   *
> @@ -45,7 +79,7 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr)
>  
>      spin_lock(&p2m->lock);
>  
> -    first = __map_domain_page(p2m->first_level);
> +    first = p2m_map_first(p2m, paddr);
>  
>      pte = first[first_table_offset(paddr)];
>      if ( !pte.p2m.valid || !pte.p2m.table )
> @@ -135,18 +169,21 @@ static int create_p2m_entries(struct domain *d,
>      struct p2m_domain *p2m = &d->arch.p2m;
>      lpae_t *first = NULL, *second = NULL, *third = NULL;
>      paddr_t addr;
> -    unsigned long cur_first_offset = ~0, cur_second_offset = ~0;
> +    unsigned long cur_first_page = ~0,
> +                  cur_first_offset = ~0,
> +                  cur_second_offset = ~0;
>  
>      spin_lock(&p2m->lock);
>  
> -    /* XXX Don''t actually handle 40 bit guest physical addresses
*/
> -    BUG_ON(start_gpaddr & 0x8000000000ULL);
> -    BUG_ON(end_gpaddr   & 0x8000000000ULL);
> -
> -    first = __map_domain_page(p2m->first_level);
> -
>      for(addr = start_gpaddr; addr < end_gpaddr; addr += PAGE_SIZE)
>      {
> +        if ( cur_first_page != p2m_first_level_index(addr) )
> +        {
> +            if ( first ) unmap_domain_page(first);
> +            first = p2m_map_first(p2m, addr);
> +            cur_first_page = p2m_first_level_index(addr);
> +        }
> +
>          if ( !first[first_table_offset(addr)].p2m.valid )
>          {
>              rc = p2m_create_table(d,
&first[first_table_offset(addr)]);
> @@ -279,15 +316,12 @@ int p2m_alloc_table(struct domain *d)
>      struct page_info *page;
>      void *p;
>  
> -    /* First level P2M is 2 consecutive pages */
> -    page = alloc_domheap_pages(NULL, 1, 0);
> +    page = alloc_domheap_pages(NULL, P2M_FIRST_ORDER, 0);
>      if ( page == NULL )
>          return -ENOMEM;
>  
>      spin_lock(&p2m->lock);
>  
> -    page_list_add(page, &p2m->pages);
> -
>      /* Clear both first level pages */
>      p = __map_domain_page(page);
>      clear_page(p);
> @@ -380,6 +414,8 @@ void p2m_teardown(struct domain *d)
>      while ( (pg = page_list_remove_head(&p2m->pages)) )
>          free_domheap_page(pg);
>  
> +    free_domheap_pages(p2m->first_level, P2M_FIRST_ORDER);
> +
>      p2m->first_level = NULL;
>  
>      p2m_free_vmid(d);
> -- 
> 1.7.10.4
>

On Thu, 2013-11-21 at 19:17 +0000, Stefano Stabellini
wrote:
> On Wed, 20 Nov 2013, Ian Campbell wrote:
> > On the X-gene platform there are resources up this high which must be
mapped
> > to dom0.
> > 
> > I''m becoming more convinced that p2m first level pages should
be mapped from
> > the xenheap so we can avoid all this faff with figuring out which page
is
> > needed.
> > 
> > Remove the first level page from the p2m->pages list since it is
actually two
> > pages and must be freed as such. Do so in p2m_teardown.
> > 
> > I''ve also punted on the implementation of dump_p2m_lookup for
high
> > addresses...
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > This is a little bit RFC...
> 
> The patch looks good but the commit message is very unclear. It is
> best to clarify what this patch intends to do and then separately what
> could be a future development.
Yeah, it is a bit stream of consciousness isn''t it.

I''ve dropped the paragraph "I''m becoming..." which I
think isn''t really
necessary in the commit log. I think without that the commit message is
ok, what do you think?