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

[Penny Zheng <penny.zheng@xxxxxxx>]

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.*/
    };
    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.
Wdyt?

> > In MMU, I could see "struct lpae_pt_t{ .avail }" is used for reference 
> > count.
> > Something like this might look better (where the hardware specific 
> > bits are not reused for sw logic)
> > struct {
> >
> >            struct __packed {
> >
> >                   ... /* the existing bits as they are */
> >
> >            } lpae_pt_t;
> >
> >            unsigned int ref_count;
> >
> > } p2m_entry
> >  > And use "p2m_entry.ref_count" for the reference counting.
> So where would you store 'ref_count'? In the existing approach, this is 
> store in the PTE and that's fine because the bits 58:55 are reserved for 
> software use.
> This is right in the middle of the PTE. So we have no other choice other 
> than describing right in the middle of lpae_pt_t.
> Cheers,