Re: [PATCH v1] xen/arm: AArch32-V8R: Add MPU register definitions

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien,

cc-ed Luca for feedback on specific points.

On 18/04/2025 05:54, Julien Grall wrote:
> Hi Ayan,
>
> On 18/04/2025 00:55, Ayan Kumar Halder wrote:
> > Add the definitions for HPRBAR<0..31>, HPRLAR<0..31> and HPRENR.
> > The definitions are taken from ARM DDI 0568A.c ID110520, E2.2.3 
> > HPRBAR<n>,
> > E2.2.4 HPRENR and E2.2.6 HPRLAR<n>.
> >
> > Introduce pr_t typedef which is a structure having the prbar and 
> > prlar members,
> > each being structured as the registers of the AArch32-V8R architecture.
> > This is the arm32 equivalent of
> > "arm/mpu: Introduce MPU memory region map structure".
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > This patch should be applied after
> > "[PATCH v3 0/7] First chunk for Arm R82 and MPU support" in order to 
> > enable
> > compilation for AArch32.
> >
> >   xen/arch/arm/include/asm/arm32/mpu.h  |  59 +++++++++++
> >   xen/arch/arm/include/asm/mpu.h        |   4 +
> >   xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++
> >   3 files changed, 198 insertions(+)
> >   create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
> >
> > diff --git a/xen/arch/arm/include/asm/arm32/mpu.h 
> > b/xen/arch/arm/include/asm/arm32/mpu.h
> > new file mode 100644
> > index 0000000000..4aabd93479
> > --- /dev/null
> > +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * mpu.h: Arm Memory Protection Unit definitions for aarch64.
> > + */
> > +
> > +#ifndef __ARM_ARM32_MPU_H
> > +#define __ARM_ARM32_MPU_H
> > +
> > +#define XN_EL2_ENABLED  0x1
> I understand the define is introduced in Luca's patch, but this a bit 
> non-descriptive... Can we find a better name? Maybe by adding the name 
> of the register and some documentation?
We can rename this as PRBAR_EL2_XN if this sounds better (cc @Luca) in 
Luca's patch
The description can be

Refer ARM DDI 0568A.c  ID110520 , E2.2.2

HPRBAR [0:1] Execute Never

> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +/* Hypervisor Protection Region Base Address Register */
> > +typedef union {
> > +    struct {
> > +        unsigned int xn:1;       /* Execute-Never */
> > +        unsigned int ap:2;       /* Acess Permission */
> > +        unsigned int sh:2;       /* Sharebility */
> > +        unsigned int res0:1;     /* Reserved as 0 */
> > +        unsigned int base:26;    /* Base Address */
> > +    } reg;
> > +    uint32_t bits;
> > +} prbar_t;
> > +
> > +/* Hypervisor Protection Region Limit Address Register */
> > +typedef union {
> > +    struct {
> > +        unsigned int en:1;     /* Region enable */
> > +        unsigned int ai:3;     /* Memory Attribute Index */
> > +        /*
> > +         * There is no actual ns bit in hardware. It is used here for
> > +         * compatibility with Armr64 code. Thus, we are reusing a 
> > res0 bit for ns.
> typo: Arm64.
Ack
> > +         */
> Hmmmm, this would mean someone may mistakenly set the bit to 0 by 
> mistake. If the field is always meant to be 0 on arm64, then I would 
> consider to name is res0 on arm64 with an explanation.
> This would make clear the bit is not supposed to have a value other 
> than 0.
On Arm64, ns == 0 as it can only support secure mode.

So we can change this on Arm64 as well :-

unsigned int res0:2; /* ns == 0 as only secure mode is supported */

@Luca to clarify

> > +        unsigned int ns:1;     /* Reserved 0 by hardware */
> > +        unsigned int res0:1;   /* Reserved 0 by hardware */
Then, we can change this on Arm32 as well.
> > +        unsigned int limit:26;  /* Limit Address */
> NIT: Can we align the comments?
Ack.
> > +    } reg;
> > +    uint32_t bits;
> > +} prlar_t;
> > +
> > +/* Protection Region */
> > +typedef struct {
> > +    prbar_t prbar;
> > +    prlar_t prlar;
> > +    uint64_t p2m_type;          /* Used to store p2m types. */
> > +} pr_t;
> This looks to be common with arm64. If so, I would prefer if the 
> structure is in a common header.
No, in arm64 this is

typedef struct {
     prbar_t prbar;
     prlar_t prlar;
} pr_t;

The reason being Arm64 uses unused bits (ie 'pad') to store the type.

> > +
> > +#endif /* __ASSEMBLY__ */
> > +
> > +#endif /* __ARM_ARM32_MPU_H */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/arch/arm/include/asm/mpu.h 
> > b/xen/arch/arm/include/asm/mpu.h
> > index 77d0566f97..67127149c0 100644
> > --- a/xen/arch/arm/include/asm/mpu.h
> > +++ b/xen/arch/arm/include/asm/mpu.h
> > @@ -8,8 +8,12 @@
> >     #if defined(CONFIG_ARM_64)
> >   # include <asm/arm64/mpu.h>
> > +#elif defined(CONFIG_ARM_32)
> > +# include <asm/arm32/mpu.h>
> >   #endif
> >   +#define PRENR_MASK  GENMASK(31, 0)
> > +
> >   #define MPU_REGION_SHIFT  6
> >   #define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
> >   #define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
> > diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h 
> > b/xen/arch/arm/include/asm/mpu/cpregs.h
> > index d5cd0e04d5..7cf52aa09a 100644
> > --- a/xen/arch/arm/include/asm/mpu/cpregs.h
> > +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
> > @@ -6,18 +6,153 @@
> >   /* CP15 CR0: MPU Type Register */
> >   #define HMPUIR          p15,4,c0,c0,4
> >   +/* CP15 CR6: Protection Region Enable Register */
> > +#define HPRENR          p15,4,c6,c1,1
> > +
> >   /* CP15 CR6: MPU Protection Region Base/Limit/Select Address 
> > Register */
> >   #define HPRSELR         p15,4,c6,c2,1
> >   #define HPRBAR          p15,4,c6,c3,0
> >   #define HPRLAR          p15,4,c6,c8,1
> >   +/* CP15 CR6: MPU Protection Region Base/Limit Address Register */
> > +#define HPRBAR0         p15,4,c6,c8,0
> > +#define HPRLAR0         p15,4,c6,c8,1
> > +#define HPRBAR1         p15,4,c6,c8,4
> > +#define HPRLAR1         p15,4,c6,c8,5
> > +#define HPRBAR2         p15,4,c6,c9,0
> > +#define HPRLAR2         p15,4,c6,c9,1
> > +#define HPRBAR3         p15,4,c6,c9,4
> > +#define HPRLAR3         p15,4,c6,c9,5
> > +#define HPRBAR4         p15,4,c6,c10,0
> > +#define HPRLAR4         p15,4,c6,c10,1
> > +#define HPRBAR5         p15,4,c6,c10,4
> > +#define HPRLAR5         p15,4,c6,c10,5
> > +#define HPRBAR6         p15,4,c6,c11,0
> > +#define HPRLAR6         p15,4,c6,c11,1
> > +#define HPRBAR7         p15,4,c6,c11,4
> > +#define HPRLAR7         p15,4,c6,c11,5
> > +#define HPRBAR8         p15,4,c6,c12,0
> > +#define HPRLAR8         p15,4,c6,c12,1
> > +#define HPRBAR9         p15,4,c6,c12,4
> > +#define HPRLAR9         p15,4,c6,c12,5
> > +#define HPRBAR10        p15,4,c6,c13,0
> > +#define HPRLAR10        p15,4,c6,c13,1
> > +#define HPRBAR11        p15,4,c6,c13,4
> > +#define HPRLAR11        p15,4,c6,c13,5
> > +#define HPRBAR12        p15,4,c6,c14,0
> > +#define HPRLAR12        p15,4,c6,c14,1
> > +#define HPRBAR13        p15,4,c6,c14,4
> > +#define HPRLAR13        p15,4,c6,c14,5
> > +#define HPRBAR14        p15,4,c6,c15,0
> > +#define HPRLAR14        p15,4,c6,c15,1
> > +#define HPRBAR15        p15,4,c6,c15,4
> > +#define HPRLAR15        p15,4,c6,c15,5
> > +#define HPRBAR16        p15,5,c6,c8,0
> > +#define HPRLAR16        p15,5,c6,c8,1
> > +#define HPRBAR17        p15,5,c6,c8,4
> > +#define HPRLAR17        p15,5,c6,c8,5
> > +#define HPRBAR18        p15,5,c6,c9,0
> > +#define HPRLAR18        p15,5,c6,c9,1
> > +#define HPRBAR19        p15,5,c6,c9,4
> > +#define HPRLAR19        p15,5,c6,c9,5
> > +#define HPRBAR20        p15,5,c6,c10,0
> > +#define HPRLAR20        p15,5,c6,c10,1
> > +#define HPRBAR21        p15,5,c6,c10,4
> > +#define HPRLAR21        p15,5,c6,c10,5
> > +#define HPRBAR22        p15,5,c6,c11,0
> > +#define HPRLAR22        p15,5,c6,c11,1
> > +#define HPRBAR23        p15,5,c6,c11,4
> > +#define HPRLAR23        p15,5,c6,c11,5
> > +#define HPRBAR24        p15,5,c6,c12,0
> > +#define HPRLAR24        p15,5,c6,c12,1
> > +#define HPRBAR25        p15,5,c6,c12,4
> > +#define HPRLAR25        p15,5,c6,c12,5
> > +#define HPRBAR26        p15,5,c6,c13,0
> > +#define HPRLAR26        p15,5,c6,c13,1
> > +#define HPRBAR27        p15,5,c6,c13,4
> > +#define HPRLAR27        p15,5,c6,c13,5
> > +#define HPRBAR28        p15,5,c6,c14,0
> > +#define HPRLAR28        p15,5,c6,c14,1
> > +#define HPRBAR29        p15,5,c6,c14,4
> > +#define HPRLAR29        p15,5,c6,c14,5
> > +#define HPRBAR30        p15,5,c6,c15,0
> > +#define HPRLAR30        p15,5,c6,c15,1
> > +#define HPRBAR31        p15,5,c6,c15,4
> > +#define HPRLAR31        p15,5,c6,c15,5
> NIT: Is there any way we could generate the values using macros?
This looks tricky. So I will prefer to keep this as it is.
> > +
> >   /* Aliases of AArch64 names for use in common code */
> >   #ifdef CONFIG_ARM_32
> >   /* Alphabetically... */
> >   #define MPUIR_EL2       HMPUIR
> >   #define PRBAR_EL2       HPRBAR
> > +#define PRBAR0_EL2      HPRBAR0
> AFAIU, the alias will be mainly used in the macro generate
> the switch. Rather than open-coding all the PR*AR_EL2, can we
> provide two macros PR{B, L}AR_N that will be implemented as
> HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?
Yes , we can have

#define PR{B,L}AR_EL2_(n)          HPR{B,L}AR##n for arm32

#define PR{B,L}AR_EL2_(n)          PR{B,L}AR##n##_EL2

I will send a v2 with these changes. Please have a look at that.

- Ayan

> >   #define PRSELR_EL2      HPRSELR
> > +
> >   #endif /* CONFIG_ARM_32 */
> >     #endif /* __ARM_MPU_CPREGS_H */
> Cheers,