Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1596083 > unrolled thread

Re: [RFC PATCH v2 12/32] x86: Add early boot support when running with SEV active

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH v2 12/32] x86: Add early boot support when running  with SEV active Borislav Petkov <bp@suse.de> - 2017-03-09 15:10 +0100
    Re: [RFC PATCH v2 12/32] x86: Add early boot support when running  with SEV active Paolo Bonzini <pbonzini@redhat.com> - 2017-03-09 17:20 +0100
      Re: [RFC PATCH v2 12/32] x86: Add early boot support when running  with SEV active Borislav Petkov <bp@suse.de> - 2017-03-09 17:40 +0100

#1596083 — Re: [RFC PATCH v2 12/32] x86: Add early boot support when running with SEV active

On Thu, Mar 02, 2017 at 10:14:48AM -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 by issuing
> a CPUID instruction.
> 
> During early compressed kernel booting, if SEV is active the pagetables are
> updated so that data is accessed and decompressed with encryption.
> 
> During uncompressed kernel booting, if SEV is the memory encryption mask is
> set and a flag is set to indicate that SEV is enabled.

I don't know how many times I have to say this but I'm going to keep
doing it until it sticks: :-)

Please, no "WHAT" in the commit messages - I can see the "WHAT - but
"WHY".

Ok?

> diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
> new file mode 100644
> index 0000000..8313c31
> --- /dev/null
> +++ b/arch/x86/boot/compressed/mem_encrypt.S
> @@ -0,0 +1,75 @@
> +/*
> + * AMD Memory Encryption Support
> + *
> + * Copyright (C) 2016 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>
> +#include <uapi/asm/kvm_para.h>
> +
> +	.text
> +	.code32
> +ENTRY(sev_enabled)
> +	xor	%eax, %eax
> +
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +	push	%ebx
> +	push	%ecx
> +	push	%edx
> +
> +	/* Check if running under a hypervisor */
> +	movl	$0x40000000, %eax
> +	cpuid
> +	cmpl	$0x40000001, %eax
> +	jb	.Lno_sev
> +
> +	movl	$0x40000001, %eax
> +	cpuid
> +	bt	$KVM_FEATURE_SEV, %eax
> +	jnc	.Lno_sev
> +
> +	/*
> +	 * Check for memory encryption feature:
> +	 *   CPUID Fn8000_001F[EAX] - Bit 0
> +	 */
> +	movl	$0x8000001f, %eax
> +	cpuid
> +	bt	$0, %eax
> +	jnc	.Lno_sev
> +
> +	/*
> +	 * Get memory encryption information:
> +	 *   CPUID Fn8000_001F[EBX] - Bits 5:0
> +	 *     Pagetable bit position used to indicate encryption
> +	 */
> +	movl	%ebx, %eax
> +	andl	$0x3f, %eax
> +	movl	%eax, sev_enc_bit(%ebp)
> +	jmp	.Lsev_exit
> +
> +.Lno_sev:
> +	xor	%eax, %eax
> +
> +.Lsev_exit:
> +	pop	%edx
> +	pop	%ecx
> +	pop	%ebx
> +
> +#endif	/* CONFIG_AMD_MEM_ENCRYPT */
> +
> +	ret
> +ENDPROC(sev_enabled)

Right, as said in another mail earlier, this could be written in C. And
then the sme_enable() piece below looks the same as this one above. So
since you want to run it before kernel decompression and after, you
could extract this code into a separate .c file which you can link in
both places, similar to what we do with verify_cpu with the difference
that verify_cpu is getting included.

Alternatively, we still have some room in setup_header.xloadflags to
pass boot info to kernel proper from before the decompression stage.

But I'd prefer linking with both stages as it is cheaper and those flags
we can use for something which really wants to use a flag like that.

> diff --git a/arch/x86/kernel/mem_encrypt_init.c b/arch/x86/kernel/mem_encrypt_init.c
> index 35c5e3d..5d514e6 100644
> --- a/arch/x86/kernel/mem_encrypt_init.c
> +++ b/arch/x86/kernel/mem_encrypt_init.c
> @@ -22,6 +22,7 @@
>  #include <asm/processor-flags.h>
>  #include <asm/msr.h>
>  #include <asm/cmdline.h>
> +#include <asm/kvm_para.h>
>  
>  static char sme_cmdline_arg_on[] __initdata = "mem_encrypt=on";
>  static char sme_cmdline_arg_off[] __initdata = "mem_encrypt=off";
> @@ -232,6 +233,29 @@ unsigned long __init sme_enable(void *boot_data)
>  	void *cmdline_arg;
>  	u64 msr;
>  
> +	/* Check if running under a hypervisor */
> +	eax = 0x40000000;
> +	ecx = 0;
> +	native_cpuid(&eax, &ebx, &ecx, &edx);
> +	if (eax > 0x40000000) {
> +		eax = 0x40000001;
> +		ecx = 0;
> +		native_cpuid(&eax, &ebx, &ecx, &edx);
> +		if (!(eax & BIT(KVM_FEATURE_SEV)))
> +			goto out;
> +
> +		eax = 0x8000001f;
> +		ecx = 0;
> +		native_cpuid(&eax, &ebx, &ecx, &edx);
> +		if (!(eax & 1))
> +			goto out;
> +
> +		sme_me_mask = 1UL << (ebx & 0x3f);
> +		sev_enabled = 1;
> +
> +		goto out;
> +	}
> +
>  	/* Check for an AMD processor */
>  	eax = 0;
>  	ecx = 0;
> 

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [next] | [standalone]

#1596181

On 09/03/2017 15:07, Borislav Petkov wrote:
> +	/* Check if running under a hypervisor */
> +	eax = 0x40000000;
> +	ecx = 0;
> +	native_cpuid(&eax, &ebx, &ecx, &edx);

This is not how you check if running under a hypervisor; you should
check the HYPERVISOR bit, i.e. bit 31 of cpuid(1).ecx.  This in turn
tells you if leaf 0x40000000 is valid.

That said, the main issue with this function is that it hardcodes the
behavior for KVM.  It is possible that another hypervisor defines its
0x40000001 leaf in such a way that KVM_FEATURE_SEV has a different meaning.

Instead, AMD should define a "well-known" bit in its own space (i.e.
0x800000xx) that is only used by hypervisors that support SEV.  This is
similar to how Intel defined one bit in leaf 1 to say "is leaf
0x40000000 valid".

Thanks,

Paolo

> +	if (eax > 0x40000000) {
> +		eax = 0x40000001;
> +		ecx = 0;
> +		native_cpuid(&eax, &ebx, &ecx, &edx);
> +		if (!(eax & BIT(KVM_FEATURE_SEV)))
> +			goto out;
> +
> +		eax = 0x8000001f;
> +		ecx = 0;
> +		native_cpuid(&eax, &ebx, &ecx, &edx);
> +		if (!(eax & 1))
> +			goto out;
> +
> +		sme_me_mask = 1UL << (ebx & 0x3f);
> +		sev_enabled = 1;
> +
> +		goto out;
> +	}
> +

[toc] | [prev] | [next] | [standalone]

#1596190

On Thu, Mar 09, 2017 at 05:13:33PM +0100, Paolo Bonzini wrote:
> This is not how you check if running under a hypervisor; you should
> check the HYPERVISOR bit, i.e. bit 31 of cpuid(1).ecx.  This in turn
> tells you if leaf 0x40000000 is valid.

Ah, good point, I already do that in the microcode loader :)

        /*
         * CPUID(1).ECX[31]: reserved for hypervisor use. This is still not
         * completely accurate as xen pv guests don't see that CPUID bit set but
         * that's good enough as they don't land on the BSP path anyway.
         */
        if (native_cpuid_ecx(1) & BIT(31))
                return *res;

> That said, the main issue with this function is that it hardcodes the
> behavior for KVM.  It is possible that another hypervisor defines its
> 0x40000001 leaf in such a way that KVM_FEATURE_SEV has a different meaning.
> 
> Instead, AMD should define a "well-known" bit in its own space (i.e.
> 0x800000xx) that is only used by hypervisors that support SEV.  This is
> similar to how Intel defined one bit in leaf 1 to say "is leaf
> 0x40000000 valid".
> 
> > +	if (eax > 0x40000000) {
> > +		eax = 0x40000001;
> > +		ecx = 0;
> > +		native_cpuid(&eax, &ebx, &ecx, &edx);
> > +		if (!(eax & BIT(KVM_FEATURE_SEV)))
> > +			goto out;
> > +
> > +		eax = 0x8000001f;
> > +		ecx = 0;
> > +		native_cpuid(&eax, &ebx, &ecx, &edx);
> > +		if (!(eax & 1))

Right, so this is testing CPUID_0x8000001f_ECX(0)[0], SME. Why not
simply set that bit for the guest too, in kvm?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web