Re: [PATCH v5 01/13] xen/arm64: head.S: Introduce enable_{boot,secondary}_cpu_mm()

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Aug 21, 2023, at 16:33, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Henry,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> From: Wei Chen <wei.chen@xxxxxxx>
>> At the moment, on MMU system, enable_mmu() will return to an
>> address in the 1:1 mapping, then each path is responsible to
>> switch to virtual runtime mapping. Then remove_identity_mapping()
>> is called on the boot CPU to remove all 1:1 mapping.
>> Since remove_identity_mapping() is not necessary on Non-MMU system,
>> and we also avoid creating empty function for Non-MMU system, trying
>> to keep only one codeflow in arm64/head.S, we move path switch and
>> remove_identity_mapping() in enable_mmu() on MMU system.
>> As the remove_identity_mapping should only be called for the boot
>> CPU only, so we introduce enable_boot_cpu_mm() for boot CPU and
>> enable_secondary_cpu_mm() for secondary CPUs in this patch.
>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
>> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx> > Signed-off-by: Henry Wang 
>> <Henry.Wang@xxxxxxx>
> 
> One remark below. With or without it addressed:
> 
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks, I will take this tag with ...

> 
> [...]
> 
>> +/*
>> + * Enable mm (turn on the data cache and the MMU) for secondary CPUs.
>> + * The function will return to the virtual address provided in LR (e.g. the
>> + * runtime mapping).
>> + *
>> + * Inputs:
>> + *   lr : Virtual address to return to.
>> + *
>> + * Clobbers x0 - x5
>> + */
>> +enable_secondary_cpu_mm:
>> +        mov   x5, lr
>> +
>> +        load_paddr x0, init_ttbr
>> +        ldr   x0, [x0]
>> +
>> +        bl    enable_mmu
>> +        mov   lr, x5
>> +
>> +        /* Return to the virtual address requested by the caller. */
>> +        ret
>> +ENDPROC(enable_secondary_cpu_mm)
> 
> NIT: enable_mmu() could directly return to the virtual address. This would 
> reduce the function to:
> 
> load_paddr x0, init_ttbr
> ldr   x0, [x0]
> 
> /* Return to the virtual address requested by the caller.
> b enable_mmu

…this fixed in v6 since I think there is likely to be a v6, and I think I also 
need
to address the commit message nit pointed out by Jan in the last patch.

Kind regards,
Henry