Re: [PATCH 0/2] x86/kexec: Implement crash kexec for Secure Boot

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/06/2026 5:49 pm, Kevin Lampis wrote:
> This replaces the previous series
> [PATCH v2 0/4] Allows Secure Boot for Kexec
> https://lore.kernel.org/xen-devel/20250507094253.10395-1-freddy77@xxxxxxxxx/
>
> The main feedback last time was that almost all the purgatory code could
> be removed.
> - The digest check is now done in machine_kexec() instead of purgatory
> - GPRs are cleared in kexec_reloc.S which shouldn't affect non-EFI kexec
>
> Kevin Lampis (1):
>   x86: Implement crash kexec for EFI
>
> Ross Lagerwall (1):
>   Add lockdown mode
>
>  xen/arch/x86/bzimage.c                   |  40 +---
>  xen/arch/x86/include/asm/machine_kexec.h |   2 +-
>  xen/arch/x86/machine_kexec.c             |  10 +-
>  xen/arch/x86/setup.c                     |   1 +
>  xen/arch/x86/x86_64/kexec_reloc.S        |  16 ++
>  xen/common/Kconfig                       |   8 +
>  xen/common/Makefile                      |   1 +
>  xen/common/kernel.c                      |   4 +
>  xen/common/kexec.c                       |  41 +++-
>  xen/common/kimage.c                      | 264 +++++++++++++++++++----
>  xen/common/lockdown.c                    |  56 +++++
>  xen/include/public/kexec.h               |  23 +-
>  xen/include/xen/kimage.h                 |  23 +-
>  xen/include/xen/lockdown.h               |   9 +
>  xen/include/xen/x86-linux.h              |  62 ++++++
>  15 files changed, 460 insertions(+), 100 deletions(-)
>  create mode 100644 xen/common/lockdown.c
>  create mode 100644 xen/include/xen/lockdown.h
>  create mode 100644 xen/include/xen/x86-linux.h

Thankyou for doing this work.  This is a far more palatable diff-stat
than adding all of Purgatory.

But, the lockdown work is still blocked on some open questions and (in
practice) the security statement.  It only matters for the -EPERMs in
the top levels of the hypercall, and that comes from a bad split of
patches in the XenServer patchqueue.  You'll want to drop it from this
series.

The rest of the work (just patch 2) needs splitting into several.

First, moving the thing called setup_header wants to be it's own patch. 
It wants to move into the existing bzimage.h (itself fixing a bug with
the copyright header) and probably wants to be renamed to bzimage_header
because, despite that being it's Linux name, "setup" is far too generic.

You have identified that bzimage is x86 specific, and for that reason it
should not be included in common/kexec.c.  We are going to need an
architecture specific way of figuring out the alignment, but looking at
kimage_find_kernel_entry_maddr(), why can't we just use the EFI header?


Doing the digest checks should be its own patch.  Why is it tied to
KEXEC_TYPE_CRASH_EFI?  It's applicable to all kexec types.  This ought
to be first first in the series so it's not tied to EFI.

As to performing and failing the digest checks, it probably wants to be
a little earlier.  Functions like kexec_crash() want to return rather
than hanging, so hwdom_shutdown() can fall through into
reboot_or_halt().  kexec_exec() wants to return propagate an error back
into the hypercall to identify that the image appears to be corrupt.

Failing the digest check should print both the expected and obtained
values.  See check_xen_buildid() for a related example that you should
follow.


Swapping xen_kexec_segment_t to struct kimage_segment wants to be it's
own patch too.  It's mechanical but needs to not be mixed in with
logical changes.


For the %rsi/boot_params pointer, that wants to be a separate patch
too.  I wouldn't label it as EFI-only, nor as rsi.  Maybe just "arg"? 
It should be wired up on the 32bit path where it's %esi too, so really
you just need to move it out of %r9 before the 32bit split.

Zeroing the other registers is fine, but again wants to be a separate
patch.  It's unrelated to the rest of the content, and wants to happen
on both paths.  Also, when zeroing registers, always use the 32bit
register form even in 64bit code.  See PUSH_AND_CLEAR_GPRS; there are
some 64bit CPUs which care.

There are a bunch of style issues too, but lets focus on getting the
structure right first.

~Andrew