From: Julien Grall <jgrall@amazon.com>

With the upcoming work to color Xen, the binary will not be anymore
physically contiguous. This will be a problem during boot as the
assembly code will need to work out where each piece of Xen reside.

An easy way to solve the issue is to have all code/data accessed
by the secondary CPUs while the MMU is off within a single page.

Right now, init_ttbr is used by secondary CPUs to find there page-tables
before the MMU is on. Yet it is currently in .data which is unlikely
to be within the same page as the rest of the idmap.

Create a new section .data.idmap that will be used for variables
accessed by the early boot code. The first one is init_ttbr.

The idmap is currently part of the text section and therefore will
be mapped read-only executable. This means that we need to temporarily
remap init_ttbr in order to update it.

Introduce a new function set_init_ttbr() for this purpose so the code
is not duplicated between arm64 and arm32.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/mmu/smpboot.c | 34 +++++++++++++++++++++++++++++-----
 xen/arch/arm/xen.lds.S     |  1 +
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
index b6fc0aae07f1..f1cf9252710c 100644
--- a/xen/arch/arm/mmu/smpboot.c
+++ b/xen/arch/arm/mmu/smpboot.c
@@ -9,6 +9,10 @@
 
 #include <asm/setup.h>
 
+/* Override macros from asm/page.h to make them work with mfn_t */
+#undef virt_to_mfn
+#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
+
 /*
  * Static start-of-day pagetables that we use before the allocators
  * are up. These are used by all CPUs during bringup before switching
@@ -44,7 +48,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_second);
 DEFINE_BOOT_PAGE_TABLES(boot_third, XEN_NR_ENTRIES(2));
 
 /* Non-boot CPUs use this to find the correct pagetables. */
-uint64_t init_ttbr;
+uint64_t __section(".data.idmap") init_ttbr;
 
 /* Clear a translation table and clean & invalidate the cache */
 static void clear_table(void *table)
@@ -68,6 +72,27 @@ static void clear_boot_pagetables(void)
     clear_table(boot_third);
 }
 
+static void set_init_ttbr(lpae_t *root)
+{
+    /*
+     * init_ttbr is part of the identity mapping which is read-only. So
+     * We need to re-map the region so it can be updated
+     */
+    void *ptr = map_domain_page(virt_to_mfn(&init_ttbr));
+
+    ptr += PAGE_OFFSET(&init_ttbr);
+
+    *(uint64_t *)ptr = virt_to_maddr(root);
+
+    /*
+     * init_ttbr will be accessed with the MMU off, so ensure the update
+     * is visible by cleaning the cache.
+     */
+    clean_dcache(ptr);
+
+    unmap_domain_page(ptr);
+}
+
 #ifdef CONFIG_ARM_64
 int prepare_secondary_mm(int cpu)
 {
@@ -77,8 +102,8 @@ int prepare_secondary_mm(int cpu)
      * 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 = virt_to_maddr(xen_pgtable);
-    clean_dcache(init_ttbr);
+    set_init_ttbr(xen_pgtable);
+
     return 0;
 }
 #else
@@ -109,8 +134,7 @@ int prepare_secondary_mm(int cpu)
     clear_boot_pagetables();
 
     /* Set init_ttbr for this CPU coming up */
-    init_ttbr = __pa(first);
-    clean_dcache(init_ttbr);
+    set_init_ttbr(first);
 
     return 0;
 }
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 20598c6963ce..470c8f22084f 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -36,6 +36,7 @@ SECTIONS
        *(.text.header)
        *(.text.idmap)
        *(.rodata.idmap)
+       *(.data.idmap)
        _idmap_end = .;
 
        *(.text.cold)
-- 
2.40.1

Hi Julien,

On 16/01/2024 15:37, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> With the upcoming work to color Xen, the binary will not be anymore
> physically contiguous. This will be a problem during boot as the
> assembly code will need to work out where each piece of Xen reside.
> 
> An easy way to solve the issue is to have all code/data accessed
> by the secondary CPUs while the MMU is off within a single page.
> 
> Right now, init_ttbr is used by secondary CPUs to find there page-tables
> before the MMU is on. Yet it is currently in .data which is unlikely
> to be within the same page as the rest of the idmap.
> 
> Create a new section .data.idmap that will be used for variables
> accessed by the early boot code. The first one is init_ttbr.
> 
> The idmap is currently part of the text section and therefore will
> be mapped read-only executable. This means that we need to temporarily
> remap init_ttbr in order to update it.
> 
> Introduce a new function set_init_ttbr() for this purpose so the code
> is not duplicated between arm64 and arm32.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

with ...

> ---
>  xen/arch/arm/mmu/smpboot.c | 34 +++++++++++++++++++++++++++++-----
>  xen/arch/arm/xen.lds.S     |  1 +
>  2 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
> index b6fc0aae07f1..f1cf9252710c 100644
> --- a/xen/arch/arm/mmu/smpboot.c
> +++ b/xen/arch/arm/mmu/smpboot.c
> @@ -9,6 +9,10 @@
> 
>  #include <asm/setup.h>
> 
> +/* Override macros from asm/page.h to make them work with mfn_t */
> +#undef virt_to_mfn
> +#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
> +
>  /*
>   * Static start-of-day pagetables that we use before the allocators
>   * are up. These are used by all CPUs during bringup before switching
> @@ -44,7 +48,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_second);
>  DEFINE_BOOT_PAGE_TABLES(boot_third, XEN_NR_ENTRIES(2));
> 
>  /* Non-boot CPUs use this to find the correct pagetables. */
> -uint64_t init_ttbr;
> +uint64_t __section(".data.idmap") init_ttbr;
Do we need to keep the declaration in mmu/mm.h? This variable is only used in this file
and in assembly, so maybe better to drop declaration and use asmlinkage instead?

> 
>  /* Clear a translation table and clean & invalidate the cache */
>  static void clear_table(void *table)
> @@ -68,6 +72,27 @@ static void clear_boot_pagetables(void)
>      clear_table(boot_third);
>  }
> 
> +static void set_init_ttbr(lpae_t *root)
> +{
> +    /*
> +     * init_ttbr is part of the identity mapping which is read-only. So
> +     * We need to re-map the region so it can be updated
Would you mind fixing s/So We/So we/ and add a full stop after last sentence?

> +     */
> +    void *ptr = map_domain_page(virt_to_mfn(&init_ttbr));
> +
> +    ptr += PAGE_OFFSET(&init_ttbr);
> +
> +    *(uint64_t *)ptr = virt_to_maddr(root);
> +
> +    /*
> +     * init_ttbr will be accessed with the MMU off, so ensure the update
> +     * is visible by cleaning the cache.
> +     */
> +    clean_dcache(ptr);
> +
> +    unmap_domain_page(ptr);
> +}
> +
>  #ifdef CONFIG_ARM_64
>  int prepare_secondary_mm(int cpu)
>  {
> @@ -77,8 +102,8 @@ int prepare_secondary_mm(int cpu)
>       * 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 = virt_to_maddr(xen_pgtable);
> -    clean_dcache(init_ttbr);
> +    set_init_ttbr(xen_pgtable);
> +
>      return 0;
>  }
>  #else
> @@ -109,8 +134,7 @@ int prepare_secondary_mm(int cpu)
>      clear_boot_pagetables();
> 
>      /* Set init_ttbr for this CPU coming up */
> -    init_ttbr = __pa(first);
> -    clean_dcache(init_ttbr);
> +    set_init_ttbr(first);
> 
>      return 0;
>  }
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 20598c6963ce..470c8f22084f 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -36,6 +36,7 @@ SECTIONS
>         *(.text.header)
>         *(.text.idmap)
>         *(.rodata.idmap)
> +       *(.data.idmap)
>         _idmap_end = .;
> 
>         *(.text.cold)
> --
> 2.40.1
> 

~Michal

On 17/01/2024 08:30, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 16/01/2024 15:37, Julien Grall wrote:
>>
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> With the upcoming work to color Xen, the binary will not be anymore
>> physically contiguous. This will be a problem during boot as the
>> assembly code will need to work out where each piece of Xen reside.
>>
>> An easy way to solve the issue is to have all code/data accessed
>> by the secondary CPUs while the MMU is off within a single page.
>>
>> Right now, init_ttbr is used by secondary CPUs to find there page-tables
>> before the MMU is on. Yet it is currently in .data which is unlikely
>> to be within the same page as the rest of the idmap.
>>
>> Create a new section .data.idmap that will be used for variables
>> accessed by the early boot code. The first one is init_ttbr.
>>
>> The idmap is currently part of the text section and therefore will
>> be mapped read-only executable. This means that we need to temporarily
>> remap init_ttbr in order to update it.
>>
>> Introduce a new function set_init_ttbr() for this purpose so the code
>> is not duplicated between arm64 and arm32.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> with ...
> 
>> ---
>>   xen/arch/arm/mmu/smpboot.c | 34 +++++++++++++++++++++++++++++-----
>>   xen/arch/arm/xen.lds.S     |  1 +
>>   2 files changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
>> index b6fc0aae07f1..f1cf9252710c 100644
>> --- a/xen/arch/arm/mmu/smpboot.c
>> +++ b/xen/arch/arm/mmu/smpboot.c
>> @@ -9,6 +9,10 @@
>>
>>   #include <asm/setup.h>
>>
>> +/* Override macros from asm/page.h to make them work with mfn_t */
>> +#undef virt_to_mfn
>> +#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
>> +
>>   /*
>>    * Static start-of-day pagetables that we use before the allocators
>>    * are up. These are used by all CPUs during bringup before switching
>> @@ -44,7 +48,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_second);
>>   DEFINE_BOOT_PAGE_TABLES(boot_third, XEN_NR_ENTRIES(2));
>>
>>   /* Non-boot CPUs use this to find the correct pagetables. */
>> -uint64_t init_ttbr;
>> +uint64_t __section(".data.idmap") init_ttbr;
> Do we need to keep the declaration in mmu/mm.h? This variable is only used in this file
> and in assembly, so maybe better to drop declaration and use asmlinkage instead?

I don't see the problem of keeping the declaration in mmu/mm.h. In any 
case, this seems to be unrelated to this patch.

> 
>>
>>   /* Clear a translation table and clean & invalidate the cache */
>>   static void clear_table(void *table)
>> @@ -68,6 +72,27 @@ static void clear_boot_pagetables(void)
>>       clear_table(boot_third);
>>   }
>>
>> +static void set_init_ttbr(lpae_t *root)
>> +{
>> +    /*
>> +     * init_ttbr is part of the identity mapping which is read-only. So
>> +     * We need to re-map the region so it can be updated
> Would you mind fixing s/So We/So we/ and add a full stop after last sentence?

I can do that.

Cheers,

-- 
Julien Grall

On 17/01/2024 13:10, Julien Grall wrote:
> 
> 
> On 17/01/2024 08:30, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 16/01/2024 15:37, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> With the upcoming work to color Xen, the binary will not be anymore
>>> physically contiguous. This will be a problem during boot as the
>>> assembly code will need to work out where each piece of Xen reside.
>>>
>>> An easy way to solve the issue is to have all code/data accessed
>>> by the secondary CPUs while the MMU is off within a single page.
>>>
>>> Right now, init_ttbr is used by secondary CPUs to find there page-tables
>>> before the MMU is on. Yet it is currently in .data which is unlikely
>>> to be within the same page as the rest of the idmap.
>>>
>>> Create a new section .data.idmap that will be used for variables
>>> accessed by the early boot code. The first one is init_ttbr.
>>>
>>> The idmap is currently part of the text section and therefore will
>>> be mapped read-only executable. This means that we need to temporarily
>>> remap init_ttbr in order to update it.
>>>
>>> Introduce a new function set_init_ttbr() for this purpose so the code
>>> is not duplicated between arm64 and arm32.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> with ...
>>
>>> ---
>>>   xen/arch/arm/mmu/smpboot.c | 34 +++++++++++++++++++++++++++++-----
>>>   xen/arch/arm/xen.lds.S     |  1 +
>>>   2 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
>>> index b6fc0aae07f1..f1cf9252710c 100644
>>> --- a/xen/arch/arm/mmu/smpboot.c
>>> +++ b/xen/arch/arm/mmu/smpboot.c
>>> @@ -9,6 +9,10 @@
>>>
>>>   #include <asm/setup.h>
>>>
>>> +/* Override macros from asm/page.h to make them work with mfn_t */
>>> +#undef virt_to_mfn
>>> +#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
>>> +
>>>   /*
>>>    * Static start-of-day pagetables that we use before the allocators
>>>    * are up. These are used by all CPUs during bringup before switching
>>> @@ -44,7 +48,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_second);
>>>   DEFINE_BOOT_PAGE_TABLES(boot_third, XEN_NR_ENTRIES(2));
>>>
>>>   /* Non-boot CPUs use this to find the correct pagetables. */
>>> -uint64_t init_ttbr;
>>> +uint64_t __section(".data.idmap") init_ttbr;
>> Do we need to keep the declaration in mmu/mm.h? This variable is only used in this file
>> and in assembly, so maybe better to drop declaration and use asmlinkage instead?
> 
> I don't see the problem of keeping the declaration in mmu/mm.h. In any
> case, this seems to be unrelated to this patch.
This was just a question about the sense of a declaration that is not used/needed at all.
If you also thought so, it could be done in this patch given that it touches definition.
Since you don't, no problem whatsoever.

~Michal