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

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

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?
> 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).
> +
> +/* 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? If so, please provide the 
32-bit name and add an alias below.
> +
> +#define MPUIR_EL2       HMPUIR
> +#define PRSELR_EL2      HPRSELR
Please add a comment on top explaining why we have the aliases (see in 
cpregs.h).
> +
> +#endif /* CONFIG_ARM_32 */
> +#endif /* __ASM_ARM_MPU_CPREGS_H */
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
Cheers,

--
Julien Grall