Re: [PATCH v6 2/3] xen/arm32: Create the same boot-time MPU regions as arm64

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 14/04/2025 12:21, Julien Grall wrote:
> Hi Ayan,
Hi Julien,

A few clarifications.

> On 11/04/2025 20:04, Ayan Kumar Halder wrote:
> > diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h 
> > b/xen/arch/arm/include/asm/arm32/sysregs.h
> > index 22871999af..8d7b95d982 100644
> > --- a/xen/arch/arm/include/asm/arm32/sysregs.h
> > +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
> > @@ -20,6 +20,15 @@
> >    * uses r0 as a placeholder register. */
> >   #define CMD_CP32(name...)      "mcr " __stringify(CP32(r0, name)) ";"
> >   +#define REGION_TEXT_PRBAR       0x18    /* SH=11 AP=10 XN=0 */
> > +#define REGION_RO_PRBAR         0x1D    /* SH=11 AP=10 XN=1 */
> > +#define REGION_DATA_PRBAR       0x19    /* SH=11 AP=00 XN=1 */
> > +#define REGION_DEVICE_PRBAR     0x11    /* SH=10 AP=00 XN=1 */
> > +
> > +#ifdef __ASSEMBLY__
> > +#define WRITE_SYSREG_ASM(v, name) mcr CP32(v, name)
> > +#endif /* __ASSEMBLY__ */
> > +
> >   #ifndef __ASSEMBLY__
> >     /* C wrappers */
> > diff --git a/xen/arch/arm/include/asm/cpregs.h 
> > b/xen/arch/arm/include/asm/cpregs.h
> > index aec9e8f329..a7503a190f 100644
> > --- a/xen/arch/arm/include/asm/cpregs.h
> > +++ b/xen/arch/arm/include/asm/cpregs.h
> > @@ -1,6 +1,8 @@
> >   #ifndef __ASM_ARM_CPREGS_H
> >   #define __ASM_ARM_CPREGS_H
> >   +#include <asm/mpu/cpregs.h>
> Just to confirm, the CP registers used by the MPU will never be used 
> for an other purpose on MMU systems, is that correct?
Yes, this is correct. The registers are specific to PMSAv8-32 which will 
not be present on MMU systems.
> > diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h 
> > b/xen/arch/arm/include/asm/mpu/cpregs.h
> > new file mode 100644
> > index 0000000000..e2f3b2264c
> > --- /dev/null
> > +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef __ASM_ARM_MPU_CPREGS_H
> > +#define __ASM_ARM_MPU_CPREGS_H
> > +
> > +#ifdef CONFIG_ARM_32
> I am not sure I agree with the placement of this ifdef. Is the 
> implication that 32-bit domain will never be supported on arm64? If 
> not, then everything but the aliases should be available for 64-bit 
> (like we already do in asm/cpregs.h).
Yes, I will enclose the alias only.
> > +
> > +/* CP15 CR0: MPU Type Register */
> > +#define HMPUIR          p15,4,c0,c0,4
> > +
> > +/* CP15 CR6: MPU Protection Region Base/Limit/Select Address 
> > Register */
> > +#define HPRSELR         p15,4,c6,c2,1
> > +#define PRBAR_EL2       p15,4,c6,c3,0
> > +#define PRLAR_EL2       p15,4,c6,c8,1
> I am a little bit surprised the registers have _EL2 in their name. By 
> any chance are you using the aarch64 naming? 
yes.
> If so, please provide the 32-bit name and add an alias below.
yes, sorry this is a bit mixed up. I did not understand the purpose for 
defining alias , so I used the common name.
I will use HPRBAR and HPRLAR here and ....

> > +
> > +#define MPUIR_EL2       HMPUIR
> > +#define PRSELR_EL2      HPRSELR
#define PRBAR_EL2 HPRBAR

#define PRLAR_EL2 HPRLAR

> Please add a comment on top explaining why we have the aliases (see in 
> cpregs.h).
Actually, that comment (in asm/cpregs.h) did not make sense to me

"/* Aliases of AArch64 names for use in common code when building for 
AArch32 */"
Do you mean the common code is used for building both AArch64 and AArch32 ?

If so, then the comment I should put here

/* Aliases of AArch32 names for use in common code */

Does this sound correct ?

- Ayan

> > +
> > +#endif /* CONFIG_ARM_32 */
> > +#endif /* __ASM_ARM_MPU_CPREGS_H */
> > +
> > +/*
> > + * Local variables:
> > + * mode: ASM
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> Cheers,