[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/arm: Don't use copy_from_paddr for DTB relocation
On 26/02/2025 11:45, Luca Fancellu wrote: > > > Hi Michal, > >> On 26 Feb 2025, at 10:38, Orzel, Michal <michal.orzel@xxxxxxx> 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.fancellu@xxxxxxx> >>> --- >>> 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); You already make assumption about device_tree_flattened being global, so why not assigning device_tree_flattened = fdt in the function as well? ~Michal
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |