Re: [PATCH 8/9] arm/mpu: Implement early_fdt_map support in MPU systems

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Luca,

On 28/02/2025 16:18, Luca Fancellu 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.
>
>
> Implement early_fdt_map() function, that is responsible to map the
> device tree blob in the early stages of the boot process, since at
> this stage the MPU C data structure are not yet initialised, it is
> using low level APIs to write into the MPU registers at a fixed
> MPU region number.
>
> The MPU memory management is designed to work on pages of PAGE_SIZE
> in order to reuse helpers and macros already available on the Xen
> memory management system.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
>   xen/arch/arm/mpu/Makefile |  1 +
>   xen/arch/arm/mpu/setup.c  | 72 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 73 insertions(+)
>   create mode 100644 xen/arch/arm/mpu/setup.c
>
> diff --git a/xen/arch/arm/mpu/Makefile b/xen/arch/arm/mpu/Makefile
> index b18cec483671..04df0b2ee760 100644
> --- a/xen/arch/arm/mpu/Makefile
> +++ b/xen/arch/arm/mpu/Makefile
> @@ -1 +1,2 @@
>   obj-y += mm.o
> +obj-y += setup.init.o
> diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
> new file mode 100644
> index 000000000000..290baaca9fd7
> --- /dev/null
> +++ b/xen/arch/arm/mpu/setup.c
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * xen/arch/arm/mpu/setup.c
> + *
> + * MPU system boot code for Armv8-R AArch64.
> + *
> + */
> +
> +#include <xen/bootfdt.h>
> +#include <xen/init.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/mm.h>
> +
> +/* Needs to be kept in sync with the regions programmed in arm64/mpu/head.S */
> +#define EARLY_FDT_MAP_REGION_NUMBER 6
> +
> +void * __init early_fdt_map(paddr_t fdt_paddr)
> +{
> +    /* Map at least a page containing the DTB address, exclusive range */
> +    paddr_t base_paddr = round_pgdown(fdt_paddr);
> +    paddr_t end_paddr = round_pgup(fdt_paddr + sizeof(struct fdt_header));
> +    unsigned int flags = PAGE_HYPERVISOR_RO;
> +    void *fdt_virt = (void *)fdt_paddr; /* virt == paddr for MPU */
> +    pr_t fdt_region;
> +
> +    /*
> +     * 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;
> +
> +    /* Map the device tree blob header  */
> +    fdt_region = pr_of_xenaddr(base_paddr, end_paddr, PAGE_AI_MASK(flags));
Instead of this new macro (PAGE_AI_MASK(flags)), can I reuse the 
existing one (ie MT_NORMAL) ?
> +    fdt_region.prbar.reg.ap = PAGE_AP_MASK(flags);
May be somethng like

s/PAGE_AP_MASK(flags)/AP_RO_EL2

And define this in the common mpu.h

/* Read-only at EL2, No Access at EL1/EL0. */ #define AP_RO_EL2 0x2

> +    fdt_region.prbar.reg.xn = PAGE_XN_MASK(flags);
Similar comment as before.
> +
> +    write_protection_region(&fdt_region, EARLY_FDT_MAP_REGION_NUMBER);
> +    context_sync_mpu();
> +
> +    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> +        return NULL;
> +
> +    end_paddr = round_pgup(fdt_paddr + fdt_totalsize(fdt_virt));
> +
> +    /*
> +     * If the mapped range is not enough, map the rest of the DTB, pr_get_limit
> +     * returns an inclusive address of the range, hence the increment.
Can you explain this a bit more ? Why do we need to create 2 MPU regions 
for mapping DTB.
> +     */
> +    if ( end_paddr > (pr_get_limit(&fdt_region) + 1) )
> +    {
> +        pr_set_limit(&fdt_region, end_paddr);
> +
> +        write_protection_region(&fdt_region, EARLY_FDT_MAP_REGION_NUMBER);
> +        context_sync_mpu();
> +    }
> +
> +    return fdt_virt;
> +}
- Ayan
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.34.1