[PATCH 0/4] x86/sev: Rework SNP Guest Request

[PATCH 0/4] x86/sev: Rework SNP Guest Request

[Alexey Kardashevskiy]

Started as a part of the fix for
ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex")

these are the remainder of followup fixes/improvements as
the SNP Guest Request code evolved enough to make it a bit hard to
follow.


This is based on sha1
6c201ee95a30 Ingo Molnar "Merge branch into tip/master: 'x86/sgx'".

Please comment. Thanks.



Alexey Kardashevskiy (4):
  virt: sev-guest: Contain snp_guest_request_ioctl in sev-guest
  x86/sev: Allocate request in TSC_INFO_REQ on stack
  x86/sev: Document requirement for linear mapping of Guest Request
    buffers
  x86/sev: Drop unnecessary parameter in snp_issue_guest_request

 arch/x86/include/asm/sev.h              | 10 ++-
 arch/x86/coco/sev/core.c                | 73 +++++++++-----------
 drivers/virt/coco/sev-guest/sev-guest.c |  9 ++-
 3 files changed, 44 insertions(+), 48 deletions(-)

-- 
2.49.0

[PATCH 1/4] virt: sev-guest: Contain snp_guest_request_ioctl in sev-guest

[Alexey Kardashevskiy]

SNP Guest Request only uses exitinfo2 which is a return value from GHCB,
has meaning beyond ioctl and therefore belongs to snp_guest_req.

Move exitinfo2 to snp_guest_req, remove snp_guest_request_ioctl from
the SEV platform code.

No functional change intended.

Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---

Not passing exitinfo2 as a parameter as it's been mentioned on v2:

===
having a struct request which collects all fields you need to pass down or return to callers is the RightThing(tm) to do.
===
---
 arch/x86/include/asm/sev.h              | 10 +++---
 arch/x86/coco/sev/core.c                | 36 +++++++-------------
 drivers/virt/coco/sev-guest/sev-guest.c |  9 +++--
 3 files changed, 23 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 13a88a4b52a0..db233a03f472 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -231,6 +231,7 @@ struct snp_guest_req {
 	size_t resp_sz;
 
 	u64 exit_code;
+	u64 exitinfo2;
 	unsigned int vmpck_id;
 	u8 msg_version;
 	u8 msg_type;
@@ -486,8 +487,6 @@ static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate)
 	return rc;
 }
 
-struct snp_guest_request_ioctl;
-
 void setup_ghcb(void);
 void early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr,
 				  unsigned long npages);
@@ -513,8 +512,7 @@ void snp_kexec_begin(void);
 int snp_msg_init(struct snp_msg_desc *mdesc, int vmpck_id);
 struct snp_msg_desc *snp_msg_alloc(void);
 void snp_msg_free(struct snp_msg_desc *mdesc);
-int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
-			   struct snp_guest_request_ioctl *rio);
+int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req);
 
 int snp_svsm_vtpm_send_command(u8 *buffer);
 
@@ -565,8 +563,8 @@ static inline void snp_kexec_begin(void) { }
 static inline int snp_msg_init(struct snp_msg_desc *mdesc, int vmpck_id) { return -1; }
 static inline struct snp_msg_desc *snp_msg_alloc(void) { return NULL; }
 static inline void snp_msg_free(struct snp_msg_desc *mdesc) { }
-static inline int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
-					 struct snp_guest_request_ioctl *rio) { return -ENODEV; }
+static inline int snp_send_guest_request(struct snp_msg_desc *mdesc,
+					 struct snp_guest_req *req) { return -ENODEV; }
 static inline int snp_svsm_vtpm_send_command(u8 *buffer) { return -ENODEV; }
 static inline void __init snp_secure_tsc_prepare(void) { }
 static inline void __init snp_secure_tsc_init(void) { }
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 5b145446e991..7a49c896feb4 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1322,8 +1322,7 @@ int snp_issue_svsm_attest_req(u64 call_id, struct svsm_call *call,
 }
 EXPORT_SYMBOL_GPL(snp_issue_svsm_attest_req);
 
-static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_data *input,
-				   struct snp_guest_request_ioctl *rio)
+static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_data *input)
 {
 	struct ghcb_state state;
 	struct es_em_ctxt ctxt;
@@ -1331,7 +1330,7 @@ static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_dat
 	struct ghcb *ghcb;
 	int ret;
 
-	rio->exitinfo2 = SEV_RET_NO_FW_CALL;
+	req->exitinfo2 = SEV_RET_NO_FW_CALL;
 
 	/*
 	 * __sev_get_ghcb() needs to run with IRQs disabled because it is using
@@ -1356,8 +1355,8 @@ static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_dat
 	if (ret)
 		goto e_put;
 
-	rio->exitinfo2 = ghcb->save.sw_exit_info_2;
-	switch (rio->exitinfo2) {
+	req->exitinfo2 = ghcb->save.sw_exit_info_2;
+	switch (req->exitinfo2) {
 	case 0:
 		break;
 
@@ -1852,8 +1851,7 @@ static int enc_payload(struct snp_msg_desc *mdesc, u64 seqno, struct snp_guest_r
 	return 0;
 }
 
-static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
-				  struct snp_guest_request_ioctl *rio)
+static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req)
 {
 	unsigned long req_start = jiffies;
 	unsigned int override_npages = 0;
@@ -1867,7 +1865,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
 	 * sequence number must be incremented or the VMPCK must be deleted to
 	 * prevent reuse of the IV.
 	 */
-	rc = snp_issue_guest_request(req, &req->input, rio);
+	rc = snp_issue_guest_request(req, &req->input);
 	switch (rc) {
 	case -ENOSPC:
 		/*
@@ -1920,7 +1918,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
 	snp_inc_msg_seqno(mdesc);
 
 	if (override_err) {
-		rio->exitinfo2 = override_err;
+		req->exitinfo2 = override_err;
 
 		/*
 		 * If an extended guest request was issued and the supplied certificate
@@ -1938,8 +1936,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
 	return rc;
 }
 
-int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
-			   struct snp_guest_request_ioctl *rio)
+int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req)
 {
 	u64 seqno;
 	int rc;
@@ -1976,14 +1973,14 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req
 	req->input.resp_gpa = __pa(mdesc->response);
 	req->input.data_gpa = req->certs_data ? __pa(req->certs_data) : 0;
 
-	rc = __handle_guest_request(mdesc, req, rio);
+	rc = __handle_guest_request(mdesc, req);
 	if (rc) {
 		if (rc == -EIO &&
-		    rio->exitinfo2 == SNP_GUEST_VMM_ERR(SNP_GUEST_VMM_ERR_INVALID_LEN))
+		    req->exitinfo2 == SNP_GUEST_VMM_ERR(SNP_GUEST_VMM_ERR_INVALID_LEN))
 			return rc;
 
 		pr_alert("Detected error from ASP request. rc: %d, exitinfo2: 0x%llx\n",
-			 rc, rio->exitinfo2);
+			 rc, req->exitinfo2);
 
 		snp_disable_vmpck(mdesc);
 		return rc;
@@ -2002,7 +1999,6 @@ EXPORT_SYMBOL_GPL(snp_send_guest_request);
 
 static int __init snp_get_tsc_info(void)
 {
-	struct snp_guest_request_ioctl *rio;
 	struct snp_tsc_info_resp *tsc_resp;
 	struct snp_tsc_info_req *tsc_req;
 	struct snp_msg_desc *mdesc;
@@ -2026,13 +2022,9 @@ static int __init snp_get_tsc_info(void)
 	if (!req)
 		goto e_free_tsc_resp;
 
-	rio = kzalloc(sizeof(*rio), GFP_KERNEL);
-	if (!rio)
-		goto e_free_req;
-
 	mdesc = snp_msg_alloc();
 	if (IS_ERR_OR_NULL(mdesc))
-		goto e_free_rio;
+		goto e_free_req;
 
 	rc = snp_msg_init(mdesc, snp_vmpl);
 	if (rc)
@@ -2047,7 +2039,7 @@ static int __init snp_get_tsc_info(void)
 	req->resp_sz = sizeof(*tsc_resp) + AUTHTAG_LEN;
 	req->exit_code = SVM_VMGEXIT_GUEST_REQUEST;
 
-	rc = snp_send_guest_request(mdesc, req, rio);
+	rc = snp_send_guest_request(mdesc, req);
 	if (rc)
 		goto e_request;
 
@@ -2068,8 +2060,6 @@ 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_rio:
-	kfree(rio);
 e_free_req:
 	kfree(req);
  e_free_tsc_resp:
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index cf3fb61f4d5b..b18504398619 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -101,7 +101,8 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 	req.resp_sz = resp_len;
 	req.exit_code = SVM_VMGEXIT_GUEST_REQUEST;
 
-	rc = snp_send_guest_request(mdesc, &req, arg);
+	rc = snp_send_guest_request(mdesc, &req);
+	arg->exitinfo2 = req.exitinfo2;
 	if (rc)
 		goto e_free;
 
@@ -152,7 +153,8 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
 	req.resp_sz = resp_len;
 	req.exit_code = SVM_VMGEXIT_GUEST_REQUEST;
 
-	rc = snp_send_guest_request(mdesc, &req, arg);
+	rc = snp_send_guest_request(mdesc, &req);
+	arg->exitinfo2 = req.exitinfo2;
 	if (rc)
 		return rc;
 
@@ -249,7 +251,8 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	req.resp_sz = resp_len;
 	req.exit_code = SVM_VMGEXIT_EXT_GUEST_REQUEST;
 
-	ret = snp_send_guest_request(mdesc, &req, arg);
+	ret = snp_send_guest_request(mdesc, &req);
+	arg->exitinfo2 = req.exitinfo2;
 
 	/* If certs length is invalid then copy the returned length */
 	if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
-- 
2.49.0

[PATCH 2/4] x86/sev: Allocate request in TSC_INFO_REQ on stack

[Alexey Kardashevskiy]

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

[PATCH 3/4] x86/sev: Document requirement for linear mapping of Guest Request buffers

[Alexey Kardashevskiy]

The Guest Request supports 3 types of messages now, the largest is
the extended variant of MSG_REPORT_REQ: sizeof(snp_ext_report_req)==112.
These used to be allocated on stack and then moved to the SNP guest
platform device (snp_guest_dev) for the reason explained in

commit db10cb9b5746 ("virt: sevguest: Fix passing a stack buffer as
a scatterlist target"):

aesgcm_encrypt() and aesgcm_decrypt() are used for guest messages and
might potentially use a crypto accelerator which requires DMA buffers
to be in the linear mapping.

Add a comment, pr_warn() and return -EINVAL when the buffers are not
in linear mapping.

Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---
Changes:
v4:
* ditched BUG_ON and ifdef
---
 arch/x86/coco/sev/core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 818ae7b1694b..0b714df62445 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1941,6 +1941,16 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req
 	u64 seqno;
 	int rc;
 
+	/*
+	 * enc_payload() calling aesgcm_encrypt() can potentially offload to HW
+	 * which involves DMA so the data to encrypt in the SG list has to be
+	 * in linear mapping.
+	 */
+	if (!virt_addr_valid(req->req_buf) || !virt_addr_valid(req->resp_buf)) {
+		pr_warn("AES-GSM buffers must be in linear mapping");
+		return -EINVAL;
+	}
+
 	guard(mutex)(&snp_cmd_mutex);
 
 	/* Check if the VMPCK is not empty */
-- 
2.49.0

[PATCH 4/4] x86/sev: Drop unnecessary parameter in snp_issue_guest_request

[Alexey Kardashevskiy]

Commit 3e385c0d6ce8 ("virt: sev-guest: Move SNP Guest Request data
pages handling under snp_cmd_mutex")

moved @input from snp_msg_desc to snp_guest_req which is passed to
snp_issue_guest_request().

Drop the extra parameter.

No functional change intended.

Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---
 arch/x86/coco/sev/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 0b714df62445..0334c40a5c8d 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1322,8 +1322,9 @@ int snp_issue_svsm_attest_req(u64 call_id, struct svsm_call *call,
 }
 EXPORT_SYMBOL_GPL(snp_issue_svsm_attest_req);
 
-static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_data *input)
+static int snp_issue_guest_request(struct snp_guest_req *req)
 {
+	struct snp_req_data *input = &req->input;
 	struct ghcb_state state;
 	struct es_em_ctxt ctxt;
 	unsigned long flags;
@@ -1865,7 +1866,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
 	 * sequence number must be incremented or the VMPCK must be deleted to
 	 * prevent reuse of the IV.
 	 */
-	rc = snp_issue_guest_request(req, &req->input);
+	rc = snp_issue_guest_request(req);
 	switch (rc) {
 	case -ENOSPC:
 		/*
-- 
2.49.0

Re: [PATCH 1/4] virt: sev-guest: Contain snp_guest_request_ioctl in sev-guest

[Dionna Amalie Glaze]

On Mon, May 5, 2025 at 7:13 AM Alexey Kardashevskiy <aik@amd.com> wrote:
>
> SNP Guest Request only uses exitinfo2 which is a return value from GHCB,
> has meaning beyond ioctl and therefore belongs to snp_guest_req.
>
> Move exitinfo2 to snp_guest_req, remove snp_guest_request_ioctl from
> the SEV platform code.
>
> 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)

Re: [PATCH 2/4] x86/sev: Allocate request in TSC_INFO_REQ on stack

[Dionna Amalie Glaze]

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)

Re: [PATCH 3/4] x86/sev: Document requirement for linear mapping of Guest Request buffers

[Dionna Amalie Glaze]

On Mon, May 5, 2025 at 7:14 AM Alexey Kardashevskiy <aik@amd.com> wrote:
>
> The Guest Request supports 3 types of messages now, the largest is
> the extended variant of MSG_REPORT_REQ: sizeof(snp_ext_report_req)==112.
> These used to be allocated on stack and then moved to the SNP guest
> platform device (snp_guest_dev) for the reason explained in
>
> commit db10cb9b5746 ("virt: sevguest: Fix passing a stack buffer as
> a scatterlist target"):
>
> aesgcm_encrypt() and aesgcm_decrypt() are used for guest messages and
> might potentially use a crypto accelerator which requires DMA buffers
> to be in the linear mapping.
>
> Add a comment, pr_warn() and return -EINVAL when the buffers are not
> in linear mapping.
>
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>

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

> ---
> Changes:
> v4:
> * ditched BUG_ON and ifdef
> ---
>  arch/x86/coco/sev/core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 818ae7b1694b..0b714df62445 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -1941,6 +1941,16 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req
>         u64 seqno;
>         int rc;
>
> +       /*
> +        * enc_payload() calling aesgcm_encrypt() can potentially offload to HW
> +        * which involves DMA so the data to encrypt in the SG list has to be
> +        * in linear mapping.
> +        */

The grammar here is a bit off. Would recommend

+       /*
+        * enc_payload() calls aesgcm_encrypt(), which can potentially
offload to HW.
+        * The offload's DMA SG list of data to encrypt has to be in
linear mapping.
+        */



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

Re: [PATCH 4/4] x86/sev: Drop unnecessary parameter in snp_issue_guest_request

[Dionna Amalie Glaze]

On Mon, May 5, 2025 at 7:14 AM Alexey Kardashevskiy <aik@amd.com> wrote:
>
> Commit 3e385c0d6ce8 ("virt: sev-guest: Move SNP Guest Request data
> pages handling under snp_cmd_mutex")
>
> moved @input from snp_msg_desc to snp_guest_req which is passed to
> snp_issue_guest_request().
>
> Drop the extra parameter.
>
> 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)

Re: [PATCH 2/4] x86/sev: Allocate request in TSC_INFO_REQ on stack

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

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.

[-- Attachment #2: snp_get_tsc_info-__free.patch --]
[-- Type: text/x-patch, Size: 2388 bytes --]

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index b0c1a7a57497..618eaae190e2 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -3174,41 +3174,32 @@ EXPORT_SYMBOL_GPL(snp_send_guest_request);
 
 static int __init snp_get_tsc_info(void)
 {
-	struct snp_guest_request_ioctl *rio;
-	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_request_ioctl *rio __free(kfree) = NULL;
+	struct snp_tsc_info_resp *tsc_resp  __free(kfree_sensitive) = NULL;
+	struct snp_tsc_info_req *tsc_req    __free(kfree) = NULL;
+	struct snp_guest_req *req	    __free(kfree) = NULL;
+	struct snp_msg_desc *mdesc	    __free(snp_msg_free) = NULL;
 	int rc = -ENOMEM;
 
-	tsc_req = kzalloc(sizeof(*tsc_req), GFP_KERNEL);
-	if (!tsc_req)
-		return rc;
-
 	/*
 	 * The intermediate response buffer is used while decrypting the
 	 * response payload. Make sure that it has enough space to cover
 	 * the authtag.
 	 */
 	tsc_resp = kzalloc(sizeof(*tsc_resp) + AUTHTAG_LEN, GFP_KERNEL);
-	if (!tsc_resp)
-		goto e_free_tsc_req;
-
+	tsc_req = kzalloc(sizeof(*tsc_req), GFP_KERNEL);
 	req = kzalloc(sizeof(*req), GFP_KERNEL);
-	if (!req)
-		goto e_free_tsc_resp;
-
 	rio = kzalloc(sizeof(*rio), GFP_KERNEL);
-	if (!rio)
-		goto e_free_req;
+	if (!(tsc_resp && tsc_req && req && rio))
+		return rc;
 
 	mdesc = snp_msg_alloc();
 	if (IS_ERR_OR_NULL(mdesc))
-		goto e_free_rio;
+		return rc;
 
 	rc = snp_msg_init(mdesc, snp_vmpl);
 	if (rc)
-		goto e_free_mdesc;
+		return rc;
 
 	req->msg_version = MSG_HDR_VER;
 	req->msg_type = SNP_MSG_TSC_INFO_REQ;
@@ -3221,7 +3212,7 @@ static int __init snp_get_tsc_info(void)
 
 	rc = snp_send_guest_request(mdesc, req, rio);
 	if (rc)
-		goto e_request;
+		return rc;
 
 	pr_debug("%s: response status 0x%x scale 0x%llx offset 0x%llx factor 0x%x\n",
 		 __func__, tsc_resp->status, tsc_resp->tsc_scale, tsc_resp->tsc_offset,
@@ -3235,20 +3226,6 @@ static int __init snp_get_tsc_info(void)
 		rc = -EIO;
 	}
 
-e_request:
-	/* The response buffer contains sensitive data, explicitly clear it. */
-	memzero_explicit(tsc_resp, sizeof(*tsc_resp) + AUTHTAG_LEN);
-e_free_mdesc:
-	snp_msg_free(mdesc);
-e_free_rio:
-	kfree(rio);
-e_free_req:
-	kfree(req);
- e_free_tsc_resp:
-	kfree(tsc_resp);
-e_free_tsc_req:
-	kfree(tsc_req);
-
 	return rc;
 }
 

Re: [PATCH 2/4] x86/sev: Allocate request in TSC_INFO_REQ on stack

[Alexey Kardashevskiy]

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

Re: [PATCH 0/4] x86/sev: Rework SNP Guest Request

[Tom Lendacky]

On 5/5/25 09:12, Alexey Kardashevskiy wrote:
> Started as a part of the fix for
> ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex")
> 
> these are the remainder of followup fixes/improvements as
> the SNP Guest Request code evolved enough to make it a bit hard to
> follow.
> 
> 
> This is based on sha1
> 6c201ee95a30 Ingo Molnar "Merge branch into tip/master: 'x86/sgx'".
> 
> Please comment. Thanks.
> 
> 
> 
> Alexey Kardashevskiy (4):
>   virt: sev-guest: Contain snp_guest_request_ioctl in sev-guest
>   x86/sev: Allocate request in TSC_INFO_REQ on stack
>   x86/sev: Document requirement for linear mapping of Guest Request
>     buffers
>   x86/sev: Drop unnecessary parameter in snp_issue_guest_request
> 
>  arch/x86/include/asm/sev.h              | 10 ++-
>  arch/x86/coco/sev/core.c                | 73 +++++++++-----------
>  drivers/virt/coco/sev-guest/sev-guest.c |  9 ++-
>  3 files changed, 44 insertions(+), 48 deletions(-)

For the series:

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> 

Re: [PATCH 0/4] x86/sev: Rework SNP Guest Request

[Alexey Kardashevskiy]

On 7/5/25 04:55, Tom Lendacky wrote:
> On 5/5/25 09:12, Alexey Kardashevskiy wrote:
>> Started as a part of the fix for
>> ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex")
>>
>> these are the remainder of followup fixes/improvements as
>> the SNP Guest Request code evolved enough to make it a bit hard to
>> follow.
>>
>>
>> This is based on sha1
>> 6c201ee95a30 Ingo Molnar "Merge branch into tip/master: 'x86/sgx'".
>>
>> Please comment. Thanks.
>>
>>
>>
>> Alexey Kardashevskiy (4):
>>    virt: sev-guest: Contain snp_guest_request_ioctl in sev-guest
>>    x86/sev: Allocate request in TSC_INFO_REQ on stack
>>    x86/sev: Document requirement for linear mapping of Guest Request
>>      buffers
>>    x86/sev: Drop unnecessary parameter in snp_issue_guest_request
>>
>>   arch/x86/include/asm/sev.h              | 10 ++-
>>   arch/x86/coco/sev/core.c                | 73 +++++++++-----------
>>   drivers/virt/coco/sev-guest/sev-guest.c |  9 ++-
>>   3 files changed, 44 insertions(+), 48 deletions(-)
> 
> For the series:
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>


Ping?

> 
>>

-- 
Alexey

Re: [PATCH 0/4] x86/sev: Rework SNP Guest Request

[Borislav Petkov]

On Thu, Jun 05, 2025 at 12:40:15PM +1000, Alexey Kardashevskiy wrote:
> Ping?

Yes, please send a new version collecting all feedback and rebased on
6.16-rc1 after it releases next week.

Thx.