Re: [PATCH] xen/arm: Don't use copy_from_paddr for DTB relocation

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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