Hi Michal,
> On 26 Feb 2025, at 10:38, Orzel, Michal <michal.or...@amd.com> wrote:
>
>
>
> On 26/02/2025 09:36, Luca Fancellu wrote:
>>
>>
>> Currently the early stage of the Arm boot maps the DTB using
>> early_fdt_map() using PAGE_HYPERVISOR_RO which is cacheable
>> read-only memory, later during DTB relocation the function
>> copy_from_paddr() is used to map pages in the same range on
>> the fixmap but using PAGE_HYPERVISOR_WC which is non-cacheable
>> read-write memory.
>>
>> The Arm specifications, ARM DDI0487L.a, section B2.11 "Mismatched
>> memory attributes" discourage using mismatched attributes for
>> aliases of the same location.
>>
>> Given that there is nothing preventing the relocation since the region
>> is already mapped, fix that by open-coding copy_from_paddr inside
>> relocate_fdt, without mapping on the fixmap.
>>
>> Fixes: 1bdc81dac816 ("arm: setup MM using information from the device tree")
> Why Fixes tag? I don't see it as a bug and something we need to backport...
ok I’ll remove it
>
>> Signed-off-by: Luca Fancellu <luca.fance...@arm.com>
>> ---
>> xen/arch/arm/setup.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index c1f2d1b89d43..b1fd4b44a2e1 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -237,14 +237,15 @@ void __init discard_initial_modules(void)
>> }
>>
>> /* Relocate the FDT in Xen heap */
>> -static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)
>> +static void * __init relocate_fdt(const void *dtb_vaddr, size_t dtb_size)
>> {
>> void *fdt = xmalloc_bytes(dtb_size);
>>
>> if ( !fdt )
>> panic("Unable to allocate memory for relocating the Device-Tree.\n");
>>
>> - copy_from_paddr(fdt, dtb_paddr, dtb_size);
>> + memcpy(fdt, dtb_vaddr, dtb_size);
>> + clean_dcache_va_range(dtb_vaddr, dtb_size);
> The purpose of cleaning dcache after memcpy is to clean the new area i.e.
> destination == fdt, not source region.
woops, my bad, I’ll fix
>
>>
>> return fdt;
>> }
>> @@ -362,7 +363,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
>> if ( acpi_disabled )
>> {
>> printk("Booting using Device Tree\n");
>> - device_tree_flattened = relocate_fdt(fdt_paddr, fdt_size);
>> + device_tree_flattened = relocate_fdt(device_tree_flattened,
>> fdt_size);
> NIT: It can be just my PoV but it reads odd. Why can't relocate_fdt modify
> device_tree_flattened pointer directly in the function?
you mean something like:
static void * __init relocate_fdt(size_t dtb_size)
{
void *fdt = xmalloc_bytes(dtb_size);
if ( !fdt )
panic("Unable to allocate memory for relocating the Device-Tree.\n");
memcpy(fdt, device_tree_flattened, dtb_size);
clean_dcache_va_range(fdt, dtb_size);
return fdt;
}
Cheers,
Luca
