Re: [PATCH 2/5] xen/arm: mpu: Enclose access to MMU specific registers under CONFIG_MMU (arm32)

[Julien Grall <julien@xxxxxxx>]

Hi,

On 06/02/2025 14:48, Luca Fancellu wrote:
> > On 4 Feb 2025, at 19:23, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> wrote:
> >
> > All the EL2 MMU specific registers in head.S are enclosed within CONFIG_MMU.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > xen/arch/arm/arm32/head.S | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> > index 4ff5c220bc..1d0f84b18f 100644
> > --- a/xen/arch/arm/arm32/head.S
> > +++ b/xen/arch/arm/arm32/head.S
> > @@ -224,6 +224,7 @@ cpu_init_done:
> >          mcr   CP32(r0, HMAIR0)
> >          mcr   CP32(r1, HMAIR1)
> >
> > +#ifdef CONFIG_MMU
> >          /*
> >           * Set up the HTCR:
> >           * PT walks use Inner-Shareable accesses,
> > @@ -232,6 +233,7 @@ cpu_init_done:
> >           */
> >          mov_w r0, 
> > (TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
> >          mcr   CP32(r0, HTCR)
> > +#endif
> I was wondering if here it was better, for readability, to have this part 
> defined in the arm32/mmu/head.S and
> arm32/mpu/head.S could have implemented a stub, maybe the maintainer could help 
> with that.
The current logic is a bit odd because the MM specific registers are 
initialized in two different places (cpu_init and enable_mmu).
It would be better if we have a single place. So I would move setting 
HTCR (and event HMAIR{0,1} even if it means duplication) to enable_mmu.
The same would apply for arm64.

Cheers,

--
Julien Grall