[Xen-devel] [PATCH v2 00/35] xen/arm: Rework head.S to make it more compliant with the Arm Arm

[Xen-devel] [PATCH v2 00/35] xen/arm: Rework head.S to make it more compliant with the Arm Arm

[Julien Grall]

Hi all,

This is part of the boot/memory rework for Xen on Arm, but not sent as
MM-PARTx as this is focusing on the boot code.

Similar to the memory code, the boot code is not following the Arm Arm and
could lead to memory corruption/TLB conflict abort. I am not aware
of any platforms where Xen fails to boot, yet it should be fixed sooner
rather than later.

While making the code more compliant, I have also took the opportunity
to simplify the boot and also add more documentation.

After this series, the boot CPU and secondary CPUs path is mostly compliant
with the Arm Arm. The only non-compliant places I am aware of are:
    1) create_page_tables: Some rework is necessary to update the page-tables
       safely without the MMU on.
    2) The switches between boot and runtime page-tables (for both boot CPU
       and secondary CPUs) are not safe.
    3) The 1:1 mapping should only use page granularity mapping to avoid
    mapping memory that should not be accessed

All will be addressed in follow-up series. The boot code would also benefits
another proof read for missing isb()/dsb().

The arm32 code has been aded in this version but so far lightly tested.

For convenience I provided a branch based on staging:
   git://xenbits.xen.org/people/julieng/xen-unstable.git branch boot/v2

Cheers,

Julien Grall (35):
  xen/arm64: macros: Introduce an assembly macro to alias x30
  xen/arm64: head: Mark the end of subroutines with ENDPROC
  xen/arm64: head: Don't clobber x30/lr in the macro PRINT
  xen/arm64: head: Rework UART initialization on boot CPU
  xen/arm64: head: Introduce print_reg
  xen/arm64: head: Introduce distinct paths for the boot CPU and
    secondary CPUs
  xen/arm64: head: Rework and document check_cpu_mode()
  xen/arm64: head: Rework and document zero_bss()
  xen/arm64: head: Improve coding style and document cpu_init()
  xen/arm64: head: Improve coding style and document
    create_pages_tables()
  xen/arm64: head: Document enable_mmu()
  xen/arm64: head: Move assembly switch to the runtime PT in secondary
    CPUs path
  xen/arm64: head: Don't setup the fixmap on secondary CPUs
  xen/arm64: head: Remove 1:1 mapping as soon as it is not used
  xen/arm64: head: Rework and document setup_fixmap()
  xen/arm64: head: Rework and document launch()
  xen/arm64: head: Setup TTBR_EL2 in enable_mmu() and add missing isb
  xen/arm64: head: Introduce a macro to get a PC-relative address of a
    symbol
  xen/arm32: head: Add a macro to move an immediate constant into a
    32-bit register
  xen/arm32: head: Mark the end of subroutines with ENDPROC
  xen/arm32: head: Don't clobber r14/lr in the macro PRINT
  xen/arm32: head: Rework UART initialization on boot CPU
  xen/arm32: head: Introduce print_reg
  xen/arm32: head: Introduce distinct paths for the boot CPU and
    secondary CPUs
  xen/arm32: head: Rework and document check_cpu_mode()
  xen/arm32: head: Rework and document zero_bss()
  xen/arm32: head: Document create_pages_tables()
  xen/arm32: head: Document enable_mmu()
  xen/arm32: head: Move assembly switch to the runtime PT in secondary
    CPUs path
  xen/arm32: head: Don't setup the fixmap on secondary CPUs
  xen/arm32: head: Remove 1:1 mapping as soon as it is not used
  xen/arm32: head: Rework and document setup_fixmap()
  xen/arm32: head: Rework and document launch()
  xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb
  xen/arm: Zero BSS after the MMU and D-cache is turned on

 xen/arch/arm/arm32/head.S          | 393 +++++++++++++++++++++++----------
 xen/arch/arm/arm64/entry.S         |   5 -
 xen/arch/arm/arm64/head.S          | 432 +++++++++++++++++++++++++++----------
 xen/arch/arm/mm.c                  |  23 +-
 xen/include/asm-arm/arm64/macros.h |   5 +
 5 files changed, 619 insertions(+), 239 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH v2 01/35] xen/arm64: macros: Introduce an assembly macro to alias x30

[Julien Grall]

The return address of a function is always stored in x30. For convenience,
introduce a register alias so "lr" can be used in assembly.

This is defined in asm-arm/arm64/macros.h to allow all assembly files
to use it.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm64/entry.S         | 5 -----
 xen/include/asm-arm/arm64/macros.h | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 97b05f53ea..2d9a2713a1 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -7,11 +7,6 @@
 #include <public/xen.h>
 
 /*
- * Register aliases.
- */
-lr      .req    x30             /* link register */
-
-/*
  * Stack pushing/popping (register pairs only). Equivalent to store decrement
  * before, load increment after.
  */
diff --git a/xen/include/asm-arm/arm64/macros.h b/xen/include/asm-arm/arm64/macros.h
index 9c5e676b37..f981b4f43e 100644
--- a/xen/include/asm-arm/arm64/macros.h
+++ b/xen/include/asm-arm/arm64/macros.h
@@ -21,5 +21,10 @@
     ldr     \dst, [\dst, \tmp]
     .endm
 
+/*
+ * Register aliases.
+ */
+lr      .req    x30             /* link register */
+
 #endif /* __ASM_ARM_ARM64_MACROS_H */
 
-- 
2.11.0


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

[Xen-devel] [PATCH v2 02/35] xen/arm64: head: Mark the end of subroutines with ENDPROC

[Julien Grall]

putn() and puts() are two subroutines. Add ENDPROC for the benefits of
static analysis tools and the reader.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Fix typo in the commit title
        - Add Stefano's reviewed-by
---
 xen/arch/arm/arm64/head.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 08094a273e..f2d7445f6a 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -638,6 +638,7 @@ puts:
         b     puts
 1:
         ret
+ENDPROC(puts)
 
 /* Print a 32-bit number in hex.  Specific to the PL011 UART.
  * x0: Number to print.
@@ -656,6 +657,7 @@ putn:
         subs  x3, x3, #1
         b.ne  1b
         ret
+ENDPROC(putn)
 
 hex:    .ascii "0123456789abcdef"
         .align 2
-- 
2.11.0


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

[Xen-devel] [PATCH v2 03/35] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Julien Grall]

The current implementation of the macro PRINT will clobber x30/lr. This
means the user should save lr if it cares about it.

Follow-up patches will introduce more use of PRINT in place where lr
should be preserved. Rather than requiring all the users to preserve
lr, the macro PRINT is modified to save and restore it.

While the comment state x3 will be clobbered, this is not the case. So
PRINT will use x3 to preserve lr.

Lastly, take the opportunity to move the comment on top of PRINT and use
PRINT in init_uart. Both changes will be helpful in a follow-up patch.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Add Stefano's reviewed-by
---
 xen/arch/arm/arm64/head.S | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index f2d7445f6a..6afe83c347 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -78,12 +78,17 @@
  *  x30 - lr
  */
 
-/* Macro to print a string to the UART, if there is one.
- * Clobbers x0-x3. */
 #ifdef CONFIG_EARLY_PRINTK
-#define PRINT(_s)           \
-        adr   x0, 98f ;     \
-        bl    puts    ;     \
+/*
+ * Macro to print a string to the UART, if there is one.
+ *
+ * Clobbers x0 - x3
+ */
+#define PRINT(_s)          \
+        mov   x3, lr ;     \
+        adr   x0, 98f ;    \
+        bl    puts    ;    \
+        mov   lr, x3 ;     \
         RODATA_STR(98, _s)
 #else /* CONFIG_EARLY_PRINTK */
 #define PRINT(s)
@@ -622,9 +627,8 @@ init_uart:
 #ifdef EARLY_PRINTK_INIT_UART
         early_uart_init x23, 0
 #endif
-        adr   x0, 1f
-        b     puts
-RODATA_STR(1, "- UART enabled -\r\n")
+        PRINT("- UART enabled -\r\n")
+        ret
 
 /* Print early debug messages.
  * x0: Nul-terminated string to print.
-- 
2.11.0


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

[Xen-devel] [PATCH v2 04/35] xen/arm64: head: Rework UART initialization on boot CPU

[Julien Grall]

Anything executed after the label common_start can be executed on all
CPUs. However most of the instructions executed between the label
common_start and init_uart are not executed on the boot CPU.

The only instructions executed are to lookup the CPUID so it can be
printed on the console (if earlyprintk is enabled). Printing the CPUID
is not entirely useful to have for the boot CPU and requires a
conditional branch to bypass unused instructions.

Furthermore, the function init_uart is only called for boot CPU
requiring another conditional branch. This makes the code a bit tricky
to follow.

The UART initialization is now moved before the label common_start. This
now requires to have a slightly altered print for the boot CPU and set
the early UART base address in each the two path (boot CPU and
secondary CPUs).

This has the nice effect to remove a couple of conditional branch in
the code.

After this rework, the CPUID is only used at the very beginning of the
secondary CPUs boot path. So there is no need to "reserve" x24 for the
CPUID.

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

---
    Changes in v2:
        - Fold "xen/arm64: head: Don't "reserve" x24 for the CPUID" in
        this patch
---
 xen/arch/arm/arm64/head.S | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 6afe83c347..b684091aac 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -69,7 +69,7 @@
  *  x21 - DTB address (boot cpu only)
  *  x22 - is_secondary_cpu
  *  x23 - UART address
- *  x24 - cpuid
+ *  x24 -
  *  x25 - identity map in place
  *  x26 - skip_zero_bss
  *  x27 -
@@ -265,6 +265,12 @@ real_start_efi:
         load_paddr x21, _sdtb
 #endif
 
+        /* Initialize the UART if earlyprintk has been enabled. */
+#ifdef CONFIG_EARLY_PRINTK
+        bl    init_uart
+#endif
+        PRINT("- Boot CPU booting -\r\n")
+
         mov   x22, #0                /* x22 := is_secondary_cpu */
 
         b     common_start
@@ -281,14 +287,11 @@ GLOBAL(init_secondary)
         /* Boot CPU already zero BSS so skip it on secondary CPUs. */
         mov   x26, #1                /* X26 := skip_zero_bss */
 
-common_start:
         mrs   x0, mpidr_el1
         ldr   x13, =(~MPIDR_HWID_MASK)
         bic   x24, x0, x13           /* Mask out flags to get CPU ID */
 
-        /* Non-boot CPUs wait here until __cpu_up is ready for them */
-        cbz   x22, 1f
-
+        /* Wait here until __cpu_up is ready to handle the CPU */
         load_paddr x0, smp_up_cpu
         dsb   sy
 2:      ldr   x1, [x0]
@@ -300,14 +303,14 @@ common_start:
 
 #ifdef CONFIG_EARLY_PRINTK
         ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
-        cbnz  x22, 1f
-        bl    init_uart                 /* Boot CPU sets up the UART too */
-1:      PRINT("- CPU ")
+        PRINT("- CPU ")
         mov   x0, x24
         bl    putn
         PRINT(" booting -\r\n")
 #endif
 
+common_start:
+
         PRINT("- Current EL ")
         mrs   x4, CurrentEL
         mov   x0, x4
@@ -620,10 +623,16 @@ ENTRY(switch_ttbr)
         ret
 
 #ifdef CONFIG_EARLY_PRINTK
-/* Bring up the UART.
- * x23: Early UART base address
- * Clobbers x0-x1 */
+/*
+ * Initialize the UART. Should only be called on the boot CPU.
+ *
+ * Ouput:
+ *  x23: Early UART base physical address
+ *
+ * Clobbers x0 - x1
+ */
 init_uart:
+        ldr   x23, =EARLY_UART_BASE_ADDRESS
 #ifdef EARLY_PRINTK_INIT_UART
         early_uart_init x23, 0
 #endif
-- 
2.11.0


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

[Xen-devel] [PATCH v2 05/35] xen/arm64: head: Introduce print_reg

[Julien Grall]

At the moment, the user should save x30/lr if it cares about it.

Follow-up patches will introduce more use of putn in place where lr
should be preserved.

Furthermore, any user of putn should also move the value to register x0
if it was stored in a different register.

For convenience, a new macro is introduced to print a given register.
The macro will take care for us to move the value to x0 and also
preserve lr.

Lastly the new macro is used to replace all the callsite of putn. This
will simplify rework/review later on.

Note that CurrentEL is now stored in x5 instead of x4 because the latter
will be clobbered by the macro print_reg.

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

---
    Changes in v2:
        - Re-order the instructions to avoid cloberring x4 early. This
        allows to print content of x4.
---
 xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index b684091aac..63d63bc8ec 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -90,8 +90,25 @@
         bl    puts    ;    \
         mov   lr, x3 ;     \
         RODATA_STR(98, _s)
+
+/*
+ * Macro to print the value of register \xb
+ *
+ * Clobbers x0 - x4
+ */
+.macro print_reg xb
+        mov   x0, \xb
+        mov   x4, lr
+        bl    putn
+        mov   lr, x4
+.endm
+
 #else /* CONFIG_EARLY_PRINTK */
 #define PRINT(s)
+
+.macro print_reg xb
+.endm
+
 #endif /* !CONFIG_EARLY_PRINTK */
 
 /* Load the physical address of a symbol into xb */
@@ -304,22 +321,20 @@ GLOBAL(init_secondary)
 #ifdef CONFIG_EARLY_PRINTK
         ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
         PRINT("- CPU ")
-        mov   x0, x24
-        bl    putn
+        print_reg x24
         PRINT(" booting -\r\n")
 #endif
 
 common_start:
 
         PRINT("- Current EL ")
-        mrs   x4, CurrentEL
-        mov   x0, x4
-        bl    putn
+        mrs   x5, CurrentEL
+        print_reg x5
         PRINT(" -\r\n")
 
         /* Are we in EL2 */
-        cmp   x4, #PSR_MODE_EL2t
-        ccmp  x4, #PSR_MODE_EL2h, #0x4, ne
+        cmp   x5, #PSR_MODE_EL2t
+        ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
         b.eq  el2 /* Yes */
 
         /* OK, we're boned. */
-- 
2.11.0


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

[Xen-devel] [PATCH v2 06/35] xen/arm64: head: Introduce distinct paths for the boot CPU and secondary CPUs

[Julien Grall]

The boot code is currently quite difficult to go through because of the
lack of documentation and a number of indirection to avoid executing
some path in either the boot CPU or secondary CPUs.

In an attempt to make the boot code easier to follow, each parts of the
boot are now in separate functions. Furthermore, the paths for the boot
CPU and secondary CPUs are now distinct and for now will call each
functions.

Follow-ups will remove unnecessary calls and do further improvement
(such as adding documentation and reshuffling).

Note that the switch from using the 1:1 mapping to the runtime mapping
is duplicated for each path. This is because in the future we will need
to stay longer in the 1:1 mapping for the boot CPU.

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

---
    Changes in v2:
        - Avoid infinite loop on boot CPU
        - Fix typoes in the commit message
        - s/ID/1:1/
---
 xen/arch/arm/arm64/head.S | 60 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 63d63bc8ec..df797a1573 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -290,7 +290,19 @@ real_start_efi:
 
         mov   x22, #0                /* x22 := is_secondary_cpu */
 
-        b     common_start
+        bl    check_cpu_mode
+        bl    zero_bss
+        bl    cpu_init
+        bl    create_page_tables
+        bl    enable_mmu
+
+        /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
+        ldr   x0, =primary_switched
+        br    x0
+primary_switched:
+        bl    setup_fixmap
+        b     launch
+ENDPROC(real_start)
 
 GLOBAL(init_secondary)
         msr   DAIFSet, 0xf           /* Disable all interrupts */
@@ -324,9 +336,21 @@ GLOBAL(init_secondary)
         print_reg x24
         PRINT(" booting -\r\n")
 #endif
-
-common_start:
-
+        bl    check_cpu_mode
+        bl    zero_bss
+        bl    cpu_init
+        bl    create_page_tables
+        bl    enable_mmu
+
+        /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
+        ldr   x0, =secondary_switched
+        br    x0
+secondary_switched:
+        bl    setup_fixmap
+        b     launch
+ENDPROC(init_secondary)
+
+check_cpu_mode:
         PRINT("- Current EL ")
         mrs   x5, CurrentEL
         print_reg x5
@@ -343,7 +367,10 @@ common_start:
         b fail
 
 el2:    PRINT("- Xen starting at EL2 -\r\n")
+        ret
+ENDPROC(check_cpu_mode)
 
+zero_bss:
         /* Zero BSS only when requested */
         cbnz  x26, skip_bss
 
@@ -356,6 +383,10 @@ el2:    PRINT("- Xen starting at EL2 -\r\n")
         b.lo  1b
 
 skip_bss:
+        ret
+ENDPROC(zero_bss)
+
+cpu_init:
         PRINT("- Setting up control registers -\r\n")
 
         /* Set up memory attribute type tables */
@@ -382,7 +413,10 @@ skip_bss:
          * are handled using the EL2 stack pointer, rather
          * than SP_EL0. */
         msr spsel, #1
+        ret
+ENDPROC(cpu_init)
 
+create_page_tables:
         /* Rebuild the boot pagetable's first-level entries. The structure
          * is described in mm.c.
          *
@@ -507,6 +541,10 @@ virtphys_clash:
         b     fail
 
 1:
+        ret
+ENDPROC(create_page_tables)
+
+enable_mmu:
         PRINT("- Turning on paging -\r\n")
 
         /*
@@ -516,16 +554,16 @@ virtphys_clash:
         tlbi  alle2                  /* Flush hypervisor TLBs */
         dsb   nsh
 
-        ldr   x1, =paging            /* Explicit vaddr, not RIP-relative */
         mrs   x0, SCTLR_EL2
         orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
         orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
         dsb   sy                     /* Flush PTE writes and finish reads */
         msr   SCTLR_EL2, x0          /* now paging is enabled */
         isb                          /* Now, flush the icache */
-        br    x1                     /* Get a proper vaddr into PC */
-paging:
+        ret
+ENDPROC(enable_mmu)
 
+setup_fixmap:
         /* Now we can install the fixmap and dtb mappings, since we
          * don't need the 1:1 map any more */
         dsb   sy
@@ -567,11 +605,14 @@ paging:
         tlbi  alle2
         dsb   sy                     /* Ensure completion of TLB flush */
         isb
+        ret
+ENDPROC(setup_fixmap)
 
+launch:
         PRINT("- Ready -\r\n")
 
         /* The boot CPU should go straight into C now */
-        cbz   x22, launch
+        cbz   x22, 1f
 
         /* Non-boot CPUs need to move on to the proper pagetables, which were
          * setup in init_secondary_pagetables. */
@@ -586,7 +627,7 @@ paging:
         dsb   sy                     /* Ensure completion of TLB flush */
         isb
 
-launch:
+1:
         ldr   x0, =init_data
         add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
         ldr   x0, [x0]
@@ -601,6 +642,7 @@ launch:
         b     start_xen              /* and disappear into the land of C */
 1:
         b     start_secondary        /* (to the appropriate entry point) */
+ENDPROC(launch)
 
 /* Fail-stop */
 fail:   PRINT("- Boot failed -\r\n")
-- 
2.11.0


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

[Xen-devel] [PATCH v2 07/35] xen/arm64: head: Rework and document check_cpu_mode()

[Julien Grall]

A branch in the success case can be avoided by inverting the branch
condition. At the same time, remove a pointless comment as Xen can only
run at EL2.

Lastly, document the behavior and the main registers usage within the
function.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Add Stefano's reviewed-by
---
 xen/arch/arm/arm64/head.S | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index df797a1573..fbcc792ade 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -350,6 +350,13 @@ secondary_switched:
         b     launch
 ENDPROC(init_secondary)
 
+/*
+ * Check if the CPU has been booted in Hypervisor mode.
+ * This function will never return when the CPU is booted in another mode
+ * than Hypervisor mode.
+ *
+ * Clobbers x0 - x5
+ */
 check_cpu_mode:
         PRINT("- Current EL ")
         mrs   x5, CurrentEL
@@ -359,15 +366,13 @@ check_cpu_mode:
         /* Are we in EL2 */
         cmp   x5, #PSR_MODE_EL2t
         ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
-        b.eq  el2 /* Yes */
-
+        b.ne  1f /* No */
+        ret
+1:
         /* OK, we're boned. */
         PRINT("- Xen must be entered in NS EL2 mode -\r\n")
         PRINT("- Please update the bootloader -\r\n")
         b fail
-
-el2:    PRINT("- Xen starting at EL2 -\r\n")
-        ret
 ENDPROC(check_cpu_mode)
 
 zero_bss:
-- 
2.11.0


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

[Xen-devel] [PATCH v2 08/35] xen/arm64: head: Rework and document zero_bss()

[Julien Grall]

On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
zeroed once at boot. So the call in the secondary CPUs path can be
removed. It also means that x26 does not need to be set for secondary
CPU.

Note that we will need to keep x26 around for the boot CPU as BSS should
not be reset when booting via UEFI.

Lastly, document the behavior and the main registers usage within the
function.

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

---
    Changes in v2:
        - Clarify the commit message
        - Mention x20 is used as an input
---
 xen/arch/arm/arm64/head.S | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index fbcc792ade..92c8338d71 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -71,7 +71,7 @@
  *  x23 - UART address
  *  x24 -
  *  x25 - identity map in place
- *  x26 - skip_zero_bss
+ *  x26 - skip_zero_bss (boot cpu only)
  *  x27 -
  *  x28 -
  *  x29 -
@@ -313,8 +313,6 @@ GLOBAL(init_secondary)
         sub   x20, x19, x0           /* x20 := phys-offset */
 
         mov   x22, #1                /* x22 := is_secondary_cpu */
-        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
-        mov   x26, #1                /* X26 := skip_zero_bss */
 
         mrs   x0, mpidr_el1
         ldr   x13, =(~MPIDR_HWID_MASK)
@@ -337,7 +335,6 @@ GLOBAL(init_secondary)
         PRINT(" booting -\r\n")
 #endif
         bl    check_cpu_mode
-        bl    zero_bss
         bl    cpu_init
         bl    create_page_tables
         bl    enable_mmu
@@ -375,6 +372,15 @@ check_cpu_mode:
         b fail
 ENDPROC(check_cpu_mode)
 
+/*
+ * Zero BSS
+ *
+ * Inputs:
+ *   x20: Physical offset
+ *   x26: Do we need to zero BSS?
+ *
+ * Clobbers x0 - x3
+ */
 zero_bss:
         /* Zero BSS only when requested */
         cbnz  x26, skip_bss
-- 
2.11.0


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

[Xen-devel] [PATCH v2 09/35] xen/arm64: head: Improve coding style and document cpu_init()

[Julien Grall]

Adjust the coding style used in the comments within cpu_init(). Take the
opportunity to alter the early print to match the function name.

Lastly, document the behavior and the main registers usage within the
function.

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

---
    Changes in v2:
        - We don't clobber x4 so update the comment
---
 xen/arch/arm/arm64/head.S | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 92c8338d71..ddc5167020 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -397,19 +397,26 @@ skip_bss:
         ret
 ENDPROC(zero_bss)
 
+/*
+ * Initialize the processor for turning the MMU on.
+ *
+ * Clobbers x0 - x3
+ */
 cpu_init:
-        PRINT("- Setting up control registers -\r\n")
+        PRINT("- Initialize CPU -\r\n")
 
         /* Set up memory attribute type tables */
         ldr   x0, =MAIRVAL
         msr   mair_el2, x0
 
-        /* Set up TCR_EL2:
+        /*
+         * Set up TCR_EL2:
          * PS -- Based on ID_AA64MMFR0_EL1.PARange
          * Top byte is used
          * PT walks use Inner-Shareable accesses,
          * PT walks are write-back, write-allocate in both cache levels,
-         * 48-bit virtual address space goes through this table. */
+         * 48-bit virtual address space goes through this table.
+         */
         ldr   x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(64-48))
         /* ID_AA64MMFR0_EL1[3:0] (PARange) corresponds to TCR_EL2[18:16] (PS) */
         mrs   x1, ID_AA64MMFR0_EL1
@@ -420,9 +427,11 @@ cpu_init:
         ldr   x0, =SCTLR_EL2_SET
         msr   SCTLR_EL2, x0
 
-        /* Ensure that any exceptions encountered at EL2
+        /*
+         * Ensure that any exceptions encountered at EL2
          * are handled using the EL2 stack pointer, rather
-         * than SP_EL0. */
+         * than SP_EL0.
+         */
         msr spsel, #1
         ret
 ENDPROC(cpu_init)
-- 
2.11.0


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

[Xen-devel] [PATCH v2 10/35] xen/arm64: head: Improve coding style and document create_pages_tables()

[Julien Grall]

Adjust the coding style used in the comments within create_pages_tables()

Lastly, document the behavior and the main registers usage within the
function. Note that x25 is now only used within the function, so it does
not need to be part of the common register.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index ddc5167020..eddf663021 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -70,7 +70,7 @@
  *  x22 - is_secondary_cpu
  *  x23 - UART address
  *  x24 -
- *  x25 - identity map in place
+ *  x25 -
  *  x26 - skip_zero_bss (boot cpu only)
  *  x27 -
  *  x28 -
@@ -436,16 +436,27 @@ cpu_init:
         ret
 ENDPROC(cpu_init)
 
+/*
+ * Rebuild the boot pagetable's first-level entries. The structure
+ * is described in mm.c.
+ *
+ * After the CPU enables paging it will add the fixmap mapping
+ * to these page tables, however this may clash with the 1:1
+ * mapping. So each CPU must rebuild the page tables here with
+ * the 1:1 in place.
+ *
+ * Inputs:
+ *   x19: paddr(start)
+ *   x20: phys offset
+ *
+ * Clobbers x0 - x4, x25
+ *
+ * Register usage within this function:
+ *   x25: Identity map in place
+ */
 create_page_tables:
-        /* Rebuild the boot pagetable's first-level entries. The structure
-         * is described in mm.c.
-         *
-         * After the CPU enables paging it will add the fixmap mapping
-         * to these page tables, however this may clash with the 1:1
-         * mapping. So each CPU must rebuild the page tables here with
-         * the 1:1 in place. */
-
-        /* If Xen is loaded at exactly XEN_VIRT_START then we don't
+        /*
+         * If Xen is loaded at exactly XEN_VIRT_START then we don't
          * need an additional 1:1 mapping, the virtual mapping will
          * suffice.
          */
@@ -469,7 +480,8 @@ create_page_tables:
         cbz   x1, 1f                 /* It's in slot 0, map in boot_first
                                       * or boot_second later on */
 
-        /* Level zero does not support superpage mappings, so we have
+        /*
+         * Level zero does not support superpage mappings, so we have
          * to use an extra first level page in which we create a 1GB mapping.
          */
         load_paddr x2, boot_first_id
-- 
2.11.0


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

[Xen-devel] [PATCH v2 11/35] xen/arm64: head: Document enable_mmu()

[Julien Grall]

Document the behavior and the main registers usage within enable_mmu().

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

---
    Changes in v2:
        - x2 and x3 are also clobbers. Update the comment accordingly
        - s/ID/1:1/
---
 xen/arch/arm/arm64/head.S | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index eddf663021..63563ef5e3 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -576,6 +576,13 @@ virtphys_clash:
         ret
 ENDPROC(create_page_tables)
 
+/*
+ * Turn on the Data Cache and the MMU. The function will return on the 1:1
+ * mapping. In other word, the caller is responsible to switch to the runtime
+ * mapping.
+ *
+ * Clobbers x0 - x3
+ */
 enable_mmu:
         PRINT("- Turning on paging -\r\n")
 
-- 
2.11.0


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

[Xen-devel] [PATCH v2 12/35] xen/arm64: head: Move assembly switch to the runtime PT in secondary CPUs path

[Julien Grall]

The assembly switch to the runtime PT is only necessary for the
secondary CPUs. So move the code in the secondary CPUs path.

While this is definitely not compliant with the Arm Arm as we are
switching between two differents set of page-tables without turning off
the MMU. Turning off the MMU is impossible here as the ID map may clash
with other mappings in the runtime page-tables. This will require more
rework to avoid the problem. So for now add a TODO in the code.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2
        - Add Stefano's acked-by
---
 xen/arch/arm/arm64/head.S | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 63563ef5e3..4ce4895a0d 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -344,6 +344,23 @@ GLOBAL(init_secondary)
         br    x0
 secondary_switched:
         bl    setup_fixmap
+
+        /*
+         * Non-boot CPUs need to move on to the proper pagetables, which were
+         * setup in init_secondary_pagetables.
+         *
+         * XXX: This is not compliant with the Arm Arm.
+         */
+        ldr   x4, =init_ttbr         /* VA of TTBR0_EL2 stashed by CPU 0 */
+        ldr   x4, [x4]               /* Actual value */
+        dsb   sy
+        msr   TTBR0_EL2, x4
+        dsb   sy
+        isb
+        tlbi  alle2
+        dsb   sy                     /* Ensure completion of TLB flush */
+        isb
+
         b     launch
 ENDPROC(init_secondary)
 
@@ -650,23 +667,6 @@ ENDPROC(setup_fixmap)
 launch:
         PRINT("- Ready -\r\n")
 
-        /* The boot CPU should go straight into C now */
-        cbz   x22, 1f
-
-        /* Non-boot CPUs need to move on to the proper pagetables, which were
-         * setup in init_secondary_pagetables. */
-
-        ldr   x4, =init_ttbr         /* VA of TTBR0_EL2 stashed by CPU 0 */
-        ldr   x4, [x4]               /* Actual value */
-        dsb   sy
-        msr   TTBR0_EL2, x4
-        dsb   sy
-        isb
-        tlbi  alle2
-        dsb   sy                     /* Ensure completion of TLB flush */
-        isb
-
-1:
         ldr   x0, =init_data
         add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
         ldr   x0, [x0]
-- 
2.11.0


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

[Xen-devel] [PATCH v2 13/35] xen/arm64: head: Don't setup the fixmap on secondary CPUs

[Julien Grall]

setup_fixmap() will setup the fixmap in the boot page tables in order to
use earlyprintk and also update the register x23 holding the address to
the UART.

However, secondary CPUs are not using earlyprintk between turning the
MMU on and switching to the runtime page table. So setting up the
fixmap in the boot pages table is pointless.

This means most of setup_fixmap() is not necessary for the secondary
CPUs. The update of UART address is now moved out of setup_fixmap() and
duplicated in the CPU boot and secondary CPUs boot. Additionally, the
call to setup_fixmap() is removed from secondary CPUs boot.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <stabellini@kernel.org>

---
    Changes in v2:
        - Fix typo
        - Clarify the commit message
        - Add Stefano's reviewed-by
---
 xen/arch/arm/arm64/head.S | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 4ce4895a0d..28efe9230c 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -301,6 +301,10 @@ real_start_efi:
         br    x0
 primary_switched:
         bl    setup_fixmap
+#ifdef CONFIG_EARLY_PRINTK
+        /* Use a virtual address to access the UART. */
+        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
+#endif
         b     launch
 ENDPROC(real_start)
 
@@ -343,8 +347,6 @@ GLOBAL(init_secondary)
         ldr   x0, =secondary_switched
         br    x0
 secondary_switched:
-        bl    setup_fixmap
-
         /*
          * Non-boot CPUs need to move on to the proper pagetables, which were
          * setup in init_secondary_pagetables.
@@ -361,6 +363,10 @@ secondary_switched:
         dsb   sy                     /* Ensure completion of TLB flush */
         isb
 
+#ifdef CONFIG_EARLY_PRINTK
+        /* Use a virtual address to access the UART. */
+        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
+#endif
         b     launch
 ENDPROC(init_secondary)
 
@@ -624,10 +630,6 @@ setup_fixmap:
          * don't need the 1:1 map any more */
         dsb   sy
 #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
-        /* Non-boot CPUs don't need to rebuild the fixmap itself, just
-         * the mapping from boot_second to xen_fixmap */
-        cbnz  x22, 1f
-
         /* Add UART to the fixmap table */
         ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
         lsr   x2, x23, #THIRD_SHIFT
@@ -635,7 +637,6 @@ setup_fixmap:
         mov   x3, #PT_DEV_L3
         orr   x2, x2, x3             /* x2 := 4K dev map including UART */
         str   x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
-1:
 
         /* Map fixmap into boot_second */
         ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
@@ -645,9 +646,6 @@ setup_fixmap:
         ldr   x1, =FIXMAP_ADDR(0)
         lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
         str   x2, [x4, x1]           /* Map it in the fixmap's slot */
-
-        /* Use a virtual address to access the UART. */
-        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
 
         /*
-- 
2.11.0


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

[Xen-devel] [PATCH v2 14/35] xen/arm64: head: Remove 1:1 mapping as soon as it is not used

[Julien Grall]

The 1:1 mapping may clash with other parts of the Xen virtual memory
layout. At the moment, Xen is handling the clash by only creating a
mapping to the runtime virtual address before enabling the MMU.

The rest of the mappings (such as the fixmap) will be mapped after the
MMU is enabled. However, the code doing the mapping is not safe as it
replace mapping without using the Break-Before-Make sequence.

As the 1:1 mapping can be anywhere in the memory, it is easier to remove
all the entries added as soon as the 1:1 mapping is not used rather than
adding the Break-Before-Make sequence everywhere.

It is difficult to track where exactly the 1:1 mapping was created
without a full rework of create_page_tables(). Instead, introduce a new
function remove_identity_mapping() will look where is the top-level entry
for the 1:1 mapping and remove it.

The new function is only called for the boot CPU. Secondary CPUs will
switch directly to the runtime page-tables so there are no need to
remove the 1:1 mapping. Note that this still doesn't make the Secondary
CPUs path safe but it is not making it worst.

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

---
    It is very likely we will need to re-introduce the 1:1 mapping to cater
    secondary CPUs boot and suspend/resume. For now, the attempt is to make
    boot CPU path fully Arm Arm compliant.

    Changes in v2:
        - s/ID map/1:1 mapping/
        - Rename remove_id_map() to remove_identity_mapping()
        - Add missing signed-off-by
---
 xen/arch/arm/arm64/head.S | 86 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 28efe9230c..a607b3bdb1 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -300,6 +300,13 @@ real_start_efi:
         ldr   x0, =primary_switched
         br    x0
 primary_switched:
+        /*
+         * The 1:1 map may clash with other parts of the Xen virtual memory
+         * layout. As it is not used anymore, remove it completely to
+         * avoid having to worry about replacing existing mapping
+         * afterwards.
+         */
+        bl    remove_identity_mapping
         bl    setup_fixmap
 #ifdef CONFIG_EARLY_PRINTK
         /* Use a virtual address to access the UART. */
@@ -625,10 +632,68 @@ enable_mmu:
         ret
 ENDPROC(enable_mmu)
 
+/*
+ * Remove the 1:1 map for the page-tables. It is not easy to keep track
+ * where the 1:1 map was mapped, so we will look for the top-level entry
+ * exclusive to the 1:1 map and remove it.
+ *
+ * Inputs:
+ *   x19: paddr(start)
+ *
+ * Clobbers x0 - x1
+ */
+remove_identity_mapping:
+        /*
+         * Find the zeroeth slot used. Remove the entry from zeroeth
+         * table if the slot is not 0. For slot 0, the 1:1 mapping was either
+         * done in first or second table.
+         */
+        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
+        cbz   x1, 1f
+        /* It is not in slot 0, remove the entry */
+        ldr   x0, =boot_pgtable         /* x0 := root table */
+        str   xzr, [x0, x1, lsl #3]
+        b     identity_mapping_removed
+
+1:
+        /*
+         * Find the first slot used. Remove the entry for the first
+         * table if the slot is not 0. For slot 0, the 1:1 mapping was
+         * done in the second table.
+         */
+        lsr   x1, x19, #FIRST_SHIFT
+        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
+        cbz   x1, 1f
+        /* It is not in slot 0, remove the entry */
+        ldr   x0, =boot_first           /* x0 := first table */
+        str   xzr, [x0, x1, lsl #3]
+        b     identity_mapping_removed
+
+1:
+        /*
+         * Find the second slot used. Remove the entry for the first
+         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
+         * For slot 1, it means the 1:1 mapping was not created.
+         */
+        lsr   x1, x19, #SECOND_SHIFT
+        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
+        cmp   x1, #1
+        beq   identity_mapping_removed
+        /* It is not in slot 1, remove the entry */
+        ldr   x0, =boot_second          /* x0 := second table */
+        str   xzr, [x0, x1, lsl #3]
+
+identity_mapping_removed:
+        /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */
+        dsb   nshst
+        tlbi  alle2
+        dsb   nsh
+        isb
+
+        ret
+ENDPROC(remove_identity_mapping)
+
 setup_fixmap:
-        /* Now we can install the fixmap and dtb mappings, since we
-         * don't need the 1:1 map any more */
-        dsb   sy
 #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
         /* Add UART to the fixmap table */
         ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
@@ -646,19 +711,10 @@ setup_fixmap:
         ldr   x1, =FIXMAP_ADDR(0)
         lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
         str   x2, [x4, x1]           /* Map it in the fixmap's slot */
-#endif
 
-        /*
-         * Flush the TLB in case the 1:1 mapping happens to clash with
-         * the virtual addresses used by the fixmap or DTB.
-         */
-        dsb   sy                     /* Ensure any page table updates made above
-                                      * have occurred. */
-
-        isb
-        tlbi  alle2
-        dsb   sy                     /* Ensure completion of TLB flush */
-        isb
+        /* Ensure any page table updates made above have occurred. */
+        dsb   nshst
+#endif
         ret
 ENDPROC(setup_fixmap)
 
-- 
2.11.0


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

[Xen-devel] [PATCH v2 15/35] xen/arm64: head: Rework and document setup_fixmap()

[Julien Grall]

At the moment, the fixmap table is only hooked when earlyprintk is used.
This is fine today because in C land, the fixmap is not used by anyone
until the the boot CPU is switching to the runtime page-tables.

In the future, the boot CPU will not switch between page-tables to
avoid TLB incoherency. Thus, the fixmap table will need to be always
hooked beofre any use. Let's start doing it now in setup_fixmap().

Lastly, document the behavior and the main registers usage within the
function.

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

---
    Changes in v2:
        - Update the comment to reflect that we clobbers x1 - x4 and not
        x0 - x1.
        - Add the list of input registers
        - s/ID map/1:1 mapping/
        - Reword the commit message
---
 xen/arch/arm/arm64/head.S | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index a607b3bdb1..f165dd61ca 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -693,8 +693,21 @@ identity_mapping_removed:
         ret
 ENDPROC(remove_identity_mapping)
 
+/*
+ * Map the UART in the fixmap (when earlyprintk is used) and hook the
+ * fixmap table in the page tables.
+ *
+ * The fixmap cannot be mapped in create_page_tables because it may
+ * clash with the 1:1 mapping.
+ *
+ * Inputs:
+ *   x20: Physical offset
+ *   x23: Early UART base physical address
+ *
+ * Clobbers x1 - x4
+ */
 setup_fixmap:
-#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
+#ifdef CONFIG_EARLY_PRINTK
         /* Add UART to the fixmap table */
         ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
         lsr   x2, x23, #THIRD_SHIFT
@@ -702,6 +715,7 @@ setup_fixmap:
         mov   x3, #PT_DEV_L3
         orr   x2, x2, x3             /* x2 := 4K dev map including UART */
         str   x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
+#endif
 
         /* Map fixmap into boot_second */
         ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
@@ -714,7 +728,7 @@ setup_fixmap:
 
         /* Ensure any page table updates made above have occurred. */
         dsb   nshst
-#endif
+
         ret
 ENDPROC(setup_fixmap)
 
-- 
2.11.0


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

[Xen-devel] [PATCH v2 16/35] xen/arm64: head: Rework and document launch()

[Julien Grall]

Boot CPU and secondary CPUs will use different entry point to C code. At
the moment, the decision on which entry to use is taken within launch().

In order to avoid a branch for the decision and make the code clearer,
launch() is reworked to take in parameters the entry point and its
arguments.

Lastly, document the behavior and the main registers usage within the
function.

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

---
    Changes in v2:
        - Use x3 instead of x4
        - Add a clobbers section
---
 xen/arch/arm/arm64/head.S | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index f165dd61ca..7541635102 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -312,6 +312,11 @@ primary_switched:
         /* Use a virtual address to access the UART. */
         ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
+        PRINT("- Ready -\r\n")
+        /* Setup the arguments for start_xen and jump to C world */
+        mov   x0, x20                /* x0 := Physical offset */
+        mov   x1, x21                /* x1 := paddr(FDT) */
+        ldr   x2, =start_xen
         b     launch
 ENDPROC(real_start)
 
@@ -374,6 +379,9 @@ secondary_switched:
         /* Use a virtual address to access the UART. */
         ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
+        PRINT("- Ready -\r\n")
+        /* Jump to C world */
+        ldr   x2, =start_secondary
         b     launch
 ENDPROC(init_secondary)
 
@@ -732,23 +740,26 @@ setup_fixmap:
         ret
 ENDPROC(setup_fixmap)
 
+/*
+ * Setup the initial stack and jump to the C world
+ *
+ * Inputs:
+ *   x0 : Argument 0 of the C function to call
+ *   x1 : Argument 1 of the C function to call
+ *   x2 : C entry point
+ *
+ * Clobbers x3
+ */
 launch:
-        PRINT("- Ready -\r\n")
-
-        ldr   x0, =init_data
-        add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
-        ldr   x0, [x0]
-        add   x0, x0, #STACK_SIZE    /* (which grows down from the top). */
-        sub   x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */
-        mov   sp, x0
-
-        cbnz  x22, 1f
-
-        mov   x0, x20                /* Marshal args: - phys_offset */
-        mov   x1, x21                /*               - FDT */
-        b     start_xen              /* and disappear into the land of C */
-1:
-        b     start_secondary        /* (to the appropriate entry point) */
+        ldr   x3, =init_data
+        add   x3, x3, #INITINFO_stack /* Find the boot-time stack */
+        ldr   x3, [x3]
+        add   x3, x3, #STACK_SIZE    /* (which grows down from the top). */
+        sub   x3, x3, #CPUINFO_sizeof /* Make room for CPU save record */
+        mov   sp, x3
+
+        /* Jump to C world */
+        br    x2
 ENDPROC(launch)
 
 /* Fail-stop */
-- 
2.11.0


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

[Xen-devel] [PATCH v2 17/35] xen/arm64: head: Setup TTBR_EL2 in enable_mmu() and add missing isb

[Julien Grall]

At the moment, TTBR_EL2 is setup in create_page_tables(). This is fine
as it is called by every CPUs.

However, such assumption may not hold in the future. To make change
easier, the TTBR_EL2 is not setup in enable_mmu().

Take the opportunity to add the missing isb() to ensure the TTBR_EL2 is
seen before the MMU is turned on.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm64/head.S | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 7541635102..9afd89d447 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -501,9 +501,7 @@ create_page_tables:
         cmp   x19, #XEN_VIRT_START
         cset  x25, eq                /* x25 := identity map in place, or not */
 
-        /* Write Xen's PT's paddr into TTBR0_EL2 */
         load_paddr x4, boot_pgtable
-        msr   TTBR0_EL2, x4
 
         /* Setup boot_pgtable: */
         load_paddr x1, boot_first
@@ -631,6 +629,11 @@ enable_mmu:
         tlbi  alle2                  /* Flush hypervisor TLBs */
         dsb   nsh
 
+        /* Write Xen's PT's paddr into TTBR0_EL2 */
+        load_paddr x0, boot_pgtable
+        msr   TTBR0_EL2, x0
+        isb
+
         mrs   x0, SCTLR_EL2
         orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
         orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
-- 
2.11.0


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

[Xen-devel] [PATCH v2 18/35] xen/arm64: head: Introduce a macro to get a PC-relative address of a symbol

[Julien Grall]

Arm64 provides instructions to load a PC-relative address, but with some
limitations:
   - adr is enable to cope with +/-1MB
   - adrp is enale to cope with +/-4GB but relative to a 4KB page
     address

Because of that, the code requires to use 2 instructions to load any Xen
symbol. To make the code more obvious, introducing a new macro adr_l is
introduced.

The new macro is used to replace a couple of open-coded use in
efi_xen_start.

The macro is copied from Linux 5.2-rc4.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm64/head.S | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 9afd89d447..2287f3ce48 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -111,6 +111,18 @@
 
 #endif /* !CONFIG_EARLY_PRINTK */
 
+/*
+ * Pseudo-op for PC relative adr <reg>, <symbol> where <symbol> is
+ * within the range +/- 4GB of the PC.
+ *
+ * @dst: destination register (64 bit wide)
+ * @sym: name of the symbol
+ */
+.macro  adr_l, dst, sym
+        adrp \dst, \sym
+        add  \dst, \dst, :lo12:\sym
+.endm
+
 /* Load the physical address of a symbol into xb */
 .macro load_paddr xb, sym
         ldr \xb, =\sym
@@ -886,11 +898,9 @@ ENTRY(efi_xen_start)
          * Flush dcache covering current runtime addresses
          * of xen text/data. Then flush all of icache.
          */
-        adrp  x1, _start
-        add   x1, x1, #:lo12:_start
+        adr_l x1, _start
         mov   x0, x1
-        adrp  x2, _end
-        add   x2, x2, #:lo12:_end
+        adr_l x2, _end
         sub   x1, x2, x1
 
         bl    __flush_dcache_area
-- 
2.11.0


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

[Xen-devel] [PATCH v2 19/35] xen/arm32: head: Add a macro to move an immediate constant into a 32-bit register

[Julien Grall]

The current boot code is using the pattern ldr rX, =... to move an
immediate constant into a 32-bit register.

This pattern implies to load the immediate constant from a literal pool,
meaning a memory access will be performed.

The memory access can be avoided by using movw/movt instructions.

A new macro is introduced to move an immediate constant into a 32-bit
register without a memory load. Follow-up patches will make use of it.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 18ded49a04..99f4af18d8 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -37,6 +37,15 @@
 #endif
 
 /*
+ * Move an immediate constant into a 32-bit register using movw/movt
+ * instructions.
+ */
+.macro mov_w reg, word
+        movw  \reg, #:lower16:\word
+        movt  \reg, #:upper16:\word
+.endm
+
+/*
  * Common register usage in this file:
  *   r0  -
  *   r1  -
-- 
2.11.0


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

[Xen-devel] [PATCH v2 20/35] xen/arm32: head: Mark the end of subroutines with ENDPROC

[Julien Grall]

putn() and puts() are two subroutines. Add ENDPROC for the benefits of
static analysis tools and the reader.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 99f4af18d8..8b4c8a4714 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -518,6 +518,7 @@ puts:
         moveq pc, lr
         early_uart_transmit r11, r1
         b puts
+ENDPROC(puts)
 
 /*
  * Print a 32-bit number in hex.  Specific to the PL011 UART.
@@ -537,6 +538,7 @@ putn:
         subs  r3, r3, #1
         bne   1b
         mov   pc, lr
+ENDPROC(putn)
 
 hex:    .ascii "0123456789abcdef"
         .align 2
-- 
2.11.0


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

[Xen-devel] [PATCH v2 21/35] xen/arm32: head: Don't clobber r14/lr in the macro PRINT

[Julien Grall]

The current implementation of the macro PRINT will clobber r14/lr. This
means the user should save r14 if it cares about it.

Follow-up patches will introduce more use of PRINT in places where lr
should be preserved. Rather than requiring all the user to preserve lr,
the macro PRINT is modified to save and restore it.

While the comment state r3 will be clobbered, this is not the case. So
PRINT will use r3 to preserve lr.

Lastly, take the opportunity to move the comment on top of PRINT and use
PRINT in init_uart. Both changes will be helpful in a follow-up patch.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 8b4c8a4714..b54331c19d 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -64,15 +64,20 @@
  *   r14 - LR
  *   r15 - PC
  */
-/* Macro to print a string to the UART, if there is one.
- * Clobbers r0-r3. */
 #ifdef CONFIG_EARLY_PRINTK
-#define PRINT(_s)       \
-        adr   r0, 98f ; \
-        bl    puts    ; \
-        b     99f     ; \
-98:     .asciz _s     ; \
-        .align 2      ; \
+/*
+ * Macro to print a string to the UART, if there is one.
+ *
+ * Clobbers r0 - r3
+ */
+#define PRINT(_s)           \
+        mov   r3, lr       ;\
+        adr   r0, 98f      ;\
+        bl    puts         ;\
+        mov   lr, r3       ;\
+        b     99f          ;\
+98:     .asciz _s          ;\
+        .align 2           ;\
 99:
 #else /* CONFIG_EARLY_PRINTK */
 #define PRINT(s)
@@ -500,10 +505,8 @@ init_uart:
 #ifdef EARLY_PRINTK_INIT_UART
         early_uart_init r11, r1, r2
 #endif
-        adr   r0, 1f
-        b     puts                  /* Jump to puts */
-1:      .asciz "- UART enabled -\r\n"
-        .align 4
+        PRINT("- UART enabled -\r\n")
+        mov   pc, lr
 
 /*
  * Print early debug messages.
-- 
2.11.0


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

[Xen-devel] [PATCH v2 22/35] xen/arm32: head: Rework UART initialization on boot CPU

[Julien Grall]

Anything executed after the label common_start can be executed on all
CPUs. However most of the instructions executed between the label
common_start and init_uart are not executed on the boot CPU.

The only instructions executed are to lookup the CPUID so it can be
printed on the console (if earlyprintk is enabled). Printing the CPUID
is not entirely useful to have for the boot CPU and requires a
conditional branch to bypass unused instructions.

Furthermore, the function init_uart is only called for boot CPU
requiring another conditional branch. This makes the code a bit tricky
to follow.

The UART initialization is now moved before the label common_start. This
now requires to have a slightly altered print for the boot CPU and set
the early UART base address in each the two path (boot CPU and
secondary CPUs).

This has the nice effect to remove a couple of conditional branch in
the code.

After this rework, the CPUID is only used at the very beginning of the
secondary CPUs boot path. So there is no need to "reserve" x24 for the
CPUID.

Lastly, take the opportunity to replace load from literal pool with the
new macro mov_w.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index b54331c19d..134c3dda92 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -54,7 +54,7 @@
  *   r4  -
  *   r5  -
  *   r6  - identity map in place
- *   r7  - CPUID
+ *   r7  -
  *   r8  - DTB address (boot CPU only)
  *   r9  - paddr(start)
  *   r10 - phys offset
@@ -123,6 +123,12 @@ past_zImage:
         add   r8, r10                /* r8 := paddr(DTB) */
 #endif
 
+        /* Initialize the UART if earlyprintk has been enabled. */
+#ifdef CONFIG_EARLY_PRINTK
+        bl    init_uart
+#endif
+        PRINT("- Boot CPU booting -\r\n")
+
         mov   r12, #0                /* r12 := is_secondary_cpu */
 
         b     common_start
@@ -137,14 +143,9 @@ GLOBAL(init_secondary)
 
         mov   r12, #1                /* r12 := is_secondary_cpu */
 
-common_start:
         mrc   CP32(r1, MPIDR)
         bic   r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
 
-        /* Non-boot CPUs wait here until __cpu_up is ready for them */
-        teq   r12, #0
-        beq   1f
-
         ldr   r0, =smp_up_cpu
         add   r0, r0, r10            /* Apply physical offset */
         dsb
@@ -156,15 +157,14 @@ common_start:
 1:
 
 #ifdef CONFIG_EARLY_PRINTK
-        ldr   r11, =EARLY_UART_BASE_ADDRESS  /* r11 := UART base address */
-        teq   r12, #0                /* Boot CPU sets up the UART too */
-        bleq  init_uart
+        mov_w r11, EARLY_UART_BASE_ADDRESS   /* r11 := UART base address */
         PRINT("- CPU ")
         mov   r0, r7
         bl    putn
         PRINT(" booting -\r\n")
 #endif
 
+common_start:
         /* Check that this CPU has Hyp mode */
         mrc   CP32(r0, ID_PFR1)
         and   r0, r0, #0xf000        /* Bits 12-15 define virt extensions */
@@ -497,11 +497,15 @@ ENTRY(switch_ttbr)
 
 #ifdef CONFIG_EARLY_PRINTK
 /*
- * Bring up the UART.
- * r11: Early UART base address
- * Clobbers r0-r2
+ * Initialize the UART. Should only be called on the boot CPU.
+ *
+ * Ouput:
+ *  r11: Early UART base physical address
+ *
+ * Clobbers r0 - r3
  */
 init_uart:
+        mov_w r11, EARLY_UART_BASE_ADDRESS
 #ifdef EARLY_PRINTK_INIT_UART
         early_uart_init r11, r1, r2
 #endif
-- 
2.11.0


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

[Xen-devel] [PATCH v2 23/35] xen/arm32: head: Introduce print_reg

[Julien Grall]

At the moment, the user should save r14/lr if it cares about it.

Follow-up patches will introduce more use of putn in place where lr
should be preserved.

Furthermore, any user of putn should also move the value to register r0
if it was stored in a different register.

For convenience, a new macro is introduced to print a given register.
The macro will take care for us to move the value to r0 and also
preserve lr.

Lastly the new macro is used to replace all the callsite of putn. This
will simplify rework/review later on.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 134c3dda92..bbcfdcd351 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -79,8 +79,25 @@
 98:     .asciz _s          ;\
         .align 2           ;\
 99:
+
+/*
+ * Macro to print the value of register \rb
+ *
+ * Clobbers r0 - r4
+ */
+.macro print_reg rb
+        mov   r0, \rb
+        mov   r4, lr
+        bl    putn
+        mov   lr, r4
+.endm
+
 #else /* CONFIG_EARLY_PRINTK */
 #define PRINT(s)
+
+.macro print_reg rb
+.endm
+
 #endif /* !CONFIG_EARLY_PRINTK */
 
         .arm
@@ -159,8 +176,7 @@ GLOBAL(init_secondary)
 #ifdef CONFIG_EARLY_PRINTK
         mov_w r11, EARLY_UART_BASE_ADDRESS   /* r11 := UART base address */
         PRINT("- CPU ")
-        mov   r0, r7
-        bl    putn
+        print_reg r7
         PRINT(" booting -\r\n")
 #endif
 
@@ -211,8 +227,7 @@ skip_bss:
         bne   1f
         mov   r4, r0
         PRINT("- Missing processor info: ")
-        mov   r0, r4
-        bl    putn
+        print_reg r4
         PRINT(" -\r\n")
         b     fail
 1:
-- 
2.11.0


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

[Xen-devel] [PATCH v2 24/35] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs

[Julien Grall]

The boot code is currently quite difficult to go through because of the
lack of documentation and a number of indirection to avoid executing
some path in either the boot CPU or secondary CPUs.

In an attempt to make the boot code easier to follow, each parts of the
boot are now in separate functions. Furthermore, the paths for the boot
CPU and secondary CPUs are now distinct and for now will call each
functions.

Follow-ups will remove unnecessary calls and do further improvement
(such as adding documentation and reshuffling).

Note that the switch from using the ID mapping to the runtime mapping
is duplicated for each path. This is because in the future we will need
to stay longer in the ID mapping for the boot CPU.

Lastly, it is now required to save lr in cpu_init() becauswe the
function will call other functions and therefore clobber lr.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 64 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index bbcfdcd351..13793e85d8 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -148,7 +148,19 @@ past_zImage:
 
         mov   r12, #0                /* r12 := is_secondary_cpu */
 
-        b     common_start
+        bl    check_cpu_mode
+        bl    zero_bss
+        bl    cpu_init
+        bl    create_page_tables
+        bl    enable_mmu
+
+        /* We are still in the ID map. Jump to the runtime Virtual Address */
+        ldr   r0, =primary_switched
+        mov   pc, r0
+primary_switched:
+        bl    setup_fixmap
+        b     launch
+ENDPROC(start)
 
 GLOBAL(init_secondary)
         cpsid aif                    /* Disable all interrupts */
@@ -179,8 +191,21 @@ GLOBAL(init_secondary)
         print_reg r7
         PRINT(" booting -\r\n")
 #endif
-
-common_start:
+        bl    check_cpu_mode
+        bl    zero_bss
+        bl    cpu_init
+        bl    create_page_tables
+        bl    enable_mmu
+
+        /* We are still in the ID map. Jump to the runtime Virtual Address. */
+        ldr   r0, =secondary_switched
+        mov   pc, r0
+secondary_switched:
+        bl    setup_fixmap
+        b     launch
+ENDPROC(init_secondary)
+
+check_cpu_mode:
         /* Check that this CPU has Hyp mode */
         mrc   CP32(r0, ID_PFR1)
         and   r0, r0, #0xf000        /* Bits 12-15 define virt extensions */
@@ -202,7 +227,10 @@ common_start:
         b     fail
 
 hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
+        mov   pc, lr
+ENDPROC(check_cpu_mode)
 
+zero_bss:
         /* Zero BSS On the boot CPU to avoid nasty surprises */
         teq   r12, #0
         bne   skip_bss
@@ -219,8 +247,14 @@ hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
         blo   1b
 
 skip_bss:
+        mov   pc, lr
+ENDPROC(zero_bss)
+
+cpu_init:
         PRINT("- Setting up control registers -\r\n")
 
+        mov   r5, lr			            /* r5 := return address */
+
         /* Get processor specific proc info into r1 */
         bl    __lookup_processor_type
         teq   r1, #0
@@ -231,7 +265,6 @@ skip_bss:
         PRINT(" -\r\n")
         b     fail
 1:
-
         /* Jump to cpu_init */
         ldr   r1, [r1, #PROCINFO_cpu_init]  /* r1 := vaddr(init func) */
         adr   lr, cpu_init_done             /* Save return address */
@@ -256,6 +289,10 @@ cpu_init_done:
         ldr   r0, =HSCTLR_SET
         mcr   CP32(r0, HSCTLR)
 
+        mov   pc, r5                        /* Return address is in r5 */
+ENDPROC(cpu_init)
+
+create_page_tables:
         /*
          * Rebuild the boot pagetable's first-level entries. The structure
          * is described in mm.c.
@@ -359,15 +396,16 @@ cpu_init_done:
         /* boot pagetable setup complete */
 
         cmp   r6, #1                /* Did we manage to create an identity mapping ? */
-        beq   1f
+        moveq pc, lr
         PRINT("Unable to build boot page tables - Failed to identity map Xen.\r\n")
         b     fail
 virtphys_clash:
         /* Identity map clashes with boot_third, which we cannot handle yet */
         PRINT("- Unable to build boot page tables - virt and phys addresses clash. -\r\n")
         b     fail
+ENDPROC(create_page_tables)
 
-1:
+enable_mmu:
         PRINT("- Turning on paging -\r\n")
 
         /*
@@ -377,16 +415,16 @@ virtphys_clash:
         mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLBs */
         dsb   nsh
 
-        ldr   r1, =paging            /* Explicit vaddr, not RIP-relative */
         mrc   CP32(r0, HSCTLR)
         /* Enable MMU and D-cache */
         orr   r0, r0, #(SCTLR_Axx_ELx_M|SCTLR_Axx_ELx_C)
         dsb                          /* Flush PTE writes and finish reads */
         mcr   CP32(r0, HSCTLR)       /* now paging is enabled */
         isb                          /* Now, flush the icache */
-        mov   pc, r1                 /* Get a proper vaddr into PC */
-paging:
+        mov   pc, lr
+ENDPROC(enable_mmu)
 
+setup_fixmap:
         /*
          * Now we can install the fixmap and dtb mappings, since we
          * don't need the 1:1 map any more
@@ -436,12 +474,15 @@ paging:
         mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLB */
         dsb                          /* Ensure completion of TLB flush */
         isb
+        mov   pc, lr
+ENDPROC(setup_fixmap)
 
+launch:
         PRINT("- Ready -\r\n")
 
         /* The boot CPU should go straight into C now */
         teq   r12, #0
-        beq   launch
+        beq   1f
 
         /*
          * Non-boot CPUs need to move on to the proper pagetables, which were
@@ -460,7 +501,7 @@ paging:
         dsb                          /* Ensure completion of TLB+BP flush */
         isb
 
-launch:
+1:
         ldr   r0, =init_data
         add   r0, #INITINFO_stack    /* Find the boot-time stack */
         ldr   sp, [r0]
@@ -471,6 +512,7 @@ launch:
         moveq r1, r8                 /*               - DTB address */
         beq   start_xen              /* and disappear into the land of C */
         b     start_secondary        /* (to the appropriate entry point) */
+ENDPROC(launch)
 
 /* Fail-stop */
 fail:   PRINT("- Boot failed -\r\n")
-- 
2.11.0


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

[Xen-devel] [PATCH v2 25/35] xen/arm32: head: Rework and document check_cpu_mode()

[Julien Grall]

A branch in the success case can be avoided by inverting the branch
condition. At the same time, remove a pointless comment as Xen can only
run at Hypervisor Mode.

Lastly, document the behavior and the main registers usage within the
function.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 13793e85d8..0959c4f044 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -205,6 +205,16 @@ secondary_switched:
         b     launch
 ENDPROC(init_secondary)
 
+/*
+ * Check if the CPU supports virtualization extensions and has been booted
+ * in Hypervisor mode.
+ *
+ * This function will never return when the CPU doesn't support
+ * virtualization extensions or is booted in another mode than
+ * Hypervisor mode.
+ *
+ * Clobbers r0 - r3
+ */
 check_cpu_mode:
         /* Check that this CPU has Hyp mode */
         mrc   CP32(r0, ID_PFR1)
@@ -219,15 +229,12 @@ check_cpu_mode:
         mrs   r0, cpsr
         and   r0, r0, #0x1f          /* Mode is in the low 5 bits of CPSR */
         teq   r0, #0x1a              /* Hyp Mode? */
-        beq   hyp
+        moveq pc, lr                 /* Yes, return */
 
         /* OK, we're boned. */
         PRINT("- Xen must be entered in NS Hyp mode -\r\n")
         PRINT("- Please update the bootloader -\r\n")
         b     fail
-
-hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
-        mov   pc, lr
 ENDPROC(check_cpu_mode)
 
 zero_bss:
-- 
2.11.0


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

[Xen-devel] [PATCH v2 26/35] xen/arm32: head: Rework and document zero_bss()

[Julien Grall]

On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
zeroed once at boot. So the call in the secondary CPUs path can be
removed.

Lastly, document the behavior and the main registers usage within the
function.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 0959c4f044..119d2a0df8 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -192,7 +192,6 @@ GLOBAL(init_secondary)
         PRINT(" booting -\r\n")
 #endif
         bl    check_cpu_mode
-        bl    zero_bss
         bl    cpu_init
         bl    create_page_tables
         bl    enable_mmu
@@ -237,11 +236,15 @@ check_cpu_mode:
         b     fail
 ENDPROC(check_cpu_mode)
 
+/*
+ * Zero BSS
+ *
+ * Inputs:
+ *   r10: Physical offset
+ *
+ * Clobbers r0 - r3
+ */
 zero_bss:
-        /* Zero BSS On the boot CPU to avoid nasty surprises */
-        teq   r12, #0
-        bne   skip_bss
-
         PRINT("- Zero BSS -\r\n")
         ldr   r0, =__bss_start       /* Load start & end of bss */
         ldr   r1, =__bss_end
@@ -253,7 +256,6 @@ zero_bss:
         cmp   r0, r1
         blo   1b
 
-skip_bss:
         mov   pc, lr
 ENDPROC(zero_bss)
 
-- 
2.11.0


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

[Xen-devel] [PATCH v2 27/35] xen/arm32: head: Document create_pages_tables()

[Julien Grall]

Document the behavior and the main registers usage within the function.
Note that r6 is now only used within the function, so it does not need
to be part of the common register.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 119d2a0df8..ef8979959b 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -53,7 +53,7 @@
  *   r3  -
  *   r4  -
  *   r5  -
- *   r6  - identity map in place
+ *   r6  -
  *   r7  -
  *   r8  - DTB address (boot CPU only)
  *   r9  - paddr(start)
@@ -301,18 +301,26 @@ cpu_init_done:
         mov   pc, r5                        /* Return address is in r5 */
 ENDPROC(cpu_init)
 
+/*
+ * Rebuild the boot pagetable's first-level entries. The structure
+ * is described in mm.c.
+ *
+ * After the CPU enables paging it will add the fixmap mapping
+ * to these page tables, however this may clash with the 1:1
+ * mapping. So each CPU must rebuild the page tables here with
+ * the 1:1 in place.
+ *
+ * Inputs:
+ *   r9 : paddr(start)
+ *   r10: phys offset
+ *
+ * Clobbers r0 - r6
+ *
+ * Register usage within this function:
+ *   r6 : Identity map in place
+ */
 create_page_tables:
         /*
-         * Rebuild the boot pagetable's first-level entries. The structure
-         * is described in mm.c.
-         *
-         * After the CPU enables paging it will add the fixmap mapping
-         * to these page tables, however this may clash with the 1:1
-         * mapping. So each CPU must rebuild the page tables here with
-         * the 1:1 in place.
-         */
-
-        /*
          * If Xen is loaded at exactly XEN_VIRT_START then we don't
          * need an additional 1:1 mapping, the virtual mapping will
          * suffice.
-- 
2.11.0


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

[Xen-devel] [PATCH v2 28/35] xen/arm32: head: Document enable_mmu()

[Julien Grall]

Document the behavior and the main registers usage within enable_mmu().

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index ef8979959b..4081a52dfa 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -422,6 +422,13 @@ virtphys_clash:
         b     fail
 ENDPROC(create_page_tables)
 
+/*
+ * Turn on the Data Cache and the MMU. The function will return on the 1:1
+ * mapping. In other word, the caller is responsible to switch to the runtime
+ * mapping.
+ *
+ * Clobbers r0 - r3
+ */
 enable_mmu:
         PRINT("- Turning on paging -\r\n")
 
-- 
2.11.0


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

[Xen-devel] [PATCH v2 29/35] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path

[Julien Grall]

The assembly switch to the runtime PT is only necessary for the
secondary CPUs. So move the code in the secondary CPUs path.

While this is definitely not compliant with the Arm Arm as we are
switching between two differents set of page-tables without turning off
the MMU. Turning off the MMU is impossible here as the ID map may clash
with other mappings in the runtime page-tables. This will require more
rework to avoid the problem. So for now add a TODO in the code.

Finally, the code is currently assume that r5 will be properly set to 0
before hand. This is done by create_page_tables() which is called quite
early in the boot process. There are a risk this may be oversight in the
future and therefore breaking secondary CPUs boot. Instead, set r5 to 0
just before using it.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 4081a52dfa..6dc6032498 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -201,6 +201,26 @@ GLOBAL(init_secondary)
         mov   pc, r0
 secondary_switched:
         bl    setup_fixmap
+
+        /*
+         * Non-boot CPUs need to move on to the proper pagetables, which were
+         * setup in init_secondary_pagetables.
+         *
+         * XXX: This is not compliant with the Arm Arm.
+         */
+        ldr   r4, =init_ttbr         /* VA of HTTBR value stashed by CPU 0 */
+        mov   r5, #0
+        ldrd  r4, r5, [r4]           /* Actual value */
+        dsb
+        mcrr  CP64(r4, r5, HTTBR)
+        dsb
+        isb
+        mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLB */
+        mcr   CP32(r0, ICIALLU)      /* Flush I-cache */
+        mcr   CP32(r0, BPIALL)       /* Flush branch predictor */
+        dsb                          /* Ensure completion of TLB+BP flush */
+        isb
+
         b     launch
 ENDPROC(init_secondary)
 
@@ -504,28 +524,6 @@ ENDPROC(setup_fixmap)
 launch:
         PRINT("- Ready -\r\n")
 
-        /* The boot CPU should go straight into C now */
-        teq   r12, #0
-        beq   1f
-
-        /*
-         * Non-boot CPUs need to move on to the proper pagetables, which were
-         * setup in init_secondary_pagetables.
-         */
-
-        ldr   r4, =init_ttbr         /* VA of HTTBR value stashed by CPU 0 */
-        ldrd  r4, r5, [r4]           /* Actual value */
-        dsb
-        mcrr  CP64(r4, r5, HTTBR)
-        dsb
-        isb
-        mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLB */
-        mcr   CP32(r0, ICIALLU)      /* Flush I-cache */
-        mcr   CP32(r0, BPIALL)       /* Flush branch predictor */
-        dsb                          /* Ensure completion of TLB+BP flush */
-        isb
-
-1:
         ldr   r0, =init_data
         add   r0, #INITINFO_stack    /* Find the boot-time stack */
         ldr   sp, [r0]
-- 
2.11.0


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

[Xen-devel] [PATCH v2 30/35] xen/arm32: head: Don't setup the fixmap on secondary CPUs

[Julien Grall]

setup_fixmap() will setup the fixmap in the boot page tables in order to
use earlyprintk and also update the register r11 holding the address to
the UART.

However, secondary CPUs are not using earlyprintk between turning the
MMU on and switching to the runtime page table. So setting up the
fixmap in the boot pages table is pointless.

This means most of setup_fixmap() is not necessary for the secondary
CPUs. The update of UART address is now moved out of setup_fixmap() and
duplicated in the CPU boot and secondary CPUs boot. Additionally, the
call to setup_fixmap() is removed from secondary CPUs boot.

Lastly, take the opportunity to replace load from literal pool with the
new macro mov_w.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 6dc6032498..0a5c3a8525 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -159,6 +159,10 @@ past_zImage:
         mov   pc, r0
 primary_switched:
         bl    setup_fixmap
+#ifdef CONFIG_EARLY_PRINTK
+        /* Use a virtual address to access the UART. */
+        mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
+#endif
         b     launch
 ENDPROC(start)
 
@@ -200,8 +204,6 @@ GLOBAL(init_secondary)
         ldr   r0, =secondary_switched
         mov   pc, r0
 secondary_switched:
-        bl    setup_fixmap
-
         /*
          * Non-boot CPUs need to move on to the proper pagetables, which were
          * setup in init_secondary_pagetables.
@@ -221,6 +223,10 @@ secondary_switched:
         dsb                          /* Ensure completion of TLB+BP flush */
         isb
 
+#ifdef CONFIG_EARLY_PRINTK
+        /* Use a virtual address to access the UART. */
+        mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
+#endif
         b     launch
 ENDPROC(init_secondary)
 
@@ -475,13 +481,6 @@ setup_fixmap:
          */
         dsb
 #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
-        /*
-         * Non-boot CPUs don't need to rebuild the fixmap itself, just
-         * the mapping from boot_second to xen_fixmap
-         */
-        teq   r12, #0
-        bne   1f
-
         /* Add UART to the fixmap table */
         ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
         lsr   r2, r11, #THIRD_SHIFT
@@ -502,9 +501,6 @@ setup_fixmap:
         mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
         mov   r3, #0x0
         strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
-
-        /* Use a virtual address to access the UART. */
-        ldr   r11, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
 
         /*
-- 
2.11.0


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

[Xen-devel] [PATCH v2 31/35] xen/arm32: head: Remove 1:1 mapping as soon as it is not used

[Julien Grall]

The 1:1 mapping may clash with other parts of the Xen virtual memory
layout. At the moment, Xen is handling the clash by only creating a
mapping to the runtime virtual address before enabling the MMU.

The rest of the mappings (such as the fixmap) will be mapped after the
MMU is enabled. However, the code doing the mapping is not safe as it
replace mapping without using the Break-Before-Make sequence.

As the 1:1 mapping can be anywhere in the memory, it is easier to remove
all the entries added as soon as the 1:1 mapping is not used rather than
adding the Break-Before-Make sequence everywhere.

It is difficult to track where exactly the 1:1 mapping was created
without a full rework of create_page_tables(). Instead, introduce a new
function remove_identity_mapping() will look where is the top-level entry
for the 1:1 mapping and remove it.

The new function is only called for the boot CPU. Secondary CPUs will
switch directly to the runtime page-tables so there are no need to
remove the 1:1 mapping. Note that this still doesn't make the Secondary
CPUs path safe but it is not making it worst.

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

---
    It is very likely we will need to re-introduce the 1:1 mapping to cater
    secondary CPUs boot and suspend/resume. For now, the attempt is to make
    boot CPU path fully Arm Arm compliant.

    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 80 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 64 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 0a5c3a8525..56e2d09ed4 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -158,6 +158,13 @@ past_zImage:
         ldr   r0, =primary_switched
         mov   pc, r0
 primary_switched:
+        /*
+         * The 1:1 map may clash with other parts of the Xen virtual memory
+         * layout. As it is not used anymore, remove it completely to
+         * avoid having to worry about replacing existing mapping
+         * afterwards.
+         */
+        bl    remove_identity_mapping
         bl    setup_fixmap
 #ifdef CONFIG_EARLY_PRINTK
         /* Use a virtual address to access the UART. */
@@ -474,12 +481,62 @@ enable_mmu:
         mov   pc, lr
 ENDPROC(enable_mmu)
 
-setup_fixmap:
+/*
+ * Remove the 1:1 map for the page-tables. It is not easy to keep track
+ * where the 1:1 map was mapped, so we will look for the top-level entry
+ * exclusive to the 1:1 map and remove it.
+ *
+ * Inputs:
+ *   r9 : paddr(start)
+ *
+ * Clobbers r0 - r3
+ */
+remove_identity_mapping:
+        /* r2:r3 := invalid page-table entry */
+        mov   r2, #0x0
+        mov   r3, #0x0
         /*
-         * Now we can install the fixmap and dtb mappings, since we
-         * don't need the 1:1 map any more
+         * Find the first slot used. Remove the entry for the first
+         * table if the slot is not 0. For slot 0, the 1:1 mapping was
+         * done in the second table.
          */
-        dsb
+        lsr   r1, r9, #FIRST_SHIFT
+        mov_w r0, LPAE_ENTRY_MASK
+        ands  r1, r1, r0             /* r1 := first slot */
+        beq   1f
+        /* It is not in slot 0, remove the entry */
+        ldr   r0, =boot_pgtable      /* r0 := root table */
+        lsl   r1, r1, #3             /* r1 := Slot offset */
+        strd  r2, r3, [r0, r1]
+        b     identity_mapping_removed
+
+1:
+        /*
+         * Find the second slot used. Remove the entry for the first
+         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
+         * For slot 1, it means the 1:1 mapping was not created.
+         */
+        lsr   r1, r9, #SECOND_SHIFT
+        mov_w r0, LPAE_ENTRY_MASK
+        and   r1, r1, r0             /* r1 := second slot */
+        cmp   r1, #1
+        beq   identity_mapping_removed
+        /* It is not in slot 1, remove the entry */
+        ldr   r0, =boot_second       /* r0 := second table */
+        lsl   r1, r1, #3             /* r1 := Slot offset */
+        strd  r2, r3, [r0, r1]
+
+identity_mapping_removed:
+        /* See asm-arm/arm32/flushtlb.h for the explanation of the sequence. */
+        dsb   nshst
+        mcr   CP32(r0, TLBIALLH)
+        dsb   nsh
+        isb
+
+        mov   pc, lr
+ENDPROC(remove_identity_mapping)
+
+setup_fixmap:
 #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
         /* Add UART to the fixmap table */
         ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
@@ -501,19 +558,10 @@ setup_fixmap:
         mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
         mov   r3, #0x0
         strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
-#endif
-
-        /*
-         * Flush the TLB in case the 1:1 mapping happens to clash with
-         * the virtual addresses used by the fixmap or DTB.
-         */
-        dsb                          /* Ensure any page table updates made above
-                                      * have occurred. */
 
-        isb
-        mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLB */
-        dsb                          /* Ensure completion of TLB flush */
-        isb
+        /* Ensure any page table updates made above have occurred. */
+        dsb   nshst
+#endif
         mov   pc, lr
 ENDPROC(setup_fixmap)
 
-- 
2.11.0


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

[Xen-devel] [PATCH v2 32/35] xen/arm32: head: Rework and document setup_fixmap()

[Julien Grall]

At the moment, the fixmap table is only hooked when earlyprintk is used.
This is fine today because in C land, the fixmap is not used by anyone
until the the boot CPU is switching to the runtime page-tables.

In the future, the boot CPU will not switch between page-tables to
avoid TLB incoherency. Thus, the fixmap table will need to be always
hooked beofre any use. Let's start doing it now in setup_fixmap().

Lastly, document the behavior and the main registers usage within the
function.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 56e2d09ed4..e0f8c2e0cb 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -536,8 +536,21 @@ identity_mapping_removed:
         mov   pc, lr
 ENDPROC(remove_identity_mapping)
 
+/*
+ * Map the UART in the fixmap (when earlyprintk is used) and hook the
+ * fixmap table in the page tables.
+ *
+ * The fixmap cannot be mapped in create_page_tables because it may
+ * clash with the 1:1 mapping.
+ *
+ * Inputs:
+ *   r10: Physical offset
+ *   r11: Early UART base physical address
+ *
+ * Clobbers r1 - r4
+ */
 setup_fixmap:
-#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
+#if defined(CONFIG_EARLY_PRINTK)
         /* Add UART to the fixmap table */
         ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
         lsr   r2, r11, #THIRD_SHIFT
@@ -546,7 +559,7 @@ setup_fixmap:
         orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
         mov   r3, #0x0
         strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
-1:
+#endif
 
         /* Map fixmap into boot_second */
         ldr   r1, =boot_second       /* r1 := vaddr (boot_second) */
@@ -561,7 +574,7 @@ setup_fixmap:
 
         /* Ensure any page table updates made above have occurred. */
         dsb   nshst
-#endif
+
         mov   pc, lr
 ENDPROC(setup_fixmap)
 
-- 
2.11.0


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

[Xen-devel] [PATCH v2 33/35] xen/arm32: head: Rework and document launch()

[Julien Grall]

Boot CPU and secondary CPUs will use different entry point to C code. At
the moment, the decision on which entry to use is taken within launch().

In order to avoid using conditional instruction and make the call
clearer, launch() is reworked to take in parameters the entry point and its
arguments.

Lastly, document the behavior and the main registers usage within the
function.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index e0f8c2e0cb..6d55a2119a 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -170,6 +170,11 @@ primary_switched:
         /* Use a virtual address to access the UART. */
         mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
 #endif
+        PRINT("- Ready -\r\n")
+        /* Setup the arguments for start_xen and jump to C world */
+        mov   r0, r10                /* r0 := Physical offset */
+        mov   r1, r8                 /* r1 := paddr(FDT) */
+        ldr   r2, =start_xen
         b     launch
 ENDPROC(start)
 
@@ -234,6 +239,9 @@ secondary_switched:
         /* Use a virtual address to access the UART. */
         mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
 #endif
+        PRINT("- Ready -\r\n")
+        /* Jump to C world */
+        ldr   r2, =start_secondary
         b     launch
 ENDPROC(init_secondary)
 
@@ -578,19 +586,25 @@ setup_fixmap:
         mov   pc, lr
 ENDPROC(setup_fixmap)
 
+/*
+ * Setup the initial stack and jump to the C world
+ *
+ * Inputs:
+ *   r0 : Argument 0 of the C function to call
+ *   r1 : Argument 1 of the C function to call
+ *   r2 : C entry point
+ *
+ * Clobbers r3
+ */
 launch:
-        PRINT("- Ready -\r\n")
-
-        ldr   r0, =init_data
-        add   r0, #INITINFO_stack    /* Find the boot-time stack */
-        ldr   sp, [r0]
+        ldr   r3, =init_data
+        add   r3, #INITINFO_stack    /* Find the boot-time stack */
+        ldr   sp, [r3]
         add   sp, #STACK_SIZE        /* (which grows down from the top). */
         sub   sp, #CPUINFO_sizeof    /* Make room for CPU save record */
-        teq   r12, #0
-        moveq r0, r10                /* Marshal args: - phys_offset */
-        moveq r1, r8                 /*               - DTB address */
-        beq   start_xen              /* and disappear into the land of C */
-        b     start_secondary        /* (to the appropriate entry point) */
+
+        /* Jump to C world */
+       bx    r2
 ENDPROC(launch)
 
 /* Fail-stop */
-- 
2.11.0


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

[Xen-devel] [PATCH v2 34/35] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb

[Julien Grall]

At the moment, HTTBR is setup in create_page_tables(). This is fine as
it is called by every CPUs.

However, such assumption may not hold in the future. To make change
easier, the HTTBR is not setup in enable_mmu().

Take the opportunity to add the missing isb() to ensure the HTTBR is
seen before the MMU is turned on.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 6d55a2119a..8a1e272aab 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -373,8 +373,6 @@ create_page_tables:
         /* Write Xen's PT's paddr into the HTTBR */
         ldr   r4, =boot_pgtable
         add   r4, r4, r10            /* r4 := paddr (boot_pagetable) */
-        mov   r5, #0                 /* r4:r5 is paddr (boot_pagetable) */
-        mcrr  CP64(r4, r5, HTTBR)
 
         /* Setup boot_pgtable: */
         ldr   r1, =boot_second
@@ -480,6 +478,12 @@ enable_mmu:
         mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLBs */
         dsb   nsh
 
+        ldr   r0, =boot_pgtable
+        add   r0, r0, r10            /* r0 := paddr (boot_pagetable) */
+        mov   r1, #0                 /* r0:r1 is paddr (boot_pagetable) */
+        mcrr  CP64(r0, r1, HTTBR)
+        isb
+
         mrc   CP32(r0, HSCTLR)
         /* Enable MMU and D-cache */
         orr   r0, r0, #(SCTLR_Axx_ELx_M|SCTLR_Axx_ELx_C)
-- 
2.11.0


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

[Xen-devel] [PATCH v2 35/35] xen/arm: Zero BSS after the MMU and D-cache is turned on

[Julien Grall]

At the moment BSS is zeroed before the MMU and D-Cache is turned on.
In other words, the cache will be bypassed when zeroing the BSS section.

On Arm64, per the Image protocol [1], the state of the cache for BSS region
is not known because it is not part of the "loaded kernel image".

On Arm32, the boot protocol [2] does not mention anything about the
state of the cache. Therefore, it should be assumed that it is not known
for BSS region.

This means that the cache will need to be invalidated twice for the BSS
region:
    1) Before zeroing to remove any dirty cache line. Otherwise they may
    get evicted while zeroing and therefore overriding the value.
    2) After zeroing to remove any cache line that may have been
    speculated. Otherwise when turning on MMU and D-Cache, the CPU may
    see old values.

At the moment, the only reason to have BSS zeroed early is because the
boot page tables are part of it. To avoid the two cache invalidations,
it would be better if the boot page tables are part of the "loaded
kernel image" and therefore be zeroed when loading the image into
memory. A good candidate is the section .data.page_aligned.

A new macro DEFINE_BOOT_PAGE_TABLE is introduced to create and mark
page-tables used before BSS is zeroed. This includes all boot_* but also
xen_fixmap as zero_bss() will print a message when earlyprintk is
enabled.

[1] linux/Documentation/arm64/booting.txt
[2] linux/Documentation/arm/Booting

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

---

    Changes in v2:
        - Add missing signed-off
        - Clarify commit message
        - Add arm32 parts
---
 xen/arch/arm/arm32/head.S | 11 +++--------
 xen/arch/arm/arm64/head.S |  7 +++----
 xen/arch/arm/mm.c         | 23 +++++++++++++++++------
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 8a1e272aab..48cad6103f 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -149,7 +149,6 @@ past_zImage:
         mov   r12, #0                /* r12 := is_secondary_cpu */
 
         bl    check_cpu_mode
-        bl    zero_bss
         bl    cpu_init
         bl    create_page_tables
         bl    enable_mmu
@@ -170,6 +169,7 @@ primary_switched:
         /* Use a virtual address to access the UART. */
         mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
 #endif
+        bl    zero_bss
         PRINT("- Ready -\r\n")
         /* Setup the arguments for start_xen and jump to C world */
         mov   r0, r10                /* r0 := Physical offset */
@@ -280,17 +280,12 @@ ENDPROC(check_cpu_mode)
 /*
  * Zero BSS
  *
- * Inputs:
- *   r10: Physical offset
- *
  * Clobbers r0 - r3
  */
 zero_bss:
         PRINT("- Zero BSS -\r\n")
-        ldr   r0, =__bss_start       /* Load start & end of bss */
-        ldr   r1, =__bss_end
-        add   r0, r0, r10            /* Apply physical offset */
-        add   r1, r1, r10
+        ldr   r0, =__bss_start       /* r0 := vaddr(__bss_start) */
+        ldr   r1, =__bss_end         /* r1 := vaddr(__bss_start) */
 
         mov   r2, #0
 1:      str   r2, [r0], #4
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 2287f3ce48..b671e0e59f 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -303,7 +303,6 @@ real_start_efi:
         mov   x22, #0                /* x22 := is_secondary_cpu */
 
         bl    check_cpu_mode
-        bl    zero_bss
         bl    cpu_init
         bl    create_page_tables
         bl    enable_mmu
@@ -324,6 +323,7 @@ primary_switched:
         /* Use a virtual address to access the UART. */
         ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
+        bl    zero_bss
         PRINT("- Ready -\r\n")
         /* Setup the arguments for start_xen and jump to C world */
         mov   x0, x20                /* x0 := Physical offset */
@@ -426,7 +426,6 @@ ENDPROC(check_cpu_mode)
  * Zero BSS
  *
  * Inputs:
- *   x20: Physical offset
  *   x26: Do we need to zero BSS?
  *
  * Clobbers x0 - x3
@@ -436,8 +435,8 @@ zero_bss:
         cbnz  x26, skip_bss
 
         PRINT("- Zero BSS -\r\n")
-        load_paddr x0, __bss_start    /* Load paddr of start & end of bss */
-        load_paddr x1, __bss_end
+        ldr   x0, =__bss_start       /* x0 := vaddr(__bss_start) */
+        ldr   x1, =__bss_end         /* x1 := vaddr(__bss_start) */
 
 1:      str   xzr, [x0], #8
         cmp   x0, x1
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 44258ad89c..670a3089ea 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -62,6 +62,17 @@ mm_printk(const char *fmt, ...) {}
     } while (0);
 #endif
 
+/*
+ * Macros to define page-tables:
+ *  - DEFINE_BOOT_PAGE_TABLE is used to define page-table that are used
+ *  in assembly code before BSS is zeroed.
+ *  - DEFINE_PAGE_TABLE{,S} are used to define one or multiple
+ *  page-tables to be used after BSS is zeroed (typically they are only used
+ *  in C).
+ */
+#define DEFINE_BOOT_PAGE_TABLE(name)                                          \
+lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned") name[LPAE_ENTRIES]
+
 #define DEFINE_PAGE_TABLES(name, nr)                    \
 lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
 
@@ -90,13 +101,13 @@ lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
  * Finally, if EARLY_PRINTK is enabled then xen_fixmap will be mapped
  * by the CPU once it has moved off the 1:1 mapping.
  */
-DEFINE_PAGE_TABLE(boot_pgtable);
+DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
 #ifdef CONFIG_ARM_64
-DEFINE_PAGE_TABLE(boot_first);
-DEFINE_PAGE_TABLE(boot_first_id);
+DEFINE_BOOT_PAGE_TABLE(boot_first);
+DEFINE_BOOT_PAGE_TABLE(boot_first_id);
 #endif
-DEFINE_PAGE_TABLE(boot_second);
-DEFINE_PAGE_TABLE(boot_third);
+DEFINE_BOOT_PAGE_TABLE(boot_second);
+DEFINE_BOOT_PAGE_TABLE(boot_third);
 
 /* Main runtime page tables */
 
@@ -149,7 +160,7 @@ static __initdata int xenheap_first_first_slot = -1;
  */
 static DEFINE_PAGE_TABLES(xen_second, 2);
 /* First level page table used for fixmap */
-DEFINE_PAGE_TABLE(xen_fixmap);
+DEFINE_BOOT_PAGE_TABLE(xen_fixmap);
 /* First level page table used to map Xen itself with the XN bit set
  * as appropriate. */
 static DEFINE_PAGE_TABLE(xen_xenmap);
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v2 01/35] xen/arm64: macros: Introduce an assembly macro to alias x30

[Volodymyr Babchuk]

Julien Grall writes:

> The return address of a function is always stored in x30. For convenience,
> introduce a register alias so "lr" can be used in assembly.
>
> This is defined in asm-arm/arm64/macros.h to allow all assembly files
> to use it.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

>
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm64/entry.S         | 5 -----
>  xen/include/asm-arm/arm64/macros.h | 5 +++++
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 97b05f53ea..2d9a2713a1 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -7,11 +7,6 @@
>  #include <public/xen.h>
>  
>  /*
> - * Register aliases.
> - */
> -lr      .req    x30             /* link register */
> -
> -/*
>   * Stack pushing/popping (register pairs only). Equivalent to store decrement
>   * before, load increment after.
>   */
> diff --git a/xen/include/asm-arm/arm64/macros.h b/xen/include/asm-arm/arm64/macros.h
> index 9c5e676b37..f981b4f43e 100644
> --- a/xen/include/asm-arm/arm64/macros.h
> +++ b/xen/include/asm-arm/arm64/macros.h
> @@ -21,5 +21,10 @@
>      ldr     \dst, [\dst, \tmp]
>      .endm
>  
> +/*
> + * Register aliases.
> + */
> +lr      .req    x30             /* link register */
> +
>  #endif /* __ASM_ARM_ARM64_MACROS_H */


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 01/35] xen/arm64: macros: Introduce an assembly macro to alias x30

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> The return address of a function is always stored in x30. For convenience,
> introduce a register alias so "lr" can be used in assembly.
> 
> This is defined in asm-arm/arm64/macros.h to allow all assembly files
> to use it.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

and committed


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm64/entry.S         | 5 -----
>  xen/include/asm-arm/arm64/macros.h | 5 +++++
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 97b05f53ea..2d9a2713a1 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -7,11 +7,6 @@
>  #include <public/xen.h>
>  
>  /*
> - * Register aliases.
> - */
> -lr      .req    x30             /* link register */
> -
> -/*
>   * Stack pushing/popping (register pairs only). Equivalent to store decrement
>   * before, load increment after.
>   */
> diff --git a/xen/include/asm-arm/arm64/macros.h b/xen/include/asm-arm/arm64/macros.h
> index 9c5e676b37..f981b4f43e 100644
> --- a/xen/include/asm-arm/arm64/macros.h
> +++ b/xen/include/asm-arm/arm64/macros.h
> @@ -21,5 +21,10 @@
>      ldr     \dst, [\dst, \tmp]
>      .endm
>  
> +/*
> + * Register aliases.
> + */
> +lr      .req    x30             /* link register */
> +
>  #endif /* __ASM_ARM_ARM64_MACROS_H */
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 04/35] xen/arm64: head: Rework UART initialization on boot CPU

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Anything executed after the label common_start can be executed on all
> CPUs. However most of the instructions executed between the label
> common_start and init_uart are not executed on the boot CPU.
> 
> The only instructions executed are to lookup the CPUID so it can be
> printed on the console (if earlyprintk is enabled). Printing the CPUID
> is not entirely useful to have for the boot CPU and requires a
> conditional branch to bypass unused instructions.
> 
> Furthermore, the function init_uart is only called for boot CPU
> requiring another conditional branch. This makes the code a bit tricky
> to follow.
> 
> The UART initialization is now moved before the label common_start. This
> now requires to have a slightly altered print for the boot CPU and set
> the early UART base address in each the two path (boot CPU and
> secondary CPUs).
> 
> This has the nice effect to remove a couple of conditional branch in
> the code.
> 
> After this rework, the CPUID is only used at the very beginning of the
> secondary CPUs boot path. So there is no need to "reserve" x24 for the
> CPUID.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

and committed

> ---
>     Changes in v2:
>         - Fold "xen/arm64: head: Don't "reserve" x24 for the CPUID" in
>         this patch
> ---
>  xen/arch/arm/arm64/head.S | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 6afe83c347..b684091aac 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -69,7 +69,7 @@
>   *  x21 - DTB address (boot cpu only)
>   *  x22 - is_secondary_cpu
>   *  x23 - UART address
> - *  x24 - cpuid
> + *  x24 -
>   *  x25 - identity map in place
>   *  x26 - skip_zero_bss
>   *  x27 -
> @@ -265,6 +265,12 @@ real_start_efi:
>          load_paddr x21, _sdtb
>  #endif
>  
> +        /* Initialize the UART if earlyprintk has been enabled. */
> +#ifdef CONFIG_EARLY_PRINTK
> +        bl    init_uart
> +#endif
> +        PRINT("- Boot CPU booting -\r\n")
> +
>          mov   x22, #0                /* x22 := is_secondary_cpu */
>  
>          b     common_start
> @@ -281,14 +287,11 @@ GLOBAL(init_secondary)
>          /* Boot CPU already zero BSS so skip it on secondary CPUs. */
>          mov   x26, #1                /* X26 := skip_zero_bss */
>  
> -common_start:
>          mrs   x0, mpidr_el1
>          ldr   x13, =(~MPIDR_HWID_MASK)
>          bic   x24, x0, x13           /* Mask out flags to get CPU ID */
>  
> -        /* Non-boot CPUs wait here until __cpu_up is ready for them */
> -        cbz   x22, 1f
> -
> +        /* Wait here until __cpu_up is ready to handle the CPU */
>          load_paddr x0, smp_up_cpu
>          dsb   sy
>  2:      ldr   x1, [x0]
> @@ -300,14 +303,14 @@ common_start:
>  
>  #ifdef CONFIG_EARLY_PRINTK
>          ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
> -        cbnz  x22, 1f
> -        bl    init_uart                 /* Boot CPU sets up the UART too */
> -1:      PRINT("- CPU ")
> +        PRINT("- CPU ")
>          mov   x0, x24
>          bl    putn
>          PRINT(" booting -\r\n")
>  #endif
>  
> +common_start:
> +
>          PRINT("- Current EL ")
>          mrs   x4, CurrentEL
>          mov   x0, x4
> @@ -620,10 +623,16 @@ ENTRY(switch_ttbr)
>          ret
>  
>  #ifdef CONFIG_EARLY_PRINTK
> -/* Bring up the UART.
> - * x23: Early UART base address
> - * Clobbers x0-x1 */
> +/*
> + * Initialize the UART. Should only be called on the boot CPU.
> + *
> + * Ouput:
> + *  x23: Early UART base physical address
> + *
> + * Clobbers x0 - x1
> + */
>  init_uart:
> +        ldr   x23, =EARLY_UART_BASE_ADDRESS
>  #ifdef EARLY_PRINTK_INIT_UART
>          early_uart_init x23, 0
>  #endif
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 05/35] xen/arm64: head: Introduce print_reg

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> At the moment, the user should save x30/lr if it cares about it.
> 
> Follow-up patches will introduce more use of putn in place where lr
> should be preserved.
> 
> Furthermore, any user of putn should also move the value to register x0
> if it was stored in a different register.
> 
> For convenience, a new macro is introduced to print a given register.
> The macro will take care for us to move the value to x0 and also
> preserve lr.
> 
> Lastly the new macro is used to replace all the callsite of putn. This
> will simplify rework/review later on.
> 
> Note that CurrentEL is now stored in x5 instead of x4 because the latter
> will be clobbered by the macro print_reg.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

and committed

> ---
>     Changes in v2:
>         - Re-order the instructions to avoid cloberring x4 early. This
>         allows to print content of x4.
> ---
>  xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index b684091aac..63d63bc8ec 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -90,8 +90,25 @@
>          bl    puts    ;    \
>          mov   lr, x3 ;     \
>          RODATA_STR(98, _s)
> +
> +/*
> + * Macro to print the value of register \xb
> + *
> + * Clobbers x0 - x4
> + */
> +.macro print_reg xb
> +        mov   x0, \xb
> +        mov   x4, lr
> +        bl    putn
> +        mov   lr, x4
> +.endm
> +
>  #else /* CONFIG_EARLY_PRINTK */
>  #define PRINT(s)
> +
> +.macro print_reg xb
> +.endm
> +
>  #endif /* !CONFIG_EARLY_PRINTK */
>  
>  /* Load the physical address of a symbol into xb */
> @@ -304,22 +321,20 @@ GLOBAL(init_secondary)
>  #ifdef CONFIG_EARLY_PRINTK
>          ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
>          PRINT("- CPU ")
> -        mov   x0, x24
> -        bl    putn
> +        print_reg x24
>          PRINT(" booting -\r\n")
>  #endif
>  
>  common_start:
>  
>          PRINT("- Current EL ")
> -        mrs   x4, CurrentEL
> -        mov   x0, x4
> -        bl    putn
> +        mrs   x5, CurrentEL
> +        print_reg x5
>          PRINT(" -\r\n")
>  
>          /* Are we in EL2 */
> -        cmp   x4, #PSR_MODE_EL2t
> -        ccmp  x4, #PSR_MODE_EL2h, #0x4, ne
> +        cmp   x5, #PSR_MODE_EL2t
> +        ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
>          b.eq  el2 /* Yes */
>  
>          /* OK, we're boned. */
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 06/35] xen/arm64: head: Introduce distinct paths for the boot CPU and secondary CPUs

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> The boot code is currently quite difficult to go through because of the
> lack of documentation and a number of indirection to avoid executing
> some path in either the boot CPU or secondary CPUs.
> 
> In an attempt to make the boot code easier to follow, each parts of the
> boot are now in separate functions. Furthermore, the paths for the boot
> CPU and secondary CPUs are now distinct and for now will call each
> functions.
> 
> Follow-ups will remove unnecessary calls and do further improvement
> (such as adding documentation and reshuffling).
> 
> Note that the switch from using the 1:1 mapping to the runtime mapping
> is duplicated for each path. This is because in the future we will need
> to stay longer in the 1:1 mapping for the boot CPU.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Avoid infinite loop on boot CPU
>         - Fix typoes in the commit message
>         - s/ID/1:1/
> ---
>  xen/arch/arm/arm64/head.S | 60 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 63d63bc8ec..df797a1573 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -290,7 +290,19 @@ real_start_efi:
>  
>          mov   x22, #0                /* x22 := is_secondary_cpu */
>  
> -        b     common_start
> +        bl    check_cpu_mode
> +        bl    zero_bss
> +        bl    cpu_init
> +        bl    create_page_tables
> +        bl    enable_mmu
> +
> +        /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
> +        ldr   x0, =primary_switched
> +        br    x0
> +primary_switched:
> +        bl    setup_fixmap
> +        b     launch
> +ENDPROC(real_start)
>  
>  GLOBAL(init_secondary)
>          msr   DAIFSet, 0xf           /* Disable all interrupts */
> @@ -324,9 +336,21 @@ GLOBAL(init_secondary)
>          print_reg x24
>          PRINT(" booting -\r\n")
>  #endif
> -
> -common_start:
> -
> +        bl    check_cpu_mode
> +        bl    zero_bss
> +        bl    cpu_init
> +        bl    create_page_tables
> +        bl    enable_mmu
> +
> +        /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
> +        ldr   x0, =secondary_switched
> +        br    x0
> +secondary_switched:
> +        bl    setup_fixmap
> +        b     launch
> +ENDPROC(init_secondary)
> +
> +check_cpu_mode:
>          PRINT("- Current EL ")
>          mrs   x5, CurrentEL
>          print_reg x5
> @@ -343,7 +367,10 @@ common_start:
>          b fail
>  
>  el2:    PRINT("- Xen starting at EL2 -\r\n")
> +        ret
> +ENDPROC(check_cpu_mode)
>  
> +zero_bss:
>          /* Zero BSS only when requested */
>          cbnz  x26, skip_bss
>  
> @@ -356,6 +383,10 @@ el2:    PRINT("- Xen starting at EL2 -\r\n")
>          b.lo  1b
>  
>  skip_bss:
> +        ret
> +ENDPROC(zero_bss)
> +
> +cpu_init:
>          PRINT("- Setting up control registers -\r\n")
>  
>          /* Set up memory attribute type tables */
> @@ -382,7 +413,10 @@ skip_bss:
>           * are handled using the EL2 stack pointer, rather
>           * than SP_EL0. */
>          msr spsel, #1
> +        ret
> +ENDPROC(cpu_init)
>  
> +create_page_tables:
>          /* Rebuild the boot pagetable's first-level entries. The structure
>           * is described in mm.c.
>           *
> @@ -507,6 +541,10 @@ virtphys_clash:
>          b     fail
>  
>  1:
> +        ret
> +ENDPROC(create_page_tables)
> +
> +enable_mmu:
>          PRINT("- Turning on paging -\r\n")
>  
>          /*
> @@ -516,16 +554,16 @@ virtphys_clash:
>          tlbi  alle2                  /* Flush hypervisor TLBs */
>          dsb   nsh
>  
> -        ldr   x1, =paging            /* Explicit vaddr, not RIP-relative */
>          mrs   x0, SCTLR_EL2
>          orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
>          orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
>          dsb   sy                     /* Flush PTE writes and finish reads */
>          msr   SCTLR_EL2, x0          /* now paging is enabled */
>          isb                          /* Now, flush the icache */
> -        br    x1                     /* Get a proper vaddr into PC */
> -paging:
> +        ret
> +ENDPROC(enable_mmu)
>  
> +setup_fixmap:
>          /* Now we can install the fixmap and dtb mappings, since we
>           * don't need the 1:1 map any more */
>          dsb   sy
> @@ -567,11 +605,14 @@ paging:
>          tlbi  alle2
>          dsb   sy                     /* Ensure completion of TLB flush */
>          isb
> +        ret
> +ENDPROC(setup_fixmap)
>  
> +launch:
>          PRINT("- Ready -\r\n")
>  
>          /* The boot CPU should go straight into C now */
> -        cbz   x22, launch
> +        cbz   x22, 1f
>  
>          /* Non-boot CPUs need to move on to the proper pagetables, which were
>           * setup in init_secondary_pagetables. */
> @@ -586,7 +627,7 @@ paging:
>          dsb   sy                     /* Ensure completion of TLB flush */
>          isb
>  
> -launch:
> +1:
>          ldr   x0, =init_data
>          add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
>          ldr   x0, [x0]
> @@ -601,6 +642,7 @@ launch:
>          b     start_xen              /* and disappear into the land of C */
>  1:
>          b     start_secondary        /* (to the appropriate entry point) */
> +ENDPROC(launch)
>  
>  /* Fail-stop */
>  fail:   PRINT("- Boot failed -\r\n")
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 08/35] xen/arm64: head: Rework and document zero_bss()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
> zeroed once at boot. So the call in the secondary CPUs path can be
> removed. It also means that x26 does not need to be set for secondary
> CPU.
> 
> Note that we will need to keep x26 around for the boot CPU as BSS should
> not be reset when booting via UEFI.
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Clarify the commit message
>         - Mention x20 is used as an input
> ---
>  xen/arch/arm/arm64/head.S | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index fbcc792ade..92c8338d71 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -71,7 +71,7 @@
>   *  x23 - UART address
>   *  x24 -
>   *  x25 - identity map in place
> - *  x26 - skip_zero_bss
> + *  x26 - skip_zero_bss (boot cpu only)
>   *  x27 -
>   *  x28 -
>   *  x29 -
> @@ -313,8 +313,6 @@ GLOBAL(init_secondary)
>          sub   x20, x19, x0           /* x20 := phys-offset */
>  
>          mov   x22, #1                /* x22 := is_secondary_cpu */
> -        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
> -        mov   x26, #1                /* X26 := skip_zero_bss */
>  
>          mrs   x0, mpidr_el1
>          ldr   x13, =(~MPIDR_HWID_MASK)
> @@ -337,7 +335,6 @@ GLOBAL(init_secondary)
>          PRINT(" booting -\r\n")
>  #endif
>          bl    check_cpu_mode
> -        bl    zero_bss
>          bl    cpu_init
>          bl    create_page_tables
>          bl    enable_mmu
> @@ -375,6 +372,15 @@ check_cpu_mode:
>          b fail
>  ENDPROC(check_cpu_mode)
>  
> +/*
> + * Zero BSS
> + *
> + * Inputs:
> + *   x20: Physical offset
> + *   x26: Do we need to zero BSS?
> + *
> + * Clobbers x0 - x3
> + */
>  zero_bss:
>          /* Zero BSS only when requested */
>          cbnz  x26, skip_bss
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 09/35] xen/arm64: head: Improve coding style and document cpu_init()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Adjust the coding style used in the comments within cpu_init(). Take the
> opportunity to alter the early print to match the function name.
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - We don't clobber x4 so update the comment
> ---
>  xen/arch/arm/arm64/head.S | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 92c8338d71..ddc5167020 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -397,19 +397,26 @@ skip_bss:
>          ret
>  ENDPROC(zero_bss)
>  
> +/*
> + * Initialize the processor for turning the MMU on.
> + *
> + * Clobbers x0 - x3
> + */
>  cpu_init:
> -        PRINT("- Setting up control registers -\r\n")
> +        PRINT("- Initialize CPU -\r\n")
>  
>          /* Set up memory attribute type tables */
>          ldr   x0, =MAIRVAL
>          msr   mair_el2, x0
>  
> -        /* Set up TCR_EL2:
> +        /*
> +         * Set up TCR_EL2:
>           * PS -- Based on ID_AA64MMFR0_EL1.PARange
>           * Top byte is used
>           * PT walks use Inner-Shareable accesses,
>           * PT walks are write-back, write-allocate in both cache levels,
> -         * 48-bit virtual address space goes through this table. */
> +         * 48-bit virtual address space goes through this table.
> +         */
>          ldr   x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(64-48))
>          /* ID_AA64MMFR0_EL1[3:0] (PARange) corresponds to TCR_EL2[18:16] (PS) */
>          mrs   x1, ID_AA64MMFR0_EL1
> @@ -420,9 +427,11 @@ cpu_init:
>          ldr   x0, =SCTLR_EL2_SET
>          msr   SCTLR_EL2, x0
>  
> -        /* Ensure that any exceptions encountered at EL2
> +        /*
> +         * Ensure that any exceptions encountered at EL2
>           * are handled using the EL2 stack pointer, rather
> -         * than SP_EL0. */
> +         * than SP_EL0.
> +         */
>          msr spsel, #1
>          ret
>  ENDPROC(cpu_init)
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 10/35] xen/arm64: head: Improve coding style and document create_pages_tables()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Adjust the coding style used in the comments within create_pages_tables()
> 
> Lastly, document the behavior and the main registers usage within the
> function. Note that x25 is now only used within the function, so it does
> not need to be part of the common register.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/arm64/head.S | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index ddc5167020..eddf663021 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -70,7 +70,7 @@
>   *  x22 - is_secondary_cpu
>   *  x23 - UART address
>   *  x24 -
> - *  x25 - identity map in place
> + *  x25 -
>   *  x26 - skip_zero_bss (boot cpu only)
>   *  x27 -
>   *  x28 -
> @@ -436,16 +436,27 @@ cpu_init:
>          ret
>  ENDPROC(cpu_init)
>  
> +/*
> + * Rebuild the boot pagetable's first-level entries. The structure
> + * is described in mm.c.
> + *
> + * After the CPU enables paging it will add the fixmap mapping
> + * to these page tables, however this may clash with the 1:1
> + * mapping. So each CPU must rebuild the page tables here with
> + * the 1:1 in place.
> + *
> + * Inputs:
> + *   x19: paddr(start)
> + *   x20: phys offset
> + *
> + * Clobbers x0 - x4, x25
> + *
> + * Register usage within this function:
> + *   x25: Identity map in place
> + */
>  create_page_tables:
> -        /* Rebuild the boot pagetable's first-level entries. The structure
> -         * is described in mm.c.
> -         *
> -         * After the CPU enables paging it will add the fixmap mapping
> -         * to these page tables, however this may clash with the 1:1
> -         * mapping. So each CPU must rebuild the page tables here with
> -         * the 1:1 in place. */
> -
> -        /* If Xen is loaded at exactly XEN_VIRT_START then we don't
> +        /*
> +         * If Xen is loaded at exactly XEN_VIRT_START then we don't
>           * need an additional 1:1 mapping, the virtual mapping will
>           * suffice.
>           */
> @@ -469,7 +480,8 @@ create_page_tables:
>          cbz   x1, 1f                 /* It's in slot 0, map in boot_first
>                                        * or boot_second later on */
>  
> -        /* Level zero does not support superpage mappings, so we have
> +        /*
> +         * Level zero does not support superpage mappings, so we have
>           * to use an extra first level page in which we create a 1GB mapping.
>           */
>          load_paddr x2, boot_first_id
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 11/35] xen/arm64: head: Document enable_mmu()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Document the behavior and the main registers usage within enable_mmu().
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - x2 and x3 are also clobbers. Update the comment accordingly
>         - s/ID/1:1/
> ---
>  xen/arch/arm/arm64/head.S | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index eddf663021..63563ef5e3 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -576,6 +576,13 @@ virtphys_clash:
>          ret
>  ENDPROC(create_page_tables)
>  
> +/*
> + * Turn on the Data Cache and the MMU. The function will return on the 1:1
> + * mapping. In other word, the caller is responsible to switch to the runtime
> + * mapping.
> + *
> + * Clobbers x0 - x3
> + */
>  enable_mmu:
>          PRINT("- Turning on paging -\r\n")
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 15/35] xen/arm64: head: Rework and document setup_fixmap()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> At the moment, the fixmap table is only hooked when earlyprintk is used.
> This is fine today because in C land, the fixmap is not used by anyone
> until the the boot CPU is switching to the runtime page-tables.
> 
> In the future, the boot CPU will not switch between page-tables to
> avoid TLB incoherency. Thus, the fixmap table will need to be always
> hooked beofre any use. Let's start doing it now in setup_fixmap().
         ^ before

Assuming you fix the typo:

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


> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Update the comment to reflect that we clobbers x1 - x4 and not
>         x0 - x1.
>         - Add the list of input registers
>         - s/ID map/1:1 mapping/
>         - Reword the commit message
> ---
>  xen/arch/arm/arm64/head.S | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index a607b3bdb1..f165dd61ca 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -693,8 +693,21 @@ identity_mapping_removed:
>          ret
>  ENDPROC(remove_identity_mapping)
>  
> +/*
> + * Map the UART in the fixmap (when earlyprintk is used) and hook the
> + * fixmap table in the page tables.
> + *
> + * The fixmap cannot be mapped in create_page_tables because it may
> + * clash with the 1:1 mapping.
> + *
> + * Inputs:
> + *   x20: Physical offset
> + *   x23: Early UART base physical address
> + *
> + * Clobbers x1 - x4
> + */
>  setup_fixmap:
> -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
> +#ifdef CONFIG_EARLY_PRINTK
>          /* Add UART to the fixmap table */
>          ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
>          lsr   x2, x23, #THIRD_SHIFT
> @@ -702,6 +715,7 @@ setup_fixmap:
>          mov   x3, #PT_DEV_L3
>          orr   x2, x2, x3             /* x2 := 4K dev map including UART */
>          str   x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
> +#endif
>  
>          /* Map fixmap into boot_second */
>          ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> @@ -714,7 +728,7 @@ setup_fixmap:
>  
>          /* Ensure any page table updates made above have occurred. */
>          dsb   nshst
> -#endif
> +
>          ret
>  ENDPROC(setup_fixmap)
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 16/35] xen/arm64: head: Rework and document launch()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Boot CPU and secondary CPUs will use different entry point to C code. At
> the moment, the decision on which entry to use is taken within launch().
> 
> In order to avoid a branch for the decision and make the code clearer,
> launch() is reworked to take in parameters the entry point and its
> arguments.
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Use x3 instead of x4
>         - Add a clobbers section
> ---
>  xen/arch/arm/arm64/head.S | 43 +++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index f165dd61ca..7541635102 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -312,6 +312,11 @@ primary_switched:
>          /* Use a virtual address to access the UART. */
>          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> +        PRINT("- Ready -\r\n")
> +        /* Setup the arguments for start_xen and jump to C world */
> +        mov   x0, x20                /* x0 := Physical offset */
> +        mov   x1, x21                /* x1 := paddr(FDT) */
> +        ldr   x2, =start_xen
>          b     launch
>  ENDPROC(real_start)
>  
> @@ -374,6 +379,9 @@ secondary_switched:
>          /* Use a virtual address to access the UART. */
>          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> +        PRINT("- Ready -\r\n")
> +        /* Jump to C world */
> +        ldr   x2, =start_secondary
>          b     launch
>  ENDPROC(init_secondary)
>  
> @@ -732,23 +740,26 @@ setup_fixmap:
>          ret
>  ENDPROC(setup_fixmap)
>  
> +/*
> + * Setup the initial stack and jump to the C world
> + *
> + * Inputs:
> + *   x0 : Argument 0 of the C function to call
> + *   x1 : Argument 1 of the C function to call
> + *   x2 : C entry point
> + *
> + * Clobbers x3
> + */
>  launch:
> -        PRINT("- Ready -\r\n")
> -
> -        ldr   x0, =init_data
> -        add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
> -        ldr   x0, [x0]
> -        add   x0, x0, #STACK_SIZE    /* (which grows down from the top). */
> -        sub   x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */
> -        mov   sp, x0
> -
> -        cbnz  x22, 1f
> -
> -        mov   x0, x20                /* Marshal args: - phys_offset */
> -        mov   x1, x21                /*               - FDT */
> -        b     start_xen              /* and disappear into the land of C */
> -1:
> -        b     start_secondary        /* (to the appropriate entry point) */
> +        ldr   x3, =init_data
> +        add   x3, x3, #INITINFO_stack /* Find the boot-time stack */
> +        ldr   x3, [x3]
> +        add   x3, x3, #STACK_SIZE    /* (which grows down from the top). */
                                        ^ please move 1 space to the
                                        right

Aside from this minor code style thing

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


> +        sub   x3, x3, #CPUINFO_sizeof /* Make room for CPU save record */
> +        mov   sp, x3
> +
> +        /* Jump to C world */
> +        br    x2
>  ENDPROC(launch)
>  
>  /* Fail-stop */
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 17/35] xen/arm64: head: Setup TTBR_EL2 in enable_mmu() and add missing isb

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> At the moment, TTBR_EL2 is setup in create_page_tables(). This is fine
> as it is called by every CPUs.
> 
> However, such assumption may not hold in the future. To make change
> easier, the TTBR_EL2 is not setup in enable_mmu().
> 
> Take the opportunity to add the missing isb() to ensure the TTBR_EL2 is
> seen before the MMU is turned on.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Makes sense

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


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm64/head.S | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 7541635102..9afd89d447 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -501,9 +501,7 @@ create_page_tables:
>          cmp   x19, #XEN_VIRT_START
>          cset  x25, eq                /* x25 := identity map in place, or not */
>  
> -        /* Write Xen's PT's paddr into TTBR0_EL2 */
>          load_paddr x4, boot_pgtable
> -        msr   TTBR0_EL2, x4
>  
>          /* Setup boot_pgtable: */
>          load_paddr x1, boot_first
> @@ -631,6 +629,11 @@ enable_mmu:
>          tlbi  alle2                  /* Flush hypervisor TLBs */
>          dsb   nsh
>  
> +        /* Write Xen's PT's paddr into TTBR0_EL2 */
> +        load_paddr x0, boot_pgtable
> +        msr   TTBR0_EL2, x0
> +        isb
> +
>          mrs   x0, SCTLR_EL2
>          orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
>          orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 18/35] xen/arm64: head: Introduce a macro to get a PC-relative address of a symbol

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Arm64 provides instructions to load a PC-relative address, but with some
> limitations:
>    - adr is enable to cope with +/-1MB
>    - adrp is enale to cope with +/-4GB but relative to a 4KB page
>      address
> 
> Because of that, the code requires to use 2 instructions to load any Xen
> symbol. To make the code more obvious, introducing a new macro adr_l is
> introduced.
> 
> The new macro is used to replace a couple of open-coded use in
> efi_xen_start.
> 
> The macro is copied from Linux 5.2-rc4.

I was going to ask why the strange name "adr_l", now I know why :-)

I'd suggest to name it more clearly to maybe "adr_relative"?
In any case:

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


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

Typo in your address


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm64/head.S | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 9afd89d447..2287f3ce48 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -111,6 +111,18 @@
>  
>  #endif /* !CONFIG_EARLY_PRINTK */
>  
> +/*
> + * Pseudo-op for PC relative adr <reg>, <symbol> where <symbol> is
> + * within the range +/- 4GB of the PC.
> + *
> + * @dst: destination register (64 bit wide)
> + * @sym: name of the symbol
> + */
> +.macro  adr_l, dst, sym
> +        adrp \dst, \sym
> +        add  \dst, \dst, :lo12:\sym
> +.endm
> +
>  /* Load the physical address of a symbol into xb */
>  .macro load_paddr xb, sym
>          ldr \xb, =\sym
> @@ -886,11 +898,9 @@ ENTRY(efi_xen_start)
>           * Flush dcache covering current runtime addresses
>           * of xen text/data. Then flush all of icache.
>           */
> -        adrp  x1, _start
> -        add   x1, x1, #:lo12:_start
> +        adr_l x1, _start
>          mov   x0, x1
> -        adrp  x2, _end
> -        add   x2, x2, #:lo12:_end
> +        adr_l x2, _end
>          sub   x1, x2, x1
>  
>          bl    __flush_dcache_area
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 20/35] xen/arm32: head: Mark the end of subroutines with ENDPROC

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> putn() and puts() are two subroutines. Add ENDPROC for the benefits of
> static analysis tools and the reader.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 99f4af18d8..8b4c8a4714 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -518,6 +518,7 @@ puts:
>          moveq pc, lr
>          early_uart_transmit r11, r1
>          b puts
> +ENDPROC(puts)
>  
>  /*
>   * Print a 32-bit number in hex.  Specific to the PL011 UART.
> @@ -537,6 +538,7 @@ putn:
>          subs  r3, r3, #1
>          bne   1b
>          mov   pc, lr
> +ENDPROC(putn)
>  
>  hex:    .ascii "0123456789abcdef"
>          .align 2
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 21/35] xen/arm32: head: Don't clobber r14/lr in the macro PRINT

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> The current implementation of the macro PRINT will clobber r14/lr. This
> means the user should save r14 if it cares about it.
> 
> Follow-up patches will introduce more use of PRINT in places where lr
> should be preserved. Rather than requiring all the user to preserve lr,
> the macro PRINT is modified to save and restore it.
> 
> While the comment state r3 will be clobbered, this is not the case. So
> PRINT will use r3 to preserve lr.
> 
> Lastly, take the opportunity to move the comment on top of PRINT and use
> PRINT in init_uart. Both changes will be helpful in a follow-up patch.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 8b4c8a4714..b54331c19d 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -64,15 +64,20 @@
>   *   r14 - LR
>   *   r15 - PC
>   */
> -/* Macro to print a string to the UART, if there is one.
> - * Clobbers r0-r3. */
>  #ifdef CONFIG_EARLY_PRINTK
> -#define PRINT(_s)       \
> -        adr   r0, 98f ; \
> -        bl    puts    ; \
> -        b     99f     ; \
> -98:     .asciz _s     ; \
> -        .align 2      ; \
> +/*
> + * Macro to print a string to the UART, if there is one.
> + *
> + * Clobbers r0 - r3
> + */
> +#define PRINT(_s)           \
> +        mov   r3, lr       ;\
> +        adr   r0, 98f      ;\
> +        bl    puts         ;\
> +        mov   lr, r3       ;\
> +        b     99f          ;\
> +98:     .asciz _s          ;\
> +        .align 2           ;\
>  99:
>  #else /* CONFIG_EARLY_PRINTK */
>  #define PRINT(s)
> @@ -500,10 +505,8 @@ init_uart:
>  #ifdef EARLY_PRINTK_INIT_UART
>          early_uart_init r11, r1, r2
>  #endif
> -        adr   r0, 1f
> -        b     puts                  /* Jump to puts */
> -1:      .asciz "- UART enabled -\r\n"
> -        .align 4
> +        PRINT("- UART enabled -\r\n")
> +        mov   pc, lr
>  
>  /*
>   * Print early debug messages.
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 22/35] xen/arm32: head: Rework UART initialization on boot CPU

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Anything executed after the label common_start can be executed on all
> CPUs. However most of the instructions executed between the label
> common_start and init_uart are not executed on the boot CPU.
> 
> The only instructions executed are to lookup the CPUID so it can be
> printed on the console (if earlyprintk is enabled). Printing the CPUID
> is not entirely useful to have for the boot CPU and requires a
> conditional branch to bypass unused instructions.
> 
> Furthermore, the function init_uart is only called for boot CPU
> requiring another conditional branch. This makes the code a bit tricky
> to follow.
> 
> The UART initialization is now moved before the label common_start. This
> now requires to have a slightly altered print for the boot CPU and set
> the early UART base address in each the two path (boot CPU and
> secondary CPUs).
> 
> This has the nice effect to remove a couple of conditional branch in
> the code.
> 
> After this rework, the CPUID is only used at the very beginning of the
> secondary CPUs boot path. So there is no need to "reserve" x24 for the
> CPUID.
> 
> Lastly, take the opportunity to replace load from literal pool with the
> new macro mov_w.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index b54331c19d..134c3dda92 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -54,7 +54,7 @@
>   *   r4  -
>   *   r5  -
>   *   r6  - identity map in place
> - *   r7  - CPUID
> + *   r7  -
>   *   r8  - DTB address (boot CPU only)
>   *   r9  - paddr(start)
>   *   r10 - phys offset
> @@ -123,6 +123,12 @@ past_zImage:
>          add   r8, r10                /* r8 := paddr(DTB) */
>  #endif
>  
> +        /* Initialize the UART if earlyprintk has been enabled. */
> +#ifdef CONFIG_EARLY_PRINTK
> +        bl    init_uart
> +#endif
> +        PRINT("- Boot CPU booting -\r\n")
> +
>          mov   r12, #0                /* r12 := is_secondary_cpu */
>  
>          b     common_start
> @@ -137,14 +143,9 @@ GLOBAL(init_secondary)
>  
>          mov   r12, #1                /* r12 := is_secondary_cpu */
>  
> -common_start:
>          mrc   CP32(r1, MPIDR)
>          bic   r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
>  
> -        /* Non-boot CPUs wait here until __cpu_up is ready for them */
> -        teq   r12, #0
> -        beq   1f
> -
>          ldr   r0, =smp_up_cpu
>          add   r0, r0, r10            /* Apply physical offset */
>          dsb
> @@ -156,15 +157,14 @@ common_start:
>  1:
>  
>  #ifdef CONFIG_EARLY_PRINTK
> -        ldr   r11, =EARLY_UART_BASE_ADDRESS  /* r11 := UART base address */
> -        teq   r12, #0                /* Boot CPU sets up the UART too */
> -        bleq  init_uart
> +        mov_w r11, EARLY_UART_BASE_ADDRESS   /* r11 := UART base address */
>          PRINT("- CPU ")
>          mov   r0, r7
>          bl    putn
>          PRINT(" booting -\r\n")
>  #endif
>  
> +common_start:
>          /* Check that this CPU has Hyp mode */
>          mrc   CP32(r0, ID_PFR1)
>          and   r0, r0, #0xf000        /* Bits 12-15 define virt extensions */
> @@ -497,11 +497,15 @@ ENTRY(switch_ttbr)
>  
>  #ifdef CONFIG_EARLY_PRINTK
>  /*
> - * Bring up the UART.
> - * r11: Early UART base address
> - * Clobbers r0-r2
> + * Initialize the UART. Should only be called on the boot CPU.
> + *
> + * Ouput:
      ^ this should be output, and in the arm64 patch too (already committed)

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



> + *  r11: Early UART base physical address
> + *
> + * Clobbers r0 - r3
>   */
>  init_uart:
> +        mov_w r11, EARLY_UART_BASE_ADDRESS
>  #ifdef EARLY_PRINTK_INIT_UART
>          early_uart_init r11, r1, r2
>  #endif


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

Re: [Xen-devel] [PATCH v2 23/35] xen/arm32: head: Introduce print_reg

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> At the moment, the user should save r14/lr if it cares about it.
> 
> Follow-up patches will introduce more use of putn in place where lr
> should be preserved.
> 
> Furthermore, any user of putn should also move the value to register r0
> if it was stored in a different register.
> 
> For convenience, a new macro is introduced to print a given register.
> The macro will take care for us to move the value to r0 and also
> preserve lr.
> 
> Lastly the new macro is used to replace all the callsite of putn. This
> will simplify rework/review later on.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 134c3dda92..bbcfdcd351 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -79,8 +79,25 @@
>  98:     .asciz _s          ;\
>          .align 2           ;\
>  99:
> +
> +/*
> + * Macro to print the value of register \rb
> + *
> + * Clobbers r0 - r4
> + */
> +.macro print_reg rb
> +        mov   r0, \rb
> +        mov   r4, lr
> +        bl    putn
> +        mov   lr, r4
> +.endm
> +
>  #else /* CONFIG_EARLY_PRINTK */
>  #define PRINT(s)
> +
> +.macro print_reg rb
> +.endm
> +
>  #endif /* !CONFIG_EARLY_PRINTK */
>  
>          .arm
> @@ -159,8 +176,7 @@ GLOBAL(init_secondary)
>  #ifdef CONFIG_EARLY_PRINTK
>          mov_w r11, EARLY_UART_BASE_ADDRESS   /* r11 := UART base address */
>          PRINT("- CPU ")
> -        mov   r0, r7
> -        bl    putn
> +        print_reg r7
>          PRINT(" booting -\r\n")
>  #endif
>  
> @@ -211,8 +227,7 @@ skip_bss:
>          bne   1f
>          mov   r4, r0
>          PRINT("- Missing processor info: ")
> -        mov   r0, r4
> -        bl    putn
> +        print_reg r4
>          PRINT(" -\r\n")
>          b     fail
>  1:
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 18/35] xen/arm64: head: Introduce a macro to get a PC-relative address of a symbol

[Julien Grall]

Hi,

On 7/30/19 7:24 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> Arm64 provides instructions to load a PC-relative address, but with some
>> limitations:
>>     - adr is enable to cope with +/-1MB
>>     - adrp is enale to cope with +/-4GB but relative to a 4KB page
>>       address
>>
>> Because of that, the code requires to use 2 instructions to load any Xen
>> symbol. To make the code more obvious, introducing a new macro adr_l is
>> introduced.
>>
>> The new macro is used to replace a couple of open-coded use in
>> efi_xen_start.
>>
>> The macro is copied from Linux 5.2-rc4.
> 
> I was going to ask why the strange name "adr_l", now I know why :-)

I think this stands for "load".

> 
> I'd suggest to name it more clearly to maybe "adr_relative"?

This is a bit weird to have one full word and the other one shorten. The 
current solution has the advantage to be short and therefore looks like 
an instruction (and so keep everything correctly aligned).

So I would prefer to keep the function as is.

> In any case:
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Thank you.

> 
>> Signed-off-by: Julien Grall <julien.grall@arm.coM>
> 
> Typo in your address
> 
> 
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm64/head.S | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 9afd89d447..2287f3ce48 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -111,6 +111,18 @@
>>   
>>   #endif /* !CONFIG_EARLY_PRINTK */
>>   
>> +/*
>> + * Pseudo-op for PC relative adr <reg>, <symbol> where <symbol> is
>> + * within the range +/- 4GB of the PC.
>> + *
>> + * @dst: destination register (64 bit wide)
>> + * @sym: name of the symbol
>> + */
>> +.macro  adr_l, dst, sym
>> +        adrp \dst, \sym
>> +        add  \dst, \dst, :lo12:\sym
>> +.endm
>> +
>>   /* Load the physical address of a symbol into xb */
>>   .macro load_paddr xb, sym
>>           ldr \xb, =\sym
>> @@ -886,11 +898,9 @@ ENTRY(efi_xen_start)
>>            * Flush dcache covering current runtime addresses
>>            * of xen text/data. Then flush all of icache.
>>            */
>> -        adrp  x1, _start
>> -        add   x1, x1, #:lo12:_start
>> +        adr_l x1, _start
>>           mov   x0, x1
>> -        adrp  x2, _end
>> -        add   x2, x2, #:lo12:_end
>> +        adr_l x2, _end
>>           sub   x1, x2, x1
>>   
>>           bl    __flush_dcache_area
>> -- 
>> 2.11.0
>>

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 24/35] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> The boot code is currently quite difficult to go through because of the
> lack of documentation and a number of indirection to avoid executing
> some path in either the boot CPU or secondary CPUs.
> 
> In an attempt to make the boot code easier to follow, each parts of the
> boot are now in separate functions. Furthermore, the paths for the boot
> CPU and secondary CPUs are now distinct and for now will call each
> functions.
> 
> Follow-ups will remove unnecessary calls and do further improvement
> (such as adding documentation and reshuffling).
> 
> Note that the switch from using the ID mapping to the runtime mapping
> is duplicated for each path. This is because in the future we will need
> to stay longer in the ID mapping for the boot CPU.
> 
> Lastly, it is now required to save lr in cpu_init() becauswe the
> function will call other functions and therefore clobber lr.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 64 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 53 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index bbcfdcd351..13793e85d8 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -148,7 +148,19 @@ past_zImage:
>  
>          mov   r12, #0                /* r12 := is_secondary_cpu */
>  
> -        b     common_start
> +        bl    check_cpu_mode
> +        bl    zero_bss
> +        bl    cpu_init
> +        bl    create_page_tables
> +        bl    enable_mmu
> +
> +        /* We are still in the ID map. Jump to the runtime Virtual Address */

The arm64 patch has been switched to use "1:1", it would be good to be
consistent. In the commit message too.


> +        ldr   r0, =primary_switched
> +        mov   pc, r0
> +primary_switched:
> +        bl    setup_fixmap
> +        b     launch
> +ENDPROC(start)
>  
>  GLOBAL(init_secondary)
>          cpsid aif                    /* Disable all interrupts */
> @@ -179,8 +191,21 @@ GLOBAL(init_secondary)
>          print_reg r7
>          PRINT(" booting -\r\n")
>  #endif
> -
> -common_start:
> +        bl    check_cpu_mode
> +        bl    zero_bss
> +        bl    cpu_init
> +        bl    create_page_tables
> +        bl    enable_mmu
> +
> +        /* We are still in the ID map. Jump to the runtime Virtual Address. */

Same here.


> +        ldr   r0, =secondary_switched
> +        mov   pc, r0
> +secondary_switched:
> +        bl    setup_fixmap
> +        b     launch
> +ENDPROC(init_secondary)
> +
> +check_cpu_mode:
>          /* Check that this CPU has Hyp mode */
>          mrc   CP32(r0, ID_PFR1)
>          and   r0, r0, #0xf000        /* Bits 12-15 define virt extensions */
> @@ -202,7 +227,10 @@ common_start:
>          b     fail
>  
>  hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
> +        mov   pc, lr
> +ENDPROC(check_cpu_mode)
>  
> +zero_bss:
>          /* Zero BSS On the boot CPU to avoid nasty surprises */
>          teq   r12, #0
>          bne   skip_bss
> @@ -219,8 +247,14 @@ hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
>          blo   1b
>  
>  skip_bss:
> +        mov   pc, lr
> +ENDPROC(zero_bss)
> +
> +cpu_init:
>          PRINT("- Setting up control registers -\r\n")
>  
> +        mov   r5, lr			            /* r5 := return address */

Please align the comment with the others in this proc.

Other than these minor comments the patch looks fine. Have you had a
chance to test it on real hardware?


> +
>          /* Get processor specific proc info into r1 */
>          bl    __lookup_processor_type
>          teq   r1, #0
> @@ -231,7 +265,6 @@ skip_bss:
>          PRINT(" -\r\n")
>          b     fail
>  1:
> -
>          /* Jump to cpu_init */
>          ldr   r1, [r1, #PROCINFO_cpu_init]  /* r1 := vaddr(init func) */
>          adr   lr, cpu_init_done             /* Save return address */
> @@ -256,6 +289,10 @@ cpu_init_done:
>          ldr   r0, =HSCTLR_SET
>          mcr   CP32(r0, HSCTLR)
>  
> +        mov   pc, r5                        /* Return address is in r5 */
> +ENDPROC(cpu_init)
> +
> +create_page_tables:
>          /*
>           * Rebuild the boot pagetable's first-level entries. The structure
>           * is described in mm.c.
> @@ -359,15 +396,16 @@ cpu_init_done:
>          /* boot pagetable setup complete */
>  
>          cmp   r6, #1                /* Did we manage to create an identity mapping ? */
> -        beq   1f
> +        moveq pc, lr
>          PRINT("Unable to build boot page tables - Failed to identity map Xen.\r\n")
>          b     fail
>  virtphys_clash:
>          /* Identity map clashes with boot_third, which we cannot handle yet */
>          PRINT("- Unable to build boot page tables - virt and phys addresses clash. -\r\n")
>          b     fail
> +ENDPROC(create_page_tables)
>  
> -1:
> +enable_mmu:
>          PRINT("- Turning on paging -\r\n")
>  
>          /*
> @@ -377,16 +415,16 @@ virtphys_clash:
>          mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLBs */
>          dsb   nsh
>  
> -        ldr   r1, =paging            /* Explicit vaddr, not RIP-relative */
>          mrc   CP32(r0, HSCTLR)
>          /* Enable MMU and D-cache */
>          orr   r0, r0, #(SCTLR_Axx_ELx_M|SCTLR_Axx_ELx_C)
>          dsb                          /* Flush PTE writes and finish reads */
>          mcr   CP32(r0, HSCTLR)       /* now paging is enabled */
>          isb                          /* Now, flush the icache */
> -        mov   pc, r1                 /* Get a proper vaddr into PC */
> -paging:
> +        mov   pc, lr
> +ENDPROC(enable_mmu)
>  
> +setup_fixmap:
>          /*
>           * Now we can install the fixmap and dtb mappings, since we
>           * don't need the 1:1 map any more
> @@ -436,12 +474,15 @@ paging:
>          mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLB */
>          dsb                          /* Ensure completion of TLB flush */
>          isb
> +        mov   pc, lr
> +ENDPROC(setup_fixmap)
>  
> +launch:
>          PRINT("- Ready -\r\n")
>  
>          /* The boot CPU should go straight into C now */
>          teq   r12, #0
> -        beq   launch
> +        beq   1f
>  
>          /*
>           * Non-boot CPUs need to move on to the proper pagetables, which were
> @@ -460,7 +501,7 @@ paging:
>          dsb                          /* Ensure completion of TLB+BP flush */
>          isb
>  
> -launch:
> +1:
>          ldr   r0, =init_data
>          add   r0, #INITINFO_stack    /* Find the boot-time stack */
>          ldr   sp, [r0]
> @@ -471,6 +512,7 @@ launch:
>          moveq r1, r8                 /*               - DTB address */
>          beq   start_xen              /* and disappear into the land of C */
>          b     start_secondary        /* (to the appropriate entry point) */
> +ENDPROC(launch)
>  
>  /* Fail-stop */
>  fail:   PRINT("- Boot failed -\r\n")
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 25/35] xen/arm32: head: Rework and document check_cpu_mode()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> A branch in the success case can be avoided by inverting the branch
> condition. At the same time, remove a pointless comment as Xen can only
> run at Hypervisor Mode.
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 13793e85d8..0959c4f044 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -205,6 +205,16 @@ secondary_switched:
>          b     launch
>  ENDPROC(init_secondary)
>  
> +/*
> + * Check if the CPU supports virtualization extensions and has been booted
> + * in Hypervisor mode.
> + *
> + * This function will never return when the CPU doesn't support
> + * virtualization extensions or is booted in another mode than
> + * Hypervisor mode.
> + *
> + * Clobbers r0 - r3
> + */
>  check_cpu_mode:
>          /* Check that this CPU has Hyp mode */
>          mrc   CP32(r0, ID_PFR1)
> @@ -219,15 +229,12 @@ check_cpu_mode:
>          mrs   r0, cpsr
>          and   r0, r0, #0x1f          /* Mode is in the low 5 bits of CPSR */
>          teq   r0, #0x1a              /* Hyp Mode? */
> -        beq   hyp
> +        moveq pc, lr                 /* Yes, return */
>  
>          /* OK, we're boned. */
>          PRINT("- Xen must be entered in NS Hyp mode -\r\n")
>          PRINT("- Please update the bootloader -\r\n")
>          b     fail
> -
> -hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
> -        mov   pc, lr
>  ENDPROC(check_cpu_mode)
>  
>  zero_bss:
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 26/35] xen/arm32: head: Rework and document zero_bss()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
> zeroed once at boot. So the call in the secondary CPUs path can be
> removed.
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 0959c4f044..119d2a0df8 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -192,7 +192,6 @@ GLOBAL(init_secondary)
>          PRINT(" booting -\r\n")
>  #endif
>          bl    check_cpu_mode
> -        bl    zero_bss
>          bl    cpu_init
>          bl    create_page_tables
>          bl    enable_mmu
> @@ -237,11 +236,15 @@ check_cpu_mode:
>          b     fail
>  ENDPROC(check_cpu_mode)
>  
> +/*
> + * Zero BSS
> + *
> + * Inputs:
> + *   r10: Physical offset
> + *
> + * Clobbers r0 - r3
> + */
>  zero_bss:
> -        /* Zero BSS On the boot CPU to avoid nasty surprises */
> -        teq   r12, #0
> -        bne   skip_bss
> -
>          PRINT("- Zero BSS -\r\n")
>          ldr   r0, =__bss_start       /* Load start & end of bss */
>          ldr   r1, =__bss_end
> @@ -253,7 +256,6 @@ zero_bss:
>          cmp   r0, r1
>          blo   1b
>  
> -skip_bss:
>          mov   pc, lr
>  ENDPROC(zero_bss)
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 27/35] xen/arm32: head: Document create_pages_tables()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Document the behavior and the main registers usage within the function.
> Note that r6 is now only used within the function, so it does not need
> to be part of the common register.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 119d2a0df8..ef8979959b 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -53,7 +53,7 @@
>   *   r3  -
>   *   r4  -
>   *   r5  -
> - *   r6  - identity map in place
> + *   r6  -
>   *   r7  -
>   *   r8  - DTB address (boot CPU only)
>   *   r9  - paddr(start)
> @@ -301,18 +301,26 @@ cpu_init_done:
>          mov   pc, r5                        /* Return address is in r5 */
>  ENDPROC(cpu_init)
>  
> +/*
> + * Rebuild the boot pagetable's first-level entries. The structure
> + * is described in mm.c.
> + *
> + * After the CPU enables paging it will add the fixmap mapping
> + * to these page tables, however this may clash with the 1:1
> + * mapping. So each CPU must rebuild the page tables here with
> + * the 1:1 in place.
> + *
> + * Inputs:
> + *   r9 : paddr(start)
> + *   r10: phys offset
> + *
> + * Clobbers r0 - r6
> + *
> + * Register usage within this function:
> + *   r6 : Identity map in place
> + */
>  create_page_tables:
>          /*
> -         * Rebuild the boot pagetable's first-level entries. The structure
> -         * is described in mm.c.
> -         *
> -         * After the CPU enables paging it will add the fixmap mapping
> -         * to these page tables, however this may clash with the 1:1
> -         * mapping. So each CPU must rebuild the page tables here with
> -         * the 1:1 in place.
> -         */
> -
> -        /*
>           * If Xen is loaded at exactly XEN_VIRT_START then we don't
>           * need an additional 1:1 mapping, the virtual mapping will
>           * suffice.
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 28/35] xen/arm32: head: Document enable_mmu()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Document the behavior and the main registers usage within enable_mmu().
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index ef8979959b..4081a52dfa 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -422,6 +422,13 @@ virtphys_clash:
>          b     fail
>  ENDPROC(create_page_tables)
>  
> +/*
> + * Turn on the Data Cache and the MMU. The function will return on the 1:1
> + * mapping. In other word, the caller is responsible to switch to the runtime
> + * mapping.
> + *
> + * Clobbers r0 - r3
> + */
>  enable_mmu:
>          PRINT("- Turning on paging -\r\n")
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 29/35] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> The assembly switch to the runtime PT is only necessary for the
> secondary CPUs. So move the code in the secondary CPUs path.
> 
> While this is definitely not compliant with the Arm Arm as we are
> switching between two differents set of page-tables without turning off
> the MMU. Turning off the MMU is impossible here as the ID map may clash
> with other mappings in the runtime page-tables. This will require more
> rework to avoid the problem. So for now add a TODO in the code.
> 
> Finally, the code is currently assume that r5 will be properly set to 0
> before hand. This is done by create_page_tables() which is called quite
> early in the boot process. There are a risk this may be oversight in the
> future and therefore breaking secondary CPUs boot. Instead, set r5 to 0
> just before using it.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 4081a52dfa..6dc6032498 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -201,6 +201,26 @@ GLOBAL(init_secondary)
>          mov   pc, r0
>  secondary_switched:
>          bl    setup_fixmap
> +
> +        /*
> +         * Non-boot CPUs need to move on to the proper pagetables, which were
> +         * setup in init_secondary_pagetables.
> +         *
> +         * XXX: This is not compliant with the Arm Arm.
> +         */
> +        ldr   r4, =init_ttbr         /* VA of HTTBR value stashed by CPU 0 */
> +        mov   r5, #0

Why do we need to zero r5? Shouldn't ldrd overwrite r5 anyway?


> +        ldrd  r4, r5, [r4]           /* Actual value */
> +        dsb
> +        mcrr  CP64(r4, r5, HTTBR)
> +        dsb
> +        isb
> +        mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLB */
> +        mcr   CP32(r0, ICIALLU)      /* Flush I-cache */
> +        mcr   CP32(r0, BPIALL)       /* Flush branch predictor */
> +        dsb                          /* Ensure completion of TLB+BP flush */
> +        isb
> +
>          b     launch
>  ENDPROC(init_secondary)
>  
> @@ -504,28 +524,6 @@ ENDPROC(setup_fixmap)
>  launch:
>          PRINT("- Ready -\r\n")
>  
> -        /* The boot CPU should go straight into C now */
> -        teq   r12, #0
> -        beq   1f
> -
> -        /*
> -         * Non-boot CPUs need to move on to the proper pagetables, which were
> -         * setup in init_secondary_pagetables.
> -         */
> -
> -        ldr   r4, =init_ttbr         /* VA of HTTBR value stashed by CPU 0 */
> -        ldrd  r4, r5, [r4]           /* Actual value */
> -        dsb
> -        mcrr  CP64(r4, r5, HTTBR)
> -        dsb
> -        isb
> -        mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLB */
> -        mcr   CP32(r0, ICIALLU)      /* Flush I-cache */
> -        mcr   CP32(r0, BPIALL)       /* Flush branch predictor */
> -        dsb                          /* Ensure completion of TLB+BP flush */
> -        isb
> -
> -1:
>          ldr   r0, =init_data
>          add   r0, #INITINFO_stack    /* Find the boot-time stack */
>          ldr   sp, [r0]
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 30/35] xen/arm32: head: Don't setup the fixmap on secondary CPUs

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> setup_fixmap() will setup the fixmap in the boot page tables in order to
> use earlyprintk and also update the register r11 holding the address to
> the UART.
> 
> However, secondary CPUs are not using earlyprintk between turning the
> MMU on and switching to the runtime page table. So setting up the
> fixmap in the boot pages table is pointless.
> 
> This means most of setup_fixmap() is not necessary for the secondary
> CPUs. The update of UART address is now moved out of setup_fixmap() and
> duplicated in the CPU boot and secondary CPUs boot. Additionally, the
> call to setup_fixmap() is removed from secondary CPUs boot.
> 
> Lastly, take the opportunity to replace load from literal pool with the
> new macro mov_w.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 6dc6032498..0a5c3a8525 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -159,6 +159,10 @@ past_zImage:
>          mov   pc, r0
>  primary_switched:
>          bl    setup_fixmap
> +#ifdef CONFIG_EARLY_PRINTK
> +        /* Use a virtual address to access the UART. */
> +        mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> +#endif
>          b     launch
>  ENDPROC(start)
>  
> @@ -200,8 +204,6 @@ GLOBAL(init_secondary)
>          ldr   r0, =secondary_switched
>          mov   pc, r0
>  secondary_switched:
> -        bl    setup_fixmap
> -
>          /*
>           * Non-boot CPUs need to move on to the proper pagetables, which were
>           * setup in init_secondary_pagetables.
> @@ -221,6 +223,10 @@ secondary_switched:
>          dsb                          /* Ensure completion of TLB+BP flush */
>          isb
>  
> +#ifdef CONFIG_EARLY_PRINTK
> +        /* Use a virtual address to access the UART. */
> +        mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> +#endif
>          b     launch
>  ENDPROC(init_secondary)
>  
> @@ -475,13 +481,6 @@ setup_fixmap:
>           */
>          dsb
>  #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
> -        /*
> -         * Non-boot CPUs don't need to rebuild the fixmap itself, just
> -         * the mapping from boot_second to xen_fixmap
> -         */
> -        teq   r12, #0
> -        bne   1f
> -
>          /* Add UART to the fixmap table */
>          ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
>          lsr   r2, r11, #THIRD_SHIFT
> @@ -502,9 +501,6 @@ setup_fixmap:
>          mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
>          mov   r3, #0x0
>          strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
> -
> -        /* Use a virtual address to access the UART. */
> -        ldr   r11, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
>  
>          /*
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 29/35] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path

[Julien Grall]

Hi Stefano,

On 7/30/19 9:25 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> The assembly switch to the runtime PT is only necessary for the
>> secondary CPUs. So move the code in the secondary CPUs path.
>>
>> While this is definitely not compliant with the Arm Arm as we are
>> switching between two differents set of page-tables without turning off
>> the MMU. Turning off the MMU is impossible here as the ID map may clash
>> with other mappings in the runtime page-tables. This will require more
>> rework to avoid the problem. So for now add a TODO in the code.
>>
>> Finally, the code is currently assume that r5 will be properly set to 0
>> before hand. This is done by create_page_tables() which is called quite
>> early in the boot process. There are a risk this may be oversight in the
>> future and therefore breaking secondary CPUs boot. Instead, set r5 to 0
>> just before using it.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm32/head.S | 42 ++++++++++++++++++++----------------------
>>   1 file changed, 20 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 4081a52dfa..6dc6032498 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -201,6 +201,26 @@ GLOBAL(init_secondary)
>>           mov   pc, r0
>>   secondary_switched:
>>           bl    setup_fixmap
>> +
>> +        /*
>> +         * Non-boot CPUs need to move on to the proper pagetables, which were
>> +         * setup in init_secondary_pagetables.
>> +         *
>> +         * XXX: This is not compliant with the Arm Arm.
>> +         */
>> +        ldr   r4, =init_ttbr         /* VA of HTTBR value stashed by CPU 0 */
>> +        mov   r5, #0
> 
> Why do we need to zero r5? Shouldn't ldrd overwrite r5 anyway?

I was on auto-pilot mode and saw the trailing "d" and thought it was a 
"strd". I will remove it in the next version.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 19/35] xen/arm32: head: Add a macro to move an immediate constant into a 32-bit register

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> The current boot code is using the pattern ldr rX, =... to move an
> immediate constant into a 32-bit register.
> 
> This pattern implies to load the immediate constant from a literal pool,
> meaning a memory access will be performed.
> 
> The memory access can be avoided by using movw/movt instructions.
> 
> A new macro is introduced to move an immediate constant into a 32-bit
> register without a memory load. Follow-up patches will make use of it.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

That's interesting!

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


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 18ded49a04..99f4af18d8 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -37,6 +37,15 @@
>  #endif
>  
>  /*
> + * Move an immediate constant into a 32-bit register using movw/movt
> + * instructions.
> + */
> +.macro mov_w reg, word
> +        movw  \reg, #:lower16:\word
> +        movt  \reg, #:upper16:\word
> +.endm
> +
> +/*
>   * Common register usage in this file:
>   *   r0  -
>   *   r1  -
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 32/35] xen/arm32: head: Rework and document setup_fixmap()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> At the moment, the fixmap table is only hooked when earlyprintk is used.
> This is fine today because in C land, the fixmap is not used by anyone
> until the the boot CPU is switching to the runtime page-tables.
> 
> In the future, the boot CPU will not switch between page-tables to
> avoid TLB incoherency. Thus, the fixmap table will need to be always
> hooked beofre any use. Let's start doing it now in setup_fixmap().
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 56e2d09ed4..e0f8c2e0cb 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -536,8 +536,21 @@ identity_mapping_removed:
>          mov   pc, lr
>  ENDPROC(remove_identity_mapping)
>  
> +/*
> + * Map the UART in the fixmap (when earlyprintk is used) and hook the
> + * fixmap table in the page tables.
> + *
> + * The fixmap cannot be mapped in create_page_tables because it may
> + * clash with the 1:1 mapping.
> + *
> + * Inputs:
> + *   r10: Physical offset
> + *   r11: Early UART base physical address
> + *
> + * Clobbers r1 - r4
> + */
>  setup_fixmap:
> -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
> +#if defined(CONFIG_EARLY_PRINTK)
>          /* Add UART to the fixmap table */
>          ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
>          lsr   r2, r11, #THIRD_SHIFT
> @@ -546,7 +559,7 @@ setup_fixmap:
>          orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
>          mov   r3, #0x0
>          strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
> -1:
> +#endif

Patch is OK. However, the 1: should be removed in the previous patch
"xen/arm32: head: Don't setup the fixmap on secondary CPUs", where we
took away the beq.

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


>          /* Map fixmap into boot_second */
>          ldr   r1, =boot_second       /* r1 := vaddr (boot_second) */
> @@ -561,7 +574,7 @@ setup_fixmap:
>  
>          /* Ensure any page table updates made above have occurred. */
>          dsb   nshst
> -#endif
> +
>          mov   pc, lr
>  ENDPROC(setup_fixmap)
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 33/35] xen/arm32: head: Rework and document launch()

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> Boot CPU and secondary CPUs will use different entry point to C code. At
> the moment, the decision on which entry to use is taken within launch().
> 
> In order to avoid using conditional instruction and make the call
> clearer, launch() is reworked to take in parameters the entry point and its
> arguments.
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index e0f8c2e0cb..6d55a2119a 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -170,6 +170,11 @@ primary_switched:
>          /* Use a virtual address to access the UART. */
>          mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> +        PRINT("- Ready -\r\n")
> +        /* Setup the arguments for start_xen and jump to C world */
> +        mov   r0, r10                /* r0 := Physical offset */
> +        mov   r1, r8                 /* r1 := paddr(FDT) */
> +        ldr   r2, =start_xen
>          b     launch
>  ENDPROC(start)
>  
> @@ -234,6 +239,9 @@ secondary_switched:
>          /* Use a virtual address to access the UART. */
>          mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> +        PRINT("- Ready -\r\n")
> +        /* Jump to C world */
> +        ldr   r2, =start_secondary
>          b     launch
>  ENDPROC(init_secondary)
>  
> @@ -578,19 +586,25 @@ setup_fixmap:
>          mov   pc, lr
>  ENDPROC(setup_fixmap)
>  
> +/*
> + * Setup the initial stack and jump to the C world
> + *
> + * Inputs:
> + *   r0 : Argument 0 of the C function to call
> + *   r1 : Argument 1 of the C function to call
> + *   r2 : C entry point
> + *
> + * Clobbers r3
> + */
>  launch:
> -        PRINT("- Ready -\r\n")
> -
> -        ldr   r0, =init_data
> -        add   r0, #INITINFO_stack    /* Find the boot-time stack */
> -        ldr   sp, [r0]
> +        ldr   r3, =init_data
> +        add   r3, #INITINFO_stack    /* Find the boot-time stack */
> +        ldr   sp, [r3]
>          add   sp, #STACK_SIZE        /* (which grows down from the top). */
>          sub   sp, #CPUINFO_sizeof    /* Make room for CPU save record */
> -        teq   r12, #0
> -        moveq r0, r10                /* Marshal args: - phys_offset */
> -        moveq r1, r8                 /*               - DTB address */
> -        beq   start_xen              /* and disappear into the land of C */
> -        b     start_secondary        /* (to the appropriate entry point) */
> +
> +        /* Jump to C world */
> +       bx    r2

Why bx?


>  ENDPROC(launch)
>  
>  /* Fail-stop */
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 34/35] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> At the moment, HTTBR is setup in create_page_tables(). This is fine as
> it is called by every CPUs.
> 
> However, such assumption may not hold in the future. To make change
> easier, the HTTBR is not setup in enable_mmu().
> 
> Take the opportunity to add the missing isb() to ensure the HTTBR is
> seen before the MMU is turned on.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 6d55a2119a..8a1e272aab 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -373,8 +373,6 @@ create_page_tables:
>          /* Write Xen's PT's paddr into the HTTBR */

This comment needs to be moved


>          ldr   r4, =boot_pgtable
>          add   r4, r4, r10            /* r4 := paddr (boot_pagetable) */
> -        mov   r5, #0                 /* r4:r5 is paddr (boot_pagetable) */
> -        mcrr  CP64(r4, r5, HTTBR)

Interestingly r5 is not clobbered by create_page_tables anymore, we need
to update the comment on top.


>          /* Setup boot_pgtable: */
>          ldr   r1, =boot_second
> @@ -480,6 +478,12 @@ enable_mmu:
>          mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLBs */
>          dsb   nsh
>  
> +        ldr   r0, =boot_pgtable
> +        add   r0, r0, r10            /* r0 := paddr (boot_pagetable) */
> +        mov   r1, #0                 /* r0:r1 is paddr (boot_pagetable) */
> +        mcrr  CP64(r0, r1, HTTBR)
> +        isb
> +
>          mrc   CP32(r0, HSCTLR)
>          /* Enable MMU and D-cache */
>          orr   r0, r0, #(SCTLR_Axx_ELx_M|SCTLR_Axx_ELx_C)
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 35/35] xen/arm: Zero BSS after the MMU and D-cache is turned on

[Stefano Stabellini]

On Mon, 22 Jul 2019, Julien Grall wrote:
> At the moment BSS is zeroed before the MMU and D-Cache is turned on.
> In other words, the cache will be bypassed when zeroing the BSS section.
> 
> On Arm64, per the Image protocol [1], the state of the cache for BSS region
> is not known because it is not part of the "loaded kernel image".
> 
> On Arm32, the boot protocol [2] does not mention anything about the
> state of the cache. Therefore, it should be assumed that it is not known
> for BSS region.
> 
> This means that the cache will need to be invalidated twice for the BSS
> region:
>     1) Before zeroing to remove any dirty cache line. Otherwise they may
>     get evicted while zeroing and therefore overriding the value.
>     2) After zeroing to remove any cache line that may have been
>     speculated. Otherwise when turning on MMU and D-Cache, the CPU may
>     see old values.
> 
> At the moment, the only reason to have BSS zeroed early is because the
> boot page tables are part of it. To avoid the two cache invalidations,
> it would be better if the boot page tables are part of the "loaded
> kernel image" and therefore be zeroed when loading the image into
> memory. A good candidate is the section .data.page_aligned.
> 
> A new macro DEFINE_BOOT_PAGE_TABLE is introduced to create and mark
> page-tables used before BSS is zeroed. This includes all boot_* but also
> xen_fixmap as zero_bss() will print a message when earlyprintk is
> enabled.
> 
> [1] linux/Documentation/arm64/booting.txt
> [2] linux/Documentation/arm/Booting
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
> 
>     Changes in v2:
>         - Add missing signed-off
>         - Clarify commit message
>         - Add arm32 parts
> ---
>  xen/arch/arm/arm32/head.S | 11 +++--------
>  xen/arch/arm/arm64/head.S |  7 +++----
>  xen/arch/arm/mm.c         | 23 +++++++++++++++++------
>  3 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 8a1e272aab..48cad6103f 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -149,7 +149,6 @@ past_zImage:
>          mov   r12, #0                /* r12 := is_secondary_cpu */
>  
>          bl    check_cpu_mode
> -        bl    zero_bss
>          bl    cpu_init
>          bl    create_page_tables
>          bl    enable_mmu
> @@ -170,6 +169,7 @@ primary_switched:
>          /* Use a virtual address to access the UART. */
>          mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> +        bl    zero_bss
>          PRINT("- Ready -\r\n")
>          /* Setup the arguments for start_xen and jump to C world */
>          mov   r0, r10                /* r0 := Physical offset */
> @@ -280,17 +280,12 @@ ENDPROC(check_cpu_mode)
>  /*
>   * Zero BSS
>   *
> - * Inputs:
> - *   r10: Physical offset
> - *
>   * Clobbers r0 - r3
>   */
>  zero_bss:
>          PRINT("- Zero BSS -\r\n")
> -        ldr   r0, =__bss_start       /* Load start & end of bss */
> -        ldr   r1, =__bss_end
> -        add   r0, r0, r10            /* Apply physical offset */
> -        add   r1, r1, r10
> +        ldr   r0, =__bss_start       /* r0 := vaddr(__bss_start) */
> +        ldr   r1, =__bss_end         /* r1 := vaddr(__bss_start) */
>  
>          mov   r2, #0
>  1:      str   r2, [r0], #4
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 2287f3ce48..b671e0e59f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -303,7 +303,6 @@ real_start_efi:
>          mov   x22, #0                /* x22 := is_secondary_cpu */
>  
>          bl    check_cpu_mode
> -        bl    zero_bss
>          bl    cpu_init
>          bl    create_page_tables
>          bl    enable_mmu
> @@ -324,6 +323,7 @@ primary_switched:
>          /* Use a virtual address to access the UART. */
>          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> +        bl    zero_bss
>          PRINT("- Ready -\r\n")
>          /* Setup the arguments for start_xen and jump to C world */
>          mov   x0, x20                /* x0 := Physical offset */
> @@ -426,7 +426,6 @@ ENDPROC(check_cpu_mode)
>   * Zero BSS
>   *
>   * Inputs:
> - *   x20: Physical offset
>   *   x26: Do we need to zero BSS?
>   *
>   * Clobbers x0 - x3
> @@ -436,8 +435,8 @@ zero_bss:
>          cbnz  x26, skip_bss
>  
>          PRINT("- Zero BSS -\r\n")
> -        load_paddr x0, __bss_start    /* Load paddr of start & end of bss */
> -        load_paddr x1, __bss_end
> +        ldr   x0, =__bss_start       /* x0 := vaddr(__bss_start) */
> +        ldr   x1, =__bss_end         /* x1 := vaddr(__bss_start) */
>  
>  1:      str   xzr, [x0], #8
>          cmp   x0, x1
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 44258ad89c..670a3089ea 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -62,6 +62,17 @@ mm_printk(const char *fmt, ...) {}
>      } while (0);
>  #endif
>  
> +/*
> + * Macros to define page-tables:
> + *  - DEFINE_BOOT_PAGE_TABLE is used to define page-table that are used
> + *  in assembly code before BSS is zeroed.
> + *  - DEFINE_PAGE_TABLE{,S} are used to define one or multiple
> + *  page-tables to be used after BSS is zeroed (typically they are only used
> + *  in C).
> + */
> +#define DEFINE_BOOT_PAGE_TABLE(name)                                          \
> +lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned") name[LPAE_ENTRIES]
> +
>  #define DEFINE_PAGE_TABLES(name, nr)                    \
>  lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
>  
> @@ -90,13 +101,13 @@ lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
>   * Finally, if EARLY_PRINTK is enabled then xen_fixmap will be mapped
>   * by the CPU once it has moved off the 1:1 mapping.
>   */
> -DEFINE_PAGE_TABLE(boot_pgtable);
> +DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
>  #ifdef CONFIG_ARM_64
> -DEFINE_PAGE_TABLE(boot_first);
> -DEFINE_PAGE_TABLE(boot_first_id);
> +DEFINE_BOOT_PAGE_TABLE(boot_first);
> +DEFINE_BOOT_PAGE_TABLE(boot_first_id);
>  #endif
> -DEFINE_PAGE_TABLE(boot_second);
> -DEFINE_PAGE_TABLE(boot_third);
> +DEFINE_BOOT_PAGE_TABLE(boot_second);
> +DEFINE_BOOT_PAGE_TABLE(boot_third);
>  
>  /* Main runtime page tables */
>  
> @@ -149,7 +160,7 @@ static __initdata int xenheap_first_first_slot = -1;
>   */
>  static DEFINE_PAGE_TABLES(xen_second, 2);
>  /* First level page table used for fixmap */
> -DEFINE_PAGE_TABLE(xen_fixmap);
> +DEFINE_BOOT_PAGE_TABLE(xen_fixmap);
>  /* First level page table used to map Xen itself with the XN bit set
>   * as appropriate. */
>  static DEFINE_PAGE_TABLE(xen_xenmap);
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH v2 33/35] xen/arm32: head: Rework and document launch()

[Julien Grall]

On 30/07/2019 22:21, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> Boot CPU and secondary CPUs will use different entry point to C code. At
>> the moment, the decision on which entry to use is taken within launch().
>>
>> In order to avoid using conditional instruction and make the call
>> clearer, launch() is reworked to take in parameters the entry point and its
>> arguments.
>>
>> Lastly, document the behavior and the main registers usage within the
>> function.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm32/head.S | 34 ++++++++++++++++++++++++----------
>>   1 file changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index e0f8c2e0cb..6d55a2119a 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -170,6 +170,11 @@ primary_switched:
>>           /* Use a virtual address to access the UART. */
>>           mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
>>   #endif
>> +        PRINT("- Ready -\r\n")
>> +        /* Setup the arguments for start_xen and jump to C world */
>> +        mov   r0, r10                /* r0 := Physical offset */
>> +        mov   r1, r8                 /* r1 := paddr(FDT) */
>> +        ldr   r2, =start_xen
>>           b     launch
>>   ENDPROC(start)
>>   
>> @@ -234,6 +239,9 @@ secondary_switched:
>>           /* Use a virtual address to access the UART. */
>>           mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
>>   #endif
>> +        PRINT("- Ready -\r\n")
>> +        /* Jump to C world */
>> +        ldr   r2, =start_secondary
>>           b     launch
>>   ENDPROC(init_secondary)
>>   
>> @@ -578,19 +586,25 @@ setup_fixmap:
>>           mov   pc, lr
>>   ENDPROC(setup_fixmap)
>>   
>> +/*
>> + * Setup the initial stack and jump to the C world
>> + *
>> + * Inputs:
>> + *   r0 : Argument 0 of the C function to call
>> + *   r1 : Argument 1 of the C function to call
>> + *   r2 : C entry point
>> + *
>> + * Clobbers r3
>> + */
>>   launch:
>> -        PRINT("- Ready -\r\n")
>> -
>> -        ldr   r0, =init_data
>> -        add   r0, #INITINFO_stack    /* Find the boot-time stack */
>> -        ldr   sp, [r0]
>> +        ldr   r3, =init_data
>> +        add   r3, #INITINFO_stack    /* Find the boot-time stack */
>> +        ldr   sp, [r3]
>>           add   sp, #STACK_SIZE        /* (which grows down from the top). */
>>           sub   sp, #CPUINFO_sizeof    /* Make room for CPU save record */
>> -        teq   r12, #0
>> -        moveq r0, r10                /* Marshal args: - phys_offset */
>> -        moveq r1, r8                 /*               - DTB address */
>> -        beq   start_xen              /* and disappear into the land of C */
>> -        b     start_secondary        /* (to the appropriate entry point) */
>> +
>> +        /* Jump to C world */
>> +       bx    r2
> 
> Why bx?
The only two other possible instructions would be:
    1) blx r2: we don't need to save the return address
    2) mov pc, r2: The Arm Arm recommends to use bx/blx instead of this.

So bx seems the best fit. Any other suggestion?

Also, I would probably replace all the "mov pc, lr" I added with "bx lr".

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 16/35] xen/arm64: head: Rework and document launch()

[Julien Grall]

Hi Stefano,

On 7/30/19 6:45 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> Boot CPU and secondary CPUs will use different entry point to C code. At
>> the moment, the decision on which entry to use is taken within launch().
>>
>> In order to avoid a branch for the decision and make the code clearer,
>> launch() is reworked to take in parameters the entry point and its
>> arguments.
>>
>> Lastly, document the behavior and the main registers usage within the
>> function.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Use x3 instead of x4
>>          - Add a clobbers section
>> ---
>>   xen/arch/arm/arm64/head.S | 43 +++++++++++++++++++++++++++----------------
>>   1 file changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index f165dd61ca..7541635102 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -312,6 +312,11 @@ primary_switched:
>>           /* Use a virtual address to access the UART. */
>>           ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>>   #endif
>> +        PRINT("- Ready -\r\n")
>> +        /* Setup the arguments for start_xen and jump to C world */
>> +        mov   x0, x20                /* x0 := Physical offset */
>> +        mov   x1, x21                /* x1 := paddr(FDT) */
>> +        ldr   x2, =start_xen
>>           b     launch
>>   ENDPROC(real_start)
>>   
>> @@ -374,6 +379,9 @@ secondary_switched:
>>           /* Use a virtual address to access the UART. */
>>           ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>>   #endif
>> +        PRINT("- Ready -\r\n")
>> +        /* Jump to C world */
>> +        ldr   x2, =start_secondary
>>           b     launch
>>   ENDPROC(init_secondary)
>>   
>> @@ -732,23 +740,26 @@ setup_fixmap:
>>           ret
>>   ENDPROC(setup_fixmap)
>>   
>> +/*
>> + * Setup the initial stack and jump to the C world
>> + *
>> + * Inputs:
>> + *   x0 : Argument 0 of the C function to call
>> + *   x1 : Argument 1 of the C function to call
>> + *   x2 : C entry point
>> + *
>> + * Clobbers x3
>> + */
>>   launch:
>> -        PRINT("- Ready -\r\n")
>> -
>> -        ldr   x0, =init_data
>> -        add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
>> -        ldr   x0, [x0]
>> -        add   x0, x0, #STACK_SIZE    /* (which grows down from the top). */
>> -        sub   x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */
>> -        mov   sp, x0
>> -
>> -        cbnz  x22, 1f
>> -
>> -        mov   x0, x20                /* Marshal args: - phys_offset */
>> -        mov   x1, x21                /*               - FDT */
>> -        b     start_xen              /* and disappear into the land of C */
>> -1:
>> -        b     start_secondary        /* (to the appropriate entry point) */
>> +        ldr   x3, =init_data
>> +        add   x3, x3, #INITINFO_stack /* Find the boot-time stack */
>> +        ldr   x3, [x3]
>> +        add   x3, x3, #STACK_SIZE    /* (which grows down from the top). */
>                                          ^ please move 1 space to the
>                                          right
> 
> Aside from this minor code style thing

If I wanted to be picky, all the rest of the code in this file is 
indentation at column 38. So this line has the correct indentation but 
the two others are not. However, this would means there are not space 
between the instruction and the comment:

foobar/* ... */

So I guess, indenting at column 39 would be the best here, although I 
already know someone that will be unhappy with in the future ;).

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

Thank you!

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 22/35] xen/arm32: head: Rework UART initialization on boot CPU

[Julien Grall]

Hi Stefano,

On 7/30/19 8:40 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> @@ -497,11 +497,15 @@ ENTRY(switch_ttbr)
>>   
>>   #ifdef CONFIG_EARLY_PRINTK
>>   /*
>> - * Bring up the UART.
>> - * r11: Early UART base address
>> - * Clobbers r0-r2
>> + * Initialize the UART. Should only be called on the boot CPU.
>> + *
>> + * Ouput:
>        ^ this should be output, and in the arm64 patch too (already committed)

Most of the arm32 code is a copy and paste from arm64 with little 
adaptation! Hence the same typo :).

I have fixed the arm32 part directly in this patch and create a new one 
for fixing the arm64 part.


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

Thank you!

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 33/35] xen/arm32: head: Rework and document launch()

[Stefano Stabellini]

On Tue, 30 Jul 2019, Julien Grall wrote:
> On 30/07/2019 22:21, Stefano Stabellini wrote:
> > On Mon, 22 Jul 2019, Julien Grall wrote:
> >> Boot CPU and secondary CPUs will use different entry point to C code. At
> >> the moment, the decision on which entry to use is taken within launch().
> >>
> >> In order to avoid using conditional instruction and make the call
> >> clearer, launch() is reworked to take in parameters the entry point and its
> >> arguments.
> >>
> >> Lastly, document the behavior and the main registers usage within the
> >> function.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@arm.com>
> >>
> >> ---
> >>      Changes in v2:
> >>          - Patch added
> >> ---
> >>   xen/arch/arm/arm32/head.S | 34 ++++++++++++++++++++++++----------
> >>   1 file changed, 24 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> >> index e0f8c2e0cb..6d55a2119a 100644
> >> --- a/xen/arch/arm/arm32/head.S
> >> +++ b/xen/arch/arm/arm32/head.S
> >> @@ -170,6 +170,11 @@ primary_switched:
> >>           /* Use a virtual address to access the UART. */
> >>           mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> >>   #endif
> >> +        PRINT("- Ready -\r\n")
> >> +        /* Setup the arguments for start_xen and jump to C world */
> >> +        mov   r0, r10                /* r0 := Physical offset */
> >> +        mov   r1, r8                 /* r1 := paddr(FDT) */
> >> +        ldr   r2, =start_xen
> >>           b     launch
> >>   ENDPROC(start)
> >>   
> >> @@ -234,6 +239,9 @@ secondary_switched:
> >>           /* Use a virtual address to access the UART. */
> >>           mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> >>   #endif
> >> +        PRINT("- Ready -\r\n")
> >> +        /* Jump to C world */
> >> +        ldr   r2, =start_secondary
> >>           b     launch
> >>   ENDPROC(init_secondary)
> >>   
> >> @@ -578,19 +586,25 @@ setup_fixmap:
> >>           mov   pc, lr
> >>   ENDPROC(setup_fixmap)
> >>   
> >> +/*
> >> + * Setup the initial stack and jump to the C world
> >> + *
> >> + * Inputs:
> >> + *   r0 : Argument 0 of the C function to call
> >> + *   r1 : Argument 1 of the C function to call
> >> + *   r2 : C entry point
> >> + *
> >> + * Clobbers r3
> >> + */
> >>   launch:
> >> -        PRINT("- Ready -\r\n")
> >> -
> >> -        ldr   r0, =init_data
> >> -        add   r0, #INITINFO_stack    /* Find the boot-time stack */
> >> -        ldr   sp, [r0]
> >> +        ldr   r3, =init_data
> >> +        add   r3, #INITINFO_stack    /* Find the boot-time stack */
> >> +        ldr   sp, [r3]
> >>           add   sp, #STACK_SIZE        /* (which grows down from the top). */
> >>           sub   sp, #CPUINFO_sizeof    /* Make room for CPU save record */
> >> -        teq   r12, #0
> >> -        moveq r0, r10                /* Marshal args: - phys_offset */
> >> -        moveq r1, r8                 /*               - DTB address */
> >> -        beq   start_xen              /* and disappear into the land of C */
> >> -        b     start_secondary        /* (to the appropriate entry point) */
> >> +
> >> +        /* Jump to C world */
> >> +       bx    r2
> > 
> > Why bx?
> The only two other possible instructions would be:
>     1) blx r2: we don't need to save the return address
>     2) mov pc, r2: The Arm Arm recommends to use bx/blx instead of this.
> 
> So bx seems the best fit. Any other suggestion?
> 
> Also, I would probably replace all the "mov pc, lr" I added with "bx lr".

There is really no alternative to bx. I forgot that "b" doesn't support
a register as an operand.

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

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

Re: [Xen-devel] [PATCH v2 24/35] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs

[Julien Grall]

Hi Stefano,

On 7/30/19 9:07 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> The boot code is currently quite difficult to go through because of the
>> lack of documentation and a number of indirection to avoid executing
>> some path in either the boot CPU or secondary CPUs.
>>
>> In an attempt to make the boot code easier to follow, each parts of the
>> boot are now in separate functions. Furthermore, the paths for the boot
>> CPU and secondary CPUs are now distinct and for now will call each
>> functions.
>>
>> Follow-ups will remove unnecessary calls and do further improvement
>> (such as adding documentation and reshuffling).
>>
>> Note that the switch from using the ID mapping to the runtime mapping
>> is duplicated for each path. This is because in the future we will need
>> to stay longer in the ID mapping for the boot CPU.
>>
>> Lastly, it is now required to save lr in cpu_init() becauswe the
>> function will call other functions and therefore clobber lr.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm32/head.S | 64 +++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 53 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index bbcfdcd351..13793e85d8 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -148,7 +148,19 @@ past_zImage:
>>   
>>           mov   r12, #0                /* r12 := is_secondary_cpu */
>>   
>> -        b     common_start
>> +        bl    check_cpu_mode
>> +        bl    zero_bss
>> +        bl    cpu_init
>> +        bl    create_page_tables
>> +        bl    enable_mmu
>> +
>> +        /* We are still in the ID map. Jump to the runtime Virtual Address */
> 
> The arm64 patch has been switched to use "1:1", it would be good to be
> consistent. In the commit message too.
> 
> 
>> +        ldr   r0, =primary_switched
>> +        mov   pc, r0
>> +primary_switched:
>> +        bl    setup_fixmap
>> +        b     launch
>> +ENDPROC(start)
>>   
>>   GLOBAL(init_secondary)
>>           cpsid aif                    /* Disable all interrupts */
>> @@ -179,8 +191,21 @@ GLOBAL(init_secondary)
>>           print_reg r7
>>           PRINT(" booting -\r\n")
>>   #endif
>> -
>> -common_start:
>> +        bl    check_cpu_mode
>> +        bl    zero_bss
>> +        bl    cpu_init
>> +        bl    create_page_tables
>> +        bl    enable_mmu
>> +
>> +        /* We are still in the ID map. Jump to the runtime Virtual Address. */
> 
> Same here.
> 
> 
>> +        ldr   r0, =secondary_switched
>> +        mov   pc, r0
>> +secondary_switched:
>> +        bl    setup_fixmap
>> +        b     launch
>> +ENDPROC(init_secondary)
>> +
>> +check_cpu_mode:
>>           /* Check that this CPU has Hyp mode */
>>           mrc   CP32(r0, ID_PFR1)
>>           and   r0, r0, #0xf000        /* Bits 12-15 define virt extensions */
>> @@ -202,7 +227,10 @@ common_start:
>>           b     fail
>>   
>>   hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
>> +        mov   pc, lr
>> +ENDPROC(check_cpu_mode)
>>   
>> +zero_bss:
>>           /* Zero BSS On the boot CPU to avoid nasty surprises */
>>           teq   r12, #0
>>           bne   skip_bss
>> @@ -219,8 +247,14 @@ hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
>>           blo   1b
>>   
>>   skip_bss:
>> +        mov   pc, lr
>> +ENDPROC(zero_bss)
>> +
>> +cpu_init:
>>           PRINT("- Setting up control registers -\r\n")
>>   
>> +        mov   r5, lr			            /* r5 := return address */
> 
> Please align the comment with the others in this proc.

It looks like the line was containing some hard tab. I have replaced 
them with soft tab.

> 
> Other than these minor comments the patch looks fine. Have you had a
> chance to test it on real hardware?

I actually didn't test the SMP path on the model until I sent it because 
I had some issues getting secondary CPU up. I wanted to get some 
feedback first. Hence the "lightly tested" in the cover letter :).

This series fully boot on the model and I still need to test on real 
hardware. I have a TC2 (Cortex-A15 + Cortex-A7) on my desk, so I will 
give it a try there first.

Anyway, I probably need to resend this patch to replace all the "mov pc, 
lr" with "bx lr"

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 32/35] xen/arm32: head: Rework and document setup_fixmap()

[Julien Grall]

Hi Stefano,

On 7/30/19 10:14 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> At the moment, the fixmap table is only hooked when earlyprintk is used.
>> This is fine today because in C land, the fixmap is not used by anyone
>> until the the boot CPU is switching to the runtime page-tables.
>>
>> In the future, the boot CPU will not switch between page-tables to
>> avoid TLB incoherency. Thus, the fixmap table will need to be always
>> hooked beofre any use. Let's start doing it now in setup_fixmap().
>>
>> Lastly, document the behavior and the main registers usage within the
>> function.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm32/head.S | 19 ++++++++++++++++---
>>   1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 56e2d09ed4..e0f8c2e0cb 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -536,8 +536,21 @@ identity_mapping_removed:
>>           mov   pc, lr
>>   ENDPROC(remove_identity_mapping)
>>   
>> +/*
>> + * Map the UART in the fixmap (when earlyprintk is used) and hook the
>> + * fixmap table in the page tables.
>> + *
>> + * The fixmap cannot be mapped in create_page_tables because it may
>> + * clash with the 1:1 mapping.
>> + *
>> + * Inputs:
>> + *   r10: Physical offset
>> + *   r11: Early UART base physical address
>> + *
>> + * Clobbers r1 - r4
>> + */
>>   setup_fixmap:
>> -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
>> +#if defined(CONFIG_EARLY_PRINTK)
>>           /* Add UART to the fixmap table */
>>           ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
>>           lsr   r2, r11, #THIRD_SHIFT
>> @@ -546,7 +559,7 @@ setup_fixmap:
>>           orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
>>           mov   r3, #0x0
>>           strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
>> -1:
>> +#endif
> 
> Patch is OK. However, the 1: should be removed in the previous patch
> "xen/arm32: head: Don't setup the fixmap on secondary CPUs", where we
> took away the beq.

Good point. I have now moved this to the previous patch.

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

Thank you!

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 34/35] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb

[Julien Grall]

Hi Stefano,

On 7/30/19 10:26 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> At the moment, HTTBR is setup in create_page_tables(). This is fine as
>> it is called by every CPUs.
>>
>> However, such assumption may not hold in the future. To make change
>> easier, the HTTBR is not setup in enable_mmu().
>>
>> Take the opportunity to add the missing isb() to ensure the HTTBR is
>> seen before the MMU is turned on.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm32/head.S | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 6d55a2119a..8a1e272aab 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -373,8 +373,6 @@ create_page_tables:
>>           /* Write Xen's PT's paddr into the HTTBR */
> 
> This comment needs to be moved

Good spot!

> 
> 
>>           ldr   r4, =boot_pgtable
>>           add   r4, r4, r10            /* r4 := paddr (boot_pagetable) */
>> -        mov   r5, #0                 /* r4:r5 is paddr (boot_pagetable) */
>> -        mcrr  CP64(r4, r5, HTTBR)
> 
> Interestingly r5 is not clobbered by create_page_tables anymore, we need
> to update the comment on top.

I knew documenting the clobbered registers are going to cause some 
trouble when updating the code :). I will fix it in the next version.

Cheers,

-- 
Julien Grall

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