Re: [PATCH v3 5/7] arm/mpu: Introduce MPU memory mapping flags

[Julien Grall <julien@xxxxxxx>]

On 16/04/2025 17:52, Luca Fancellu wrote:
> Hi Julien,
Hi Luca,

Sorry for the late answer.

> > On 14 Apr 2025, at 12:48, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi Luca,
> >
> > On 11/04/2025 23:56, Luca Fancellu wrote:
> > > Introduce the MPU memory mapping flags in asm/page.h.
> > > Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > > ---
> > >   xen/arch/arm/include/asm/page.h | 25 +++++++++++++++++++++++++
> > >   1 file changed, 25 insertions(+)
> > > diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
> > > index 69f817d1e68a..22f7d2c6cb30 100644
> > > --- a/xen/arch/arm/include/asm/page.h
> > > +++ b/xen/arch/arm/include/asm/page.h
> > > @@ -62,6 +62,7 @@
> > >     #define MAIRVAL (MAIR1VAL << 32 | MAIR0VAL)
> > >   +#ifdef CONFIG_MMU
> > >   /*
> > >    * Layout of the flags used for updating the hypervisor page tables
> > >    *
> > > @@ -90,6 +91,30 @@
> > >   #define _PAGE_CONTIG_BIT    8
> > >   #define _PAGE_CONTIG        (1U << _PAGE_CONTIG_BIT)
> > >   +#else /* !CONFIG_MMU */
> > > +
> > > +/*
> > > + * Layout of the flags used for updating MPU memory region attributes
> > > + * [0:2] Memory attribute Index
> > > + * [3:4] Execute Never
> > > + * [5:6] Access Permission
> > I am rather confused why we are splitting Execute Never from the Access 
> > Permission. I guess you tried to match the HW, but it also means we need to 
> > duplicate a lot of define between the MMU and MPU code.
> >
> > Instead, I would rather try to re-use the existing ones and ignore the ones we 
> > don't need (e.g. BLOCK_BIT and CONTIG).
> I’m having a bit of trouble understanding the MMU part:
>
> /*
> * Layout of the flags used for updating the hypervisor page tables
> *
> * [0:2] Memory Attribute Index
> * [3:4] Permission flags
> * [5] Page present
> * [6] Only populate page tables
> * [7] Superpage mappings is allowed
> * [8] Set contiguous bit (internal flag)
> */
> #define PAGE_AI_MASK(x) ((x) & 0x7U)
>
> #define _PAGE_XN_BIT 3
> #define _PAGE_RO_BIT 4
> #define _PAGE_XN (1U << _PAGE_XN_BIT)
> #define _PAGE_RO (1U << _PAGE_RO_BIT)
> #define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
> #define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
>
> I can see on the MMU basically AP[1] means RO or not, AP[0] means XN or not, 
> from the arm spec
> (verison L.a, D8.4.2.1.1 Stage 2 data accesses using Direct permissions) I can 
> see stage 2 AP[1:0] is:
>   - 00: no access
>   - 01: RO
>   - 10: WO
>   - 11: RW
This is describing the stage-2 S2AP field. Whereas the flags above are 
only used for EL2 stage-1. So you want to look at the table
Table D8-62. That said...

> So:
>   - 00: read-only is zero and execution is allowed
>   - 01: read-only is zero and execution is not allowed
>   - 10: read-only is one (??), execution is allowed
>   - 11: read-only is one (??), execution is not allowed
... part of flags doesn't directly correspond to AP. Instead, we 
individually set AP[2] (called ro for Xen), AP[1] is RES1 (as we have 
only one exception level). And then 'xn' is set separately.
See mfn_to_xen_entry() and xen_pt_update_entry():

        /* Set permission */
        pte.pt.ro = PAGE_RO_MASK(flags);
        pte.pt.xn = PAGE_XN_MASK(flags);

Let me know if it makes sense.

Cheers,

--
Julien Grall