On Thu, Aug 14, 2025 at 12:53:15AM +0000, Evangelos Petrongonas wrote:
> Hey Mike, thanks for your review,
>
> On Mon, 11 Aug 2025 09:39:50 +0300, Mike Rapoport <r...@kernel.org> wrote:
> > On Fri, Aug 08, 2025 at 04:36:51PM +0000, Evangelos Petrongonas wrote:
> > > When KHO (Kexec HandOver) is enabled, it sets up scratch memory regions
> > > early during device tree scanning. After kexec, the new kernel
> > > exclusively uses this region for memory allocations during boot up to
> > > the initialization of the page allocator
> > >
> > > However, when booting with EFI, EFI's reserve_regions() uses
> > > memblock_remove(0, PHYS_ADDR_MAX) to clear all memory regions before
> > > rebuilding them from EFI data. This destroys KHO scratch regions and
> > > their flags, thus causing a kernel panic, as there are no scratch
> > > memory regions.
> > >
> > > Instead of wholesale removal, iterate through memory regions and only
> > > remove non-KHO ones. This preserves KHO scratch regions while still
> > > allowing EFI to rebuild its memory map.
> >
> > It's worth mentioning that scratch areas are "good known memory" :)
> >
>
> I Will do so on Rev2.
>
> > > Signed-off-by: Evangelos Petrongonas <epet...@amazon.de>
> > > ---
> > >
> > > */
> > > memblock_dump_all();
> > > - memblock_remove(0, PHYS_ADDR_MAX);
> > > +
> > > + if (IS_ENABLED(CONFIG_MEMBLOCK_KHO_SCRATCH)) {
> >
> > It's better to condition this on kho_get_fdt() that means that we are
> > actually doing a handover.
> >
>
> Hmm, I see that `kho_get_fdt()` is static. My first instinct was to use
> kho_enable() instead. Diving a bit more into the initialisation flow,
> during the `setup_arch()`->`efi_init()`, `kho_enable()` will return
> true if kho is enabled in the cmdline, but not if we are actually doing
> a KHO enabled kexec. However, in this case, the parsing of memory
> regions is going to be a noop in terms of functionality, but will
> contribute, negatively —though the overhead would likely be
> unmeasurable to the (cold) boot time. If we want to avoid that, we
> might consider adding another function to the KHO API, like
> `is_booting_with_kho()`, that practically wraps the `kho_get_fdt()`.
> IMO, it feels a bit cleaner this way, as other components don't
> necessarily (need to) know the internal FDT based implementation of
> KHO. That being said, I am definitely not the most experienced person
> when it comes to API design, so there is a high chance that I am way
> off :)
>
> So to sum it up, I see three paths forward:
> 1. Condition with `kho_is_enabled()` instead of the CONFIG (accepting
> the minor cold boot overhead)
> 2. Post another patch that extends the KHO API, adding a wrapper for
> the `kho_get_fdt()`, like `is_booting_with_kho()` indicating that we
> are booting with KHO enabled
> 3. Post another patch that exports the `kho_get_fdt()` directly.
My preference is for the second option, I'd just name it is_kho_boot()
> I am happy to implement any of the three, or any other suggestion you
> might have.
>
> > > + struct memblock_region *reg;
> > > + phys_addr_t start, size;
> > > + int i;
> > > +
> > > + /* Remove all non-KHO regions */
> > > + for (i = memblock.memory.cnt - 1; i >= 0; i--) {
> >
> > Please use for_each_mem_region()
> >
>
> Todo in Rev2.
>
> --
> Kind Regards,
> Evangelos.
--
Sincerely yours,
Mike.
