RE: [PATCH v2 20/40] xen/mpu: plump early_fdt_map in MPU systems

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Monday, February 6, 2023 6:11 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v2 20/40] xen/mpu: plump early_fdt_map in MPU
> systems
> 
> Hi,
> 
> A few more remarks.
> 
> On 13/01/2023 05:28, Penny Zheng wrote:
> > In MPU system, device tree binary can be packed with Xen image through
> > CONFIG_DTB_FILE, or provided by bootloader through x0.
> >
> > In MPU system, each section in xen.lds.S is PAGE_SIZE aligned.
> > So in order to not overlap with the previous BSS section, dtb section
> > 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 at
> > rear with REGION_HYPERVISOR_BOOT.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/arch/arm/include/asm/arm64/mpu.h |  5 +++
> >   xen/arch/arm/mm_mpu.c                | 63 +++++++++++++++++++++++++---
> >   xen/arch/arm/xen.lds.S               |  5 ++-
> >   3 files changed, 67 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/arm64/mpu.h
> > b/xen/arch/arm/include/asm/arm64/mpu.h
> > index fcde6ad0db..b85e420a90 100644
> > --- a/xen/arch/arm/include/asm/arm64/mpu.h
> > +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> > @@ -45,18 +45,22 @@
> >    * [3:4] Execute Never
> >    * [5:6] Access Permission
> >    * [7]   Region Present
> > + * [8]   Boot-only Region
> >    */
> >   #define _REGION_AI_BIT            0
> >   #define _REGION_XN_BIT            3
> >   #define _REGION_AP_BIT            5
> >   #define _REGION_PRESENT_BIT       7
> > +#define _REGION_BOOTONLY_BIT      8
> >   #define _REGION_XN                (2U << _REGION_XN_BIT)
> >   #define _REGION_RO                (2U << _REGION_AP_BIT)
> >   #define _REGION_PRESENT           (1U << _REGION_PRESENT_BIT)
> > +#define _REGION_BOOTONLY          (1U << _REGION_BOOTONLY_BIT)
> >   #define REGION_AI_MASK(x)         (((x) >> _REGION_AI_BIT) & 0x7U)
> >   #define REGION_XN_MASK(x)         (((x) >> _REGION_XN_BIT) & 0x3U)
> >   #define REGION_AP_MASK(x)         (((x) >> _REGION_AP_BIT) & 0x3U)
> >   #define REGION_RO_MASK(x)         (((x) >> _REGION_AP_BIT) & 0x2U)
> > +#define REGION_BOOTONLY_MASK(x)   (((x) >> _REGION_BOOTONLY_BIT)
> & 0x1U)
> >
> >   /*
> >    * _REGION_NORMAL is convenience define. It is not meant to be used
> > @@ -68,6 +72,7 @@
> >   #define REGION_HYPERVISOR_RO
> (_REGION_NORMAL|_REGION_XN|_REGION_RO)
> >
> >   #define REGION_HYPERVISOR         REGION_HYPERVISOR_RW
> > +#define REGION_HYPERVISOR_BOOT
> (REGION_HYPERVISOR_RW|_REGION_BOOTONLY)
> >
> >   #define INVALID_REGION            (~0UL)
> >
> > diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c index
> > 08720a7c19..b34dbf4515 100644
> > --- a/xen/arch/arm/mm_mpu.c
> > +++ b/xen/arch/arm/mm_mpu.c
> > @@ -20,11 +20,16 @@
> >    */
> >
> >   #include <xen/init.h>
> > +#include <xen/libfdt/libfdt.h>
> >   #include <xen/mm.h>
> >   #include <xen/page-size.h>
> > +#include <xen/pfn.h>
> > +#include <xen/sizes.h>
> >   #include <xen/spinlock.h>
> >   #include <asm/arm64/mpu.h>
> > +#include <asm/early_printk.h>
> >   #include <asm/page.h>
> > +#include <asm/setup.h>
> >
> >   #ifdef NDEBUG
> >   static inline void
> > @@ -62,6 +67,8 @@ uint64_t __ro_after_init max_xen_mpumap;
> >
> >   static DEFINE_SPINLOCK(xen_mpumap_lock);
> >
> > +static paddr_t dtb_paddr;
> > +
> >   /* Write a MPU protection region */
> >   #define WRITE_PROTECTION_REGION(sel, pr, prbar_el2, prlar_el2) ({       \
> >       uint64_t _sel = sel;                                                \
> > @@ -403,7 +410,16 @@ static int xen_mpumap_update_entry(paddr_t
> base,
> > paddr_t limit,
> >
> >           /* During boot time, the default index is next_fixed_region_idx. 
> > */
> >           if ( system_state <= SYS_STATE_active )
> > -            idx = next_fixed_region_idx;
> > +        {
> > +            /*
> > +             * If it is a boot-only region (i.e. region for early FDT),
> > +             * it shall be added from the tail for late init re-organizing
> > +             */
> > +            if ( REGION_BOOTONLY_MASK(flags) )
> > +                idx = next_transient_region_idx;
> > +            else
> > +                idx = next_fixed_region_idx;
> > +        }
> >
> >           xen_mpumap[idx] = pr_of_xenaddr(base, limit - 1,
> REGION_AI_MASK(flags));
> >           /* Set permission */
> > @@ -465,14 +481,51 @@ int map_pages_to_xen(unsigned long virt,
> >                                mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags);
> >   }
> >
> > -/* TODO: Implementation on the first usage */ -void
> > dump_hyp_walk(vaddr_t addr)
> > +void * __init early_fdt_map(paddr_t fdt_paddr)
> >   {
> > +    void *fdt_virt;
> > +    uint32_t size;
> > +
> > +    /*
> > +     * Check whether the physical FDT address is set and meets the
> minimum
> > +     * alignment requirement. Since we are relying on MIN_FDT_ALIGN to
> be at
> > +     * least 8 bytes so that we always access the magic and size fields
> > +     * of the FDT header after mapping the first chunk, double check if
> > +     * that is indeed the case.
> > +     */
> > +     BUILD_BUG_ON(MIN_FDT_ALIGN < 8);
> > +     if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN )
> > +         return NULL;
> > +
> > +    dtb_paddr = fdt_paddr;
> > +    /*
> > +     * In MPU system, device tree binary can be packed with Xen image
> > +     * through CONFIG_DTB_FILE, or provided by bootloader through x0.
> 
> The behavior you describe is not specific to the MPU system. I also don't
> quite understand how describing the method to pass the DT actually matters
> here.
> 
> > +     * Map FDT with a transient MPU memory region of MAX_FDT_SIZE.
> > +     * After that, we can do some magic check.
> > +     */
> > +    if ( map_pages_to_xen(round_pgdown(fdt_paddr),
> 
> I haven't looked at the rest of the series. But from here, it seems a bit 
> strange
> to use map_pages_to_xen() because the virt and the phys should be the
> same.
> 

Hmm, t thought map_pages_to_xen, is to set up memory mapping for access.
In MPU, we also need to set up a MPU memory region for the FDT, even without
virt-to-phys conversion 

> Do you plan to share some code where map_pages_to_xen() will be used?
> 

Each time, in C boot-time, we build up a new MPU memory region for stage 1
EL2 memory mapping, we use this map_pages_to_xen to complete.
I think it has the same effect as it has in MMU, other than MMU sets up
virt-to-phys memory mapping and MPU always sets up identity memory mapping.

> > +                          maddr_to_mfn(round_pgdown(fdt_paddr)),
> > +                          round_pgup(MAX_FDT_SIZE) >> PAGE_SHIFT,
> 
> This will not work properly is the Device-Tree is MAX_FDT_SIZE (could
> already be page-aligned) but the start address is not page-aligned.
> 
> But I think trying to map the maximum size from the start could potentially
> result to some issue. Below the excerpt from the Image
> documentation:
> 
> "The device tree blob (dtb) must be placed on an 8-byte boundary and must
> not exceed 2 megabytes in size. Since the dtb will be mapped cacheable using
> blocks of up to 2 megabytes in size, it must not be placed within any 2M
> region which must be mapped with any specific attributes."
> 
> So it would be better to map the first 2MB. Check the size and then re-map
> with an extra 2MB if needed.
> 

Oh, under special circumstances, the current implementation will map exceeding 
2MB.
Thanks for explanation!
I will map as you suggested.

> > +                          REGION_HYPERVISOR_BOOT) ) > +        
> > panic("Unable to
> map the device-tree.\n");
> > +
> > +    /* VA == PA */
> 
> I have seen in a few places where you add a similar comment. But I am not
> sure to understand how this help to describe the implementation of
> maddr_to_virt().
> 
> > +    fdt_virt = maddr_to_virt(fdt_paddr);
> > +
> > +    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> > +        return NULL;
> > +
> > +    size = fdt_totalsize(fdt_virt);
> > +    if ( size > MAX_FDT_SIZE )
> > +        return NULL;
> > +
> > +    return fdt_virt;
> >   }
> >
> > -void * __init early_fdt_map(paddr_t fdt_paddr)
> > +/* TODO: Implementation on the first usage */ void
> > +dump_hyp_walk(vaddr_t addr)
> >   {
> > -    return NULL;
> >   }
> >
> >   void __init remove_early_mappings(void) diff --git
> > a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index
> > 79965a3c17..0565e22a1f 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -218,7 +218,10 @@ SECTIONS
> >     _end = . ;
> >
> >     /* Section for the device tree blob (if any). */
> > -  .dtb : { *(.dtb) } :text
> > +  .dtb : {
> > +      . = ALIGN(PAGE_SIZE);
> > +      *(.dtb)
> > +  } :text
> >
> >     DWARF2_DEBUG_SECTIONS
> >
> 
> Cheers,
> 
> --
> Julien Grall