Re: [RFC Part1 PATCH v3 14/17] x86/boot: Add early boot support when running with SEV active

[Tom Lendacky <thomas.lendacky@amd.com>]

On 8/23/2017 10:30 AM, Borislav Petkov wrote:
> On Mon, Jul 24, 2017 at 02:07:54PM -0500, Brijesh Singh wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> Early in the boot process, add checks to determine if the kernel is
>> running with Secure Encrypted Virtualization (SEV) active.
>>
>> Checking for SEV requires checking that the kernel is running under a
>> hypervisor (CPUID 0x00000001, bit 31), that the SEV feature is available
>> (CPUID 0x8000001f, bit 1) and then check a non-interceptable SEV MSR
>> (0xc0010131, bit 0).
>>
>> This check is required so that during early compressed kernel booting the
>> pagetables (both the boot pagetables and KASLR pagetables (if enabled) are
>> updated to include the encryption mask so that when the kernel is
>> decompressed into encrypted memory.
> 
> 				    , it can boot properly.
> 
> :)
> 

Yup, kinda didn't complete that sentence.

>> After the kernel is decompressed and continues booting the same logic is
>> used to check if SEV is active and set a flag indicating so.  This allows
>> us to distinguish between SME and SEV, each of which have unique
>> differences in how certain things are handled: e.g. DMA (always bounce
>> buffered with SEV) or EFI tables (always access decrypted with SME).
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   arch/x86/boot/compressed/Makefile      |   2 +
>>   arch/x86/boot/compressed/head_64.S     |  16 +++++
>>   arch/x86/boot/compressed/mem_encrypt.S | 103 +++++++++++++++++++++++++++++++++
>>   arch/x86/boot/compressed/misc.h        |   2 +
>>   arch/x86/boot/compressed/pagetable.c   |   8 ++-
>>   arch/x86/include/asm/mem_encrypt.h     |   3 +
>>   arch/x86/include/asm/msr-index.h       |   3 +
>>   arch/x86/include/uapi/asm/kvm_para.h   |   1 -
>>   arch/x86/mm/mem_encrypt.c              |  42 +++++++++++---
>>   9 files changed, 169 insertions(+), 11 deletions(-)
>>   create mode 100644 arch/x86/boot/compressed/mem_encrypt.S
>>
>> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
>> index 2c860ad..d2fe901 100644
>> --- a/arch/x86/boot/compressed/Makefile
>> +++ b/arch/x86/boot/compressed/Makefile
>> @@ -72,6 +72,8 @@ vmlinux-objs-y := $(obj)/vmlinux.lds $(obj)/head_$(BITS).o $(obj)/misc.o \
>>   	$(obj)/string.o $(obj)/cmdline.o $(obj)/error.o \
>>   	$(obj)/piggy.o $(obj)/cpuflags.o
>>   
>> +vmlinux-objs-$(CONFIG_X86_64) += $(obj)/mem_encrypt.o
> 
> There's a
> 
> ifdef CONFIG_X86_64
> 
> a couple of lines below. Just put it there.

Will do.

> 
> ...
> 
>> +++ b/arch/x86/boot/compressed/mem_encrypt.S
>> @@ -0,0 +1,103 @@
>> +/*
>> + * AMD Memory Encryption Support
>> + *
>> + * Copyright (C) 2017 Advanced Micro Devices, Inc.
>> + *
>> + * Author: Tom Lendacky <thomas.lendacky@amd.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/linkage.h>
>> +
>> +#include <asm/processor-flags.h>
>> +#include <asm/msr.h>
>> +#include <asm/asm-offsets.h>
>> +
>> +	.text
>> +	.code32
>> +ENTRY(get_sev_encryption_bit)
>> +	xor	%eax, %eax
>> +
>> +#ifdef CONFIG_AMD_MEM_ENCRYPT
>> +	push	%ebx
>> +	push	%ecx
>> +	push	%edx
>> +
>> +	/* Check if running under a hypervisor */
>> +	movl	$1, %eax
>> +	cpuid
>> +	bt	$31, %ecx		/* Check the hypervisor bit */
>> +	jnc	.Lno_sev
>> +
>> +	movl	$0x80000000, %eax	/* CPUID to check the highest leaf */
>> +	cpuid
>> +	cmpl	$0x8000001f, %eax	/* See if 0x8000001f is available */
>> +	jb	.Lno_sev
>> +
>> +	/*
>> +	 * Check for the SEV feature:
>> +	 *   CPUID Fn8000_001F[EAX] - Bit 1
>> +	 *   CPUID Fn8000_001F[EBX] - Bits 5:0
>> +	 *     Pagetable bit position used to indicate encryption
>> +	 */
>> +	movl	$0x8000001f, %eax
>> +	cpuid
>> +	bt	$1, %eax		/* Check if SEV is available */
>> +	jnc	.Lno_sev
>> +
>> +	movl	$MSR_F17H_SEV, %ecx	/* Read the SEV MSR */
>> +	rdmsr
>> +	bt	$MSR_F17H_SEV_ENABLED_BIT, %eax	/* Check if SEV is active */
>> +	jnc	.Lno_sev
>> +
>> +	/*
>> +	 * Get memory encryption information:
>> +	 */
> 
> The side-comment is enough. This one above can go.

Done.

> 
>> +	movl	%ebx, %eax
>> +	andl	$0x3f, %eax		/* Return the encryption bit location */
>> +	jmp	.Lsev_exit
>> +
>> +.Lno_sev:
>> +	xor	%eax, %eax
>> +
>> +.Lsev_exit:
>> +	pop	%edx
>> +	pop	%ecx
>> +	pop	%ebx
>> +
>> +#endif	/* CONFIG_AMD_MEM_ENCRYPT */
>> +
>> +	ret
>> +ENDPROC(get_sev_encryption_bit)
>> +
>> +	.code64
>> +ENTRY(get_sev_encryption_mask)
>> +	xor	%rax, %rax
>> +
>> +#ifdef CONFIG_AMD_MEM_ENCRYPT
>> +	push	%rbp
>> +	push	%rdx
>> +
>> +	movq	%rsp, %rbp		/* Save current stack pointer */
>> +
>> +	call	get_sev_encryption_bit	/* Get the encryption bit position */
> 
> So we get to call get_sev_encryption_bit() here again and noodle through
> the CPUID discovery and MSR access. We should instead cache that info
> and return the encryption bit directly on a second call. (And initialize
> it to -1 to denote that get_sev_encryption_bit() hasn't run yet).

Ok, I'll look into that optimization.

> 
> ...
> 
>> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
>> index 9274ec7..9cb6472 100644
>> --- a/arch/x86/include/asm/mem_encrypt.h
>> +++ b/arch/x86/include/asm/mem_encrypt.h
>> @@ -19,6 +19,9 @@
>>   
>>   #include <asm/bootparam.h>
>>   
>> +#define AMD_SME_FEATURE_BIT	BIT(0)
>> +#define AMD_SEV_FEATURE_BIT	BIT(1)
> 
> s/_FEATURE//
> 
> AMD_SME_BIT and AMD_SEV_BIT is good enough :)
> 
> And frankly, if you're going to use them only below in sme_enable() - I
> need to check more thoroughly the remaining patches - but if you only
> are going to use them there, just define them inside the function so
> that they're close.

Sounds good. I believe that is the only place they are/will be used
so I'll make that change.

> 
>> +
>>   #ifdef CONFIG_AMD_MEM_ENCRYPT
>>   
>>   extern unsigned long sme_me_mask;
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index e399d68..530020f 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -326,6 +326,9 @@
>>   
>>   /* Fam 17h MSRs */
>>   #define MSR_F17H_IRPERF			0xc00000e9
>> +#define MSR_F17H_SEV			0xc0010131
> 
> If that MSR is going to be used later on - and I don't see why not - you
> probably should make it an arch one: MSR_AMD64_SEV. Even if it isn't yet
> officially. :-)
> 

Will do.

>> +#define MSR_F17H_SEV_ENABLED_BIT	0
>> +#define MSR_F17H_SEV_ENABLED		BIT_ULL(MSR_F17H_SEV_ENABLED_BIT)
>>   
>>   /* Fam 16h MSRs */
>>   #define MSR_F16H_L2I_PERF_CTL		0xc0010230
>> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
>> index a965e5b..c202ba3 100644
>> --- a/arch/x86/include/uapi/asm/kvm_para.h
>> +++ b/arch/x86/include/uapi/asm/kvm_para.h
>> @@ -109,5 +109,4 @@ struct kvm_vcpu_pv_apf_data {
>>   #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
>>   #define KVM_PV_EOI_DISABLED 0x0
>>   
>> -
>>   #endif /* _UAPI_ASM_X86_KVM_PARA_H */
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 5e5d460..ed8780e 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -288,7 +288,9 @@ void __init mem_encrypt_init(void)
>>   	if (sev_active())
>>   		dma_ops = &sme_dma_ops;
>>   
>> -	pr_info("AMD Secure Memory Encryption (SME) active\n");
>> +	pr_info("AMD %s active\n",
>> +		sev_active() ? "Secure Encrypted Virtualization (SEV)"
>> +			     : "Secure Memory Encryption (SME)");
>>   }
>>   
>>   void swiotlb_set_mem_attributes(void *vaddr, unsigned long size)
>> @@ -616,12 +618,23 @@ void __init __nostackprotector sme_enable(struct boot_params *bp)
>>   {
>>   	const char *cmdline_ptr, *cmdline_arg, *cmdline_on, *cmdline_off;
>>   	unsigned int eax, ebx, ecx, edx;
>> +	unsigned long feature_mask;
>>   	bool active_by_default;
>>   	unsigned long me_mask;
>>   	char buffer[16];
>>   	u64 msr;
>>   
>> -	/* Check for the SME support leaf */
>> +	/*
>> +	 * Set the feature mask (SME or SEV) based on whether we are
>> +	 * running under a hypervisor.
>> +	 */
>> +	eax = 1;
>> +	ecx = 0;
>> +	native_cpuid(&eax, &ebx, &ecx, &edx);
>> +	feature_mask = (ecx & BIT(31)) ? AMD_SEV_FEATURE_BIT
>> +				       : AMD_SME_FEATURE_BIT;
> 
> Set that feature mask before using it...
> 
>> +
>> +	/* Check for the SME/SEV support leaf */
> 
> ... because if that check exits due to no SME leaf, you're uselessly
> doing all the above.

Ok, I'll move that down after the leaf check.

> 
>>   	eax = 0x80000000;
>>   	ecx = 0;
>>   	native_cpuid(&eax, &ebx, &ecx, &edx);
>> @@ -629,24 +642,39 @@ void __init __nostackprotector sme_enable(struct boot_params *bp)
>>   		return;
>>   
>>   	/*
>> -	 * Check for the SME feature:
>> +	 * Check for the SME/SEV feature:
>>   	 *   CPUID Fn8000_001F[EAX] - Bit 0
>>   	 *     Secure Memory Encryption support
>> +	 *   CPUID Fn8000_001F[EAX] - Bit 1
> 
> No need to repeat the CPUID leaf here - only Bit 1:
> 
>           *   CPUID Fn8000_001F[EAX]
> 	 * - Bit 0:  Secure Memory Encryption support
>           * - Bit 1:  Secure Encrypted Virtualization support
> 

Ok, I'll clean that up.

Thanks,
Tom

> 
>> +	 *     Secure Encrypted Virtualization support
>>   	 *   CPUID Fn8000_001F[EBX] - Bits 5:0
>>   	 *     Pagetable bit position used to indicate encryption
>>   	 */
>>   	eax = 0x8000001f;
>>   	ecx = 0;
>>   	native_cpuid(&eax, &ebx, &ecx, &edx);
>> -	if (!(eax & 1))
>> +	if (!(eax & feature_mask))
>>   		return;
>>   
>>   	me_mask = 1UL << (ebx & 0x3f);
>>   
>> -	/* Check if SME is enabled */
>> -	msr = __rdmsr(MSR_K8_SYSCFG);
>> -	if (!(msr & MSR_K8_SYSCFG_MEM_ENCRYPT))
>> +	/* Check if memory encryption is enabled */
>> +	if (feature_mask == AMD_SME_FEATURE_BIT) {
>> +		/* For SME, check the SYSCFG MSR */
>> +		msr = __rdmsr(MSR_K8_SYSCFG);
>> +		if (!(msr & MSR_K8_SYSCFG_MEM_ENCRYPT))
>> +			return;
>> +	} else {
>> +		/* For SEV, check the SEV MSR */
>> +		msr = __rdmsr(MSR_F17H_SEV);
>> +		if (!(msr & MSR_F17H_SEV_ENABLED))
>> +			return;
>> +
>> +		/* SEV state cannot be controlled by a command line option */
>> +		sme_me_mask = me_mask;
>> +		sev_enabled = 1;
>>   		return;
>> +	}
> 
> Nice. Two birds with one stone is always better. :)
> 
>