xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm
@ 2019-08-12 17:29 Julien Grall
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 01/28] xen/arm: lpae: Allow more LPAE helpers to be used in assembly Julien Grall
                   ` (27 more replies)
  0 siblings, 28 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:29 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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.

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


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

Cheers,

Julien Grall (28):
  xen/arm: lpae: Allow more LPAE helpers to be used in assembly
  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/arm64: head: Fix typo in the documentation on top of init_uart()
  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/arm64: head: Introduce macros to create table and mapping entry
  xen/arm64: head: Use a page mapping for the 1:1 mapping in
    create_page_tables()
  xen/arm32: head: Introduce macros to create table and mapping entry
  xen/arm32: head: Use a page mapping for the 1:1 mapping in
    create_page_tables()

 xen/arch/arm/arm32/head.S  | 601 ++++++++++++++++++++++++++++++---------------
 xen/arch/arm/arm64/head.S  | 438 +++++++++++++++++++++------------
 xen/arch/arm/mm.c          |  25 +-
 xen/include/asm-arm/lpae.h |  10 +-
 4 files changed, 716 insertions(+), 358 deletions(-)

-- 
2.11.0


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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 01/28] xen/arm: lpae: Allow more LPAE helpers to be used in assembly
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
@ 2019-08-12 17:29 ` Julien Grall
  2019-08-22 17:07   ` Stefano Stabellini
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used Julien Grall
                   ` (26 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:29 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

A follow-up patch will require to use *_table_offset() and *_MASK helpers
from assembly. This can be achieved by using _AT() macro to remove the type
when called from assembly.

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

---
    Changes in v3:
        - Patch added
---
 xen/include/asm-arm/lpae.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index c22780f8f3..4797f9cee4 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -245,19 +245,19 @@ TABLE_OFFSET_HELPERS(64);
 
 #define THIRD_SHIFT    (PAGE_SHIFT)
 #define THIRD_ORDER    (THIRD_SHIFT - PAGE_SHIFT)
-#define THIRD_SIZE     ((paddr_t)1 << THIRD_SHIFT)
+#define THIRD_SIZE     (_AT(paddr_t, 1) << THIRD_SHIFT)
 #define THIRD_MASK     (~(THIRD_SIZE - 1))
 #define SECOND_SHIFT   (THIRD_SHIFT + LPAE_SHIFT)
 #define SECOND_ORDER   (SECOND_SHIFT - PAGE_SHIFT)
-#define SECOND_SIZE    ((paddr_t)1 << SECOND_SHIFT)
+#define SECOND_SIZE    (_AT(paddr_t, 1) << SECOND_SHIFT)
 #define SECOND_MASK    (~(SECOND_SIZE - 1))
 #define FIRST_SHIFT    (SECOND_SHIFT + LPAE_SHIFT)
 #define FIRST_ORDER    (FIRST_SHIFT - PAGE_SHIFT)
-#define FIRST_SIZE     ((paddr_t)1 << FIRST_SHIFT)
+#define FIRST_SIZE     (_AT(paddr_t, 1) << FIRST_SHIFT)
 #define FIRST_MASK     (~(FIRST_SIZE - 1))
 #define ZEROETH_SHIFT  (FIRST_SHIFT + LPAE_SHIFT)
 #define ZEROETH_ORDER  (ZEROETH_SHIFT - PAGE_SHIFT)
-#define ZEROETH_SIZE   ((paddr_t)1 << ZEROETH_SHIFT)
+#define ZEROETH_SIZE   (_AT(paddr_t, 1) << ZEROETH_SHIFT)
 #define ZEROETH_MASK   (~(ZEROETH_SIZE - 1))
 
 /* Calculate the offsets into the pagetables for a given VA */
@@ -266,7 +266,7 @@ TABLE_OFFSET_HELPERS(64);
 #define second_linear_offset(va) ((va) >> SECOND_SHIFT)
 #define third_linear_offset(va) ((va) >> THIRD_SHIFT)
 
-#define TABLE_OFFSET(offs) ((unsigned int)(offs) & LPAE_ENTRY_MASK)
+#define TABLE_OFFSET(offs) (_AT(unsigned int, offs) & LPAE_ENTRY_MASK)
 #define first_table_offset(va)  TABLE_OFFSET(first_linear_offset(va))
 #define second_table_offset(va) TABLE_OFFSET(second_linear_offset(va))
 #define third_table_offset(va)  TABLE_OFFSET(third_linear_offset(va))
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 01/28] xen/arm: lpae: Allow more LPAE helpers to be used in assembly Julien Grall
@ 2019-08-12 17:29 ` Julien Grall
  2019-08-22 17:58   ` Stefano Stabellini
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 03/28] xen/arm64: head: Rework and document setup_fixmap() Julien Grall
                   ` (25 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:29 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Avoid hardcoding slots

    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 | 94 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 79 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 50cff08756..ec138aae3e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -33,6 +33,11 @@
 #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
 #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
 
+/* Convenience defines to get slot used by Xen mapping. */
+#define XEN_ZEROETH_SLOT    zeroeth_table_offset(XEN_VIRT_START)
+#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
+#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
+
 #define __HEAD_FLAG_PAGE_SIZE   ((PAGE_SHIFT - 10) / 2)
 
 #define __HEAD_FLAG_PHYS_BASE   1
@@ -301,6 +306,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. */
@@ -626,10 +638,71 @@ 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 XEN_ZEROETH_SLOT. For slot
+         * XEN_ZEROETH_SLOT, the 1:1 mapping was either done in first
+         * or second table.
+         */
+        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
+        cmp   x1, #XEN_ZEROETH_SLOT
+        beq   1f
+        /* It is not in slot XEN_ZEROETH_SLOT, 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 XEN_FIRST_SLOT. For slot XEN_FIRST_SLOT,
+         * the 1:1 mapping was done in the second table.
+         */
+        lsr   x1, x19, #FIRST_SHIFT
+        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
+        cmp   x1, #XEN_FIRST_SLOT
+        beq   1f
+        /* It is not in slot XEN_FIRST_SLOT, 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 XEN_SECOND_SLOT. For slot XEN_SECOND_SLOT,
+         * 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, #XEN_SECOND_SLOT
+        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) */
@@ -647,19 +720,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 03/28] xen/arm64: head: Rework and document setup_fixmap()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 01/28] xen/arm: lpae: Allow more LPAE helpers to be used in assembly Julien Grall
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used Julien Grall
@ 2019-08-12 17:29 ` Julien Grall
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 04/28] xen/arm64: head: Rework and document launch() Julien Grall
                   ` (24 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:29 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 before 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>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v3:
        - Fix typo in the commit message
        - Add Stefano's Acked-by

    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 ec138aae3e..7b6b820726 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -702,8 +702,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
@@ -711,6 +724,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) */
@@ -723,7 +737,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 04/28] xen/arm64: head: Rework and document launch()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (2 preceding siblings ...)
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 03/28] xen/arm64: head: Rework and document setup_fixmap() Julien Grall
@ 2019-08-12 17:29 ` Julien Grall
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 05/28] xen/arm64: head: Setup TTBR_EL2 in enable_mmu() and add missing isb Julien Grall
                   ` (23 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:29 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v3:
        - Indent all the comments the same way
        - Add Stefano's reviewed-by

    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 7b6b820726..925fb93e58 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -318,6 +318,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)
 
@@ -380,6 +385,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)
 
@@ -741,23 +749,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 05/28] xen/arm64: head: Setup TTBR_EL2 in enable_mmu() and add missing isb
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (3 preceding siblings ...)
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 04/28] xen/arm64: head: Rework and document launch() Julien Grall
@ 2019-08-12 17:29 ` Julien Grall
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 06/28] xen/arm64: head: Introduce a macro to get a PC-relative address of a symbol Julien Grall
                   ` (22 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:29 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v3:
        - Add Stefano's acked-by

    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 925fb93e58..9b703e79ce 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -507,9 +507,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
@@ -637,6 +635,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 06/28] xen/arm64: head: Introduce a macro to get a PC-relative address of a symbol
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (4 preceding siblings ...)
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 05/28] xen/arm64: head: Setup TTBR_EL2 in enable_mmu() and add missing isb Julien Grall
@ 2019-08-12 17:29 ` Julien Grall
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 07/28] xen/arm64: head: Fix typo in the documentation on top of init_uart() Julien Grall
                   ` (21 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:29 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v3:
        - Fix typo in my e-mail address
        - Add Stefano's acked-by

    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 9b703e79ce..cd03101196 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -117,6 +117,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
@@ -895,11 +907,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 07/28] xen/arm64: head: Fix typo in the documentation on top of init_uart()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (5 preceding siblings ...)
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 06/28] xen/arm64: head: Introduce a macro to get a PC-relative address of a symbol Julien Grall
@ 2019-08-12 17:29 ` Julien Grall
  2019-08-22 17:08   ` Stefano Stabellini
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 08/28] xen/arm32: head: Add a macro to move an immediate constant into a 32-bit register Julien Grall
                   ` (20 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:29 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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

---
    Changes in v3:
        - Patch added
---
 xen/arch/arm/arm64/head.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index cd03101196..a6f3aa4ee5 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -825,7 +825,7 @@ ENTRY(switch_ttbr)
 /*
  * Initialize the UART. Should only be called on the boot CPU.
  *
- * Ouput:
+ * Output:
  *  x23: Early UART base physical address
  *
  * Clobbers x0 - x1
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 08/28] xen/arm32: head: Add a macro to move an immediate constant into a 32-bit register
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (6 preceding siblings ...)
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 07/28] xen/arm64: head: Fix typo in the documentation on top of init_uart() Julien Grall
@ 2019-08-12 17:29 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 09/28] xen/arm32: head: Mark the end of subroutines with ENDPROC Julien Grall
                   ` (19 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:29 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v3:
        - Add Stefano's acked-by

    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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 09/28] xen/arm32: head: Mark the end of subroutines with ENDPROC
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (7 preceding siblings ...)
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 08/28] xen/arm32: head: Add a macro to move an immediate constant into a 32-bit register Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:33   ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 10/28] xen/arm32: head: Don't clobber r14/lr in the macro PRINT Julien Grall
                   ` (18 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel
  Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Stefano Stabellini

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 <sstabllini@kernel.org>

---
    Changes in v3:
        - Add Stefano's acked-by

    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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 10/28] xen/arm32: head: Don't clobber r14/lr in the macro PRINT
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (8 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 09/28] xen/arm32: head: Mark the end of subroutines with ENDPROC Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 11/28] xen/arm32: head: Rework UART initialization on boot CPU Julien Grall
                   ` (17 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Add Stefano's acked-by

    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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 11/28] xen/arm32: head: Rework UART initialization on boot CPU
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (9 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 10/28] xen/arm32: head: Don't clobber r14/lr in the macro PRINT Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 12/28] xen/arm32: head: Introduce print_reg Julien Grall
                   ` (16 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v3:
        - s/Ouput/Output/
        - Add Stefano's reviewed-by

    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..fd3a273550 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.
+ *
+ * Output:
+ *  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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 12/28] xen/arm32: head: Introduce print_reg
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (10 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 11/28] xen/arm32: head: Rework UART initialization on boot CPU Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 13/28] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs Julien Grall
                   ` (15 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Add Stefano's reviewed-by

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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 13/28] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (11 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 12/28] xen/arm32: head: Introduce print_reg Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-22 17:11   ` Stefano Stabellini
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 14/28] xen/arm32: head: Rework and document check_cpu_mode() Julien Grall
                   ` (14 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Remove hard tab
        - s/ID map/1:1 mapping/

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

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index c4ee06ba93..4285f76463 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 1:1 mapping. 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,22 @@ 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 1:1 mapping. 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 +228,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 +248,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 +266,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 +290,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 +397,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 +416,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 +475,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 +502,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 +513,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 14/28] xen/arm32: head: Rework and document check_cpu_mode()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (12 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 13/28] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-22 17:14   ` Stefano Stabellini
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 15/28] xen/arm32: head: Rework and document zero_bss() Julien Grall
                   ` (13 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 4285f76463..c7b4fe4cd4 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -206,6 +206,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)
@@ -220,15 +230,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 15/28] xen/arm32: head: Rework and document zero_bss()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (13 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 14/28] xen/arm32: head: Rework and document check_cpu_mode() Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 16/28] xen/arm32: head: Document create_pages_tables() Julien Grall
                   ` (12 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Add Stefano's reviewed-by

    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 c7b4fe4cd4..1189ed6c47 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
@@ -238,11 +237,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
@@ -254,7 +257,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 16/28] xen/arm32: head: Document create_pages_tables()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (14 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 15/28] xen/arm32: head: Rework and document zero_bss() Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 17/28] xen/arm32: head: Document enable_mmu() Julien Grall
                   ` (11 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Add Stefano's acked-by

    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 1189ed6c47..83f8774e2a 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)
@@ -302,18 +302,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 17/28] xen/arm32: head: Document enable_mmu()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (15 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 16/28] xen/arm32: head: Document create_pages_tables() Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 18/28] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path Julien Grall
                   ` (10 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Add Stefano's acked-by

    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 83f8774e2a..f8603051e4 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -423,6 +423,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 18/28] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (16 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 17/28] xen/arm32: head: Document enable_mmu() Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-22 17:17   ` Stefano Stabellini
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 19/28] xen/arm32: head: Don't setup the fixmap on secondary CPUs Julien Grall
                   ` (9 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - There is no need to zero r5

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

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index f8603051e4..0c95d1c432 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -202,6 +202,25 @@ 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 */
+        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)
 
@@ -505,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 19/28] xen/arm32: head: Don't setup the fixmap on secondary CPUs
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (17 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 18/28] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used Julien Grall
                   ` (8 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Add Stefano's reviewed-by

    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 0c95d1c432..8f945d318a 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)
 
@@ -201,8 +205,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (18 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 19/28] xen/arm32: head: Don't setup the fixmap on secondary CPUs Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-22 18:17   ` Stefano Stabellini
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 21/28] xen/arm32: head: Rework and document setup_fixmap() Julien Grall
                   ` (7 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Remove unused label
        - Avoid harcoding slots

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

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 8f945d318a..34fc9ffcee 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -32,6 +32,10 @@
 #define PT_UPPER(x) (PT_##x & 0xf00)
 #define PT_LOWER(x) (PT_##x & 0x0ff)
 
+/* Convenience defines to get slot used by Xen mapping. */
+#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
+#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
+
 #if (defined (CONFIG_EARLY_PRINTK)) && (defined (EARLY_PRINTK_INC))
 #include EARLY_PRINTK_INC
 #endif
@@ -158,6 +162,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 +485,63 @@ 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 XEN_FIRST_SLOT. For slot XEN_FIRST_SLOT,
+         * the 1:1 mapping was done in the second table.
          */
-        dsb
+        lsr   r1, r9, #FIRST_SHIFT
+        mov_w r0, LPAE_ENTRY_MASK
+        and  r1, r1, r0              /* r1 := first slot */
+        cmp  r1, #XEN_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 XEN_SECOND_SLOT. For slot XEN_SECOND_SLOT,
+         * 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, #XEN_SECOND_SLOT
+        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) */
@@ -489,7 +551,6 @@ 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:
 
         /* Map fixmap into boot_second */
         ldr   r1, =boot_second       /* r1 := vaddr (boot_second) */
@@ -501,19 +562,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 21/28] xen/arm32: head: Rework and document setup_fixmap()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (19 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 22/28] xen/arm32: head: Rework and document launch() Julien Grall
                   ` (6 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v3:
        - The unused label is now removed in the previous patch
        - Add Stefano's reviewed-by

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

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 34fc9ffcee..ee5c3d2af8 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -541,8 +541,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
@@ -551,6 +564,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 */
+#endif
 
         /* Map fixmap into boot_second */
         ldr   r1, =boot_second       /* r1 := vaddr (boot_second) */
@@ -565,7 +579,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 22/28] xen/arm32: head: Rework and document launch()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (20 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 21/28] xen/arm32: head: Rework and document setup_fixmap() Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 23/28] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb Julien Grall
                   ` (5 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v3:
        - Add Stefano's reviewed-by

    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 ee5c3d2af8..3c18037575 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -174,6 +174,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)
 
@@ -238,6 +243,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)
 
@@ -583,19 +591,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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 23/28] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (21 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 22/28] xen/arm32: head: Rework and document launch() Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-22 17:15   ` Stefano Stabellini
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 24/28] xen/arm: Zero BSS after the MMU and D-cache is turned on Julien Grall
                   ` (4 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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.

Lastly, the only use of r5 in create_page_tables() is now removed. So
the register can be removed from the clobber list of the function.

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

---
    Changes in v3:
        - Move the comment in the correct place
        - r5 is not cloberred anymore

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

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 3c18037575..2317fb8855 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -359,7 +359,7 @@ ENDPROC(cpu_init)
  *   r9 : paddr(start)
  *   r10: phys offset
  *
- * Clobbers r0 - r6
+ * Clobbers r0 - r4, r6
  *
  * Register usage within this function:
  *   r6 : Identity map in place
@@ -374,11 +374,8 @@ create_page_tables:
         moveq r6, #1                 /* r6 := identity map now in place */
         movne r6, #0                 /* r6 := identity map not yet in place */
 
-        /* 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
@@ -484,6 +481,13 @@ enable_mmu:
         mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLBs */
         dsb   nsh
 
+        /* Write Xen's PT's paddr into the HTTBR */
+        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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 24/28] xen/arm: Zero BSS after the MMU and D-cache is turned on
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (22 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 23/28] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 25/28] xen/arm64: head: Introduce macros to create table and mapping entry Julien Grall
                   ` (3 subsequent siblings)
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

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 v3:
        - Add Stefano's reviewed-by

    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 2317fb8855..e86a9f95e7 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -153,7 +153,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
@@ -174,6 +173,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 */
@@ -284,17 +284,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 a6f3aa4ee5..f2a0e1d3b0 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -309,7 +309,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
@@ -330,6 +329,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 */
@@ -432,7 +432,6 @@ ENDPROC(check_cpu_mode)
  * Zero BSS
  *
  * Inputs:
- *   x20: Physical offset
  *   x26: Do we need to zero BSS?
  *
  * Clobbers x0 - x3
@@ -442,8 +441,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 e1cdeaaf2f..65552da4ba 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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 25/28] xen/arm64: head: Introduce macros to create table and mapping entry
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (23 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 24/28] xen/arm: Zero BSS after the MMU and D-cache is turned on Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-22 23:31   ` Stefano Stabellini
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 26/28] xen/arm64: head: Use a page mapping for the 1:1 mapping in create_page_tables() Julien Grall
                   ` (2 subsequent siblings)
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

At the moment, any update to the boot-pages are open-coded. This is
making more difficult to understand the logic of a function as each
update roughly requires 6 instructions.

To ease the readability, two new macros are introduced:
    - create_table_entry: Create a page-table entry in a given table.
    This can work at any level.
    - create_mapping_entry: Create a mapping entry in a given table.
    None of the users will require to map at any other level than 3rd
    (i.e page granularity). So the macro is supporting support 3rd level
    mapping.

Furthermore, the two macros are capable to work independently of the
state of the MMU.

Lastly, take the opportunity to replace open-coded version in
setup_fixmap() by the two new macros. The ones in create_page_tables()
will be replaced in a follow-up patch.

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

---
    Changes in v3:
        - Patch added
---
 xen/arch/arm/arm64/head.S | 83 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index f2a0e1d3b0..f4177dbba1 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -492,6 +492,68 @@ cpu_init:
 ENDPROC(cpu_init)
 
 /*
+ * Macro to create a page table entry in \ptbl to \tbl
+ *
+ * ptbl:    table symbol where the entry will be created
+ * tbl:     table symbol to point to
+ * virt:    virtual address
+ * shift:   #imm page table shift
+ * tmp1:    scratch register
+ * tmp2:    scratch register
+ * tmp3:    scratch register
+ *
+ * Preserves \virt
+ * Clobbers \tmp1, \tmp2, \tmp3
+ *
+ * Also use x20 for the phys offset.
+ *
+ * Note that all parameters using registers should be distinct.
+ */
+.macro create_table_entry, ptbl, tbl, virt, shift, tmp1, tmp2, tmp3
+        lsr   \tmp1, \virt, #\shift
+        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
+
+        load_paddr \tmp2, \tbl
+        mov   \tmp3, #PT_PT                 /* \tmp3 := right for linear PT */
+        orr   \tmp3, \tmp3, \tmp2           /*          + \tlb paddr */
+
+        adr_l \tmp2, \ptbl
+
+        str   \tmp3, [\tmp2, \tmp1, lsl #3]
+.endm
+
+/*
+ * Macro to create a mapping entry in \tbl to \phys. Only mapping in 3rd
+ * level table (i.e page granularity) is supported.
+ *
+ * tbl:     table symbol where the entry will be created
+ * virt:    virtual address
+ * phys:    physical address (should be page aligned)
+ * tmp1:    scratch register
+ * tmp2:    scratch register
+ * tmp3:    scratch register
+ * type:    mapping type. If not specified it will be normal memory (PT_MEM_L3)
+ *
+ * Preserves \virt, \phys
+ * Clobbers \tmp1, \tmp2, \tmp3
+ *
+ * Note that all parameters using registers should be distinct.
+ */
+.macro create_mapping_entry, tbl, virt, phys, tmp1, tmp2, tmp3, type=PT_MEM_L3
+        and   \tmp3, \phys, #THIRD_MASK     /* \tmp3 := PAGE_ALIGNED(phys) */
+
+        lsr   \tmp1, \virt, #THIRD_SHIFT
+        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
+
+        mov   \tmp2, #\type                 /* \tmp2 := right for section PT */
+        orr   \tmp2, \tmp2, \tmp3           /*          + PAGE_ALIGNED(phys) */
+
+        adr_l \tmp3, \tbl
+
+        str   \tmp2, [\tmp3, \tmp1, lsl #3]
+.endm
+
+/*
  * Rebuild the boot pagetable's first-level entries. The structure
  * is described in mm.c.
  *
@@ -735,28 +797,17 @@ ENDPROC(remove_identity_mapping)
  *   x20: Physical offset
  *   x23: Early UART base physical address
  *
- * Clobbers x1 - x4
+ * Clobbers x0 - x3
  */
 setup_fixmap:
 #ifdef CONFIG_EARLY_PRINTK
         /* Add UART to the fixmap table */
-        ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
-        lsr   x2, x23, #THIRD_SHIFT
-        lsl   x2, x2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
-        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 */
+        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
+        create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3
 #endif
-
         /* Map fixmap into boot_second */
-        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
-        load_paddr x2, xen_fixmap
-        mov   x3, #PT_PT
-        orr   x2, x2, x3             /* x2 := table map of xen_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 */
-
+        ldr   x0, =FIXMAP_ADDR(0)
+        create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, x1, x2, x3
         /* Ensure any page table updates made above have occurred. */
         dsb   nshst
 
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 26/28] xen/arm64: head: Use a page mapping for the 1:1 mapping in create_page_tables()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (24 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 25/28] xen/arm64: head: Introduce macros to create table and mapping entry Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-24  1:16   ` Stefano Stabellini
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 27/28] xen/arm32: head: Introduce macros to create table and mapping entry Julien Grall
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 28/28] xen/arm32: head: Use a page mapping for the 1:1 mapping in create_page_tables() Julien Grall
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

At the moment the function create_page_tables() will use 1GB/2MB
mapping for the identity mapping. As we don't know what is present
before and after Xen in memory, we may end up to map
device/reserved-memory with cacheable memory. This may result to
mismatched attributes as other users may access the same region
differently.

To prevent any issues, we should only map the strict minimum in the
1:1 mapping. A check in xen.lds.S already guarantees anything
necessary for turning on the MMU fits in a page (at the moment 4K).

As only one page will be mapped for the 1:1 mapping, it is necessary
to pre-allocate a page for the 3rd level table.

For simplicity, all the tables that may be necessary for setting up the
1:1 mapping are linked together in advance. They will then be linked to
the boot page tables at the correct level.

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

---
    Changes in v3:
        - Patch added
---
 xen/arch/arm/arm64/head.S | 176 ++++++++++++++++++++--------------------------
 xen/arch/arm/mm.c         |   2 +
 2 files changed, 78 insertions(+), 100 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index f4177dbba1..1e5b1035b8 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -566,100 +566,17 @@ ENDPROC(cpu_init)
  *   x19: paddr(start)
  *   x20: phys offset
  *
- * Clobbers x0 - x4, x25
- *
- * Register usage within this function:
- *   x25: Identity map in place
+ * Clobbers x0 - x4
  */
 create_page_tables:
-        /*
-         * If Xen is loaded at exactly XEN_VIRT_START then we don't
-         * need an additional 1:1 mapping, the virtual mapping will
-         * suffice.
-         */
-        cmp   x19, #XEN_VIRT_START
-        cset  x25, eq                /* x25 := identity map in place, or not */
-
-        load_paddr x4, boot_pgtable
-
-        /* Setup boot_pgtable: */
-        load_paddr x1, boot_first
-
-        /* ... map boot_first in boot_pgtable[0] */
-        mov   x3, #PT_PT             /* x2 := table map of boot_first */
-        orr   x2, x1, x3             /*       + rights for linear PT */
-        str   x2, [x4, #0]           /* Map it in slot 0 */
-
-        /* ... map of paddr(start) in boot_pgtable+boot_first_id */
-        lsr   x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in boot_pgtable */
-        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
-         * to use an extra first level page in which we create a 1GB mapping.
-         */
-        load_paddr x2, boot_first_id
-
-        mov   x3, #PT_PT             /* x2 := table map of boot_first_id */
-        orr   x2, x2, x3             /*       + rights for linear PT */
-        str   x2, [x4, x1, lsl #3]
-
-        load_paddr x4, boot_first_id
-
-        lsr   x1, x19, #FIRST_SHIFT  /* x1 := Offset of base paddr in boot_first_id */
-        lsl   x2, x1, #FIRST_SHIFT   /* x2 := Base address for 1GB mapping */
-        mov   x3, #PT_MEM            /* x2 := Section map */
-        orr   x2, x2, x3
-        and   x1, x1, #LPAE_ENTRY_MASK /* x1 := Slot offset */
-        str   x2, [x4, x1, lsl #3]   /* Mapping of paddr(start) */
-        mov   x25, #1                /* x25 := identity map now in place */
-
-1:      /* Setup boot_first: */
-        load_paddr x4, boot_first   /* Next level into boot_first */
-
-        /* ... map boot_second in boot_first[0] */
-        load_paddr x1, boot_second
-        mov   x3, #PT_PT             /* x2 := table map of boot_second */
-        orr   x2, x1, x3             /*       + rights for linear PT */
-        str   x2, [x4, #0]           /* Map it in slot 0 */
-
-        /* ... map of paddr(start) in boot_first */
-        cbnz  x25, 1f                /* x25 is set if already created */
-        lsr   x2, x19, #FIRST_SHIFT  /* x2 := Offset of base paddr in boot_first */
-        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
-        cbz   x1, 1f                 /* It's in slot 0, map in boot_second */
-
-        lsl   x2, x2, #FIRST_SHIFT   /* Base address for 1GB mapping */
-        mov   x3, #PT_MEM            /* x2 := Section map */
-        orr   x2, x2, x3
-        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
-        mov   x25, #1                /* x25 := identity map now in place */
-
-1:      /* Setup boot_second: */
-        load_paddr x4, boot_second
-
-        /* ... map boot_third in boot_second[1] */
-        load_paddr x1, boot_third
-        mov   x3, #PT_PT             /* x2 := table map of boot_third */
-        orr   x2, x1, x3             /*       + rights for linear PT */
-        str   x2, [x4, #8]           /* Map it in slot 1 */
-
-        /* ... map of paddr(start) in boot_second */
-        cbnz  x25, 1f                /* x25 is set if already created */
-        lsr   x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in boot_second */
-        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
-        cmp   x1, #1
-        b.eq  virtphys_clash         /* It's in slot 1, which we cannot handle */
-
-        lsl   x2, x2, #SECOND_SHIFT  /* Base address for 2MB mapping */
-        mov   x3, #PT_MEM            /* x2 := Section map */
-        orr   x2, x2, x3
-        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
-        mov   x25, #1                /* x25 := identity map now in place */
+        /* Prepare the page-tables for mapping Xen */
+        ldr   x0, =XEN_VIRT_START
+        create_table_entry boot_pgtable, boot_first, x0, ZEROETH_SHIFT, x1, x2, x3
+        create_table_entry boot_first, boot_second, x0, FIRST_SHIFT, x1, x2, x3
+        create_table_entry boot_second, boot_third, x0, SECOND_SHIFT, x1, x2, x3
 
-1:      /* Setup boot_third: */
-        load_paddr x4, boot_third
+        /* Map Xen */
+        adr_l x4, boot_third
 
         lsr   x2, x19, #THIRD_SHIFT  /* Base address for 4K mapping */
         lsl   x2, x2, #THIRD_SHIFT
@@ -674,21 +591,80 @@ create_page_tables:
         cmp   x1, #(LPAE_ENTRIES<<3) /* 512 entries per page */
         b.lt  1b
 
-        /* Defer fixmap and dtb mapping until after paging enabled, to
-         * avoid them clashing with the 1:1 mapping. */
+        /*
+         * If Xen is loaded at exactly XEN_VIRT_START then we don't
+         * need an additional 1:1 mapping, the virtual mapping will
+         * suffice.
+         */
+        cmp   x19, #XEN_VIRT_START
+        bne   1f
+        ret
+1:
+        /*
+         * Only the first page of Xen will be part of the 1:1 mapping.
+         * All the boot_*_id tables are linked together even if they may
+         * not be all used. They will then be linked to the boot page
+         * tables at the correct level.
+         */
+        create_table_entry boot_first_id, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
+        create_table_entry boot_second_id, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
+        create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
+
+        /*
+         * Find the zeroeth slot used. Link boot_first_id into
+         * boot_pgtable if the slot is not XEN_ZEROETH_SLOT. For slot
+         * XEN_ZEROETH_SLOT, the tables associated with the 1:1 mapping
+         * will need to be linked in boot_first or boot_second.
+         */
+        lsr   x0, x19, #ZEROETH_SHIFT   /* x0 := zeroeth slot */
+        cmp   x0, #XEN_ZEROETH_SLOT
+        beq   1f
+        /*
+         * It is not in slot XEN_ZEROETH_SLOT. Link boot_first_id
+         * into boot_pgtable.
+         */
+        create_table_entry boot_pgtable, boot_first_id, x19, ZEROETH_SHIFT, x0, x1, x2
+        ret
+
+1:
+        /*
+         * Find the first slot used. Link boot_second_id into boot_first
+         * if the slot is not XEN_FIRST_SLOT. For slot XEN_FIRST_SLOT,
+         * the tables associated with the 1:1 mapping will need to be
+         * linked in boot_second.
+         */
+        lsr   x0, x19, #FIRST_SHIFT
+        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
+        cmp   x0, #XEN_FIRST_SLOT
+        beq   1f
+        /*
+         * It is not in slot XEN_FIRST_SLOT. Link boot_second_id into
+         * boot_first
+         */
+        create_table_entry boot_first, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
+        ret
 
-        /* boot pagetable setup complete */
+1:
+        /*
+         * Find the second slot used. Link boot_third_id into boot_second
+         * if the slot is not XEN_SECOND_SLOT. For slot XEN_SECOND_SLOT,
+         * Xen is not yet able to handle it.
+         */
+        lsr   x0, x19, #SECOND_SHIFT
+        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
+        cmp   x0, #XEN_SECOND_SLOT
+        beq   virtphys_clash
+        /*
+         * It is not in slot XEN_SECOND_SLOT. Link boot_third_id into
+         * boot_second.
+         */
+        create_table_entry boot_second, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
+        ret
 
-        cbnz  x25, 1f                /* Did we manage to create an identity mapping ? */
-        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
-
-1:
-        ret
 ENDPROC(create_page_tables)
 
 /*
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 65552da4ba..72ffea7472 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -105,6 +105,8 @@ DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
 #ifdef CONFIG_ARM_64
 DEFINE_BOOT_PAGE_TABLE(boot_first);
 DEFINE_BOOT_PAGE_TABLE(boot_first_id);
+DEFINE_BOOT_PAGE_TABLE(boot_second_id);
+DEFINE_BOOT_PAGE_TABLE(boot_third_id);
 #endif
 DEFINE_BOOT_PAGE_TABLE(boot_second);
 DEFINE_BOOT_PAGE_TABLE(boot_third);
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 27/28] xen/arm32: head: Introduce macros to create table and mapping entry
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (25 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 26/28] xen/arm64: head: Use a page mapping for the 1:1 mapping in create_page_tables() Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  2019-08-23  1:10   ` Stefano Stabellini
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 28/28] xen/arm32: head: Use a page mapping for the 1:1 mapping in create_page_tables() Julien Grall
  27 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

At the moment, any update to the boot-pages are open-coded. This is
making more difficult to understand the logic of a function as each
update roughly requires 6 instructions.

To ease the readability, two new macros are introduced:
    - create_table_entry: Create a page-table entry in a given table.
    This can work at any level.
    - create_mapping_entry: Create a mapping entry in a given table.
    None of the users will require to map at any other level than 3rd
    (i.e page granularity). So the macro is supporting support 3rd level
    mapping.

Unlike arm64, there are no easy way to have a PC relative address within
the range -/+4GB. In order to have the possibility to use the macro in
context with MMU on/off, the user needs to tell the state of the MMU.

Lastly, take the opportunity to replace open-coded version in
setup_fixmap() by the two new macros. The ones in create_page_tables()
will be replaced in a follow-up patch.

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

---
    The adr_l hack is a bit ugly, but I can't find nicer way to avoid
    code duplication and improve readability.

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

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index e86a9f95e7..6d03fecaf2 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -50,6 +50,20 @@
 .endm
 
 /*
+ * There are no easy way to have a PC relative address within the range
+ * +/- 4GB of the PC.
+ *
+ * This macro workaround it by asking the user to tell whether the MMU
+ * has been turned on or not.
+ */
+.macro adr_l, dst, sym, mmu
+        ldr   \dst, =\sym
+        .if \mmu == 0
+        add   \dst, \dst, r10
+        .endif
+.endm
+
+/*
  * Common register usage in this file:
  *   r0  -
  *   r1  -
@@ -342,6 +356,76 @@ cpu_init_done:
 ENDPROC(cpu_init)
 
 /*
+ * Macro to create a page table entry in \ptbl to \tbl
+ *
+ * ptbl:    table symbol where the entry will be created
+ * tbl:     table symbol to point to
+ * virt:    virtual address
+ * shift:   #imm page table shift
+ * mmu:     Is the MMU turned on/off. If not specified it will be off
+ *
+ * Preserves \virt
+ * Clobbers r1 - r4
+ *
+ * Also use r10 for the phys offset.
+ *
+ * Note that \virt should be in a register other than r1 - r4
+ */
+.macro create_table_entry, ptbl, tbl, virt, shift, mmu=0
+        lsr   r1, \virt, #\shift
+        mov_w r2, LPAE_ENTRY_MASK
+        and   r1, r1, r2             /* r1 := slot in \tlb */
+        lsl   r1, r1, #3             /* r1 := slot offset in \tlb */
+
+        ldr   r4, =\tbl
+        add   r4, r4, r10            /* r4 := paddr(\tlb) */
+
+        mov   r2, #PT_PT             /* r2:r3 := right for linear PT */
+        orr   r2, r2, r4             /*           + \tlb paddr */
+        mov   r3, #0
+
+        adr_l r4, \ptbl, \mmu
+
+        strd  r2, r3, [r4, r1]
+.endm
+
+/*
+ * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
+ * level table (i.e page granularity) is supported.
+ *
+ * tbl:     table symbol where the entry will be created
+ * virt:    virtual address
+ * phys:    physical address
+ * type:    mapping type. If not specified it will be normal memory (PT_MEM_L3)
+ * mmu:     Is the MMU turned on/off. If not specified it will be off
+ *
+ * Preserves \virt, \phys
+ * Clobbers r1 - r4
+ *
+ * * Also use r10 for the phys offset.
+ *
+ * Note that \virt and \paddr should be in other registers than r1 - r4
+ * and be distinct.
+ */
+.macro create_mapping_entry, tbl, virt, phys, type=PT_MEM_L3, mmu=0
+        lsr   r4, \phys, #THIRD_SHIFT
+        lsl   r4, r4, #THIRD_SHIFT   /* r4 := PAGE_ALIGNED(phys) */
+
+        mov_w r2, LPAE_ENTRY_MASK
+        lsr   r1, \virt, #THIRD_SHIFT
+        and   r1, r1, r2             /* r1 := slot in \tlb */
+        lsl   r1, r1, #3             /* r1 := slot offset in \tlb */
+
+        mov   r2, #\type             /* r2:r3 := right for section PT */
+        orr   r2, r2, r4             /*          + PAGE_ALIGNED(phys) */
+        mov   r3, #0
+
+        adr_l r4, \tbl, \mmu
+
+        strd  r2, r3, [r4, r1]
+.endm
+
+/*
  * Rebuild the boot pagetable's first-level entries. The structure
  * is described in mm.c.
  *
@@ -559,31 +643,17 @@ ENDPROC(remove_identity_mapping)
  *   r10: Physical offset
  *   r11: Early UART base physical address
  *
- * Clobbers r1 - r4
+ * Clobbers r0 - r4
  */
 setup_fixmap:
 #if defined(CONFIG_EARLY_PRINTK)
         /* Add UART to the fixmap table */
-        ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
-        lsr   r2, r11, #THIRD_SHIFT
-        lsl   r2, r2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
-        orr   r2, r2, #PT_UPPER(DEV_L3)
-        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 */
+        ldr   r0, =EARLY_UART_VIRTUAL_ADDRESS
+        create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3, mmu=1
 #endif
-
         /* Map fixmap into boot_second */
-        ldr   r1, =boot_second       /* r1 := vaddr (boot_second) */
-        ldr   r2, =xen_fixmap
-        add   r2, r2, r10            /* r2 := paddr (xen_fixmap) */
-        orr   r2, r2, #PT_UPPER(PT)
-        orr   r2, r2, #PT_LOWER(PT)  /* r2:r3 := table map of xen_fixmap */
-        ldr   r4, =FIXMAP_ADDR(0)
-        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 */
-
+        mov_w r0, FIXMAP_ADDR(0)
+        create_table_entry boot_second, xen_fixmap, r0, SECOND_SHIFT, mmu=1
         /* Ensure any page table updates made above have occurred. */
         dsb   nshst
 
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [Xen-devel] [PATCH v3 28/28] xen/arm32: head: Use a page mapping for the 1:1 mapping in create_page_tables()
  2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
                   ` (26 preceding siblings ...)
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 27/28] xen/arm32: head: Introduce macros to create table and mapping entry Julien Grall
@ 2019-08-12 17:30 ` Julien Grall
  27 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Volodymyr Babchuk

At the moment the function create_page_tables() will use 1GB/2MB
mapping for the identity mapping. As we don't know what is present
before and after Xen in memory, we may end up to map
device/reserved-memory with cacheable memory. This may result to
mismatched attributes as other users may access the same region
differently.

To prevent any issues, we should only map the strict minimum in the
1:1 mapping. A check in xen.lds.S already guarantees anything
necessary for turning on the MMU fits in a page (at the moment 4K).

As only one page will be mapped for the 1:1 mapping, it is necessary
to pre-allocate a page for the 3rd level table.

For simplicity, all the tables that may be necessary for setting up the
1:1 mapping are linked together in advance. They will then be linked to
the boot page tables at the correct level.

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

---
    Changes in v3:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 119 ++++++++++++++++++----------------------------
 xen/arch/arm/mm.c         |   2 +-
 2 files changed, 48 insertions(+), 73 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 6d03fecaf2..dec6266803 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -444,73 +444,13 @@ ENDPROC(cpu_init)
  *   r6 : Identity map in place
  */
 create_page_tables:
-        /*
-         * If Xen is loaded at exactly XEN_VIRT_START then we don't
-         * need an additional 1:1 mapping, the virtual mapping will
-         * suffice.
-         */
-        cmp   r9, #XEN_VIRT_START
-        moveq r6, #1                 /* r6 := identity map now in place */
-        movne r6, #0                 /* r6 := identity map not yet in place */
-
-        ldr   r4, =boot_pgtable
-        add   r4, r4, r10            /* r4 := paddr (boot_pagetable) */
-
-        /* Setup boot_pgtable: */
-        ldr   r1, =boot_second
-        add   r1, r1, r10            /* r1 := paddr (boot_second) */
-
-        /* ... map boot_second in boot_pgtable[0] */
-        orr   r2, r1, #PT_UPPER(PT)  /* r2:r3 := table map of boot_second */
-        orr   r2, r2, #PT_LOWER(PT)  /* (+ rights for linear PT) */
-        mov   r3, #0x0
-        strd  r2, r3, [r4, #0]       /* Map it in slot 0 */
-
-        /* ... map of paddr(start) in boot_pgtable */
-        lsrs  r1, r9, #FIRST_SHIFT   /* Offset of base paddr in boot_pgtable */
-        beq   1f                     /* If it is in slot 0 then map in boot_second
-                                      * later on */
-        lsl   r2, r1, #FIRST_SHIFT   /* Base address for 1GB mapping */
-        orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
-        orr   r2, r2, #PT_LOWER(MEM)
-        lsl   r1, r1, #3             /* r1 := Slot offset */
-        mov   r3, #0x0
-        strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
-        mov   r6, #1                 /* r6 := identity map now in place */
-
-1:      /* Setup boot_second: */
-        ldr   r4, =boot_second
-        add   r4, r4, r10            /* r4 := paddr (boot_second) */
-
-        ldr   r1, =boot_third
-        add   r1, r1, r10            /* r1 := paddr (boot_third) */
-
-        /* ... map boot_third in boot_second[1] */
-        orr   r2, r1, #PT_UPPER(PT)  /* r2:r3 := table map of boot_third */
-        orr   r2, r2, #PT_LOWER(PT)  /* (+ rights for linear PT) */
-        mov   r3, #0x0
-        strd  r2, r3, [r4, #8]       /* Map it in slot 1 */
-
-        /* ... map of paddr(start) in boot_second */
-        cmp   r6, #1                 /* r6 is set if already created */
-        beq   1f
-        lsr   r2, r9, #SECOND_SHIFT  /* Offset of base paddr in boot_second */
-        ldr   r3, =LPAE_ENTRY_MASK
-        and   r1, r2, r3
-        cmp   r1, #1
-        beq   virtphys_clash         /* It's in slot 1, which we cannot handle */
-
-        lsl   r2, r2, #SECOND_SHIFT  /* Base address for 2MB mapping */
-        orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
-        orr   r2, r2, #PT_LOWER(MEM)
-        mov   r3, #0x0
-        lsl   r1, r1, #3             /* r1 := Slot offset */
-        strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
-        mov   r6, #1                 /* r6 := identity map now in place */
+        /* Prepare the page-tables for mapping Xen */
+        ldr   r0, =XEN_VIRT_START
+        create_table_entry boot_pgtable, boot_second, r0, FIRST_SHIFT
+        create_table_entry boot_second, boot_third, r0, SECOND_SHIFT
 
         /* Setup boot_third: */
-1:      ldr   r4, =boot_third
-        add   r4, r4, r10            /* r4 := paddr (boot_third) */
+        adr_l r4, boot_third, mmu=0
 
         lsr   r2, r9, #THIRD_SHIFT  /* Base address for 4K mapping */
         lsl   r2, r2, #THIRD_SHIFT
@@ -527,16 +467,51 @@ create_page_tables:
         blo   1b
 
         /*
-         * Defer fixmap and dtb mapping until after paging enabled, to
-         * avoid them clashing with the 1:1 mapping.
+         * If Xen is loaded at exactly XEN_VIRT_START then we don't
+         * need an additional 1:1 mapping, the virtual mapping will
+         * suffice.
          */
+        cmp   r9, #XEN_VIRT_START
+        moveq pc, lr
 
-        /* boot pagetable setup complete */
+1:
+        /*
+         * Only the first page of Xen will be part of the 1:1 mapping.
+         * All the boot_*_id tables are linked together even if they may
+         * not be all used. They will then be linked to the boot page
+         * tables at the correct level.
+         */
+        create_table_entry boot_second_id, boot_third_id, r9, SECOND_SHIFT
+        create_mapping_entry boot_third_id, r9, r9
+
+        /*
+         * Find the first slot used. Link boot_second_id into boot_first
+         * if the slot is not 0. For slot 0, the tables associated with
+         * the 1:1 mapping will need to be linked in boot_second.
+         */
+        lsr   r0, r9, #FIRST_SHIFT
+        mov_w r1, LPAE_ENTRY_MASK
+        ands  r0, r0, r1             /* r0 := first slot */
+        beq   1f
+        /* It is not in slot 0, Link boot_second_id into boot_first */
+        create_table_entry boot_pgtable, boot_second_id, r9, FIRST_SHIFT
+        mov   pc, lr
+
+1:
+        /*
+         * Find the second slot used. Link boot_third_id into boot_second
+         * if the slot is not 1 (runtime Xen mapping is 2M - 4M).
+         * For slot 1, Xen is not yet able to handle it.
+         */
+        lsr   r0, r9, #SECOND_SHIFT
+        mov_w r1, LPAE_ENTRY_MASK
+        and   r0, r0, r1             /* x0 := first slot */
+        cmp   r0, #1
+        beq   virtphys_clash
+        /* It is not in slot 1, link boot_third_id into boot_second */
+        create_table_entry boot_second, boot_third_id, r9, SECOND_SHIFT
+        mov   pc, lr
 
-        cmp   r6, #1                /* Did we manage to create an identity mapping ? */
-        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")
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 72ffea7472..9e0fdc39f9 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -105,9 +105,9 @@ DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
 #ifdef CONFIG_ARM_64
 DEFINE_BOOT_PAGE_TABLE(boot_first);
 DEFINE_BOOT_PAGE_TABLE(boot_first_id);
+#endif
 DEFINE_BOOT_PAGE_TABLE(boot_second_id);
 DEFINE_BOOT_PAGE_TABLE(boot_third_id);
-#endif
 DEFINE_BOOT_PAGE_TABLE(boot_second);
 DEFINE_BOOT_PAGE_TABLE(boot_third);
 
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 09/28] xen/arm32: head: Mark the end of subroutines with ENDPROC
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 09/28] xen/arm32: head: Mark the end of subroutines with ENDPROC Julien Grall
@ 2019-08-12 17:33   ` Julien Grall
  0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-12 17:33 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, Volodymyr Babchuk



On 12/08/2019 18:30, 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 <sstabllini@kernel.org>

Hmmm, I made a typo in Stefano's e-mail address. I can fix it on commit message 
(or next version).

Cheers,

> 
> ---
>      Changes in v3:
>          - Add Stefano's acked-by
> 
>      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
> 

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 01/28] xen/arm: lpae: Allow more LPAE helpers to be used in assembly
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 01/28] xen/arm: lpae: Allow more LPAE helpers to be used in assembly Julien Grall
@ 2019-08-22 17:07   ` Stefano Stabellini
  0 siblings, 0 replies; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 17:07 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 2019, Julien Grall wrote:
> A follow-up patch will require to use *_table_offset() and *_MASK helpers
> from assembly. This can be achieved by using _AT() macro to remove the type
> when called from assembly.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v3:
>         - Patch added
> ---
>  xen/include/asm-arm/lpae.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index c22780f8f3..4797f9cee4 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -245,19 +245,19 @@ TABLE_OFFSET_HELPERS(64);
>  
>  #define THIRD_SHIFT    (PAGE_SHIFT)
>  #define THIRD_ORDER    (THIRD_SHIFT - PAGE_SHIFT)
> -#define THIRD_SIZE     ((paddr_t)1 << THIRD_SHIFT)
> +#define THIRD_SIZE     (_AT(paddr_t, 1) << THIRD_SHIFT)
>  #define THIRD_MASK     (~(THIRD_SIZE - 1))
>  #define SECOND_SHIFT   (THIRD_SHIFT + LPAE_SHIFT)
>  #define SECOND_ORDER   (SECOND_SHIFT - PAGE_SHIFT)
> -#define SECOND_SIZE    ((paddr_t)1 << SECOND_SHIFT)
> +#define SECOND_SIZE    (_AT(paddr_t, 1) << SECOND_SHIFT)
>  #define SECOND_MASK    (~(SECOND_SIZE - 1))
>  #define FIRST_SHIFT    (SECOND_SHIFT + LPAE_SHIFT)
>  #define FIRST_ORDER    (FIRST_SHIFT - PAGE_SHIFT)
> -#define FIRST_SIZE     ((paddr_t)1 << FIRST_SHIFT)
> +#define FIRST_SIZE     (_AT(paddr_t, 1) << FIRST_SHIFT)
>  #define FIRST_MASK     (~(FIRST_SIZE - 1))
>  #define ZEROETH_SHIFT  (FIRST_SHIFT + LPAE_SHIFT)
>  #define ZEROETH_ORDER  (ZEROETH_SHIFT - PAGE_SHIFT)
> -#define ZEROETH_SIZE   ((paddr_t)1 << ZEROETH_SHIFT)
> +#define ZEROETH_SIZE   (_AT(paddr_t, 1) << ZEROETH_SHIFT)
>  #define ZEROETH_MASK   (~(ZEROETH_SIZE - 1))
>  
>  /* Calculate the offsets into the pagetables for a given VA */
> @@ -266,7 +266,7 @@ TABLE_OFFSET_HELPERS(64);
>  #define second_linear_offset(va) ((va) >> SECOND_SHIFT)
>  #define third_linear_offset(va) ((va) >> THIRD_SHIFT)
>  
> -#define TABLE_OFFSET(offs) ((unsigned int)(offs) & LPAE_ENTRY_MASK)
> +#define TABLE_OFFSET(offs) (_AT(unsigned int, offs) & LPAE_ENTRY_MASK)
>  #define first_table_offset(va)  TABLE_OFFSET(first_linear_offset(va))
>  #define second_table_offset(va) TABLE_OFFSET(second_linear_offset(va))
>  #define third_table_offset(va)  TABLE_OFFSET(third_linear_offset(va))
> -- 
> 2.11.0
> 

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 07/28] xen/arm64: head: Fix typo in the documentation on top of init_uart()
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 07/28] xen/arm64: head: Fix typo in the documentation on top of init_uart() Julien Grall
@ 2019-08-22 17:08   ` Stefano Stabellini
  0 siblings, 0 replies; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 17:08 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 2019, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v3:
>         - Patch added
> ---
>  xen/arch/arm/arm64/head.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index cd03101196..a6f3aa4ee5 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -825,7 +825,7 @@ ENTRY(switch_ttbr)
>  /*
>   * Initialize the UART. Should only be called on the boot CPU.
>   *
> - * Ouput:
> + * Output:
>   *  x23: Early UART base physical address
>   *
>   * Clobbers x0 - x1
> -- 
> 2.11.0
> 

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 13/28] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 13/28] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs Julien Grall
@ 2019-08-22 17:11   ` Stefano Stabellini
  0 siblings, 0 replies; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 17:11 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 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>

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


> ---
>     Changes in v3:
>         - Remove hard tab
>         - s/ID map/1:1 mapping/
> 
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 65 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index c4ee06ba93..4285f76463 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 1:1 mapping. 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,22 @@ 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 1:1 mapping. 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 +228,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 +248,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 +266,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 +290,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 +397,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 +416,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 +475,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 +502,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 +513,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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 14/28] xen/arm32: head: Rework and document check_cpu_mode()
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 14/28] xen/arm32: head: Rework and document check_cpu_mode() Julien Grall
@ 2019-08-22 17:14   ` Stefano Stabellini
  2019-09-07 10:35     ` Julien Grall
  0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 17:14 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 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>

Already provided by reviewed-by last time

> ---
>     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 4285f76463..c7b4fe4cd4 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -206,6 +206,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)
> @@ -220,15 +230,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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 23/28] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 23/28] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb Julien Grall
@ 2019-08-22 17:15   ` Stefano Stabellini
  0 siblings, 0 replies; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 17:15 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 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.
> 
> Lastly, the only use of r5 in create_page_tables() is now removed. So
> the register can be removed from the clobber list of the function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v3:
>         - Move the comment in the correct place
>         - r5 is not cloberred anymore
> 
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 3c18037575..2317fb8855 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -359,7 +359,7 @@ ENDPROC(cpu_init)
>   *   r9 : paddr(start)
>   *   r10: phys offset
>   *
> - * Clobbers r0 - r6
> + * Clobbers r0 - r4, r6
>   *
>   * Register usage within this function:
>   *   r6 : Identity map in place
> @@ -374,11 +374,8 @@ create_page_tables:
>          moveq r6, #1                 /* r6 := identity map now in place */
>          movne r6, #0                 /* r6 := identity map not yet in place */
>  
> -        /* 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
> @@ -484,6 +481,13 @@ enable_mmu:
>          mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLBs */
>          dsb   nsh
>  
> +        /* Write Xen's PT's paddr into the HTTBR */
> +        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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 18/28] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 18/28] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path Julien Grall
@ 2019-08-22 17:17   ` Stefano Stabellini
  0 siblings, 0 replies; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 17:17 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 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>

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


> ---
>     Changes in v3:
>         - There is no need to zero r5
> 
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index f8603051e4..0c95d1c432 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -202,6 +202,25 @@ 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 */
> +        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)
>  
> @@ -505,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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used
  2019-08-12 17:29 ` [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used Julien Grall
@ 2019-08-22 17:58   ` Stefano Stabellini
  2019-08-22 18:25     ` Julien Grall
  0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 17:58 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 2019, Julien Grall wrote:
> 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 v3:
>         - Avoid hardcoding slots
> 
>     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 | 94 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 79 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 50cff08756..ec138aae3e 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -33,6 +33,11 @@
>  #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
>  #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
>  
> +/* Convenience defines to get slot used by Xen mapping. */
> +#define XEN_ZEROETH_SLOT    zeroeth_table_offset(XEN_VIRT_START)
> +#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
> +#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
> +
>  #define __HEAD_FLAG_PAGE_SIZE   ((PAGE_SHIFT - 10) / 2)
>  
>  #define __HEAD_FLAG_PHYS_BASE   1
> @@ -301,6 +306,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. */
> @@ -626,10 +638,71 @@ enable_mmu:
>          ret
>  ENDPROC(enable_mmu)
>  
> +/*
> + * Remove the 1:1 map for the page-tables. It is not easy to keep track
                         ^ from

> + * 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 XEN_ZEROETH_SLOT. 

This part of the comment is good


> +         * For slot XEN_ZEROETH_SLOT, the 1:1 mapping was either done in first
> +         * or second table.

I don't think this sentence is very useful now that the slot is not
hard-coded anymore. I would remove it. Instead, I would write something
like "The slot XEN_ZEROETH_SLOT is used for the XEN_VIRT_START mapping."
The same goes for all the other levels.


> +         */
> +        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
> +        cmp   x1, #XEN_ZEROETH_SLOT
> +        beq   1f
> +        /* It is not in slot XEN_ZEROETH_SLOT, 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 XEN_FIRST_SLOT. For slot XEN_FIRST_SLOT,
> +         * the 1:1 mapping was done in the second table.
> +         */
> +        lsr   x1, x19, #FIRST_SHIFT
> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> +        cmp   x1, #XEN_FIRST_SLOT
> +        beq   1f
> +        /* It is not in slot XEN_FIRST_SLOT, 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 XEN_SECOND_SLOT. For slot XEN_SECOND_SLOT,
> +         * 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, #XEN_SECOND_SLOT
> +        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

I just want to point out that asm-arm/arm64/flushtlb.h says to use:

 * DSB ISHST        // Ensure prior page-tables updates have completed
 * TLBI...          // Invalidate the TLB
 * DSB ISH          // Ensure the TLB invalidation has completed
 * ISB              // See explanation below

Also implemented as:

    asm volatile(               \
        "dsb  ishst;"           \
        "tlbi "  # tlbop  ";"   \
        "dsb  ish;"             \
        "isb;"                  \
        : : : "memory");        \

Why is non-shareable enough? Shouldn't it be inner shareable?


> +        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) */
> @@ -647,19 +720,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

Same here about shareability


> +#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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used Julien Grall
@ 2019-08-22 18:17   ` Stefano Stabellini
  2019-08-22 18:31     ` Julien Grall
  0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 18:17 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 2019, Julien Grall wrote:
> 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 v3:
>         - Remove unused label
>         - Avoid harcoding slots
> 
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 86 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 8f945d318a..34fc9ffcee 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -32,6 +32,10 @@
>  #define PT_UPPER(x) (PT_##x & 0xf00)
>  #define PT_LOWER(x) (PT_##x & 0x0ff)
>  
> +/* Convenience defines to get slot used by Xen mapping. */
> +#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
> +#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
> +
>  #if (defined (CONFIG_EARLY_PRINTK)) && (defined (EARLY_PRINTK_INC))
>  #include EARLY_PRINTK_INC
>  #endif
> @@ -158,6 +162,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 +485,63 @@ 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 XEN_FIRST_SLOT. For slot XEN_FIRST_SLOT,
> +         * the 1:1 mapping was done in the second table.

May I suggest something more similar to the arm64 version as a comment?
Something like:

"Find the first slot used. Remove the entry for the first table if the
slot is not XEN_FIRST_SLOT. In case of XEN_FIRST_SLOT, we have to look
at the second table as the slot is shared with the XEN_VIRT_START
mapping."


>           */
> -        dsb
> +        lsr   r1, r9, #FIRST_SHIFT
> +        mov_w r0, LPAE_ENTRY_MASK

ldr?


> +        and  r1, r1, r0              /* r1 := first slot */
> +        cmp  r1, #XEN_FIRST_SLOT

NIT: align


> +        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 XEN_SECOND_SLOT. For slot XEN_SECOND_SLOT,
> +         * 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, #XEN_SECOND_SLOT
> +        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

As for the arm64 patch, I would like to understand dsb ishst vs. dsb
nshst.


> +        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) */
> @@ -489,7 +551,6 @@ 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:
>  
>          /* Map fixmap into boot_second */
>          ldr   r1, =boot_second       /* r1 := vaddr (boot_second) */
> @@ -501,19 +562,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

same here


> +#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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used
  2019-08-22 17:58   ` Stefano Stabellini
@ 2019-08-22 18:25     ` Julien Grall
  2019-08-22 18:29       ` Stefano Stabellini
  0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-22 18:25 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Volodymyr Babchuk

Hi Stefano,

On 8/22/19 6:58 PM, Stefano Stabellini wrote:
> On Mon, 12 Aug 2019, Julien Grall wrote:
>> 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 v3:
>>          - Avoid hardcoding slots
>>
>>      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 | 94 +++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 79 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 50cff08756..ec138aae3e 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -33,6 +33,11 @@
>>   #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
>>   #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
>>   
>> +/* Convenience defines to get slot used by Xen mapping. */
>> +#define XEN_ZEROETH_SLOT    zeroeth_table_offset(XEN_VIRT_START)
>> +#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
>> +#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
>> +
>>   #define __HEAD_FLAG_PAGE_SIZE   ((PAGE_SHIFT - 10) / 2)
>>   
>>   #define __HEAD_FLAG_PHYS_BASE   1
>> @@ -301,6 +306,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. */
>> @@ -626,10 +638,71 @@ enable_mmu:
>>           ret
>>   ENDPROC(enable_mmu)
>>   
>> +/*
>> + * Remove the 1:1 map for the page-tables. It is not easy to keep track
>                           ^ from

I will fix it in the next version.

> 
>> + * 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 XEN_ZEROETH_SLOT.
> 
> This part of the comment is good
> 
> 
>> +         * For slot XEN_ZEROETH_SLOT, the 1:1 mapping was either done in first
>> +         * or second table.
> 
> I don't think this sentence is very useful now that the slot is not
> hard-coded anymore. I would remove it. Instead, I would write something
> like "The slot XEN_ZEROETH_SLOT is used for the XEN_VIRT_START mapping."
> The same goes for all the other levels.

I think this is a bit confusing because one may think the XEN_VIRT_START 
mapping is using the full slot. Whereas it is only partially using it.

So I would prefer to drop the sentence completely.

> 
> 
>> +         */
>> +        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
>> +        cmp   x1, #XEN_ZEROETH_SLOT
>> +        beq   1f
>> +        /* It is not in slot XEN_ZEROETH_SLOT, 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 XEN_FIRST_SLOT. For slot XEN_FIRST_SLOT,
>> +         * the 1:1 mapping was done in the second table.
>> +         */
>> +        lsr   x1, x19, #FIRST_SHIFT
>> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
>> +        cmp   x1, #XEN_FIRST_SLOT
>> +        beq   1f
>> +        /* It is not in slot XEN_FIRST_SLOT, 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 XEN_SECOND_SLOT. For slot XEN_SECOND_SLOT,
>> +         * 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, #XEN_SECOND_SLOT
>> +        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
> 
> I just want to point out that asm-arm/arm64/flushtlb.h says to use:
> 
>   * DSB ISHST        // Ensure prior page-tables updates have completed
>   * TLBI...          // Invalidate the TLB
>   * DSB ISH          // Ensure the TLB invalidation has completed
>   * ISB              // See explanation below
> 
> Also implemented as:
> 
>      asm volatile(               \
>          "dsb  ishst;"           \
>          "tlbi "  # tlbop  ";"   \
>          "dsb  ish;"             \
>          "isb;"                  \
>          : : : "memory");        \
> 
> Why is non-shareable enough? Shouldn't it be inner shareable?

I thought I answered this before. I should have probably clarified in 
the commit message.

nsh is used (rather than ish) because the TLB flush is local (see page 
D5-230 ARM DDI 0487D.a). For convenience here is the text:

"In all cases in this section where a DMB or DSB is referred to, it
refers to a DMB or DSB whose required access type is
both loads and stores. A DSB NSH is sufficient to ensure completion of
TLB maintenance instructions that apply to a
single PE. A DSB ISH is sufficient to ensure completion of TLB
maintenance instructions that apply to PEs in the
same Inner Shareable domain."

This is something Linux already does but I wasn't able to find the 
proper justification in the Arm Arm. So I chose a more conservative 
approach that is explained in section K11.5.3 (ARM DDI 0487D.a).

I have an action to update tlbflush.h before this is in a huge pile of 
cleanup/optimization.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used
  2019-08-22 18:25     ` Julien Grall
@ 2019-08-22 18:29       ` Stefano Stabellini
  0 siblings, 0 replies; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 18:29 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Thu, 22 Aug 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 8/22/19 6:58 PM, Stefano Stabellini wrote:
> > On Mon, 12 Aug 2019, Julien Grall wrote:
> > > 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 v3:
> > >          - Avoid hardcoding slots
> > > 
> > >      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 | 94
> > > +++++++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 79 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > index 50cff08756..ec138aae3e 100644
> > > --- a/xen/arch/arm/arm64/head.S
> > > +++ b/xen/arch/arm/arm64/head.S
> > > @@ -33,6 +33,11 @@
> > >   #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1
> > > */
> > >   #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1
> > > */
> > >   +/* Convenience defines to get slot used by Xen mapping. */
> > > +#define XEN_ZEROETH_SLOT    zeroeth_table_offset(XEN_VIRT_START)
> > > +#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
> > > +#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
> > > +
> > >   #define __HEAD_FLAG_PAGE_SIZE   ((PAGE_SHIFT - 10) / 2)
> > >     #define __HEAD_FLAG_PHYS_BASE   1
> > > @@ -301,6 +306,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. */
> > > @@ -626,10 +638,71 @@ enable_mmu:
> > >           ret
> > >   ENDPROC(enable_mmu)
> > >   +/*
> > > + * Remove the 1:1 map for the page-tables. It is not easy to keep track
> >                           ^ from
> 
> I will fix it in the next version.
> 
> > 
> > > + * 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 XEN_ZEROETH_SLOT.
> > 
> > This part of the comment is good
> > 
> > 
> > > +         * For slot XEN_ZEROETH_SLOT, the 1:1 mapping was either done in
> > > first
> > > +         * or second table.
> > 
> > I don't think this sentence is very useful now that the slot is not
> > hard-coded anymore. I would remove it. Instead, I would write something
> > like "The slot XEN_ZEROETH_SLOT is used for the XEN_VIRT_START mapping."
> > The same goes for all the other levels.
> 
> I think this is a bit confusing because one may think the XEN_VIRT_START
> mapping is using the full slot. Whereas it is only partially using it.
> 
> So I would prefer to drop the sentence completely.

That is also OK


> > > +         */
> > > +        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
> > > +        cmp   x1, #XEN_ZEROETH_SLOT
> > > +        beq   1f
> > > +        /* It is not in slot XEN_ZEROETH_SLOT, 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 XEN_FIRST_SLOT. For slot
> > > XEN_FIRST_SLOT,
> > > +         * the 1:1 mapping was done in the second table.
> > > +         */
> > > +        lsr   x1, x19, #FIRST_SHIFT
> > > +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> > > +        cmp   x1, #XEN_FIRST_SLOT
> > > +        beq   1f
> > > +        /* It is not in slot XEN_FIRST_SLOT, 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 XEN_SECOND_SLOT. For slot
> > > XEN_SECOND_SLOT,
> > > +         * 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, #XEN_SECOND_SLOT
> > > +        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
> > 
> > I just want to point out that asm-arm/arm64/flushtlb.h says to use:
> > 
> >   * DSB ISHST        // Ensure prior page-tables updates have completed
> >   * TLBI...          // Invalidate the TLB
> >   * DSB ISH          // Ensure the TLB invalidation has completed
> >   * ISB              // See explanation below
> > 
> > Also implemented as:
> > 
> >      asm volatile(               \
> >          "dsb  ishst;"           \
> >          "tlbi "  # tlbop  ";"   \
> >          "dsb  ish;"             \
> >          "isb;"                  \
> >          : : : "memory");        \
> > 
> > Why is non-shareable enough? Shouldn't it be inner shareable?
> 
> I thought I answered this before. I should have probably clarified in the
> commit message.

I had the feeling you already answered but couldn't find the reference.
Yes please add something to the commit message or an in-code comment,
especially given that tlbflush.h is not updated yet.


> nsh is used (rather than ish) because the TLB flush is local (see page D5-230
> ARM DDI 0487D.a). For convenience here is the text:
> 
> "In all cases in this section where a DMB or DSB is referred to, it
> refers to a DMB or DSB whose required access type is
> both loads and stores. A DSB NSH is sufficient to ensure completion of
> TLB maintenance instructions that apply to a
> single PE. A DSB ISH is sufficient to ensure completion of TLB
> maintenance instructions that apply to PEs in the
> same Inner Shareable domain."
> 
> This is something Linux already does but I wasn't able to find the proper
> justification in the Arm Arm. So I chose a more conservative approach that is
> explained in section K11.5.3 (ARM DDI 0487D.a).
> 
> I have an action to update tlbflush.h before this is in a huge pile of
> cleanup/optimization.


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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used
  2019-08-22 18:17   ` Stefano Stabellini
@ 2019-08-22 18:31     ` Julien Grall
  2019-08-22 22:53       ` Stefano Stabellini
  0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2019-08-22 18:31 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Volodymyr Babchuk

Hi Stefano,

On 8/22/19 7:17 PM, Stefano Stabellini wrote:
> On Mon, 12 Aug 2019, Julien Grall wrote:
>> 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 v3:
>>          - Remove unused label
>>          - Avoid harcoding slots
>>
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm32/head.S | 86 +++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 69 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 8f945d318a..34fc9ffcee 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -32,6 +32,10 @@
>>   #define PT_UPPER(x) (PT_##x & 0xf00)
>>   #define PT_LOWER(x) (PT_##x & 0x0ff)
>>   
>> +/* Convenience defines to get slot used by Xen mapping. */
>> +#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
>> +#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
>> +
>>   #if (defined (CONFIG_EARLY_PRINTK)) && (defined (EARLY_PRINTK_INC))
>>   #include EARLY_PRINTK_INC
>>   #endif
>> @@ -158,6 +162,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 +485,63 @@ 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 XEN_FIRST_SLOT. For slot XEN_FIRST_SLOT,
>> +         * the 1:1 mapping was done in the second table.
> 
> May I suggest something more similar to the arm64 version as a comment?
> Something like:
> 
> "Find the first slot used. Remove the entry for the first table if the
> slot is not XEN_FIRST_SLOT. In case of XEN_FIRST_SLOT, we have to look
> at the second table as the slot is shared with the XEN_VIRT_START
> mapping."

I have answered that on the arm64 version. Let's continue the 
conversation there.

> 
> 
>>            */
>> -        dsb
>> +        lsr   r1, r9, #FIRST_SHIFT
>> +        mov_w r0, LPAE_ENTRY_MASK
> 
> ldr?

What's wrong with the mov_w? Ok it is two instructions but... the 
constant will be stored in a literal and therefore induce a memory load 
(see patch #8).


> 
> 
>> +        and  r1, r1, r0              /* r1 := first slot */
>> +        cmp  r1, #XEN_FIRST_SLOT
> 
> NIT: align

align of?

> 
> 
>> +        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 XEN_SECOND_SLOT. For slot XEN_SECOND_SLOT,
>> +         * 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, #XEN_SECOND_SLOT
>> +        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
> 
> As for the arm64 patch, I would like to understand dsb ishst vs. dsb
> nshst.


I have answered that on the arm64 version. Let's continue the 
conversation there.

> 
> 
>> +        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) */
>> @@ -489,7 +551,6 @@ 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:
>>   
>>           /* Map fixmap into boot_second */
>>           ldr   r1, =boot_second       /* r1 := vaddr (boot_second) */
>> @@ -501,19 +562,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
> 
> same here


I have answered that on the arm64 version. Let's continue the 
conversation there.

> 
> 
>> +#endif
>>           mov   pc, lr
>>   ENDPROC(setup_fixmap)
>>   
>> -- 
>> 2.11.0
>>

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used
  2019-08-22 18:31     ` Julien Grall
@ 2019-08-22 22:53       ` Stefano Stabellini
  2019-08-22 23:39         ` Julien Grall
  0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 22:53 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Thu, 22 Aug 2019, Julien Grall wrote:
> > >            */
> > > -        dsb
> > > +        lsr   r1, r9, #FIRST_SHIFT
> > > +        mov_w r0, LPAE_ENTRY_MASK
> > 
> > ldr?
> 
> What's wrong with the mov_w? Ok it is two instructions but... the constant
> will be stored in a literal and therefore induce a memory load (see patch #8).

I am just wondering why you would choose mov_w when you can just do a
single simple ldr.
 

> > 
> > > +        and  r1, r1, r0              /* r1 := first slot */
> > > +        cmp  r1, #XEN_FIRST_SLOT
> > 
> > NIT: align
> 
> align of?

This is just a code style thing. It doesn't show here but we usually
have 3 spaces between the command the the register, while here there are
only 2.

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 25/28] xen/arm64: head: Introduce macros to create table and mapping entry
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 25/28] xen/arm64: head: Introduce macros to create table and mapping entry Julien Grall
@ 2019-08-22 23:31   ` Stefano Stabellini
  2019-08-22 23:44     ` Julien Grall
  0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-22 23:31 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 2019, Julien Grall wrote:
> At the moment, any update to the boot-pages are open-coded. This is
> making more difficult to understand the logic of a function as each
> update roughly requires 6 instructions.
> 
> To ease the readability, two new macros are introduced:
>     - create_table_entry: Create a page-table entry in a given table.
>     This can work at any level.
>     - create_mapping_entry: Create a mapping entry in a given table.
>     None of the users will require to map at any other level than 3rd
>     (i.e page granularity). So the macro is supporting support 3rd level
                                                         ^ you meant
                                                         only?

>     mapping.
> 
> Furthermore, the two macros are capable to work independently of the
> state of the MMU.
> 
> Lastly, take the opportunity to replace open-coded version in
> setup_fixmap() by the two new macros. The ones in create_page_tables()
> will be replaced in a follow-up patch.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v3:
>         - Patch added
> ---
>  xen/arch/arm/arm64/head.S | 83 ++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 67 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index f2a0e1d3b0..f4177dbba1 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -492,6 +492,68 @@ cpu_init:
>  ENDPROC(cpu_init)
>  
>  /*
> + * Macro to create a page table entry in \ptbl to \tbl
> + *
> + * ptbl:    table symbol where the entry will be created
> + * tbl:     table symbol to point to
> + * virt:    virtual address

Why not pass the virtual address as a symbol too?


> + * shift:   #imm page table shift
> + * tmp1:    scratch register
> + * tmp2:    scratch register
> + * tmp3:    scratch register
> + *
> + * Preserves \virt
> + * Clobbers \tmp1, \tmp2, \tmp3
> + *
> + * Also use x20 for the phys offset.
> + *
> + * Note that all parameters using registers should be distinct.
> + */
> +.macro create_table_entry, ptbl, tbl, virt, shift, tmp1, tmp2, tmp3
> +        lsr   \tmp1, \virt, #\shift
> +        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> +
> +        load_paddr \tmp2, \tbl
> +        mov   \tmp3, #PT_PT                 /* \tmp3 := right for linear PT */
> +        orr   \tmp3, \tmp3, \tmp2           /*          + \tlb paddr */
> +
> +        adr_l \tmp2, \ptbl
> +
> +        str   \tmp3, [\tmp2, \tmp1, lsl #3]
> +.endm
> +
> +/*
> + * Macro to create a mapping entry in \tbl to \phys. Only mapping in 3rd
> + * level table (i.e page granularity) is supported.
> + *
> + * tbl:     table symbol where the entry will be created

NIT: for consistency, I would prefer if you called it ptlb


> + * virt:    virtual address

It could be a symbol here, right?


> + * phys:    physical address (should be page aligned)
> + * tmp1:    scratch register
> + * tmp2:    scratch register
> + * tmp3:    scratch register
> + * type:    mapping type. If not specified it will be normal memory (PT_MEM_L3)
> + *
> + * Preserves \virt, \phys
> + * Clobbers \tmp1, \tmp2, \tmp3
> + *
> + * Note that all parameters using registers should be distinct.
> + */
> +.macro create_mapping_entry, tbl, virt, phys, tmp1, tmp2, tmp3, type=PT_MEM_L3
> +        and   \tmp3, \phys, #THIRD_MASK     /* \tmp3 := PAGE_ALIGNED(phys) */
> +
> +        lsr   \tmp1, \virt, #THIRD_SHIFT
> +        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> +
> +        mov   \tmp2, #\type                 /* \tmp2 := right for section PT */
> +        orr   \tmp2, \tmp2, \tmp3           /*          + PAGE_ALIGNED(phys) */
> +
> +        adr_l \tmp3, \tbl
> +
> +        str   \tmp2, [\tmp3, \tmp1, lsl #3]
> +.endm
> +
> +/*
>   * Rebuild the boot pagetable's first-level entries. The structure
>   * is described in mm.c.
>   *
> @@ -735,28 +797,17 @@ ENDPROC(remove_identity_mapping)
>   *   x20: Physical offset
>   *   x23: Early UART base physical address
>   *
> - * Clobbers x1 - x4
> + * Clobbers x0 - x3
>   */
>  setup_fixmap:
>  #ifdef CONFIG_EARLY_PRINTK
>          /* Add UART to the fixmap table */
> -        ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
> -        lsr   x2, x23, #THIRD_SHIFT
> -        lsl   x2, x2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
> -        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 */
> +        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
> +        create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3
>  #endif
> -
>          /* Map fixmap into boot_second */
> -        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> -        load_paddr x2, xen_fixmap
> -        mov   x3, #PT_PT
> -        orr   x2, x2, x3             /* x2 := table map of xen_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 */
> -
> +        ldr   x0, =FIXMAP_ADDR(0)
> +        create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, x1, x2, x3
>          /* Ensure any page table updates made above have occurred. */
>          dsb   nshst
>  
> -- 
> 2.11.0
> 

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used
  2019-08-22 22:53       ` Stefano Stabellini
@ 2019-08-22 23:39         ` Julien Grall
  0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-22 23:39 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, Volodymyr Babchuk


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

Sorry for the formatting.

On Thu, 22 Aug 2019, 23:55 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Thu, 22 Aug 2019, Julien Grall wrote:
> > > >            */
> > > > -        dsb
> > > > +        lsr   r1, r9, #FIRST_SHIFT
> > > > +        mov_w r0, LPAE_ENTRY_MASK
> > >
> > > ldr?
> >
> > What's wrong with the mov_w? Ok it is two instructions but... the
> constant
> > will be stored in a literal and therefore induce a memory load (see
> patch #8).
>
> I am just wondering why you would choose mov_w when you can just do a
> single simple ldr.
>

Well, I have just explained it and you likely saw the explanation in patch
#8 as you acked it. So I am not sure what other explanation you are looking
for.

The choice between ldr rX, =... and mov_w is pretty much a matter of taste
from our perspective.

They will both take 64-bit in memory, the former because of one instruction
and the constant stored in the literal pool. The latter because it is using
two instructions.

Technically, we could also reduce the number of instructions to one for
mov_w depending on the constant size. But that's micro-optimization.

mov_w will be slightly more efficient because you don't have an extra
memory load (from loading from a pool). Although it is pretty much
insignificant here.

I also have to admit that the syntax for ldr is slightly confusing. I
always have to look up in other place how it can be used and works.

So I would like to keep the ldr use to the strict minimum, i.e. for
non-cons value (such as symbol).

Cheers,

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

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

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 25/28] xen/arm64: head: Introduce macros to create table and mapping entry
  2019-08-22 23:31   ` Stefano Stabellini
@ 2019-08-22 23:44     ` Julien Grall
  0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-22 23:44 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, Volodymyr Babchuk


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

On Fri, 23 Aug 2019, 00:33 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Mon, 12 Aug 2019, Julien Grall wrote:
> > At the moment, any update to the boot-pages are open-coded. This is
> > making more difficult to understand the logic of a function as each
> > update roughly requires 6 instructions.
> >
> > To ease the readability, two new macros are introduced:
> >     - create_table_entry: Create a page-table entry in a given table.
> >     This can work at any level.
> >     - create_mapping_entry: Create a mapping entry in a given table.
> >     None of the users will require to map at any other level than 3rd
> >     (i.e page granularity). So the macro is supporting support 3rd level
>                                                          ^ you meant
>                                                          only?
>

Yes, I will fix it.


> >     mapping.
> >
> > Furthermore, the two macros are capable to work independently of the
> > state of the MMU.
> >
> > Lastly, take the opportunity to replace open-coded version in
> > setup_fixmap() by the two new macros. The ones in create_page_tables()
> > will be replaced in a follow-up patch.
> >
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> >
> > ---
> >     Changes in v3:
> >         - Patch added
> > ---
> >  xen/arch/arm/arm64/head.S | 83
> ++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 67 insertions(+), 16 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index f2a0e1d3b0..f4177dbba1 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -492,6 +492,68 @@ cpu_init:
> >  ENDPROC(cpu_init)
> >
> >  /*
> > + * Macro to create a page table entry in \ptbl to \tbl
> > + *
> > + * ptbl:    table symbol where the entry will be created
> > + * tbl:     table symbol to point to
> > + * virt:    virtual address
>
> Why not pass the virtual address as a symbol too?
>

Because we have no symbol for most of the virtual addresses. They are just
constant defined in config.h.


>
> > + * shift:   #imm page table shift
> > + * tmp1:    scratch register
> > + * tmp2:    scratch register
> > + * tmp3:    scratch register
> > + *
> > + * Preserves \virt
> > + * Clobbers \tmp1, \tmp2, \tmp3
> > + *
> > + * Also use x20 for the phys offset.
> > + *
> > + * Note that all parameters using registers should be distinct.
> > + */
> > +.macro create_table_entry, ptbl, tbl, virt, shift, tmp1, tmp2, tmp3
> > +        lsr   \tmp1, \virt, #\shift
> > +        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> > +
> > +        load_paddr \tmp2, \tbl
> > +        mov   \tmp3, #PT_PT                 /* \tmp3 := right for
> linear PT */
> > +        orr   \tmp3, \tmp3, \tmp2           /*          + \tlb paddr */
> > +
> > +        adr_l \tmp2, \ptbl
> > +
> > +        str   \tmp3, [\tmp2, \tmp1, lsl #3]
> > +.endm
> > +
> > +/*
> > + * Macro to create a mapping entry in \tbl to \phys. Only mapping in 3rd
> > + * level table (i.e page granularity) is supported.
> > + *
> > + * tbl:     table symbol where the entry will be created
>
> NIT: for consistency, I would prefer if you called it ptlb
>

Ok.


>
> > + * virt:    virtual address
>
> It could be a symbol here, right?
>

No. See above.


>
> > + * phys:    physical address (should be page aligned)
> > + * tmp1:    scratch register
> > + * tmp2:    scratch register
> > + * tmp3:    scratch register
> > + * type:    mapping type. If not specified it will be normal memory
> (PT_MEM_L3)
> > + *
> > + * Preserves \virt, \phys
> > + * Clobbers \tmp1, \tmp2, \tmp3
> > + *
> > + * Note that all parameters using registers should be distinct.
> > + */
> > +.macro create_mapping_entry, tbl, virt, phys, tmp1, tmp2, tmp3,
> type=PT_MEM_L3
> > +        and   \tmp3, \phys, #THIRD_MASK     /* \tmp3 :=
> PAGE_ALIGNED(phys) */
> > +
> > +        lsr   \tmp1, \virt, #THIRD_SHIFT
> > +        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> > +
> > +        mov   \tmp2, #\type                 /* \tmp2 := right for
> section PT */
> > +        orr   \tmp2, \tmp2, \tmp3           /*          +
> PAGE_ALIGNED(phys) */
> > +
> > +        adr_l \tmp3, \tbl
> > +
> > +        str   \tmp2, [\tmp3, \tmp1, lsl #3]
> > +.endm
> > +
> > +/*
> >   * Rebuild the boot pagetable's first-level entries. The structure
> >   * is described in mm.c.
> >   *
> > @@ -735,28 +797,17 @@ ENDPROC(remove_identity_mapping)
> >   *   x20: Physical offset
> >   *   x23: Early UART base physical address
> >   *
> > - * Clobbers x1 - x4
> > + * Clobbers x0 - x3
> >   */
> >  setup_fixmap:
> >  #ifdef CONFIG_EARLY_PRINTK
> >          /* Add UART to the fixmap table */
> > -        ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
> > -        lsr   x2, x23, #THIRD_SHIFT
> > -        lsl   x2, x2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
> > -        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 */
> > +        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
> > +        create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3,
> type=PT_DEV_L3
> >  #endif
> > -
> >          /* Map fixmap into boot_second */
> > -        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> > -        load_paddr x2, xen_fixmap
> > -        mov   x3, #PT_PT
> > -        orr   x2, x2, x3             /* x2 := table map of xen_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 */
> > -
> > +        ldr   x0, =FIXMAP_ADDR(0)
> > +        create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT,
> x1, x2, x3
> >          /* Ensure any page table updates made above have occurred. */
> >          dsb   nshst
> >
> > --
> > 2.11.0
> >
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

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

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 27/28] xen/arm32: head: Introduce macros to create table and mapping entry
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 27/28] xen/arm32: head: Introduce macros to create table and mapping entry Julien Grall
@ 2019-08-23  1:10   ` Stefano Stabellini
  2019-08-23  9:40     ` Julien Grall
  0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-23  1:10 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 2019, Julien Grall wrote:
> At the moment, any update to the boot-pages are open-coded. This is
> making more difficult to understand the logic of a function as each
> update roughly requires 6 instructions.
> 
> To ease the readability, two new macros are introduced:
>     - create_table_entry: Create a page-table entry in a given table.
>     This can work at any level.
>     - create_mapping_entry: Create a mapping entry in a given table.
>     None of the users will require to map at any other level than 3rd
>     (i.e page granularity). So the macro is supporting support 3rd level
>     mapping.
> 
> Unlike arm64, there are no easy way to have a PC relative address within
> the range -/+4GB. In order to have the possibility to use the macro in
> context with MMU on/off, the user needs to tell the state of the MMU.
> 
> Lastly, take the opportunity to replace open-coded version in
> setup_fixmap() by the two new macros. The ones in create_page_tables()
> will be replaced in a follow-up patch.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     The adr_l hack is a bit ugly, but I can't find nicer way to avoid
>     code duplication and improve readability.
> 
>     Changes in v3:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 108 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 89 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index e86a9f95e7..6d03fecaf2 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -50,6 +50,20 @@
>  .endm
>  
>  /*
> + * There are no easy way to have a PC relative address within the range
> + * +/- 4GB of the PC.
> + *
> + * This macro workaround it by asking the user to tell whether the MMU
> + * has been turned on or not.

I would add one statement saying why we are using r10 below in the
implementation. Just a suggestion to make things clearer.


> + */
> +.macro adr_l, dst, sym, mmu
> +        ldr   \dst, =\sym
> +        .if \mmu == 0
> +        add   \dst, \dst, r10
> +        .endif
> +.endm
> +
> +/*
>   * Common register usage in this file:
>   *   r0  -
>   *   r1  -
> @@ -342,6 +356,76 @@ cpu_init_done:
>  ENDPROC(cpu_init)
>  
>  /*
> + * Macro to create a page table entry in \ptbl to \tbl
> + *
> + * ptbl:    table symbol where the entry will be created
> + * tbl:     table symbol to point to
> + * virt:    virtual address
> + * shift:   #imm page table shift
> + * mmu:     Is the MMU turned on/off. If not specified it will be off
> + *
> + * Preserves \virt
> + * Clobbers r1 - r4

In the 64bit version you added the temp registers to the parameter list.
Why do things differently here, hard-coding the usage of r1-r4?


> + * Also use r10 for the phys offset.
> + *
> + * Note that \virt should be in a register other than r1 - r4
> + */
> +.macro create_table_entry, ptbl, tbl, virt, shift, mmu=0
> +        lsr   r1, \virt, #\shift
> +        mov_w r2, LPAE_ENTRY_MASK
> +        and   r1, r1, r2             /* r1 := slot in \tlb */
> +        lsl   r1, r1, #3             /* r1 := slot offset in \tlb */
> +
> +        ldr   r4, =\tbl
> +        add   r4, r4, r10            /* r4 := paddr(\tlb) */

you could use adr_l?


> +        mov   r2, #PT_PT             /* r2:r3 := right for linear PT */
> +        orr   r2, r2, r4             /*           + \tlb paddr */
> +        mov   r3, #0
> +
> +        adr_l r4, \ptbl, \mmu
> +
> +        strd  r2, r3, [r4, r1]
> +.endm
> +
> +/*
> + * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
> + * level table (i.e page granularity) is supported.
> + *
> + * tbl:     table symbol where the entry will be created
> + * virt:    virtual address
> + * phys:    physical address
> + * type:    mapping type. If not specified it will be normal memory (PT_MEM_L3)
> + * mmu:     Is the MMU turned on/off. If not specified it will be off
> + *
> + * Preserves \virt, \phys
> + * Clobbers r1 - r4

Same here


> + * * Also use r10 for the phys offset.
> + *
> + * Note that \virt and \paddr should be in other registers than r1 - r4
> + * and be distinct.
> + */
> +.macro create_mapping_entry, tbl, virt, phys, type=PT_MEM_L3, mmu=0
> +        lsr   r4, \phys, #THIRD_SHIFT
> +        lsl   r4, r4, #THIRD_SHIFT   /* r4 := PAGE_ALIGNED(phys) */

and THIRD_MASK like for arm64? Doesn't really matter but it would be
nicer if we could keep them similar.


> +        mov_w r2, LPAE_ENTRY_MASK
> +        lsr   r1, \virt, #THIRD_SHIFT
> +        and   r1, r1, r2             /* r1 := slot in \tlb */
> +        lsl   r1, r1, #3             /* r1 := slot offset in \tlb */

I would prefer if you moved these for lines up, like you have down in
create_table_entry, it is clearer to read for me. Just an optional
suggestion.


> +        mov   r2, #\type             /* r2:r3 := right for section PT */
> +        orr   r2, r2, r4             /*          + PAGE_ALIGNED(phys) */
> +        mov   r3, #0
> +
> +        adr_l r4, \tbl, \mmu
> +
> +        strd  r2, r3, [r4, r1]
> +.endm
> +
> +/*
>   * Rebuild the boot pagetable's first-level entries. The structure
>   * is described in mm.c.
>   *
> @@ -559,31 +643,17 @@ ENDPROC(remove_identity_mapping)
>   *   r10: Physical offset
>   *   r11: Early UART base physical address
>   *
> - * Clobbers r1 - r4
> + * Clobbers r0 - r4
>   */
>  setup_fixmap:
>  #if defined(CONFIG_EARLY_PRINTK)
>          /* Add UART to the fixmap table */
> -        ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
> -        lsr   r2, r11, #THIRD_SHIFT
> -        lsl   r2, r2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
> -        orr   r2, r2, #PT_UPPER(DEV_L3)
> -        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 */
> +        ldr   r0, =EARLY_UART_VIRTUAL_ADDRESS
> +        create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3, mmu=1
>  #endif
> -
>          /* Map fixmap into boot_second */
> -        ldr   r1, =boot_second       /* r1 := vaddr (boot_second) */
> -        ldr   r2, =xen_fixmap
> -        add   r2, r2, r10            /* r2 := paddr (xen_fixmap) */
> -        orr   r2, r2, #PT_UPPER(PT)
> -        orr   r2, r2, #PT_LOWER(PT)  /* r2:r3 := table map of xen_fixmap */
> -        ldr   r4, =FIXMAP_ADDR(0)
> -        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 */
> -
> +        mov_w r0, FIXMAP_ADDR(0)
> +        create_table_entry boot_second, xen_fixmap, r0, SECOND_SHIFT, mmu=1
>          /* Ensure any page table updates made above have occurred. */
>          dsb   nshst
>  
> -- 
> 2.11.0
> 

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 27/28] xen/arm32: head: Introduce macros to create table and mapping entry
  2019-08-23  1:10   ` Stefano Stabellini
@ 2019-08-23  9:40     ` Julien Grall
  0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-08-23  9:40 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Volodymyr Babchuk

Hi Stefano,

On 23/08/2019 02:10, Stefano Stabellini wrote:
> On Mon, 12 Aug 2019, Julien Grall wrote:
>> At the moment, any update to the boot-pages are open-coded. This is
>> making more difficult to understand the logic of a function as each
>> update roughly requires 6 instructions.
>>
>> To ease the readability, two new macros are introduced:
>>      - create_table_entry: Create a page-table entry in a given table.
>>      This can work at any level.
>>      - create_mapping_entry: Create a mapping entry in a given table.
>>      None of the users will require to map at any other level than 3rd
>>      (i.e page granularity). So the macro is supporting support 3rd level
>>      mapping.
>>
>> Unlike arm64, there are no easy way to have a PC relative address within
>> the range -/+4GB. In order to have the possibility to use the macro in
>> context with MMU on/off, the user needs to tell the state of the MMU.
>>
>> Lastly, take the opportunity to replace open-coded version in
>> setup_fixmap() by the two new macros. The ones in create_page_tables()
>> will be replaced in a follow-up patch.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      The adr_l hack is a bit ugly, but I can't find nicer way to avoid
>>      code duplication and improve readability.
>>
>>      Changes in v3:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm32/head.S | 108 ++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 89 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index e86a9f95e7..6d03fecaf2 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -50,6 +50,20 @@
>>   .endm
>>   
>>   /*
>> + * There are no easy way to have a PC relative address within the range
>> + * +/- 4GB of the PC.
>> + *
>> + * This macro workaround it by asking the user to tell whether the MMU
>> + * has been turned on or not.
> 
> I would add one statement saying why we are using r10 below in the
> implementation. Just a suggestion to make things clearer.

Sure.

> 
> 
>> + */
>> +.macro adr_l, dst, sym, mmu
>> +        ldr   \dst, =\sym
>> +        .if \mmu == 0
>> +        add   \dst, \dst, r10
>> +        .endif
>> +.endm
>> +
>> +/*
>>    * Common register usage in this file:
>>    *   r0  -
>>    *   r1  -
>> @@ -342,6 +356,76 @@ cpu_init_done:
>>   ENDPROC(cpu_init)
>>   
>>   /*
>> + * Macro to create a page table entry in \ptbl to \tbl
>> + *
>> + * ptbl:    table symbol where the entry will be created
>> + * tbl:     table symbol to point to
>> + * virt:    virtual address
>> + * shift:   #imm page table shift
>> + * mmu:     Is the MMU turned on/off. If not specified it will be off
>> + *
>> + * Preserves \virt
>> + * Clobbers r1 - r4
> 
> In the 64bit version you added the temp registers to the parameter list.
> Why do things differently here, hard-coding the usage of r1-r4?

A few reasons:
    1) There are 2 more parameters for the arm32 version (one more tmp register 
+ mmu). So the line will be far over 80 characters. A split of the line will not 
be inline with the idea of one line per "instruction" within the file.
    2) strd impose the two registers that will be stored to be contiguous (the 
start register should be an even number). So we would have to convey it to the user.

arm64 version is using the parameter list because I feel macro is not meant to 
modify pre-defined registers. I am aware this is what we do on arm32 but we also 
have symbols in parameters and a function would not have made things much better 
in the callers (you would have to load all symbols in registers before hand).

So this is the best compromised I have found.

> 
> 
>> + * Also use r10 for the phys offset.
>> + *
>> + * Note that \virt should be in a register other than r1 - r4
>> + */
>> +.macro create_table_entry, ptbl, tbl, virt, shift, mmu=0
>> +        lsr   r1, \virt, #\shift
>> +        mov_w r2, LPAE_ENTRY_MASK
>> +        and   r1, r1, r2             /* r1 := slot in \tlb */
>> +        lsl   r1, r1, #3             /* r1 := slot offset in \tlb */
>> +
>> +        ldr   r4, =\tbl
>> +        add   r4, r4, r10            /* r4 := paddr(\tlb) */
> 
> you could use adr_l?

adr_l is for loading relative address. Here we want the physical address 
regarding the state of the MMU.

>> +        mov   r2, #PT_PT             /* r2:r3 := right for linear PT */
>> +        orr   r2, r2, r4             /*           + \tlb paddr */
>> +        mov   r3, #0
>> +
>> +        adr_l r4, \ptbl, \mmu
>> +
>> +        strd  r2, r3, [r4, r1]
>> +.endm
>> +
>> +/*
>> + * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
>> + * level table (i.e page granularity) is supported.
>> + *
>> + * tbl:     table symbol where the entry will be created
>> + * virt:    virtual address
>> + * phys:    physical address
>> + * type:    mapping type. If not specified it will be normal memory (PT_MEM_L3)
>> + * mmu:     Is the MMU turned on/off. If not specified it will be off
>> + *
>> + * Preserves \virt, \phys
>> + * Clobbers r1 - r4
> 
> Same here
> 
> 
>> + * * Also use r10 for the phys offset.
>> + *
>> + * Note that \virt and \paddr should be in other registers than r1 - r4
>> + * and be distinct.
>> + */
>> +.macro create_mapping_entry, tbl, virt, phys, type=PT_MEM_L3, mmu=0
>> +        lsr   r4, \phys, #THIRD_SHIFT
>> +        lsl   r4, r4, #THIRD_SHIFT   /* r4 := PAGE_ALIGNED(phys) */
> 
> and THIRD_MASK like for arm64? Doesn't really matter but it would be
> nicer if we could keep them similar.
I am with you on this point. But sometimes it is not possible (they are two 
distinct instructions set with similarities) or make the code less obvious for 
the arch.

In this case, the instruction 'and' is taking a "modified immediate constant". 
The immediate will be encoded as an 8-bit number than can be rotate.

The value of THIRD_MASK is 0xfffffffffffff000. Therefore it can't be encoded as 
an immediate for the instruction 'and'.

One solution would be to load the immediate in a register, and then using the 
instruction 'and'. But I felt, the lsr/lsl solution was nicer.

> 
> 
>> +        mov_w r2, LPAE_ENTRY_MASK
>> +        lsr   r1, \virt, #THIRD_SHIFT
>> +        and   r1, r1, r2             /* r1 := slot in \tlb */
>> +        lsl   r1, r1, #3             /* r1 := slot offset in \tlb */
> 
> I would prefer if you moved these for lines up, like you have down in
> create_table_entry, it is clearer to read for me. Just an optional
> suggestion.

Sure.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 26/28] xen/arm64: head: Use a page mapping for the 1:1 mapping in create_page_tables()
  2019-08-12 17:30 ` [Xen-devel] [PATCH v3 26/28] xen/arm64: head: Use a page mapping for the 1:1 mapping in create_page_tables() Julien Grall
@ 2019-08-24  1:16   ` Stefano Stabellini
  2019-09-17 17:53     ` Julien Grall
  0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2019-08-24  1:16 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 2019, Julien Grall wrote:
> At the moment the function create_page_tables() will use 1GB/2MB
> mapping for the identity mapping. As we don't know what is present
> before and after Xen in memory, we may end up to map
> device/reserved-memory with cacheable memory. This may result to
> mismatched attributes as other users may access the same region
> differently.
> 
> To prevent any issues, we should only map the strict minimum in the
> 1:1 mapping. A check in xen.lds.S already guarantees anything
> necessary for turning on the MMU fits in a page (at the moment 4K).
> 
> As only one page will be mapped for the 1:1 mapping, it is necessary
> to pre-allocate a page for the 3rd level table.
> 
> For simplicity, all the tables that may be necessary for setting up the
> 1:1 mapping are linked together in advance. They will then be linked to
> the boot page tables at the correct level.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v3:
>         - Patch added
> ---
>  xen/arch/arm/arm64/head.S | 176 ++++++++++++++++++++--------------------------
>  xen/arch/arm/mm.c         |   2 +
>  2 files changed, 78 insertions(+), 100 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index f4177dbba1..1e5b1035b8 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -566,100 +566,17 @@ ENDPROC(cpu_init)
>   *   x19: paddr(start)
>   *   x20: phys offset
>   *
> - * Clobbers x0 - x4, x25
> - *
> - * Register usage within this function:
> - *   x25: Identity map in place
> + * Clobbers x0 - x4
>   */
>  create_page_tables:
> -        /*
> -         * If Xen is loaded at exactly XEN_VIRT_START then we don't
> -         * need an additional 1:1 mapping, the virtual mapping will
> -         * suffice.
> -         */
> -        cmp   x19, #XEN_VIRT_START
> -        cset  x25, eq                /* x25 := identity map in place, or not */
> -
> -        load_paddr x4, boot_pgtable
> -
> -        /* Setup boot_pgtable: */
> -        load_paddr x1, boot_first
> -
> -        /* ... map boot_first in boot_pgtable[0] */
> -        mov   x3, #PT_PT             /* x2 := table map of boot_first */
> -        orr   x2, x1, x3             /*       + rights for linear PT */
> -        str   x2, [x4, #0]           /* Map it in slot 0 */
> -
> -        /* ... map of paddr(start) in boot_pgtable+boot_first_id */
> -        lsr   x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in boot_pgtable */
> -        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
> -         * to use an extra first level page in which we create a 1GB mapping.
> -         */
> -        load_paddr x2, boot_first_id
> -
> -        mov   x3, #PT_PT             /* x2 := table map of boot_first_id */
> -        orr   x2, x2, x3             /*       + rights for linear PT */
> -        str   x2, [x4, x1, lsl #3]
> -
> -        load_paddr x4, boot_first_id
> -
> -        lsr   x1, x19, #FIRST_SHIFT  /* x1 := Offset of base paddr in boot_first_id */
> -        lsl   x2, x1, #FIRST_SHIFT   /* x2 := Base address for 1GB mapping */
> -        mov   x3, #PT_MEM            /* x2 := Section map */
> -        orr   x2, x2, x3
> -        and   x1, x1, #LPAE_ENTRY_MASK /* x1 := Slot offset */
> -        str   x2, [x4, x1, lsl #3]   /* Mapping of paddr(start) */
> -        mov   x25, #1                /* x25 := identity map now in place */
> -
> -1:      /* Setup boot_first: */
> -        load_paddr x4, boot_first   /* Next level into boot_first */
> -
> -        /* ... map boot_second in boot_first[0] */
> -        load_paddr x1, boot_second
> -        mov   x3, #PT_PT             /* x2 := table map of boot_second */
> -        orr   x2, x1, x3             /*       + rights for linear PT */
> -        str   x2, [x4, #0]           /* Map it in slot 0 */
> -
> -        /* ... map of paddr(start) in boot_first */
> -        cbnz  x25, 1f                /* x25 is set if already created */
> -        lsr   x2, x19, #FIRST_SHIFT  /* x2 := Offset of base paddr in boot_first */
> -        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
> -        cbz   x1, 1f                 /* It's in slot 0, map in boot_second */
> -
> -        lsl   x2, x2, #FIRST_SHIFT   /* Base address for 1GB mapping */
> -        mov   x3, #PT_MEM            /* x2 := Section map */
> -        orr   x2, x2, x3
> -        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
> -        mov   x25, #1                /* x25 := identity map now in place */
> -
> -1:      /* Setup boot_second: */
> -        load_paddr x4, boot_second
> -
> -        /* ... map boot_third in boot_second[1] */
> -        load_paddr x1, boot_third
> -        mov   x3, #PT_PT             /* x2 := table map of boot_third */
> -        orr   x2, x1, x3             /*       + rights for linear PT */
> -        str   x2, [x4, #8]           /* Map it in slot 1 */
> -
> -        /* ... map of paddr(start) in boot_second */
> -        cbnz  x25, 1f                /* x25 is set if already created */
> -        lsr   x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in boot_second */
> -        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
> -        cmp   x1, #1
> -        b.eq  virtphys_clash         /* It's in slot 1, which we cannot handle */
> -
> -        lsl   x2, x2, #SECOND_SHIFT  /* Base address for 2MB mapping */
> -        mov   x3, #PT_MEM            /* x2 := Section map */
> -        orr   x2, x2, x3
> -        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
> -        mov   x25, #1                /* x25 := identity map now in place */
> +        /* Prepare the page-tables for mapping Xen */
> +        ldr   x0, =XEN_VIRT_START
> +        create_table_entry boot_pgtable, boot_first, x0, ZEROETH_SHIFT, x1, x2, x3
> +        create_table_entry boot_first, boot_second, x0, FIRST_SHIFT, x1, x2, x3
> +        create_table_entry boot_second, boot_third, x0, SECOND_SHIFT, x1, x2, x3
>  
> -1:      /* Setup boot_third: */
> -        load_paddr x4, boot_third
> +        /* Map Xen */
> +        adr_l x4, boot_third
>  
>          lsr   x2, x19, #THIRD_SHIFT  /* Base address for 4K mapping */
>          lsl   x2, x2, #THIRD_SHIFT
> @@ -674,21 +591,80 @@ create_page_tables:
>          cmp   x1, #(LPAE_ENTRIES<<3) /* 512 entries per page */
>          b.lt  1b

Why can't we use create_mapping_entry here?


> -        /* Defer fixmap and dtb mapping until after paging enabled, to
> -         * avoid them clashing with the 1:1 mapping. */
> +        /*
> +         * If Xen is loaded at exactly XEN_VIRT_START then we don't
> +         * need an additional 1:1 mapping, the virtual mapping will
> +         * suffice.
> +         */
> +        cmp   x19, #XEN_VIRT_START
> +        bne   1f
> +        ret
> +1:
> +        /*
> +         * Only the first page of Xen will be part of the 1:1 mapping.
> +         * All the boot_*_id tables are linked together even if they may
> +         * not be all used. They will then be linked to the boot page
> +         * tables at the correct level.
> +         */
> +        create_table_entry boot_first_id, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
> +        create_table_entry boot_second_id, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2

If I understood the code right, it is not actually required to link
boot_first_id -> boot_second_id and/or boot_second_id -> boot_third_id:
it depends on whether x19 clashes with XEN_FIRST_SLOT, XEN_SECOND_SLOT,
etc. Do you think it would be possible without making the code complex
to only call create_table_entry boot_first_id, boot_second_id and
create_table_entry boot_second_id, boot_third_id when required?  So
moving the calls below after the relevant checks? It looks like it could
be done by adding those calls before "ret". I wouldn't mind if they are
duplicated but we could avoid it by adding appropriate labels and having
a single return path:

out1:
  create_table_entry boot_first_id, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
out2:
  create_table_entry boot_second_id, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
out3:
  ret

(I have similar comments for the arm32 version, I guess your answers
will be the same for both.)


> +        create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
> +
> +        /*
> +         * Find the zeroeth slot used. Link boot_first_id into
> +         * boot_pgtable if the slot is not XEN_ZEROETH_SLOT. For slot
> +         * XEN_ZEROETH_SLOT, the tables associated with the 1:1 mapping
> +         * will need to be linked in boot_first or boot_second.
> +         */
> +        lsr   x0, x19, #ZEROETH_SHIFT   /* x0 := zeroeth slot */
> +        cmp   x0, #XEN_ZEROETH_SLOT
> +        beq   1f
> +        /*
> +         * It is not in slot XEN_ZEROETH_SLOT. Link boot_first_id
> +         * into boot_pgtable.
> +         */
> +        create_table_entry boot_pgtable, boot_first_id, x19, ZEROETH_SHIFT, x0, x1, x2
> +        ret
> +
> +1:
> +        /*
> +         * Find the first slot used. Link boot_second_id into boot_first
> +         * if the slot is not XEN_FIRST_SLOT. For slot XEN_FIRST_SLOT,
> +         * the tables associated with the 1:1 mapping will need to be
> +         * linked in boot_second.
> +         */
> +        lsr   x0, x19, #FIRST_SHIFT
> +        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
> +        cmp   x0, #XEN_FIRST_SLOT
> +        beq   1f
> +        /*
> +         * It is not in slot XEN_FIRST_SLOT. Link boot_second_id into
> +         * boot_first
> +         */
> +        create_table_entry boot_first, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
> +        ret
>  
> -        /* boot pagetable setup complete */
> +1:
> +        /*
> +         * Find the second slot used. Link boot_third_id into boot_second
> +         * if the slot is not XEN_SECOND_SLOT. For slot XEN_SECOND_SLOT,
> +         * Xen is not yet able to handle it.
> +         */
> +        lsr   x0, x19, #SECOND_SHIFT
> +        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
> +        cmp   x0, #XEN_SECOND_SLOT
> +        beq   virtphys_clash
> +        /*
> +         * It is not in slot XEN_SECOND_SLOT. Link boot_third_id into
> +         * boot_second.
> +         */
> +        create_table_entry boot_second, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
> +        ret
>  
> -        cbnz  x25, 1f                /* Did we manage to create an identity mapping ? */
> -        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
> -
> -1:
> -        ret
>  ENDPROC(create_page_tables)
>  
>  /*
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 65552da4ba..72ffea7472 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -105,6 +105,8 @@ DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
>  #ifdef CONFIG_ARM_64
>  DEFINE_BOOT_PAGE_TABLE(boot_first);
>  DEFINE_BOOT_PAGE_TABLE(boot_first_id);
> +DEFINE_BOOT_PAGE_TABLE(boot_second_id);
> +DEFINE_BOOT_PAGE_TABLE(boot_third_id);
>  #endif
>  DEFINE_BOOT_PAGE_TABLE(boot_second);
>  DEFINE_BOOT_PAGE_TABLE(boot_third);
> -- 
> 2.11.0
> 

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 14/28] xen/arm32: head: Rework and document check_cpu_mode()
  2019-08-22 17:14   ` Stefano Stabellini
@ 2019-09-07 10:35     ` Julien Grall
  0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-09-07 10:35 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Volodymyr Babchuk

Hi Stefano,

On 8/22/19 6:14 PM, Stefano Stabellini wrote:
> On Mon, 12 Aug 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>
> 
> Already provided by reviewed-by last time

Whoops, I will add it in the next revision.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [Xen-devel] [PATCH v3 26/28] xen/arm64: head: Use a page mapping for the 1:1 mapping in create_page_tables()
  2019-08-24  1:16   ` Stefano Stabellini
@ 2019-09-17 17:53     ` Julien Grall
  0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2019-09-17 17:53 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Volodymyr Babchuk

Hi Stefano,

On 8/24/19 2:16 AM, Stefano Stabellini wrote:
> On Mon, 12 Aug 2019, Julien Grall wrote:
>>           lsr   x2, x19, #THIRD_SHIFT  /* Base address for 4K mapping */
>>           lsl   x2, x2, #THIRD_SHIFT
>> @@ -674,21 +591,80 @@ create_page_tables:
>>           cmp   x1, #(LPAE_ENTRIES<<3) /* 512 entries per page */
>>           b.lt  1b
> 
> Why can't we use create_mapping_entry here?

If we re-use create_mapping_entry, then we will compute everything at 
each loop.

At the moment, the loop execute 3 instructions (excluding the branch and 
comparison). We would end up to have an extra 5 instructions for arm64 
(9 instruction for arm32).

At the moment, we iterate 512 times the loop, so this adds 2560 
instructions (resp. 4608) in the boot code. Long-term plan, we will want 
to increase the Xen mapping to 4MB (so you double up the number iteration).

This is a less than ideal solution. I haven't come up with a clever 
solution so far, hence why it is left alone.

> 
> 
>> -        /* Defer fixmap and dtb mapping until after paging enabled, to
>> -         * avoid them clashing with the 1:1 mapping. */
>> +        /*
>> +         * If Xen is loaded at exactly XEN_VIRT_START then we don't
>> +         * need an additional 1:1 mapping, the virtual mapping will
>> +         * suffice.
>> +         */
>> +        cmp   x19, #XEN_VIRT_START
>> +        bne   1f
>> +        ret
>> +1:
>> +        /*
>> +         * Only the first page of Xen will be part of the 1:1 mapping.
>> +         * All the boot_*_id tables are linked together even if they may
>> +         * not be all used. They will then be linked to the boot page
>> +         * tables at the correct level.
>> +         */
>> +        create_table_entry boot_first_id, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
>> +        create_table_entry boot_second_id, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
> 
> If I understood the code right, it is not actually required to link
> boot_first_id -> boot_second_id and/or boot_second_id -> boot_third_id:
> it depends on whether x19 clashes with XEN_FIRST_SLOT, XEN_SECOND_SLOT,
> etc. Do you think it would be possible without making the code complex
> to only call create_table_entry boot_first_id, boot_second_id and
> create_table_entry boot_second_id, boot_third_id when required?  So
> moving the calls below after the relevant checks? It looks like it could
> be done by adding those calls before "ret". I wouldn't mind if they are
> duplicated but we could avoid it by adding appropriate labels and having
> a single return path:
> 
> out1:
>    create_table_entry boot_first_id, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
> out2:
>    create_table_entry boot_second_id, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
> out3:
>    ret

I have implemented something similar. I will send it as part of the next 
version in a bit.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

end of thread, other threads:[~2019-09-17 17:54 UTC | newest]

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 01/28] xen/arm: lpae: Allow more LPAE helpers to be used in assembly Julien Grall
2019-08-22 17:07   ` Stefano Stabellini
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used Julien Grall
2019-08-22 17:58   ` Stefano Stabellini
2019-08-22 18:25     ` Julien Grall
2019-08-22 18:29       ` Stefano Stabellini
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 03/28] xen/arm64: head: Rework and document setup_fixmap() Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 04/28] xen/arm64: head: Rework and document launch() Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 05/28] xen/arm64: head: Setup TTBR_EL2 in enable_mmu() and add missing isb Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 06/28] xen/arm64: head: Introduce a macro to get a PC-relative address of a symbol Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 07/28] xen/arm64: head: Fix typo in the documentation on top of init_uart() Julien Grall
2019-08-22 17:08   ` Stefano Stabellini
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 08/28] xen/arm32: head: Add a macro to move an immediate constant into a 32-bit register Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 09/28] xen/arm32: head: Mark the end of subroutines with ENDPROC Julien Grall
2019-08-12 17:33   ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 10/28] xen/arm32: head: Don't clobber r14/lr in the macro PRINT Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 11/28] xen/arm32: head: Rework UART initialization on boot CPU Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 12/28] xen/arm32: head: Introduce print_reg Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 13/28] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs Julien Grall
2019-08-22 17:11   ` Stefano Stabellini
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 14/28] xen/arm32: head: Rework and document check_cpu_mode() Julien Grall
2019-08-22 17:14   ` Stefano Stabellini
2019-09-07 10:35     ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 15/28] xen/arm32: head: Rework and document zero_bss() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 16/28] xen/arm32: head: Document create_pages_tables() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 17/28] xen/arm32: head: Document enable_mmu() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 18/28] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path Julien Grall
2019-08-22 17:17   ` Stefano Stabellini
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 19/28] xen/arm32: head: Don't setup the fixmap on secondary CPUs Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used Julien Grall
2019-08-22 18:17   ` Stefano Stabellini
2019-08-22 18:31     ` Julien Grall
2019-08-22 22:53       ` Stefano Stabellini
2019-08-22 23:39         ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 21/28] xen/arm32: head: Rework and document setup_fixmap() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 22/28] xen/arm32: head: Rework and document launch() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 23/28] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb Julien Grall
2019-08-22 17:15   ` Stefano Stabellini
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 24/28] xen/arm: Zero BSS after the MMU and D-cache is turned on Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 25/28] xen/arm64: head: Introduce macros to create table and mapping entry Julien Grall
2019-08-22 23:31   ` Stefano Stabellini
2019-08-22 23:44     ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 26/28] xen/arm64: head: Use a page mapping for the 1:1 mapping in create_page_tables() Julien Grall
2019-08-24  1:16   ` Stefano Stabellini
2019-09-17 17:53     ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 27/28] xen/arm32: head: Introduce macros to create table and mapping entry Julien Grall
2019-08-23  1:10   ` Stefano Stabellini
2019-08-23  9:40     ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 28/28] xen/arm32: head: Use a page mapping for the 1:1 mapping in create_page_tables() Julien Grall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).