[PATCH early-RFC 0/5] xen/arm: Don't switch TTBR while the MMU is on

[PATCH early-RFC 0/5] xen/arm: Don't switch TTBR while the MMU is on

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

Currently, Xen on Arm will switch TTBR whilst the MMU is on. This is
similar to replacing existing mappings with new ones. So we need to
follow a break-before-make sequence.

When switching the TTBR, we need to temporary disable the MMU
before update the TTBR. This means the page-tables must contain an
identity mapping.

The current memory layout is not very flexible and has an higher chance
to clash with the identity mapping.

On Arm64, we have plenty of unused virtual address space Therefore, we can
simply reshuffle the layout to leave the first part of the virtual
address space empty.

On Arm32, the virtual address space is already quite full. That said,
we are currently reserving 2GB for the temporary mapping. This is far
too much given those mappings are temporary. It would be sufficient
to only reserve a few MBs.

The Arm32 part is not yet addressed in this version. The series is
sent as an early RFC to gather some feedback on the approach.

After this series, most of Xen page-table code should be compliant
with the Arm Arm. The last two issues I am aware of are:
 - domheap: Mappings are replaced without using the Break-Before-Make
   approach.
 - The cache is not cleaned/invalidated when updating the page-tables
   with Data cache off (like during early boot).

This series is based on "xen/arm: mm: Remove open-coding mappings"
along with some extra small patches. Some of them are already merged.
For convience, I pushed a branch with everything applied:

https://xenbits.xen.org/git-http/people/julieng/xen-unstable.git
branch boot-pt-rework-v1

Note the build for arm32 is likely broken. This will be addressed
on the next version.

Cheers,

Julien GralL (1):
  xen/arm: mm: Introduce helpers to prepare/enable/disable the identity
    mapping

Julien Grall (4):
  xen/arm: Clean-up the memory layout
  xen/arm64: Rework the memory layout
  xen/arm: mm: Rework switch_ttbr()
  xen/arm: smpboot: Directly switch to the runtime page-tables

 xen/arch/arm/arm64/head.S         |  63 ++++++++--------
 xen/arch/arm/include/asm/config.h |  44 +++++++----
 xen/arch/arm/include/asm/mm.h     |   2 +
 xen/arch/arm/mm.c                 | 120 +++++++++++++++++++++++-------
 xen/arch/arm/smpboot.c            |   3 +
 5 files changed, 160 insertions(+), 72 deletions(-)

-- 
2.32.0

[PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

In a follow-up patch, the base address for the common mappings will
vary between arm32 and arm64. To avoid any duplication, define
every mapping in the common region from the previous one.

Take the opportunity to add mising *_SIZE for some mappings.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

After the next patch, the term "common" will sound strange because
the base address is different. Any better suggestion?
---
 xen/arch/arm/include/asm/config.h | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index aedb586c8d27..5db28a8dbd56 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -107,16 +107,26 @@
  *  Unused
  */
 
-#define XEN_VIRT_START         _AT(vaddr_t,0x00200000)
-#define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
+#define COMMON_VIRT_START       _AT(vaddr_t, 0)
 
-#define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
-#define BOOT_FDT_SLOT_SIZE     MB(4)
-#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
+#define XEN_VIRT_START          (COMMON_VIRT_START + MB(2))
+#define XEN_SLOT_SIZE           MB(2)
+#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SLOT_SIZE)
+
+#define FIXMAP_VIRT_START       XEN_VIRT_END
+#define FIXMAP_SLOT_SIZE        MB(2)
+#define FIXMAP_VIRT_END         (FIXMAP_VIRT_START + FIXMAP_SLOT_SIZE)
+
+#define FIXMAP_ADDR(n)          (FIXMAP_VIRT_START + (n) * PAGE_SIZE)
+
+#define BOOT_FDT_VIRT_START     FIXMAP_VIRT_END
+#define BOOT_FDT_SLOT_SIZE      MB(4)
+#define BOOT_FDT_VIRT_END       (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
 
 #ifdef CONFIG_LIVEPATCH
-#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
-#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
+#define LIVEPATCH_VMAP_START   BOOT_FDT_VIRT_END
+#define LIVEPATCH_SLOT_SIZE    MB(2)
+#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + LIVEPATCH_SLOT_SIZE)
 #endif
 
 #define HYPERVISOR_VIRT_START  XEN_VIRT_START
-- 
2.32.0

Re: [PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout

[Bertrand Marquis]

Hi Julien,

> On 9 Mar 2022, at 11:20, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> In a follow-up patch, the base address for the common mappings will
> vary between arm32 and arm64. To avoid any duplication, define
> every mapping in the common region from the previous one.
> 
> Take the opportunity to add mising *_SIZE for some mappings.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Changes looks ok to me so:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Only one question after.

> 
> ---
> 
> After the next patch, the term "common" will sound strange because
> the base address is different. Any better suggestion?

MAPPING_VIRT_START ?

Or space maybe..

> ---
> xen/arch/arm/include/asm/config.h | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
> index aedb586c8d27..5db28a8dbd56 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -107,16 +107,26 @@
>  *  Unused
>  */
> 
> -#define XEN_VIRT_START         _AT(vaddr_t,0x00200000)
> -#define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
> +#define COMMON_VIRT_START       _AT(vaddr_t, 0)
> 
> -#define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
> -#define BOOT_FDT_SLOT_SIZE     MB(4)
> -#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
> +#define XEN_VIRT_START          (COMMON_VIRT_START + MB(2))
> +#define XEN_SLOT_SIZE           MB(2)

I know this is not modified by your patch, but any idea why SLOT is used here ?
XEN_VIRT_SIZE would sound a bit more logic.

Cheers
Bertrand

Re: [PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout

[Julien Grall]

On 17/03/2022 15:23, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

>> On 9 Mar 2022, at 11:20, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> In a follow-up patch, the base address for the common mappings will
>> vary between arm32 and arm64. To avoid any duplication, define
>> every mapping in the common region from the previous one.
>>
>> Take the opportunity to add mising *_SIZE for some mappings.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Changes looks ok to me so:
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Only one question after.
> 
>>
>> ---
>>
>> After the next patch, the term "common" will sound strange because
>> the base address is different. Any better suggestion?
> 
> MAPPING_VIRT_START ?

For arm32, I am planning to reshuffle the layout so the runtime address 
is always at the end of the layout.

So I think MAPPING_VIRT_START may be as confusing. How about 
SHARED_ARCH_VIRT_MAPPING?

> 
> Or space maybe..

I am not sure I understand this suggestion. Can you clarify?

> 
>> ---
>> xen/arch/arm/include/asm/config.h | 24 +++++++++++++++++-------
>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
>> index aedb586c8d27..5db28a8dbd56 100644
>> --- a/xen/arch/arm/include/asm/config.h
>> +++ b/xen/arch/arm/include/asm/config.h
>> @@ -107,16 +107,26 @@
>>   *  Unused
>>   */
>>
>> -#define XEN_VIRT_START         _AT(vaddr_t,0x00200000)
>> -#define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>> +#define COMMON_VIRT_START       _AT(vaddr_t, 0)
>>
>> -#define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
>> -#define BOOT_FDT_SLOT_SIZE     MB(4)
>> -#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
>> +#define XEN_VIRT_START          (COMMON_VIRT_START + MB(2))
>> +#define XEN_SLOT_SIZE           MB(2)
> 
> I know this is not modified by your patch, but any idea why SLOT is used here ?
> XEN_VIRT_SIZE would sound a bit more logic.

No idea. I can add a patch to rename it.

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout

[Bertrand Marquis]

Hi Julien,

> On 17 Mar 2022, at 20:32, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 17/03/2022 15:23, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 9 Mar 2022, at 11:20, Julien Grall <julien@xen.org> wrote:
>>> 
>>> From: Julien Grall <jgrall@amazon.com>
>>> 
>>> In a follow-up patch, the base address for the common mappings will
>>> vary between arm32 and arm64. To avoid any duplication, define
>>> every mapping in the common region from the previous one.
>>> 
>>> Take the opportunity to add mising *_SIZE for some mappings.
>>> 
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Changes looks ok to me so:
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> Only one question after.
>>> 
>>> ---
>>> 
>>> After the next patch, the term "common" will sound strange because
>>> the base address is different. Any better suggestion?
>> MAPPING_VIRT_START ?
> 
> For arm32, I am planning to reshuffle the layout so the runtime address is always at the end of the layout.
> 
> So I think MAPPING_VIRT_START may be as confusing. How about SHARED_ARCH_VIRT_MAPPING?

> 
>> Or space maybe..
> 
> I am not sure I understand this suggestion. Can you clarify?

VIRT_SPACE_START was in my mind at that time but that is also not good

How about using BASE instead of start: MAPPING_COMMON_VIRT_BASE ?

Anyway hard to find a nice name, so your solution with SHARED is ok for me unless someone has a better suggestion.

> 
>>> ---
>>> xen/arch/arm/include/asm/config.h | 24 +++++++++++++++++-------
>>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
>>> index aedb586c8d27..5db28a8dbd56 100644
>>> --- a/xen/arch/arm/include/asm/config.h
>>> +++ b/xen/arch/arm/include/asm/config.h
>>> @@ -107,16 +107,26 @@
>>>  *  Unused
>>>  */
>>> 
>>> -#define XEN_VIRT_START         _AT(vaddr_t,0x00200000)
>>> -#define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>>> +#define COMMON_VIRT_START       _AT(vaddr_t, 0)
>>> 
>>> -#define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
>>> -#define BOOT_FDT_SLOT_SIZE     MB(4)
>>> -#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
>>> +#define XEN_VIRT_START          (COMMON_VIRT_START + MB(2))
>>> +#define XEN_SLOT_SIZE           MB(2)
>> I know this is not modified by your patch, but any idea why SLOT is used here ?
>> XEN_VIRT_SIZE would sound a bit more logic.
> 
> No idea. I can add a patch to rename it.

I think it would be a good idea, we already have a lot of terms in here and SLOT is just adding to the confusion I find.

Thanks
Bertrand

[PATCH early-RFC 2/5] xen/arm64: Rework the memory layout

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Xen is currently not fully compliant with the Arm because it will
switch the TTBR with the MMU on.

In order to be compliant, we need to disable the MMU before
switching the TTBR. The implication is the page-tables should
contain an identity mapping of the code switching the TTBR.

If we don't rework the memory layout, we would need to find a
virtual address that matches a physical address and doesn't clash
with the static virtual regions. This can be a bit tricky.

On arm64, the memory layout  has plenty of unused space. In most of
the case we expect Xen to be loaded in low memory.

The memory layout is reshuffled to keep the 0th slot free. Xen will now
be loaded at (512GB + 2MB). This requires a slight tweak of the boot
code as XEN_VIRT_START cannot be used as an immediate.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

    TODO:
        - I vaguely recall that one of the early platform we supported add
          the memory starting in high memory (> 1TB). I need to check
          whether the new layout will be fine.
        - Update the documentation to reflect the new layout
---
 xen/arch/arm/arm64/head.S         |  3 ++-
 xen/arch/arm/include/asm/config.h | 20 ++++++++++++++------
 xen/arch/arm/mm.c                 | 14 +++++++-------
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 66d862fc8137..878649280d73 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -594,7 +594,8 @@ create_page_tables:
          * need an additional 1:1 mapping, the virtual mapping will
          * suffice.
          */
-        cmp   x19, #XEN_VIRT_START
+        ldr   x0, =XEN_VIRT_START
+        cmp   x19, x0
         bne   1f
         ret
 1:
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 5db28a8dbd56..b2f31a914103 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -107,8 +107,20 @@
  *  Unused
  */
 
+#ifdef CONFIG_ARM_32
+
 #define COMMON_VIRT_START       _AT(vaddr_t, 0)
 
+#else
+
+#define SLOT0_ENTRY_BITS  39
+#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
+#define SLOT0_ENTRY_SIZE  SLOT0(1)
+
+#define COMMON_VIRT_START       SLOT(1)
+
+#endif
+
 #define XEN_VIRT_START          (COMMON_VIRT_START + MB(2))
 #define XEN_SLOT_SIZE           MB(2)
 #define XEN_VIRT_END            (XEN_VIRT_START + XEN_SLOT_SIZE)
@@ -161,14 +173,10 @@
 
 #else /* ARM_64 */
 
-#define SLOT0_ENTRY_BITS  39
-#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
-#define SLOT0_ENTRY_SIZE  SLOT0(1)
-
-#define VMAP_VIRT_START  GB(1)
+#define VMAP_VIRT_START  (SLOT0(1) + GB(1))
 #define VMAP_VIRT_END    (VMAP_VIRT_START + GB(1))
 
-#define FRAMETABLE_VIRT_START  GB(32)
+#define FRAMETABLE_VIRT_START  (SLOT0(1) + GB(32))
 #define FRAMETABLE_SIZE        GB(32)
 #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
 #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 6b7c41d827ca..75ed9a3ce249 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -187,11 +187,10 @@ static void __init __maybe_unused build_assertions(void)
     BUILD_BUG_ON(DIRECTMAP_VIRT_START & ~FIRST_MASK);
 #endif
     /* Page table structure constraints */
-#ifdef CONFIG_ARM_64
-    BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START));
-#endif
     BUILD_BUG_ON(first_table_offset(XEN_VIRT_START));
+#ifdef CONFIG_ARM_32
     BUILD_BUG_ON(second_linear_offset(XEN_VIRT_START) >= XEN_PT_LPAE_ENTRIES);
+#endif
 #ifdef CONFIG_DOMAIN_PAGE
     BUILD_BUG_ON(DOMHEAP_VIRT_START & ~FIRST_MASK);
 #endif
@@ -611,10 +610,11 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
     phys_offset = boot_phys_offset;
 
 #ifdef CONFIG_ARM_64
-    p = (void *) xen_pgtable;
-    p[0] = pte_of_xenaddr((uintptr_t)xen_first);
-    p[0].pt.table = 1;
-    p[0].pt.xn = 0;
+    pte = pte_of_xenaddr((uintptr_t)xen_first);
+    pte.pt.table = 1;
+    pte.pt.xn = 0;
+    xen_pgtable[zeroeth_table_offset(XEN_VIRT_START)] = pte;
+
     p = (void *) xen_first;
 #else
     p = (void *) cpu0_pgtable;
-- 
2.32.0

Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout

[Julien Grall]

Hi,

On 09/03/2022 11:20, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Xen is currently not fully compliant with the Arm because it will
> switch the TTBR with the MMU on.
> 
> In order to be compliant, we need to disable the MMU before
> switching the TTBR. The implication is the page-tables should
> contain an identity mapping of the code switching the TTBR.
> 
> If we don't rework the memory layout, we would need to find a
> virtual address that matches a physical address and doesn't clash
> with the static virtual regions. This can be a bit tricky.
> 
> On arm64, the memory layout  has plenty of unused space. In most of
> the case we expect Xen to be loaded in low memory.
> 
> The memory layout is reshuffled to keep the 0th slot free. Xen will now
> be loaded at (512GB + 2MB). This requires a slight tweak of the boot
> code as XEN_VIRT_START cannot be used as an immediate.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
>      TODO:
>          - I vaguely recall that one of the early platform we supported add
>            the memory starting in high memory (> 1TB). I need to check
>            whether the new layout will be fine.
>          - Update the documentation to reflect the new layout
> ---
>   xen/arch/arm/arm64/head.S         |  3 ++-
>   xen/arch/arm/include/asm/config.h | 20 ++++++++++++++------
>   xen/arch/arm/mm.c                 | 14 +++++++-------
>   3 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 66d862fc8137..878649280d73 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -594,7 +594,8 @@ create_page_tables:
>            * need an additional 1:1 mapping, the virtual mapping will
>            * suffice.
>            */
> -        cmp   x19, #XEN_VIRT_START
> +        ldr   x0, =XEN_VIRT_START
> +        cmp   x19, x0
>           bne   1f
>           ret
>   1:
> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
> index 5db28a8dbd56..b2f31a914103 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -107,8 +107,20 @@
>    *  Unused
>    */
>   
> +#ifdef CONFIG_ARM_32
> +
>   #define COMMON_VIRT_START       _AT(vaddr_t, 0)
>   
> +#else
> +
> +#define SLOT0_ENTRY_BITS  39
> +#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
> +#define SLOT0_ENTRY_SIZE  SLOT0(1)
> +
> +#define COMMON_VIRT_START       SLOT(1)

This should have been SLOT0(). I got it right in my tree but merge the 
change to the wrong patch :(.

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout

[Bertrand Marquis]

Hi Julien,

> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Xen is currently not fully compliant with the Arm because it will
I think you wanted to say “arm arm” her.

> switch the TTBR with the MMU on.
> 
> In order to be compliant, we need to disable the MMU before
> switching the TTBR. The implication is the page-tables should
> contain an identity mapping of the code switching the TTBR.
> 
> If we don't rework the memory layout, we would need to find a
> virtual address that matches a physical address and doesn't clash
> with the static virtual regions. This can be a bit tricky.

This sentence is a bit misleading. Even with the rework you need 
to do that just by moving the Xen virtual address upper you make
sure that anything physical memory under 512GB can be mapped
1:1 without clashing with other Xen mappings (unless Xen is loaded
in memory at physical address 512GB which would end in the same issue).

I think should be rephrased.

> 
> On arm64, the memory layout  has plenty of unused space. In most of
> the case we expect Xen to be loaded in low memory.
> 
> The memory layout is reshuffled to keep the 0th slot free. Xen will now

0th slot of first level of page table.

> be loaded at (512GB + 2MB). This requires a slight tweak of the boot
> code as XEN_VIRT_START cannot be used as an immediate.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
>    TODO:
>        - I vaguely recall that one of the early platform we supported add
>          the memory starting in high memory (> 1TB). I need to check
>          whether the new layout will be fine.

I think we have some Juno with some memory like that, tell me if you need help here.

>        - Update the documentation to reflect the new layout
> ---
> xen/arch/arm/arm64/head.S         |  3 ++-
> xen/arch/arm/include/asm/config.h | 20 ++++++++++++++------
> xen/arch/arm/mm.c                 | 14 +++++++-------
> 3 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 66d862fc8137..878649280d73 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -594,7 +594,8 @@ create_page_tables:
>          * need an additional 1:1 mapping, the virtual mapping will
>          * suffice.
>          */
> -        cmp   x19, #XEN_VIRT_START
> +        ldr   x0, =XEN_VIRT_START
> +        cmp   x19, x0
A comment in the code would be good here to prevent someone reverting this.

>         bne   1f
>         ret
> 1:
> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
> index 5db28a8dbd56..b2f31a914103 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -107,8 +107,20 @@
>  *  Unused
>  */
> 
> +#ifdef CONFIG_ARM_32
> +
> #define COMMON_VIRT_START       _AT(vaddr_t, 0)
> 
> +#else
> +
> +#define SLOT0_ENTRY_BITS  39
> +#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
> +#define SLOT0_ENTRY_SIZE  SLOT0(1)
> +
> +#define COMMON_VIRT_START       SLOT(1)
> +
> +#endif
> +
> #define XEN_VIRT_START          (COMMON_VIRT_START + MB(2))
> #define XEN_SLOT_SIZE           MB(2)
> #define XEN_VIRT_END            (XEN_VIRT_START + XEN_SLOT_SIZE)
> @@ -161,14 +173,10 @@
> 
> #else /* ARM_64 */
> 
> -#define SLOT0_ENTRY_BITS  39
> -#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
> -#define SLOT0_ENTRY_SIZE  SLOT0(1)
> -
> -#define VMAP_VIRT_START  GB(1)
> +#define VMAP_VIRT_START  (SLOT0(1) + GB(1))
> #define VMAP_VIRT_END    (VMAP_VIRT_START + GB(1))
> 
> -#define FRAMETABLE_VIRT_START  GB(32)
> +#define FRAMETABLE_VIRT_START  (SLOT0(1) + GB(32))
> #define FRAMETABLE_SIZE        GB(32)
> #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
> #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 6b7c41d827ca..75ed9a3ce249 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -187,11 +187,10 @@ static void __init __maybe_unused build_assertions(void)
>     BUILD_BUG_ON(DIRECTMAP_VIRT_START & ~FIRST_MASK);
> #endif
>     /* Page table structure constraints */
> -#ifdef CONFIG_ARM_64
> -    BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START));
> -#endif
Don’t you want to enforce the opposite now ? Check that it is not on slot 0 ?

>     BUILD_BUG_ON(first_table_offset(XEN_VIRT_START));
> +#ifdef CONFIG_ARM_32
>     BUILD_BUG_ON(second_linear_offset(XEN_VIRT_START) >= XEN_PT_LPAE_ENTRIES);
> +#endif
> #ifdef CONFIG_DOMAIN_PAGE
>     BUILD_BUG_ON(DOMHEAP_VIRT_START & ~FIRST_MASK);
> #endif
> @@ -611,10 +610,11 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
>     phys_offset = boot_phys_offset;
> 
> #ifdef CONFIG_ARM_64
> -    p = (void *) xen_pgtable;
> -    p[0] = pte_of_xenaddr((uintptr_t)xen_first);
> -    p[0].pt.table = 1;
> -    p[0].pt.xn = 0;
> +    pte = pte_of_xenaddr((uintptr_t)xen_first);
> +    pte.pt.table = 1;
> +    pte.pt.xn = 0;
> +    xen_pgtable[zeroeth_table_offset(XEN_VIRT_START)] = pte;
> +
>     p = (void *) xen_first;
> #else
>     p = (void *) cpu0_pgtable;
> -- 
> 2.32.0
> 

Cheers
Bertrand

Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout

[Julien Grall]

On 25/03/2022 13:17, Bertrand Marquis wrote:
> Hi Julien,

Hi,

>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Xen is currently not fully compliant with the Arm because it will
> I think you wanted to say “arm arm” her.

Yes. I will update it.

> 
>> switch the TTBR with the MMU on.
>>
>> In order to be compliant, we need to disable the MMU before
>> switching the TTBR. The implication is the page-tables should
>> contain an identity mapping of the code switching the TTBR.
>>
>> If we don't rework the memory layout, we would need to find a
>> virtual address that matches a physical address and doesn't clash
>> with the static virtual regions. This can be a bit tricky.
> 
> This sentence is a bit misleading. Even with the rework you need
> to do that just by moving the Xen virtual address upper you make
> sure that anything physical memory under 512GB can be mapped
> 1:1 without clashing with other Xen mappings (unless Xen is loaded
> in memory at physical address 512GB which would end in the same issue).

So the key difference is with the rework, it is trivial to create the 
1:1 mapping as we know it doesn't clash. This is not the case without 
the rework.

> 
> I think should be rephrased.

I am not entirely sure how to rephrase it. Do you have a proposal?

> 
>>
>> On arm64, the memory layout  has plenty of unused space. In most of
>> the case we expect Xen to be loaded in low memory.
>>
>> The memory layout is reshuffled to keep the 0th slot free. Xen will now
> 
> 0th slot of first level of page table.

We want to keep the first 512GB free. So did you intend to write "zero 
level"?

> 
>> be loaded at (512GB + 2MB). This requires a slight tweak of the boot
>> code as XEN_VIRT_START cannot be used as an immediate.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ---
>>
>>     TODO:
>>         - I vaguely recall that one of the early platform we supported add
>>           the memory starting in high memory (> 1TB). I need to check
>>           whether the new layout will be fine.
> 
> I think we have some Juno with some memory like that, tell me if you need help here.

Would you be able to check the memory layout and confirm?

> 
>>         - Update the documentation to reflect the new layout
>> ---
>> xen/arch/arm/arm64/head.S         |  3 ++-
>> xen/arch/arm/include/asm/config.h | 20 ++++++++++++++------
>> xen/arch/arm/mm.c                 | 14 +++++++-------
>> 3 files changed, 23 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 66d862fc8137..878649280d73 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -594,7 +594,8 @@ create_page_tables:
>>           * need an additional 1:1 mapping, the virtual mapping will
>>           * suffice.
>>           */
>> -        cmp   x19, #XEN_VIRT_START
>> +        ldr   x0, =XEN_VIRT_START
>> +        cmp   x19, x0
> A comment in the code would be good here to prevent someone reverting this.

Anyone trying to revert the change will face a compilation error:

   CC      arch/arm/arm64/head.o
arch/arm/arm64/head.S: Assembler messages:
arch/arm/arm64/head.S:597: Error: immediate out of range

So I don't think a comment is necessary because this is not specific to 
a compiler/assembler.
>> -#define SLOT0_ENTRY_BITS  39
>> -#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
>> -#define SLOT0_ENTRY_SIZE  SLOT0(1)
>> -
>> -#define VMAP_VIRT_START  GB(1)
>> +#define VMAP_VIRT_START  (SLOT0(1) + GB(1))
>> #define VMAP_VIRT_END    (VMAP_VIRT_START + GB(1))
>>
>> -#define FRAMETABLE_VIRT_START  GB(32)
>> +#define FRAMETABLE_VIRT_START  (SLOT0(1) + GB(32))
>> #define FRAMETABLE_SIZE        GB(32)
>> #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
>> #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 6b7c41d827ca..75ed9a3ce249 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -187,11 +187,10 @@ static void __init __maybe_unused build_assertions(void)
>>      BUILD_BUG_ON(DIRECTMAP_VIRT_START & ~FIRST_MASK);
>> #endif
>>      /* Page table structure constraints */
>> -#ifdef CONFIG_ARM_64
>> -    BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START));
>> -#endif
> Don’t you want to enforce the opposite now ? Check that it is not on slot 0 ?

I can. But this is not going to guarantee us the first 512GB is going to 
be free.

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout

[Bertrand Marquis]

Hi Julien,

> On 25 Mar 2022, at 14:35, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 25/03/2022 13:17, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi,
> 
>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>> 
>>> From: Julien Grall <jgrall@amazon.com>
>>> 
>>> Xen is currently not fully compliant with the Arm because it will
>> I think you wanted to say “arm arm” her.
> 
> Yes. I will update it.
> 
>>> switch the TTBR with the MMU on.
>>> 
>>> In order to be compliant, we need to disable the MMU before
>>> switching the TTBR. The implication is the page-tables should
>>> contain an identity mapping of the code switching the TTBR.
>>> 
>>> If we don't rework the memory layout, we would need to find a
>>> virtual address that matches a physical address and doesn't clash
>>> with the static virtual regions. This can be a bit tricky.
>> This sentence is a bit misleading. Even with the rework you need
>> to do that just by moving the Xen virtual address upper you make
>> sure that anything physical memory under 512GB can be mapped
>> 1:1 without clashing with other Xen mappings (unless Xen is loaded
>> in memory at physical address 512GB which would end in the same issue).
> 
> So the key difference is with the rework, it is trivial to create the 1:1 mapping as we know it doesn't clash. This is not the case without the rework.

Agree

> 
>> I think should be rephrased.
> 
> I am not entirely sure how to rephrase it. Do you have a proposal?

Turn it into the positive:
Rework the memory layout to put Xen over 512GB. This makes it trivial to create
a 1:1 mapping, with the assumption that the physical memory is under 512GB.

Something in this area, telling what we do and not what we don't

> 
>>> 
>>> On arm64, the memory layout  has plenty of unused space. In most of
>>> the case we expect Xen to be loaded in low memory.
>>> 
>>> The memory layout is reshuffled to keep the 0th slot free. Xen will now
>> 0th slot of first level of page table.
> 
> We want to keep the first 512GB free. So did you intend to write "zero level"?

Yes sorry.

> 
>>> be loaded at (512GB + 2MB). This requires a slight tweak of the boot
>>> code as XEN_VIRT_START cannot be used as an immediate.
>>> 
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>> 
>>> ---
>>> 
>>>    TODO:
>>>        - I vaguely recall that one of the early platform we supported add
>>>          the memory starting in high memory (> 1TB). I need to check
>>>          whether the new layout will be fine.
>> I think we have some Juno with some memory like that, tell me if you need help here.
> 
> Would you be able to check the memory layout and confirm?

I checked and the Juno we have as the high memory a lot lower than that:
RAM: 0000000880000000 - 00000009ffffffff

No idea why it was a lot higher in my mind.

> 
>>>        - Update the documentation to reflect the new layout
>>> ---
>>> xen/arch/arm/arm64/head.S         |  3 ++-
>>> xen/arch/arm/include/asm/config.h | 20 ++++++++++++++------
>>> xen/arch/arm/mm.c                 | 14 +++++++-------
>>> 3 files changed, 23 insertions(+), 14 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>> index 66d862fc8137..878649280d73 100644
>>> --- a/xen/arch/arm/arm64/head.S
>>> +++ b/xen/arch/arm/arm64/head.S
>>> @@ -594,7 +594,8 @@ create_page_tables:
>>>          * need an additional 1:1 mapping, the virtual mapping will
>>>          * suffice.
>>>          */
>>> -        cmp   x19, #XEN_VIRT_START
>>> +        ldr   x0, =XEN_VIRT_START
>>> +        cmp   x19, x0
>> A comment in the code would be good here to prevent someone reverting this.
> 
> Anyone trying to revert the change will face a compilation error:
> 
>  CC      arch/arm/arm64/head.o
> arch/arm/arm64/head.S: Assembler messages:
> arch/arm/arm64/head.S:597: Error: immediate out of range
> 
> So I don't think a comment is necessary because this is not specific to a compiler/assembler.

Right I should have thought of the compilation error.


>>> -#define SLOT0_ENTRY_BITS  39
>>> -#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
>>> -#define SLOT0_ENTRY_SIZE  SLOT0(1)
>>> -
>>> -#define VMAP_VIRT_START  GB(1)
>>> +#define VMAP_VIRT_START  (SLOT0(1) + GB(1))
>>> #define VMAP_VIRT_END    (VMAP_VIRT_START + GB(1))
>>> 
>>> -#define FRAMETABLE_VIRT_START  GB(32)
>>> +#define FRAMETABLE_VIRT_START  (SLOT0(1) + GB(32))
>>> #define FRAMETABLE_SIZE        GB(32)
>>> #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
>>> #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index 6b7c41d827ca..75ed9a3ce249 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -187,11 +187,10 @@ static void __init __maybe_unused build_assertions(void)
>>>     BUILD_BUG_ON(DIRECTMAP_VIRT_START & ~FIRST_MASK);
>>> #endif
>>>     /* Page table structure constraints */
>>> -#ifdef CONFIG_ARM_64
>>> -    BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START));
>>> -#endif
>> Don’t you want to enforce the opposite now ? Check that it is not on slot 0 ?
> 
> I can. But this is not going to guarantee us the first 512GB is going to be free.

It could still make sure that XEN_VIRT_START is not in the slot 0.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout

[Julien Grall]

Hi,

On 25/03/2022 14:05, Bertrand Marquis wrote:
>> On 25 Mar 2022, at 14:35, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 25/03/2022 13:17, Bertrand Marquis wrote:
>>> Hi Julien,
>>
>> Hi,
>>
>>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Xen is currently not fully compliant with the Arm because it will
>>> I think you wanted to say “arm arm” her.
>>
>> Yes. I will update it.
>>
>>>> switch the TTBR with the MMU on.
>>>>
>>>> In order to be compliant, we need to disable the MMU before
>>>> switching the TTBR. The implication is the page-tables should
>>>> contain an identity mapping of the code switching the TTBR.
>>>>
>>>> If we don't rework the memory layout, we would need to find a
>>>> virtual address that matches a physical address and doesn't clash
>>>> with the static virtual regions. This can be a bit tricky.
>>> This sentence is a bit misleading. Even with the rework you need
>>> to do that just by moving the Xen virtual address upper you make
>>> sure that anything physical memory under 512GB can be mapped
>>> 1:1 without clashing with other Xen mappings (unless Xen is loaded
>>> in memory at physical address 512GB which would end in the same issue).
>>
>> So the key difference is with the rework, it is trivial to create the 1:1 mapping as we know it doesn't clash. This is not the case without the rework.
> 
> Agree
> 
>>
>>> I think should be rephrased.
>>
>> I am not entirely sure how to rephrase it. Do you have a proposal?
> 
> Turn it into the positive:
> Rework the memory layout to put Xen over 512GB. This makes it trivial to create
> a 1:1 mapping, with the assumption that the physical memory is under 512GB.

I will use this wording in the next version.

>>>> be loaded at (512GB + 2MB). This requires a slight tweak of the boot
>>>> code as XEN_VIRT_START cannot be used as an immediate.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>
>>>> ---
>>>>
>>>>     TODO:
>>>>         - I vaguely recall that one of the early platform we supported add
>>>>           the memory starting in high memory (> 1TB). I need to check
>>>>           whether the new layout will be fine.
>>> I think we have some Juno with some memory like that, tell me if you need help here.
>>
>> Would you be able to check the memory layout and confirm?
> 
> I checked and the Juno we have as the high memory a lot lower than that:
> RAM: 0000000880000000 - 00000009ffffffff
> 
> No idea why it was a lot higher in my mind.

I have only encountered one board with the memory over 512GB. I can't 
remember whether it is AMD Seattle or X-Gene.

> 
>>
>>>>         - Update the documentation to reflect the new layout
>>>> ---
>>>> xen/arch/arm/arm64/head.S         |  3 ++-
>>>> xen/arch/arm/include/asm/config.h | 20 ++++++++++++++------
>>>> xen/arch/arm/mm.c                 | 14 +++++++-------
>>>> 3 files changed, 23 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>>> index 66d862fc8137..878649280d73 100644
>>>> --- a/xen/arch/arm/arm64/head.S
>>>> +++ b/xen/arch/arm/arm64/head.S
>>>> @@ -594,7 +594,8 @@ create_page_tables:
>>>>           * need an additional 1:1 mapping, the virtual mapping will
>>>>           * suffice.
>>>>           */
>>>> -        cmp   x19, #XEN_VIRT_START
>>>> +        ldr   x0, =XEN_VIRT_START
>>>> +        cmp   x19, x0
>>> A comment in the code would be good here to prevent someone reverting this.
>>
>> Anyone trying to revert the change will face a compilation error:
>>
>>   CC      arch/arm/arm64/head.o
>> arch/arm/arm64/head.S: Assembler messages:
>> arch/arm/arm64/head.S:597: Error: immediate out of range
>>
>> So I don't think a comment is necessary because this is not specific to a compiler/assembler.
> 
> Right I should have thought of the compilation error.

TBH, I would have preferred to keep the single instruction. AFAICT, the 
immediate should be either between 0 - 4095. Or a number between 4096 
and 2^24 that is 4KB aligned.

So it would not suit us here.

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout

[Julien Grall]

On 25/03/2022 14:36, Julien Grall wrote:
>>>>> be loaded at (512GB + 2MB). This requires a slight tweak of the boot
>>>>> code as XEN_VIRT_START cannot be used as an immediate.
>>>>>
>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>>
>>>>> ---
>>>>>
>>>>>     TODO:
>>>>>         - I vaguely recall that one of the early platform we 
>>>>> supported add
>>>>>           the memory starting in high memory (> 1TB). I need to check
>>>>>           whether the new layout will be fine.
>>>> I think we have some Juno with some memory like that, tell me if you 
>>>> need help here.
>>>
>>> Would you be able to check the memory layout and confirm?
>>
>> I checked and the Juno we have as the high memory a lot lower than that:
>> RAM: 0000000880000000 - 00000009ffffffff
>>
>> No idea why it was a lot higher in my mind.
> 
> I have only encountered one board with the memory over 512GB. I can't 
> remember whether it is AMD Seattle or X-Gene.

So I found the answer. This was AMD Seattle where the memory start at 
512GB. So I will map Xen starting at 2TB (so there is a bit of slack).

Cheers,

-- 
Julien Grall

[PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping

[Julien Grall]

From: Julien GralL <jgrall@amazon.com>

In follow-up patches we will need to have part of Xen identity mapped in
order to safely switch the TTBR.

On some platform, the identity mapping may have to start at 0. If we always
keep the identity region mapped, NULL pointer ference would lead to access
to valid mapping.

It would be possible to relocate Xen to avoid clashing with address 0.
However the identity mapping is only meant to be used in very limited
places. Therefore it would be better to keep the identity region invalid
for most of the time.

Two new helpers are introduced:
    - prepare_identity_mapping() will setup the page-tables so it is
      easy to create the mapping afterwards.
    - update_identity_mapping() will create/remove the identity mapping

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/include/asm/mm.h |  2 +
 xen/arch/arm/mm.c             | 73 +++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 045a8ba4bb63..76973ea9a0ff 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -177,6 +177,8 @@ extern unsigned long total_pages;
 
 /* Boot-time pagetable setup */
 extern void setup_pagetables(unsigned long boot_phys_offset);
+/* Enable/disable the identity mapping */
+extern void update_identity_mapping(bool enable);
 /* Map FDT in boot pagetable */
 extern void *early_fdt_map(paddr_t fdt_paddr);
 /* Remove early mappings */
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 75ed9a3ce249..5c4dece16f7f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -138,6 +138,12 @@ static DEFINE_PAGE_TABLE(cpu0_pgtable);
 static DEFINE_PAGE_TABLES(cpu0_dommap, DOMHEAP_SECOND_PAGES);
 #endif
 
+#ifdef CONFIG_ARM_64
+static DEFINE_PAGE_TABLE(xen_first_id);
+static DEFINE_PAGE_TABLE(xen_second_id);
+static DEFINE_PAGE_TABLE(xen_third_id);
+#endif
+
 /* Common pagetable leaves */
 /* Second level page tables.
  *
@@ -573,6 +579,70 @@ void __init remove_early_mappings(void)
     BUG_ON(rc);
 }
 
+/*
+ * The identity mapping may start at physical address 0. So don't want
+ * to keep it mapped longer than necessary.
+ *
+ * When this is called, we are still using the boot_pgtable.
+ *
+ * XXX: Handle Arm32 properly.
+ */
+static void prepare_identity_mapping(void)
+{
+    paddr_t id_addr = virt_to_maddr(_start);
+    lpae_t pte;
+    DECLARE_OFFSETS(id_offsets, id_addr);
+
+    printk("id_addr 0x%lx\n", id_addr);
+#ifdef CONFIG_ARM_64
+    if ( id_offsets[0] != 0 )
+        panic("Cannot handled ID mapping above 512GB\n");
+#endif
+
+    /* Link first ID table */
+    pte = pte_of_xenaddr((vaddr_t)xen_first_id);
+    pte.pt.table = 1;
+    pte.pt.xn = 0;
+
+    write_pte(&boot_pgtable[id_offsets[0]], pte);
+
+    /* Link second ID table */
+    pte = pte_of_xenaddr((vaddr_t)xen_second_id);
+    pte.pt.table = 1;
+    pte.pt.xn = 0;
+
+    write_pte(&xen_first_id[id_offsets[1]], pte);
+
+    /* Link third ID table */
+    pte = pte_of_xenaddr((vaddr_t)xen_third_id);
+    pte.pt.table = 1;
+    pte.pt.xn = 0;
+
+    write_pte(&xen_second_id[id_offsets[2]], pte);
+
+    /* The mapping in the third table will be created at a later stage */
+
+    /*
+     * Link the identity mapping in the runtime Xen page tables. No need to
+     * use write_pte here as they are not live yet.
+     */
+    xen_pgtable[id_offsets[0]] = boot_pgtable[id_offsets[0]];
+}
+
+void update_identity_mapping(bool enable)
+{
+    paddr_t id_addr = virt_to_maddr(_start);
+    int rc;
+
+    if ( enable )
+        rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
+                              PAGE_HYPERVISOR_RX);
+    else
+        rc = destroy_xen_mappings(id_addr, id_addr + PAGE_SIZE);
+
+    BUG_ON(rc);
+}
+
 /*
  * After boot, Xen page-tables should not contain mapping that are both
  * Writable and eXecutables.
@@ -609,6 +679,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
 
     phys_offset = boot_phys_offset;
 
+    /* XXX: Find a better place to call it */
+    prepare_identity_mapping();
+
 #ifdef CONFIG_ARM_64
     pte = pte_of_xenaddr((uintptr_t)xen_first);
     pte.pt.table = 1;
-- 
2.32.0

Re: [PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping

[Bertrand Marquis]

Hi Julien,

> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien GralL <jgrall@amazon.com>
> 
> In follow-up patches we will need to have part of Xen identity mapped in
> order to safely switch the TTBR.
> 
> On some platform, the identity mapping may have to start at 0. If we always
> keep the identity region mapped, NULL pointer ference would lead to access
> to valid mapping.
> 
> It would be possible to relocate Xen to avoid clashing with address 0.
> However the identity mapping is only meant to be used in very limited
> places. Therefore it would be better to keep the identity region invalid
> for most of the time.
> 
> Two new helpers are introduced:
>    - prepare_identity_mapping() will setup the page-tables so it is
>      easy to create the mapping afterwards.
>    - update_identity_mapping() will create/remove the identity mapping

Nit: Would be better to first say what the patch is doing and then explaining
the NULL pointer possible issue.

> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/arm/include/asm/mm.h |  2 +
> xen/arch/arm/mm.c             | 73 +++++++++++++++++++++++++++++++++++
> 2 files changed, 75 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 045a8ba4bb63..76973ea9a0ff 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -177,6 +177,8 @@ extern unsigned long total_pages;
> 
> /* Boot-time pagetable setup */
> extern void setup_pagetables(unsigned long boot_phys_offset);
> +/* Enable/disable the identity mapping */
> +extern void update_identity_mapping(bool enable);
> /* Map FDT in boot pagetable */
> extern void *early_fdt_map(paddr_t fdt_paddr);
> /* Remove early mappings */
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 75ed9a3ce249..5c4dece16f7f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -138,6 +138,12 @@ static DEFINE_PAGE_TABLE(cpu0_pgtable);
> static DEFINE_PAGE_TABLES(cpu0_dommap, DOMHEAP_SECOND_PAGES);
> #endif
> 
> +#ifdef CONFIG_ARM_64
> +static DEFINE_PAGE_TABLE(xen_first_id);
> +static DEFINE_PAGE_TABLE(xen_second_id);
> +static DEFINE_PAGE_TABLE(xen_third_id);
> +#endif
> +
> /* Common pagetable leaves */
> /* Second level page tables.
>  *
> @@ -573,6 +579,70 @@ void __init remove_early_mappings(void)
>     BUG_ON(rc);
> }
> 
> +/*
> + * The identity mapping may start at physical address 0. So don't want
> + * to keep it mapped longer than necessary.
> + *
> + * When this is called, we are still using the boot_pgtable.
> + *
> + * XXX: Handle Arm32 properly.
> + */
> +static void prepare_identity_mapping(void)
> +{
> +    paddr_t id_addr = virt_to_maddr(_start);
> +    lpae_t pte;
> +    DECLARE_OFFSETS(id_offsets, id_addr);
> +
> +    printk("id_addr 0x%lx\n", id_addr);

Debug print that should be removed.

> +#ifdef CONFIG_ARM_64
> +    if ( id_offsets[0] != 0 )
> +        panic("Cannot handled ID mapping above 512GB\n");

The error message here might not be really helpful for the user.
How about saying that Xen cannot be loaded in memory with
a physical address above 512GB ?

> +#endif
> +
> +    /* Link first ID table */
> +    pte = pte_of_xenaddr((vaddr_t)xen_first_id);
> +    pte.pt.table = 1;
> +    pte.pt.xn = 0;
> +
> +    write_pte(&boot_pgtable[id_offsets[0]], pte);
> +
> +    /* Link second ID table */
> +    pte = pte_of_xenaddr((vaddr_t)xen_second_id);
> +    pte.pt.table = 1;
> +    pte.pt.xn = 0;
> +
> +    write_pte(&xen_first_id[id_offsets[1]], pte);
> +
> +    /* Link third ID table */
> +    pte = pte_of_xenaddr((vaddr_t)xen_third_id);
> +    pte.pt.table = 1;
> +    pte.pt.xn = 0;
> +
> +    write_pte(&xen_second_id[id_offsets[2]], pte);
> +
> +    /* The mapping in the third table will be created at a later stage */
> +
> +    /*
> +     * Link the identity mapping in the runtime Xen page tables. No need to
> +     * use write_pte here as they are not live yet.
> +     */
> +    xen_pgtable[id_offsets[0]] = boot_pgtable[id_offsets[0]];
> +}
> +
> +void update_identity_mapping(bool enable)

You probably want an __init attribute here.

> +{
> +    paddr_t id_addr = virt_to_maddr(_start);
> +    int rc;
> +
> +    if ( enable )
> +        rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
> +                              PAGE_HYPERVISOR_RX);
> +    else
> +        rc = destroy_xen_mappings(id_addr, id_addr + PAGE_SIZE);
> +
> +    BUG_ON(rc);
> +}
> +
> /*
>  * After boot, Xen page-tables should not contain mapping that are both
>  * Writable and eXecutables.
> @@ -609,6 +679,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
> 
>     phys_offset = boot_phys_offset;
> 
> +    /* XXX: Find a better place to call it */

Why do you think this place is not right ?

> +    prepare_identity_mapping();
> +
> #ifdef CONFIG_ARM_64
>     pte = pte_of_xenaddr((uintptr_t)xen_first);
>     pte.pt.table = 1;
> -- 
> 2.32.0

Cheers
Bertrand

Re: [PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping

[Julien Grall]

On 25/03/2022 13:32, Bertrand Marquis wrote:
> Hi Julien,

Hi,

>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien GralL <jgrall@amazon.com>
>>
>> In follow-up patches we will need to have part of Xen identity mapped in
>> order to safely switch the TTBR.
>>
>> On some platform, the identity mapping may have to start at 0. If we always
>> keep the identity region mapped, NULL pointer ference would lead to access
>> to valid mapping.
>>
>> It would be possible to relocate Xen to avoid clashing with address 0.
>> However the identity mapping is only meant to be used in very limited
>> places. Therefore it would be better to keep the identity region invalid
>> for most of the time.
>>
>> Two new helpers are introduced:
>>     - prepare_identity_mapping() will setup the page-tables so it is
>>       easy to create the mapping afterwards.
>>     - update_identity_mapping() will create/remove the identity mapping
> 
> Nit: Would be better to first say what the patch is doing and then explaining
> the NULL pointer possible issue.
The NULL pointer is part of the problem statement. IOW, I would not have 
introduced update_identity_mapping() if we were not concerned that the 
mapping start 0.

So I don't think the commit message would read the same.

>> +/*
>> + * The identity mapping may start at physical address 0. So don't want
>> + * to keep it mapped longer than necessary.
>> + *
>> + * When this is called, we are still using the boot_pgtable.
>> + *
>> + * XXX: Handle Arm32 properly.
>> + */
>> +static void prepare_identity_mapping(void)
>> +{
>> +    paddr_t id_addr = virt_to_maddr(_start);
>> +    lpae_t pte;
>> +    DECLARE_OFFSETS(id_offsets, id_addr);
>> +
>> +    printk("id_addr 0x%lx\n", id_addr);
> 
> Debug print that should be removed.

Will do. Note the "early-RFC" in the comment. I am not looking for a 
detailed review (I didn't spend too much time cleaning up) but a 
feedback on the approach.

> 
>> +#ifdef CONFIG_ARM_64
>> +    if ( id_offsets[0] != 0 )
>> +        panic("Cannot handled ID mapping above 512GB\n");
> 
> The error message here might not be really helpful for the user.
> How about saying that Xen cannot be loaded in memory with
> a physical address above 512GB ?

Sure.

> 
>> +#endif
>> +
>> +    /* Link first ID table */
>> +    pte = pte_of_xenaddr((vaddr_t)xen_first_id);
>> +    pte.pt.table = 1;
>> +    pte.pt.xn = 0;
>> +
>> +    write_pte(&boot_pgtable[id_offsets[0]], pte);
>> +
>> +    /* Link second ID table */
>> +    pte = pte_of_xenaddr((vaddr_t)xen_second_id);
>> +    pte.pt.table = 1;
>> +    pte.pt.xn = 0;
>> +
>> +    write_pte(&xen_first_id[id_offsets[1]], pte);
>> +
>> +    /* Link third ID table */
>> +    pte = pte_of_xenaddr((vaddr_t)xen_third_id);
>> +    pte.pt.table = 1;
>> +    pte.pt.xn = 0;
>> +
>> +    write_pte(&xen_second_id[id_offsets[2]], pte);
>> +
>> +    /* The mapping in the third table will be created at a later stage */
>> +
>> +    /*
>> +     * Link the identity mapping in the runtime Xen page tables. No need to
>> +     * use write_pte here as they are not live yet.
>> +     */
>> +    xen_pgtable[id_offsets[0]] = boot_pgtable[id_offsets[0]];
>> +}
>> +
>> +void update_identity_mapping(bool enable)
> 
> You probably want an __init attribute here.
I expect this helper to be necessary after boot (e.g. CPU bring-up, 
suspend/resume). So I decided to keep it without _init.

> 
>> +{
>> +    paddr_t id_addr = virt_to_maddr(_start);
>> +    int rc;
>> +
>> +    if ( enable )
>> +        rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
>> +                              PAGE_HYPERVISOR_RX);
>> +    else
>> +        rc = destroy_xen_mappings(id_addr, id_addr + PAGE_SIZE);
>> +
>> +    BUG_ON(rc);
>> +}
>> +
>> /*
>>   * After boot, Xen page-tables should not contain mapping that are both
>>   * Writable and eXecutables.
>> @@ -609,6 +679,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
>>
>>      phys_offset = boot_phys_offset;
>>
>> +    /* XXX: Find a better place to call it */
> 
> Why do you think this place is not right ?
Because the use in setup_pagetables() will soon disappear (my plan it to 
completely remove setup_pagetables) and this will used in other 
subsystem (CPU bring-up, suspend/resume).

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping

[Bertrand Marquis]

Hi Julien

> On 25 Mar 2022, at 14:48, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 25/03/2022 13:32, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi,
> 
>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>> 
>>> From: Julien GralL <jgrall@amazon.com>
>>> 
>>> In follow-up patches we will need to have part of Xen identity mapped in
>>> order to safely switch the TTBR.
>>> 
>>> On some platform, the identity mapping may have to start at 0. If we always
>>> keep the identity region mapped, NULL pointer ference would lead to access
>>> to valid mapping.
>>> 
>>> It would be possible to relocate Xen to avoid clashing with address 0.
>>> However the identity mapping is only meant to be used in very limited
>>> places. Therefore it would be better to keep the identity region invalid
>>> for most of the time.
>>> 
>>> Two new helpers are introduced:
>>>    - prepare_identity_mapping() will setup the page-tables so it is
>>>      easy to create the mapping afterwards.
>>>    - update_identity_mapping() will create/remove the identity mapping
>> Nit: Would be better to first say what the patch is doing and then explaining
>> the NULL pointer possible issue.
> The NULL pointer is part of the problem statement. IOW, I would not have introduced update_identity_mapping() if we were not concerned that the mapping start 0.
> 
> So I don't think the commit message would read the same.

Somehow reading your commit message, the link between the first 2 paragraph and the helpers introduced is not quite clear.

The NULL pointer problem is a consequence of the usage of an identity mapping and maybe this explanation should be more in documentation or in a code comment around this functionality.

> 
>>> +/*
>>> + * The identity mapping may start at physical address 0. So don't want
>>> + * to keep it mapped longer than necessary.
>>> + *
>>> + * When this is called, we are still using the boot_pgtable.
>>> + *
>>> + * XXX: Handle Arm32 properly.
>>> + */
>>> +static void prepare_identity_mapping(void)
>>> +{
>>> +    paddr_t id_addr = virt_to_maddr(_start);
>>> +    lpae_t pte;
>>> +    DECLARE_OFFSETS(id_offsets, id_addr);
>>> +
>>> +    printk("id_addr 0x%lx\n", id_addr);
>> Debug print that should be removed.
> 
> Will do. Note the "early-RFC" in the comment. I am not looking for a detailed review (I didn't spend too much time cleaning up) but a feedback on the approach.

I did read the code and it is easy to forget to remove those, so I just pointed it :-)

> 
>>> +#ifdef CONFIG_ARM_64
>>> +    if ( id_offsets[0] != 0 )
>>> +        panic("Cannot handled ID mapping above 512GB\n");
>> The error message here might not be really helpful for the user.
>> How about saying that Xen cannot be loaded in memory with
>> a physical address above 512GB ?
> 
> Sure.
> 
>>> +#endif
>>> +
>>> +    /* Link first ID table */
>>> +    pte = pte_of_xenaddr((vaddr_t)xen_first_id);
>>> +    pte.pt.table = 1;
>>> +    pte.pt.xn = 0;
>>> +
>>> +    write_pte(&boot_pgtable[id_offsets[0]], pte);
>>> +
>>> +    /* Link second ID table */
>>> +    pte = pte_of_xenaddr((vaddr_t)xen_second_id);
>>> +    pte.pt.table = 1;
>>> +    pte.pt.xn = 0;
>>> +
>>> +    write_pte(&xen_first_id[id_offsets[1]], pte);
>>> +
>>> +    /* Link third ID table */
>>> +    pte = pte_of_xenaddr((vaddr_t)xen_third_id);
>>> +    pte.pt.table = 1;
>>> +    pte.pt.xn = 0;
>>> +
>>> +    write_pte(&xen_second_id[id_offsets[2]], pte);
>>> +
>>> +    /* The mapping in the third table will be created at a later stage */
>>> +
>>> +    /*
>>> +     * Link the identity mapping in the runtime Xen page tables. No need to
>>> +     * use write_pte here as they are not live yet.
>>> +     */
>>> +    xen_pgtable[id_offsets[0]] = boot_pgtable[id_offsets[0]];
>>> +}
>>> +
>>> +void update_identity_mapping(bool enable)
>> You probably want an __init attribute here.
> I expect this helper to be necessary after boot (e.g. CPU bring-up, suspend/resume). So I decided to keep it without _init.

Ok

> 
>>> +{
>>> +    paddr_t id_addr = virt_to_maddr(_start);
>>> +    int rc;
>>> +
>>> +    if ( enable )
>>> +        rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
>>> +                              PAGE_HYPERVISOR_RX);
>>> +    else
>>> +        rc = destroy_xen_mappings(id_addr, id_addr + PAGE_SIZE);
>>> +
>>> +    BUG_ON(rc);
>>> +}
>>> +
>>> /*
>>>  * After boot, Xen page-tables should not contain mapping that are both
>>>  * Writable and eXecutables.
>>> @@ -609,6 +679,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
>>> 
>>>     phys_offset = boot_phys_offset;
>>> 
>>> +    /* XXX: Find a better place to call it */
>> Why do you think this place is not right ?
> Because the use in setup_pagetables() will soon disappear (my plan it to completely remove setup_pagetables) and this will used in other subsystem (CPU bring-up, suspend/resume).

Ok

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

[PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
still on.

Switching TTBR is like replacing existing mappings with new ones. So
we need to follow the break-before-make sequence.

In this case, it means the MMU needs to be switched off while the
TTBR is updated. In order to disable the MMU, we need to first
jump to an identity mapping.

Rename switch_ttbr() to switch_ttbr_id() and create an helper on
top to temporary map the identity mapping and call switch_ttbr()
via the identity address.

switch_ttbr_id() is now reworked to temporarily turn off the MMU
before updating the TTBR.

We also need to make sure the helper switch_ttbr() is part of the
identity mapping. So move _end_boot past it.

Take the opportunity to instruction cache flush as the operation is
only necessary when the memory is updated.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

    TODO:
        * Rename _end_boot to _end_id_mapping or similar
        * Check the memory barriers
        * I suspect the instruction cache flush will be necessary
          for cache coloring.
---
 xen/arch/arm/arm64/head.S | 31 ++++++++++++++++++++-----------
 xen/arch/arm/mm.c         | 14 +++++++++++++-
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 878649280d73..c5cc72b8fe6f 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -803,36 +803,45 @@ fail:   PRINT("- Boot failed -\r\n")
         b     1b
 ENDPROC(fail)
 
-GLOBAL(_end_boot)
-
 /*
  * Switch TTBR
  *
  * x0    ttbr
  *
- * TODO: This code does not comply with break-before-make.
+ * XXX: Check the barriers
  */
-ENTRY(switch_ttbr)
+ENTRY(switch_ttbr_id)
         dsb   sy                     /* Ensure the flushes happen before
                                       * continuing */
         isb                          /* Ensure synchronization with previous
                                       * changes to text */
+
+        /* Turn off MMU */
+        mrs    x1, SCTLR_EL2
+        bic    x1, x1, #SCTLR_Axx_ELx_M
+        msr    SCTLR_EL2, x1
+        dsb    sy
+        isb
+
         tlbi   alle2                 /* Flush hypervisor TLB */
-        ic     iallu                 /* Flush I-cache */
         dsb    sy                    /* Ensure completion of TLB flush */
         isb
 
-        msr    TTBR0_EL2, x0
+        msr   TTBR0_EL2, x0
+
+        mrs   x1, SCTLR_EL2
+        orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
+        msr   SCTLR_EL2, x1
 
         isb                          /* Ensure synchronization with previous
                                       * changes to text */
-        tlbi   alle2                 /* Flush hypervisor TLB */
-        ic     iallu                 /* Flush I-cache */
-        dsb    sy                    /* Ensure completion of TLB flush */
-        isb
+        /* Turn on the MMU */
+
 
         ret
-ENDPROC(switch_ttbr)
+ENDPROC(switch_ttbr_id)
+
+GLOBAL(_end_boot)
 
 #ifdef CONFIG_EARLY_PRINTK
 /*
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 5c4dece16f7f..a53760af7af0 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -660,7 +660,19 @@ static void xen_pt_enforce_wnx(void)
     flush_xen_tlb_local();
 }
 
-extern void switch_ttbr(uint64_t ttbr);
+extern void switch_ttbr_id(uint64_t ttbr);
+
+typedef void (switch_ttbr_fn)(uint64_t ttbr);
+
+static void switch_ttbr(uint64_t ttbr)
+{
+    vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
+    switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
+
+    update_identity_mapping(true);
+    fn(ttbr);
+    update_identity_mapping(false);
+}
 
 /* Clear a translation table and clean & invalidate the cache */
 static void clear_table(void *table)
-- 
2.32.0

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Stefano Stabellini]

On Wed, 9 Mar 2022, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
> still on.
> 
> Switching TTBR is like replacing existing mappings with new ones. So
> we need to follow the break-before-make sequence.
> 
> In this case, it means the MMU needs to be switched off while the
> TTBR is updated. In order to disable the MMU, we need to first
> jump to an identity mapping.
> 
> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
> top to temporary map the identity mapping and call switch_ttbr()
> via the identity address.
> 
> switch_ttbr_id() is now reworked to temporarily turn off the MMU
> before updating the TTBR.
> 
> We also need to make sure the helper switch_ttbr() is part of the
> identity mapping. So move _end_boot past it.
> 
> Take the opportunity to instruction cache flush as the operation is
> only necessary when the memory is updated.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
>     TODO:
>         * Rename _end_boot to _end_id_mapping or similar
>         * Check the memory barriers
>         * I suspect the instruction cache flush will be necessary
>           for cache coloring.
> ---
>  xen/arch/arm/arm64/head.S | 31 ++++++++++++++++++++-----------
>  xen/arch/arm/mm.c         | 14 +++++++++++++-
>  2 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 878649280d73..c5cc72b8fe6f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -803,36 +803,45 @@ fail:   PRINT("- Boot failed -\r\n")
>          b     1b
>  ENDPROC(fail)
>  
> -GLOBAL(_end_boot)
> -
>  /*
>   * Switch TTBR
>   *
>   * x0    ttbr
>   *
> - * TODO: This code does not comply with break-before-make.
> + * XXX: Check the barriers
>   */
> -ENTRY(switch_ttbr)
> +ENTRY(switch_ttbr_id)
>          dsb   sy                     /* Ensure the flushes happen before
>                                        * continuing */
>          isb                          /* Ensure synchronization with previous
>                                        * changes to text */
> +
> +        /* Turn off MMU */
> +        mrs    x1, SCTLR_EL2
> +        bic    x1, x1, #SCTLR_Axx_ELx_M
> +        msr    SCTLR_EL2, x1
> +        dsb    sy
> +        isb
> +
>          tlbi   alle2                 /* Flush hypervisor TLB */
> -        ic     iallu                 /* Flush I-cache */
>          dsb    sy                    /* Ensure completion of TLB flush */
>          isb
>  
> -        msr    TTBR0_EL2, x0
> +        msr   TTBR0_EL2, x0
> +
> +        mrs   x1, SCTLR_EL2
> +        orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
> +        msr   SCTLR_EL2, x1
>  
>          isb                          /* Ensure synchronization with previous
>                                        * changes to text */
> -        tlbi   alle2                 /* Flush hypervisor TLB */
> -        ic     iallu                 /* Flush I-cache */
> -        dsb    sy                    /* Ensure completion of TLB flush */
> -        isb
> +        /* Turn on the MMU */
> +
>  
>          ret
> -ENDPROC(switch_ttbr)
> +ENDPROC(switch_ttbr_id)
> +
> +GLOBAL(_end_boot)
>  
>  #ifdef CONFIG_EARLY_PRINTK
>  /*
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 5c4dece16f7f..a53760af7af0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -660,7 +660,19 @@ static void xen_pt_enforce_wnx(void)
>      flush_xen_tlb_local();
>  }
>  
> -extern void switch_ttbr(uint64_t ttbr);
> +extern void switch_ttbr_id(uint64_t ttbr);
> +
> +typedef void (switch_ttbr_fn)(uint64_t ttbr);
> +
> +static void switch_ttbr(uint64_t ttbr)
> +{
> +    vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
> +    switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
> +
> +    update_identity_mapping(true);
> +    fn(ttbr);
> +    update_identity_mapping(false);
> +}

As far as I can tell this should work for coloring too. In the case of
coloring:

    /* running on the old mapping, same as non-coloring */
    update_identity_mapping(true);

    /* jumping to the 1:1 mapping of the old Xen and switching to the
     * new pagetable */
    fn(ttbr);

    /* new pagetable is enabled, now we are back to addresses greater
     * than XEN_VIRT_START, which correspond to new cache-colored Xen */
    update_identity_mapping(false);


The only doubt that I have is: are we sure than a single page of 1:1
mapping is enough? What if:

virt_to_maddr(switch_ttbr_id) - virt_to_maddr(_start) > PAGE_SIZE


We might have to do a 1:1 mapping of size = _end-_start. It would work
with coloring too because we are doing a 1:1 mapping of the old copy of
Xen which is non-colored and contiguous (not the new copy which is
colored and fragmented).


Thanks Julien very much for your help!

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Julien Grall]

Hi Stefano,

On 12/03/2022 01:17, Stefano Stabellini wrote:
> On Wed, 9 Mar 2022, Julien Grall wrote:
> As far as I can tell this should work for coloring too. In the case of
> coloring:
> 
>      /* running on the old mapping, same as non-coloring */
>      update_identity_mapping(true);
> 
>      /* jumping to the 1:1 mapping of the old Xen and switching to the
>       * new pagetable */
>      fn(ttbr);
> 
>      /* new pagetable is enabled, now we are back to addresses greater
>       * than XEN_VIRT_START, which correspond to new cache-colored Xen */
>      update_identity_mapping(false);
> 
> 
> The only doubt that I have is: are we sure than a single page of 1:1
> mapping is enough? What if:
> 
> virt_to_maddr(switch_ttbr_id) - virt_to_maddr(_start) > PAGE_SIZE

switch_ttbr_id() is placed before _end_boot (this needs to be renamed). 
We are veryfing a link time (see the check in xen.lds.S) that _end_boot 
- _start is always smaller than 4KB.

At the moment, the size is less than 2KB. So we have plenty of space to 
grow. Also, there are some code that is technically not used while using 
the ID map. So if necessary we can shrink the size to always fit in a 
PAGE_SIZE.

> We might have to do a 1:1 mapping of size = _end-_start. It would work
> with coloring too because we are doing a 1:1 mapping of the old copy of
> Xen which is non-colored and contiguous (not the new copy which is
> colored and fragmented).

I would like to keep the size of the ID mapping to the strict minimum. A 
PAGE_SIZE should be sufficient for most of what we need in Xen.

This would help to get rid of the old copy of Xen in case of the cache 
coloring. Otherwise, you technically have to keep it forever if you plan 
to support suspend/resume or even allow CPU hotplug.

Furthemore, if you keep the two copy around, it is more difficult to 
know which mapping is used and we increase the risk to use the wrong 
one. For instance, the current implementation of cache coloring is 
clearing the wrong set of boot pagetables.

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Stefano Stabellini]

On Sat, 12 Mar 2022, Julien Grall wrote:
> On 12/03/2022 01:17, Stefano Stabellini wrote:
> > On Wed, 9 Mar 2022, Julien Grall wrote:
> > As far as I can tell this should work for coloring too. In the case of
> > coloring:
> > 
> >      /* running on the old mapping, same as non-coloring */
> >      update_identity_mapping(true);
> > 
> >      /* jumping to the 1:1 mapping of the old Xen and switching to the
> >       * new pagetable */
> >      fn(ttbr);
> > 
> >      /* new pagetable is enabled, now we are back to addresses greater
> >       * than XEN_VIRT_START, which correspond to new cache-colored Xen */
> >      update_identity_mapping(false);
> > 
> > 
> > The only doubt that I have is: are we sure than a single page of 1:1
> > mapping is enough? What if:
> > 
> > virt_to_maddr(switch_ttbr_id) - virt_to_maddr(_start) > PAGE_SIZE
> 
> switch_ttbr_id() is placed before _end_boot (this needs to be renamed). We are
> veryfing a link time (see the check in xen.lds.S) that _end_boot - _start is
> always smaller than 4KB.

Yes I see:
ASSERT( _end_boot - start <= PAGE_SIZE, "Boot code is larger than 4K")

Excellent!


> At the moment, the size is less than 2KB. So we have plenty of space to grow.
> Also, there are some code that is technically not used while using the ID map.
> So if necessary we can shrink the size to always fit in a PAGE_SIZE.

+1


> > We might have to do a 1:1 mapping of size = _end-_start. It would work
> > with coloring too because we are doing a 1:1 mapping of the old copy of
> > Xen which is non-colored and contiguous (not the new copy which is
> > colored and fragmented).
> 
> I would like to keep the size of the ID mapping to the strict minimum. A
> PAGE_SIZE should be sufficient for most of what we need in Xen.

Yep


> This would help to get rid of the old copy of Xen in case of the cache
> coloring. Otherwise, you technically have to keep it forever if you plan to
> support suspend/resume or even allow CPU hotplug.
> 
> Furthemore, if you keep the two copy around, it is more difficult to know
> which mapping is used and we increase the risk to use the wrong one. For
> instance, the current implementation of cache coloring is clearing the wrong
> set of boot pagetables.
 
Right. Given that we know it is a single page, then we could use a 1:1
of the colored copy without issues.

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Stefano Stabellini]

On Wed, 9 Mar 2022, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
> still on.
> 
> Switching TTBR is like replacing existing mappings with new ones. So
> we need to follow the break-before-make sequence.
> 
> In this case, it means the MMU needs to be switched off while the
> TTBR is updated. In order to disable the MMU, we need to first
> jump to an identity mapping.
> 
> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
> top to temporary map the identity mapping and call switch_ttbr()
> via the identity address.
> 
> switch_ttbr_id() is now reworked to temporarily turn off the MMU
> before updating the TTBR.
> 
> We also need to make sure the helper switch_ttbr() is part of the
> identity mapping. So move _end_boot past it.
> 
> Take the opportunity to instruction cache flush as the operation is
> only necessary when the memory is updated.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
>     TODO:
>         * Rename _end_boot to _end_id_mapping or similar
>         * Check the memory barriers
>         * I suspect the instruction cache flush will be necessary
>           for cache coloring.
> ---
>  xen/arch/arm/arm64/head.S | 31 ++++++++++++++++++++-----------
>  xen/arch/arm/mm.c         | 14 +++++++++++++-
>  2 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 878649280d73..c5cc72b8fe6f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -803,36 +803,45 @@ fail:   PRINT("- Boot failed -\r\n")
>          b     1b
>  ENDPROC(fail)
>  
> -GLOBAL(_end_boot)
> -
>  /*
>   * Switch TTBR
>   *
>   * x0    ttbr
>   *
> - * TODO: This code does not comply with break-before-make.
> + * XXX: Check the barriers
>   */
> -ENTRY(switch_ttbr)
> +ENTRY(switch_ttbr_id)
>          dsb   sy                     /* Ensure the flushes happen before
>                                        * continuing */
>          isb                          /* Ensure synchronization with previous
>                                        * changes to text */
> +
> +        /* Turn off MMU */
> +        mrs    x1, SCTLR_EL2
> +        bic    x1, x1, #SCTLR_Axx_ELx_M
> +        msr    SCTLR_EL2, x1
> +        dsb    sy
> +        isb
> +
>          tlbi   alle2                 /* Flush hypervisor TLB */
> -        ic     iallu                 /* Flush I-cache */
>          dsb    sy                    /* Ensure completion of TLB flush */
>          isb
>  
> -        msr    TTBR0_EL2, x0
> +        msr   TTBR0_EL2, x0
> +
> +        mrs   x1, SCTLR_EL2
> +        orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
> +        msr   SCTLR_EL2, x1
>  
>          isb                          /* Ensure synchronization with previous
>                                        * changes to text */
> -        tlbi   alle2                 /* Flush hypervisor TLB */
> -        ic     iallu                 /* Flush I-cache */
> -        dsb    sy                    /* Ensure completion of TLB flush */
> -        isb
> +        /* Turn on the MMU */
> +
>  
>          ret
> -ENDPROC(switch_ttbr)
> +ENDPROC(switch_ttbr_id)
> +
> +GLOBAL(_end_boot)
>  
>  #ifdef CONFIG_EARLY_PRINTK
>  /*
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 5c4dece16f7f..a53760af7af0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -660,7 +660,19 @@ static void xen_pt_enforce_wnx(void)
>      flush_xen_tlb_local();
>  }
>  
> -extern void switch_ttbr(uint64_t ttbr);
> +extern void switch_ttbr_id(uint64_t ttbr);
> +
> +typedef void (switch_ttbr_fn)(uint64_t ttbr);
> +
> +static void switch_ttbr(uint64_t ttbr)
> +{
> +    vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
> +    switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
> +
> +    update_identity_mapping(true);
> +    fn(ttbr);
> +    update_identity_mapping(false);
> +}

Controversial question: does it really matter that XEN_VIRT_START >
512GB and that _start < 512GB?

I am totally fine with the limit, I am just brainstorming: given that
the mapping is used very temporarely, it wouldn't really be an issue if
it conflicts with something important. Let's say that it conflicts with
the VMAP or the FRAMETABLE. As long as:

- we save the current mapping
- update it with the Xen 1:1
- switch_ttbr
- remove Xen 1:1
- restore mapping

It should work, right? Basically, a mapping conflict shouldn't be an
issue given that the mapping has only to live long enough to call
switch_ttbr_id.

I am less sure about patch #5 but it doesn't seem it would be a problem
there either.

That said, I am totally fine with _start < 512GB.

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Julien Grall]

On 12/03/2022 01:31, Stefano Stabellini wrote:
> On Wed, 9 Mar 2022, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
>> still on.
>>
>> Switching TTBR is like replacing existing mappings with new ones. So
>> we need to follow the break-before-make sequence.
>>
>> In this case, it means the MMU needs to be switched off while the
>> TTBR is updated. In order to disable the MMU, we need to first
>> jump to an identity mapping.
>>
>> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
>> top to temporary map the identity mapping and call switch_ttbr()
>> via the identity address.
>>
>> switch_ttbr_id() is now reworked to temporarily turn off the MMU
>> before updating the TTBR.
>>
>> We also need to make sure the helper switch_ttbr() is part of the
>> identity mapping. So move _end_boot past it.
>>
>> Take the opportunity to instruction cache flush as the operation is
>> only necessary when the memory is updated.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ---
>>
>>      TODO:
>>          * Rename _end_boot to _end_id_mapping or similar
>>          * Check the memory barriers
>>          * I suspect the instruction cache flush will be necessary
>>            for cache coloring.
>> ---
>>   xen/arch/arm/arm64/head.S | 31 ++++++++++++++++++++-----------
>>   xen/arch/arm/mm.c         | 14 +++++++++++++-
>>   2 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 878649280d73..c5cc72b8fe6f 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -803,36 +803,45 @@ fail:   PRINT("- Boot failed -\r\n")
>>           b     1b
>>   ENDPROC(fail)
>>   
>> -GLOBAL(_end_boot)
>> -
>>   /*
>>    * Switch TTBR
>>    *
>>    * x0    ttbr
>>    *
>> - * TODO: This code does not comply with break-before-make.
>> + * XXX: Check the barriers
>>    */
>> -ENTRY(switch_ttbr)
>> +ENTRY(switch_ttbr_id)
>>           dsb   sy                     /* Ensure the flushes happen before
>>                                         * continuing */
>>           isb                          /* Ensure synchronization with previous
>>                                         * changes to text */
>> +
>> +        /* Turn off MMU */
>> +        mrs    x1, SCTLR_EL2
>> +        bic    x1, x1, #SCTLR_Axx_ELx_M
>> +        msr    SCTLR_EL2, x1
>> +        dsb    sy
>> +        isb
>> +
>>           tlbi   alle2                 /* Flush hypervisor TLB */
>> -        ic     iallu                 /* Flush I-cache */
>>           dsb    sy                    /* Ensure completion of TLB flush */
>>           isb
>>   
>> -        msr    TTBR0_EL2, x0
>> +        msr   TTBR0_EL2, x0
>> +
>> +        mrs   x1, SCTLR_EL2
>> +        orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
>> +        msr   SCTLR_EL2, x1
>>   
>>           isb                          /* Ensure synchronization with previous
>>                                         * changes to text */
>> -        tlbi   alle2                 /* Flush hypervisor TLB */
>> -        ic     iallu                 /* Flush I-cache */
>> -        dsb    sy                    /* Ensure completion of TLB flush */
>> -        isb
>> +        /* Turn on the MMU */
>> +
>>   
>>           ret
>> -ENDPROC(switch_ttbr)
>> +ENDPROC(switch_ttbr_id)
>> +
>> +GLOBAL(_end_boot)
>>   
>>   #ifdef CONFIG_EARLY_PRINTK
>>   /*
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 5c4dece16f7f..a53760af7af0 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -660,7 +660,19 @@ static void xen_pt_enforce_wnx(void)
>>       flush_xen_tlb_local();
>>   }
>>   
>> -extern void switch_ttbr(uint64_t ttbr);
>> +extern void switch_ttbr_id(uint64_t ttbr);
>> +
>> +typedef void (switch_ttbr_fn)(uint64_t ttbr);
>> +
>> +static void switch_ttbr(uint64_t ttbr)
>> +{
>> +    vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
>> +    switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
>> +
>> +    update_identity_mapping(true);
>> +    fn(ttbr);
>> +    update_identity_mapping(false);
>> +}
> 
> Controversial question: does it really matter that XEN_VIRT_START >
> 512GB and that _start < 512GB?
> 
> I am totally fine with the limit, I am just brainstorming: given that
> the mapping is used very temporarely, it wouldn't really be an issue if
> it conflicts with something important. Let's say that it conflicts with
> the VMAP or the FRAMETABLE. As long as:
> 
> - we save the current mapping
> - update it with the Xen 1:1
> - switch_ttbr
> - remove Xen 1:1
> - restore mapping
> 
> It should work, right? Basically, a mapping conflict shouldn't be an
> issue given that the mapping has only to live long enough to call
> switch_ttbr_id.

Today switch_ttbr() is called before we initialized most of the memory 
layout. So clashing with the VMAP and frametable is not a problem.

However, the identity mapping may also clash with the region used to map 
Xen. That said, technically, we are not able to handle Xen when its 
start address is in region 2MB + 4K to 4MB (Xen is loaded at a 4KB 
aligned address).

The trouble is some features (e.g. UBSAN, GCOV) can generate Xen image 
over 2MB. IOW, the range where Xen cannot be loaded will increase.

This is an issue because AFAIK, there is no away to tell GRUB "You can't 
load Xen at this region". But even if there were one, I feel this 
restriction is sort of random.

I already wrote a patch to get rid of the restriction. The code is not 
too bad (we only need an extra indirection). But I haven't sent it yet 
because it is less critical with the re-shuffling of the memory layout.

Anyway, that's a long way to say that it will soon become an issue if 
the ID mapping is clashing with Xen mappings.

> 
> I am less sure about patch #5 but it doesn't seem it would be a problem
> there either.

This is actually going to be problematic. On Arm64, the page-tables are 
shared with all the CPUs. You would need to prevent the CPUs to touch 
any of the mapping we removed.

While booting, idle pCPUs will usually scrub the pages. So the 
frametable will be used. In theory, we could make sure the CPUs are not 
scrubbing. This would get trick for CPU hotpluggling (not yet supported) 
as CPU would need to idle. IMHO, this would be unnaceptable to block all 
the CPUs just to bring a new one.

Furthermore, we would need to be careful anytime we define new regions 
in the memory layout or reshuffle it as we need to ensure that no-one 
else use them when the ID mapping is inplace.

The memory layout is far from been full on Arm64. So to me, the extra 
risk is not worth it. The same goes for Arm32 (even thought the memory 
has much less space).

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Stefano Stabellini]

On Sat, 12 Mar 2022, Julien Grall wrote:
> On 12/03/2022 01:31, Stefano Stabellini wrote:
> > On Wed, 9 Mar 2022, Julien Grall wrote:
> > > From: Julien Grall <jgrall@amazon.com>
> > > 
> > > At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
> > > still on.
> > > 
> > > Switching TTBR is like replacing existing mappings with new ones. So
> > > we need to follow the break-before-make sequence.
> > > 
> > > In this case, it means the MMU needs to be switched off while the
> > > TTBR is updated. In order to disable the MMU, we need to first
> > > jump to an identity mapping.
> > > 
> > > Rename switch_ttbr() to switch_ttbr_id() and create an helper on
> > > top to temporary map the identity mapping and call switch_ttbr()
> > > via the identity address.
> > > 
> > > switch_ttbr_id() is now reworked to temporarily turn off the MMU
> > > before updating the TTBR.
> > > 
> > > We also need to make sure the helper switch_ttbr() is part of the
> > > identity mapping. So move _end_boot past it.
> > > 
> > > Take the opportunity to instruction cache flush as the operation is
> > > only necessary when the memory is updated.
> > > 
> > > Signed-off-by: Julien Grall <jgrall@amazon.com>
> > > 
> > > ---
> > > 
> > >      TODO:
> > >          * Rename _end_boot to _end_id_mapping or similar
> > >          * Check the memory barriers
> > >          * I suspect the instruction cache flush will be necessary
> > >            for cache coloring.
> > > ---
> > >   xen/arch/arm/arm64/head.S | 31 ++++++++++++++++++++-----------
> > >   xen/arch/arm/mm.c         | 14 +++++++++++++-
> > >   2 files changed, 33 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > index 878649280d73..c5cc72b8fe6f 100644
> > > --- a/xen/arch/arm/arm64/head.S
> > > +++ b/xen/arch/arm/arm64/head.S
> > > @@ -803,36 +803,45 @@ fail:   PRINT("- Boot failed -\r\n")
> > >           b     1b
> > >   ENDPROC(fail)
> > >   -GLOBAL(_end_boot)
> > > -
> > >   /*
> > >    * Switch TTBR
> > >    *
> > >    * x0    ttbr
> > >    *
> > > - * TODO: This code does not comply with break-before-make.
> > > + * XXX: Check the barriers
> > >    */
> > > -ENTRY(switch_ttbr)
> > > +ENTRY(switch_ttbr_id)
> > >           dsb   sy                     /* Ensure the flushes happen before
> > >                                         * continuing */
> > >           isb                          /* Ensure synchronization with
> > > previous
> > >                                         * changes to text */
> > > +
> > > +        /* Turn off MMU */
> > > +        mrs    x1, SCTLR_EL2
> > > +        bic    x1, x1, #SCTLR_Axx_ELx_M
> > > +        msr    SCTLR_EL2, x1
> > > +        dsb    sy
> > > +        isb
> > > +
> > >           tlbi   alle2                 /* Flush hypervisor TLB */
> > > -        ic     iallu                 /* Flush I-cache */
> > >           dsb    sy                    /* Ensure completion of TLB flush
> > > */
> > >           isb
> > >   -        msr    TTBR0_EL2, x0
> > > +        msr   TTBR0_EL2, x0
> > > +
> > > +        mrs   x1, SCTLR_EL2
> > > +        orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
> > > +        msr   SCTLR_EL2, x1
> > >             isb                          /* Ensure synchronization with
> > > previous
> > >                                         * changes to text */
> > > -        tlbi   alle2                 /* Flush hypervisor TLB */
> > > -        ic     iallu                 /* Flush I-cache */
> > > -        dsb    sy                    /* Ensure completion of TLB flush */
> > > -        isb
> > > +        /* Turn on the MMU */
> > > +
> > >             ret
> > > -ENDPROC(switch_ttbr)
> > > +ENDPROC(switch_ttbr_id)
> > > +
> > > +GLOBAL(_end_boot)
> > >     #ifdef CONFIG_EARLY_PRINTK
> > >   /*
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index 5c4dece16f7f..a53760af7af0 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -660,7 +660,19 @@ static void xen_pt_enforce_wnx(void)
> > >       flush_xen_tlb_local();
> > >   }
> > >   -extern void switch_ttbr(uint64_t ttbr);
> > > +extern void switch_ttbr_id(uint64_t ttbr);
> > > +
> > > +typedef void (switch_ttbr_fn)(uint64_t ttbr);
> > > +
> > > +static void switch_ttbr(uint64_t ttbr)
> > > +{
> > > +    vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
> > > +    switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
> > > +
> > > +    update_identity_mapping(true);
> > > +    fn(ttbr);
> > > +    update_identity_mapping(false);
> > > +}
> > 
> > Controversial question: does it really matter that XEN_VIRT_START >
> > 512GB and that _start < 512GB?
> > 
> > I am totally fine with the limit, I am just brainstorming: given that
> > the mapping is used very temporarely, it wouldn't really be an issue if
> > it conflicts with something important. Let's say that it conflicts with
> > the VMAP or the FRAMETABLE. As long as:
> > 
> > - we save the current mapping
> > - update it with the Xen 1:1
> > - switch_ttbr
> > - remove Xen 1:1
> > - restore mapping
> > 
> > It should work, right? Basically, a mapping conflict shouldn't be an
> > issue given that the mapping has only to live long enough to call
> > switch_ttbr_id.
> 
> Today switch_ttbr() is called before we initialized most of the memory layout.
> So clashing with the VMAP and frametable is not a problem.
> 
> However, the identity mapping may also clash with the region used to map Xen.
> That said, technically, we are not able to handle Xen when its start address
> is in region 2MB + 4K to 4MB (Xen is loaded at a 4KB aligned address).
> 
> The trouble is some features (e.g. UBSAN, GCOV) can generate Xen image over
> 2MB. IOW, the range where Xen cannot be loaded will increase.
> 
> This is an issue because AFAIK, there is no away to tell GRUB "You can't load
> Xen at this region". But even if there were one, I feel this restriction is
> sort of random.
> 
> I already wrote a patch to get rid of the restriction. The code is not too bad
> (we only need an extra indirection). But I haven't sent it yet because it is
> less critical with the re-shuffling of the memory layout.

Interesting! I am curious: how did you manage to do it?

For now and for this series the current approach and the 512GB limit are
fine. My replies here are brainstorming to see if there are potential
alternatives in the future in case the need arises.

I can see that a clash with Xen mapping could be problematic and the
chances of that happening are low but non-zero. We could make sure that
ImageBuilder always picks safe addresses and that would help but
wouldn't remove the issue if someone is not using ImageBuilder.

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Julien Grall]

Hi Stefano,

On 14/03/2022 23:48, Stefano Stabellini wrote:
>>> - we save the current mapping
>>> - update it with the Xen 1:1
>>> - switch_ttbr
>>> - remove Xen 1:1
>>> - restore mapping
>>>
>>> It should work, right? Basically, a mapping conflict shouldn't be an
>>> issue given that the mapping has only to live long enough to call
>>> switch_ttbr_id.
>>
>> Today switch_ttbr() is called before we initialized most of the memory layout.
>> So clashing with the VMAP and frametable is not a problem.
>>
>> However, the identity mapping may also clash with the region used to map Xen.
>> That said, technically, we are not able to handle Xen when its start address
>> is in region 2MB + 4K to 4MB (Xen is loaded at a 4KB aligned address).
>>
>> The trouble is some features (e.g. UBSAN, GCOV) can generate Xen image over
>> 2MB. IOW, the range where Xen cannot be loaded will increase.
>>
>> This is an issue because AFAIK, there is no away to tell GRUB "You can't load
>> Xen at this region". But even if there were one, I feel this restriction is
>> sort of random.
>>
>> I already wrote a patch to get rid of the restriction. The code is not too bad
>> (we only need an extra indirection). But I haven't sent it yet because it is
>> less critical with the re-shuffling of the memory layout.
> 
> Interesting! I am curious: how did you manage to do it?

When the identity mapping is clashing with Xen runtime address, I am 
creating a temporary mapping for Xen at a different fixed address.

Once the MMU is turned on, we can jump to the temporary mapping. After 
that we are safe to remove the identity mapping and create the runtime 
Xen mapping. The last step is to jump on the runtime mapping and then 
remove the temporary mapping.

> 
> For now and for this series the current approach and the 512GB limit are
> fine. My replies here are brainstorming to see if there are potential
> alternatives in the future in case the need arises.

On Arm64, we have 256TB worth of virtual address. So I think we can 
easily spare 512GB for the foreseeable :).

If we are at the point where we can't space 512GB, then we would need to 
have a more dynamic layout as I plan on arm32.

Xen would still be mapped at a specific virtual address so we don't need 
to update the relocations. But we could decide at runtime the position 
of other large mappings (e.g. vmap, domheap).

Probably the safest way is to link Xen at a very high address. It is 
quite unlikely that Xen will be loaded at such high address.

If it is, we could exceptionally relocate Xen (with this series it 
should be safer to do). That said, I would like to avoid relocating Xen 
until we see a use for that.

> 
> I can see that a clash with Xen mapping could be problematic and the
> chances of that happening are low but non-zero. We could make sure that
> ImageBuilder always picks safe addresses and that would help but
> wouldn't remove the issue if someone is not using ImageBuilder.

AFAIU, ImageBuilder is here to cater U-boot users. I am not too worry 
about those setups because a user can pick any address they want. So as 
long as Xen print an error during the clash (already the case), the user 
can easily update their scripts.

This is more a problem for UEFI/GRUB where, AFAICT, we can't control 
where Xen will be loaded.

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Stefano Stabellini]

On Tue, 15 Mar 2022, Julien Grall wrote:
> On 14/03/2022 23:48, Stefano Stabellini wrote:
> > > > - we save the current mapping
> > > > - update it with the Xen 1:1
> > > > - switch_ttbr
> > > > - remove Xen 1:1
> > > > - restore mapping
> > > > 
> > > > It should work, right? Basically, a mapping conflict shouldn't be an
> > > > issue given that the mapping has only to live long enough to call
> > > > switch_ttbr_id.
> > > 
> > > Today switch_ttbr() is called before we initialized most of the memory
> > > layout.
> > > So clashing with the VMAP and frametable is not a problem.
> > > 
> > > However, the identity mapping may also clash with the region used to map
> > > Xen.
> > > That said, technically, we are not able to handle Xen when its start
> > > address
> > > is in region 2MB + 4K to 4MB (Xen is loaded at a 4KB aligned address).
> > > 
> > > The trouble is some features (e.g. UBSAN, GCOV) can generate Xen image
> > > over
> > > 2MB. IOW, the range where Xen cannot be loaded will increase.
> > > 
> > > This is an issue because AFAIK, there is no away to tell GRUB "You can't
> > > load
> > > Xen at this region". But even if there were one, I feel this restriction
> > > is
> > > sort of random.
> > > 
> > > I already wrote a patch to get rid of the restriction. The code is not too
> > > bad
> > > (we only need an extra indirection). But I haven't sent it yet because it
> > > is
> > > less critical with the re-shuffling of the memory layout.
> > 
> > Interesting! I am curious: how did you manage to do it?
> 
> When the identity mapping is clashing with Xen runtime address, I am creating
> a temporary mapping for Xen at a different fixed address.
> 
> Once the MMU is turned on, we can jump to the temporary mapping. After that we
> are safe to remove the identity mapping and create the runtime Xen mapping.
> The last step is to jump on the runtime mapping and then remove the temporary
> mapping.

Cool! I was guessing something along those lines.

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Bertrand Marquis]

Hi Julien,

> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
> still on.
> 
> Switching TTBR is like replacing existing mappings with new ones. So
> we need to follow the break-before-make sequence.
> 
> In this case, it means the MMU needs to be switched off while the
> TTBR is updated. In order to disable the MMU, we need to first
> jump to an identity mapping.
> 
> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
> top to temporary map the identity mapping and call switch_ttbr()
> via the identity address.
> 
> switch_ttbr_id() is now reworked to temporarily turn off the MMU
> before updating the TTBR.
> 
> We also need to make sure the helper switch_ttbr() is part of the
> identity mapping. So move _end_boot past it.
> 
> Take the opportunity to instruction cache flush as the operation is
> only necessary when the memory is updated.

Your code is actually remove the instruction cache invalidation so
this sentence is a bit misleading.

Also an open question: shouldn’t we flush the data cache ?
As we switch from one TTBR to an other, there might be some data
in the cache dependent that could be flushed while the MMU is off or
that would have no mapping once it is reactivated.


> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
>    TODO:
>        * Rename _end_boot to _end_id_mapping or similar
>        * Check the memory barriers
>        * I suspect the instruction cache flush will be necessary
>          for cache coloring.
> ---
> xen/arch/arm/arm64/head.S | 31 ++++++++++++++++++++-----------
> xen/arch/arm/mm.c         | 14 +++++++++++++-
> 2 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 878649280d73..c5cc72b8fe6f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -803,36 +803,45 @@ fail:   PRINT("- Boot failed -\r\n")
>         b     1b
> ENDPROC(fail)
> 
> -GLOBAL(_end_boot)
> -
> /*
>  * Switch TTBR
>  *
>  * x0    ttbr
>  *
> - * TODO: This code does not comply with break-before-make.
> + * XXX: Check the barriers
>  */
> -ENTRY(switch_ttbr)
> +ENTRY(switch_ttbr_id)
>         dsb   sy                     /* Ensure the flushes happen before
>                                       * continuing */
>         isb                          /* Ensure synchronization with previous
>                                       * changes to text */
> +
> +        /* Turn off MMU */
> +        mrs    x1, SCTLR_EL2
> +        bic    x1, x1, #SCTLR_Axx_ELx_M
> +        msr    SCTLR_EL2, x1
> +        dsb    sy
> +        isb
> +
>         tlbi   alle2                 /* Flush hypervisor TLB */
> -        ic     iallu                 /* Flush I-cache */
>         dsb    sy                    /* Ensure completion of TLB flush */
>         isb
> 
> -        msr    TTBR0_EL2, x0
> +        msr   TTBR0_EL2, x0
> +
> +        mrs   x1, SCTLR_EL2
> +        orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
> +        msr   SCTLR_EL2, x1
> 
>         isb                          /* Ensure synchronization with previous
>                                       * changes to text */
> -        tlbi   alle2                 /* Flush hypervisor TLB */
> -        ic     iallu                 /* Flush I-cache */
> -        dsb    sy                    /* Ensure completion of TLB flush */
> -        isb
> +        /* Turn on the MMU */
> +
> 
>         ret
> -ENDPROC(switch_ttbr)
> +ENDPROC(switch_ttbr_id)
> +
> +GLOBAL(_end_boot)
> 
> #ifdef CONFIG_EARLY_PRINTK
> /*
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 5c4dece16f7f..a53760af7af0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -660,7 +660,19 @@ static void xen_pt_enforce_wnx(void)
>     flush_xen_tlb_local();
> }
> 
> -extern void switch_ttbr(uint64_t ttbr);
> +extern void switch_ttbr_id(uint64_t ttbr);
> +
> +typedef void (switch_ttbr_fn)(uint64_t ttbr);
> +
> +static void switch_ttbr(uint64_t ttbr)
> +{
> +    vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
> +    switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
> +
> +    update_identity_mapping(true);
> +    fn(ttbr);
> +    update_identity_mapping(false);
> +}
> 
> /* Clear a translation table and clean & invalidate the cache */
> static void clear_table(void *table)
> -- 
> 2.32.0
> 

Cheers
Bertrand

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Julien Grall]

On 25/03/2022 13:47, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
>> still on.
>>
>> Switching TTBR is like replacing existing mappings with new ones. So
>> we need to follow the break-before-make sequence.
>>
>> In this case, it means the MMU needs to be switched off while the
>> TTBR is updated. In order to disable the MMU, we need to first
>> jump to an identity mapping.
>>
>> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
>> top to temporary map the identity mapping and call switch_ttbr()
>> via the identity address.
>>
>> switch_ttbr_id() is now reworked to temporarily turn off the MMU
>> before updating the TTBR.
>>
>> We also need to make sure the helper switch_ttbr() is part of the
>> identity mapping. So move _end_boot past it.
>>
>> Take the opportunity to instruction cache flush as the operation is
>> only necessary when the memory is updated.
> 
> Your code is actually remove the instruction cache invalidation so
> this sentence is a bit misleading.

I forgot to add the word "remove" in the sentence.

> 
> Also an open question: shouldn’t we flush the data cache ?
Do you mean clean/invalidate to PoC/PoU? Something else?

> As we switch from one TTBR to an other, there might be some data
> in the cache dependent that could be flushed while the MMU is off 

I am a bit confused. Those flush could also happen with the MMU on. So 
how turning off the MMU would result to a problem? Note that the data 
cache is still enabled during the switch.

> or
> that would have no mapping once it is reactivated.
The cache line will be flushed at some point in the future. I would 
argue if the caller need it earlier, then it should make sure to issue 
the flush before switch_ttbr().

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Bertrand Marquis]

Hi Julien,

> On 25 Mar 2022, at 15:24, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 25/03/2022 13:47, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>> 
>>> From: Julien Grall <jgrall@amazon.com>
>>> 
>>> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
>>> still on.
>>> 
>>> Switching TTBR is like replacing existing mappings with new ones. So
>>> we need to follow the break-before-make sequence.
>>> 
>>> In this case, it means the MMU needs to be switched off while the
>>> TTBR is updated. In order to disable the MMU, we need to first
>>> jump to an identity mapping.
>>> 
>>> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
>>> top to temporary map the identity mapping and call switch_ttbr()
>>> via the identity address.
>>> 
>>> switch_ttbr_id() is now reworked to temporarily turn off the MMU
>>> before updating the TTBR.
>>> 
>>> We also need to make sure the helper switch_ttbr() is part of the
>>> identity mapping. So move _end_boot past it.
>>> 
>>> Take the opportunity to instruction cache flush as the operation is
>>> only necessary when the memory is updated.
>> Your code is actually remove the instruction cache invalidation so
>> this sentence is a bit misleading.
> 
> I forgot to add the word "remove" in the sentence.

Ok (my sentence was also wrong by the way)

> 
>> Also an open question: shouldn’t we flush the data cache ?
> Do you mean clean/invalidate to PoC/PoU? Something else?

Yes, probably to PoU.

> 
>> As we switch from one TTBR to an other, there might be some data
>> in the cache dependent that could be flushed while the MMU is off 
> 
> I am a bit confused. Those flush could also happen with the MMU on. So how turning off the MMU would result to a problem? Note that the data cache is still enabled during the switch.

If the first level of cache is VIPT and we turn off the MMU, I am wondering if this could not create troubles and could require the cache to be flushed before turning the MMU off.
I have no idea if this is a problem or not, just raising the question.
I can try to dig on that at Arm when I am back in 10 days. 

> 
>> or
>> that would have no mapping once it is reactivated.
> The cache line will be flushed at some point in the future. I would argue if the caller need it earlier, then it should make sure to issue the flush before switch_ttbr().
Ok.

I will still try to check if there is some kind of recommandation to turn the MMU off.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Julien Grall]

Hi Bertrand,

On 25/03/2022 14:35, Bertrand Marquis wrote:
>> On 25 Mar 2022, at 15:24, Julien Grall <julien@xen.org> wrote:
>> On 25/03/2022 13:47, Bertrand Marquis wrote:
>>> Hi Julien,
>>
>> Hi Bertrand,
>>
>>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
>>>> still on.
>>>>
>>>> Switching TTBR is like replacing existing mappings with new ones. So
>>>> we need to follow the break-before-make sequence.
>>>>
>>>> In this case, it means the MMU needs to be switched off while the
>>>> TTBR is updated. In order to disable the MMU, we need to first
>>>> jump to an identity mapping.
>>>>
>>>> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
>>>> top to temporary map the identity mapping and call switch_ttbr()
>>>> via the identity address.
>>>>
>>>> switch_ttbr_id() is now reworked to temporarily turn off the MMU
>>>> before updating the TTBR.
>>>>
>>>> We also need to make sure the helper switch_ttbr() is part of the
>>>> identity mapping. So move _end_boot past it.
>>>>
>>>> Take the opportunity to instruction cache flush as the operation is
>>>> only necessary when the memory is updated.
>>> Your code is actually remove the instruction cache invalidation so
>>> this sentence is a bit misleading.
>>
>> I forgot to add the word "remove" in the sentence.
> 
> Ok (my sentence was also wrong by the way)
> 
>>
>>> Also an open question: shouldn’t we flush the data cache ?
>> Do you mean clean/invalidate to PoC/PoU? Something else?
> 
> Yes, probably to PoU.
> 
>>
>>> As we switch from one TTBR to an other, there might be some data
>>> in the cache dependent that could be flushed while the MMU is off
>>
>> I am a bit confused. Those flush could also happen with the MMU on. So how turning off the MMU would result to a problem? Note that the data cache is still enabled during the switch.
> 
> If the first level of cache is VIPT and we turn off the MMU, I am wondering if this could not create troubles and could require the cache to be flushed before turning the MMU off.
My reading of the Arm Arm (D5.11.1 "Data and unified caches" ARM DDI 
0487F.c) suggests the data cache is always PIPT.

> I have no idea if this is a problem or not, just raising the question.
> I can try to dig on that at Arm when I am back in 10 days.

Enjoy it!

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Bertrand Marquis]

Hi Julien,

> On 25 Mar 2022, at 15:42, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 25/03/2022 14:35, Bertrand Marquis wrote:
>>> On 25 Mar 2022, at 15:24, Julien Grall <julien@xen.org> wrote:
>>> On 25/03/2022 13:47, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi Bertrand,
>>> 
>>>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>> 
>>>>> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
>>>>> still on.
>>>>> 
>>>>> Switching TTBR is like replacing existing mappings with new ones. So
>>>>> we need to follow the break-before-make sequence.
>>>>> 
>>>>> In this case, it means the MMU needs to be switched off while the
>>>>> TTBR is updated. In order to disable the MMU, we need to first
>>>>> jump to an identity mapping.
>>>>> 
>>>>> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
>>>>> top to temporary map the identity mapping and call switch_ttbr()
>>>>> via the identity address.
>>>>> 
>>>>> switch_ttbr_id() is now reworked to temporarily turn off the MMU
>>>>> before updating the TTBR.
>>>>> 
>>>>> We also need to make sure the helper switch_ttbr() is part of the
>>>>> identity mapping. So move _end_boot past it.
>>>>> 
>>>>> Take the opportunity to instruction cache flush as the operation is
>>>>> only necessary when the memory is updated.
>>>> Your code is actually remove the instruction cache invalidation so
>>>> this sentence is a bit misleading.
>>> 
>>> I forgot to add the word "remove" in the sentence.
>> Ok (my sentence was also wrong by the way)
>>> 
>>>> Also an open question: shouldn’t we flush the data cache ?
>>> Do you mean clean/invalidate to PoC/PoU? Something else?
>> Yes, probably to PoU.
>>> 
>>>> As we switch from one TTBR to an other, there might be some data
>>>> in the cache dependent that could be flushed while the MMU is off
>>> 
>>> I am a bit confused. Those flush could also happen with the MMU on. So how turning off the MMU would result to a problem? Note that the data cache is still enabled during the switch.
>> If the first level of cache is VIPT and we turn off the MMU, I am wondering if this could not create troubles and could require the cache to be flushed before turning the MMU off.
> My reading of the Arm Arm (D5.11.1 "Data and unified caches" ARM DDI 0487F.c) suggests the data cache is always PIPT.

You are right, only the instruction cache is VIPT.
So the problem most probably does not exist.

> 
>> I have no idea if this is a problem or not, just raising the question.
>> I can try to dig on that at Arm when I am back in 10 days.
> 
> Enjoy it!

Thanks
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Julien Grall]

Hi,

On 25/03/2022 14:48, Bertrand Marquis wrote:
>> On 25 Mar 2022, at 15:42, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Bertrand,
>>
>> On 25/03/2022 14:35, Bertrand Marquis wrote:
>>>> On 25 Mar 2022, at 15:24, Julien Grall <julien@xen.org> wrote:
>>>> On 25/03/2022 13:47, Bertrand Marquis wrote:
>>>>> Hi Julien,
>>>>
>>>> Hi Bertrand,
>>>>
>>>>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>>>>>
>>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>>
>>>>>> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
>>>>>> still on.
>>>>>>
>>>>>> Switching TTBR is like replacing existing mappings with new ones. So
>>>>>> we need to follow the break-before-make sequence.
>>>>>>
>>>>>> In this case, it means the MMU needs to be switched off while the
>>>>>> TTBR is updated. In order to disable the MMU, we need to first
>>>>>> jump to an identity mapping.
>>>>>>
>>>>>> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
>>>>>> top to temporary map the identity mapping and call switch_ttbr()
>>>>>> via the identity address.
>>>>>>
>>>>>> switch_ttbr_id() is now reworked to temporarily turn off the MMU
>>>>>> before updating the TTBR.
>>>>>>
>>>>>> We also need to make sure the helper switch_ttbr() is part of the
>>>>>> identity mapping. So move _end_boot past it.
>>>>>>
>>>>>> Take the opportunity to instruction cache flush as the operation is
>>>>>> only necessary when the memory is updated.
>>>>> Your code is actually remove the instruction cache invalidation so
>>>>> this sentence is a bit misleading.
>>>>
>>>> I forgot to add the word "remove" in the sentence.
>>> Ok (my sentence was also wrong by the way)
>>>>
>>>>> Also an open question: shouldn’t we flush the data cache ?
>>>> Do you mean clean/invalidate to PoC/PoU? Something else?
>>> Yes, probably to PoU.
>>>>
>>>>> As we switch from one TTBR to an other, there might be some data
>>>>> in the cache dependent that could be flushed while the MMU is off
>>>>
>>>> I am a bit confused. Those flush could also happen with the MMU on. So how turning off the MMU would result to a problem? Note that the data cache is still enabled during the switch.
>>> If the first level of cache is VIPT and we turn off the MMU, I am wondering if this could not create troubles and could require the cache to be flushed before turning the MMU off.
>> My reading of the Arm Arm (D5.11.1 "Data and unified caches" ARM DDI 0487F.c) suggests the data cache is always PIPT.
> 
> You are right, only the instruction cache is VIPT.
> So the problem most probably does not exist.

As discussed yesterda, I tweaked a bit switch_ttbr(). Below the version 
I plan to use:

         /* 1) Ensure any previous read/write have completed */
         dsb   sy /* XXX: Can this be a ish? */
         isb

         /* 2) Turn off MMU */
         mrs    x1, SCTLR_EL2
         bic    x1, x1, #SCTLR_Axx_ELx_M
         msr    SCTLR_EL2, x1
         isb

         /*
          * 3) Flush the TLBs.
          * See asm/arm64/flushtlb.h for the explanation of the sequence.
          */
         dsb   nshst
         tlbi  alle2
         dsb   nsh
         isb

         /* 4) Update the TTBR */
         msr   TTBR0_EL2, x0
         isb

         /* 5) Turn on the MMU */
         mrs   x1, SCTLR_EL2
         orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
         msr   SCTLR_EL2, x1
         isb

         ret

Cheers,

-- 
Julien Grall

Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Bertrand Marquis]

Hi Julien,

> On 7 Apr 2022, at 16:38, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 25/03/2022 14:48, Bertrand Marquis wrote:
>>> On 25 Mar 2022, at 15:42, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On 25/03/2022 14:35, Bertrand Marquis wrote:
>>>>> On 25 Mar 2022, at 15:24, Julien Grall <julien@xen.org> wrote:
>>>>> On 25/03/2022 13:47, Bertrand Marquis wrote:
>>>>>> Hi Julien,
>>>>> 
>>>>> Hi Bertrand,
>>>>> 
>>>>>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>>>>>> 
>>>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>>> 
>>>>>>> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
>>>>>>> still on.
>>>>>>> 
>>>>>>> Switching TTBR is like replacing existing mappings with new ones. So
>>>>>>> we need to follow the break-before-make sequence.
>>>>>>> 
>>>>>>> In this case, it means the MMU needs to be switched off while the
>>>>>>> TTBR is updated. In order to disable the MMU, we need to first
>>>>>>> jump to an identity mapping.
>>>>>>> 
>>>>>>> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
>>>>>>> top to temporary map the identity mapping and call switch_ttbr()
>>>>>>> via the identity address.
>>>>>>> 
>>>>>>> switch_ttbr_id() is now reworked to temporarily turn off the MMU
>>>>>>> before updating the TTBR.
>>>>>>> 
>>>>>>> We also need to make sure the helper switch_ttbr() is part of the
>>>>>>> identity mapping. So move _end_boot past it.
>>>>>>> 
>>>>>>> Take the opportunity to instruction cache flush as the operation is
>>>>>>> only necessary when the memory is updated.
>>>>>> Your code is actually remove the instruction cache invalidation so
>>>>>> this sentence is a bit misleading.
>>>>> 
>>>>> I forgot to add the word "remove" in the sentence.
>>>> Ok (my sentence was also wrong by the way)
>>>>> 
>>>>>> Also an open question: shouldn’t we flush the data cache ?
>>>>> Do you mean clean/invalidate to PoC/PoU? Something else?
>>>> Yes, probably to PoU.
>>>>> 
>>>>>> As we switch from one TTBR to an other, there might be some data
>>>>>> in the cache dependent that could be flushed while the MMU is off
>>>>> 
>>>>> I am a bit confused. Those flush could also happen with the MMU on. So how turning off the MMU would result to a problem? Note that the data cache is still enabled during the switch.
>>>> If the first level of cache is VIPT and we turn off the MMU, I am wondering if this could not create troubles and could require the cache to be flushed before turning the MMU off.
>>> My reading of the Arm Arm (D5.11.1 "Data and unified caches" ARM DDI 0487F.c) suggests the data cache is always PIPT.
>> You are right, only the instruction cache is VIPT.
>> So the problem most probably does not exist.
> 
> As discussed yesterda, I tweaked a bit switch_ttbr(). Below the version I plan to use:
> 
>        /* 1) Ensure any previous read/write have completed */
>        dsb   sy /* XXX: Can this be a ish? */

I think here “ish” is enough as we only need the domain impacted by the MMU off operation to have his operations done.

For reference this is an extract from the code of trusted firmware before enabling MMU:

        /*
         * Ensure all translation table writes have drained into memory, the TLB
         * invalidation is complete, and translation register writes are
         * committed before enabling the MMU
         */
        dsb ish
        isb


>        isb
> 
>        /* 2) Turn off MMU */
>        mrs    x1, SCTLR_EL2
>        bic    x1, x1, #SCTLR_Axx_ELx_M
>        msr    SCTLR_EL2, x1
>        isb
> 
>        /*
>         * 3) Flush the TLBs.
>         * See asm/arm64/flushtlb.h for the explanation of the sequence.
>         */
>        dsb   nshst
>        tlbi  alle2
>        dsb   nsh
>        isb
> 
>        /* 4) Update the TTBR */
>        msr   TTBR0_EL2, x0
>        isb
> 
>        /* 5) Turn on the MMU */
>        mrs   x1, SCTLR_EL2
>        orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
>        msr   SCTLR_EL2, x1
>        isb

Rest here sounds ok to me.

Cheers
Bertrand

> 
>        ret
> 
> Cheers,
> 
> -- 
> Julien Grall

[PATCH early-RFC 5/5] xen/arm: smpboot: Directly switch to the runtime page-tables

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Switching TTBR while the MMU is on is not safe. Now that the identity
mapping will not clash with the rest of the memory layout, we can avoid
creating temporary page-tables every time a CPU is brought up.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/arm64/head.S | 29 +++++++++--------------------
 xen/arch/arm/mm.c         | 19 -------------------
 xen/arch/arm/smpboot.c    |  3 +++
 3 files changed, 12 insertions(+), 39 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index c5cc72b8fe6f..f0ac5a3295cc 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -309,6 +309,7 @@ real_start_efi:
         bl    check_cpu_mode
         bl    cpu_init
         bl    create_page_tables
+        load_paddr x0, boot_pgtable
         bl    enable_mmu
 
         /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
@@ -368,29 +369,14 @@ GLOBAL(init_secondary)
 #endif
         bl    check_cpu_mode
         bl    cpu_init
-        bl    create_page_tables
+        load_paddr x0, init_ttbr
+        ldr   x0, [x0]
         bl    enable_mmu
 
         /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
         ldr   x0, =secondary_switched
         br    x0
 secondary_switched:
-        /*
-         * Non-boot CPUs need to move on to the proper pagetables, which were
-         * setup in init_secondary_pagetables.
-         *
-         * XXX: This is not compliant with the Arm Arm.
-         */
-        ldr   x4, =init_ttbr         /* VA of TTBR0_EL2 stashed by CPU 0 */
-        ldr   x4, [x4]               /* Actual value */
-        dsb   sy
-        msr   TTBR0_EL2, x4
-        dsb   sy
-        isb
-        tlbi  alle2
-        dsb   sy                     /* Ensure completion of TLB flush */
-        isb
-
 #ifdef CONFIG_EARLY_PRINTK
         /* Use a virtual address to access the UART. */
         ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
@@ -661,9 +647,13 @@ ENDPROC(create_page_tables)
  * mapping. In other word, the caller is responsible to switch to the runtime
  * mapping.
  *
- * Clobbers x0 - x3
+ * Inputs:
+ *   x0 : Physical address of the page tables.
+ *
+ * Clobbers x0 - x4
  */
 enable_mmu:
+        mov   x4, x0
         PRINT("- Turning on paging -\r\n")
 
         /*
@@ -674,8 +664,7 @@ enable_mmu:
         dsb   nsh
 
         /* Write Xen's PT's paddr into TTBR0_EL2 */
-        load_paddr x0, boot_pgtable
-        msr   TTBR0_EL2, x0
+        msr   TTBR0_EL2, x4
         isb
 
         mrs   x0, SCTLR_EL2
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a53760af7af0..be808073844a 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -771,26 +771,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
 #endif
 }
 
-static void clear_boot_pagetables(void)
-{
-    /*
-     * Clear the copy of the boot pagetables. Each secondary CPU
-     * rebuilds these itself (see head.S).
-     */
-    clear_table(boot_pgtable);
-#ifdef CONFIG_ARM_64
-    clear_table(boot_first);
-    clear_table(boot_first_id);
-#endif
-    clear_table(boot_second);
-    clear_table(boot_third);
-}
-
 #ifdef CONFIG_ARM_64
 int init_secondary_pagetables(int cpu)
 {
-    clear_boot_pagetables();
-
     /* Set init_ttbr for this CPU coming up. All CPus share a single setof
      * pagetables, but rewrite it each time for consistency with 32 bit. */
     init_ttbr = (uintptr_t) xen_pgtable + phys_offset;
@@ -833,8 +816,6 @@ int init_secondary_pagetables(int cpu)
     per_cpu(xen_pgtable, cpu) = first;
     per_cpu(xen_dommap, cpu) = domheap;
 
-    clear_boot_pagetables();
-
     /* Set init_ttbr for this CPU coming up */
     init_ttbr = __pa(first);
     clean_dcache(init_ttbr);
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 7bfd0a73a7d2..72931d0cef93 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -457,12 +457,14 @@ int __cpu_up(unsigned int cpu)
     smp_up_cpu = cpu_logical_map(cpu);
     clean_dcache(smp_up_cpu);
 
+    update_identity_mapping(true);
     rc = arch_cpu_up(cpu);
 
     console_end_sync();
 
     if ( rc < 0 )
     {
+        update_identity_mapping(false);
         printk("Failed to bring up CPU%d\n", cpu);
         return rc;
     }
@@ -493,6 +495,7 @@ int __cpu_up(unsigned int cpu)
     init_data.cpuid = ~0;
     smp_up_cpu = MPIDR_INVALID;
     clean_dcache(smp_up_cpu);
+    update_identity_mapping(false);
 
     if ( !cpu_online(cpu) )
     {
-- 
2.32.0