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

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Thu, Jan 22, 2026 at 05:41:10PM +0100, Jan Beulich wrote:
> 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.)

Will export position of those entries from assembly and compute CS
selector in C.

> > @@ -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.

Right.

> > +        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?

This code is now called from assembly before start_secondary() and it
receives APIC ID as an argument, so there is no get_apic_id() or
x2apic_ap_setup().

> > +        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.

`monitor` and `mwait` are now done separately to be able to check
whether condition is satisfied between them.

> > +                          "d"(0) : "memory");
> > +            cpu = smp_processor_id();
>
> What purpose does this serve?
>
> Jan

I think `cpu` wasn't necessarily correct at this point because AP could
get here before BSP set up its structure.  As above, this is also gone
in the new version (the old one stopped working after some upstream
changes related to CPU init).

Regards