Re: [PATCH] x86/boot: Fix load_system_tables() to be NMI/#MC-safe

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/05/2020 14:19, Jan Beulich wrote:
> On 27.05.2020 15:06, Andrew Cooper wrote:
>> @@ -720,30 +721,26 @@ void load_system_tables(void)
>>  		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>  	};
>>  
>> -	*tss = (struct tss64){
>> -		/* Main stack for interrupts/exceptions. */
>> -		.rsp0 = stack_bottom,
>> -
>> -		/* Ring 1 and 2 stacks poisoned. */
>> -		.rsp1 = 0x8600111111111111ul,
>> -		.rsp2 = 0x8600111111111111ul,
>> -
>> -		/*
>> -		 * MCE, NMI and Double Fault handlers get their own stacks.
>> -		 * All others poisoned.
>> -		 */
>> -		.ist = {
>> -			[IST_MCE - 1] = stack_top + IST_MCE * PAGE_SIZE,
>> -			[IST_DF  - 1] = stack_top + IST_DF  * PAGE_SIZE,
>> -			[IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE,
>> -			[IST_DB  - 1] = stack_top + IST_DB  * PAGE_SIZE,
>> -
>> -			[IST_MAX ... ARRAY_SIZE(tss->ist) - 1] =
>> -				0x8600111111111111ul,
>> -		},
>> -
>> -		.bitmap = IOBMP_INVALID_OFFSET,
>> -	};
>> +	/*
>> +	 * Set up the TSS.  Warning - may be live, and the NMI/#MC must remain
>> +	 * valid on every instruction boundary.  (Note: these are all
>> +	 * semantically ACCESS_ONCE() due to tss's volatile qualifier.)
>> +	 *
>> +	 * rsp0 refers to the primary stack.  #MC, #DF, NMI and #DB handlers
>> +	 * each get their own stacks.  No IO Bitmap.
>> +	 */
>> +	tss->rsp0 = stack_bottom;
>> +	tss->ist[IST_MCE - 1] = stack_top + IST_MCE * PAGE_SIZE;
>> +	tss->ist[IST_DF  - 1] = stack_top + IST_DF  * PAGE_SIZE;
>> +	tss->ist[IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE;
>> +	tss->ist[IST_DB  - 1] = stack_top + IST_DB  * PAGE_SIZE;
>> +	tss->bitmap = IOBMP_INVALID_OFFSET;
>> +
>> +	/* All other stack pointers poisioned. */
>> +	for ( i = IST_MAX; i < ARRAY_SIZE(tss->ist); ++i )
>> +		tss->ist[i] = 0x8600111111111111ul;
>> +	tss->rsp1 = 0x8600111111111111ul;
>> +	tss->rsp2 = 0x8600111111111111ul;
> ACCESS_ONCE() unfortunately only has one of the two needed effects:
> It guarantees that each memory location gets accessed exactly once
> (which I assume can also be had with just the volatile addition,
> but without the moving away from using an initializer), but it does
> not guarantee single-insn accesses.

Linux's memory-barriers.txt disagrees, and specifically gives an example
with a misaligned int (vs two shorts) and the use volatile cast (by way
of {READ,WRITE}_ONCE()) to prevent load/store tearing, as the memory
location is of a size which can be fit in a single access.

I'm fairly sure we're safe here.

>  I consider this in particular
> relevant here because all of the 64-bit fields are misaligned. By
> doing it like you do, we're setting us up to have to re-do this yet
> again in a couple of years time (presumably using write_atomic()
> instead then).
>
> Nevertheless it is a clear improvement, so if you want to leave it
> like this
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew