Re: [PATCH v6 07/14] x86: Secure Launch kernel early boot stub

[Ross Philipson]

On 5/12/23 14:04, Thomas Gleixner wrote:
> On Thu, May 04 2023 at 14:50, Ross Philipson wrote:
> > +
> > +/* CPUID: leaf 1, ECX, SMX feature bit */
> > +#define X86_FEATURE_BIT_SMX	(1 << 6)
> > +
> > +/* Can't include apiddef.h in asm */
>
> Why not? All it needs is a #ifndef __ASSEMBLY__ guard around the C parts.

I guess I was reluctant to fiddle with another header file but I can do 
this.

> > +#define XAPIC_ENABLE	(1 << 11)
> > +#define X2APIC_ENABLE	(1 << 10)
> > +
> > +/* Can't include traps.h in asm */
>
> NMI_VECTOR is defined in irq_vectors.h which just has a include
> <linux/threads.h> for no real good reason.

Ack

> > +#define X86_TRAP_NMI	2
>
> <SNIP>
>
> > +/*
> > + * See the comment in head_64.S for detailed informatoin on what this macro
> > + * is used for.
> > + */
> > +#define rva(X) ((X) - sl_stub_entry)
>
> I'm having a hard time to find that comment in head_64.S. At least it's
> not in this patch.

The is a macro very much like this one and large comment in head_64.S. I 
am just referencing that. If you would rather see that comment 
duplicated here, I can.

> > +.Lsl_ap_cs:
> > +	/* Load the relocated AP IDT */
> [ 11 more citation lines. Click/Enter to show. ]
> > +	lidt	(sl_ap_idt_desc - sl_txt_ap_wake_begin)(%ecx)
> > +
> > +	/* Fixup MTRRs and misc enable MSR on APs too */
> > +	call	sl_txt_load_regs
> > +
> > +	/* Enable SMI with GETSEC[SMCTRL] */
> > +	GETSEC $(SMX_X86_GETSEC_SMCTRL)
> > +
> > +	/* IRET-to-self can be used to enable NMIs which SENTER disabled */
> > +	leal	rva(.Lnmi_enabled_ap)(%ebx), %eax
> > +	pushfl
> > +	pushl	$(__SL32_CS)
> > +	pushl	%eax
> > +	iret
>
> So from here on any NMI which hits the AP before it can reach the wait
> loop will corrupt EDX...
>
> > +/* This is the beginning of the relocated AP wake code block */
> > +	.global sl_txt_ap_wake_begin
> [ 10 more citation lines. Click/Enter to show. ]
> > +sl_txt_ap_wake_begin:
> > +
> > +	/*
> > +	 * Wait for NMI IPI in the relocated AP wake block which was provided
> > +	 * and protected in the memory map by the prelaunch code. Leave all
> > +	 * other interrupts masked since we do not expect anything but an NMI.
> > +	 */
> > +	xorl	%edx, %edx
> > +
> > +1:
> > +	hlt
> > +	testl	%edx, %edx
> > +	jz	1b
>
> This really makes me nervous. A stray NMI and the AP starts going.
>
> Can't this NMI just bring the AP out of HLT w/o changing any state and
> the AP evaluates a memory location which indicates whether it should
> start up or not.

Given you comments on patch 09, perhaps all this NMI business becomes 
obsolete.

> > +	/*
> > +	 * This is the long absolute jump to the 32b Secure Launch protected
> > +	 * mode stub code in the rmpiggy. The jump address will be fixed in
>
> Providing an actual name for the stub might spare to rummage through
> code to figure out where this is supposed to jump to.

That can be done.

> > +	 * the SMP boot code when the first AP is brought up. This whole area
> > +	 * is provided and protected in the memory map by the prelaunch code.
> [ 2 more citation lines. Click/Enter to show. ]
> > +	 */
> > +	.byte	0xea
> > +sl_ap_jmp_offset:
> > +	.long	0x00000000
> > +	.word	__SL32_CS
>
> Thanks,
>
> 	tglx

Thanks
Ross