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 <sanasta...@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 <jgr...@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,
--
Julien Grall
