All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-4.9 0/4] xen/arm: Properly map the FDT in the boot page table
@ 2017-04-19 17:13 Julien Grall
  2017-04-19 17:13 ` [PATCH for-4.9 1/4] xen/arm: Add BOOT_FDT_VIRT_END and BOOT_FDT_SLOT_SIZE Julien Grall
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Julien Grall @ 2017-04-19 17:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, sstabellini

Hi,

Whilst doing some testing on Juno using GRUB, I noticed random early crash
depending ([1]) on the binaries I was using.

This is because Xen is assuming that the FDT will always fit in a 2MB
superpage whilst the boot documentation allow the FDT to cross a 2MB boundary.

The first patch move the code that map the FDT in the boot page table from
assembly to C making easier to modify the code.

This series is candidate for Xen 4.9. Whilst this early boot rework sounds
scary, a user can see random early crash without this series. I chose
to move all the FDT mapping code in C right now because it is less error-prone
to write C code than assembly.

I have tested both ARM32 and ARM64 with different position of the FDT without
noticing any issue.

Cheers,

[1]

(XEN) Hypervisor Trap. HSR=0x96000006 EC=0x25 IL=1 Syndrome=0x6
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) ----[ Xen-4.9-unstable  arm64  debug=y   Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     0000000000264140 strlen+0x10/0x84
(XEN) LR:     00000000002401c0
(XEN) SP:     00000000002cfc20
(XEN) CPSR:   400003c9 MODE:64-bit EL2h (Hypervisor, handler)
(XEN)      X0: 0000000000801230  X1: 0000000000801230  X2: 0000000000005230
(XEN)      X3: 0000000000000030  X4: 0000000000000030  X5: 0000000000000038
(XEN)      X6: 0000000000000034  X7: 0000000000000000  X8: 7f7f7f7f7f7f7f7f
(XEN)      X9: 64622c6479687222 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
(XEN)     X12: 0000000000000030 X13: ffffff00ff000000 X14: 0800000003000000
(XEN)     X15: ffffffffffffffff X16: 00000000fefff610 X17: 00000000000000f0
(XEN)     X18: 0000000000000004 X19: 0000000000000008 X20: 00000000007fc040
(XEN)     X21: 00000000007fc000 X22: 000000000000000e X23: 0000000000000000
(XEN)     X24: 00000000002a9f58 X25: 0000000000801230 X26: 00000000002a9f68
(XEN)     X27: 00000000002a9f58 X28: 0000000000298910  FP: 00000000002cfc20
(XEN)
(XEN)   VTCR_EL2: 80010c40
(XEN)  VTTBR_EL2: 0000082800203000
(XEN)
(XEN)  SCTLR_EL2: 30c5183d
(XEN)    HCR_EL2: 000000000038663f
(XEN)  TTBR0_EL2: 00000000f4912000
(XEN)
(XEN)    ESR_EL2: 96000006
(XEN)  HPFAR_EL2: 00000000e8071000
(XEN)    FAR_EL2: 0000000000801230
(XEN)
(XEN) Xen stack trace from sp=00000000002cfc20:
(XEN)    00000000002cfc70 0000000000240254 00000000002a9f58 00000000007fc000
(XEN)    0000000000000000 0000000000000000 0000000000000000 00000000007fc03c
(XEN)    00000000002cfd78 0000000000000000 00000000002cfca0 00000000002986fc
(XEN)    0000000000000000 00000000007fc000 0000000000000000 0000000000000000
(XEN)    00000000002cfcc0 0000000000298f1c 0000000000000000 00000000007fc000
(XEN)    00000000002cfdc0 000000000029904c 00000000f47fc000 00000000f4604000
(XEN)    00000000f47fc000 00000000007fc000 0000000000400000 0000000000000100
(XEN)    00000000f4604000 0000000000000001 0000000000000001 8000000000000002
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 00000000002cfdc0 0000000000299038
(XEN)    00000000f47fc000 00000000f4604000 00000000f47fc000 0000000000000000
(XEN)    00000000002cfe20 000000000029c420 00000000002d8000 00000000f4604000
(XEN)    00000000f47fc000 0000000000000000 0000000000400000 0000000000000100
(XEN)    00000000f4604000 0000000000000001 00000000f47fc000 000000000029c404
(XEN)    00000000fefff510 0000000000200624 00000000f4804000 00000000f4604000
(XEN)    00000000f47fc000 0000000000000000 0000000000400000 0000000000000100
(XEN)    0000000000000001 0000000000000001 0000000000000001 8000000000000002
(XEN)    00000000f47fc000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<0000000000264140>] strlen+0x10/0x84 (PC)
(XEN)    [<00000000002401c0>] fdt_get_property_namelen+0x9c/0xf0 (LR)
(XEN)    [<0000000000240254>] fdt_get_property+0x40/0x50
(XEN)    [<00000000002986fc>] bootfdt.c#device_tree_get_u32+0x18/0x5c
(XEN)    [<0000000000298f1c>] device_tree_for_each_node+0x84/0x144
(XEN)    [<000000000029904c>] boot_fdt_info+0x70/0x23c
(XEN)    [<000000000029c420>] start_xen+0x9c/0xd30
(XEN)    [<0000000000200624>] arm64/head.o#paging+0x84/0xbc
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN)
(XEN) ****************************************

Julien Grall (4):
  xen/arm: Add BOOT_FDT_VIRT_END and BOOT_FDT_SLOT_SIZE
  xen/arm: Move the code to map FDT in the boot tables from assembly to
    C
  xen/arm: Check if the FDT passed by the bootloader is valid
  xen/arm: Properly map the FDT in the boot page table

 xen/arch/arm/arm32/head.S    | 14 ----------
 xen/arch/arm/arm64/head.S    | 13 ----------
 xen/arch/arm/mm.c            | 61 +++++++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/setup.c         | 10 +++++---
 xen/include/asm-arm/config.h | 16 +++++++-----
 xen/include/asm-arm/mm.h     |  2 ++
 xen/include/asm-arm/setup.h  |  3 +++
 7 files changed, 82 insertions(+), 37 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [PATCH for-4.9 1/4] xen/arm: Add BOOT_FDT_VIRT_END and BOOT_FDT_SLOT_SIZE
  2017-04-19 17:13 [PATCH for-4.9 0/4] xen/arm: Properly map the FDT in the boot page table Julien Grall
@ 2017-04-19 17:13 ` Julien Grall
  2017-04-19 21:01   ` Stefano Stabellini
  2017-04-19 17:13 ` [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C Julien Grall
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Julien Grall @ 2017-04-19 17:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, sstabellini

The 2 new defines will help to avoid hardcoding the size and the end of
the slot in the code.

The newlines are added for clarity.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/config.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index b2edf95f72..9c14a385e7 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -111,7 +111,11 @@
 
 #define XEN_VIRT_START         _AT(vaddr_t,0x00200000)
 #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
+
 #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
+#define BOOT_FDT_SLOT_SIZE     MB(2)
+#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
+
 #define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00800000)
 #ifdef CONFIG_LIVEPATCH
 #define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00800000)
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C
  2017-04-19 17:13 [PATCH for-4.9 0/4] xen/arm: Properly map the FDT in the boot page table Julien Grall
  2017-04-19 17:13 ` [PATCH for-4.9 1/4] xen/arm: Add BOOT_FDT_VIRT_END and BOOT_FDT_SLOT_SIZE Julien Grall
@ 2017-04-19 17:13 ` Julien Grall
  2017-04-19 21:01   ` Stefano Stabellini
  2017-04-19 17:13 ` [PATCH for-4.9 3/4] xen/arm: Check if the FDT passed by the bootloader is valid Julien Grall
  2017-04-19 17:13 ` [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table Julien Grall
  3 siblings, 1 reply; 17+ messages in thread
From: Julien Grall @ 2017-04-19 17:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, sstabellini

The FDT will not be accessed before start_xen (begining of C code) is
called and it will be easier to maintain as the code could be common
between AArch32 and AArch64.

A new function early_fdt_map is introduced to map the FDT in the boot
page table.

Note that create_mapping will flush the TLBs for us so no need to add an
extra on in case the entry was previously used by the 1:1 mapping.

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

---

    I can move the function create_mappings at the beginning of the
    function instead of adding a static declaration. But I thought it
    was not Xen 4.9 material. Any opinions?
---
 xen/arch/arm/arm32/head.S | 14 --------------
 xen/arch/arm/arm64/head.S | 13 -------------
 xen/arch/arm/mm.c         | 20 ++++++++++++++++++++
 xen/arch/arm/setup.c      |  4 +---
 xen/include/asm-arm/mm.h  |  2 ++
 5 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index ec63ba4c04..4090f4a744 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -389,20 +389,6 @@ paging:
         /* Use a virtual address to access the UART. */
         ldr   r11, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
-        /* Map the DTB in the boot misc slot */
-        teq   r12, #0                /* Only on boot CPU */
-        bne   1f
-
-        ldr   r1, =boot_second
-        mov   r3, #0x0
-        lsr   r2, r8, #SECOND_SHIFT
-        lsl   r2, r2, #SECOND_SHIFT  /* r2: 2MB-aligned paddr of DTB */
-        orr   r2, r2, #PT_UPPER(MEM)
-        orr   r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */
-        ldr   r4, =BOOT_FDT_VIRT_START
-        mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* Slot for BOOT_FDT_VIRT_START */
-        strd  r2, r3, [r1, r4]       /* Map it in the early fdt slot */
-1:
 
         /*
          * Flush the TLB in case the 1:1 mapping happens to clash with
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 72ea4e0233..78292f4396 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -550,19 +550,6 @@ paging:
         ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
 
-        /* Map the DTB in the boot misc slot */
-        cbnz  x22, 1f                /* Only on boot CPU */
-
-        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
-        lsr   x2, x21, #SECOND_SHIFT
-        lsl   x2, x2, #SECOND_SHIFT  /* x2 := 2MB-aligned paddr of DTB */
-        mov   x3, #PT_MEM            /* x2 := 2MB RAM incl. DTB */
-        orr   x2, x2, x3
-        ldr   x1, =BOOT_FDT_VIRT_START
-        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x4 := Slot for BOOT_FDT_VIRT_START */
-        str   x2, [x4, x1]           /* Map it in the early fdt slot */
-1:
-
         /*
          * Flush the TLB in case the 1:1 mapping happens to clash with
          * the virtual addresses used by the fixmap or DTB.
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index f0a2eddaaf..0d076489c6 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -38,6 +38,13 @@
 #include <xen/vmap.h>
 #include <xsm/xsm.h>
 #include <xen/pfn.h>
+#include <xen/sizes.h>
+
+static void __init create_mappings(lpae_t *second,
+                                   unsigned long virt_offset,
+                                   unsigned long base_mfn,
+                                   unsigned long nr_mfns,
+                                   unsigned int mapping_size);
 
 struct domain *dom_xen, *dom_io, *dom_cow;
 
@@ -434,6 +441,19 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va)
     return mfn_to_xen_entry(mfn, WRITEALLOC);
 }
 
+
+/* Map the FDT in the early boot page table */
+void * __init early_fdt_map(paddr_t fdt_paddr)
+{
+    /* We are using 2MB superpage for mapping the FDT */
+    paddr_t base_paddr = fdt_paddr & SECOND_MASK;
+
+    create_mappings(boot_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
+                    SZ_2M >> PAGE_SHIFT, SZ_2M);
+
+    return (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE);
+}
+
 void __init remove_early_mappings(void)
 {
     lpae_t pte = {0};
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 92a2de6b70..986398970f 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -724,9 +724,7 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     smp_clear_cpu_maps();
 
-    /* This is mapped by head.S */
-    device_tree_flattened = (void *)BOOT_FDT_VIRT_START
-        + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
+    device_tree_flattened = early_fdt_map(fdt_paddr);
     fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
 
     cmdline = boot_fdt_cmdline(device_tree_flattened);
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 0fef612f42..f6915ad882 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -160,6 +160,8 @@ extern unsigned long total_pages;
 
 /* Boot-time pagetable setup */
 extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr);
+/* Map FDT in boot pagetable */
+extern void *early_fdt_map(paddr_t fdt_paddr);
 /* Remove early mappings */
 extern void remove_early_mappings(void);
 /* Allocate and initialise pagetables for a secondary CPU. Sets init_ttbr to the
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH for-4.9 3/4] xen/arm: Check if the FDT passed by the bootloader is valid
  2017-04-19 17:13 [PATCH for-4.9 0/4] xen/arm: Properly map the FDT in the boot page table Julien Grall
  2017-04-19 17:13 ` [PATCH for-4.9 1/4] xen/arm: Add BOOT_FDT_VIRT_END and BOOT_FDT_SLOT_SIZE Julien Grall
  2017-04-19 17:13 ` [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C Julien Grall
@ 2017-04-19 17:13 ` Julien Grall
  2017-04-19 21:01   ` Stefano Stabellini
  2017-04-19 17:13 ` [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table Julien Grall
  3 siblings, 1 reply; 17+ messages in thread
From: Julien Grall @ 2017-04-19 17:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, sstabellini

There is currently no sanity check on the FDT passed by the bootloader.
Whilst they are stricly not necessary, it will avoid us to spend hours
to try to find out why it does not work.

From the booting documentation for AArch32 [1] and AArch64 [2] must :
    - be placed on 8-byte boundary
    - not exceed 2MB (only on AArch64)

Even if AArch32 does not seem to limit the size, Xen is not currently
able to support more the 2MB FDT. It is better to crash rather with a nice
error message than claiming we are supporting any size of FDT.

The checks are mostly borrowed from the Linux code (see fixmap_remap_fdt
in arch/arm64/mm/mmu.c).

[1] Section 2 in linux/Documentation/arm64/booting.txt
[2] Section 4b in linux/Documentation/arm/Booting

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c           | 29 ++++++++++++++++++++++++++++-
 xen/arch/arm/setup.c        |  6 ++++++
 xen/include/asm-arm/setup.h |  3 +++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 0d076489c6..53d36e2ce2 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -39,6 +39,8 @@
 #include <xsm/xsm.h>
 #include <xen/pfn.h>
 #include <xen/sizes.h>
+#include <xen/libfdt/libfdt.h>
+#include <asm/setup.h>
 
 static void __init create_mappings(lpae_t *second,
                                    unsigned long virt_offset,
@@ -447,11 +449,36 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
 {
     /* We are using 2MB superpage for mapping the FDT */
     paddr_t base_paddr = fdt_paddr & SECOND_MASK;
+    paddr_t offset;
+    void *fdt_virt;
+
+    /*
+     * Check whether the physical FDT address is set and meets the minimum
+     * alignment requirement. Since we are relying on MIN_FDT_ALIGN to be at
+     * least 8 bytes so that we always access the magic and size fields
+     * of the FDT header after mapping the first chunk, double check if
+     * that is indeed the case.
+     */
+    BUILD_BUG_ON(MIN_FDT_ALIGN < 8);
+    if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN )
+        return NULL;
+
+    /* The FDT is mapped using 2MB superpage */
+    BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
 
     create_mappings(boot_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
                     SZ_2M >> PAGE_SHIFT, SZ_2M);
 
-    return (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE);
+    offset = fdt_paddr % SECOND_SIZE;
+    fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;
+
+    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
+        return NULL;
+
+    if ( fdt_totalsize(fdt_virt) > MAX_FDT_SIZE )
+        return NULL;
+
+    return fdt_virt;
 }
 
 void __init remove_early_mappings(void)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 986398970f..8f72f31fb5 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -725,6 +725,12 @@ void __init start_xen(unsigned long boot_phys_offset,
     smp_clear_cpu_maps();
 
     device_tree_flattened = early_fdt_map(fdt_paddr);
+    if ( !device_tree_flattened )
+        panic("Invalid device tree blob at physical address %#lx\n"
+              "The DTB must be 8-byte aligned and must not exceed 2 MB in size\n"
+              "\nPlease check your bootloader.",
+              fdt_paddr);
+
     fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
 
     cmdline = boot_fdt_cmdline(device_tree_flattened);
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 7c761851d2..7ff2c34dab 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -3,6 +3,9 @@
 
 #include <public/version.h>
 
+#define MIN_FDT_ALIGN 8
+#define MAX_FDT_SIZE SZ_2M
+
 #define NR_MEM_BANKS 64
 
 #define MAX_MODULES 5 /* Current maximum useful modules */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table
  2017-04-19 17:13 [PATCH for-4.9 0/4] xen/arm: Properly map the FDT in the boot page table Julien Grall
                   ` (2 preceding siblings ...)
  2017-04-19 17:13 ` [PATCH for-4.9 3/4] xen/arm: Check if the FDT passed by the bootloader is valid Julien Grall
@ 2017-04-19 17:13 ` Julien Grall
  2017-04-19 21:01   ` Stefano Stabellini
  3 siblings, 1 reply; 17+ messages in thread
From: Julien Grall @ 2017-04-19 17:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, sstabellini

Currently, Xen is assuming the FDT will always fit in a 2MB section.
Recently, I noticed an early crash on Xen when using GRUB with the
following call trace:

(XEN) Hypervisor Trap. HSR=0x96000006 EC=0x25 IL=1 Syndrome=0x6
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) ----[ Xen-4.9-unstable  arm64  debug=y   Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     0000000000264140 strlen+0x10/0x84
(XEN) LR:     00000000002401c0
(XEN) SP:     00000000002cfc20
(XEN) CPSR:   400003c9 MODE:64-bit EL2h (Hypervisor, handler)
(XEN)      X0: 0000000000801230  X1: 0000000000801230  X2: 0000000000005230
(XEN)      X3: 0000000000000030  X4: 0000000000000030  X5: 0000000000000038
(XEN)      X6: 0000000000000034  X7: 0000000000000000  X8: 7f7f7f7f7f7f7f7f
(XEN)      X9: 64622c6479687222 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
(XEN)     X12: 0000000000000030 X13: ffffff00ff000000 X14: 0800000003000000
(XEN)     X15: ffffffffffffffff X16: 00000000fefff610 X17: 00000000000000f0
(XEN)     X18: 0000000000000004 X19: 0000000000000008 X20: 00000000007fc040
(XEN)     X21: 00000000007fc000 X22: 000000000000000e X23: 0000000000000000
(XEN)     X24: 00000000002a9f58 X25: 0000000000801230 X26: 00000000002a9f68
(XEN)     X27: 00000000002a9f58 X28: 0000000000298910  FP: 00000000002cfc20
(XEN)
(XEN)   VTCR_EL2: 80010c40
(XEN)  VTTBR_EL2: 0000082800203000
(XEN)
(XEN)  SCTLR_EL2: 30c5183d
(XEN)    HCR_EL2: 000000000038663f
(XEN)  TTBR0_EL2: 00000000f4912000
(XEN)
(XEN)    ESR_EL2: 96000006
(XEN)  HPFAR_EL2: 00000000e8071000
(XEN)    FAR_EL2: 0000000000801230
(XEN)
(XEN) Xen stack trace from sp=00000000002cfc20:
(XEN)    00000000002cfc70 0000000000240254 00000000002a9f58 00000000007fc000
(XEN)    0000000000000000 0000000000000000 0000000000000000 00000000007fc03c
(XEN)    00000000002cfd78 0000000000000000 00000000002cfca0 00000000002986fc
(XEN)    0000000000000000 00000000007fc000 0000000000000000 0000000000000000
(XEN)    00000000002cfcc0 0000000000298f1c 0000000000000000 00000000007fc000
(XEN)    00000000002cfdc0 000000000029904c 00000000f47fc000 00000000f4604000
(XEN)    00000000f47fc000 00000000007fc000 0000000000400000 0000000000000100
(XEN)    00000000f4604000 0000000000000001 0000000000000001 8000000000000002
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 00000000002cfdc0 0000000000299038
(XEN)    00000000f47fc000 00000000f4604000 00000000f47fc000 0000000000000000
(XEN)    00000000002cfe20 000000000029c420 00000000002d8000 00000000f4604000
(XEN)    00000000f47fc000 0000000000000000 0000000000400000 0000000000000100
(XEN)    00000000f4604000 0000000000000001 00000000f47fc000 000000000029c404
(XEN)    00000000fefff510 0000000000200624 00000000f4804000 00000000f4604000
(XEN)    00000000f47fc000 0000000000000000 0000000000400000 0000000000000100
(XEN)    0000000000000001 0000000000000001 0000000000000001 8000000000000002
(XEN)    00000000f47fc000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<0000000000264140>] strlen+0x10/0x84 (PC)
(XEN)    [<00000000002401c0>] fdt_get_property_namelen+0x9c/0xf0 (LR)
(XEN)    [<0000000000240254>] fdt_get_property+0x40/0x50
(XEN)    [<00000000002986fc>] bootfdt.c#device_tree_get_u32+0x18/0x5c
(XEN)    [<0000000000298f1c>] device_tree_for_each_node+0x84/0x144
(XEN)    [<000000000029904c>] boot_fdt_info+0x70/0x23c
(XEN)    [<000000000029c420>] start_xen+0x9c/0xd30
(XEN)    [<0000000000200624>] arm64/head.o#paging+0x84/0xbc
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN)
(XEN) ****************************************

Indeed, the booting documentation for AArch32 and AArch64 only requires
the FDT to be placed on a 8-byte boundary. This means the Device-Tree can
cross a 2MB boundary.

Given that Xen limits the size of the FDT to 2MB, it will always fit in
a 4MB slot. So extend the fixmap slot for FDT from 2MB to 4MB.

The second 2MB superpage will only be mapped if the FDT is cross the 2MB
boundary.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c            | 16 ++++++++++++++--
 xen/include/asm-arm/config.h | 14 +++++++-------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 53d36e2ce2..9cff1619c6 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -451,6 +451,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
     paddr_t base_paddr = fdt_paddr & SECOND_MASK;
     paddr_t offset;
     void *fdt_virt;
+    uint32_t size;
 
     /*
      * Check whether the physical FDT address is set and meets the minimum
@@ -475,9 +476,17 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
     if ( fdt_magic(fdt_virt) != FDT_MAGIC )
         return NULL;
 
-    if ( fdt_totalsize(fdt_virt) > MAX_FDT_SIZE )
+    size = fdt_totalsize(fdt_virt);
+    if ( size > MAX_FDT_SIZE )
         return NULL;
 
+    if ( (offset + size) > SZ_2M )
+    {
+        create_mappings(boot_second, BOOT_FDT_VIRT_START + SZ_2M,
+                        paddr_to_pfn(base_paddr + SZ_2M),
+                        SZ_2M >> PAGE_SHIFT, SZ_2M);
+    }
+
     return fdt_virt;
 }
 
@@ -485,7 +494,8 @@ void __init remove_early_mappings(void)
 {
     lpae_t pte = {0};
     write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
-    flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, SECOND_SIZE);
+    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
+    flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
 }
 
 extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
@@ -544,6 +554,8 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
     /* ... DTB */
     pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)];
     xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte;
+    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
+    xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
 
     /* ... Boot Misc area for xen relocation */
     dest_va = BOOT_RELOC_VIRT_START;
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 9c14a385e7..5b6f3c985d 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -77,12 +77,12 @@
  *   0  -   2M   Unmapped
  *   2M -   4M   Xen text, data, bss
  *   4M -   6M   Fixmap: special-purpose 4K mapping slots
- *   6M -   8M   Early boot mapping of FDT
- *   8M -  10M   Early relocation address (used when relocating Xen)
+ *   6M -  10M   Early boot mapping of FDT
+ *   10M - 12M   Early relocation address (used when relocating Xen)
  *               and later for livepatch vmap (if compiled in)
  *
  * ARM32 layout:
- *   0  -  10M   <COMMON>
+ *   0  -  12M   <COMMON>
  *
  *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
  * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
@@ -93,7 +93,7 @@
  *
  * ARM64 layout:
  * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
- *   0  -  10M   <COMMON>
+ *   0  -  12M   <COMMON>
  *
  *   1G -   2G   VMAP: ioremap and early_ioremap
  *
@@ -113,12 +113,12 @@
 #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
 
 #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
-#define BOOT_FDT_SLOT_SIZE     MB(2)
+#define BOOT_FDT_SLOT_SIZE     MB(4)
 #define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
 
-#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00800000)
+#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00a00000)
 #ifdef CONFIG_LIVEPATCH
-#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00800000)
+#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
 #define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
 #endif
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table
  2017-04-19 17:13 ` [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table Julien Grall
@ 2017-04-19 21:01   ` Stefano Stabellini
  2017-04-19 21:18     ` Julien Grall
  0 siblings, 1 reply; 17+ messages in thread
From: Stefano Stabellini @ 2017-04-19 21:01 UTC (permalink / raw)
  To: Julien Grall; +Cc: sstabellini, xen-devel

On Wed, 19 Apr 2017, Julien Grall wrote:
> Currently, Xen is assuming the FDT will always fit in a 2MB section.
> Recently, I noticed an early crash on Xen when using GRUB with the
> following call trace:
> 
> (XEN) Hypervisor Trap. HSR=0x96000006 EC=0x25 IL=1 Syndrome=0x6
> (XEN) CPU0: Unexpected Trap: Hypervisor
> (XEN) ----[ Xen-4.9-unstable  arm64  debug=y   Not tainted ]----
> (XEN) CPU:    0
> (XEN) PC:     0000000000264140 strlen+0x10/0x84
> (XEN) LR:     00000000002401c0
> (XEN) SP:     00000000002cfc20
> (XEN) CPSR:   400003c9 MODE:64-bit EL2h (Hypervisor, handler)
> (XEN)      X0: 0000000000801230  X1: 0000000000801230  X2: 0000000000005230
> (XEN)      X3: 0000000000000030  X4: 0000000000000030  X5: 0000000000000038
> (XEN)      X6: 0000000000000034  X7: 0000000000000000  X8: 7f7f7f7f7f7f7f7f
> (XEN)      X9: 64622c6479687222 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
> (XEN)     X12: 0000000000000030 X13: ffffff00ff000000 X14: 0800000003000000
> (XEN)     X15: ffffffffffffffff X16: 00000000fefff610 X17: 00000000000000f0
> (XEN)     X18: 0000000000000004 X19: 0000000000000008 X20: 00000000007fc040
> (XEN)     X21: 00000000007fc000 X22: 000000000000000e X23: 0000000000000000
> (XEN)     X24: 00000000002a9f58 X25: 0000000000801230 X26: 00000000002a9f68
> (XEN)     X27: 00000000002a9f58 X28: 0000000000298910  FP: 00000000002cfc20
> (XEN)
> (XEN)   VTCR_EL2: 80010c40
> (XEN)  VTTBR_EL2: 0000082800203000
> (XEN)
> (XEN)  SCTLR_EL2: 30c5183d
> (XEN)    HCR_EL2: 000000000038663f
> (XEN)  TTBR0_EL2: 00000000f4912000
> (XEN)
> (XEN)    ESR_EL2: 96000006
> (XEN)  HPFAR_EL2: 00000000e8071000
> (XEN)    FAR_EL2: 0000000000801230
> (XEN)
> (XEN) Xen stack trace from sp=00000000002cfc20:
> (XEN)    00000000002cfc70 0000000000240254 00000000002a9f58 00000000007fc000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 00000000007fc03c
> (XEN)    00000000002cfd78 0000000000000000 00000000002cfca0 00000000002986fc
> (XEN)    0000000000000000 00000000007fc000 0000000000000000 0000000000000000
> (XEN)    00000000002cfcc0 0000000000298f1c 0000000000000000 00000000007fc000
> (XEN)    00000000002cfdc0 000000000029904c 00000000f47fc000 00000000f4604000
> (XEN)    00000000f47fc000 00000000007fc000 0000000000400000 0000000000000100
> (XEN)    00000000f4604000 0000000000000001 0000000000000001 8000000000000002
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 00000000002cfdc0 0000000000299038
> (XEN)    00000000f47fc000 00000000f4604000 00000000f47fc000 0000000000000000
> (XEN)    00000000002cfe20 000000000029c420 00000000002d8000 00000000f4604000
> (XEN)    00000000f47fc000 0000000000000000 0000000000400000 0000000000000100
> (XEN)    00000000f4604000 0000000000000001 00000000f47fc000 000000000029c404
> (XEN)    00000000fefff510 0000000000200624 00000000f4804000 00000000f4604000
> (XEN)    00000000f47fc000 0000000000000000 0000000000400000 0000000000000100
> (XEN)    0000000000000001 0000000000000001 0000000000000001 8000000000000002
> (XEN)    00000000f47fc000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) Xen call trace:
> (XEN)    [<0000000000264140>] strlen+0x10/0x84 (PC)
> (XEN)    [<00000000002401c0>] fdt_get_property_namelen+0x9c/0xf0 (LR)
> (XEN)    [<0000000000240254>] fdt_get_property+0x40/0x50
> (XEN)    [<00000000002986fc>] bootfdt.c#device_tree_get_u32+0x18/0x5c
> (XEN)    [<0000000000298f1c>] device_tree_for_each_node+0x84/0x144
> (XEN)    [<000000000029904c>] boot_fdt_info+0x70/0x23c
> (XEN)    [<000000000029c420>] start_xen+0x9c/0xd30
> (XEN)    [<0000000000200624>] arm64/head.o#paging+0x84/0xbc
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) CPU0: Unexpected Trap: Hypervisor
> (XEN)
> (XEN) ****************************************
> 
> Indeed, the booting documentation for AArch32 and AArch64 only requires
> the FDT to be placed on a 8-byte boundary. This means the Device-Tree can
> cross a 2MB boundary.
> 
> Given that Xen limits the size of the FDT to 2MB, it will always fit in
> a 4MB slot. So extend the fixmap slot for FDT from 2MB to 4MB.
> 
> The second 2MB superpage will only be mapped if the FDT is cross the 2MB
> boundary.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mm.c            | 16 ++++++++++++++--
>  xen/include/asm-arm/config.h | 14 +++++++-------
>  2 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 53d36e2ce2..9cff1619c6 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -451,6 +451,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>      paddr_t base_paddr = fdt_paddr & SECOND_MASK;
>      paddr_t offset;
>      void *fdt_virt;
> +    uint32_t size;
>  
>      /*
>       * Check whether the physical FDT address is set and meets the minimum
> @@ -475,9 +476,17 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>      if ( fdt_magic(fdt_virt) != FDT_MAGIC )
>          return NULL;
>  
> -    if ( fdt_totalsize(fdt_virt) > MAX_FDT_SIZE )
> +    size = fdt_totalsize(fdt_virt);
> +    if ( size > MAX_FDT_SIZE )
>          return NULL;
>  
> +    if ( (offset + size) > SZ_2M )
> +    {
> +        create_mappings(boot_second, BOOT_FDT_VIRT_START + SZ_2M,
> +                        paddr_to_pfn(base_paddr + SZ_2M),
> +                        SZ_2M >> PAGE_SHIFT, SZ_2M);
> +    }
> +
>      return fdt_virt;
>  }
>  
> @@ -485,7 +494,8 @@ void __init remove_early_mappings(void)
>  {
>      lpae_t pte = {0};
>      write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
> -    flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, SECOND_SIZE);
> +    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);

Aren't you writing the same pte twice at the same address?


> +    flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
>  }
>  
>  extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
> @@ -544,6 +554,8 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>      /* ... DTB */
>      pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)];
>      xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte;
> +    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
> +    xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
>  
>      /* ... Boot Misc area for xen relocation */
>      dest_va = BOOT_RELOC_VIRT_START;
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index 9c14a385e7..5b6f3c985d 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -77,12 +77,12 @@
>   *   0  -   2M   Unmapped
>   *   2M -   4M   Xen text, data, bss
>   *   4M -   6M   Fixmap: special-purpose 4K mapping slots
> - *   6M -   8M   Early boot mapping of FDT
> - *   8M -  10M   Early relocation address (used when relocating Xen)
> + *   6M -  10M   Early boot mapping of FDT
> + *   10M - 12M   Early relocation address (used when relocating Xen)
>   *               and later for livepatch vmap (if compiled in)
>   *
>   * ARM32 layout:
> - *   0  -  10M   <COMMON>
> + *   0  -  12M   <COMMON>
>   *
>   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
> @@ -93,7 +93,7 @@
>   *
>   * ARM64 layout:
>   * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
> - *   0  -  10M   <COMMON>
> + *   0  -  12M   <COMMON>
>   *
>   *   1G -   2G   VMAP: ioremap and early_ioremap
>   *
> @@ -113,12 +113,12 @@
>  #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>  
>  #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
> -#define BOOT_FDT_SLOT_SIZE     MB(2)
> +#define BOOT_FDT_SLOT_SIZE     MB(4)
>  #define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
>  
> -#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00800000)
> +#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00a00000)

Wouldn't it be better to define it as BOOT_FDT_VIRT_END?


>  #ifdef CONFIG_LIVEPATCH
> -#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00800000)
> +#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)

same here


>  #define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
>  #endif
>  
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 3/4] xen/arm: Check if the FDT passed by the bootloader is valid
  2017-04-19 17:13 ` [PATCH for-4.9 3/4] xen/arm: Check if the FDT passed by the bootloader is valid Julien Grall
@ 2017-04-19 21:01   ` Stefano Stabellini
  2017-04-19 21:14     ` Julien Grall
  0 siblings, 1 reply; 17+ messages in thread
From: Stefano Stabellini @ 2017-04-19 21:01 UTC (permalink / raw)
  To: Julien Grall; +Cc: sstabellini, xen-devel

On Wed, 19 Apr 2017, Julien Grall wrote:
> There is currently no sanity check on the FDT passed by the bootloader.
> Whilst they are stricly not necessary, it will avoid us to spend hours
> to try to find out why it does not work.
> 
> >From the booting documentation for AArch32 [1] and AArch64 [2] must :
>     - be placed on 8-byte boundary
>     - not exceed 2MB (only on AArch64)
> 
> Even if AArch32 does not seem to limit the size, Xen is not currently
> able to support more the 2MB FDT. It is better to crash rather with a nice
> error message than claiming we are supporting any size of FDT.
> 
> The checks are mostly borrowed from the Linux code (see fixmap_remap_fdt
> in arch/arm64/mm/mmu.c).
> 
> [1] Section 2 in linux/Documentation/arm64/booting.txt
> [2] Section 4b in linux/Documentation/arm/Booting
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mm.c           | 29 ++++++++++++++++++++++++++++-
>  xen/arch/arm/setup.c        |  6 ++++++
>  xen/include/asm-arm/setup.h |  3 +++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0d076489c6..53d36e2ce2 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -39,6 +39,8 @@
>  #include <xsm/xsm.h>
>  #include <xen/pfn.h>
>  #include <xen/sizes.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <asm/setup.h>
>  
>  static void __init create_mappings(lpae_t *second,
>                                     unsigned long virt_offset,
> @@ -447,11 +449,36 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>  {
>      /* We are using 2MB superpage for mapping the FDT */
>      paddr_t base_paddr = fdt_paddr & SECOND_MASK;
> +    paddr_t offset;
> +    void *fdt_virt;
> +
> +    /*
> +     * Check whether the physical FDT address is set and meets the minimum
> +     * alignment requirement. Since we are relying on MIN_FDT_ALIGN to be at
> +     * least 8 bytes so that we always access the magic and size fields
> +     * of the FDT header after mapping the first chunk, double check if
> +     * that is indeed the case.
> +     */
> +    BUILD_BUG_ON(MIN_FDT_ALIGN < 8);

I think that this BUILD_BUG_ON is overkill, a comment would be enough


> +    if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN )
> +        return NULL;
> +
> +    /* The FDT is mapped using 2MB superpage */
> +    BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);

Same here.


>      create_mappings(boot_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
>                      SZ_2M >> PAGE_SHIFT, SZ_2M);
>  
> -    return (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE);
> +    offset = fdt_paddr % SECOND_SIZE;
> +    fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;

offset is useless, you can just:

      fdt_virt = (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE);


> +    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> +        return NULL;
> +
> +    if ( fdt_totalsize(fdt_virt) > MAX_FDT_SIZE )
> +        return NULL;
> +
> +    return fdt_virt;
>  }
>  
>  void __init remove_early_mappings(void)
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 986398970f..8f72f31fb5 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -725,6 +725,12 @@ void __init start_xen(unsigned long boot_phys_offset,
>      smp_clear_cpu_maps();
>  
>      device_tree_flattened = early_fdt_map(fdt_paddr);
> +    if ( !device_tree_flattened )
> +        panic("Invalid device tree blob at physical address %#lx\n"
> +              "The DTB must be 8-byte aligned and must not exceed 2 MB in size\n"
> +              "\nPlease check your bootloader.",
> +              fdt_paddr);

Strange, why did you place the "\n" at the beginning of the line?

These are all minor stylistic nits, so

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


>      fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
>  
>      cmdline = boot_fdt_cmdline(device_tree_flattened);
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 7c761851d2..7ff2c34dab 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -3,6 +3,9 @@
>  
>  #include <public/version.h>
>  
> +#define MIN_FDT_ALIGN 8
> +#define MAX_FDT_SIZE SZ_2M
> +
>  #define NR_MEM_BANKS 64
>  
>  #define MAX_MODULES 5 /* Current maximum useful modules */
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C
  2017-04-19 17:13 ` [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C Julien Grall
@ 2017-04-19 21:01   ` Stefano Stabellini
  2017-04-19 21:12     ` Julien Grall
  0 siblings, 1 reply; 17+ messages in thread
From: Stefano Stabellini @ 2017-04-19 21:01 UTC (permalink / raw)
  To: Julien Grall; +Cc: sstabellini, xen-devel

On Wed, 19 Apr 2017, Julien Grall wrote:
> The FDT will not be accessed before start_xen (begining of C code) is
> called and it will be easier to maintain as the code could be common
> between AArch32 and AArch64.
> 
> A new function early_fdt_map is introduced to map the FDT in the boot
> page table.
> 
> Note that create_mapping will flush the TLBs for us so no need to add an
> extra on in case the entry was previously used by the 1:1 mapping.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
>     I can move the function create_mappings at the beginning of the
>     function instead of adding a static declaration. But I thought it
>     was not Xen 4.9 material. Any opinions?
> ---
>  xen/arch/arm/arm32/head.S | 14 --------------
>  xen/arch/arm/arm64/head.S | 13 -------------
>  xen/arch/arm/mm.c         | 20 ++++++++++++++++++++
>  xen/arch/arm/setup.c      |  4 +---
>  xen/include/asm-arm/mm.h  |  2 ++
>  5 files changed, 23 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index ec63ba4c04..4090f4a744 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -389,20 +389,6 @@ paging:
>          /* Use a virtual address to access the UART. */
>          ldr   r11, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> -        /* Map the DTB in the boot misc slot */
> -        teq   r12, #0                /* Only on boot CPU */
> -        bne   1f
> -
> -        ldr   r1, =boot_second
> -        mov   r3, #0x0
> -        lsr   r2, r8, #SECOND_SHIFT
> -        lsl   r2, r2, #SECOND_SHIFT  /* r2: 2MB-aligned paddr of DTB */
> -        orr   r2, r2, #PT_UPPER(MEM)
> -        orr   r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */
> -        ldr   r4, =BOOT_FDT_VIRT_START
> -        mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* Slot for BOOT_FDT_VIRT_START */
> -        strd  r2, r3, [r1, r4]       /* Map it in the early fdt slot */
> -1:
>  
>          /*
>           * Flush the TLB in case the 1:1 mapping happens to clash with
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 72ea4e0233..78292f4396 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -550,19 +550,6 @@ paging:
>          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
>  
> -        /* Map the DTB in the boot misc slot */
> -        cbnz  x22, 1f                /* Only on boot CPU */
> -
> -        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> -        lsr   x2, x21, #SECOND_SHIFT
> -        lsl   x2, x2, #SECOND_SHIFT  /* x2 := 2MB-aligned paddr of DTB */
> -        mov   x3, #PT_MEM            /* x2 := 2MB RAM incl. DTB */
> -        orr   x2, x2, x3
> -        ldr   x1, =BOOT_FDT_VIRT_START
> -        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x4 := Slot for BOOT_FDT_VIRT_START */
> -        str   x2, [x4, x1]           /* Map it in the early fdt slot */
> -1:
> -
>          /*
>           * Flush the TLB in case the 1:1 mapping happens to clash with
>           * the virtual addresses used by the fixmap or DTB.
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index f0a2eddaaf..0d076489c6 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -38,6 +38,13 @@
>  #include <xen/vmap.h>
>  #include <xsm/xsm.h>
>  #include <xen/pfn.h>
> +#include <xen/sizes.h>
> +
> +static void __init create_mappings(lpae_t *second,
> +                                   unsigned long virt_offset,
> +                                   unsigned long base_mfn,
> +                                   unsigned long nr_mfns,
> +                                   unsigned int mapping_size);

I would have added early_fdt_map to this file in a way to avoid the need
for duplicating the declaration of create_mappings (because this version
doesn't have the useful comment on top).

In any case

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


>  struct domain *dom_xen, *dom_io, *dom_cow;
>  
> @@ -434,6 +441,19 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va)
>      return mfn_to_xen_entry(mfn, WRITEALLOC);
>  }
>  
> +
> +/* Map the FDT in the early boot page table */
> +void * __init early_fdt_map(paddr_t fdt_paddr)
> +{
> +    /* We are using 2MB superpage for mapping the FDT */
> +    paddr_t base_paddr = fdt_paddr & SECOND_MASK;
> +
> +    create_mappings(boot_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
> +                    SZ_2M >> PAGE_SHIFT, SZ_2M);
> +
> +    return (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE);
> +}
> +
>  void __init remove_early_mappings(void)
>  {
>      lpae_t pte = {0};
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 92a2de6b70..986398970f 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -724,9 +724,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      smp_clear_cpu_maps();
>  
> -    /* This is mapped by head.S */
> -    device_tree_flattened = (void *)BOOT_FDT_VIRT_START
> -        + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
> +    device_tree_flattened = early_fdt_map(fdt_paddr);
>      fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
>  
>      cmdline = boot_fdt_cmdline(device_tree_flattened);
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 0fef612f42..f6915ad882 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -160,6 +160,8 @@ extern unsigned long total_pages;
>  
>  /* Boot-time pagetable setup */
>  extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr);
> +/* Map FDT in boot pagetable */
> +extern void *early_fdt_map(paddr_t fdt_paddr);
>  /* Remove early mappings */
>  extern void remove_early_mappings(void);
>  /* Allocate and initialise pagetables for a secondary CPU. Sets init_ttbr to the
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 1/4] xen/arm: Add BOOT_FDT_VIRT_END and BOOT_FDT_SLOT_SIZE
  2017-04-19 17:13 ` [PATCH for-4.9 1/4] xen/arm: Add BOOT_FDT_VIRT_END and BOOT_FDT_SLOT_SIZE Julien Grall
@ 2017-04-19 21:01   ` Stefano Stabellini
  0 siblings, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2017-04-19 21:01 UTC (permalink / raw)
  To: Julien Grall; +Cc: sstabellini, xen-devel

On Wed, 19 Apr 2017, Julien Grall wrote:
> The 2 new defines will help to avoid hardcoding the size and the end of
> the slot in the code.
> 
> The newlines are added for clarity.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/include/asm-arm/config.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index b2edf95f72..9c14a385e7 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -111,7 +111,11 @@
>  
>  #define XEN_VIRT_START         _AT(vaddr_t,0x00200000)
>  #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
> +
>  #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
> +#define BOOT_FDT_SLOT_SIZE     MB(2)
> +#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
> +
>  #define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00800000)
>  #ifdef CONFIG_LIVEPATCH
>  #define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00800000)
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C
  2017-04-19 21:01   ` Stefano Stabellini
@ 2017-04-19 21:12     ` Julien Grall
  2017-04-19 21:18       ` Stefano Stabellini
  2017-04-19 21:32       ` Andrew Cooper
  0 siblings, 2 replies; 17+ messages in thread
From: Julien Grall @ 2017-04-19 21:12 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: nd, xen-devel

Hi Stefano,

On 19/04/2017 22:01, Stefano Stabellini wrote:
> On Wed, 19 Apr 2017, Julien Grall wrote:
>> The FDT will not be accessed before start_xen (begining of C code) is
>> called and it will be easier to maintain as the code could be common
>> between AArch32 and AArch64.
>>
>> A new function early_fdt_map is introduced to map the FDT in the boot
>> page table.
>>
>> Note that create_mapping will flush the TLBs for us so no need to add an
>> extra on in case the entry was previously used by the 1:1 mapping.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>
>>     I can move the function create_mappings at the beginning of the
>>     function instead of adding a static declaration. But I thought it
>>     was not Xen 4.9 material. Any opinions?
>> ---
>>  xen/arch/arm/arm32/head.S | 14 --------------
>>  xen/arch/arm/arm64/head.S | 13 -------------
>>  xen/arch/arm/mm.c         | 20 ++++++++++++++++++++
>>  xen/arch/arm/setup.c      |  4 +---
>>  xen/include/asm-arm/mm.h  |  2 ++
>>  5 files changed, 23 insertions(+), 30 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index ec63ba4c04..4090f4a744 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -389,20 +389,6 @@ paging:
>>          /* Use a virtual address to access the UART. */
>>          ldr   r11, =EARLY_UART_VIRTUAL_ADDRESS
>>  #endif
>> -        /* Map the DTB in the boot misc slot */
>> -        teq   r12, #0                /* Only on boot CPU */
>> -        bne   1f
>> -
>> -        ldr   r1, =boot_second
>> -        mov   r3, #0x0
>> -        lsr   r2, r8, #SECOND_SHIFT
>> -        lsl   r2, r2, #SECOND_SHIFT  /* r2: 2MB-aligned paddr of DTB */
>> -        orr   r2, r2, #PT_UPPER(MEM)
>> -        orr   r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */
>> -        ldr   r4, =BOOT_FDT_VIRT_START
>> -        mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* Slot for BOOT_FDT_VIRT_START */
>> -        strd  r2, r3, [r1, r4]       /* Map it in the early fdt slot */
>> -1:
>>
>>          /*
>>           * Flush the TLB in case the 1:1 mapping happens to clash with
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 72ea4e0233..78292f4396 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -550,19 +550,6 @@ paging:
>>          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>>  #endif
>>
>> -        /* Map the DTB in the boot misc slot */
>> -        cbnz  x22, 1f                /* Only on boot CPU */
>> -
>> -        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
>> -        lsr   x2, x21, #SECOND_SHIFT
>> -        lsl   x2, x2, #SECOND_SHIFT  /* x2 := 2MB-aligned paddr of DTB */
>> -        mov   x3, #PT_MEM            /* x2 := 2MB RAM incl. DTB */
>> -        orr   x2, x2, x3
>> -        ldr   x1, =BOOT_FDT_VIRT_START
>> -        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x4 := Slot for BOOT_FDT_VIRT_START */
>> -        str   x2, [x4, x1]           /* Map it in the early fdt slot */
>> -1:
>> -
>>          /*
>>           * Flush the TLB in case the 1:1 mapping happens to clash with
>>           * the virtual addresses used by the fixmap or DTB.
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index f0a2eddaaf..0d076489c6 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -38,6 +38,13 @@
>>  #include <xen/vmap.h>
>>  #include <xsm/xsm.h>
>>  #include <xen/pfn.h>
>> +#include <xen/sizes.h>
>> +
>> +static void __init create_mappings(lpae_t *second,
>> +                                   unsigned long virt_offset,
>> +                                   unsigned long base_mfn,
>> +                                   unsigned long nr_mfns,
>> +                                   unsigned int mapping_size);
>
> I would have added early_fdt_map to this file in a way to avoid the need
> for duplicating the declaration of create_mappings (because this version
> doesn't have the useful comment on top).

I wanted to keep the function close to the counterpart 
remove_early_mappings rather than adding somewhere that make less sense.

Hence why I suggested to move the create_mappings function. Would you be 
fine with code motion for Xen 4.9?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 3/4] xen/arm: Check if the FDT passed by the bootloader is valid
  2017-04-19 21:01   ` Stefano Stabellini
@ 2017-04-19 21:14     ` Julien Grall
  0 siblings, 0 replies; 17+ messages in thread
From: Julien Grall @ 2017-04-19 21:14 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: nd, xen-devel

Hi Stefano,

On 19/04/2017 22:01, Stefano Stabellini wrote:
> On Wed, 19 Apr 2017, Julien Grall wrote:
>> There is currently no sanity check on the FDT passed by the bootloader.
>> Whilst they are stricly not necessary, it will avoid us to spend hours
>> to try to find out why it does not work.
>>
>> >From the booting documentation for AArch32 [1] and AArch64 [2] must :
>>     - be placed on 8-byte boundary
>>     - not exceed 2MB (only on AArch64)
>>
>> Even if AArch32 does not seem to limit the size, Xen is not currently
>> able to support more the 2MB FDT. It is better to crash rather with a nice
>> error message than claiming we are supporting any size of FDT.
>>
>> The checks are mostly borrowed from the Linux code (see fixmap_remap_fdt
>> in arch/arm64/mm/mmu.c).
>>
>> [1] Section 2 in linux/Documentation/arm64/booting.txt
>> [2] Section 4b in linux/Documentation/arm/Booting
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/arm/mm.c           | 29 ++++++++++++++++++++++++++++-
>>  xen/arch/arm/setup.c        |  6 ++++++
>>  xen/include/asm-arm/setup.h |  3 +++
>>  3 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 0d076489c6..53d36e2ce2 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -39,6 +39,8 @@
>>  #include <xsm/xsm.h>
>>  #include <xen/pfn.h>
>>  #include <xen/sizes.h>
>> +#include <xen/libfdt/libfdt.h>
>> +#include <asm/setup.h>
>>
>>  static void __init create_mappings(lpae_t *second,
>>                                     unsigned long virt_offset,
>> @@ -447,11 +449,36 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>>  {
>>      /* We are using 2MB superpage for mapping the FDT */
>>      paddr_t base_paddr = fdt_paddr & SECOND_MASK;
>> +    paddr_t offset;
>> +    void *fdt_virt;
>> +
>> +    /*
>> +     * Check whether the physical FDT address is set and meets the minimum
>> +     * alignment requirement. Since we are relying on MIN_FDT_ALIGN to be at
>> +     * least 8 bytes so that we always access the magic and size fields
>> +     * of the FDT header after mapping the first chunk, double check if
>> +     * that is indeed the case.
>> +     */
>> +    BUILD_BUG_ON(MIN_FDT_ALIGN < 8);
>
> I think that this BUILD_BUG_ON is overkill, a comment would be enough
>
>
>> +    if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN )
>> +        return NULL;
>> +
>> +    /* The FDT is mapped using 2MB superpage */
>> +    BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
>
> Same here.

Whilst I agree the first one is not that useful, this one will catch any 
change in the memory memory map that will not keep BOOT_FDT_VIRT_START 
2MB aligned.

>
>
>>      create_mappings(boot_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
>>                      SZ_2M >> PAGE_SHIFT, SZ_2M);
>>
>> -    return (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE);
>> +    offset = fdt_paddr % SECOND_SIZE;
>> +    fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;
>
> offset is useless, you can just:

It will be useful later on and wanted to avoid too much code 
modification in later patch.

>
>       fdt_virt = (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE);
>
>
>> +    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
>> +        return NULL;
>> +
>> +    if ( fdt_totalsize(fdt_virt) > MAX_FDT_SIZE )
>> +        return NULL;
>> +
>> +    return fdt_virt;
>>  }
>>
>>  void __init remove_early_mappings(void)
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 986398970f..8f72f31fb5 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -725,6 +725,12 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      smp_clear_cpu_maps();
>>
>>      device_tree_flattened = early_fdt_map(fdt_paddr);
>> +    if ( !device_tree_flattened )
>> +        panic("Invalid device tree blob at physical address %#lx\n"
>> +              "The DTB must be 8-byte aligned and must not exceed 2 MB in size\n"
>> +              "\nPlease check your bootloader.",
>> +              fdt_paddr);
>
> Strange, why did you place the "\n" at the beginning of the line?

I blindly copied from the kernel. I can fix that.

>
> These are all minor stylistic nits, so
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
>
>>      fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
>>
>>      cmdline = boot_fdt_cmdline(device_tree_flattened);
>> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
>> index 7c761851d2..7ff2c34dab 100644
>> --- a/xen/include/asm-arm/setup.h
>> +++ b/xen/include/asm-arm/setup.h
>> @@ -3,6 +3,9 @@
>>
>>  #include <public/version.h>
>>
>> +#define MIN_FDT_ALIGN 8
>> +#define MAX_FDT_SIZE SZ_2M
>> +
>>  #define NR_MEM_BANKS 64
>>
>>  #define MAX_MODULES 5 /* Current maximum useful modules */
>> --
>> 2.11.0
>>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C
  2017-04-19 21:12     ` Julien Grall
@ 2017-04-19 21:18       ` Stefano Stabellini
  2017-04-20 12:59         ` Julien Grall
  2017-04-19 21:32       ` Andrew Cooper
  1 sibling, 1 reply; 17+ messages in thread
From: Stefano Stabellini @ 2017-04-19 21:18 UTC (permalink / raw)
  To: Julien Grall; +Cc: nd, Stefano Stabellini, xen-devel

On Wed, 19 Apr 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 19/04/2017 22:01, Stefano Stabellini wrote:
> > On Wed, 19 Apr 2017, Julien Grall wrote:
> > > The FDT will not be accessed before start_xen (begining of C code) is
> > > called and it will be easier to maintain as the code could be common
> > > between AArch32 and AArch64.
> > > 
> > > A new function early_fdt_map is introduced to map the FDT in the boot
> > > page table.
> > > 
> > > Note that create_mapping will flush the TLBs for us so no need to add an
> > > extra on in case the entry was previously used by the 1:1 mapping.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > 
> > > ---
> > > 
> > >     I can move the function create_mappings at the beginning of the
> > >     function instead of adding a static declaration. But I thought it
> > >     was not Xen 4.9 material. Any opinions?
> > > ---
> > >  xen/arch/arm/arm32/head.S | 14 --------------
> > >  xen/arch/arm/arm64/head.S | 13 -------------
> > >  xen/arch/arm/mm.c         | 20 ++++++++++++++++++++
> > >  xen/arch/arm/setup.c      |  4 +---
> > >  xen/include/asm-arm/mm.h  |  2 ++
> > >  5 files changed, 23 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> > > index ec63ba4c04..4090f4a744 100644
> > > --- a/xen/arch/arm/arm32/head.S
> > > +++ b/xen/arch/arm/arm32/head.S
> > > @@ -389,20 +389,6 @@ paging:
> > >          /* Use a virtual address to access the UART. */
> > >          ldr   r11, =EARLY_UART_VIRTUAL_ADDRESS
> > >  #endif
> > > -        /* Map the DTB in the boot misc slot */
> > > -        teq   r12, #0                /* Only on boot CPU */
> > > -        bne   1f
> > > -
> > > -        ldr   r1, =boot_second
> > > -        mov   r3, #0x0
> > > -        lsr   r2, r8, #SECOND_SHIFT
> > > -        lsl   r2, r2, #SECOND_SHIFT  /* r2: 2MB-aligned paddr of DTB */
> > > -        orr   r2, r2, #PT_UPPER(MEM)
> > > -        orr   r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */
> > > -        ldr   r4, =BOOT_FDT_VIRT_START
> > > -        mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* Slot for
> > > BOOT_FDT_VIRT_START */
> > > -        strd  r2, r3, [r1, r4]       /* Map it in the early fdt slot */
> > > -1:
> > > 
> > >          /*
> > >           * Flush the TLB in case the 1:1 mapping happens to clash with
> > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > index 72ea4e0233..78292f4396 100644
> > > --- a/xen/arch/arm/arm64/head.S
> > > +++ b/xen/arch/arm/arm64/head.S
> > > @@ -550,19 +550,6 @@ paging:
> > >          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
> > >  #endif
> > > 
> > > -        /* Map the DTB in the boot misc slot */
> > > -        cbnz  x22, 1f                /* Only on boot CPU */
> > > -
> > > -        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> > > -        lsr   x2, x21, #SECOND_SHIFT
> > > -        lsl   x2, x2, #SECOND_SHIFT  /* x2 := 2MB-aligned paddr of DTB */
> > > -        mov   x3, #PT_MEM            /* x2 := 2MB RAM incl. DTB */
> > > -        orr   x2, x2, x3
> > > -        ldr   x1, =BOOT_FDT_VIRT_START
> > > -        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x4 := Slot for
> > > BOOT_FDT_VIRT_START */
> > > -        str   x2, [x4, x1]           /* Map it in the early fdt slot */
> > > -1:
> > > -
> > >          /*
> > >           * Flush the TLB in case the 1:1 mapping happens to clash with
> > >           * the virtual addresses used by the fixmap or DTB.
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index f0a2eddaaf..0d076489c6 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -38,6 +38,13 @@
> > >  #include <xen/vmap.h>
> > >  #include <xsm/xsm.h>
> > >  #include <xen/pfn.h>
> > > +#include <xen/sizes.h>
> > > +
> > > +static void __init create_mappings(lpae_t *second,
> > > +                                   unsigned long virt_offset,
> > > +                                   unsigned long base_mfn,
> > > +                                   unsigned long nr_mfns,
> > > +                                   unsigned int mapping_size);
> > 
> > I would have added early_fdt_map to this file in a way to avoid the need
> > for duplicating the declaration of create_mappings (because this version
> > doesn't have the useful comment on top).
> 
> I wanted to keep the function close to the counterpart remove_early_mappings
> rather than adding somewhere that make less sense.
> 
> Hence why I suggested to move the create_mappings function. Would you be fine
> with code motion for Xen 4.9?

Sure. But please keep the code motion in its own separate patch.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table
  2017-04-19 21:01   ` Stefano Stabellini
@ 2017-04-19 21:18     ` Julien Grall
  2017-04-19 21:56       ` Stefano Stabellini
  0 siblings, 1 reply; 17+ messages in thread
From: Julien Grall @ 2017-04-19 21:18 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: nd, xen-devel

Hi Stefano,

On 19/04/2017 22:01, Stefano Stabellini wrote:
> On Wed, 19 Apr 2017, Julien Grall wrote:
>> Currently, Xen is assuming the FDT will always fit in a 2MB section.
>> Recently, I noticed an early crash on Xen when using GRUB with the
>> following call trace:
>>
>> (XEN) Hypervisor Trap. HSR=0x96000006 EC=0x25 IL=1 Syndrome=0x6
>> (XEN) CPU0: Unexpected Trap: Hypervisor
>> (XEN) ----[ Xen-4.9-unstable  arm64  debug=y   Not tainted ]----
>> (XEN) CPU:    0
>> (XEN) PC:     0000000000264140 strlen+0x10/0x84
>> (XEN) LR:     00000000002401c0
>> (XEN) SP:     00000000002cfc20
>> (XEN) CPSR:   400003c9 MODE:64-bit EL2h (Hypervisor, handler)
>> (XEN)      X0: 0000000000801230  X1: 0000000000801230  X2: 0000000000005230
>> (XEN)      X3: 0000000000000030  X4: 0000000000000030  X5: 0000000000000038
>> (XEN)      X6: 0000000000000034  X7: 0000000000000000  X8: 7f7f7f7f7f7f7f7f
>> (XEN)      X9: 64622c6479687222 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
>> (XEN)     X12: 0000000000000030 X13: ffffff00ff000000 X14: 0800000003000000
>> (XEN)     X15: ffffffffffffffff X16: 00000000fefff610 X17: 00000000000000f0
>> (XEN)     X18: 0000000000000004 X19: 0000000000000008 X20: 00000000007fc040
>> (XEN)     X21: 00000000007fc000 X22: 000000000000000e X23: 0000000000000000
>> (XEN)     X24: 00000000002a9f58 X25: 0000000000801230 X26: 00000000002a9f68
>> (XEN)     X27: 00000000002a9f58 X28: 0000000000298910  FP: 00000000002cfc20
>> (XEN)
>> (XEN)   VTCR_EL2: 80010c40
>> (XEN)  VTTBR_EL2: 0000082800203000
>> (XEN)
>> (XEN)  SCTLR_EL2: 30c5183d
>> (XEN)    HCR_EL2: 000000000038663f
>> (XEN)  TTBR0_EL2: 00000000f4912000
>> (XEN)
>> (XEN)    ESR_EL2: 96000006
>> (XEN)  HPFAR_EL2: 00000000e8071000
>> (XEN)    FAR_EL2: 0000000000801230
>> (XEN)
>> (XEN) Xen stack trace from sp=00000000002cfc20:
>> (XEN)    00000000002cfc70 0000000000240254 00000000002a9f58 00000000007fc000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 00000000007fc03c
>> (XEN)    00000000002cfd78 0000000000000000 00000000002cfca0 00000000002986fc
>> (XEN)    0000000000000000 00000000007fc000 0000000000000000 0000000000000000
>> (XEN)    00000000002cfcc0 0000000000298f1c 0000000000000000 00000000007fc000
>> (XEN)    00000000002cfdc0 000000000029904c 00000000f47fc000 00000000f4604000
>> (XEN)    00000000f47fc000 00000000007fc000 0000000000400000 0000000000000100
>> (XEN)    00000000f4604000 0000000000000001 0000000000000001 8000000000000002
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 00000000002cfdc0 0000000000299038
>> (XEN)    00000000f47fc000 00000000f4604000 00000000f47fc000 0000000000000000
>> (XEN)    00000000002cfe20 000000000029c420 00000000002d8000 00000000f4604000
>> (XEN)    00000000f47fc000 0000000000000000 0000000000400000 0000000000000100
>> (XEN)    00000000f4604000 0000000000000001 00000000f47fc000 000000000029c404
>> (XEN)    00000000fefff510 0000000000200624 00000000f4804000 00000000f4604000
>> (XEN)    00000000f47fc000 0000000000000000 0000000000400000 0000000000000100
>> (XEN)    0000000000000001 0000000000000001 0000000000000001 8000000000000002
>> (XEN)    00000000f47fc000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN) Xen call trace:
>> (XEN)    [<0000000000264140>] strlen+0x10/0x84 (PC)
>> (XEN)    [<00000000002401c0>] fdt_get_property_namelen+0x9c/0xf0 (LR)
>> (XEN)    [<0000000000240254>] fdt_get_property+0x40/0x50
>> (XEN)    [<00000000002986fc>] bootfdt.c#device_tree_get_u32+0x18/0x5c
>> (XEN)    [<0000000000298f1c>] device_tree_for_each_node+0x84/0x144
>> (XEN)    [<000000000029904c>] boot_fdt_info+0x70/0x23c
>> (XEN)    [<000000000029c420>] start_xen+0x9c/0xd30
>> (XEN)    [<0000000000200624>] arm64/head.o#paging+0x84/0xbc
>> (XEN)
>> (XEN)
>> (XEN) ****************************************
>> (XEN) Panic on CPU 0:
>> (XEN) CPU0: Unexpected Trap: Hypervisor
>> (XEN)
>> (XEN) ****************************************
>>
>> Indeed, the booting documentation for AArch32 and AArch64 only requires
>> the FDT to be placed on a 8-byte boundary. This means the Device-Tree can
>> cross a 2MB boundary.
>>
>> Given that Xen limits the size of the FDT to 2MB, it will always fit in
>> a 4MB slot. So extend the fixmap slot for FDT from 2MB to 4MB.
>>
>> The second 2MB superpage will only be mapped if the FDT is cross the 2MB
>> boundary.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/arm/mm.c            | 16 ++++++++++++++--
>>  xen/include/asm-arm/config.h | 14 +++++++-------
>>  2 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 53d36e2ce2..9cff1619c6 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -451,6 +451,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>>      paddr_t base_paddr = fdt_paddr & SECOND_MASK;
>>      paddr_t offset;
>>      void *fdt_virt;
>> +    uint32_t size;
>>
>>      /*
>>       * Check whether the physical FDT address is set and meets the minimum
>> @@ -475,9 +476,17 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>>      if ( fdt_magic(fdt_virt) != FDT_MAGIC )
>>          return NULL;
>>
>> -    if ( fdt_totalsize(fdt_virt) > MAX_FDT_SIZE )
>> +    size = fdt_totalsize(fdt_virt);
>> +    if ( size > MAX_FDT_SIZE )
>>          return NULL;
>>
>> +    if ( (offset + size) > SZ_2M )
>> +    {
>> +        create_mappings(boot_second, BOOT_FDT_VIRT_START + SZ_2M,
>> +                        paddr_to_pfn(base_paddr + SZ_2M),
>> +                        SZ_2M >> PAGE_SHIFT, SZ_2M);
>> +    }
>> +
>>      return fdt_virt;
>>  }
>>
>> @@ -485,7 +494,8 @@ void __init remove_early_mappings(void)
>>  {
>>      lpae_t pte = {0};
>>      write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
>> -    flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, SECOND_SIZE);
>> +    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
>
> Aren't you writing the same pte twice at the same address?

It was meant to be BOOT_FDT_VIRT_START + SZ_2M as to remove the mapping 
from the table. I will send a new version with that fixed.

>
>
>> +    flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
>>  }
>>
>>  extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
>> @@ -544,6 +554,8 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>>      /* ... DTB */
>>      pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)];
>>      xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte;
>> +    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
>> +    xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
>>
>>      /* ... Boot Misc area for xen relocation */
>>      dest_va = BOOT_RELOC_VIRT_START;
>> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
>> index 9c14a385e7..5b6f3c985d 100644
>> --- a/xen/include/asm-arm/config.h
>> +++ b/xen/include/asm-arm/config.h
>> @@ -77,12 +77,12 @@
>>   *   0  -   2M   Unmapped
>>   *   2M -   4M   Xen text, data, bss
>>   *   4M -   6M   Fixmap: special-purpose 4K mapping slots
>> - *   6M -   8M   Early boot mapping of FDT
>> - *   8M -  10M   Early relocation address (used when relocating Xen)
>> + *   6M -  10M   Early boot mapping of FDT
>> + *   10M - 12M   Early relocation address (used when relocating Xen)
>>   *               and later for livepatch vmap (if compiled in)
>>   *
>>   * ARM32 layout:
>> - *   0  -  10M   <COMMON>
>> + *   0  -  12M   <COMMON>
>>   *
>>   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>>   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>> @@ -93,7 +93,7 @@
>>   *
>>   * ARM64 layout:
>>   * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
>> - *   0  -  10M   <COMMON>
>> + *   0  -  12M   <COMMON>
>>   *
>>   *   1G -   2G   VMAP: ioremap and early_ioremap
>>   *
>> @@ -113,12 +113,12 @@
>>  #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>>
>>  #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
>> -#define BOOT_FDT_SLOT_SIZE     MB(2)
>> +#define BOOT_FDT_SLOT_SIZE     MB(4)
>>  #define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
>>
>> -#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00800000)
>> +#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00a00000)
>
> Wouldn't it be better to define it as BOOT_FDT_VIRT_END?

It is kind of not really related to this patch and how we handle all the 
region in config.h today. So if we define in term of *_END this one, 
then we need to do it for everyone.

Also, BOOT_RELOC_VIRT_END is not defined. IHMO, these changes are post 
Xen 4.9 materials.

>
>
>>  #ifdef CONFIG_LIVEPATCH
>> -#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00800000)
>> +#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
>
> same here
>
>
>>  #define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
>>  #endif
>>
>> --
>> 2.11.0
>>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C
  2017-04-19 21:12     ` Julien Grall
  2017-04-19 21:18       ` Stefano Stabellini
@ 2017-04-19 21:32       ` Andrew Cooper
  1 sibling, 0 replies; 17+ messages in thread
From: Andrew Cooper @ 2017-04-19 21:32 UTC (permalink / raw)
  To: Julien Grall, Stefano Stabellini; +Cc: nd, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 734 bytes --]

On 19/04/2017 22:12, Julien Grall wrote:
> Would you be fine with code motion for Xen 4.9?

The only important question is whether the patch/series is suitable
material for 4.9.  By the looks of this series, it most certainly is
(and at the end of the day, you have final say on this matter).

Other than that, the modification should conform to the normal
guidelines (break different logical changes apart into different
patches, don't skimp on style, etc).

What matters (moreso during a stabilisation period) is review-ability of
the changes, not the quantity of patches.  I'd have no qualms submitting
a 25 patch series for 4.9, if I decided the bugfix was important enough
and 25 patches is what it took to do sensibly.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 1295 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table
  2017-04-19 21:18     ` Julien Grall
@ 2017-04-19 21:56       ` Stefano Stabellini
  2017-04-20 14:31         ` Julien Grall
  0 siblings, 1 reply; 17+ messages in thread
From: Stefano Stabellini @ 2017-04-19 21:56 UTC (permalink / raw)
  To: Julien Grall; +Cc: nd, Stefano Stabellini, xen-devel

On Wed, 19 Apr 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 19/04/2017 22:01, Stefano Stabellini wrote:
> > On Wed, 19 Apr 2017, Julien Grall wrote:
> > > Currently, Xen is assuming the FDT will always fit in a 2MB section.
> > > Recently, I noticed an early crash on Xen when using GRUB with the
> > > following call trace:
> > > 
> > > (XEN) Hypervisor Trap. HSR=0x96000006 EC=0x25 IL=1 Syndrome=0x6
> > > (XEN) CPU0: Unexpected Trap: Hypervisor
> > > (XEN) ----[ Xen-4.9-unstable  arm64  debug=y   Not tainted ]----
> > > (XEN) CPU:    0
> > > (XEN) PC:     0000000000264140 strlen+0x10/0x84
> > > (XEN) LR:     00000000002401c0
> > > (XEN) SP:     00000000002cfc20
> > > (XEN) CPSR:   400003c9 MODE:64-bit EL2h (Hypervisor, handler)
> > > (XEN)      X0: 0000000000801230  X1: 0000000000801230  X2:
> > > 0000000000005230
> > > (XEN)      X3: 0000000000000030  X4: 0000000000000030  X5:
> > > 0000000000000038
> > > (XEN)      X6: 0000000000000034  X7: 0000000000000000  X8:
> > > 7f7f7f7f7f7f7f7f
> > > (XEN)      X9: 64622c6479687222 X10: 7f7f7f7f7f7f7f7f X11:
> > > 0101010101010101
> > > (XEN)     X12: 0000000000000030 X13: ffffff00ff000000 X14:
> > > 0800000003000000
> > > (XEN)     X15: ffffffffffffffff X16: 00000000fefff610 X17:
> > > 00000000000000f0
> > > (XEN)     X18: 0000000000000004 X19: 0000000000000008 X20:
> > > 00000000007fc040
> > > (XEN)     X21: 00000000007fc000 X22: 000000000000000e X23:
> > > 0000000000000000
> > > (XEN)     X24: 00000000002a9f58 X25: 0000000000801230 X26:
> > > 00000000002a9f68
> > > (XEN)     X27: 00000000002a9f58 X28: 0000000000298910  FP:
> > > 00000000002cfc20
> > > (XEN)
> > > (XEN)   VTCR_EL2: 80010c40
> > > (XEN)  VTTBR_EL2: 0000082800203000
> > > (XEN)
> > > (XEN)  SCTLR_EL2: 30c5183d
> > > (XEN)    HCR_EL2: 000000000038663f
> > > (XEN)  TTBR0_EL2: 00000000f4912000
> > > (XEN)
> > > (XEN)    ESR_EL2: 96000006
> > > (XEN)  HPFAR_EL2: 00000000e8071000
> > > (XEN)    FAR_EL2: 0000000000801230
> > > (XEN)
> > > (XEN) Xen stack trace from sp=00000000002cfc20:
> > > (XEN)    00000000002cfc70 0000000000240254 00000000002a9f58
> > > 00000000007fc000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 00000000007fc03c
> > > (XEN)    00000000002cfd78 0000000000000000 00000000002cfca0
> > > 00000000002986fc
> > > (XEN)    0000000000000000 00000000007fc000 0000000000000000
> > > 0000000000000000
> > > (XEN)    00000000002cfcc0 0000000000298f1c 0000000000000000
> > > 00000000007fc000
> > > (XEN)    00000000002cfdc0 000000000029904c 00000000f47fc000
> > > 00000000f4604000
> > > (XEN)    00000000f47fc000 00000000007fc000 0000000000400000
> > > 0000000000000100
> > > (XEN)    00000000f4604000 0000000000000001 0000000000000001
> > > 8000000000000002
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 00000000002cfdc0
> > > 0000000000299038
> > > (XEN)    00000000f47fc000 00000000f4604000 00000000f47fc000
> > > 0000000000000000
> > > (XEN)    00000000002cfe20 000000000029c420 00000000002d8000
> > > 00000000f4604000
> > > (XEN)    00000000f47fc000 0000000000000000 0000000000400000
> > > 0000000000000100
> > > (XEN)    00000000f4604000 0000000000000001 00000000f47fc000
> > > 000000000029c404
> > > (XEN)    00000000fefff510 0000000000200624 00000000f4804000
> > > 00000000f4604000
> > > (XEN)    00000000f47fc000 0000000000000000 0000000000400000
> > > 0000000000000100
> > > (XEN)    0000000000000001 0000000000000001 0000000000000001
> > > 8000000000000002
> > > (XEN)    00000000f47fc000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN)    0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) Xen call trace:
> > > (XEN)    [<0000000000264140>] strlen+0x10/0x84 (PC)
> > > (XEN)    [<00000000002401c0>] fdt_get_property_namelen+0x9c/0xf0 (LR)
> > > (XEN)    [<0000000000240254>] fdt_get_property+0x40/0x50
> > > (XEN)    [<00000000002986fc>] bootfdt.c#device_tree_get_u32+0x18/0x5c
> > > (XEN)    [<0000000000298f1c>] device_tree_for_each_node+0x84/0x144
> > > (XEN)    [<000000000029904c>] boot_fdt_info+0x70/0x23c
> > > (XEN)    [<000000000029c420>] start_xen+0x9c/0xd30
> > > (XEN)    [<0000000000200624>] arm64/head.o#paging+0x84/0xbc
> > > (XEN)
> > > (XEN)
> > > (XEN) ****************************************
> > > (XEN) Panic on CPU 0:
> > > (XEN) CPU0: Unexpected Trap: Hypervisor
> > > (XEN)
> > > (XEN) ****************************************
> > > 
> > > Indeed, the booting documentation for AArch32 and AArch64 only requires
> > > the FDT to be placed on a 8-byte boundary. This means the Device-Tree can
> > > cross a 2MB boundary.
> > > 
> > > Given that Xen limits the size of the FDT to 2MB, it will always fit in
> > > a 4MB slot. So extend the fixmap slot for FDT from 2MB to 4MB.
> > > 
> > > The second 2MB superpage will only be mapped if the FDT is cross the 2MB
> > > boundary.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > ---
> > >  xen/arch/arm/mm.c            | 16 ++++++++++++++--
> > >  xen/include/asm-arm/config.h | 14 +++++++-------
> > >  2 files changed, 21 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index 53d36e2ce2..9cff1619c6 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -451,6 +451,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> > >      paddr_t base_paddr = fdt_paddr & SECOND_MASK;
> > >      paddr_t offset;
> > >      void *fdt_virt;
> > > +    uint32_t size;
> > > 
> > >      /*
> > >       * Check whether the physical FDT address is set and meets the
> > > minimum
> > > @@ -475,9 +476,17 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> > >      if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> > >          return NULL;
> > > 
> > > -    if ( fdt_totalsize(fdt_virt) > MAX_FDT_SIZE )
> > > +    size = fdt_totalsize(fdt_virt);
> > > +    if ( size > MAX_FDT_SIZE )
> > >          return NULL;
> > > 
> > > +    if ( (offset + size) > SZ_2M )
> > > +    {
> > > +        create_mappings(boot_second, BOOT_FDT_VIRT_START + SZ_2M,
> > > +                        paddr_to_pfn(base_paddr + SZ_2M),
> > > +                        SZ_2M >> PAGE_SHIFT, SZ_2M);
> > > +    }
> > > +
> > >      return fdt_virt;
> > >  }
> > > 
> > > @@ -485,7 +494,8 @@ void __init remove_early_mappings(void)
> > >  {
> > >      lpae_t pte = {0};
> > >      write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START),
> > > pte);
> > > -    flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, SECOND_SIZE);
> > > +    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START),
> > > pte);
> > 
> > Aren't you writing the same pte twice at the same address?
> 
> It was meant to be BOOT_FDT_VIRT_START + SZ_2M as to remove the mapping from
> the table. I will send a new version with that fixed.
> 
> > 
> > 
> > > +    flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
> > >  }
> > > 
> > >  extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t
> > > len);
> > > @@ -544,6 +554,8 @@ void __init setup_pagetables(unsigned long
> > > boot_phys_offset, paddr_t xen_paddr)
> > >      /* ... DTB */
> > >      pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)];
> > >      xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte;
> > > +    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
> > > +    xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
> > > 
> > >      /* ... Boot Misc area for xen relocation */
> > >      dest_va = BOOT_RELOC_VIRT_START;
> > > diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> > > index 9c14a385e7..5b6f3c985d 100644
> > > --- a/xen/include/asm-arm/config.h
> > > +++ b/xen/include/asm-arm/config.h
> > > @@ -77,12 +77,12 @@
> > >   *   0  -   2M   Unmapped
> > >   *   2M -   4M   Xen text, data, bss
> > >   *   4M -   6M   Fixmap: special-purpose 4K mapping slots
> > > - *   6M -   8M   Early boot mapping of FDT
> > > - *   8M -  10M   Early relocation address (used when relocating Xen)
> > > + *   6M -  10M   Early boot mapping of FDT
> > > + *   10M - 12M   Early relocation address (used when relocating Xen)
> > >   *               and later for livepatch vmap (if compiled in)
> > >   *
> > >   * ARM32 layout:
> > > - *   0  -  10M   <COMMON>
> > > + *   0  -  12M   <COMMON>
> > >   *
> > >   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
> > >   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
> > > @@ -93,7 +93,7 @@
> > >   *
> > >   * ARM64 layout:
> > >   * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
> > > - *   0  -  10M   <COMMON>
> > > + *   0  -  12M   <COMMON>
> > >   *
> > >   *   1G -   2G   VMAP: ioremap and early_ioremap
> > >   *
> > > @@ -113,12 +113,12 @@
> > >  #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
> > > 
> > >  #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
> > > -#define BOOT_FDT_SLOT_SIZE     MB(2)
> > > +#define BOOT_FDT_SLOT_SIZE     MB(4)
> > >  #define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
> > > 
> > > -#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00800000)
> > > +#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00a00000)
> > 
> > Wouldn't it be better to define it as BOOT_FDT_VIRT_END?
> 
> It is kind of not really related to this patch and how we handle all the
> region in config.h today. So if we define in term of *_END this one, then we
> need to do it for everyone.

Fair enough


> Also, BOOT_RELOC_VIRT_END is not defined. IHMO, these changes are post Xen 4.9
> materials.
>
>> > 
> > >  #ifdef CONFIG_LIVEPATCH
> > > -#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00800000)
> > > +#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
> > 
> > same here
> > 
> > 
> > >  #define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
> > >  #endif
> > > 
> > > --
> > > 2.11.0
> > > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C
  2017-04-19 21:18       ` Stefano Stabellini
@ 2017-04-20 12:59         ` Julien Grall
  0 siblings, 0 replies; 17+ messages in thread
From: Julien Grall @ 2017-04-20 12:59 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: nd, xen-devel

Hi Stefano,

On 19/04/17 22:18, Stefano Stabellini wrote:
> On Wed, 19 Apr 2017, Julien Grall wrote:
>> On 19/04/2017 22:01, Stefano Stabellini wrote:
>>> On Wed, 19 Apr 2017, Julien Grall wrote:
>>> I would have added early_fdt_map to this file in a way to avoid the need
>>> for duplicating the declaration of create_mappings (because this version
>>> doesn't have the useful comment on top).
>>
>> I wanted to keep the function close to the counterpart remove_early_mappings
>> rather than adding somewhere that make less sense.
>>
>> Hence why I suggested to move the create_mappings function. Would you be fine
>> with code motion for Xen 4.9?
>
> Sure. But please keep the code motion in its own separate patch.

I was planning to do the code motion in a separate patch :). My concern 
was to shuffle the code unnecessarily during code freeze and was 
planning to do it after the freeze.

Anyway, I will resend this series with a patch to move the function.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table
  2017-04-19 21:56       ` Stefano Stabellini
@ 2017-04-20 14:31         ` Julien Grall
  0 siblings, 0 replies; 17+ messages in thread
From: Julien Grall @ 2017-04-20 14:31 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: nd, xen-devel

Hi Stefano,

On 19/04/17 22:56, Stefano Stabellini wrote:
> On Wed, 19 Apr 2017, Julien Grall wrote:
>> On 19/04/2017 22:01, Stefano Stabellini wrote:
>>> On Wed, 19 Apr 2017, Julien Grall wrote:
>>>> @@ -113,12 +113,12 @@
>>>>  #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>>>>
>>>>  #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
>>>> -#define BOOT_FDT_SLOT_SIZE     MB(2)
>>>> +#define BOOT_FDT_SLOT_SIZE     MB(4)
>>>>  #define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
>>>>
>>>> -#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00800000)
>>>> +#define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00a00000)
>>>
>>> Wouldn't it be better to define it as BOOT_FDT_VIRT_END?
>>
>> It is kind of not really related to this patch and how we handle all the
>> region in config.h today. So if we define in term of *_END this one, then we
>> need to do it for everyone.
>
> Fair enough

I will prepare a patch for that and send it later on.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-04-20 14:31 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-19 17:13 [PATCH for-4.9 0/4] xen/arm: Properly map the FDT in the boot page table Julien Grall
2017-04-19 17:13 ` [PATCH for-4.9 1/4] xen/arm: Add BOOT_FDT_VIRT_END and BOOT_FDT_SLOT_SIZE Julien Grall
2017-04-19 21:01   ` Stefano Stabellini
2017-04-19 17:13 ` [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C Julien Grall
2017-04-19 21:01   ` Stefano Stabellini
2017-04-19 21:12     ` Julien Grall
2017-04-19 21:18       ` Stefano Stabellini
2017-04-20 12:59         ` Julien Grall
2017-04-19 21:32       ` Andrew Cooper
2017-04-19 17:13 ` [PATCH for-4.9 3/4] xen/arm: Check if the FDT passed by the bootloader is valid Julien Grall
2017-04-19 21:01   ` Stefano Stabellini
2017-04-19 21:14     ` Julien Grall
2017-04-19 17:13 ` [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table Julien Grall
2017-04-19 21:01   ` Stefano Stabellini
2017-04-19 21:18     ` Julien Grall
2017-04-19 21:56       ` Stefano Stabellini
2017-04-20 14:31         ` Julien Grall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.