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

[Ross Philipson <ross.philipson@oracle.com>]

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec