[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 5/7] xen/ppc: Enable bootfdt and boot allocator
Hi Julien, On 12/20/23 7:49 AM, Julien Grall wrote: > Hi, > > On 15/12/2023 02:44, Shawn Anastasio wrote: >> diff --git a/xen/common/device-tree/bootfdt.c >> b/xen/common/device-tree/bootfdt.c >> index 796ac01c18..7ddfcc7e2a 100644 >> --- a/xen/common/device-tree/bootfdt.c >> +++ b/xen/common/device-tree/bootfdt.c >> @@ -543,12 +543,33 @@ 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((void *)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); > > The consequence is BOOTMOD_FDT will not be added. AFAICT, Arm doesn't > try to access BOOTMOD_FDT, but I think it would be worth to print message. > A message will already be printed by `meminfo_overlap_check` in setup.c when this condition is hit, like this: (XEN) Region: [0x0000003076e9b0, 0x00000030775215) overlapping with bank[3]: [0x00000030600000, 0x00000031000000) If you'd like me to add another more descriptive message here to let the user know that BOOTMOD_FDT wasn't added I could do that, though. >> + >> + /* >> + * Xen relocates itself at the ppc64 entrypoint, so we need to >> manually mark >> + * the kernel module. >> + */ >> + if ( IS_ENABLED(CONFIG_PPC64) ) { >> + paddr_t xen_start, xen_end; >> + >> + xen_start = __pa(_start); >> + xen_end = PAGE_ALIGN(__pa(_end)); >> + if ( !add_boot_module(BOOTMOD_XEN, xen_start, xen_end, false) ) >> + panic("Xen overlaps reserved memory! %016lx - %016lx\n", >> xen_start, >> + xen_end); >> + } > > Can you explain why this is added here rather than in the caller of > boot_fdt_info()? Either before or after? > > If after, I guess it is because of early_print_info(). In which case, I > would suggest to move off early_print_info() to caller on each > architecture. > Right, it can't be after due to the early_print_info() call, but doing it before actually works fine. Thank you for pointing that out -- I'll change this in the next revision. > Cheers, > Thanks, Shawn
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |