| Message ID | adebcaa145af5e3de7fba07dc84b0993866e98ac.1710443965.git.sanastasio@raptorengineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Early Boot Allocation on Power | expand |
Hi 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,
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
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(-)