Re: [PATCH v3 04/14] virt: sev-guest: Add SNP guest request structure

From: Tom Lendacky
Date: Tue Aug 01 2023 - 11:49:53 EST


On 7/22/23 06:18, Nikunj A Dadhania wrote:
Add a snp_guest_req structure to simplify the function arguments. The
structure will be used to call the SNP Guest message request API
instead of passing a long list of parameters.

Add two helper functions for filling up the parameters:
handle_guest_request() and handle_guest_request_ext(). GET_EXT_REPORT
queries for certs_data from the AMD Security processor.
handle_guest_request_ext() provides those extra parameters for
receiving certs_data from the AMD security processor.

Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
---
.../x86/include/asm}/sev-guest.h | 11 ++
arch/x86/include/asm/sev.h | 7 --
arch/x86/kernel/sev.c | 15 ++-
drivers/virt/coco/sev-guest/sev-guest.c | 107 ++++++++++++------
4 files changed, 93 insertions(+), 47 deletions(-)
rename {drivers/virt/coco/sev-guest => arch/x86/include/asm}/sev-guest.h (80%)


@@ -398,6 +393,46 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
return 0;
}
+static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
+ struct snp_guest_request_ioctl *rio, u8 type,
+ void *req_buf, size_t req_sz, void *resp_buf,
+ u32 resp_sz)
+{
+ struct snp_guest_req guest_req = {
+ .msg_version = rio->msg_version,
+ .msg_type = type,
+ .vmpck_id = vmpck_id,
+ .req_buf = req_buf,
+ .req_sz = req_sz,
+ .resp_buf = resp_buf,
+ .resp_sz = resp_sz,
+ .exit_code = exit_code,
+ };
+
+ return snp_send_guest_request(snp_dev, &guest_req, rio);
+}
+
+static int handle_guest_request_ext(struct snp_guest_dev *snp_dev, u64 exit_code,
+ struct snp_guest_request_ioctl *rio, u8 type,
+ void *req_buf, size_t req_sz, void *resp_buf,
+ u32 resp_sz, void *certs_data, size_t *npages)
+{
+ struct snp_guest_req guest_req = {
+ .msg_version = rio->msg_version,
+ .msg_type = type,
+ .vmpck_id = vmpck_id,
+ .req_buf = req_buf,
+ .req_sz = req_sz,
+ .resp_buf = resp_buf,
+ .resp_sz = resp_sz,
+ .exit_code = exit_code,
+ .data = certs_data,
+ .data_npages = npages,
+ };
+
+ return snp_send_guest_request(snp_dev, &guest_req, rio);
+}

I'm not sure these intermediate funcitons are really necessary. Can't you create/build the struct in get_report() and get_ext_report() and then just call snp_send_guest_request() directly from those functions?

Thanks,
Tom

+
static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
{
struct snp_report_resp *resp;
@@ -480,7 +515,8 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
{
struct snp_ext_report_req req;
struct snp_report_resp *resp;
- int ret, npages = 0, resp_len;
+ int ret, resp_len;
+ size_t npages = 0;
lockdep_assert_held(&snp_dev->cmd_mutex);
@@ -520,14 +556,14 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
if (!resp)
return -ENOMEM;
- snp_dev->input.data_npages = npages;
- ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg,
- SNP_MSG_REPORT_REQ, &req.data,
- sizeof(req.data), resp->data, resp_len);
+ ret = handle_guest_request_ext(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST,
+ arg, SNP_MSG_REPORT_REQ, &req.data,
+ sizeof(req.data), resp->data, resp_len,
+ snp_dev->certs_data, &npages);
/* If certs length is invalid then copy the returned length */
if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
- req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
+ req.certs_len = npages << PAGE_SHIFT;
if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
ret = -EFAULT;