Re: [PATCH v3 15/22] x86/smpboot.c: TXT AP bringup

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2025 15:17, Sergii Dmytruk wrote:
> @@ -154,6 +164,13 @@ gdt_48:
>          .quad   0x00cf93000000ffff /* 0x0018: ring 0 data */
>          .quad   0x00009b000000ffff /* 0x0020: real-mode code @ 
> BOOT_TRAMPOLINE */
>          .quad   0x000093000000ffff /* 0x0028: real-mode data @ 
> BOOT_TRAMPOLINE */
> +        /*
> +         * Intel TXT requires these two in exact order. This isn't compatible
> +         * with order required by syscall, so we have duplicated entries...
> +         * If order ever changes, update selector numbers in asm/intel-txt.h.
> +         */
> +        .quad   0x00cf9b000000ffff /* 0x0030: ring 0 code, 32-bit mode */
> +        .quad   0x00cf93000000ffff /* 0x0038: ring 0 data */

Especially since the corresponding #define-s sit ...

> --- a/xen/arch/x86/include/asm/intel-txt.h
> +++ b/xen/arch/x86/include/asm/intel-txt.h
> @@ -91,6 +91,9 @@
>  
>  #define SLAUNCH_BOOTLOADER_MAGIC             0x4c534254
>  
> +#define TXT_AP_BOOT_CS                  0x0030
> +#define TXT_AP_BOOT_DS                  0x0038

... entirely elsewhere, I think at least the comments above want to mention
these names. (Even better would be to not hard-code these numbers, or to
use the numbers to establish the offsets in trampoline_gdt.)

> @@ -321,6 +323,29 @@ void asmlinkage start_secondary(void)
>      struct cpu_info *info = get_cpu_info();
>      unsigned int cpu = smp_processor_id();
>  
> +    if ( ap_boot_method == AP_BOOT_TXT ) {

Style nit (also again later): Brace on its own line please.

> +        uint64_t misc_enable;
> +        uint32_t my_apicid;
> +        struct txt_sinit_mle_data *sinit_mle =
> +              txt_sinit_mle_data_start(__va(txt_read(TXTCR_HEAP_BASE)));
> +
> +        /* TXT released us with MONITOR disabled in IA32_MISC_ENABLE. */
> +        rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> +        wrmsrl(MSR_IA32_MISC_ENABLE,
> +               misc_enable | MSR_IA32_MISC_ENABLE_MONITOR_ENABLE);
> +
> +        /* get_apic_id() reads from x2APIC if it thinks it is enabled. */
> +        x2apic_ap_setup();
> +        my_apicid = get_apic_id();

Despite the comment putting the call to x2apic_ap_setup() here looks rather
arbitrary. Also you do nothing about the other call from smp_callin(). Surely
the function better wouldn't be called twice?

> +        while ( my_apicid != x86_cpu_to_apicid[cpu] ) {
> +            asm volatile ("monitor; xor %0,%0; mwait"
> +                          :: "a"(__va(sinit_mle->rlp_wakeup_addr)), "c"(0),

You alter %0, so it can't be just an input.

> +                          "d"(0) : "memory");
> +            cpu = smp_processor_id();

What purpose does this serve?

Jan