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 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.
This bit _PAGE_TRANSIENT is to tell whether the new adding MPU region is 
a fixed one, or a temporary one.
The MPU region created for early FDT is a temporary one, as it will be
unmapped at the end of boot.

> bits [47:6] are for the base address.
>
> If my understanding is correct, then please take it out of this patch 
> and put it in a separate MMU related patch.
> >   #define _PAGE_AI                (7U << _PAGE_AI_BIT)
> >   #define _PAGE_XN                (2U << _PAGE_XN_BIT)
> >   #define _PAGE_RO                (2U << _PAGE_AP_BIT)
> >   #define _PAGE_PRESENT           (1U << _PAGE_PRESENT_BIT)
> > +#define _PAGE_TRANSIENT         (1U << _PAGE_TRANSIENT_BIT)
> >   #define PAGE_AI_MASK(x)         (((x) >> _PAGE_AI_BIT) & 0x7U)
> >   #define PAGE_XN_MASK(x)         (((x) >> _PAGE_XN_BIT) & 0x3U)
> >   #define PAGE_AP_MASK(x)         (((x) >> _PAGE_AP_BIT) & 0x3U)
> >   #define PAGE_RO_MASK(x)         (((x) >> _PAGE_AP_BIT) & 0x2U)
> > +#define PAGE_TRANSIENT_MASK(x)  (((x) >> _PAGE_TRANSIENT_BIT) & 0x1U)
> >   #endif /* CONFIG_HAS_MPU */
> >
> >   /*
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index d35e7e280f..8625066256 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -61,8 +61,17 @@ void flush_page_to_ram(unsigned long mfn, bool 
> > sync_icache)
> >   void * __init early_fdt_map(paddr_t fdt_paddr)
> >   {
> > +#ifndef CONFIG_HAS_MPU
> >       /* We are using 2MB superpage for mapping the FDT */
> >       paddr_t base_paddr = fdt_paddr & SECOND_MASK;
> > +    unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_BLOCK;
> > +    unsigned long base_virt = BOOT_FDT_VIRT_START;
> > +#else
> > +    /* MPU region must be PAGE aligned */
> > +    paddr_t base_paddr = fdt_paddr & PAGE_MASK;
> > +    unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_TRANSIENT;
> > +    unsigned long base_virt = ~0UL;
> > +#endif
> >       paddr_t offset;
> >       void *fdt_virt;
> >       uint32_t size;
> > @@ -79,18 +88,24 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> >       if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN )
> >           return NULL;
> >
> > +#ifndef CONFIG_HAS_MPU
> >       /* The FDT is mapped using 2MB superpage */
> >       BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
> > +#endif
> >
> > -    rc = map_pages_to_xen(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr),
> > -                          SZ_2M >> PAGE_SHIFT,
> > -                          PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> > +    rc = map_pages_to_xen(base_virt, maddr_to_mfn(base_paddr),
> > +                          SZ_2M >> PAGE_SHIFT, flags);
> >       if ( rc )
> >           panic("Unable to map the device-tree.\n");
> >
> >
> > +#ifndef CONFIG_HAS_MPU
> >       offset = fdt_paddr % SECOND_SIZE;
> >       fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;
> > +#else
> > +    offset = fdt_paddr % PAGE_SIZE;
> > +    fdt_virt = (void *)fdt_paddr;
> > +#endif
> >
> >       if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> >           return NULL;
> > @@ -101,10 +116,9 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> >
> >       if ( (offset + size) > SZ_2M )
> >       {
> > -        rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M,
> > +        rc = map_pages_to_xen(base_virt + SZ_2M,
> >                                 maddr_to_mfn(base_paddr + SZ_2M),
> > -                              SZ_2M >> PAGE_SHIFT,
> > -                              PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> > +                              SZ_2M >> PAGE_SHIFT, flags);
> >           if ( rc )
> >               panic("Unable to map the device-tree\n");
> >       }
> > diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> > index 14a1309ca1..f4ce19d36a 100644
> > --- a/xen/arch/arm/mpu/mm.c
> > +++ b/xen/arch/arm/mpu/mm.c
> > @@ -448,6 +448,7 @@ static int xen_mpumap_update_entry(paddr_t base, 
> > paddr_t limit,
> >           /* Set permission */
> >           xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags);
> >           xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);
> > +        xen_mpumap[idx].prlar.reg.tran = PAGE_TRANSIENT_MASK(flags);
> ReferARM DDI 0600A.dID120821 , G1.3.23, PRLAR_EL2, Protection Region 
> Limit Address Register (EL2), I don't seethis bit described anywhere. Do 
> we really need this bit for MPU ?
Yes, It is introduced for software implementation.
You could see commit [PATCH v3 36/52] xen/mpu: implememt ioremap_xxx in 
MPU and commit [PATCH v3 38/52] xen/mpu: map domain page in MPU system 
for better understanding.
> - Ayan
>
> >           write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx);
> >       }
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index 4f7daa7dca..f2715d7cb7 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -216,7 +216,10 @@ SECTIONS
> >     _end = . ;
> >
> >     /* Section for the device tree blob (if any). */
> > -  .dtb : { *(.dtb) } :text
> > +  .dtb : {
> > +      . = ALIGN(PAGE_SIZE);
> > +      *(.dtb)
> > +  } :text
> >
> >     DWARF2_DEBUG_SECTIONS
> >
> > --
> > 2.25.1