Re: [PATCH v3 31/52] xen/mpu: make early_fdt_map support in MPU systems

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Julien

On 2023/7/3 17:20, Julien Grall wrote:
> Hi,
>
> On 03/07/2023 06:12, Penny Zheng wrote:
> > Hi,
> >
> >
> > On 2023/6/30 23:02, Julien Grall wrote:
> > > Hi,
> > >
> > > On 30/06/2023 15:42, Ayan Kumar Halder wrote:
> > > > Hi Julien,
> > > >
> > > > On 30/06/2023 12:22, Julien Grall wrote:
> > > > > On 30/06/2023 11:49, Ayan Kumar Halder wrote:
> > > > > > On 30/06/2023 05:07, Penny Zheng wrote:
> > > > > > > Hi,
> > > > > > Hi Penny,
> > > > > > > On 2023/6/30 01:22, Ayan Kumar Halder wrote:
> > > > > > > > On 26/06/2023 04:34, Penny Zheng wrote:
> > > > > > > > > CAUTION: This message has originated from an External Source. 
> > > > > > > > > Please use proper judgment and caution when opening 
> > > > > > > > > attachments, clicking links, or responding to this email.
> > > > > > > > >
> > > > > > > > > In MPU system, MPU memory region is always mapped PAGE_ALIGN, 
> > > > > > > > > so in order to
> > > > > > > > > not access unexpected memory area, dtb section in xen.lds.S 
> > > > > > > > > should be made
> > > > > > > > > page-aligned too.
> > > > > > > > > We add . = ALIGN(PAGE_SIZE); in the head of dtb section to make 
> > > > > > > > > it happen.
> > > > > > > > > In this commit, we map early FDT with a transient MPU memory 
> > > > > > > > > region, as
> > > > > > > > > it will be relocated into heap and unmapped at the end of boot.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > > > > > > > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > > > > > > > ---
> > > > > > > > > v3:
> > > > > > > > > - map the first 2MB. Check the size and then re-map with an 
> > > > > > > > > extra 2MB if needed
> > > > > > > > > ---
> > > > > > > > >   xen/arch/arm/include/asm/arm64/mpu.h |  3 ++-
> > > > > > > > >   xen/arch/arm/include/asm/page.h      |  5 +++++
> > > > > > > > >   xen/arch/arm/mm.c                    | 26 
> > > > > > > > > ++++++++++++++++++++------
> > > > > > > > >   xen/arch/arm/mpu/mm.c                |  1 +
> > > > > > > > >   xen/arch/arm/xen.lds.S               |  5 ++++-
> > > > > > > > >   5 files changed, 32 insertions(+), 8 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
> > > > > > > > > b/xen/arch/arm/include/asm/arm64/mpu.h
> > > > > > > > > index a6b07bab02..715ea69884 100644
> > > > > > > > > --- a/xen/arch/arm/include/asm/arm64/mpu.h
> > > > > > > > > +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> > > > > > > > > @@ -72,7 +72,8 @@ typedef union {
> > > > > > > > >           unsigned long ns:1;     /* Not-Secure */
> > > > > > > > >           unsigned long res:1;    /* Reserved 0 by hardware */
> > > > > > > > >           unsigned long limit:42; /* Limit Address */
> > > > > > > > > -        unsigned long pad:16;
> > > > > > > > > +        unsigned long pad:15;
> > > > > > > > > +        unsigned long tran:1;   /* Transient region */
> > > > > > > > >       } reg;
> > > > > > > > >       uint64_t bits;
> > > > > > > > >   } prlar_t;
> > > > > > > > > diff --git a/xen/arch/arm/include/asm/page.h 
> > > > > > > > > b/xen/arch/arm/include/asm/page.h
> > > > > > > > > index 85ecd5e4de..a434e2205a 100644
> > > > > > > > > --- a/xen/arch/arm/include/asm/page.h
> > > > > > > > > +++ b/xen/arch/arm/include/asm/page.h
> > > > > > > > > @@ -97,19 +97,24 @@
> > > > > > > > >    * [3:4] Execute Never
> > > > > > > > >    * [5:6] Access Permission
> > > > > > > > >    * [7]   Region Present
> > > > > > > > > + * [8]   Transient Region, e.g. MPU memory region is temproraily
> > > > > > > > > + *                              mapped for a short time
> > > > > > > > >    */
> > > > > > > > >   #define _PAGE_AI_BIT            0
> > > > > > > > >   #define _PAGE_XN_BIT            3
> > > > > > > > >   #define _PAGE_AP_BIT            5
> > > > > > > > >   #define _PAGE_PRESENT_BIT       7
> > > > > > > > > +#define _PAGE_TRANSIENT_BIT     8
> > > > > > > > I don't think this is related to MPU. At least when I look at 
> > > > > > > > the bit representation of PRBAR_EL1/2,
> > > > > > > This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 
> > > > > > > register map.
> > > > > > > It is a flag passed to function map_pages_to_xen() to indicate 
> > > > > > > memory
> > > > > > > attributes and permission.
> > > > > > But aren't you writing these flags to PRBAR_EL1/EL2 when you call 
> > > > > > xen_mpumap_update_entry().
> > > > > > In the below snippet of xen_mpumap_update_entry(), IIUC, you are 
> > > > > > writing these flags.
> > > > > >          xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags);
> > > > > >          xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);
> > > > > >
> > > > > >          write_protection_region((const pr_t*)(&xen_mpumap[idx]), 
> > > > > > idx);
> > > > > > Please clarify here.
> > > > > >
> > > > > > In this case, I don't prefer mixing hardware specific bits with 
> > > > > > software only representation for these reasons :-
> > > > > > 1. It makes it confusing and hard to differentiate the hardware 
> > > > > > specific attrbutes from software only.
> > > > > Penny's approach matches what we are doing in the MMU code. We want 
> > > > > to have a way for the caller to pass just set of flags and let the 
> > > > > callee to decide what to do with them.
> > > > > This may be flags converted for HW fields or just used by the logic.
> > > > >
> > > > > If you disagree with this approach, then can you propose a 
> > > > > different way that we can discuss?
> > Thanks ayan for pointing out that RES0 is not suitable for storing 
> > software-only flags, agreed.
> > Then, maybe we should refine the existing "struct pr_t" to store these
> > sw bits, like:
> > ```
> > typedef union {
> >      struct {
> >         uint8_t tran:1; /* Transient region */
> >         uint8_t p2m_type:4; /* Used to store p2m types.*/
> Why do you need the p2m_type?
I inherited the usage from MMU. Right now, in commit "[PATCH v3 46/52] 
xen/mpu: look up entry in p2m table", we introduce the first usage to
tell whether it is a valid P2M MPU memory region. In the future,
we may also use it to check whether it works as RAM region(p2m_ram_rw).

> >      };
> >      uint8_t bits;
> > } pr_flags;
> >
> > /* MPU Protection Region */
> > typedef struct {
> >          prbar_t prbar;
> >          prlar_t prlar;
> >      pr_flags flags;
> > } pr_t;
> > ```
> > The drawback is that, considering the padding, "struct pr_t" expands 
> > from 16 bytes to 24 bytes.
> For clarifications, pr_t is going to be used to create an array in Xen, 
> right? If so, what's the expected size of the array?
Yes, it is going to be an array. And the maximum length is 255.
MPUIR_EL2 identifies the number of regions supported by the EL2 MPU,
which is 8-bits wide.
The original 16 bytes, even with 255 regions at most, will take up
less than 4KB. One page is enough. The following definition could have 
covered all scenarios.
```
/* EL2 Xen MPU memory region mapping table. */
pr_t __aligned(PAGE_SIZE) __section(".data.page_aligned")
     xen_mpumap[ARM_MAX_MPU_MEMORY_REGIONS];
```

Now, FWIT, with 24 bytes, the static xen_mpumap definition only works 
before the heap allocator are up.
When heap is up, we may need to re-allocate xen_mpumap from heap. When
MPUIR_EL2.Region is more than 170, an extra page is needed.

> Cheers,