Re: [PATCH v3 03/22] x86/boot: add MLE header and Secure Launch entry point

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2025 15:17, Sergii Dmytruk wrote:
> From: Kacper Stojek <kacper.stojek@xxxxxxxxx>
> 
> Signed-off-by: Kacper Stojek <kacper.stojek@xxxxxxxxx>
> Signed-off-by: Krystian Hebel <krystian.hebel@xxxxxxxxx>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>

Such a change can hardly come without any description. As just one aspect,
neither here nor ...

> --- a/docs/hypervisor-guide/x86/how-xen-boots.rst
> +++ b/docs/hypervisor-guide/x86/how-xen-boots.rst
> @@ -55,6 +55,11 @@ If ``CONFIG_PVH_GUEST`` was selected at build time, an Elf 
> note is included
>  which indicates the ability to use the PVH boot protocol, and registers
>  ``__pvh_start`` as the entrypoint, entered in 32bit mode.
>  
> +A combination of Multiboot 2 and MLE headers is used to implement DRTM for
> +legacy (BIOS) boot. The separate entry point is used mainly to differentiate

... here the MLE acronym is being deciphered. Same for DRTM here. There's
also no reference anywhere as to some kind of spec (except in the cover
letter, but that won't land in the tree).

> +from other kinds of boots. It moves a magic number to EAX before jumping into
> +common startup code.
> +
>  
>  xen.gz
>  ~~~~~~

Any reason the single blank line is converted to a double one? Generally, in
particular for patch context to be more meaningful, we'd prefer to not have
double blank lines. In documentation they _sometimes_ may be warranted.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -4,6 +4,7 @@
>  #include <public/xen.h>
>  #include <asm/asm_defns.h>
>  #include <asm/fixmap.h>
> +#include <asm/intel-txt.h>
>  #include <asm/page.h>
>  #include <asm/processor.h>
>  #include <asm/msr-index.h>
> @@ -126,6 +127,25 @@ multiboot2_header:
>          .size multiboot2_header, . - multiboot2_header
>          .type multiboot2_header, @object
>  
> +        .balign 16
> +mle_header:
> +        .long   0x9082ac5a  /* UUID0 */
> +        .long   0x74a7476f  /* UUID1 */
> +        .long   0xa2555c0f  /* UUID2 */
> +        .long   0x42b651cb  /* UUID3 */
> +        .long   0x00000034  /* MLE header size */

Better use an expression (difference of two labels)?

> +        .long   0x00020002  /* MLE version 2.2 */
> +        .long   (slaunch_stub_entry - start)  /* Linear entry point of MLE 
> (SINIT virt. address) */
> +        .long   0x00000000  /* First valid page of MLE */
> +        .long   0x00000000  /* Offset within binary of first byte of MLE */
> +        .long   (_end - start)  /* Offset within binary of last byte + 1 of 
> MLE */

Is the data here describing xen.gz or (rather) xen.efi? In the latter case,
does data past _end (in particular the .reloc section) not matter here?

> +        .long   0x00000723  /* Bit vector of MLE-supported capabilities */
> +        .long   0x00000000  /* Starting linear address of command line 
> (unused) */
> +        .long   0x00000000  /* Ending linear address of command line 
> (unused) */
> +
> +        .size mle_header, .-mle_header
> +        .type mle_header, @object

Please use what xen/linkage.h provides now.

However, the entire additions here and below likely want to go inside some
#ifdef CONFIG_xyz, just like additions in subsequent patches. Which obviously
would require a suitable Kconfig option to be introduced up front.

> @@ -332,6 +352,38 @@ cs32_switch:
>          /* Jump to earlier loaded address. */
>          jmp     *%edi
>  
> +        /*
> +         * Entry point for TrenchBoot Secure Launch on Intel TXT platforms.
> +         *
> +         * CPU is in 32b protected mode with paging disabled. On entry:
> +         * - %ebx = %eip = MLE entry point,
> +         * - stack pointer is undefined,
> +         * - CS is flat 4GB code segment,
> +         * - DS, ES, SS, FS and GS are undefined according to TXT SDG, but 
> this
> +         *   would make it impossible to initialize GDTR, because GDT base 
> must
> +         *   be relocated in the descriptor, which requires write access that
> +         *   CS doesn't provide. Instead we have to assume that DS is set by
> +         *   SINIT ACM as flat 4GB data segment.

Do you really _have to_? At least as plausibly SS might be properly set up,
while DS might not be.

> +         * Additional restrictions:
> +         * - some MSRs are partially cleared, among them IA32_MISC_ENABLE, so
> +         *   some capabilities might be reported as disabled even if they are
> +         *   supported by CPU
> +         * - interrupts (including NMIs and SMIs) are disabled and must be
> +         *   enabled later
> +         * - trying to enter real mode results in reset
> +         * - APs must be brought up by MONITOR or GETSEC[WAKEUP], depending 
> on
> +         *   which is supported by a given SINIT ACM

I'm curious: How would MONITOR allow to bring up an AP? That's not even a
memory access.

> +         */
> +slaunch_stub_entry:
> +        /* Calculate the load base address. */
> +        mov     %ebx, %esi
> +        sub     $sym_offs(slaunch_stub_entry), %esi
> +
> +        /* Mark Secure Launch boot protocol and jump to common entry. */
> +        mov     $SLAUNCH_BOOTLOADER_MAGIC, %eax

While I understand you can't add real handling of this case just yet, wouldn't
it be better to at least cover the case by checking for this magic later, and
in that case enter, say, an infinite loop? You don't want to give the wrong
impression of this path functioning, do you?

Jan