[PATCH v5 01/10] KVM: SVM: Fix gctx page leak on invalid inputs

[PATCH v5 01/10] KVM: SVM: Fix gctx page leak on invalid inputs

[Dionna Glaze]

Ensure that snp gctx page allocation is adequately deallocated on
failure during snp_launch_start.

Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>
CC: stable@vger.kernel.org

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
Acked-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c6c8524859001..357906375ec59 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (sev->snp_context)
 		return -EINVAL;
 
-	sev->snp_context = snp_context_create(kvm, argp);
-	if (!sev->snp_context)
-		return -ENOTTY;
-
 	if (params.flags)
 		return -EINVAL;
 
@@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
 		return -EINVAL;
 
+	sev->snp_context = snp_context_create(kvm, argp);
+	if (!sev->snp_context)
+		return -ENOTTY;
+
 	start.gctx_paddr = __psp_pa(sev->snp_context);
 	start.policy = params.policy;
 	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
-- 
2.47.0.277.g8800431eea-goog

[PATCH v5 02/10] KVM: SVM: Fix snp_context_create error reporting

[Dionna Glaze]

Failure to allocate should not return -ENOTTY.
Command failure has multiple possible error modes.

Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>
CC: stable@vger.kernel.org

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 arch/x86/kvm/svm/sev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 357906375ec59..d0e0152aefb32 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2171,7 +2171,7 @@ static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	/* Allocate memory for context page */
 	context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
 	if (!context)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	data.address = __psp_pa(context);
 	rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error);
@@ -2179,7 +2179,7 @@ static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		pr_warn("Failed to create SEV-SNP context, rc %d fw_error %d",
 			rc, argp->error);
 		snp_free_firmware_page(context);
-		return NULL;
+		return ERR_PTR(rc);
 	}
 
 	return context;
@@ -2227,8 +2227,8 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		return -EINVAL;
 
 	sev->snp_context = snp_context_create(kvm, argp);
-	if (!sev->snp_context)
-		return -ENOTTY;
+	if (IS_ERR(sev->snp_context))
+		return PTR_ERR(sev->snp_context);
 
 	start.gctx_paddr = __psp_pa(sev->snp_context);
 	start.policy = params.policy;
-- 
2.47.0.277.g8800431eea-goog

[PATCH v5 08/10] KVM: SVM: move sev_issue_cmd_external_user to new API

[Dionna Glaze]

ccp now prefers all calls from external drivers to dominate all calls
into the driver on behalf of a user with a successful
sev_check_external_user call.

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 arch/x86/kvm/svm/sev.c       | 18 +++++++++++++++---
 drivers/crypto/ccp/sev-dev.c | 12 ------------
 include/linux/psp-sev.h      | 27 ---------------------------
 3 files changed, 15 insertions(+), 42 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index d0e0152aefb32..cea41b8cdabe4 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -528,21 +528,33 @@ static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
 	return ret;
 }
 
-static int __sev_issue_cmd(int fd, int id, void *data, int *error)
+static int sev_check_external_user(int fd)
 {
 	struct fd f;
-	int ret;
+	int ret = 0;
 
 	f = fdget(fd);
 	if (!fd_file(f))
 		return -EBADF;
 
-	ret = sev_issue_cmd_external_user(fd_file(f), id, data, error);
+	if (!file_is_sev(fd_file(f)))
+		ret = -EBADF;
 
 	fdput(f);
 	return ret;
 }
 
+static int __sev_issue_cmd(int fd, int id, void *data, int *error)
+{
+	int ret;
+
+	ret = sev_check_external_user(fd);
+	if (ret)
+		return ret;
+
+	return sev_do_cmd(id, data, error);
+}
+
 static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index f92e6a222da8a..67f6425b7ed07 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2493,18 +2493,6 @@ bool file_is_sev(struct file *p)
 }
 EXPORT_SYMBOL_GPL(file_is_sev);
 
-int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
-				void *data, int *error)
-{
-	int rc = file_is_sev(filep) ? 0 : -EBADF;
-
-	if (rc)
-		return rc;
-
-	return sev_do_cmd(cmd, data, error);
-}
-EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
-
 void sev_pci_init(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index ed85c0cfcfcbe..b4164d3600702 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -860,30 +860,6 @@ int sev_platform_init(struct sev_platform_init_args *args);
  */
 int sev_platform_status(struct sev_user_data_status *status, int *error);
 
-/**
- * sev_issue_cmd_external_user - issue SEV command by other driver with a file
- * handle.
- *
- * This function can be used by other drivers to issue a SEV command on
- * behalf of userspace. The caller must pass a valid SEV file descriptor
- * so that we know that it has access to SEV device.
- *
- * @filep - SEV device file pointer
- * @cmd - command to issue
- * @data - command buffer
- * @error: SEV command return code
- *
- * Returns:
- * 0 if the SEV successfully processed the command
- * -%ENODEV    if the SEV device is not available
- * -%ENOTSUPP  if the SEV does not support SEV
- * -%ETIMEDOUT if the SEV command timed out
- * -%EIO       if the SEV returned a non-zero return code
- * -%EBADF     if the file pointer is bad or does not grant access
- */
-int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
-				void *data, int *error);
-
 /**
  * file_is_sev - returns whether a file pointer is for the SEV device
  *
@@ -1043,9 +1019,6 @@ sev_guest_activate(struct sev_data_activate *data, int *error) { return -ENODEV;
 
 static inline int sev_guest_df_flush(int *error) { return -ENODEV; }
 
-static inline int
-sev_issue_cmd_external_user(struct file *filep, unsigned int id, void *data, int *error) { return -ENODEV; }
-
 static inline bool file_is_sev(struct file *filep) { return false; }
 
 static inline void *psp_copy_user_blob(u64 __user uaddr, u32 len) { return ERR_PTR(-EINVAL); }
-- 
2.47.0.277.g8800431eea-goog

[PATCH v5 09/10] KVM: SVM: Use new ccp GCTX API

[Dionna Glaze]

Guest context pages should be near 1-to-1 with allocated ASIDs. With the
GCTX API, the ccp driver is better able to associate guest context pages
with the ASID that is/will be bound to it.

This is important to the firmware hotloading implementation to not
corrupt any running VM's guest context page before userspace commits a
new firmware.

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 arch/x86/kvm/svm/sev.c | 74 ++++++++++++------------------------------
 1 file changed, 20 insertions(+), 54 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index cea41b8cdabe4..d7cef84750b33 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -89,7 +89,7 @@ static unsigned int nr_asids;
 static unsigned long *sev_asid_bitmap;
 static unsigned long *sev_reclaim_asid_bitmap;
 
-static int snp_decommission_context(struct kvm *kvm);
+static int kvm_decommission_snp_context(struct kvm *kvm);
 
 struct enc_region {
 	struct list_head list;
@@ -2168,51 +2168,12 @@ int sev_dev_get_attr(u32 group, u64 attr, u64 *val)
 	}
 }
 
-/*
- * The guest context contains all the information, keys and metadata
- * associated with the guest that the firmware tracks to implement SEV
- * and SNP features. The firmware stores the guest context in hypervisor
- * provide page via the SNP_GCTX_CREATE command.
- */
-static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp)
-{
-	struct sev_data_snp_addr data = {};
-	void *context;
-	int rc;
-
-	/* Allocate memory for context page */
-	context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
-	if (!context)
-		return ERR_PTR(-ENOMEM);
-
-	data.address = __psp_pa(context);
-	rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error);
-	if (rc) {
-		pr_warn("Failed to create SEV-SNP context, rc %d fw_error %d",
-			rc, argp->error);
-		snp_free_firmware_page(context);
-		return ERR_PTR(rc);
-	}
-
-	return context;
-}
-
-static int snp_bind_asid(struct kvm *kvm, int *error)
-{
-	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_snp_activate data = {0};
-
-	data.gctx_paddr = __psp_pa(sev->snp_context);
-	data.asid = sev_get_asid(kvm);
-	return sev_issue_cmd(kvm, SEV_CMD_SNP_ACTIVATE, &data, error);
-}
-
 static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
 	struct sev_data_snp_launch_start start = {0};
 	struct kvm_sev_snp_launch_start params;
-	int rc;
+	int rc, asid;
 
 	if (!sev_snp_guest(kvm))
 		return -ENOTTY;
@@ -2238,14 +2199,19 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
 		return -EINVAL;
 
-	sev->snp_context = snp_context_create(kvm, argp);
+	rc = sev_check_external_user(argp->sev_fd);
+	if (rc)
+		return rc;
+
+	asid = sev_get_asid(kvm);
+	sev->snp_context = sev_snp_create_context(asid, &argp->error);
 	if (IS_ERR(sev->snp_context))
 		return PTR_ERR(sev->snp_context);
 
 	start.gctx_paddr = __psp_pa(sev->snp_context);
 	start.policy = params.policy;
 	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
-	rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_LAUNCH_START, &start, &argp->error);
+	rc = sev_do_cmd(SEV_CMD_SNP_LAUNCH_START, &start, &argp->error);
 	if (rc) {
 		pr_debug("%s: SEV_CMD_SNP_LAUNCH_START firmware command failed, rc %d\n",
 			 __func__, rc);
@@ -2253,7 +2219,7 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	}
 
 	sev->fd = argp->sev_fd;
-	rc = snp_bind_asid(kvm, &argp->error);
+	rc = sev_snp_activate_asid(asid, &argp->error);
 	if (rc) {
 		pr_debug("%s: Failed to bind ASID to SEV-SNP context, rc %d\n",
 			 __func__, rc);
@@ -2263,7 +2229,7 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return 0;
 
 e_free_context:
-	snp_decommission_context(kvm);
+	kvm_decommission_snp_context(kvm);
 
 	return rc;
 }
@@ -2874,26 +2840,26 @@ int sev_vm_copy_enc_context_from(struct kvm *kvm, unsigned int source_fd)
 	return ret;
 }
 
-static int snp_decommission_context(struct kvm *kvm)
+static int kvm_decommission_snp_context(struct kvm *kvm)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_snp_addr data = {};
-	int ret;
+	int ret, error;
 
 	/* If context is not created then do nothing */
 	if (!sev->snp_context)
 		return 0;
 
-	/* Do the decommision, which will unbind the ASID from the SNP context */
-	data.address = __sme_pa(sev->snp_context);
+	/*
+	 * Do the decommision, which will unbind the ASID from the SNP context
+	 * and free the context page.
+	 */
 	down_write(&sev_deactivate_lock);
-	ret = sev_do_cmd(SEV_CMD_SNP_DECOMMISSION, &data, NULL);
+	ret = sev_snp_guest_decommission(sev->asid, &error);
 	up_write(&sev_deactivate_lock);
 
-	if (WARN_ONCE(ret, "Failed to release guest context, ret %d", ret))
+	if (WARN_ONCE(ret, "Failed to release guest context, ret %d fw err %d", ret, error))
 		return ret;
 
-	snp_free_firmware_page(sev->snp_context);
 	sev->snp_context = NULL;
 
 	return 0;
@@ -2947,7 +2913,7 @@ void sev_vm_destroy(struct kvm *kvm)
 		 * Decomission handles unbinding of the ASID. If it fails for
 		 * some unexpected reason, just leak the ASID.
 		 */
-		if (snp_decommission_context(kvm))
+		if (kvm_decommission_snp_context(kvm))
 			return;
 	} else {
 		sev_unbind_asid(kvm, sev->handle);
-- 
2.47.0.277.g8800431eea-goog

[PATCH v5 10/10] KVM: SVM: Delay legacy platform initialization on SNP

[Dionna Glaze]

When no SEV or SEV-ES guests are active, then the firmware can be
updated while (SEV-SNP) VM guests are active.

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>

Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Reviewed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 arch/x86/kvm/svm/sev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index d7cef84750b33..0d57a0a6b30fc 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -444,7 +444,11 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (ret)
 		goto e_no_asid;
 
-	init_args.probe = false;
+	/*
+	 * Probe will skip SEV/SEV-ES platform initialization in order for
+	 * SNP firmware hotloading to be available when SEV-SNP VMs are running.
+	 */
+	init_args.probe = vm_type != KVM_X86_SEV_VM && vm_type != KVM_X86_SEV_ES_VM;
 	ret = sev_platform_init(&init_args);
 	if (ret)
 		goto e_free;
-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH v5 08/10] KVM: SVM: move sev_issue_cmd_external_user to new API

[Tom Lendacky]

On 11/7/24 17:24, Dionna Glaze wrote:
> ccp now prefers all calls from external drivers to dominate all calls
> into the driver on behalf of a user with a successful
> sev_check_external_user call.

Would it be simpler to have the new APIs take an fd for an argument,
instead of doing this rework?

Thanks,
Tom

> 
> CC: Sean Christopherson <seanjc@google.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: Ashish Kalra <ashish.kalra@amd.com>
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: John Allen <john.allen@amd.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Michael Roth <michael.roth@amd.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> CC: Danilo Krummrich <dakr@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@kernel.org>
> CC: Tianfei zhang <tianfei.zhang@intel.com>
> CC: Alexey Kardashevskiy <aik@amd.com>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  arch/x86/kvm/svm/sev.c       | 18 +++++++++++++++---
>  drivers/crypto/ccp/sev-dev.c | 12 ------------
>  include/linux/psp-sev.h      | 27 ---------------------------
>  3 files changed, 15 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index d0e0152aefb32..cea41b8cdabe4 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -528,21 +528,33 @@ static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
>  	return ret;
>  }
>  
> -static int __sev_issue_cmd(int fd, int id, void *data, int *error)
> +static int sev_check_external_user(int fd)
>  {
>  	struct fd f;
> -	int ret;
> +	int ret = 0;
>  
>  	f = fdget(fd);
>  	if (!fd_file(f))
>  		return -EBADF;
>  
> -	ret = sev_issue_cmd_external_user(fd_file(f), id, data, error);
> +	if (!file_is_sev(fd_file(f)))
> +		ret = -EBADF;
>  
>  	fdput(f);
>  	return ret;
>  }
>  
> +static int __sev_issue_cmd(int fd, int id, void *data, int *error)
> +{
> +	int ret;
> +
> +	ret = sev_check_external_user(fd);
> +	if (ret)
> +		return ret;
> +
> +	return sev_do_cmd(id, data, error);
> +}
> +
>  static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
>  {
>  	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index f92e6a222da8a..67f6425b7ed07 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -2493,18 +2493,6 @@ bool file_is_sev(struct file *p)
>  }
>  EXPORT_SYMBOL_GPL(file_is_sev);
>  
> -int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
> -				void *data, int *error)
> -{
> -	int rc = file_is_sev(filep) ? 0 : -EBADF;
> -
> -	if (rc)
> -		return rc;
> -
> -	return sev_do_cmd(cmd, data, error);
> -}
> -EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
> -
>  void sev_pci_init(void)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index ed85c0cfcfcbe..b4164d3600702 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -860,30 +860,6 @@ int sev_platform_init(struct sev_platform_init_args *args);
>   */
>  int sev_platform_status(struct sev_user_data_status *status, int *error);
>  
> -/**
> - * sev_issue_cmd_external_user - issue SEV command by other driver with a file
> - * handle.
> - *
> - * This function can be used by other drivers to issue a SEV command on
> - * behalf of userspace. The caller must pass a valid SEV file descriptor
> - * so that we know that it has access to SEV device.
> - *
> - * @filep - SEV device file pointer
> - * @cmd - command to issue
> - * @data - command buffer
> - * @error: SEV command return code
> - *
> - * Returns:
> - * 0 if the SEV successfully processed the command
> - * -%ENODEV    if the SEV device is not available
> - * -%ENOTSUPP  if the SEV does not support SEV
> - * -%ETIMEDOUT if the SEV command timed out
> - * -%EIO       if the SEV returned a non-zero return code
> - * -%EBADF     if the file pointer is bad or does not grant access
> - */
> -int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
> -				void *data, int *error);
> -
>  /**
>   * file_is_sev - returns whether a file pointer is for the SEV device
>   *
> @@ -1043,9 +1019,6 @@ sev_guest_activate(struct sev_data_activate *data, int *error) { return -ENODEV;
>  
>  static inline int sev_guest_df_flush(int *error) { return -ENODEV; }
>  
> -static inline int
> -sev_issue_cmd_external_user(struct file *filep, unsigned int id, void *data, int *error) { return -ENODEV; }
> -
>  static inline bool file_is_sev(struct file *filep) { return false; }
>  
>  static inline void *psp_copy_user_blob(u64 __user uaddr, u32 len) { return ERR_PTR(-EINVAL); }

Re: [PATCH v5 09/10] KVM: SVM: Use new ccp GCTX API

[Tom Lendacky]

On 11/7/24 17:24, Dionna Glaze wrote:
> Guest context pages should be near 1-to-1 with allocated ASIDs. With the
> GCTX API, the ccp driver is better able to associate guest context pages
> with the ASID that is/will be bound to it.
> 
> This is important to the firmware hotloading implementation to not
> corrupt any running VM's guest context page before userspace commits a
> new firmware.
> 
> CC: Sean Christopherson <seanjc@google.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: Ashish Kalra <ashish.kalra@amd.com>
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: John Allen <john.allen@amd.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Michael Roth <michael.roth@amd.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> CC: Danilo Krummrich <dakr@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@kernel.org>
> CC: Tianfei zhang <tianfei.zhang@intel.com>
> CC: Alexey Kardashevskiy <aik@amd.com>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  arch/x86/kvm/svm/sev.c | 74 ++++++++++++------------------------------
>  1 file changed, 20 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index cea41b8cdabe4..d7cef84750b33 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -89,7 +89,7 @@ static unsigned int nr_asids;
>  static unsigned long *sev_asid_bitmap;
>  static unsigned long *sev_reclaim_asid_bitmap;
>  
> -static int snp_decommission_context(struct kvm *kvm);
> +static int kvm_decommission_snp_context(struct kvm *kvm);

Why the name change? It seems like it just makes the patch a bit harder
to follow since there are two things going on.

Thanks,
Tom

>  
>  struct enc_region {
>  	struct list_head list;
> @@ -2168,51 +2168,12 @@ int sev_dev_get_attr(u32 group, u64 attr, u64 *val)
>  	}
>  }
>  
> -/*
> - * The guest context contains all the information, keys and metadata
> - * associated with the guest that the firmware tracks to implement SEV
> - * and SNP features. The firmware stores the guest context in hypervisor
> - * provide page via the SNP_GCTX_CREATE command.
> - */
> -static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp)
> -{
> -	struct sev_data_snp_addr data = {};
> -	void *context;
> -	int rc;
> -
> -	/* Allocate memory for context page */
> -	context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
> -	if (!context)
> -		return ERR_PTR(-ENOMEM);
> -
> -	data.address = __psp_pa(context);
> -	rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error);
> -	if (rc) {
> -		pr_warn("Failed to create SEV-SNP context, rc %d fw_error %d",
> -			rc, argp->error);
> -		snp_free_firmware_page(context);
> -		return ERR_PTR(rc);
> -	}
> -
> -	return context;
> -}
> -
> -static int snp_bind_asid(struct kvm *kvm, int *error)
> -{
> -	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	struct sev_data_snp_activate data = {0};
> -
> -	data.gctx_paddr = __psp_pa(sev->snp_context);
> -	data.asid = sev_get_asid(kvm);
> -	return sev_issue_cmd(kvm, SEV_CMD_SNP_ACTIVATE, &data, error);
> -}
> -
>  static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  {
>  	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>  	struct sev_data_snp_launch_start start = {0};
>  	struct kvm_sev_snp_launch_start params;
> -	int rc;
> +	int rc, asid;
>  
>  	if (!sev_snp_guest(kvm))
>  		return -ENOTTY;
> @@ -2238,14 +2199,19 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
>  		return -EINVAL;
>  
> -	sev->snp_context = snp_context_create(kvm, argp);
> +	rc = sev_check_external_user(argp->sev_fd);
> +	if (rc)
> +		return rc;
> +
> +	asid = sev_get_asid(kvm);
> +	sev->snp_context = sev_snp_create_context(asid, &argp->error);
>  	if (IS_ERR(sev->snp_context))
>  		return PTR_ERR(sev->snp_context);
>  
>  	start.gctx_paddr = __psp_pa(sev->snp_context);
>  	start.policy = params.policy;
>  	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
> -	rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_LAUNCH_START, &start, &argp->error);
> +	rc = sev_do_cmd(SEV_CMD_SNP_LAUNCH_START, &start, &argp->error);
>  	if (rc) {
>  		pr_debug("%s: SEV_CMD_SNP_LAUNCH_START firmware command failed, rc %d\n",
>  			 __func__, rc);
> @@ -2253,7 +2219,7 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	}
>  
>  	sev->fd = argp->sev_fd;
> -	rc = snp_bind_asid(kvm, &argp->error);
> +	rc = sev_snp_activate_asid(asid, &argp->error);
>  	if (rc) {
>  		pr_debug("%s: Failed to bind ASID to SEV-SNP context, rc %d\n",
>  			 __func__, rc);
> @@ -2263,7 +2229,7 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	return 0;
>  
>  e_free_context:
> -	snp_decommission_context(kvm);
> +	kvm_decommission_snp_context(kvm);
>  
>  	return rc;
>  }
> @@ -2874,26 +2840,26 @@ int sev_vm_copy_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>  	return ret;
>  }
>  
> -static int snp_decommission_context(struct kvm *kvm)
> +static int kvm_decommission_snp_context(struct kvm *kvm)
>  {
>  	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	struct sev_data_snp_addr data = {};
> -	int ret;
> +	int ret, error;
>  
>  	/* If context is not created then do nothing */
>  	if (!sev->snp_context)
>  		return 0;
>  
> -	/* Do the decommision, which will unbind the ASID from the SNP context */
> -	data.address = __sme_pa(sev->snp_context);
> +	/*
> +	 * Do the decommision, which will unbind the ASID from the SNP context
> +	 * and free the context page.
> +	 */
>  	down_write(&sev_deactivate_lock);
> -	ret = sev_do_cmd(SEV_CMD_SNP_DECOMMISSION, &data, NULL);
> +	ret = sev_snp_guest_decommission(sev->asid, &error);
>  	up_write(&sev_deactivate_lock);
>  
> -	if (WARN_ONCE(ret, "Failed to release guest context, ret %d", ret))
> +	if (WARN_ONCE(ret, "Failed to release guest context, ret %d fw err %d", ret, error))
>  		return ret;
>  
> -	snp_free_firmware_page(sev->snp_context);
>  	sev->snp_context = NULL;
>  
>  	return 0;
> @@ -2947,7 +2913,7 @@ void sev_vm_destroy(struct kvm *kvm)
>  		 * Decomission handles unbinding of the ASID. If it fails for
>  		 * some unexpected reason, just leak the ASID.
>  		 */
> -		if (snp_decommission_context(kvm))
> +		if (kvm_decommission_snp_context(kvm))
>  			return;
>  	} else {
>  		sev_unbind_asid(kvm, sev->handle);

Re: [PATCH v5 10/10] KVM: SVM: Delay legacy platform initialization on SNP

[Tom Lendacky]

On 11/7/24 17:24, Dionna Glaze wrote:
> When no SEV or SEV-ES guests are active, then the firmware can be
> updated while (SEV-SNP) VM guests are active.
> 
> CC: Sean Christopherson <seanjc@google.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: Ashish Kalra <ashish.kalra@amd.com>
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: John Allen <john.allen@amd.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Michael Roth <michael.roth@amd.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> CC: Danilo Krummrich <dakr@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@kernel.org>
> CC: Tianfei zhang <tianfei.zhang@intel.com>
> CC: Alexey Kardashevskiy <aik@amd.com>
> 
> Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> Reviewed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  arch/x86/kvm/svm/sev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index d7cef84750b33..0d57a0a6b30fc 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -444,7 +444,11 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  	if (ret)
>  		goto e_no_asid;
>  
> -	init_args.probe = false;
> +	/*
> +	 * Probe will skip SEV/SEV-ES platform initialization in order for

s/Probe/Setting probe/
s/in order/for an SEV-SNP guest in order/

> +	 * SNP firmware hotloading to be available when SEV-SNP VMs are running.

s/when/when only/

Thanks,
Tom

> +	 */
> +	init_args.probe = vm_type != KVM_X86_SEV_VM && vm_type != KVM_X86_SEV_ES_VM;
>  	ret = sev_platform_init(&init_args);
>  	if (ret)
>  		goto e_free;

Re: [PATCH v5 08/10] KVM: SVM: move sev_issue_cmd_external_user to new API

[Dionna Amalie Glaze]

On Tue, Nov 12, 2024 at 7:52 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 11/7/24 17:24, Dionna Glaze wrote:
> > ccp now prefers all calls from external drivers to dominate all calls
> > into the driver on behalf of a user with a successful
> > sev_check_external_user call.
>
> Would it be simpler to have the new APIs take an fd for an argument,
> instead of doing this rework?

Simpler but I think worse?
The choice of using sev_do_cmd versus __sev_issue_cmd in kvm's
implementation is the matter of dominance of access checking.
There's no need to check the fd in the activate function or
decommission function. It's not needed to be checked in a loop for
snp_launch_update.
I can either complete the removal of __sev_issue_cmd in this patch or
move to make the context creation function take an fd. What do you
think is better?


>
> Thanks,
> Tom
>
> >
> > CC: Sean Christopherson <seanjc@google.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Thomas Gleixner <tglx@linutronix.de>
> > CC: Ingo Molnar <mingo@redhat.com>
> > CC: Borislav Petkov <bp@alien8.de>
> > CC: Dave Hansen <dave.hansen@linux.intel.com>
> > CC: Ashish Kalra <ashish.kalra@amd.com>
> > CC: Tom Lendacky <thomas.lendacky@amd.com>
> > CC: John Allen <john.allen@amd.com>
> > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: Michael Roth <michael.roth@amd.com>
> > CC: Luis Chamberlain <mcgrof@kernel.org>
> > CC: Russ Weight <russ.weight@linux.dev>
> > CC: Danilo Krummrich <dakr@redhat.com>
> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > CC: "Rafael J. Wysocki" <rafael@kernel.org>
> > CC: Tianfei zhang <tianfei.zhang@intel.com>
> > CC: Alexey Kardashevskiy <aik@amd.com>
> >
> > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> > ---
> >  arch/x86/kvm/svm/sev.c       | 18 +++++++++++++++---
> >  drivers/crypto/ccp/sev-dev.c | 12 ------------
> >  include/linux/psp-sev.h      | 27 ---------------------------
> >  3 files changed, 15 insertions(+), 42 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index d0e0152aefb32..cea41b8cdabe4 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -528,21 +528,33 @@ static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
> >       return ret;
> >  }
> >
> > -static int __sev_issue_cmd(int fd, int id, void *data, int *error)
> > +static int sev_check_external_user(int fd)
> >  {
> >       struct fd f;
> > -     int ret;
> > +     int ret = 0;
> >
> >       f = fdget(fd);
> >       if (!fd_file(f))
> >               return -EBADF;
> >
> > -     ret = sev_issue_cmd_external_user(fd_file(f), id, data, error);
> > +     if (!file_is_sev(fd_file(f)))
> > +             ret = -EBADF;
> >
> >       fdput(f);
> >       return ret;
> >  }
> >
> > +static int __sev_issue_cmd(int fd, int id, void *data, int *error)
> > +{
> > +     int ret;
> > +
> > +     ret = sev_check_external_user(fd);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return sev_do_cmd(id, data, error);
> > +}
> > +
> >  static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
> >  {
> >       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index f92e6a222da8a..67f6425b7ed07 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -2493,18 +2493,6 @@ bool file_is_sev(struct file *p)
> >  }
> >  EXPORT_SYMBOL_GPL(file_is_sev);
> >
> > -int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
> > -                             void *data, int *error)
> > -{
> > -     int rc = file_is_sev(filep) ? 0 : -EBADF;
> > -
> > -     if (rc)
> > -             return rc;
> > -
> > -     return sev_do_cmd(cmd, data, error);
> > -}
> > -EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
> > -
> >  void sev_pci_init(void)
> >  {
> >       struct sev_device *sev = psp_master->sev_data;
> > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> > index ed85c0cfcfcbe..b4164d3600702 100644
> > --- a/include/linux/psp-sev.h
> > +++ b/include/linux/psp-sev.h
> > @@ -860,30 +860,6 @@ int sev_platform_init(struct sev_platform_init_args *args);
> >   */
> >  int sev_platform_status(struct sev_user_data_status *status, int *error);
> >
> > -/**
> > - * sev_issue_cmd_external_user - issue SEV command by other driver with a file
> > - * handle.
> > - *
> > - * This function can be used by other drivers to issue a SEV command on
> > - * behalf of userspace. The caller must pass a valid SEV file descriptor
> > - * so that we know that it has access to SEV device.
> > - *
> > - * @filep - SEV device file pointer
> > - * @cmd - command to issue
> > - * @data - command buffer
> > - * @error: SEV command return code
> > - *
> > - * Returns:
> > - * 0 if the SEV successfully processed the command
> > - * -%ENODEV    if the SEV device is not available
> > - * -%ENOTSUPP  if the SEV does not support SEV
> > - * -%ETIMEDOUT if the SEV command timed out
> > - * -%EIO       if the SEV returned a non-zero return code
> > - * -%EBADF     if the file pointer is bad or does not grant access
> > - */
> > -int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
> > -                             void *data, int *error);
> > -
> >  /**
> >   * file_is_sev - returns whether a file pointer is for the SEV device
> >   *
> > @@ -1043,9 +1019,6 @@ sev_guest_activate(struct sev_data_activate *data, int *error) { return -ENODEV;
> >
> >  static inline int sev_guest_df_flush(int *error) { return -ENODEV; }
> >
> > -static inline int
> > -sev_issue_cmd_external_user(struct file *filep, unsigned int id, void *data, int *error) { return -ENODEV; }
> > -
> >  static inline bool file_is_sev(struct file *filep) { return false; }
> >
> >  static inline void *psp_copy_user_blob(u64 __user uaddr, u32 len) { return ERR_PTR(-EINVAL); }



--
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

Re: [PATCH v5 09/10] KVM: SVM: Use new ccp GCTX API

[Dionna Amalie Glaze]

> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index cea41b8cdabe4..d7cef84750b33 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -89,7 +89,7 @@ static unsigned int nr_asids;
> >  static unsigned long *sev_asid_bitmap;
> >  static unsigned long *sev_reclaim_asid_bitmap;
> >
> > -static int snp_decommission_context(struct kvm *kvm);
> > +static int kvm_decommission_snp_context(struct kvm *kvm);
>
> Why the name change? It seems like it just makes the patch a bit harder
> to follow since there are two things going on.
>

KVM and ccp both seem to like to name their functions starting with
sev_ or snp_, and it's particularly hard to determine provenance.

snp_decommision_context and sev_snp_guest_decommission... which is
from where? It's weird to me.

> Thanks,
> Tom
>


-- 
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

Re: [PATCH v5 09/10] KVM: SVM: Use new ccp GCTX API

[Tom Lendacky]

On 11/12/24 13:33, Dionna Amalie Glaze wrote:
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index cea41b8cdabe4..d7cef84750b33 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -89,7 +89,7 @@ static unsigned int nr_asids;
>>>  static unsigned long *sev_asid_bitmap;
>>>  static unsigned long *sev_reclaim_asid_bitmap;
>>>
>>> -static int snp_decommission_context(struct kvm *kvm);
>>> +static int kvm_decommission_snp_context(struct kvm *kvm);
>>
>> Why the name change? It seems like it just makes the patch a bit harder
>> to follow since there are two things going on.
>>
> 
> KVM and ccp both seem to like to name their functions starting with
> sev_ or snp_, and it's particularly hard to determine provenance.
> 
> snp_decommision_context and sev_snp_guest_decommission... which is
> from where? It's weird to me.

I guess I don't see the problem, a quick git grep -w of the name will
show you where each is. Its a static function in the file, so if
anything just changing/shortening the name to decommission_snp_context()
would be better (especially since nothing in the svm directory should
have a name that starts with kvm_).

Thanks,
Tom

> 
>> Thanks,
>> Tom
>>
> 
> 

Re: [PATCH v5 08/10] KVM: SVM: move sev_issue_cmd_external_user to new API

[Tom Lendacky]

On 11/12/24 13:30, Dionna Amalie Glaze wrote:
> On Tue, Nov 12, 2024 at 7:52 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 11/7/24 17:24, Dionna Glaze wrote:
>>> ccp now prefers all calls from external drivers to dominate all calls
>>> into the driver on behalf of a user with a successful
>>> sev_check_external_user call.
>>
>> Would it be simpler to have the new APIs take an fd for an argument,
>> instead of doing this rework?
> 
> Simpler but I think worse?
> The choice of using sev_do_cmd versus __sev_issue_cmd in kvm's
> implementation is the matter of dominance of access checking.
> There's no need to check the fd in the activate function or
> decommission function. It's not needed to be checked in a loop for
> snp_launch_update.

Very true.

> I can either complete the removal of __sev_issue_cmd in this patch or
> move to make the context creation function take an fd. What do you
> think is better?

The re-work you're looking at doing is probably a patch series on its
own. I don't think you need to do all that work for this series. You
just need to be sure that each command invocation that requires the fd
check doesn't lose that in an ioctl() path for now.

Thanks,
Tom

> 
> 
>>
>> Thanks,
>> Tom
>>
>>>
>>> CC: Sean Christopherson <seanjc@google.com>
>>> CC: Paolo Bonzini <pbonzini@redhat.com>
>>> CC: Thomas Gleixner <tglx@linutronix.de>
>>> CC: Ingo Molnar <mingo@redhat.com>
>>> CC: Borislav Petkov <bp@alien8.de>
>>> CC: Dave Hansen <dave.hansen@linux.intel.com>
>>> CC: Ashish Kalra <ashish.kalra@amd.com>
>>> CC: Tom Lendacky <thomas.lendacky@amd.com>
>>> CC: John Allen <john.allen@amd.com>
>>> CC: Herbert Xu <herbert@gondor.apana.org.au>
>>> CC: "David S. Miller" <davem@davemloft.net>
>>> CC: Michael Roth <michael.roth@amd.com>
>>> CC: Luis Chamberlain <mcgrof@kernel.org>
>>> CC: Russ Weight <russ.weight@linux.dev>
>>> CC: Danilo Krummrich <dakr@redhat.com>
>>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> CC: "Rafael J. Wysocki" <rafael@kernel.org>
>>> CC: Tianfei zhang <tianfei.zhang@intel.com>
>>> CC: Alexey Kardashevskiy <aik@amd.com>
>>>
>>> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
>>> ---
>>>  arch/x86/kvm/svm/sev.c       | 18 +++++++++++++++---
>>>  drivers/crypto/ccp/sev-dev.c | 12 ------------
>>>  include/linux/psp-sev.h      | 27 ---------------------------
>>>  3 files changed, 15 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index d0e0152aefb32..cea41b8cdabe4 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -528,21 +528,33 @@ static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
>>>       return ret;
>>>  }
>>>
>>> -static int __sev_issue_cmd(int fd, int id, void *data, int *error)
>>> +static int sev_check_external_user(int fd)
>>>  {
>>>       struct fd f;
>>> -     int ret;
>>> +     int ret = 0;
>>>
>>>       f = fdget(fd);
>>>       if (!fd_file(f))
>>>               return -EBADF;
>>>
>>> -     ret = sev_issue_cmd_external_user(fd_file(f), id, data, error);
>>> +     if (!file_is_sev(fd_file(f)))
>>> +             ret = -EBADF;
>>>
>>>       fdput(f);
>>>       return ret;
>>>  }
>>>
>>> +static int __sev_issue_cmd(int fd, int id, void *data, int *error)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = sev_check_external_user(fd);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     return sev_do_cmd(id, data, error);
>>> +}
>>> +
>>>  static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
>>>  {
>>>       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> index f92e6a222da8a..67f6425b7ed07 100644
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -2493,18 +2493,6 @@ bool file_is_sev(struct file *p)
>>>  }
>>>  EXPORT_SYMBOL_GPL(file_is_sev);
>>>
>>> -int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>> -                             void *data, int *error)
>>> -{
>>> -     int rc = file_is_sev(filep) ? 0 : -EBADF;
>>> -
>>> -     if (rc)
>>> -             return rc;
>>> -
>>> -     return sev_do_cmd(cmd, data, error);
>>> -}
>>> -EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>>> -
>>>  void sev_pci_init(void)
>>>  {
>>>       struct sev_device *sev = psp_master->sev_data;
>>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>>> index ed85c0cfcfcbe..b4164d3600702 100644
>>> --- a/include/linux/psp-sev.h
>>> +++ b/include/linux/psp-sev.h
>>> @@ -860,30 +860,6 @@ int sev_platform_init(struct sev_platform_init_args *args);
>>>   */
>>>  int sev_platform_status(struct sev_user_data_status *status, int *error);
>>>
>>> -/**
>>> - * sev_issue_cmd_external_user - issue SEV command by other driver with a file
>>> - * handle.
>>> - *
>>> - * This function can be used by other drivers to issue a SEV command on
>>> - * behalf of userspace. The caller must pass a valid SEV file descriptor
>>> - * so that we know that it has access to SEV device.
>>> - *
>>> - * @filep - SEV device file pointer
>>> - * @cmd - command to issue
>>> - * @data - command buffer
>>> - * @error: SEV command return code
>>> - *
>>> - * Returns:
>>> - * 0 if the SEV successfully processed the command
>>> - * -%ENODEV    if the SEV device is not available
>>> - * -%ENOTSUPP  if the SEV does not support SEV
>>> - * -%ETIMEDOUT if the SEV command timed out
>>> - * -%EIO       if the SEV returned a non-zero return code
>>> - * -%EBADF     if the file pointer is bad or does not grant access
>>> - */
>>> -int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
>>> -                             void *data, int *error);
>>> -
>>>  /**
>>>   * file_is_sev - returns whether a file pointer is for the SEV device
>>>   *
>>> @@ -1043,9 +1019,6 @@ sev_guest_activate(struct sev_data_activate *data, int *error) { return -ENODEV;
>>>
>>>  static inline int sev_guest_df_flush(int *error) { return -ENODEV; }
>>>
>>> -static inline int
>>> -sev_issue_cmd_external_user(struct file *filep, unsigned int id, void *data, int *error) { return -ENODEV; }
>>> -
>>>  static inline bool file_is_sev(struct file *filep) { return false; }
>>>
>>>  static inline void *psp_copy_user_blob(u64 __user uaddr, u32 len) { return ERR_PTR(-EINVAL); }
> 
> 
> 
> --
> -Dionna Glaze, PhD, CISSP, CCSP (she/her)

Re: [PATCH v5 09/10] KVM: SVM: Use new ccp GCTX API

[Sean Christopherson]

On Tue, Nov 12, 2024, Tom Lendacky wrote:
> On 11/12/24 13:33, Dionna Amalie Glaze wrote:
> >>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> >>> index cea41b8cdabe4..d7cef84750b33 100644
> >>> --- a/arch/x86/kvm/svm/sev.c
> >>> +++ b/arch/x86/kvm/svm/sev.c
> >>> @@ -89,7 +89,7 @@ static unsigned int nr_asids;
> >>>  static unsigned long *sev_asid_bitmap;
> >>>  static unsigned long *sev_reclaim_asid_bitmap;
> >>>
> >>> -static int snp_decommission_context(struct kvm *kvm);
> >>> +static int kvm_decommission_snp_context(struct kvm *kvm);
> >>
> >> Why the name change? It seems like it just makes the patch a bit harder
> >> to follow since there are two things going on.
> >>
> > 
> > KVM and ccp both seem to like to name their functions starting with
> > sev_ or snp_, and it's particularly hard to determine provenance.
> > 
> > snp_decommision_context and sev_snp_guest_decommission... which is
> > from where? It's weird to me.
> 
> I guess I don't see the problem, a quick git grep -w of the name will
> show you where each is. Its a static function in the file, so if
> anything just changing/shortening the name to decommission_snp_context()

Eh, that creates just as many problems as it solves, because it mucks up the
namespace and leads to discontinuity between the decommission helper and things
like snp_launch_update_vmsa() and snp_launch_finish().

I agree that there isn't a strong need to fixup static symbols.  That said, I do
think drivers/crypto/ccp/sev-dev.c in particular needs a different namespace, and
needs to use it consistently, to make it somewhat obvious that it's (almost) all
about the PSP/ASP.

But IMO, an even bigger mess in that area is the lack of consistency in the APIs
themselves.  E.g. this code where KVM uses sev_do_cmd() directly for SNP, but
bounces through a wrapper for !SNP.  Eww.

	wbinvd_on_all_cpus();

	if (sev_snp_enabled)
		ret = sev_do_cmd(SEV_CMD_SNP_DF_FLUSH, NULL, &error);
	else
		ret = sev_guest_df_flush(&error);

	up_write(&sev_deactivate_lock);


And then KVM has snp_page_reclaim(), but the PSP/ASP driver has snp_reclaim_pages().

So if we want to start renaming things, I vote to go a step further and clean up
the APIs, e.g. with a goal of eliminating sev_do_cmd(), and possibly of making
the majority of the PSP-defined structures in include/linux/psp-sev.h "private"
to the PSP/ASP driver.

> would be better (especially since nothing in the svm directory should
> have a name that starts with kvm_).

+1 to not using "kvm_".  KVM often uses "kvm_" to differentiate globally visible
symbols from local (static) symbols.  I.e. prepending "kvm_" just trades one
confusing name for another.