Skip to contentSkip to conversation

SEV fixes and performance enhancements

PATCH Applied Lendacky, Thomas 3 participants 8 messages Oct 3, 2019 – Oct 9, 2019

Subsystems (5)
x86 KVM Crypto API KVM x86 +1 more subsystems
lore
Share & tools
lore

Lendacky, Thomas

author maintainer
#0 lore raw

cover SEV fixes and performance enhancements

From: Tom Lendacky <[email protected]>

This patch series provides fixes in the area of ASID assignment and VM deactivation.

Additionally, it provides some performance enhancements by reducing the number of WBINVD/DF_FLUSH invocations that are made.

Note: The third patch in the series modifies a file that is outside of
the arch/x86/kvm directory.

---

Patches based on https://git.kernel.org/pub/scm/virt/kvm/kvm.git next and commit:
fd3edd4a9066 ("KVM: nVMX: cleanup and fix host 64-bit mode checks")

Tom Lendacky (5):
KVM: SVM: Serialize access to the SEV ASID bitmap
KVM: SVM: Guard against DEACTIVATE when performing WBINVD/DF_FLUSH
KVM: SVM: Remove unneeded WBINVD and DF_FLUSH when starting SEV guests
KVM: SVM: Convert DEACTIVATE mutex to read/write semaphore
KVM: SVM: Reduce WBINVD/DF_FLUSH invocations

 arch/x86/kvm/svm.c           | 105 +++++++++++++++++++++++++++--------
 drivers/crypto/ccp/psp-dev.c |   9 +++
 2 files changed, 92 insertions(+), 22 deletions(-)
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH 1/5 KVM: SVM: Serialize access to the SEV ASID bitmap

From: Tom Lendacky <[email protected]>

The SEV ASID bitmap currently is not protected against parallel SEV guest startups. This can result in an SEV guest failing to start because another SEV guest could have been assigned the same ASID value. Use a mutex to serialize access to the SEV ASID bitmap.

Fixes 1654efcbc431 ("KVM: SVM: Add KVM_SEV_INIT command")
Tested-by David Rientjes <[email protected]>
Signed-off-by Tom Lendacky <[email protected]>
---
 arch/x86/kvm/svm.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)
arch/x86/kvm/svm.c +17 -12
index f8ecb6df5106..d371007ab109 100644
@@ -418,6 +418,7 @@ enum {
 
 #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
 
+static DEFINE_MUTEX(sev_bitmap_lock);
 static unsigned int max_sev_asid;
 static unsigned int min_sev_asid;
 static unsigned long *sev_asid_bitmap;

@@ -1723,25 +1724,22 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static void __sev_asid_free(int asid)
+static void sev_asid_free(int asid)
 {
 	struct svm_cpu_data *sd;
 	int cpu, pos;
 
+	mutex_lock(&sev_bitmap_lock);
+
 	pos = asid - 1;
-	clear_bit(pos, sev_asid_bitmap);
+	__clear_bit(pos, sev_asid_bitmap);
 
 	for_each_possible_cpu(cpu) {
 		sd = per_cpu(svm_data, cpu);
 		sd->sev_vmcbs[pos] = NULL;
 	}
-}
-
-static void sev_asid_free(struct kvm *kvm)
-{
-	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
 
-	__sev_asid_free(sev->asid);
+	mutex_unlock(&sev_bitmap_lock);
 }
 
 static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)

@@ -1910,7 +1908,7 @@ static void sev_vm_destroy(struct kvm *kvm)
 	mutex_unlock(&kvm->lock);
 
 	sev_unbind_asid(kvm, sev->handle);
-	sev_asid_free(kvm);
+	sev_asid_free(sev->asid);
 }
 
 static void avic_vm_destroy(struct kvm *kvm)

@@ -6268,14 +6266,21 @@ static int sev_asid_new(void)
 {
 	int pos;
 
+	mutex_lock(&sev_bitmap_lock);
+
 	/*
 	 * SEV-enabled guest must use asid from min_sev_asid to max_sev_asid.
 	 */
 	pos = find_next_zero_bit(sev_asid_bitmap, max_sev_asid, min_sev_asid - 1);
-	if (pos >= max_sev_asid)
+	if (pos >= max_sev_asid) {
+		mutex_unlock(&sev_bitmap_lock);
 		return -EBUSY;
+	}
+
+	__set_bit(pos, sev_asid_bitmap);
+
+	mutex_unlock(&sev_bitmap_lock);
 
-	set_bit(pos, sev_asid_bitmap);
 	return pos + 1;
 }
 

@@ -6303,7 +6308,7 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return 0;
 
 e_free:
-	__sev_asid_free(asid);
+	sev_asid_free(asid);
 	return ret;
 }
Signature · 1 line
2.17.1
Reviewed-by on v1 1/5
#2 lore raw

Re: [PATCH 1/5] KVM: SVM: Serialize access to the SEV ASID bitmap

Quoted 11 lines · Thomas

From: Tom Lendacky <[email protected]>

The SEV ASID bitmap currently is not protected against parallel SEV guest startups. This can result in an SEV guest failing to start because another SEV guest could have been assigned the same ASID value. Use a mutex to serialize access to the SEV ASID bitmap.

Fixes 1654efcbc431 ("KVM: SVM: Add KVM_SEV_INIT command")
Tested-by David Rientjes <[email protected]>
Signed-off-by Tom Lendacky <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
author maintainer
#3 lore raw

PATCH 2/5 KVM: SVM: Guard against DEACTIVATE when performing WBINVD/DF_FLUSH

In reply to Lendacky, Thomas

From: Tom Lendacky <[email protected]>

The SEV firmware DEACTIVATE command disassociates an SEV guest from an ASID, clears the WBINVD indicator on all threads and indicates that the SEV firmware DF_FLUSH command must be issued before the ASID can be re-used. The SEV firmware DF_FLUSH command will return an error if a WBINVD has not been performed on every thread before it has been invoked. A window exists between the WBINVD and the invocation of the DF_FLUSH command where an SEV firmware DEACTIVATE command could be invoked on another thread, clearing the WBINVD indicator. This will cause the subsequent SEV firmware DF_FLUSH command to fail which, in turn, results in the SEV firmware ACTIVATE command failing for the reclaimed ASID. This results in the SEV guest failing to start.

Use a mutex to close the WBINVD/DF_FLUSH window by obtaining the mutex before the DEACTIVATE and releasing it after the DF_FLUSH. This ensures that any DEACTIVATE cannot run before a DF_FLUSH has completed.

Fixes 59414c989220 ("KVM: SVM: Add support for KVM_SEV_LAUNCH_START command")
Tested-by David Rientjes <[email protected]>
Signed-off-by Tom Lendacky <[email protected]>
---
 arch/x86/kvm/svm.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
arch/x86/kvm/svm.c +20 -0
index d371007ab109..1d217680cf83 100644
@@ -418,6 +418,7 @@ enum {
 
 #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
 
+static DEFINE_MUTEX(sev_deactivate_lock);
 static DEFINE_MUTEX(sev_bitmap_lock);
 static unsigned int max_sev_asid;
 static unsigned int min_sev_asid;

@@ -1756,10 +1757,20 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
 
 	/* deactivate handle */
 	data->handle = handle;
+
+	/*
+	 * Guard against a parallel DEACTIVATE command before the DF_FLUSH
+	 * command has completed.
+	 */
+	mutex_lock(&sev_deactivate_lock);
+
 	sev_guest_deactivate(data, NULL);
 
 	wbinvd_on_all_cpus();
 	sev_guest_df_flush(NULL);
+
+	mutex_unlock(&sev_deactivate_lock);
+
 	kfree(data);
 
 	decommission = kzalloc(sizeof(*decommission), GFP_KERNEL);

@@ -6318,9 +6329,18 @@ static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
 	int asid = sev_get_asid(kvm);
 	int ret;
 
+	/*
+	 * Guard against a DEACTIVATE command before the DF_FLUSH command
+	 * has completed.
+	 */
+	mutex_lock(&sev_deactivate_lock);
+
 	wbinvd_on_all_cpus();
 
 	ret = sev_guest_df_flush(error);
+
+	mutex_unlock(&sev_deactivate_lock);
+
 	if (ret)
 		return ret;
Signature · 1 line
2.17.1
author maintainer
#4 lore raw

PATCH 3/5 KVM: SVM: Remove unneeded WBINVD and DF_FLUSH when starting SEV guests

In reply to Lendacky, Thomas

From: Tom Lendacky <[email protected]>

Performing a WBINVD and DF_FLUSH are expensive operations. The SEV support currently performs this WBINVD/DF_FLUSH combination when an SEV guest is terminated, so there is no need for it to be done before LAUNCH.

However, when the SEV firmware transitions the platform from UNINIT state to INIT state, all ASIDs will be marked invalid across all threads. Therefore, as part of transitioning the platform to INIT state, perform a WBINVD/DF_FLUSH after a successful INIT in the PSP/SEV device driver. Since the PSP/SEV device driver is x86 only, it can reference and use the WBINVD related functions directly.

Cc Gary Hook <[email protected]>
Cc Herbert Xu <[email protected]>
Cc "David S. Miller" <[email protected]>
Tested-by David Rientjes <[email protected]>
Signed-off-by Tom Lendacky <[email protected]>
---
 arch/x86/kvm/svm.c           | 15 ---------------
 drivers/crypto/ccp/psp-dev.c |  9 +++++++++
 2 files changed, 9 insertions(+), 15 deletions(-)
arch/x86/kvm/svm.c +0 -15
index 1d217680cf83..389dfd7594eb 100644
@@ -6329,21 +6329,6 @@ static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
 	int asid = sev_get_asid(kvm);
 	int ret;
 
-	/*
-	 * Guard against a DEACTIVATE command before the DF_FLUSH command
-	 * has completed.
-	 */
-	mutex_lock(&sev_deactivate_lock);
-
-	wbinvd_on_all_cpus();
-
-	ret = sev_guest_df_flush(error);
-
-	mutex_unlock(&sev_deactivate_lock);
-
-	if (ret)
-		return ret;
-
 	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
 	if (!data)
 		return -ENOMEM;
drivers/crypto/ccp/psp-dev.c +9 -0
index 6b17d179ef8a..39fdd0641637 100644
@@ -21,6 +21,8 @@
 #include <linux/ccp.h>
 #include <linux/firmware.h>
 
+#include <asm/smp.h>
+
 #include "sp-dev.h"
 #include "psp-dev.h"
 

@@ -235,6 +237,13 @@ static int __sev_platform_init_locked(int *error)
 		return rc;
 
 	psp->sev_state = SEV_STATE_INIT;
+
+	/* Prepare for first SEV guest launch after INIT */
+	wbinvd_on_all_cpus();
+	rc = __sev_do_cmd_locked(SEV_CMD_DF_FLUSH, NULL, error);
+	if (rc)
+		return rc;
+
 	dev_dbg(psp->dev, "SEV firmware initialized\n");
 
 	return rc;
Signature · 1 line
2.17.1
author maintainer
#5 lore raw

PATCH 4/5 KVM: SVM: Convert DEACTIVATE mutex to read/write semaphore

In reply to Lendacky, Thomas

From: Tom Lendacky <[email protected]>

In preparation for an upcoming patch, convert the mutex that guards the DEACTIVATE/WBINVD/DF_FLUSH sequence into a read/write semaphore. The conversion will convert the mutex lock and unlock into down_write and up_write so that the mutex behavior is maintained.

Tested-by David Rientjes <[email protected]>
Signed-off-by Tom Lendacky <[email protected]>
---
 arch/x86/kvm/svm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
arch/x86/kvm/svm.c +4 -3
index 389dfd7594eb..b995d7ac1516 100644
@@ -38,6 +38,7 @@
 #include <linux/file.h>
 #include <linux/pagemap.h>
 #include <linux/swap.h>
+#include <linux/rwsem.h>
 
 #include <asm/apic.h>
 #include <asm/perf_event.h>

@@ -418,7 +419,7 @@ enum {
 
 #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
 
-static DEFINE_MUTEX(sev_deactivate_lock);
+static DECLARE_RWSEM(sev_deactivate_lock);
 static DEFINE_MUTEX(sev_bitmap_lock);
 static unsigned int max_sev_asid;
 static unsigned int min_sev_asid;

@@ -1762,14 +1763,14 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
 	 * Guard against a parallel DEACTIVATE command before the DF_FLUSH
 	 * command has completed.
 	 */
-	mutex_lock(&sev_deactivate_lock);
+	down_write(&sev_deactivate_lock);
 
 	sev_guest_deactivate(data, NULL);
 
 	wbinvd_on_all_cpus();
 	sev_guest_df_flush(NULL);
 
-	mutex_unlock(&sev_deactivate_lock);
+	up_write(&sev_deactivate_lock);
 
 	kfree(data);
Signature · 1 line
2.17.1
author maintainer
#6 lore raw

PATCH 5/5 KVM: SVM: Reduce WBINVD/DF_FLUSH invocations

In reply to Lendacky, Thomas

From: Tom Lendacky <[email protected]>

Performing a WBINVD and DF_FLUSH are expensive operations. Currently, a WBINVD/DF_FLUSH is performed every time an SEV guest terminates. However, the WBINVD/DF_FLUSH is only required when an ASID is being re-allocated to a new SEV guest. Also, a single WBINVD/DF_FLUSH can enable all ASIDs that have been disassociated from guests through DEACTIVATE.

To reduce the number of WBINVD/DF_FLUSH invocations, introduce a new ASID bitmap to track ASIDs that need to be reclaimed. When an SEV guest is terminated, add its ASID to the reclaim bitmap instead of clearing the bitmap in the existing SEV ASID bitmap. This delays the need to perform a WBINVD/DF_FLUSH invocation when an SEV guest terminates until all of the available SEV ASIDs have been used. At that point, the WBINVD/DF_FLUSH invocation can be performed and all ASIDs in the reclaim bitmap moved to the available ASIDs bitmap.

The semaphore around DEACTIVATE can be changed to a read semaphore with the semaphore taken in write mode before performing the WBINVD/DF_FLUSH.

Tested-by David Rientjes <[email protected]>
Signed-off-by Tom Lendacky <[email protected]>
---
 arch/x86/kvm/svm.c | 78 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 14 deletions(-)
arch/x86/kvm/svm.c +64 -14
index b995d7ac1516..62b0938b62ef 100644
@@ -419,11 +419,13 @@ enum {
 
 #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
 
+static int sev_flush_asids(void);
 static DECLARE_RWSEM(sev_deactivate_lock);
 static DEFINE_MUTEX(sev_bitmap_lock);
 static unsigned int max_sev_asid;
 static unsigned int min_sev_asid;
 static unsigned long *sev_asid_bitmap;
+static unsigned long *sev_reclaim_asid_bitmap;
 #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
 
 struct enc_region {

@@ -1232,11 +1234,15 @@ static __init int sev_hardware_setup(void)
 	/* Minimum ASID value that should be used for SEV guest */
 	min_sev_asid = cpuid_edx(0x8000001F);
 
-	/* Initialize SEV ASID bitmap */
+	/* Initialize SEV ASID bitmaps */
 	sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
 	if (!sev_asid_bitmap)
 		return 1;
 
+	sev_reclaim_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
+	if (!sev_reclaim_asid_bitmap)
+		return 1;
+
 	status = kmalloc(sizeof(*status), GFP_KERNEL);
 	if (!status)
 		return 1;

@@ -1415,8 +1421,12 @@ static __exit void svm_hardware_unsetup(void)
 {
 	int cpu;
 
-	if (svm_sev_enabled())
+	if (svm_sev_enabled()) {
 		bitmap_free(sev_asid_bitmap);
+		bitmap_free(sev_reclaim_asid_bitmap);
+
+		sev_flush_asids();
+	}
 
 	for_each_possible_cpu(cpu)
 		svm_cpu_uninit(cpu);

@@ -1734,7 +1744,7 @@ static void sev_asid_free(int asid)
 	mutex_lock(&sev_bitmap_lock);
 
 	pos = asid - 1;
-	__clear_bit(pos, sev_asid_bitmap);
+	__set_bit(pos, sev_reclaim_asid_bitmap);
 
 	for_each_possible_cpu(cpu) {
 		sd = per_cpu(svm_data, cpu);

@@ -1759,18 +1769,10 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
 	/* deactivate handle */
 	data->handle = handle;
 
-	/*
-	 * Guard against a parallel DEACTIVATE command before the DF_FLUSH
-	 * command has completed.
-	 */
-	down_write(&sev_deactivate_lock);
-
+	/* Guard DEACTIVATE against WBINVD/DF_FLUSH used in ASID recycling */
+	down_read(&sev_deactivate_lock);
 	sev_guest_deactivate(data, NULL);
-
-	wbinvd_on_all_cpus();
-	sev_guest_df_flush(NULL);
-
-	up_write(&sev_deactivate_lock);
+	up_read(&sev_deactivate_lock);
 
 	kfree(data);
 

@@ -6274,8 +6276,51 @@ static int enable_smi_window(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static int sev_flush_asids(void)
+{
+	int ret, error;
+
+	/*
+	 * DEACTIVATE will clear the WBINVD indicator causing DF_FLUSH to fail,
+	 * so it must be guarded.
+	 */
+	down_write(&sev_deactivate_lock);
+
+	wbinvd_on_all_cpus();
+	ret = sev_guest_df_flush(&error);
+
+	up_write(&sev_deactivate_lock);
+
+	if (ret)
+		pr_err("SEV: DF_FLUSH failed, ret=%d, error=%#x\n", ret, error);
+
+	return ret;
+}
+
+/* Must be called with the sev_bitmap_lock held */
+static bool __sev_recycle_asids(void)
+{
+	int pos;
+
+	/* Check if there are any ASIDs to reclaim before performing a flush */
+	pos = find_next_bit(sev_reclaim_asid_bitmap,
+			    max_sev_asid, min_sev_asid - 1);
+	if (pos >= max_sev_asid)
+		return false;
+
+	if (sev_flush_asids())
+		return false;
+
+	bitmap_xor(sev_asid_bitmap, sev_asid_bitmap, sev_reclaim_asid_bitmap,
+		   max_sev_asid);
+	bitmap_zero(sev_reclaim_asid_bitmap, max_sev_asid);
+
+	return true;
+}
+
 static int sev_asid_new(void)
 {
+	bool retry = true;
 	int pos;
 
 	mutex_lock(&sev_bitmap_lock);

@@ -6283,8 +6328,13 @@ static int sev_asid_new(void)
 	/*
 	 * SEV-enabled guest must use asid from min_sev_asid to max_sev_asid.
 	 */
+again:
 	pos = find_next_zero_bit(sev_asid_bitmap, max_sev_asid, min_sev_asid - 1);
 	if (pos >= max_sev_asid) {
+		if (retry && __sev_recycle_asids()) {
+			retry = false;
+			goto again;
+		}
 		mutex_unlock(&sev_bitmap_lock);
 		return -EBUSY;
 	}
Signature · 1 line
2.17.1
maintainer Applied
#7 lore raw
In reply to Lendacky, Thomas
Quoted 24 lines · Thomas

From: Tom Lendacky <[email protected]>

This patch series provides fixes in the area of ASID assignment and VM deactivation.

Additionally, it provides some performance enhancements by reducing the number of WBINVD/DF_FLUSH invocations that are made.

Note: The third patch in the series modifies a file that is outside of
the arch/x86/kvm directory.

---

Patches based on https://git.kernel.org/pub/scm/virt/kvm/kvm.git next and commit:
fd3edd4a9066 ("KVM: nVMX: cleanup and fix host 64-bit mode checks")

Tom Lendacky (5):
KVM: SVM: Serialize access to the SEV ASID bitmap
KVM: SVM: Guard against DEACTIVATE when performing WBINVD/DF_FLUSH
KVM: SVM: Remove unneeded WBINVD and DF_FLUSH when starting SEV guests
KVM: SVM: Convert DEACTIVATE mutex to read/write semaphore
KVM: SVM: Reduce WBINVD/DF_FLUSH invocations

Quoted 4 lines
 arch/x86/kvm/svm.c           | 105 +++++++++++++++++++++++++++--------
 drivers/crypto/ccp/psp-dev.c |   9 +++
 2 files changed, 92 insertions(+), 22 deletions(-)

Queued, thanks. I squashed 4/5 and 5/5 since there's not much that survives of patch 4 after the next one.

Paolo