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
