Hi Ayan,
> On 4 Mar 2025, at 12:13, Ayan Kumar Halder <ayank...@amd.com> wrote:
>
> 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.fance...@arm.com>
>> ---
>> 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.
No I don’t agree on that, I believe we would better use the same “interfaces”
as the MMU code in order to
reuse most of the code and be consistent on our memory management subsystems.
Unless a technical limitation prevents to use them, I would like to continue in
this way as
the serie extensively use these.
>> +
>> + 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.
First one will map a page that comprises the DTB header, second one will map
the rest.
The size of the DTB is not known at first, hence the two operation, anyway here
I’m not mapping
2 regions, I’m adjusting the same region with the updated range.
>> + */
>> + 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
>>
>>
