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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien/Henry,

Thanks for the explanation.

On 07/08/2023 12:47, Julien Grall wrote:
> 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.
Makes sense. I am happy with it.

- Ayan

> Cheers,