Allocate a 88 byte request structure on stack and skip needless
kzalloc/kfree.

While at this, correct indent.

No functional change intended.

Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---
Changes:
v4:
* keep initialization of @req in one place as before
---
 arch/x86/coco/sev/core.c | 30 ++++++++------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 7a49c896feb4..818ae7b1694b 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -2002,7 +2002,7 @@ static int __init snp_get_tsc_info(void)
 	struct snp_tsc_info_resp *tsc_resp;
 	struct snp_tsc_info_req *tsc_req;
 	struct snp_msg_desc *mdesc;
-	struct snp_guest_req *req;
+	struct snp_guest_req req = {};
 	int rc = -ENOMEM;
 
 	tsc_req = kzalloc(sizeof(*tsc_req), GFP_KERNEL);
@@ -2018,28 +2018,24 @@ static int __init snp_get_tsc_info(void)
 	if (!tsc_resp)
 		goto e_free_tsc_req;
 
-	req = kzalloc(sizeof(*req), GFP_KERNEL);
-	if (!req)
-		goto e_free_tsc_resp;
-
 	mdesc = snp_msg_alloc();
 	if (IS_ERR_OR_NULL(mdesc))
-		goto e_free_req;
+		goto e_free_tsc_resp;
 
 	rc = snp_msg_init(mdesc, snp_vmpl);
 	if (rc)
 		goto e_free_mdesc;
 
-	req->msg_version = MSG_HDR_VER;
-	req->msg_type = SNP_MSG_TSC_INFO_REQ;
-	req->vmpck_id = snp_vmpl;
-	req->req_buf = tsc_req;
-	req->req_sz = sizeof(*tsc_req);
-	req->resp_buf = (void *)tsc_resp;
-	req->resp_sz = sizeof(*tsc_resp) + AUTHTAG_LEN;
-	req->exit_code = SVM_VMGEXIT_GUEST_REQUEST;
+	req.msg_version = MSG_HDR_VER;
+	req.msg_type = SNP_MSG_TSC_INFO_REQ;
+	req.vmpck_id = snp_vmpl;
+	req.req_buf = tsc_req;
+	req.req_sz = sizeof(*tsc_req);
+	req.resp_buf = (void *)tsc_resp;
+	req.resp_sz = sizeof(*tsc_resp) + AUTHTAG_LEN;
+	req.exit_code = SVM_VMGEXIT_GUEST_REQUEST;
 
-	rc = snp_send_guest_request(mdesc, req);
+	rc = snp_send_guest_request(mdesc, &req);
 	if (rc)
 		goto e_request;
 
@@ -2060,9 +2056,7 @@ static int __init snp_get_tsc_info(void)
 	memzero_explicit(tsc_resp, sizeof(*tsc_resp) + AUTHTAG_LEN);
 e_free_mdesc:
 	snp_msg_free(mdesc);
-e_free_req:
-	kfree(req);
- e_free_tsc_resp:
+e_free_tsc_resp:
 	kfree(tsc_resp);
 e_free_tsc_req:
 	kfree(tsc_req);
-- 
2.49.0

On 5/5/25 07:12, Alexey Kardashevskiy wrote:
> Allocate a 88 byte request structure on stack and skip needless
> kzalloc/kfree.

Could you maybe take a closer look at _all_ of these rather than poking
at them one at a time?

snp_guest_request_ioctl, for example, looks to be ~32 bytes. Why fix
'struct snp_guest_req' and leave an even worse offender?

Or, maybe just be done with it and convert them all over to __free().
Yeah, some of them don't need to be kmalloc(), but kmalloc()s are cheap
and consistency is nice, like in the attached patch.

It also wouldn't be awful to mix stack and kmalloc() allocations,
especially when the freeing semantics are the same for stack and
__free()-annotated allocations.

But it would be really nice to completely eliminate the goto mess.

On 6/5/25 02:03, Dave Hansen wrote:
> On 5/5/25 07:12, Alexey Kardashevskiy wrote:
>> Allocate a 88 byte request structure on stack and skip needless
>> kzalloc/kfree.
> 
> Could you maybe take a closer look at _all_ of these rather than poking
> at them one at a time?
> 
> snp_guest_request_ioctl, for example, looks to be ~32 bytes. Why fix
> 'struct snp_guest_req' and leave an even worse offender?

snp_guest_request_ioctl is allocated on the stack in snp_guest_ioctl(), it calls, say, get_report() which allocates snp_guest_req on the stack too. Do I miss something?

> Or, maybe just be done with it and convert them all over to __free().
> Yeah, some of them don't need to be kmalloc(), but kmalloc()s are cheap
> and consistency is nice, like in the attached patch.

I'd rather not. cheap != free, also hurts to read all these __free - I know it is cheap to kmalloc() and initialize pointers on the stack with NULL but also useless.

More to the oint - it helps (at least me) to see from declarations what structure must be page aligned page size (or any other special allocation requirements) allocation for aesgcm_encrypt() to not barf later on and what does not.

> It also wouldn't be awful to mix stack and kmalloc() allocations,
> especially when the freeing semantics are the same for stack and
> __free()-annotated allocations.

If anything, I'd rather merge snp_msg_alloc() into snp_msg_init() and skip on allocating the snp_msg_desc struct.

For now I want the patch to be painfully simple to review and make the code a little easier to read.

> But it would be really nice to completely eliminate the goto mess.

I understand it is 2025 but it is not exactly mess. Thanks for the review, I am planning to follow up on this, just probably not exactly with __free-cation of everything.


-- 
Alexey

On Mon, May 5, 2025 at 7:13 AM Alexey Kardashevskiy <aik@amd.com> wrote:
>
> Allocate a 88 byte request structure on stack and skip needless
> kzalloc/kfree.
>
> While at this, correct indent.
>
> No functional change intended.
>
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>

Reviewed-by: Dionna Glaze <dionnaglaze@google.com>

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