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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 07/08/2023 12:35, Henry Wang wrote:
> 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/
This is fine. My concern is minor.

If this file is going to contain MPU specific logic as well in future, 
then suffixing a *_mmu might help to distinguish the two.
- Ayan

> Kind regards,
> Henry
>
> > - Ayan