[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


  • To: Luca Fancellu <Luca.Fancellu@xxxxxxx>
  • From: "Orzel, Michal" <michal.orzel@xxxxxxx>
  • Date: Wed, 26 Feb 2025 11:54:53 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=B2JAM85tqoX/MhVLkbHY8+/3hmXzN1btbbmTcfwchwA=; b=dDiRG2HTC65Oc6KldsVZ2sCm9+owA+DGa4WKafTRHLp+CAFYKLPE+lV5Mw2L6VfcLo5PhUXbtMGebPr5f/5Qa+HCfeqt+yT+4bgcqjlPe1VweKZP+sQxzj3noZUTieJIKTi/itVpCoTBhP+A0DDWCdmKEvF98doDy4x6Bz3SegNwjTXqlXnzMACb9Ie76X6K/6NavJfnpcY9rLBb8Olm4/j7o4Z1aWNK5rxIiZ8XfMw0Z7rep0QKVbfO4C/DOclcKpggEfUXO8lkXGcyYBS6sEXIDWEwoAPM7jyZgCUMoYHyNDyIq3aXIHuAWxA9aRIlvbJa3Mz8nv1BpVjTkhDbDw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=Vo9OpOsP/h17PVr0ZHVrXNQ78XwoCKOWrCob9msANKFMa2kI1Qkkp5cywcM1jMDGLNWeKosq1FxLTGLWkG/9FqD5gsa6pjwkFWjvAAO2pLSd0CleLrTVdVxdSiBZEw0wpPL3mYZbDRXtGhcIIg+iqy0GWORV9H8nGCgTog9YtqmB68Cod8J1HzmKyZuOZFcnSgWpmjLz4vvc64J4X/R93yv6y8osbAaCfB9l9hNkrapb5NnNtS+tXsPknLVq4HHAeGgYBSirBu5APtrfNM2q0PKpXQ/gP/uueeDwRh0mTiFJlBxsxJ/uV9qdqNoF9I0VQMVSvo3QruojYWNzz+S1og==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 26 Feb 2025 10:55:12 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>


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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.