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 13:03, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> On 14/04/2025 20:54, Ayan Kumar Halder wrote:
> > 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.
> I am not entirely sure this is answering my question. I was asking 
> whether the encoding could be re-used for a non-MPU specific register 
> in the future? IOW, is the encoding reserved for PMSAv8-32 only?
yes, these registers encoding are reserved for PMSAv8-32 only.
> > > > 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 ?
> The goal of the comment is to explain why we are defining AArch64. So 
> I think...
> > > If so, then the comment I should put here
> > /* Aliases of AArch32 names for use in common code */
> >
> > Does this sound correct ?
> it wants to be s/AArch32/AArch64/ in your proposed comment.
ok.

Now I understand. We are using AArch64 names in the common code. :)

- Ayan

> Cheers,