Re: [PATCH v3 04/22] x86/boot/slaunch-early: implement early initialization

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2025 15:17, Sergii Dmytruk wrote:
> Make head.S invoke a C function to retrieve MBI and SLRT addresses in a
> platform-specific way.  This is also the place to perform sanity checks
> of DRTM.
> 
> Signed-off-by: Krystian Hebel <krystian.hebel@xxxxxxxxx>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>
> ---
>  xen/arch/x86/Makefile                |  1 +
>  xen/arch/x86/boot/Makefile           |  5 +++-
>  xen/arch/x86/boot/head.S             | 43 ++++++++++++++++++++++++++++
>  xen/arch/x86/boot/slaunch-early.c    | 41 ++++++++++++++++++++++++++
>  xen/arch/x86/include/asm/intel-txt.h | 16 +++++++++++
>  xen/arch/x86/include/asm/slaunch.h   | 26 +++++++++++++++++
>  xen/arch/x86/slaunch.c               | 27 +++++++++++++++++
>  7 files changed, 158 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/x86/boot/slaunch-early.c
>  create mode 100644 xen/arch/x86/include/asm/slaunch.h
>  create mode 100644 xen/arch/x86/slaunch.c

As indicated in reply to patch 3 - imo all code additions here want to be
under some CONFIG_xyz. I repeat this here, but I don't think I'll repeat it
any further.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -472,6 +472,10 @@ __start:
>          /* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. 
> */
>          xor     %edx,%edx
>  
> +        /* Check for TrenchBoot slaunch bootloader. */
> +        cmp     $SLAUNCH_BOOTLOADER_MAGIC, %eax
> +        je      .Lslaunch_proto
> +
>          /* Check for Multiboot2 bootloader. */
>          cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
>          je      .Lmultiboot2_proto
> @@ -487,6 +491,45 @@ __start:
>          cmovnz  MB_mem_lower(%ebx),%edx
>          jmp     trampoline_bios_setup
>  
> +.Lslaunch_proto:
> +        /*
> +         * Upon reaching here, CPU state mostly matches the one setup by the
> +         * bootloader with ESP, ESI and EDX being clobbered above.
> +         */
> +
> +        /* Save information that TrenchBoot slaunch was used. */
> +        movb    $1, sym_esi(slaunch_active)
> +
> +        /*
> +         * Prepare space for output parameter of slaunch_early_init(), which 
> is
> +         * a structure of two uint32_t fields.
> +         */
> +        sub     $8, %esp

At the very least a textual reference to the struct type is needed here,
to be able to find it. Better would be to have the size calculated into
asm-offsets.h, to use a proper symbolic name here.

> +        push    %esp                             /* pointer to output 
> structure */
> +        lea     sym_offs(__2M_rwdata_end), %ecx  /* end of target image */
> +        lea     sym_offs(_start), %edx           /* target base address */

Why LEA when this can be expressed with (shorter) MOV?

> +        mov     %esi, %eax                       /* load base address */
> +        /*
> +         * slaunch_early_init(load/eax, tgt/edx, tgt_end/ecx, ret/stk) using
> +         * fastcall calling convention.
> +         */
> +        call    slaunch_early_init
> +        add     $4, %esp                         /* pop the fourth parameter 
> */
> +
> +        /* Move outputs of slaunch_early_init() from stack into registers. */
> +        pop     %eax  /* physical MBI address */
> +        pop     %edx  /* physical SLRT address */
> +
> +        /* Save physical address of SLRT for C code. */
> +        mov     %edx, sym_esi(slaunch_slrt)

Why go through %edx?

> +        /* Store MBI address in EBX where MB2 code expects it. */
> +        mov     %eax, %ebx

Why go through %eax?

> --- /dev/null
> +++ b/xen/arch/x86/boot/slaunch-early.c
> @@ -0,0 +1,41 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright (c) 2022-2025 3mdeb Sp. z o.o. All rights reserved.
> + */
> +
> +#include <xen/slr-table.h>
> +#include <xen/types.h>
> +#include <asm/intel-txt.h>
> +
> +struct early_init_results
> +{
> +    uint32_t mbi_pa;
> +    uint32_t slrt_pa;
> +} __packed;

Why __packed?

> +void asmlinkage slaunch_early_init(uint32_t load_base_addr,

__init ?

> +                                   uint32_t tgt_base_addr,
> +                                   uint32_t tgt_end_addr,
> +                                   struct early_init_results *result)
> +{
> +    void *txt_heap;
> +    const struct txt_os_mle_data *os_mle;
> +    const struct slr_table *slrt;
> +    const struct slr_entry_intel_info *intel_info;
> +
> +    txt_heap = txt_init();
> +    os_mle = txt_os_mle_data_start(txt_heap);
> +
> +    result->slrt_pa = os_mle->slrt;
> +    result->mbi_pa = 0;
> +
> +    slrt = (const struct slr_table *)(uintptr_t)os_mle->slrt;

I think the cast to uintptr_t wants omitting here. This is 32-bit code, and
hence the conversion to a pointer ought to go fine without. Or else you're
silently discarding bits in the earlier assignment to ->slrt_pa.

> +    intel_info = (const struct slr_entry_intel_info *)
> +        slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_INTEL_INFO);
> +    if ( intel_info == NULL || intel_info->hdr.size != sizeof(*intel_info) )
> +        return;

This size check is best effort only, isn't it? Or else how do you know
->hdr.size is actually within bounds? Further in txt_init() you use less-
than checks; why more relaxed there and more strict here?

> --- a/xen/arch/x86/include/asm/intel-txt.h
> +++ b/xen/arch/x86/include/asm/intel-txt.h
> @@ -292,6 +292,22 @@ static inline void *txt_sinit_mle_data_start(const void 
> *heap)
>             sizeof(uint64_t);
>  }
>  
> +static inline void *txt_init(void)

__init ?

> --- /dev/null
> +++ b/xen/arch/x86/include/asm/slaunch.h
> @@ -0,0 +1,26 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright (c) 2022-2025 3mdeb Sp. z o.o. All rights reserved.
> + */
> +
> +#ifndef X86_SLAUNCH_H
> +#define X86_SLAUNCH_H
> +
> +#include <xen/types.h>
> +
> +/* Indicates an active Secure Launch boot. */
> +extern bool slaunch_active;
> +
> +/*
> + * Holds physical address of SLRT.  Use slaunch_get_slrt() to access SLRT
> + * instead of mapping where this points to.
> + */
> +extern uint32_t slaunch_slrt;
> +
> +/*
> + * Retrieves pointer to SLRT.  Checks table's validity and maps it as 
> necessary.
> + */
> +struct slr_table *slaunch_get_slrt(void);

There's no definition of this here, nor a use. Why is this living in this
patch? Misra objects to declarations without definitions, and you want to
be prepared that such a large series may go in piece by piece. Hence there
may not be new Misra violations at any patch boundary.

> --- /dev/null
> +++ b/xen/arch/x86/slaunch.c
> @@ -0,0 +1,27 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright (c) 2022-2025 3mdeb Sp. z o.o. All rights reserved.
> + */
> +
> +#include <xen/compiler.h>
> +#include <xen/init.h>
> +#include <xen/macros.h>
> +#include <xen/types.h>

Looks like all you need here is xen/stdint.h?

> +#include <asm/slaunch.h>

We try to move to there being blanks lines between groups of #include-s,
e.g. all xen/ ones separated from all asm/ ones.

> +/*
> + * These variables are assigned to by the code near Xen's entry point.
> + *
> + * slaunch_active is not __initdata to allow checking for an active Secure
> + * Launch boot.
> + */
> +bool slaunch_active;

Not using __initdata is quite plausible, but why not __ro_after_init?

> +uint32_t __initdata slaunch_slrt; /* physical address */
> +
> +/* Using slaunch_active in head.S assumes it's a single byte in size, so 
> enforce
> + * this assumption. */

Please follow comment style as per ./CODING_STYLE.

Jan