Enable usage of bootfdt for populating the boot info struct from the
firmware-provided device tree. Also enable the Xen boot page allocator.
Includes minor changes to 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.
Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/arch/ppc/include/asm/setup.h | 5 +++++
xen/arch/ppc/setup.c | 21 ++++++++++++++++++++-
xen/common/device-tree/bootfdt.c | 11 +++++++++--
3 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/xen/arch/ppc/include/asm/setup.h b/xen/arch/ppc/include/asm/setup.h
index 1b2d29c5b6..fe27f61fc3 100644
--- a/xen/arch/ppc/include/asm/setup.h
+++ b/xen/arch/ppc/include/asm/setup.h
@@ -115,4 +115,9 @@ const char *boot_module_kind_as_string(bootmodule_kind kind);
struct bootcmdline *boot_cmdline_find_by_kind(bootmodule_kind kind);
void populate_boot_allocator(void);
+/*
+ * bootfdt.c
+ */
+size_t boot_fdt_info(const void *fdt, paddr_t paddr);
+
#endif /* __ASM_PPC_SETUP_H__ */
diff --git a/xen/arch/ppc/setup.c b/xen/arch/ppc/setup.c
index 101bdd8bb6..946167a56f 100644
--- a/xen/arch/ppc/setup.c
+++ b/xen/arch/ppc/setup.c
@@ -1,12 +1,14 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
#include <xen/init.h>
#include <xen/lib.h>
+#include <xen/libfdt/libfdt.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 +26,9 @@ void __init noreturn start_xen(unsigned long r3, unsigned long r4,
unsigned long r5, unsigned long r6,
unsigned long r7)
{
+ void *boot_fdt;
+ struct bootmodule *xen_bootmodule;
+
if ( r5 )
{
/* Unsupported OpenFirmware boot protocol */
@@ -32,11 +37,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 35dbdf3384..1985648b31 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -543,12 +543,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.
+ */
+ 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
--
2.30.2Hi Shawn,
On 14/03/2024 22:15, 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.
>
> Includes minor changes to 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.
>
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> ---
> xen/arch/ppc/include/asm/setup.h | 5 +++++
> xen/arch/ppc/setup.c | 21 ++++++++++++++++++++-
> xen/common/device-tree/bootfdt.c | 11 +++++++++--
> 3 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/ppc/include/asm/setup.h b/xen/arch/ppc/include/asm/setup.h
> index 1b2d29c5b6..fe27f61fc3 100644
> --- a/xen/arch/ppc/include/asm/setup.h
> +++ b/xen/arch/ppc/include/asm/setup.h
> @@ -115,4 +115,9 @@ const char *boot_module_kind_as_string(bootmodule_kind kind);
> struct bootcmdline *boot_cmdline_find_by_kind(bootmodule_kind kind);
> void populate_boot_allocator(void);
>
> +/*
> + * bootfdt.c
> + */
> +size_t boot_fdt_info(const void *fdt, paddr_t paddr);
> +
This function is common. So the prototype doesn't belong to a per-arch
header.
> #endif /* __ASM_PPC_SETUP_H__ */
> diff --git a/xen/arch/ppc/setup.c b/xen/arch/ppc/setup.c
> index 101bdd8bb6..946167a56f 100644
> --- a/xen/arch/ppc/setup.c
> +++ b/xen/arch/ppc/setup.c
> @@ -1,12 +1,14 @@
> /* SPDX-License-Identifier: GPL-2.0-or-later */
> #include <xen/init.h>
> #include <xen/lib.h>
> +#include <xen/libfdt/libfdt.h>
None of the code below doesn't seem to use libfdt.h directly. So why do
you need it?
> #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 +26,9 @@ void __init noreturn start_xen(unsigned long r3, unsigned long r4,
> unsigned long r5, unsigned long r6,
> unsigned long r7)
> {
> + void *boot_fdt;
boot_fdt is not meant to be modified. So this should be const.
> + struct bootmodule *xen_bootmodule;
Same here I guess.
> +
> if ( r5 )
> {
> /* Unsupported OpenFirmware boot protocol */
> @@ -32,11 +37,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 35dbdf3384..1985648b31 100644
> --- a/xen/common/device-tree/bootfdt.c
> +++ b/xen/common/device-tree/bootfdt.c
> @@ -543,12 +543,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.
> + */
> + add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
I am still a little bit concerned with this change. If there is an
overlap, then BOOTMOD_FDT will not be created yet Xen will continue. I
think this needs to be explained in the commit message and in the code
(maybe on top of BOOTMOD_FDT which should be common). So it will be
clearer that we cannot rely on BOOTMOD_FDT.
Also, there was some recent discussion for MISRA to check all return or
use (void) + a comment to explain this is ignored. I think you have part
of the explanation (see above for the second part), but I would also add
(void) to make clear this was on purpose.
Cheers,
--
Julien Grall
© 2016 - 2026 Red Hat, Inc.