diff mbox series

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

Message ID 46e6d4ddd74b9ecc4937d1086efe06eb39c499dd.1712893887.git.sanastasio@raptorengineering.com (mailing list archive)
State New, archived
Headers show
Series Early Boot Allocation on Power | expand

Commit Message

Shawn Anastasio April 12, 2024, 3:55 a.m. UTC 
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@raptorengineering.com>
---
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(-)

--
2.30.2

Comments

Julien Grall April 24, 2024, 6:41 p.m. UTC | #1 
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@raptorengineering.com>
> ---
> 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@amazon.com>

> 
> 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,
diff mbox series

Patch

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>
 #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;
+        boot_opal_init(boot_fdt);
     }

     setup_exceptions();

+    device_tree_flattened = boot_fdt;
+    boot_fdt_info(boot_fdt, r3);
+
+    /*
+     * 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,