Re: [PATCH v12 2/3] x86/sev: Change snp_guest_issue_request's fw_err

[Borislav Petkov]

On Fri, Jan 20, 2023 at 09:48:55PM +0000, Dionna Glaze wrote:
> Since the "fw_err" field is really exitinfo2 split into the upper bits'
> vmm error code and lower bits' firmware error code, sev-guest.h is
> updated to represent the 64 bit value as a union.

Yah, documentation needs update too:

diff --git a/Documentation/virt/coco/sev-guest.rst b/Documentation/virt/coco/sev-guest.rst
index 4f0dcc1d16e8..4f9df24b829f 100644
--- a/Documentation/virt/coco/sev-guest.rst
+++ b/Documentation/virt/coco/sev-guest.rst
@@ -57,9 +57,16 @@ counter (e.g. counter overflow), then -EIO will be returned.
                 __u64 req_data;
                 __u64 resp_data;
 
-                /* firmware error code on failure (see psp-sev.h) */
-                __u64 fw_err;
-        };
+                /* bits[63:32]: VMM error code, bits[31:0] firmware error code (see psp-sev.h) */
+                union {
+                        __u64 exitinfo2;
+                        struct {
+                                __u32 fw_error;
+                                __u32 vmm_error;
+                        };
+                };
+	};
+
 
 2.1 SNP_GET_REPORT
 ------------------

/me goes and looks at patch 3...

Yap, that change adding the union should all belong together in a single patch.

> @@ -366,24 +367,22 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
>  		 * of the VMPCK and the error code being propagated back to the
>  		 * user as an ioctl() return code.
>  		 */
> -		rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err);
> +		rc = snp_issue_guest_request(exit_code, &snp_dev->input,
> +					     &arg->exitinfo2);
>  
>  		/*
>  		 * Override the error to inform callers the given extended
>  		 * request buffer size was too small and give the caller the
>  		 * required buffer size.
>  		 */
> -		err = SNP_GUEST_REQ_INVALID_LEN;
> +		arg->vmm_error = SNP_GUEST_VMM_ERR_INVALID_LEN;
>  		snp_dev->input.data_npages = certs_npages;
>  	}
>  
> -	if (fw_err)
> -		*fw_err = err;
> -
>  	if (rc) {
>  		dev_alert(snp_dev->dev,
> -			  "Detected error from ASP request. rc: %d, fw_err: %llu\n",
> -			  rc, *fw_err);
> +			  "Detected error from ASP request. rc: %d, exitinfo2: %llu\n",
> +			  rc, arg->exitinfo2);

I guess that's better off dumped in binary now:

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index d4973d2dbc24..a5d6ea3eebe9 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -380,7 +380,7 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
 
 	if (rc) {
 		dev_alert(snp_dev->dev,
-			  "Detected error from ASP request. rc: %d, exitinfo2: %llu\n",
+			  "Detected error from ASP request. rc: %d, exitinfo2: 0x%llx\n",
 			  rc, arg->exitinfo2);
 		goto disable_vmpck;
 	}

Anyway, that's a lot of changes for a fix which needs to go to stable. I don't
mind them but not in a minimal fix.

So how is this for a minimal fix to go in now, ontop of your first patch? The
cleanups can then go later...

---
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 4ec4174e05a3..20b560a45bc1 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -322,7 +322,7 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
 				u8 type, void *req_buf, size_t req_sz, void *resp_buf,
 				u32 resp_sz, __u64 *fw_err)
 {
-	unsigned long err;
+	unsigned long err = SEV_RET_NO_FW_CALL;
 	u64 seqno;
 	int rc;
 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette