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

[Tom Lendacky <thomas.lendacky@amd.com>]

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@amd.com>
> ---
>   .../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;