Re: [PATCH v2 5/7] x86/kexec: Implement new EFI load type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.06.2026 17:18, Kevin Lampis wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_TBOOT) += tboot.o
>  obj-y += hpet.o
>  obj-$(CONFIG_VM_EVENT) += vm_event.o
>  obj-y += xstate.o
> +obj-$(CONFIG_KEXEC) += kexec.o

While there are some anomalies down here, most of this block is suitably
sorted (alphabetically). Please insert accordingly.

> --- /dev/null
> +++ b/xen/arch/x86/kexec.c
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/kexec.h>
> +#include <xen/kimage.h>
> +#include <xen/guest_access.h>
> +#include <asm/bzimage.h>
> +
> +/*
> + * Find the entry point to the new kernel, we need to map the crash region 
> into
> + * memory in order to read the kernel header.
> + */
> +#define KERNEL_SEGMENT_IDX 0
> +int64_t kimage_find_kernel_entry_maddr(struct kexec_image *image)
> +{
> +    uint64_t dest_maddr;
> +    uint32_t alignment, magic;
> +    uint16_t version;
> +    void *dest_va;
> +    const struct bzimage_header *hdr;
> +    int setup_sects;
> +    size_t kern16_size;
> +
> +    dest_maddr = image->segments[KERNEL_SEGMENT_IDX].dest_maddr +
> +                 image->segments[KERNEL_SEGMENT_IDX].dest_offset;
> +
> +    dest_va = map_domain_page(maddr_to_mfn(dest_maddr));
> +
> +    hdr = (const struct bzimage_header *)dest_va;
> +    magic = hdr->header;
> +    version = hdr->version;
> +    alignment = hdr->kernel_alignment;
> +    setup_sects = hdr->setup_sects == 0 ? 4 : hdr->setup_sects;
> +    kern16_size = (setup_sects + 1 )  * 512;
> +
> +    unmap_domain_page(dest_va);
> +
> +    if ( magic != 0x53726448 || version < 0x0202 )
> +        return -EINVAL;
> +
> +    /*
> +     * Ensure the kernel alignment is a valid LOAD_PHYSICAL_ADDR,
> +     * which ranges from 0x200000 (2MiB) to 0x1000000 (16MiB) on 64-bit 
> systems
> +     * as defined in the kernel x86 Kconfig
> +     */
> +    if ( alignment % 0x200000 != 0 ||
> +         alignment < 0x200000 ||
> +         alignment > 0x1000000 )

Use MB() to cover some of what the comment currently says? (The second of the
checks would also be easier as !alignment, seeing that the first check already
excluded all other values below MB(2).)

> --- a/xen/include/public/kexec.h
> +++ b/xen/include/public/kexec.h
> @@ -54,13 +54,16 @@
>   * - kexec into a regular kernel, very similar to a standard reboot
>   *   - KEXEC_TYPE_DEFAULT is used to specify this type
>   * - kexec into a special "crash kernel", aka kexec-on-panic
> - *   - KEXEC_TYPE_CRASH is used to specify this type
> + *   - KEXEC_TYPE_CRASH or KEXEC_TYPE_CRASH_EFI are used to specify this type
> + *   - in case of KEXEC_TYPE_CRASH_EFI the first segment will point to the
> + *     full kernel to load and entry point will point to boot params
>   *   - parts of our system may be broken at kexec-on-panic time
>   *     - the code should be kept as simple and self-contained as possible
>   */
>  
>  #define KEXEC_TYPE_DEFAULT 0
>  #define KEXEC_TYPE_CRASH   1
> +#define KEXEC_TYPE_CRASH_EFI 3
>  
>  /*
>   * Perform kexec having previously loaded a kexec or kdump kernel
> @@ -167,7 +170,11 @@ typedef struct xen_kexec_load {
>          XEN_GUEST_HANDLE(xen_kexec_segment_t) h;
>          uint64_t _pad;
>      } segments;
> -    uint64_t entry_maddr; /* image entry point machine address. */
> +    /* image entry point machine address or parameters in case of EFI. */

Nit: As you touch comments, please also correct style issues.

> +    union {
> +        uint64_t entry_maddr;
> +        uint64_t parameters;
> +    };

We don't want to use language extensions in public headers. I'm actually
surprised this still passes the -ansi header check that we do from
include/Makefile. Maybe with recent compilers this doesn't do anymore
what we expect it to do?

Jan