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

[Julien Grall <julien@xxxxxxx>]

On 07/08/2023 12:43, Ayan Kumar Halder wrote:
> 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.
For this series, it is quite important to look at the end result. In 
this case, the MMU logic will be moved in its own file. 
enable_boot_cpu_mm() and enable_second_cpu_mm() will be implemented 
differently between the MMU and MPU to avoid #ifdeferay in head.S.
Cheers,

--
Julien Grall