Re: [PATCH v2 5/7] xen/ppc: Enable bootfdt and boot allocator

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

Hi Julien,

On 12/20/23 7:49 AM, Julien Grall wrote:
> Hi,
> 
> On 15/12/2023 02:44, Shawn Anastasio wrote:
>> diff --git a/xen/common/device-tree/bootfdt.c
>> b/xen/common/device-tree/bootfdt.c
>> index 796ac01c18..7ddfcc7e2a 100644
>> --- a/xen/common/device-tree/bootfdt.c
>> +++ b/xen/common/device-tree/bootfdt.c
>> @@ -543,12 +543,33 @@ size_t __init boot_fdt_info(const void *fdt,
>> paddr_t paddr)
>>       if ( ret < 0 )
>>           panic("No valid device tree\n");
>>   -    add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
>> -
>>       ret = device_tree_for_each_node((void *)fdt, 0, early_scan_node,
>> NULL);
>>       if ( ret )
>>           panic("Early FDT parsing failed (%d)\n", ret);
>>   +    /*
>> +     * Add module for the FDT itself after the device tree has been
>> parsed. This
>> +     * is required on ppc64le where the device tree passed to Xen may
>> have been
>> +     * allocated by skiboot, in which case it will exist within a
>> reserved
>> +     * region and this call will fail. This is fine, however, since
>> either way
>> +     * the allocator will know not to step on the device tree.
>> +     */
>> +    add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
> 
> The consequence is BOOTMOD_FDT will not be added. AFAICT, Arm doesn't
> try to access BOOTMOD_FDT, but I think it would be worth to print message.
>

A message will already be printed by `meminfo_overlap_check` in setup.c
when this condition is hit, like this:

(XEN) Region: [0x0000003076e9b0, 0x00000030775215) overlapping with
bank[3]: [0x00000030600000, 0x00000031000000)

If you'd like me to add another more descriptive message here to let the
user know that BOOTMOD_FDT wasn't added I could do that, though.

>> +
>> +    /*
>> +     * Xen relocates itself at the ppc64 entrypoint, so we need to
>> manually mark
>> +     * the kernel module.
>> +     */
>> +    if ( IS_ENABLED(CONFIG_PPC64) ) {
>> +        paddr_t xen_start, xen_end;
>> +
>> +        xen_start = __pa(_start);
>> +        xen_end = PAGE_ALIGN(__pa(_end));
>> +        if ( !add_boot_module(BOOTMOD_XEN, xen_start, xen_end, false) )
>> +            panic("Xen overlaps reserved memory! %016lx - %016lx\n",
>> xen_start,
>> +                xen_end);
>> +    }
> 
> Can you explain why this is added here rather than in the caller of
> boot_fdt_info()? Either before or after?
> 
> If after, I guess it is because of early_print_info(). In which case, I
> would suggest to move off early_print_info() to caller on each
> architecture.
>

Right, it can't be after due to the early_print_info() call, but doing
it before actually works fine. Thank you for pointing that out -- I'll
change this in the next revision.

> Cheers,
>

Thanks,
Shawn