Re: [PATCH v1 repost 2/4] arm/smpboot: Move smp_up_cpu to a new section .data.idmap

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 16/01/2024 15:37, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> 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, smp_up_cpu is used by secondary CPUs to wait there turn for
s/there/their ?

> booting 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.
> 
> Move smp_up_cpu to the recently create section .data.idmap. The idmap is
s/create/created

> currently part of the text section and therefore will be mapped read-onl
s/onl/only

> executable. This means that we need to temporarily remap
> smp_up_cpu in order to update it.
> 
> Introduce a new function set_smp_up_cpu() for this purpose so the code
> is not duplicated between when opening and closing the gate.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

> ---
>  xen/arch/arm/smpboot.c | 36 +++++++++++++++++++++++++++++++-----
>  1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 7110bc11fc05..8d508a1bb258 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -29,6 +29,10 @@
>  #include <asm/psci.h>
>  #include <asm/acpi.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))
> +
>  cpumask_t cpu_online_map;
>  cpumask_t cpu_present_map;
>  cpumask_t cpu_possible_map;
> @@ -56,7 +60,7 @@ struct init_info init_data =
>  };
> 
>  /* Shared state for coordinating CPU bringup */
> -unsigned long smp_up_cpu = MPIDR_INVALID;
> +unsigned long __section(".data.idmap") smp_up_cpu = MPIDR_INVALID;
>  /* Shared state for coordinating CPU teardown */
>  static bool cpu_is_dead;
> 
> @@ -429,6 +433,28 @@ void stop_cpu(void)
>          wfi();
>  }
> 
> +static void set_smp_up_cpu(unsigned long mpidr)
> +{
> +    /*
> +     * smp_up_cpu 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(&smp_up_cpu));
> +
> +    ptr += PAGE_OFFSET(&smp_up_cpu);
> +
> +    *(unsigned long *)ptr = mpidr;
> +
> +    /*
> +     * init_ttbr will be accessed with the MMU off, so ensure the update
smp_up_cpu instead of init_ttbr

> +     * is visible by cleaning the cache.
> +     */
> +    clean_dcache(ptr);
> +
> +    unmap_domain_page(ptr);
> +
> +}
> +
>  int __init cpu_up_send_sgi(int cpu)
>  {
>      /* We don't know the GIC ID of the CPU until it has woken up, so just
> @@ -460,8 +486,7 @@ int __cpu_up(unsigned int cpu)
>      init_data.cpuid = cpu;
> 
>      /* Open the gate for this CPU */
> -    smp_up_cpu = cpu_logical_map(cpu);
> -    clean_dcache(smp_up_cpu);
> +    set_smp_up_cpu(cpu_logical_map(cpu));
> 
>      rc = arch_cpu_up(cpu);
> 
> @@ -497,8 +522,9 @@ int __cpu_up(unsigned int cpu)
>       */
>      init_data.stack = NULL;
>      init_data.cpuid = ~0;
> -    smp_up_cpu = MPIDR_INVALID;
> -    clean_dcache(smp_up_cpu);
> +
> +    set_smp_up_cpu(MPIDR_INVALID);
> +
>      arch_cpu_up_finish();
> 
>      if ( !cpu_online(cpu) )
> --
> 2.40.1
> 

~Michal