This patch set starts from a minor fix patch by adding sev decommission;
the rest 3 patches are trying to help make the interface between KVM and
psp cleaner and simpler. In particular, these patches do the following
improvements:
- avoid the requirement of psp data structures for some psp APIs.
- hide error handling within psp API, eg., using sev_guest_decommission.
- hide the serialization requirement between DF_FLUSH and DEACTIVATE.
v1 -> v2:
- split the bug fixing patch as the 1st one.
- fix the build error. [paolo]
Mingwei Zhang (3):
KVM: SVM: move sev_decommission to psp driver
KVM: SVM: move sev_bind_asid to psp
KVM: SVM: move sev_unbind_asid and DF_FLUSH logic into psp
arch/x86/kvm/svm/sev.c | 69 ++++--------------------------------
drivers/crypto/ccp/sev-dev.c | 57 +++++++++++++++++++++++++++--
include/linux/psp-sev.h | 44 ++++++++++++++++++++---
3 files changed, 102 insertions(+), 68 deletions(-)
--
2.33.0.rc1.237.g0d66db33f3-goog
sev_decommission is needed in the error path of sev_bind_asid. The purpose
of this function is to clear the firmware context. Missing this step may
cause subsequent SEV launch failures.
Although missing sev_decommission issue has previously been found and was
fixed in sev_launch_start function. It is supposed to be fixed on all
scenarios where a firmware context needs to be freed. According to the AMD
SEV API v0.24 Section 1.3.3:
"The RECEIVE_START command is the only command other than the LAUNCH_START
command that generates a new guest context and guest handle."
The above indicates that RECEIVE_START command also requires calling
sev_decommission if ASID binding fails after RECEIVE_START succeeds.
So add the sev_decommission function in sev_receive_start.
Cc: Alper Gun <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brijesh Singh <[email protected]>
Cc: David Rienjes <[email protected]>
Cc: Marc Orr <[email protected]>
Cc: John Allen <[email protected]>
Cc: Peter Gonda <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Vipin Sharma <[email protected]>
Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command")
Signed-off-by: Mingwei Zhang <[email protected]>
---
arch/x86/kvm/svm/sev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 75e0b21ad07c..55d8b9c933c3 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1397,8 +1397,10 @@ static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
/* Bind ASID to this guest */
ret = sev_bind_asid(kvm, start.handle, error);
- if (ret)
+ if (ret) {
+ sev_decommission(start.handle);
goto e_free_session;
+ }
params.handle = start.handle;
if (copy_to_user((void __user *)(uintptr_t)argp->data,
--
2.33.0.rc1.237.g0d66db33f3-goog
ccp/sev-dev.c is part of the software layer in psp that allows KVM to
manage SEV/ES/SNP enabled VMs. Among the APIs exposed in sev-dev, many of
them requires caller (KVM) to understand psp specific data structures. This
often ends up with the fact that KVM has to create its own 'wrapper' API to
make it easy to use. The following is the pattern:
kvm_func(unsigned int handle)
{
psp_data_structure data;
data.handle = handle;
psp_func(&data, NULL);
}
psp_func(psp_data_structure *data, int *error)
{
sev_do_cmd(data, error);
}
struct psp_data_structure {
u32 handle;
};
sev_decommission is one example following the above pattern. Since KVM is
the only user for this API and 'handle' is the only data that is meaningful
to KVM, simplify the interface by putting the code from kvm function
sev_decommission into the psp function sev_guest_decomssion.
No functional change intended.
Cc: Alper Gun <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brijesh Singh <[email protected]>
Cc: David Rienjes <[email protected]>
Cc: Marc Orr <[email protected]>
Cc: John Allen <[email protected]>
Cc: Peter Gonda <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Vipin Sharma <[email protected]>
Acked-by: Brijesh Singh <[email protected]>
Suggested-by: Sean Christopherson <[email protected]>
Signed-off-by: Mingwei Zhang <[email protected]>
---
arch/x86/kvm/svm/sev.c | 17 +++--------------
drivers/crypto/ccp/sev-dev.c | 10 ++++++++--
include/linux/psp-sev.h | 7 ++++---
3 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 55d8b9c933c3..b8b26a9c5369 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -198,17 +198,6 @@ static void sev_asid_free(struct kvm_sev_info *sev)
sev->misc_cg = NULL;
}
-static void sev_decommission(unsigned int handle)
-{
- struct sev_data_decommission decommission;
-
- if (!handle)
- return;
-
- decommission.handle = handle;
- sev_guest_decommission(&decommission, NULL);
-}
-
static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
{
struct sev_data_deactivate deactivate;
@@ -223,7 +212,7 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
sev_guest_deactivate(&deactivate, NULL);
up_read(&sev_deactivate_lock);
- sev_decommission(handle);
+ sev_guest_decommission(handle, NULL);
}
static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
@@ -349,7 +338,7 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
/* Bind ASID to this guest */
ret = sev_bind_asid(kvm, start.handle, error);
if (ret) {
- sev_decommission(start.handle);
+ sev_guest_decommission(start.handle, NULL);
goto e_free_session;
}
@@ -1398,7 +1387,7 @@ static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
/* Bind ASID to this guest */
ret = sev_bind_asid(kvm, start.handle, error);
if (ret) {
- sev_decommission(start.handle);
+ sev_guest_decommission(start.handle, NULL);
goto e_free_session;
}
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 91808402e0bf..e2d49bedc0ef 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -903,9 +903,15 @@ int sev_guest_activate(struct sev_data_activate *data, int *error)
}
EXPORT_SYMBOL_GPL(sev_guest_activate);
-int sev_guest_decommission(struct sev_data_decommission *data, int *error)
+int sev_guest_decommission(unsigned int handle, int *error)
{
- return sev_do_cmd(SEV_CMD_DECOMMISSION, data, error);
+ struct sev_data_decommission decommission;
+
+ if (!handle)
+ return -EINVAL;
+
+ decommission.handle = handle;
+ return sev_do_cmd(SEV_CMD_DECOMMISSION, &decommission, error);
}
EXPORT_SYMBOL_GPL(sev_guest_decommission);
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index d48a7192e881..6c0f2f451c89 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -612,17 +612,18 @@ int sev_guest_df_flush(int *error);
/**
* sev_guest_decommission - perform SEV DECOMMISSION command
*
- * @decommission: sev_data_decommission structure to be processed
+ * @handle: sev_data_decommission structure to be processed
* @sev_ret: sev command return code
*
* Returns:
* 0 if the sev successfully processed the command
+ * -%EINVAL if handle is NULL
* -%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
*/
-int sev_guest_decommission(struct sev_data_decommission *data, int *error);
+int sev_guest_decommission(unsigned int handle, int *error);
void *psp_copy_user_blob(u64 uaddr, u32 len);
@@ -637,7 +638,7 @@ static inline int
sev_guest_deactivate(struct sev_data_deactivate *data, int *error) { return -ENODEV; }
static inline int
-sev_guest_decommission(struct sev_data_decommission *data, int *error) { return -ENODEV; }
+sev_guest_decommission(unsigned int handle, int *error) { return -ENODEV; }
static inline int
sev_guest_activate(struct sev_data_activate *data, int *error) { return -ENODEV; }
--
2.33.0.rc1.237.g0d66db33f3-goog
ccp/sev-dev.c is the software layer in psp that allows KVM to manage
SEV/ES/SNP enabled VMs. Since psp API provides only primitive sev command
invocation, KVM has to do extra processing that are specific only to psp
with KVM level wrapper function.
sev_bind_asid is such a KVM function that literally wraps around
sev_guest_activate in psp with extra steps like psp data structure creation
and error processing: invoking sev_guest_decommission on activation
failure.
Since sev_bind_asid code logic is purely psp specific, putting it into psp
layer should make it more robust, since KVM does not have to worry
about error handling for all asid binding callsites.
So replace the KVM pointer in sev_bind_asid with primitive arguments: asid
and handle; slightly change the name to sev_guest_bind_asid make it
consistent with other psp APIs; add the error handling code inside
sev_guest_bind_asid and; put it into the sev-dev.c.
No functional change intended.
Cc: Alper Gun <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brijesh Singh <[email protected]>
Cc: David Rienjes <[email protected]>
Cc: Marc Orr <[email protected]>
Cc: John Allen <[email protected]>
Cc: Peter Gonda <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Vipin Sharma <[email protected]>
Acked-by: Brijesh Singh <[email protected]>
Signed-off-by: Mingwei Zhang <[email protected]>
---
arch/x86/kvm/svm/sev.c | 26 ++++----------------------
drivers/crypto/ccp/sev-dev.c | 15 +++++++++++++++
include/linux/psp-sev.h | 19 +++++++++++++++++++
3 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index b8b26a9c5369..157962aa4aff 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -252,20 +252,6 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
return ret;
}
-static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
-{
- struct sev_data_activate activate;
- int asid = sev_get_asid(kvm);
- int ret;
-
- /* activate ASID on the given handle */
- activate.handle = handle;
- activate.asid = asid;
- ret = sev_guest_activate(&activate, error);
-
- return ret;
-}
-
static int __sev_issue_cmd(int fd, int id, void *data, int *error)
{
struct fd f;
@@ -336,11 +322,9 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
goto e_free_session;
/* Bind ASID to this guest */
- ret = sev_bind_asid(kvm, start.handle, error);
- if (ret) {
- sev_guest_decommission(start.handle, NULL);
+ ret = sev_guest_bind_asid(sev_get_asid(kvm), start.handle, error);
+ if (ret)
goto e_free_session;
- }
/* return handle to userspace */
params.handle = start.handle;
@@ -1385,11 +1369,9 @@ static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
goto e_free_session;
/* Bind ASID to this guest */
- ret = sev_bind_asid(kvm, start.handle, error);
- if (ret) {
- sev_guest_decommission(start.handle, NULL);
+ ret = sev_guest_bind_asid(sev_get_asid(kvm), start.handle, error);
+ if (ret)
goto e_free_session;
- }
params.handle = start.handle;
if (copy_to_user((void __user *)(uintptr_t)argp->data,
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index e2d49bedc0ef..325e79360d9e 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -903,6 +903,21 @@ int sev_guest_activate(struct sev_data_activate *data, int *error)
}
EXPORT_SYMBOL_GPL(sev_guest_activate);
+int sev_guest_bind_asid(int asid, unsigned int handle, int *error)
+{
+ struct sev_data_activate activate;
+ int ret;
+
+ /* activate ASID on the given handle */
+ activate.handle = handle;
+ activate.asid = asid;
+ ret = sev_guest_activate(&activate, error);
+ if (ret)
+ sev_guest_decommission(handle, NULL);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sev_guest_bind_asid);
+
int sev_guest_decommission(unsigned int handle, int *error)
{
struct sev_data_decommission decommission;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 6c0f2f451c89..be50446ff3f1 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -595,6 +595,22 @@ int sev_guest_deactivate(struct sev_data_deactivate *data, int *error);
*/
int sev_guest_activate(struct sev_data_activate *data, int *error);
+/**
+ * sev_guest_bind_asid - bind an ASID with VM and does decommission on failure
+ *
+ * @asid: current ASID of the VM
+ * @handle: handle of the VM to retrieve status
+ * @sev_ret: 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
+ */
+int sev_guest_bind_asid(int asid, unsigned int handle, int *error);
+
/**
* sev_guest_df_flush - perform SEV DF_FLUSH command
*
@@ -643,6 +659,9 @@ sev_guest_decommission(unsigned int handle, int *error) { return -ENODEV; }
static inline int
sev_guest_activate(struct sev_data_activate *data, int *error) { return -ENODEV; }
+static inline int
+sev_guest_bind_asid(int asid, unsigned int handle, int *error) { return -ENODEV; }
+
static inline int sev_guest_df_flush(int *error) { return -ENODEV; }
static inline int
--
2.33.0.rc1.237.g0d66db33f3-goog
On Tue, Aug 17, 2021 at 10:39 PM Mingwei Zhang <[email protected]> wrote:
>
> sev_decommission is needed in the error path of sev_bind_asid. The purpose
> of this function is to clear the firmware context. Missing this step may
> cause subsequent SEV launch failures.
>
> Although missing sev_decommission issue has previously been found and was
> fixed in sev_launch_start function. It is supposed to be fixed on all
> scenarios where a firmware context needs to be freed. According to the AMD
> SEV API v0.24 Section 1.3.3:
>
> "The RECEIVE_START command is the only command other than the LAUNCH_START
> command that generates a new guest context and guest handle."
>
> The above indicates that RECEIVE_START command also requires calling
> sev_decommission if ASID binding fails after RECEIVE_START succeeds.
>
> So add the sev_decommission function in sev_receive_start.
>
> Cc: Alper Gun <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Brijesh Singh <[email protected]>
> Cc: David Rienjes <[email protected]>
> Cc: Marc Orr <[email protected]>
> Cc: John Allen <[email protected]>
> Cc: Peter Gonda <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Vipin Sharma <[email protected]>
>
> Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command")
> Signed-off-by: Mingwei Zhang <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 75e0b21ad07c..55d8b9c933c3 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1397,8 +1397,10 @@ static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>
> /* Bind ASID to this guest */
> ret = sev_bind_asid(kvm, start.handle, error);
> - if (ret)
> + if (ret) {
> + sev_decommission(start.handle);
> goto e_free_session;
> + }
>
> params.handle = start.handle;
> if (copy_to_user((void __user *)(uintptr_t)argp->data,
> --
> 2.33.0.rc1.237.g0d66db33f3-goog
Should this patch have the following Fixes tag?
Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command")
Reviewed-by: Marc Orr <[email protected]>
On Fri, Aug 20, 2021 at 7:11 PM Marc Orr <[email protected]> wrote:
>
> On Tue, Aug 17, 2021 at 10:39 PM Mingwei Zhang <[email protected]> wrote:
> >
> > sev_decommission is needed in the error path of sev_bind_asid. The purpose
> > of this function is to clear the firmware context. Missing this step may
> > cause subsequent SEV launch failures.
> >
> > Although missing sev_decommission issue has previously been found and was
> > fixed in sev_launch_start function. It is supposed to be fixed on all
> > scenarios where a firmware context needs to be freed. According to the AMD
> > SEV API v0.24 Section 1.3.3:
> >
> > "The RECEIVE_START command is the only command other than the LAUNCH_START
> > command that generates a new guest context and guest handle."
> >
> > The above indicates that RECEIVE_START command also requires calling
> > sev_decommission if ASID binding fails after RECEIVE_START succeeds.
> >
> > So add the sev_decommission function in sev_receive_start.
> >
> > Cc: Alper Gun <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Brijesh Singh <[email protected]>
> > Cc: David Rienjes <[email protected]>
> > Cc: Marc Orr <[email protected]>
> > Cc: John Allen <[email protected]>
> > Cc: Peter Gonda <[email protected]>
> > Cc: Sean Christopherson <[email protected]>
> > Cc: Tom Lendacky <[email protected]>
> > Cc: Vipin Sharma <[email protected]>
> >
> > Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command")
> > Signed-off-by: Mingwei Zhang <[email protected]>
> > ---
> > arch/x86/kvm/svm/sev.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 75e0b21ad07c..55d8b9c933c3 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -1397,8 +1397,10 @@ static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >
> > /* Bind ASID to this guest */
> > ret = sev_bind_asid(kvm, start.handle, error);
> > - if (ret)
> > + if (ret) {
> > + sev_decommission(start.handle);
> > goto e_free_session;
> > + }
> >
> > params.handle = start.handle;
> > if (copy_to_user((void __user *)(uintptr_t)argp->data,
> > --
> > 2.33.0.rc1.237.g0d66db33f3-goog
>
> Should this patch have the following Fixes tag?
>
> Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command")
Oops. I missed that it already has the Fixes tag. Please ignore this comment.
On Wed, Aug 18, 2021, Mingwei Zhang wrote:
> @@ -336,11 +322,9 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> goto e_free_session;
>
> /* Bind ASID to this guest */
> - ret = sev_bind_asid(kvm, start.handle, error);
> - if (ret) {
> - sev_guest_decommission(start.handle, NULL);
> + ret = sev_guest_bind_asid(sev_get_asid(kvm), start.handle, error);
> + if (ret)
> goto e_free_session;
> - }
>
> /* return handle to userspace */
> params.handle = start.handle;
...
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e2d49bedc0ef..325e79360d9e 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -903,6 +903,21 @@ int sev_guest_activate(struct sev_data_activate *data, int *error)
> }
> EXPORT_SYMBOL_GPL(sev_guest_activate);
>
> +int sev_guest_bind_asid(int asid, unsigned int handle, int *error)
> +{
> + struct sev_data_activate activate;
> + int ret;
> +
> + /* activate ASID on the given handle */
> + activate.handle = handle;
> + activate.asid = asid;
> + ret = sev_guest_activate(&activate, error);
> + if (ret)
> + sev_guest_decommission(handle, NULL);
Hrm, undoing state like this is a bad API. It assumes the caller is well-behaved,
e.g. has already done something that requires decommissioning, and it surprises
the caller, e.g. the KVM side (above) looks like it's missing error handling.
Something like this would be cleaner overall:
/* create memory encryption context */
ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_RECEIVE_START, &start,
error);
if (ret)
goto e_free_session;
/* Bind ASID to this guest */
ret = sev_guest_activate(sev_get_asid(kvm), start.handle, error);
if (ret)
goto e_decommision;
params.handle = start.handle;
if (copy_to_user((void __user *)(uintptr_t)argp->data,
¶ms, sizeof(struct kvm_sev_receive_start))) {
ret = -EFAULT;
goto e_deactivate;
}
sev->handle = start.handle;
sev->fd = argp->sev_fd;
e_deactivate:
sev_guest_deactivate(sev_get_asid(kvm), start.handle, error);
e_decommision:
sev_guest_decommission(start.handle, error);
e_free_session:
kfree(session_data);
e_free_pdh:
kfree(pdh_data);
However, I don't know that that's a good level of abstraction, e.g. the struct
details are abstracted from KVM but the exact sequencing is not, which is odd
to say the least.
Which is a good segue into my overarching complaint about the PSP API and what
made me suggest this change in the first place. IMO, the API exposed to KVM (and
others) is too low level, e.g. KVM is practically making direct calls to the PSP
via sev_issue_cmd_external_user(). Even the partially-abstracted helpers that
take a "struct sev_data_*" are too low level, KVM really shouldn't need to know
the hardware-defined structures for an off-CPU device.
My intent with the suggestion was to start driving toward a mostly-abstracted API
across the board, with an end goal of eliminating sev_issue_cmd_external_user()
and moving all of the sev_data* structs out of psp-sev.h and into a private
header. However, I think we should all explicitly agree on the desired level of
abstraction before shuffling code around.
My personal preference is obviously to work towards an abstracted API. And if
we decide to go that route, I think we should be much more aggressive with respect
to what is abstracted. Many of the functions will be rather gross due to the
sheer number of params, but I think the end result will be a net positive in terms
of readability and separation of concerns.
E.g. get KVM looking like this
static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
struct kvm_sev_receive_start params;
int ret;
if (!sev_guest(kvm))
return -ENOTTY;
/* Get parameter from the userspace */
if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data,
sizeof(struct kvm_sev_receive_start)))
return -EFAULT;
ret = sev_guest_receive_start(argp->sev_fd, &arpg->error, sev->asid,
¶ms.handle, params.policy,
params.pdh_uaddr, params.pdh_len,
params.session_uaddr, params.session_len);
if (ret)
return ret;
/* Copy params back to user even on failure, e.g. for error info. */
if (copy_to_user((void __user *)(uintptr_t)argp->data,
¶ms, sizeof(struct kvm_sev_receive_start)))
return -EFAULT;
sev->handle = params.handle;
sev->fd = argp->sev_fd;
return 0;
}
On 9/3/21 2:38 PM, Sean Christopherson wrote:
> On Wed, Aug 18, 2021, Mingwei Zhang wrote:
>> @@ -336,11 +322,9 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>> goto e_free_session;
>>
>> /* Bind ASID to this guest */
>> - ret = sev_bind_asid(kvm, start.handle, error);
>> - if (ret) {
>> - sev_guest_decommission(start.handle, NULL);
>> + ret = sev_guest_bind_asid(sev_get_asid(kvm), start.handle, error);
>> + if (ret)
>> goto e_free_session;
>> - }
>>
>> /* return handle to userspace */
>> params.handle = start.handle;
>
> ...
>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index e2d49bedc0ef..325e79360d9e 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -903,6 +903,21 @@ int sev_guest_activate(struct sev_data_activate *data, int *error)
>> }
>> EXPORT_SYMBOL_GPL(sev_guest_activate);
>>
>> +int sev_guest_bind_asid(int asid, unsigned int handle, int *error)
>> +{
>> + struct sev_data_activate activate;
>> + int ret;
>> +
>> + /* activate ASID on the given handle */
>> + activate.handle = handle;
>> + activate.asid = asid;
>> + ret = sev_guest_activate(&activate, error);
>> + if (ret)
>> + sev_guest_decommission(handle, NULL);
>
> Hrm, undoing state like this is a bad API. It assumes the caller is well-behaved,
> e.g. has already done something that requires decommissioning, and it surprises
> the caller, e.g. the KVM side (above) looks like it's missing error handling.
> Something like this would be cleaner overall:
>
> /* create memory encryption context */
> ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_RECEIVE_START, &start,
> error);
> if (ret)
> goto e_free_session;
>
> /* Bind ASID to this guest */
> ret = sev_guest_activate(sev_get_asid(kvm), start.handle, error);
> if (ret)
> goto e_decommision;
>
> params.handle = start.handle;
> if (copy_to_user((void __user *)(uintptr_t)argp->data,
> ¶ms, sizeof(struct kvm_sev_receive_start))) {
> ret = -EFAULT;
> goto e_deactivate;
> }
>
> sev->handle = start.handle;
> sev->fd = argp->sev_fd;
>
> e_deactivate:
> sev_guest_deactivate(sev_get_asid(kvm), start.handle, error);
> e_decommision:
> sev_guest_decommission(start.handle, error);
> e_free_session:
> kfree(session_data);
> e_free_pdh:
> kfree(pdh_data);
>
>
> However, I don't know that that's a good level of abstraction, e.g. the struct
> details are abstracted from KVM but the exact sequencing is not, which is odd
> to say the least.
>
> Which is a good segue into my overarching complaint about the PSP API and what
> made me suggest this change in the first place. IMO, the API exposed to KVM (and
> others) is too low level, e.g. KVM is practically making direct calls to the PSP
> via sev_issue_cmd_external_user(). Even the partially-abstracted helpers that
> take a "struct sev_data_*" are too low level, KVM really shouldn't need to know
> the hardware-defined structures for an off-CPU device.
>
> My intent with the suggestion was to start driving toward a mostly-abstracted API
> across the board, with an end goal of eliminating sev_issue_cmd_external_user()
> and moving all of the sev_data* structs out of psp-sev.h and into a private
> header. However, I think we should all explicitly agree on the desired level of
> abstraction before shuffling code around.
>
> My personal preference is obviously to work towards an abstracted API. And if
> we decide to go that route, I think we should be much more aggressive with respect
> to what is abstracted. Many of the functions will be rather gross due to the
> sheer number of params, but I think the end result will be a net positive in terms
> of readability and separation of concerns.
>
> E.g. get KVM looking like this
>
> static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> {
> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> struct kvm_sev_receive_start params;
> int ret;
>
> if (!sev_guest(kvm))
> return -ENOTTY;
>
> /* Get parameter from the userspace */
> if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data,
> sizeof(struct kvm_sev_receive_start)))
> return -EFAULT;
>
> ret = sev_guest_receive_start(argp->sev_fd, &arpg->error, sev->asid,
> ¶ms.handle, params.policy,
> params.pdh_uaddr, params.pdh_len,
> params.session_uaddr, params.session_len);
> if (ret)
> return ret;
>
> /* Copy params back to user even on failure, e.g. for error info. */
> if (copy_to_user((void __user *)(uintptr_t)argp->data,
> ¶ms, sizeof(struct kvm_sev_receive_start)))
> return -EFAULT;
>
> sev->handle = params.handle;
> sev->fd = argp->sev_fd;
> return 0;
> }
>
I have no strong preference for either of the abstraction approaches.
The sheer number of argument can also make some folks wonder whether
such abstraction makes it easy to read. e.g send-start may need up to 11.
thanks
- Brijesh
On 9/7/21 6:37 PM, Sean Christopherson wrote:
> On Tue, Sep 07, 2021, Brijesh Singh wrote:
>>
>> On 9/3/21 2:38 PM, Sean Christopherson wrote:
>>> My personal preference is obviously to work towards an abstracted API. And if
>>> we decide to go that route, I think we should be much more aggressive with respect
>>> to what is abstracted. Many of the functions will be rather gross due to the
>>> sheer number of params, but I think the end result will be a net positive in terms
>>> of readability and separation of concerns.
>>>
>>> E.g. get KVM looking like this
>>>
>>> static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>> {
>>> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>> struct kvm_sev_receive_start params;
>>> int ret;
>>>
>>> if (!sev_guest(kvm))
>>> return -ENOTTY;
>>>
>>> /* Get parameter from the userspace */
>>> if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data,
>>> sizeof(struct kvm_sev_receive_start)))
>>> return -EFAULT;
>>>
>>> ret = sev_guest_receive_start(argp->sev_fd, &arpg->error, sev->asid,
>>> ¶ms.handle, params.policy,
>>> params.pdh_uaddr, params.pdh_len,
>>> params.session_uaddr, params.session_len);
>>> if (ret)
>>> return ret;
>>>
>>> /* Copy params back to user even on failure, e.g. for error info. */
>>> if (copy_to_user((void __user *)(uintptr_t)argp->data,
>>> ¶ms, sizeof(struct kvm_sev_receive_start)))
>>> return -EFAULT;
>>>
>>> sev->handle = params.handle;
>>> sev->fd = argp->sev_fd;
>>> return 0;
>>> }
>>>
>>
>> I have no strong preference for either of the abstraction approaches. The
>> sheer number of argument can also make some folks wonder whether such
>> abstraction makes it easy to read. e.g send-start may need up to 11.
>
> Yeah, that's brutal, but IMO having a few ugly functions is an acceptable cost if
> it means the rest of the API is cleaner. E.g. KVM is not the right place to
> implement sev_deactivate_lock, as any coincident DEACTIVATE will be problematic.
> The current code "works" because KVM is the only in-tree user, but even that's a
> bit of a grey area because sev_guest_deactivate() is exported.
>
> If large param lists are problematic, one idea would be to reuse the sev_data_*
> structs for the API. I still don't like the idea of exposing those structs
> outside of the PSP driver, and the potential user vs. kernel pointer confusion
> is more than a bit ugly. On the other hand it's not exactly secret info,
> e.g. KVM's UAPI structs are already excrutiatingly close to sev_data_* structs.
>
> For future ioctls(), KVM could even define UAPI structs that are bit-for-bit
> compatible with the hardware structs. That would allow KVM to copy userspace's
> data directly into a "struct sev_data_*" and simply require the handle and any
> other KVM-defined params to be zero. KVM could then hand the whole struct over
> to the PSP driver for processing.
Most of the address field in the "struct sev_data_*" are physical
addressess. The userspace will not be able to populate those fields. PSP
or KVM may still need to assist filling the final hardware structure.
Some of fields in hardware structure must be zero, so we need to add
checks for it.
I can try posting RFC post SNP series and we can see how it all looks.
>
> We can even do a direct copy to sev_data* with KVM's current UAPI by swapping
> fields as necessary, e.g. swap policy<->handle before and after send-start, but
> that's all kinds of gross and probably not a net positive.
>
On Thu, Sep 09, 2021, Brijesh Singh wrote:
>
> On 9/7/21 6:37 PM, Sean Christopherson wrote:
> > On Tue, Sep 07, 2021, Brijesh Singh wrote:
> > > I have no strong preference for either of the abstraction approaches. The
> > > sheer number of argument can also make some folks wonder whether such
> > > abstraction makes it easy to read. e.g send-start may need up to 11.
> >
> > Yeah, that's brutal, but IMO having a few ugly functions is an acceptable cost if
> > it means the rest of the API is cleaner. E.g. KVM is not the right place to
> > implement sev_deactivate_lock, as any coincident DEACTIVATE will be problematic.
> > The current code "works" because KVM is the only in-tree user, but even that's a
> > bit of a grey area because sev_guest_deactivate() is exported.
> >
> > If large param lists are problematic, one idea would be to reuse the sev_data_*
> > structs for the API. I still don't like the idea of exposing those structs
> > outside of the PSP driver, and the potential user vs. kernel pointer confusion
> > is more than a bit ugly. On the other hand it's not exactly secret info,
> > e.g. KVM's UAPI structs are already excrutiatingly close to sev_data_* structs.
> >
> > For future ioctls(), KVM could even define UAPI structs that are bit-for-bit
> > compatible with the hardware structs. That would allow KVM to copy userspace's
> > data directly into a "struct sev_data_*" and simply require the handle and any
> > other KVM-defined params to be zero. KVM could then hand the whole struct over
> > to the PSP driver for processing.
>
> Most of the address field in the "struct sev_data_*" are physical
> addressess. The userspace will not be able to populate those fields.
Yeah, that's my biggest hesitation to using struct sev_data_* in the API, it's
both confusing and gross. But it's also why I think these helpers belong in the
PSP driver, KVM should not need to know the "on-the-wire" format for communicating
with the PSP.
> PSP or KVM may still need to assist filling the final hardware structure.
> Some of fields in hardware structure must be zero, so we need to add checks
> for it.
> I can try posting RFC post SNP series and we can see how it all looks.
I'm a bit torn. I completely understand the desire to get SNP support merged, but
at the same time KVM has accrued a fair bit of technical debt for SEV and SEV-ES,
and the lack of tests is also a concern. I don't exactly love the idea of kicking
those cans further down the road.
Paolo, any thoughts?
> > Most of the address field in the "struct sev_data_*" are physical
> > addressess. The userspace will not be able to populate those fields.
>
> Yeah, that's my biggest hesitation to using struct sev_data_* in the API, it's
> both confusing and gross. But it's also why I think these helpers belong in the
> PSP driver, KVM should not need to know the "on-the-wire" format for communicating
> with the PSP.
>
Did a simple checking for all struct sev_data_* fields defined in psp-sev.h:
The average argument is roughly 4 (103/27), detailed data appended at
last. In addition, I believe the most used commands would be the
following?
#data structure name: number of meaningful fields
sev_data_launch_start: 6
sev_data_activate: 2
sev_data_decommission: 1
sev_data_receive_update_data: 7
sev_data_send_update_vmsa: 7
sev_data_launch_measure: 3
sev_data_launch_finish: 1
sev_data_deactivate: 1
For the above frequently-used command set, the average argument length
is also around 3-4 (28/8) on average, 2.5 as the median.
So, from that perspective, I think we should just remove those
sev_data data structures in KVM, since it is more clear to read each
argument.
In addition, having to construct each sev_data_* structure in KVM code
is also a pain and consumes a lot of irrelevant lines as well.
#data structure name: number of meaningful fields
sev_data_deactivate: 1
sev_data_decommission: 1
sev_data_launch_finish: 1
sev_data_receive_finish: 1
sev_data_send_cancel: 1
sev_data_send_finish: 1
sev_data_activate: 2
sev_data_download_firmware: 2
sev_data_get_id: 2
sev_data_pek_csr: 2
sev_data_init: 3
sev_data_launch_measure: 3
sev_data_launch_update_data: 3
sev_data_launch_update_vmsa: 3
sev_data_attestation_report: 4
sev_data_dbg: 4
sev_data_guest_status: 4
sev_data_pdh_cert_export: 4
sev_data_pek_cert_import: 4
sev_data_launch_start: 6
sev_data_receive_start: 6
sev_data_launch_secret: 7
sev_data_receive_update_data: 7
sev_data_receive_update_vmsa: 7
sev_data_send_update_data: 7
sev_data_send_update_vmsa: 7
sev_data_send_start: 10
On 9/9/21 4:18 PM, Mingwei Zhang wrote:
>>> Most of the address field in the "struct sev_data_*" are physical
>>> addressess. The userspace will not be able to populate those fields.
>>
>> Yeah, that's my biggest hesitation to using struct sev_data_* in the API, it's
>> both confusing and gross. But it's also why I think these helpers belong in the
>> PSP driver, KVM should not need to know the "on-the-wire" format for communicating
>> with the PSP.
>>
>
> Did a simple checking for all struct sev_data_* fields defined in psp-sev.h:
thank you for compiling the list, let me add few more at the end of your
list (they are part of spec but PSP/KVM does not have support for it).
I am also adding SNP specific so that we get a full picture.
>
> The average argument is roughly 4 (103/27), detailed data appended at
> last. In addition, I believe the most used commands would be the
> following?
>
> #data structure name: number of meaningful fields
> sev_data_launch_start: 6
> sev_data_activate: 2
> sev_data_decommission: 1
> sev_data_receive_update_data: 7
> sev_data_send_update_vmsa: 7
> sev_data_launch_measure: 3
> sev_data_launch_finish: 1
> sev_data_deactivate: 1
>
Once the page copy, and swap in/out is implemented then it will also be
frequently used.
sev_data_copy: 4
sev_swap_out: 12
sev_swap_in: 7
snp_data_gctx_create: 1
snp_data_activate: 2
snp_data_deactivate: 2
snp_data_decommission: 1
snp_data_launch_start: 6
snp_data_launch_update: 8
snp_data_launch_finish: 6
snp_data_page_move: 4
snp_data_page_swap_out: 8
snp_data_page_swap_in: 8
snp_data_page_reclaim: 2
snp_guest_request: 3
snp_guest_request_ext: 5
> For the above frequently-used command set, the average argument length
> is also around 3-4 (28/8) on average, 2.5 as the median.
>
It averages around 4-5 (51/11) without snp. The good news is with snp
the avg is still 4-5 (107/24).
Additionally, we also need to pass the sev->fd in each of these
functions, which will increase avg to 5-6.
> So, from that perspective, I think we should just remove those
> sev_data data structures in KVM, since it is more clear to read each
> argument.
>
I believe once we are done with it, will have 5 functions that will need
>=8 arguments. I don't know if its acceptable.
> In addition, having to construct each sev_data_* structure in KVM code
> is also a pain and consumes a lot of irrelevant lines as well.
>
Maybe I am missing something, aren't those lines will be moved from KVM
to PSP driver?
I am in full support for restructuring, but lets look at full set of PSP
APIs before making the final decision.
thanks
> #data structure name: number of meaningful fields
> sev_data_deactivate: 1
> sev_data_decommission: 1
> sev_data_launch_finish: 1
> sev_data_receive_finish: 1
> sev_data_send_cancel: 1
> sev_data_send_finish: 1
> sev_data_activate: 2
> sev_data_download_firmware: 2
> sev_data_get_id: 2
> sev_data_pek_csr: 2
> sev_data_init: 3
> sev_data_launch_measure: 3
> sev_data_launch_update_data: 3
> sev_data_launch_update_vmsa: 3
> sev_data_attestation_report: 4
> sev_data_dbg: 4
> sev_data_guest_status: 4
> sev_data_pdh_cert_export: 4
> sev_data_pek_cert_import: 4
> sev_data_launch_start: 6
> sev_data_receive_start: 6
> sev_data_launch_secret: 7
> sev_data_receive_update_data: 7
> sev_data_receive_update_vmsa: 7
> sev_data_send_update_data: 7
> sev_data_send_update_vmsa: 7
> sev_data_send_start: 10
>
On Thu, Sep 9, 2021 at 6:18 PM Mingwei Zhang <[email protected]> wrote:
>
> > I believe once we are done with it, will have 5 functions that will need
> > >=8 arguments. I don't know if its acceptable.
> >
> > > In addition, having to construct each sev_data_* structure in KVM code
> > > is also a pain and consumes a lot of irrelevant lines as well.
> > >
> >
> > Maybe I am missing something, aren't those lines will be moved from KVM
> > to PSP driver?
> >
> > I am in full support for restructuring, but lets look at full set of PSP
> > APIs before making the final decision.
> >
> > thanks
> >
>
> Oh, sorry for the confusion. I think the current feedback I got is
> that my restructuring patchset was blocked due to the fact that it is
> a partial one. So, if this patchset got checked in, then the psp-sev.h
> will have two types of APIs: ones that use sev_data_* structure and
> ones that do not. So one of the worries is that this would make the
> situation even worse.
>
> So that's why I am thinking that maybe it is fine to just avoid using
> sev_data_* for all PSP functions exposed to KVM? I use the number of
> arguments as the justification. But that might not be a good one.
>
> In anycase, I will not rush into any code change before we reach a consensus.
Isn't the first patch in this patch set a straight-forward bug fix
:-)? Assuming others agree, I'd suggest to re-send that one out as a
single patch on its own, so we can get it merged while the rest of
this patch set works its way through the process.