Re: [PATCH v4 4/6] xen/ppc: Enable bootfdt and boot allocator

[Julien Grall <julien@xxxxxxx>]

Hi Shawn,

On 12/04/2024 04:55, Shawn Anastasio wrote:
> Enable usage of bootfdt for populating the boot info struct from the
> firmware-provided device tree.  Also enable the Xen boot page allocator.
>
> Additionally, modify bootfdt.c's boot_fdt_info() to tolerate the
> scenario in which the FDT overlaps a reserved memory region, as is the
> case on PPC when booted directly from skiboot. Since this means that Xen
> can now boot without a BOOTMOD_FDT present in bootinfo, clarify this
> fact in a comment above BOOTMOD_FDT's definition.
>
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> ---
> Changes in v4:
>    - drop unnecessary libfdt.h include in setup.c
>    - make boot_fdt and xen_bootmodule const
>    - more clearly document that BOOTMOD_FDT is now optional
>    - add explicit (void) cast to BOOTMOD_FDT creation
>
>   xen/arch/ppc/setup.c             | 22 +++++++++++++++++++++-
>   xen/common/device-tree/bootfdt.c | 11 +++++++++--
>   xen/include/xen/bootfdt.h        |  7 +++++++
>   3 files changed, 37 insertions(+), 3 deletions(-)
The changes look overall good. I have a few remark belows. But you can 
have my acked-by with or without the NIT addressed:
Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

> diff --git a/xen/arch/ppc/setup.c b/xen/arch/ppc/setup.c
> index 101bdd8bb6..47e997969f 100644
> --- a/xen/arch/ppc/setup.c
> +++ b/xen/arch/ppc/setup.c
> @@ -1,12 +1,15 @@
>   /* SPDX-License-Identifier: GPL-2.0-or-later */
>   #include <xen/init.h>
>   #include <xen/lib.h>
> +#include <xen/bootfdt.h>
> +#include <xen/device_tree.h>
We are tryying to keep the header order alphabetically. It looks like 
the existing code is respecting that. So can this be done for the two 
new headers?
>   #include <xen/mm.h>
>   #include <public/version.h>
>   #include <asm/boot.h>
>   #include <asm/early_printk.h>
>   #include <asm/mm.h>
>   #include <asm/processor.h>
> +#include <asm/setup.h>
>
>   /* Xen stack for bringing up the first CPU. */
>   unsigned char __initdata cpu0_boot_stack[STACK_SIZE] __aligned(STACK_SIZE);
> @@ -24,6 +27,9 @@ void __init noreturn start_xen(unsigned long r3, unsigned 
> long r4,
>                                  unsigned long r5, unsigned long r6,
>                                  unsigned long r7)
>   {
> +    const void *boot_fdt;
> +    const struct bootmodule *xen_bootmodule;
> +
>       if ( r5 )
>       {
>           /* Unsupported OpenFirmware boot protocol */
> @@ -32,11 +38,25 @@ void __init noreturn start_xen(unsigned long r3, unsigned 
> long r4,
>       else
>       {
>           /* kexec boot protocol */
> -        boot_opal_init((void *)r3);
> +        boot_fdt = (void *)r3;
NIT: Sorry I should have noticed it earlier. boot_fdt is only used to ..

> +        boot_opal_init(boot_fdt);
>       }
>
>       setup_exceptions();
>
> +    device_tree_flattened = boot_fdt;
... set device_tree_flattened and ...

> +    boot_fdt_info(boot_fdt, r3);

... used here. Is there any plan to have device_tree_flattened != 
boot_fdt? If not, then I would suggest to simply drop boot_fdt.
> +
> +    /*
> +     * Xen relocates itself at the ppc64 entrypoint, so we need to manually 
> mark
> +     * the kernel module.
> +     */
> +    xen_bootmodule = add_boot_module(BOOTMOD_XEN, __pa(_start),
> +                                     PAGE_ALIGN(__pa(_end)), false);
> +    BUG_ON(!xen_bootmodule);
> +
> +    populate_boot_allocator();
> +
>       setup_initial_pagetables();
>
>       early_printk("Hello, ppc64le!\n");
> diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
> index f01a5b5d76..76d0f72ef9 100644
> --- a/xen/common/device-tree/bootfdt.c
> +++ b/xen/common/device-tree/bootfdt.c
> @@ -542,12 +542,19 @@ 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(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.
> +     */
> +    (void)add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
> +
>       /*
>        * On Arm64 setup_directmap_mappings() expects to be called with the 
> lowest
>        * bank in memory first. There is no requirement that the DT will provide
> diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
> index 577618da16..ea3ad96bb9 100644
> --- a/xen/include/xen/bootfdt.h
> +++ b/xen/include/xen/bootfdt.h
> @@ -13,7 +13,14 @@
>
>   typedef enum {
>       BOOTMOD_XEN,
> +
> +    /*
> +     * The BOOTMOD_FDT type will only be present when the firmware-provided FDT
> +     * blob exists outside of a reserved memory region which is platform-
> +     * dependent and may not be relied upon.
> +     */
>       BOOTMOD_FDT,
> +
>       BOOTMOD_KERNEL,
>       BOOTMOD_RAMDISK,
>       BOOTMOD_XSM,
> --
> 2.30.2
Cheers,

--
Julien Grall