Re: [PATCH v2 5/6] arm/mpu: Implement early_fdt_map support in MPU systems

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 02/07/2025 16:14, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> Implement the function early_fdt_map(), which is responsible for mapping
> the Device Tree Blob in the early stages of the boot process, for MPU
> systems.
> 
> We make use of the map_pages_to_xen() and destroy_xen_mappings() APIs.
> In particular the latter function is necessary in the case that the
> initial mapping of the fdt_header is insufficient to cover the entire
> DTB, as we must destroy and then remap the region due to the APIs no
> providing support for extending the size of an existing region.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Signed-off-by: Hari Limaye <hari.limaye@xxxxxxx>
> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> Changes from v1:
> - Add Ayan's R-b
> ---
>  xen/arch/arm/mpu/setup.c | 74 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
> index b4da77003f..ab00cb944b 100644
> --- a/xen/arch/arm/mpu/setup.c
> +++ b/xen/arch/arm/mpu/setup.c
> @@ -1,17 +1,87 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <xen/bootfdt.h>
>  #include <xen/bug.h>
>  #include <xen/init.h>
> +#include <xen/libfdt/libfdt.h>
>  #include <xen/mm.h>
> +#include <xen/pfn.h>
>  #include <xen/types.h>
>  #include <asm/setup.h>
>  
> +static paddr_t __initdata mapped_fdt_paddr = INVALID_PADDR;
I think it would be better to name it mapped_fdt_base, given that in MPU
everything refers to base,limit.

> +static paddr_t __initdata mapped_fdt_limit = INVALID_PADDR;
> +
>  void __init setup_pagetables(void) {}
>  
>  void * __init early_fdt_map(paddr_t fdt_paddr)
>  {
> -    BUG_ON("unimplemented");
> -    return NULL;
> +    /* Map at least a page containing the DTB address, exclusive range */
> +    paddr_t base = round_pgdown(fdt_paddr);
> +    paddr_t limit = round_pgup(fdt_paddr + sizeof(struct fdt_header));
> +    unsigned int flags = PAGE_HYPERVISOR_RO;
> +    void *fdt_virt = (void *)fdt_paddr; /* virt == paddr for MPU */
> +    int rc;
> +    unsigned long nr_mfns;
> +
> +    /*
> +     * 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 starting at this address has already been mapped. */
When can this happen?

> +    if ( mapped_fdt_paddr == fdt_paddr )
> +        return fdt_virt;
> +
> +    /*
> +     * DTB starting at a different address has been mapped, so destroy this
> +     * before continuing.
> +     */
> +    if ( mapped_fdt_paddr != INVALID_PADDR )
> +    {
> +        rc = destroy_xen_mappings(round_pgdown(mapped_fdt_paddr),
> +                                  mapped_fdt_limit);
> +        if ( rc )
> +            panic("Unable to unmap existing device-tree.\n");
NIT: no need for a dot at the end. It only takes space and is really not needed.

> +    }
> +
> +    nr_mfns = (limit - base) >> PAGE_SHIFT;
> +
> +    rc = map_pages_to_xen(base, maddr_to_mfn(base), nr_mfns, flags);
> +    if ( rc )
> +        panic("Unable to map the device-tree.\n");
> +
> +    mapped_fdt_paddr = fdt_paddr;
> +    mapped_fdt_limit = limit;
> +
> +    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> +        return NULL;
> +
> +    limit = round_pgup(fdt_paddr + fdt_totalsize(fdt_virt));
You're missing a sanity check for MAX_FDT_SIZE

> +
> +    /* If the mapped range is not enough, map the rest of the DTB. */
> +    if ( limit > mapped_fdt_limit )
> +    {
> +        rc = destroy_xen_mappings(base, mapped_fdt_limit);
> +        if ( rc )
> +            panic("Unable to unmap the device-tree header.\n");
> +
> +        nr_mfns = (limit - base) >> PAGE_SHIFT;
> +
> +        rc = map_pages_to_xen(base, maddr_to_mfn(base), nr_mfns, flags);
> +        if ( rc )
> +            panic("Unable to map the device-tree.\n");
> +
> +        mapped_fdt_limit = limit;
> +    }
> +
> +    return fdt_virt;
>  }
>  
>  /*

~Michal