Re: [PATCH] x86/boot: Fix kexec booting failure after SEV early boot support

[Lendacky, Thomas]

On 09/26/2018 06:22 AM, Baoquan He wrote:
> On 09/26/18 at 03:32pm, Baoquan He wrote:
>> On 09/25/18 at 07:26pm, Borislav Petkov wrote:
>>> IINM, the problem can be addressed in a simpler way by getting rid of
>>> enc_bit and thus getting rid of the need to do relative addressing of
>>> anything and simply doing the whole dance of figuring out the C-bit each
>>> time. It probably wouldn't be even measurable...
>>
>> Couldn't agree more.
>>
>> Obviously enc_bit is redundent here. We only check eax each time,
>> removing it can fix the RIP-relative addressing issue in kexec.
> 
> OK, in distros CONFIG_AMD_MEM_ENCRYPT=y is set by default usually.
> enc_bit can save once in normal boot, then fetch and skip the cpuid
> detection in initialize_identity_maps(). However this only speeds up in
> amd system with SME, on intel cpu and amd cpu w/o sme, it still needs to
> do cpuid twice. Removing it should be not measurable as Boris said.
> Not sure if Tom has other concern.

No concern from me.  The original version of the patch did not cache the
value, that was added based on the patch series feedback.  So, if there
is no concern about executing some extra CPUID/RDMSR instructions, then
it would certainly simplify the code quite a bit.

Thanks,
Tom

> 
> Thanks
> Baoquan
> 
>>
>> diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
>> index eaa843a52907..0b60eb867d25 100644
>> --- a/arch/x86/boot/compressed/mem_encrypt.S
>> +++ b/arch/x86/boot/compressed/mem_encrypt.S
>> @@ -27,19 +27,6 @@ ENTRY(get_sev_encryption_bit)
>>  	push	%edx
>>  	push	%edi
>>  
>> -	/*
>> -	 * RIP-relative addressing is needed to access the encryption bit
>> -	 * variable. Since we are running in 32-bit mode we need this call/pop
>> -	 * sequence to get the proper relative addressing.
>> -	 */
>> -	call	1f
>> -1:	popl	%edi
>> -	subl	$1b, %edi
>> -
>> -	movl	enc_bit(%edi), %eax
>> -	cmpl	$0, %eax
>> -	jge	.Lsev_exit
>> -
>>  	/* Check if running under a hypervisor */
>>  	movl	$1, %eax
>>  	cpuid
>> @@ -69,12 +56,10 @@ ENTRY(get_sev_encryption_bit)
>>  
>>  	movl	%ebx, %eax
>>  	andl	$0x3f, %eax		/* Return the encryption bit location */
>> -	movl	%eax, enc_bit(%edi)
>>  	jmp	.Lsev_exit
>>  
>>  .Lno_sev:
>>  	xor	%eax, %eax
>> -	movl	%eax, enc_bit(%edi)
>>  
>>  .Lsev_exit:
>>  	pop	%edi
>> @@ -113,9 +98,6 @@ ENTRY(set_sev_encryption_mask)
>>  ENDPROC(set_sev_encryption_mask)
>>  
>>  	.data
>> -enc_bit:
>> -	.int	0xffffffff
>> -
>>  #ifdef CONFIG_AMD_MEM_ENCRYPT
>>  	.balign	8
>>  GLOBAL(sme_me_mask)