Re: [XEN v4 1/2] xen/arm32: head: Introduce enable_{boot,secondary}_cpu_mm()

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 17/11/2023 16:31, Ayan Kumar Halder wrote:
> All the MMU related functionality have been clubbed together in
> enable_boot_cpu_mm() for booting primary cpu and enable_secondary_cpu_mm() for
> booting secondary cpus.
> This is done in preparation for moving the code related to MMU in MMU specific
> file and in order to support non MMU cpus in future.
> 
> This is based on d2f8df5b3ede ("xen/arm64: head.S: Introduce 
> enable_{boot,secondary}_cpu_mm()").
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
> 
> Changes from :-
> 
> v1 - 1. Added a proper commit message.
> 2. Some style and other fixes suggested in v1. 
> 
> v2 - 1. Updated the comment on top of enable_boot_cpu_mm() and
> enable_secondary_cpu_mm() ie mentioned the input and output registers.
> 2. Updated the comment inside enable_boot_cpu_mm().
> 
> v3 - 1. No changes.
> 
>  xen/arch/arm/arm32/head.S | 102 ++++++++++++++++++++++++++++++--------
>  1 file changed, 80 insertions(+), 22 deletions(-)
> 
[...]

> +
> +/*
> + * Enable mm (turn on the data cache and the MMU) for the boot CPU.
> + * The function will return to the virtual address provided in LR (e.g. the
> + * runtime mapping).
> + *
> + * Inputs:
> + *   r9 : paddr(start)
> + *   r10: phys offset
> + *   lr : Virtual address to return to.
> + *
> + * Output:
> + *   r12: Was a temporary mapping created?
> + *
> + * Clobbers r0 - r6
> + */
> +enable_boot_cpu_mm:
> +        mov   r6, lr
> +
> +        bl    create_page_tables
> +
> +        /* Address in the runtime mapping to jump to after the MMU is 
> enabled */
> +        mov_w lr, 1f
> +        b     enable_mmu
> +1:
> +        /*
> +         * Prepare the fixmap. The function will return to the virtual 
> address
> +         * requested by the caller.
> +         */
It seems like you did not handle my remark from v3. This comment should be 
moved above branch
to setup_fixmap.

> +        mov   lr, r6
> +
> +        b     setup_fixmap
> +ENDPROC(enable_boot_cpu_mm)
> +
>  /*
>   * Remove the 1:1 map from the page-tables. It is not easy to keep track
>   * where the 1:1 map was mapped, so we will look for the top-level entry


~Michal