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

[Julien Grall <julien@xxxxxxx>]

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.
> +
> +    /*
> +     * 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.
Cheers,

--
Julien Grall