From: Mike Rapoport <rppt@kernel.org>
To: Evangelos Petrongonas <epetron@amazon.de>
Cc: ardb@kernel.org, changyuanl@google.com, graf@amazon.com,
kexec@lists.infradead.org, linux-efi@vger.kernel.org,
linux-kernel@vger.kernel.org, nh-open-source@amazon.com
Subject: Re: [PATCH] efi: Support booting with kexec handover (KHO)
Date: Thu, 14 Aug 2025 11:51:21 +0300 [thread overview]
Message-ID: <aJ2jiZ8MPGGALfGH@kernel.org> (raw)
In-Reply-To: <20250814005321.31705-1-epetron@amazon.de>
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 <rppt@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 <epetron@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.
prev parent reply other threads:[~2025-08-14 8:51 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-08 16:36 [PATCH] efi: Support booting with kexec handover (KHO) Evangelos Petrongonas
2025-08-11 6:39 ` Mike Rapoport
2025-08-14 0:53 ` Evangelos Petrongonas
2025-08-14 8:51 ` Mike Rapoport [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aJ2jiZ8MPGGALfGH@kernel.org \
--to=rppt@kernel.org \
--cc=ardb@kernel.org \
--cc=changyuanl@google.com \
--cc=epetron@amazon.de \
--cc=graf@amazon.com \
--cc=kexec@lists.infradead.org \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nh-open-source@amazon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).