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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Michal,

On 05/06/2025 08:06, Orzel, Michal wrote:
> On 04/06/2025 19:43, Ayan Kumar Halder 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
> The name of the macro does not make a lot of sense in AArch32 context
> and can create a confusion for the reader.
I know, but I want to avoid introducing ifdef or have separate 
implementation (for arm32 and arm64) for the following
Refer xen/arch/arm/include/asm/mpu.h

static inline void pr_set_base(pr_t *pr, paddr_t base)
{
    pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >> MPU_REGION_SHIFT);
}

Let me know your preference.

> > +
> > +/* Hypervisor Protection Region Base Address Register */
> > +typedef union {
> > +    struct {
> > +        unsigned int xn:1;       /* Execute-Never */
> > +        unsigned int ap_0:1;     /* Acess Permission */
> If you write AP[1] below, I would expect here AP[0]
Again same reason as before, let me know if you want to have additional 
ifdef in pr_of_addr() or separate functions for arm32 and arm64
pr_t pr_of_addr(...)

{...

    prbar = (prbar_t) {
        .reg = {
#ifdef CONFIG_ARM64
            .xn_0 = 0,
#endif
            .xn = PAGE_XN_MASK(flags),
            .ap_0 = 0,
            .ro = PAGE_RO_MASK(flags)
        }};

...

}

> Also s/acess/access/
Ack.
> > +        unsigned long ro:1;      /* Access Permission AP[1] */
> > +        unsigned int sh:2;       /* Sharebility */
> > +        unsigned int res0:1;     /* Reserved as 0 */
> Here your comment for RES0 (which IMO could be just RES0) does not match...
I will remove the comment here and ..
> > +        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 */
> this one. I don't think you need to explain what RES0 means.
here.

- Ayan

> ~Michal