| Message ID | 20250226083649.2063916-1-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Don't use copy_from_paddr for DTB relocation | expand |
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... > Signed-off-by: Luca Fancellu <luca.fancellu@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. > > 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? > dt_unflatten_host_device_tree(); > } > else > -- > 2.34.1 > ~Michal
Hi Michal, > On 26 Feb 2025, at 10:38, Orzel, Michal <michal.orzel@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.fancellu@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
On 26/02/2025 11:45, Luca Fancellu wrote: > > > Hi Michal, > >> On 26 Feb 2025, at 10:38, Orzel, Michal <michal.orzel@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.fancellu@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); You already make assumption about device_tree_flattened being global, so why not assigning device_tree_flattened = fdt in the function as well? ~Michal
>> >>> >>>> >>>> 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? just because it’s more easy to follow the global variable changes when reading the start_xen(…) code as the function is the only one modifying it. If you strongly oppose to that I’ll change, but imo it’s easier to follow the code in this way Cheers, Luca
On 26/02/2025 11:59, Luca Fancellu wrote: > > >>> >>>> >>>>> >>>>> 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? > > just because it’s more easy to follow the global variable changes when reading the start_xen(…) > code as the function is the only one modifying it. > > If you strongly oppose to that I’ll change, but imo it’s easier to follow the code in this way How about: static void __init relocate_fdt(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"); memcpy(fdt, dtb_vaddr, dtb_size); clean_dcache_va_range(fdt, dtb_size); dtb_vaddr = fdt; } This would be best IMO. That said, I don't care that much. Choose whatever makes sense. ~Michal
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); 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); dt_unflatten_host_device_tree(); } else
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") Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/setup.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)