diff mbox series

[Part2,RFC,v4,21/40] KVM: SVM: Add initial SEV-SNP support

Message ID 20210707183616.5620-22-brijesh.singh@amd.com
State New
Headers show
Series [Part2,RFC,v4,01/40] KVM: SVM: Add support to handle AP reset MSR protocol | expand

Commit Message

Brijesh Singh July 7, 2021, 6:35 p.m. UTC
The next generation of SEV is called SEV-SNP (Secure Nested Paging).
SEV-SNP builds upon existing SEV and SEV-ES functionality  while adding new
hardware based security protection. SEV-SNP adds strong memory encryption
integrity protection to help prevent malicious hypervisor-based attacks
such as data replay, memory re-mapping, and more, to create an isolated
execution environment.

The SNP feature can be enabled in the KVM by passing the sev-snp module
parameter.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/kvm/svm/sev.c | 18 ++++++++++++++++++
 arch/x86/kvm/svm/svm.h | 12 ++++++++++++
 2 files changed, 30 insertions(+)

Comments

Sean Christopherson July 16, 2021, 6 p.m. UTC | #1
On Wed, Jul 07, 2021, Brijesh Singh wrote:
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c

> index 411ed72f63af..abca2b9dee83 100644

> --- a/arch/x86/kvm/svm/sev.c

> +++ b/arch/x86/kvm/svm/sev.c

> @@ -52,9 +52,14 @@ module_param_named(sev, sev_enabled, bool, 0444);

>  /* enable/disable SEV-ES support */

>  static bool sev_es_enabled = true;

>  module_param_named(sev_es, sev_es_enabled, bool, 0444);

> +

> +/* enable/disable SEV-SNP support */

> +static bool sev_snp_enabled = true;


Is it safe to incrementally introduce SNP support?  Or should the module param
be hidden until all support is in place?  E.g. what will happen when KVM allows
userspace to create SNP guests but doesn't yet have the RMP management added?

> +module_param_named(sev_snp, sev_snp_enabled, bool, 0444);

>  #else

>  #define sev_enabled false

>  #define sev_es_enabled false

> +#define sev_snp_enabled  false

>  #endif /* CONFIG_KVM_AMD_SEV */

>  

>  #define AP_RESET_HOLD_NONE		0

> @@ -1825,6 +1830,7 @@ void __init sev_hardware_setup(void)

>  {

>  #ifdef CONFIG_KVM_AMD_SEV

>  	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;

> +	bool sev_snp_supported = false;

>  	bool sev_es_supported = false;

>  	bool sev_supported = false;

>  

> @@ -1888,9 +1894,21 @@ void __init sev_hardware_setup(void)

>  	pr_info("SEV-ES supported: %u ASIDs\n", sev_es_asid_count);

>  	sev_es_supported = true;

>  

> +	/* SEV-SNP support requested? */

> +	if (!sev_snp_enabled)

> +		goto out;

> +

> +	/* Is SEV-SNP enabled? */

> +	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))


Random question, why use cpu_feature_enabled?  Did something change in cpufeatures
that prevents using boot_cpu_has() here?

> +		goto out;

> +

> +	pr_info("SEV-SNP supported: %u ASIDs\n", min_sev_asid - 1);


Use sev_es_asid_count instead of manually recomputing the same; the latter
obfuscates the fact that ES and SNP share the same ASID pool.

Even better would be to report ES+SNP together, otherwise the user could easily
interpret ES and SNP having separate ASID pools.  And IMO the gotos for SNP are
overkill, e.g.

	sev_es_supported = true;
	sev_snp_supported = sev_snp_enabled &&
			    cpu_feature_enabled(X86_FEATURE_SEV_SNP);

	pr_info("SEV-ES %ssupported: %u ASIDs\n",
		sev_snp_supported ? "and SEV-SNP " : "", sev_es_asid_count);


> +	sev_snp_supported = true;

> +

>  out:

>  	sev_enabled = sev_supported;

>  	sev_es_enabled = sev_es_supported;

> +	sev_snp_enabled = sev_snp_supported;

>  #endif

>  }

>  

> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h

> index 1175edb02d33..b9ea99f8579e 100644

> --- a/arch/x86/kvm/svm/svm.h

> +++ b/arch/x86/kvm/svm/svm.h

> @@ -58,6 +58,7 @@ enum {

>  struct kvm_sev_info {

>  	bool active;		/* SEV enabled guest */

>  	bool es_active;		/* SEV-ES enabled guest */

> +	bool snp_active;	/* SEV-SNP enabled guest */

>  	unsigned int asid;	/* ASID used for this guest */

>  	unsigned int handle;	/* SEV firmware handle */

>  	int fd;			/* SEV device fd */

> @@ -232,6 +233,17 @@ static inline bool sev_es_guest(struct kvm *kvm)

>  #endif

>  }

>  

> +static inline bool sev_snp_guest(struct kvm *kvm)

> +{

> +#ifdef CONFIG_KVM_AMD_SEV

> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> +

> +	return sev_es_guest(kvm) && sev->snp_active;


Can't this be reduced to:

	return to_kvm_svm(kvm)->sev_info.snp_active;

KVM should never set snp_active without also setting es_active.

Side topic, I think it would also be worthwhile to add to_sev (or maybe to_kvm_sev)
given the frequency of the "&to_kvm_svm(kvm)->sev_info" pattern.

> +#else

> +	return false;

> +#endif

> +}

> +

>  static inline void vmcb_mark_all_dirty(struct vmcb *vmcb)

>  {

>  	vmcb->control.clean = 0;

> -- 

> 2.17.1

>
Brijesh Singh July 16, 2021, 6:46 p.m. UTC | #2
On 7/16/21 1:00 PM, Sean Christopherson wrote:
> On Wed, Jul 07, 2021, Brijesh Singh wrote:

>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c

>> index 411ed72f63af..abca2b9dee83 100644

>> --- a/arch/x86/kvm/svm/sev.c

>> +++ b/arch/x86/kvm/svm/sev.c

>> @@ -52,9 +52,14 @@ module_param_named(sev, sev_enabled, bool, 0444);

>>  /* enable/disable SEV-ES support */

>>  static bool sev_es_enabled = true;

>>  module_param_named(sev_es, sev_es_enabled, bool, 0444);

>> +

>> +/* enable/disable SEV-SNP support */

>> +static bool sev_snp_enabled = true;

> Is it safe to incrementally introduce SNP support?  Or should the module param

> be hidden until all support is in place?  E.g. what will happen when KVM allows

> userspace to create SNP guests but doesn't yet have the RMP management added?


The SNP support depends on the RMP management. At least the patch
ordering in this series adds the RMP management first then updates
drivers to use the RMP specific APIs. If RMP is not initialized due to
someone not picking the commits in the order, then SNP guest creation
will fail. This is mainly because the first thing a guest creation does
is to call the SNP_INIT. The SNP_INIT firmware command verifies that RMP
is initialized before creating the guest context etc..


>> +module_param_named(sev_snp, sev_snp_enabled, bool, 0444);

>>  #else

>>  #define sev_enabled false

>>  #define sev_es_enabled false

>> +#define sev_snp_enabled  false

>>  #endif /* CONFIG_KVM_AMD_SEV */

>>  

>>  #define AP_RESET_HOLD_NONE		0

>> @@ -1825,6 +1830,7 @@ void __init sev_hardware_setup(void)

>>  {

>>  #ifdef CONFIG_KVM_AMD_SEV

>>  	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;

>> +	bool sev_snp_supported = false;

>>  	bool sev_es_supported = false;

>>  	bool sev_supported = false;

>>  

>> @@ -1888,9 +1894,21 @@ void __init sev_hardware_setup(void)

>>  	pr_info("SEV-ES supported: %u ASIDs\n", sev_es_asid_count);

>>  	sev_es_supported = true;

>>  

>> +	/* SEV-SNP support requested? */

>> +	if (!sev_snp_enabled)

>> +		goto out;

>> +

>> +	/* Is SEV-SNP enabled? */

>> +	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))

> Random question, why use cpu_feature_enabled?  Did something change in cpufeatures

> that prevents using boot_cpu_has() here?



During the boot the kernel initialize the RMP table. If RMP table
initialization fail, then X86_FEATURE_SEV_SNP is cleared. In that case,
the cpu_feature_enabled() should return false. The idea is,
cpu_feature_enabled() will be set only when the RMP table is
successfully initialized and SYSCFG.SNP is set.


>> +		goto out;

>> +

>> +	pr_info("SEV-SNP supported: %u ASIDs\n", min_sev_asid - 1);

> Use sev_es_asid_count instead of manually recomputing the same; the latter

> obfuscates the fact that ES and SNP share the same ASID pool.

>

> Even better would be to report ES+SNP together, otherwise the user could easily

> interpret ES and SNP having separate ASID pools.  And IMO the gotos for SNP are

> overkill, e.g.

>

> 	sev_es_supported = true;

> 	sev_snp_supported = sev_snp_enabled &&

> 			    cpu_feature_enabled(X86_FEATURE_SEV_SNP);

>

> 	pr_info("SEV-ES %ssupported: %u ASIDs\n",

> 		sev_snp_supported ? "and SEV-SNP " : "", sev_es_asid_count);

>

Noted.


>> +	sev_snp_supported = true;

>> +

>>  out:

>>  	sev_enabled = sev_supported;

>>  	sev_es_enabled = sev_es_supported;

>> +	sev_snp_enabled = sev_snp_supported;

>>  #endif

>>  }

>>  

>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h

>> index 1175edb02d33..b9ea99f8579e 100644

>> --- a/arch/x86/kvm/svm/svm.h

>> +++ b/arch/x86/kvm/svm/svm.h

>> @@ -58,6 +58,7 @@ enum {

>>  struct kvm_sev_info {

>>  	bool active;		/* SEV enabled guest */

>>  	bool es_active;		/* SEV-ES enabled guest */

>> +	bool snp_active;	/* SEV-SNP enabled guest */

>>  	unsigned int asid;	/* ASID used for this guest */

>>  	unsigned int handle;	/* SEV firmware handle */

>>  	int fd;			/* SEV device fd */

>> @@ -232,6 +233,17 @@ static inline bool sev_es_guest(struct kvm *kvm)

>>  #endif

>>  }

>>  

>> +static inline bool sev_snp_guest(struct kvm *kvm)

>> +{

>> +#ifdef CONFIG_KVM_AMD_SEV

>> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

>> +

>> +	return sev_es_guest(kvm) && sev->snp_active;

> Can't this be reduced to:

>

> 	return to_kvm_svm(kvm)->sev_info.snp_active;

>

> KVM should never set snp_active without also setting es_active.



The approach here is similar to SEV/ES. IIRC, it was done mainly to
avoid adding dead code when CONFIG_KVM_AMD_SEV is disabled. Most of the
function related to SEV/ES/SNP call the
sev_guest()/sev_es_guest()/sev_snp_guest() on the entry. Instead of
#ifdef all those functions, we can #ifdef sev_snp_guest(); compiler will
see that if() statement will always return false, so it will not include
the remaining body of the function.


>

> Side topic, I think it would also be worthwhile to add to_sev (or maybe to_kvm_sev)

> given the frequency of the "&to_kvm_svm(kvm)->sev_info" pattern.

>

>> +#else

>> +	return false;

>> +#endif

>> +}

>> +

>>  static inline void vmcb_mark_all_dirty(struct vmcb *vmcb)

>>  {

>>  	vmcb->control.clean = 0;

>> -- 

>> 2.17.1

>>
Sean Christopherson July 16, 2021, 7:31 p.m. UTC | #3
On Fri, Jul 16, 2021, Brijesh Singh wrote:
> 

> On 7/16/21 1:00 PM, Sean Christopherson wrote:

> > On Wed, Jul 07, 2021, Brijesh Singh wrote:

> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c

> >> index 411ed72f63af..abca2b9dee83 100644

> >> --- a/arch/x86/kvm/svm/sev.c

> >> +++ b/arch/x86/kvm/svm/sev.c

> >> @@ -52,9 +52,14 @@ module_param_named(sev, sev_enabled, bool, 0444);

> >>  /* enable/disable SEV-ES support */

> >>  static bool sev_es_enabled = true;

> >>  module_param_named(sev_es, sev_es_enabled, bool, 0444);

> >> +

> >> +/* enable/disable SEV-SNP support */

> >> +static bool sev_snp_enabled = true;

> > Is it safe to incrementally introduce SNP support?  Or should the module param

> > be hidden until all support is in place?  E.g. what will happen when KVM allows

> > userspace to create SNP guests but doesn't yet have the RMP management added?

> 

> The SNP support depends on the RMP management. At least the patch

> ordering in this series adds the RMP management first then updates

> drivers to use the RMP specific APIs.


Yep, got that.

> If RMP is not initialized due to someone not picking the commits in the

> order, then SNP guest creation will fail.


That's not what I was asking.  My question is if KVM will break/fail if someone
runs a KVM build with SNP enabled halfway through the series.  E.g. if I make a
KVM build at patch 22, "KVM: SVM: Add KVM_SNP_INIT command", what will happen if
I attempt to launch an SNP guest?  Obviously it won't fully succeed, but will KVM
fail gracefully and do all the proper cleanup?  Repeat the question for all patches
between this one and the final patch of the series.

SNP simply not working is ok, but if KVM explodes or does weird things without
"full" SNP support, then at minimum the module param should be off by default
until it's safe to enable.  E.g. for the TDP MMU, I believe the approach was to
put all the machinery in place but not actually let userspace flip on the module
param until the full implementation was ready.  Bisecting and testing the
individual commits is a bit painful because it requires modifying KVM code, but
on the plus side unrelated bisects won't stumble into a half-baked state.

> >> +module_param_named(sev_snp, sev_snp_enabled, bool, 0444);

> >>  #else

> >>  #define sev_enabled false

> >>  #define sev_es_enabled false

> >> +#define sev_snp_enabled  false

> >>  #endif /* CONFIG_KVM_AMD_SEV */

> >>  

> >>  #define AP_RESET_HOLD_NONE		0

> >> @@ -1825,6 +1830,7 @@ void __init sev_hardware_setup(void)

> >>  {

> >>  #ifdef CONFIG_KVM_AMD_SEV

> >>  	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;

> >> +	bool sev_snp_supported = false;

> >>  	bool sev_es_supported = false;

> >>  	bool sev_supported = false;

> >>  

> >> @@ -1888,9 +1894,21 @@ void __init sev_hardware_setup(void)

> >>  	pr_info("SEV-ES supported: %u ASIDs\n", sev_es_asid_count);

> >>  	sev_es_supported = true;

> >>  

> >> +	/* SEV-SNP support requested? */

> >> +	if (!sev_snp_enabled)

> >> +		goto out;

> >> +

> >> +	/* Is SEV-SNP enabled? */

> >> +	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))

> > Random question, why use cpu_feature_enabled?  Did something change in cpufeatures

> > that prevents using boot_cpu_has() here?

> 

> 

> During the boot the kernel initialize the RMP table. If RMP table

> initialization fail, then X86_FEATURE_SEV_SNP is cleared. In that case,

> the cpu_feature_enabled() should return false. The idea is,

> cpu_feature_enabled() will be set only when the RMP table is

> successfully initialized and SYSCFG.SNP is set.


Ya, got that, but again not what I was asking :-)  Why use cpu_feature_enabled()
instead of boot_cpu_has()?  As a random developer, I would fully expect that
boot_cpu_has(X86_FEATURE_SEV_SNP) is true iff SNP is fully enabled by the kernel.

> >> +		goto out;

> >> +

> >> +	pr_info("SEV-SNP supported: %u ASIDs\n", min_sev_asid - 1);

> > Use sev_es_asid_count instead of manually recomputing the same; the latter

> > obfuscates the fact that ES and SNP share the same ASID pool.

> >

> > Even better would be to report ES+SNP together, otherwise the user could easily

> > interpret ES and SNP having separate ASID pools.  And IMO the gotos for SNP are

> > overkill, e.g.

> >

> > 	sev_es_supported = true;

> > 	sev_snp_supported = sev_snp_enabled &&

> > 			    cpu_feature_enabled(X86_FEATURE_SEV_SNP);

> >

> > 	pr_info("SEV-ES %ssupported: %u ASIDs\n",

> > 		sev_snp_supported ? "and SEV-SNP " : "", sev_es_asid_count);

> >

> >> +static inline bool sev_snp_guest(struct kvm *kvm)

> >> +{

> >> +#ifdef CONFIG_KVM_AMD_SEV

> >> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> >> +

> >> +	return sev_es_guest(kvm) && sev->snp_active;

> > Can't this be reduced to:

> >

> > 	return to_kvm_svm(kvm)->sev_info.snp_active;

> >

> > KVM should never set snp_active without also setting es_active.

> 

> 

> The approach here is similar to SEV/ES. IIRC, it was done mainly to

> avoid adding dead code when CONFIG_KVM_AMD_SEV is disabled.


But this is already in an #ifdef, checking sev_es_guest() is pointless.
Brijesh Singh July 16, 2021, 9:03 p.m. UTC | #4
On 7/16/21 2:31 PM, Sean Christopherson wrote:
> That's not what I was asking.  My question is if KVM will break/fail if someone

> runs a KVM build with SNP enabled halfway through the series.  E.g. if I make a

> KVM build at patch 22, "KVM: SVM: Add KVM_SNP_INIT command", what will happen if

> I attempt to launch an SNP guest?  Obviously it won't fully succeed, but will KVM

> fail gracefully and do all the proper cleanup?  Repeat the question for all patches

> between this one and the final patch of the series.

>

> SNP simply not working is ok, but if KVM explodes or does weird things without

> "full" SNP support, then at minimum the module param should be off by default

> until it's safe to enable.  E.g. for the TDP MMU, I believe the approach was to

> put all the machinery in place but not actually let userspace flip on the module

> param until the full implementation was ready.  Bisecting and testing the

> individual commits is a bit painful because it requires modifying KVM code, but

> on the plus side unrelated bisects won't stumble into a half-baked state.


There is one to two patches where I can think of that we may break the
KVM if SNP guest is created before applying the full series. In one
patch we add LAUNCH_UPDATE but reclaim is done in next patch. I like
your idea to push the module init  later in the series.


>

> Ya, got that, but again not what I was asking :-)  Why use cpu_feature_enabled()

> instead of boot_cpu_has()?  As a random developer, I would fully expect that

> boot_cpu_has(X86_FEATURE_SEV_SNP) is true iff SNP is fully enabled by the kernel.


I have to check but I think boot_cpu_has(X64_FEATURE_SEV_SNP) will
return true even when the CONFIG_MEM_ENCRYPT is disabled.


>

>> The approach here is similar to SEV/ES. IIRC, it was done mainly to

>> avoid adding dead code when CONFIG_KVM_AMD_SEV is disabled.

> But this is already in an #ifdef, checking sev_es_guest() is pointless.



Ah Good point.
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 411ed72f63af..abca2b9dee83 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -52,9 +52,14 @@  module_param_named(sev, sev_enabled, bool, 0444);
 /* enable/disable SEV-ES support */
 static bool sev_es_enabled = true;
 module_param_named(sev_es, sev_es_enabled, bool, 0444);
+
+/* enable/disable SEV-SNP support */
+static bool sev_snp_enabled = true;
+module_param_named(sev_snp, sev_snp_enabled, bool, 0444);
 #else
 #define sev_enabled false
 #define sev_es_enabled false
+#define sev_snp_enabled  false
 #endif /* CONFIG_KVM_AMD_SEV */
 
 #define AP_RESET_HOLD_NONE		0
@@ -1825,6 +1830,7 @@  void __init sev_hardware_setup(void)
 {
 #ifdef CONFIG_KVM_AMD_SEV
 	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
+	bool sev_snp_supported = false;
 	bool sev_es_supported = false;
 	bool sev_supported = false;
 
@@ -1888,9 +1894,21 @@  void __init sev_hardware_setup(void)
 	pr_info("SEV-ES supported: %u ASIDs\n", sev_es_asid_count);
 	sev_es_supported = true;
 
+	/* SEV-SNP support requested? */
+	if (!sev_snp_enabled)
+		goto out;
+
+	/* Is SEV-SNP enabled? */
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		goto out;
+
+	pr_info("SEV-SNP supported: %u ASIDs\n", min_sev_asid - 1);
+	sev_snp_supported = true;
+
 out:
 	sev_enabled = sev_supported;
 	sev_es_enabled = sev_es_supported;
+	sev_snp_enabled = sev_snp_supported;
 #endif
 }
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 1175edb02d33..b9ea99f8579e 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -58,6 +58,7 @@  enum {
 struct kvm_sev_info {
 	bool active;		/* SEV enabled guest */
 	bool es_active;		/* SEV-ES enabled guest */
+	bool snp_active;	/* SEV-SNP enabled guest */
 	unsigned int asid;	/* ASID used for this guest */
 	unsigned int handle;	/* SEV firmware handle */
 	int fd;			/* SEV device fd */
@@ -232,6 +233,17 @@  static inline bool sev_es_guest(struct kvm *kvm)
 #endif
 }
 
+static inline bool sev_snp_guest(struct kvm *kvm)
+{
+#ifdef CONFIG_KVM_AMD_SEV
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+
+	return sev_es_guest(kvm) && sev->snp_active;
+#else
+	return false;
+#endif
+}
+
 static inline void vmcb_mark_all_dirty(struct vmcb *vmcb)
 {
 	vmcb->control.clean = 0;