RE: [PATCH v4 01/13] xen/arm64: head.S: Introduce enable_{boot,secondary}_cpu_mm

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Ayan,

> -----Original Message----- 
> Hi Henry,
> 
> > 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>
> 
> With two comments
> 
> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> 
> Tested-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>

Thanks, and...

> 
> > +/*
> > + * 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:
> I will prefer "enable_secondary_cpu_mmu" as it is MMU specific. And ...

...actually this as well as...

> > +/*
> > + * 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:
> > + *   lr : Virtual address to return to.
> > + *
> > + * Clobbers x0 - x5
> > + */
> > +enable_boot_cpu_mm:
> 
> prefer "enable_boot_cpu_mmu" as it is MMU specific as well.

...this, are the name suggested by Julien in [1], so probably I will stick
to these names, unless he would prefer the proposed names. I would
personally prefer the names that Julien suggested too, because from
the comments above these two functions, these functions not only
enable the MMU, but also turn on the d-cache, hence a more generic
name (using "mm"), is more appropriate here I guess.

[1] 
https://lore.kernel.org/xen-devel/c10bc254-ad79-dada-d5fb-9ee619934ffb@xxxxxxx/

Kind regards,
Henry

> 
> - Ayan