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

[Julien Grall <julien@xxxxxxx>]

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.
Do you plan to share some code where map_pages_to_xen() will be used?

> +                          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.
> +                          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