Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack
On 07/04/21 19:34, Borislav Petkov wrote:
On Wed, Apr 07, 2021 at 05:05:07PM +, Sean Christopherson wrote:
I used memset() to defer initialization until after the various sanity
checks,
I'd actually vote for that too - I don't like doing stuff which is not
going to be used. I.e., don't change what you have.
It's three votes for that then. :)
Sean, I squashed in this change
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index ec4c01807272..a4d0ca8c4710 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1039,21 +1039,14 @@ static int sev_get_attestation_report(struct kvm *kvm,
struct kvm_sev_cmd *argp)
static int sev_send_cancel(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = _kvm_svm(kvm)->sev_info;
- struct sev_data_send_cancel *data;
+ struct sev_data_send_cancel data;
int ret;
if (!sev_guest(kvm))
return -ENOTTY;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
- data->handle = sev->handle;
- ret = sev_issue_cmd(kvm, SEV_CMD_SEND_CANCEL, data, >error);
-
- kfree(data);
- return ret;
+ data.handle = sev->handle;
+ return sev_issue_cmd(kvm, SEV_CMD_SEND_CANCEL, , >error);
}
int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
to handle SV_CMD_SEND_CANCEL which I merged before this series.
Paolo
Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack
On Wed, Apr 07, 2021 at 05:05:07PM +, Sean Christopherson wrote: > I used memset() to defer initialization until after the various sanity > checks, I'd actually vote for that too - I don't like doing stuff which is not going to be used. I.e., don't change what you have. -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack
Le 07/04/2021 à 19:05, Sean Christopherson a écrit :
On Wed, Apr 07, 2021, Borislav Petkov wrote:
First of all, I'd strongly suggest you trim your emails when you reply -
that would be much appreciated.
On Wed, Apr 07, 2021 at 07:24:54AM +0200, Christophe Leroy wrote:
@@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void
*data, int *error)
static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = _kvm_svm(kvm)->sev_info;
- struct sev_data_launch_start *start;
+ struct sev_data_launch_start start;
struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};
I don't know how this is any better than using memset...
Also, you can do
... start = { };
which is certainly the only other alternative to memset, AFAIK.
But whatever you do, you need to look at the resulting asm the compiler
generates. So let's do that:
I'm ok with Boris' version, I'm not a fan of having to count zeros. I used
memset() to defer initialization until after the various sanity checks, and
out of habit.
Yes I also like Boris' version ... start = { }; better than mine.
Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack
On Wed, Apr 07, 2021, Borislav Petkov wrote:
> First of all, I'd strongly suggest you trim your emails when you reply -
> that would be much appreciated.
>
> On Wed, Apr 07, 2021 at 07:24:54AM +0200, Christophe Leroy wrote:
> > > @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id,
> > > void *data, int *error)
> > > static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > > {
> > > struct kvm_sev_info *sev = _kvm_svm(kvm)->sev_info;
> > > - struct sev_data_launch_start *start;
> > > + struct sev_data_launch_start start;
> >
> > struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};
>
> I don't know how this is any better than using memset...
>
> Also, you can do
>
> ... start = { };
>
> which is certainly the only other alternative to memset, AFAIK.
>
> But whatever you do, you need to look at the resulting asm the compiler
> generates. So let's do that:
I'm ok with Boris' version, I'm not a fan of having to count zeros. I used
memset() to defer initialization until after the various sanity checks, and
out of habit.
Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack
First of all, I'd strongly suggest you trim your emails when you reply -
that would be much appreciated.
On Wed, Apr 07, 2021 at 07:24:54AM +0200, Christophe Leroy wrote:
> > @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void
> > *data, int *error)
> > static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > {
> > struct kvm_sev_info *sev = _kvm_svm(kvm)->sev_info;
> > - struct sev_data_launch_start *start;
> > + struct sev_data_launch_start start;
>
> struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};
I don't know how this is any better than using memset...
Also, you can do
... start = { };
which is certainly the only other alternative to memset, AFAIK.
But whatever you do, you need to look at the resulting asm the compiler
generates. So let's do that:
Your version:
# arch/x86/kvm/svm/sev.c:261: struct sev_data_launch_start _tmp = {0, 0, 0,
0, 0, 0, 0};
movq$0, 104(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
movq$0, 112(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
movq$0, 120(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
movq$0, 128(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
movl$0, 136(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
my version:
# arch/x86/kvm/svm/sev.c:261: struct sev_data_launch_start _tmp = {};
movq$0, 104(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
movq$0, 112(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
movq$0, 120(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
movq$0, 128(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
movl$0, 136(%rsp) #, MEM[(struct sev_data_launch_start *)_561]
the memset version:
# arch/x86/kvm/svm/sev.c:269: memset(&_tmp, 0, sizeof(_tmp));
#NO_APP
movq$0, 104(%rsp) #, MEM [(void *)_561]
movq$0, 112(%rsp) #, MEM [(void *)_561]
movq$0, 120(%rsp) #, MEM [(void *)_561]
movq$0, 128(%rsp) #, MEM [(void *)_561]
movl$0, 136(%rsp) #, MEM [(void *)_561]
Ok?
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG
Nürnberg
Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack
Le 07/04/2021 à 00:49, Sean Christopherson a écrit :
Use the local stack to "allocate" the structures used to communicate with
the PSP. The largest struct used by KVM, sev_data_launch_secret, clocks
in at 52 bytes, well within the realm of reasonable stack usage. The
smallest structs are a mere 4 bytes, i.e. the pointer for the allocation
is larger than the allocation itself.
Now that the PSP driver plays nice with vmalloc pointers, putting the
data on a virtually mapped stack (CONFIG_VMAP_STACK=y) will not cause
explosions.
Cc: Brijesh Singh
Cc: Tom Lendacky
Signed-off-by: Sean Christopherson
---
arch/x86/kvm/svm/sev.c | 262 +++--
1 file changed, 96 insertions(+), 166 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5457138c7347..316fd39c7aef 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -150,35 +150,22 @@ static void sev_asid_free(int asid)
static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
{
- struct sev_data_decommission *decommission;
- struct sev_data_deactivate *data;
+ struct sev_data_decommission decommission;
+ struct sev_data_deactivate deactivate;
if (!handle)
return;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return;
-
- /* deactivate handle */
- data->handle = handle;
+ deactivate.handle = handle;
/* Guard DEACTIVATE against WBINVD/DF_FLUSH used in ASID recycling */
down_read(_deactivate_lock);
- sev_guest_deactivate(data, NULL);
+ sev_guest_deactivate(, NULL);
up_read(_deactivate_lock);
- kfree(data);
-
- decommission = kzalloc(sizeof(*decommission), GFP_KERNEL);
- if (!decommission)
- return;
-
/* decommission handle */
- decommission->handle = handle;
- sev_guest_decommission(decommission, NULL);
-
- kfree(decommission);
+ decommission.handle = handle;
+ sev_guest_decommission(, NULL);
}
static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
@@ -216,19 +203,14 @@ static int sev_guest_init(struct kvm *kvm, struct
kvm_sev_cmd *argp)
static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
{
- struct sev_data_activate *data;
+ struct sev_data_activate activate;
int asid = sev_get_asid(kvm);
int ret;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- return -ENOMEM;
-
/* activate ASID on the given handle */
- data->handle = handle;
- data->asid = asid;
- ret = sev_guest_activate(data, error);
- kfree(data);
+ activate.handle = handle;
+ activate.asid = asid;
+ ret = sev_guest_activate(, error);
return ret;
}
@@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void
*data, int *error)
static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = _kvm_svm(kvm)->sev_info;
- struct sev_data_launch_start *start;
+ struct sev_data_launch_start start;
struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};
struct kvm_sev_launch_start params;
void *dh_blob, *session_blob;
int *error = >error;
@@ -270,20 +252,16 @@ static int sev_launch_start(struct kvm *kvm, struct
kvm_sev_cmd *argp)
if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
sizeof(params)))
return -EFAULT;
- start = kzalloc(sizeof(*start), GFP_KERNEL_ACCOUNT);
- if (!start)
- return -ENOMEM;
+ memset(, 0, sizeof(start));
Not needed.
dh_blob = NULL;
if (params.dh_uaddr) {
dh_blob = psp_copy_user_blob(params.dh_uaddr, params.dh_len);
- if (IS_ERR(dh_blob)) {
- ret = PTR_ERR(dh_blob);
- goto e_free;
- }
+ if (IS_ERR(dh_blob))
+ return PTR_ERR(dh_blob);
- start->dh_cert_address = __sme_set(__pa(dh_blob));
- start->dh_cert_len = params.dh_len;
+ start.dh_cert_address = __sme_set(__pa(dh_blob));
+ start.dh_cert_len = params.dh_len;
}
session_blob = NULL;
@@ -294,40 +272,38 @@ static int sev_launch_start(struct kvm *kvm, struct
kvm_sev_cmd *argp)
goto e_free_dh;
}
- start->session_address = __sme_set(__pa(session_blob));
- start->session_len = params.session_len;
+ start.session_address = __sme_set(__pa(session_blob));
+ start.session_len = params.session_len;
}
- start->handle = params.handle;
- start->policy = params.policy;
+ start.handle = params.handle;
+ start.policy = params.policy;
/* create memory encryption context */
- ret =
[PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack
Use the local stack to "allocate" the structures used to communicate with
the PSP. The largest struct used by KVM, sev_data_launch_secret, clocks
in at 52 bytes, well within the realm of reasonable stack usage. The
smallest structs are a mere 4 bytes, i.e. the pointer for the allocation
is larger than the allocation itself.
Now that the PSP driver plays nice with vmalloc pointers, putting the
data on a virtually mapped stack (CONFIG_VMAP_STACK=y) will not cause
explosions.
Cc: Brijesh Singh
Cc: Tom Lendacky
Signed-off-by: Sean Christopherson
---
arch/x86/kvm/svm/sev.c | 262 +++--
1 file changed, 96 insertions(+), 166 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5457138c7347..316fd39c7aef 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -150,35 +150,22 @@ static void sev_asid_free(int asid)
static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
{
- struct sev_data_decommission *decommission;
- struct sev_data_deactivate *data;
+ struct sev_data_decommission decommission;
+ struct sev_data_deactivate deactivate;
if (!handle)
return;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return;
-
- /* deactivate handle */
- data->handle = handle;
+ deactivate.handle = handle;
/* Guard DEACTIVATE against WBINVD/DF_FLUSH used in ASID recycling */
down_read(_deactivate_lock);
- sev_guest_deactivate(data, NULL);
+ sev_guest_deactivate(, NULL);
up_read(_deactivate_lock);
- kfree(data);
-
- decommission = kzalloc(sizeof(*decommission), GFP_KERNEL);
- if (!decommission)
- return;
-
/* decommission handle */
- decommission->handle = handle;
- sev_guest_decommission(decommission, NULL);
-
- kfree(decommission);
+ decommission.handle = handle;
+ sev_guest_decommission(, NULL);
}
static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
@@ -216,19 +203,14 @@ static int sev_guest_init(struct kvm *kvm, struct
kvm_sev_cmd *argp)
static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
{
- struct sev_data_activate *data;
+ struct sev_data_activate activate;
int asid = sev_get_asid(kvm);
int ret;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- return -ENOMEM;
-
/* activate ASID on the given handle */
- data->handle = handle;
- data->asid = asid;
- ret = sev_guest_activate(data, error);
- kfree(data);
+ activate.handle = handle;
+ activate.asid = asid;
+ ret = sev_guest_activate(, error);
return ret;
}
@@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void
*data, int *error)
static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = _kvm_svm(kvm)->sev_info;
- struct sev_data_launch_start *start;
+ struct sev_data_launch_start start;
struct kvm_sev_launch_start params;
void *dh_blob, *session_blob;
int *error = >error;
@@ -270,20 +252,16 @@ static int sev_launch_start(struct kvm *kvm, struct
kvm_sev_cmd *argp)
if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
sizeof(params)))
return -EFAULT;
- start = kzalloc(sizeof(*start), GFP_KERNEL_ACCOUNT);
- if (!start)
- return -ENOMEM;
+ memset(, 0, sizeof(start));
dh_blob = NULL;
if (params.dh_uaddr) {
dh_blob = psp_copy_user_blob(params.dh_uaddr, params.dh_len);
- if (IS_ERR(dh_blob)) {
- ret = PTR_ERR(dh_blob);
- goto e_free;
- }
+ if (IS_ERR(dh_blob))
+ return PTR_ERR(dh_blob);
- start->dh_cert_address = __sme_set(__pa(dh_blob));
- start->dh_cert_len = params.dh_len;
+ start.dh_cert_address = __sme_set(__pa(dh_blob));
+ start.dh_cert_len = params.dh_len;
}
session_blob = NULL;
@@ -294,40 +272,38 @@ static int sev_launch_start(struct kvm *kvm, struct
kvm_sev_cmd *argp)
goto e_free_dh;
}
- start->session_address = __sme_set(__pa(session_blob));
- start->session_len = params.session_len;
+ start.session_address = __sme_set(__pa(session_blob));
+ start.session_len = params.session_len;
}
- start->handle = params.handle;
- start->policy = params.policy;
+ start.handle = params.handle;
+ start.policy = params.policy;
/* create memory encryption context */
- ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, start, error);
+ ret =
