Re: [PATCH v1 1/3] arm/mpu: Introduce MPU memory region map structure

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 04/06/2025 20:19, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
> > On 4 Jun 2025, at 18:43, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> wrote:
> >
> > Introduce pr_t typedef which is a structure having the prbar and prlar members,
> > each being structured as the registers of the AArch32 Armv8-R architecture.
> >
> > Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
> > BASE or LIMIT bitfields in prbar or prlar respectively.
> >
> > Move pr_t definition to common code.
> > Also, enclose xn_0 within ARM64 as it is not present for ARM32.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
> > xen/arch/arm/include/asm/arm64/mpu.h |  6 ------
> > xen/arch/arm/include/asm/mpu.h       |  6 ++++++
> > xen/arch/arm/mpu/mm.c                |  2 ++
> > 4 files changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/arm32/mpu.h 
> > b/xen/arch/arm/include/asm/arm32/mpu.h
> > index f0d4d4055c..ae3b661fde 100644
> > --- a/xen/arch/arm/include/asm/arm32/mpu.h
> > +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> > @@ -5,11 +5,31 @@
> >
> > #ifndef __ASSEMBLY__
> >
> > -/* MPU Protection Region */
> > -typedef struct {
> > -    uint32_t prbar;
> > -    uint32_t prlar;
> > -} pr_t;
> > +#define MPU_REGION_RES0       0x0
> > +
> > +/* Hypervisor Protection Region Base Address Register */
> > +typedef union {
> > +    struct {
> > +        unsigned int xn:1;       /* Execute-Never */
> > +        unsigned int ap_0:1;     /* Acess Permission */
> > +        unsigned long ro:1;      /* Access Permission AP[1] */
> > +        unsigned int sh:2;       /* Sharebility */
> Typo: Sharebility -> Shareability.
Ack
> Also, from the patch the comments feels not aligned, is that the case?
When you apply the patch, the comments look aligned (in the codebase). 
Does it look the same for you ?
> > +        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 */
> > +        unsigned int res0:2;   /* Reserved 0 by hardware */
> > +        unsigned int limit:26; /* Limit Address */
> > +    } reg;
> > +    uint32_t bits;
> > +} prlar_t;
> >
> > #endif /* __ASSEMBLY__ */
> >
> > diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
> > b/xen/arch/arm/include/asm/arm64/mpu.h
> > index f0ce344e78..df46774dcb 100644
> > --- a/xen/arch/arm/include/asm/arm64/mpu.h
> > +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> > @@ -34,12 +34,6 @@ typedef union {
> >      uint64_t bits;
> > } prlar_t;
> >
> > -/* MPU Protection Region */
> > -typedef struct {
> > -    prbar_t prbar;
> > -    prlar_t prlar;
> > -} pr_t;
> I’m not sure I would do this, at some point there will be the transient flags 
> and p2m type
> and we know arm32 will need to store them in pr_t as additional members, instead
> arm64 will store them as part of prbar/prlar using the not used space (without 
> writing them
> in the HW registers)
Ah I see. Ok, I will keep them separate as it is.

- Ayan

> > -
> > #endif /* __ASSEMBLY__ */
> >
> > #endif /* ARM_ARM64_MPU_H */
> > diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> > index 8f06ddac0f..c8573a5980 100644
> > --- a/xen/arch/arm/include/asm/mpu.h
> > +++ b/xen/arch/arm/include/asm/mpu.h
> > @@ -25,6 +25,12 @@
> >
> > #ifndef __ASSEMBLY__
> >
> > +/* MPU Protection Region */
> > +typedef struct {
> > +    prbar_t prbar;
> > +    prlar_t prlar;
> > +} pr_t;
> > +
> > #ifdef CONFIG_ARM_64
> > /*
> >   * Set base address of MPU protection region.
> > diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> > index 86fbe105af..2fb6b822c6 100644
> > --- a/xen/arch/arm/mpu/mm.c
> > +++ b/xen/arch/arm/mpu/mm.c
> > @@ -167,7 +167,9 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int 
> > flags)
> >      /* Build up value for PRBAR_EL2. */
> >      prbar = (prbar_t) {
> >          .reg = {
> > +#ifdef CONFIG_ARM64
> >              .xn_0 = 0,
> > +#endif
> >              .xn = PAGE_XN_MASK(flags),
> >              .ap_0 = 0,
> >              .ro = PAGE_RO_MASK(flags)
> > --
> > 2.25.1
> Cheers,
> Luca