Re: [PATCH v12 05/19] virt: sev-guest: Reduce the scope of SNP command mutex

[Tom Lendacky]

On 10/9/24 04:28, Nikunj A Dadhania wrote:
> The SNP command mutex is used to serialize access to the shared buffer,
> command handling, and message sequence number.
> 
> All shared buffer, command handling, and message sequence updates are done
> within snp_send_guest_request(), so moving the mutex to this function is
> appropriate and maintains the critical section.
> 
> Since the mutex is now taken at a later point in time, remove the lockdep
> checks that occur before taking the mutex.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>

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

> ---
>  drivers/virt/coco/sev-guest/sev-guest.c | 35 ++++++-------------------
>  1 file changed, 8 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 2a1b542168b1..1bddef822446 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -345,6 +345,14 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
>  	u64 seqno;
>  	int rc;
>  
> +	guard(mutex)(&snp_cmd_mutex);
> +
> +	/* Check if the VMPCK is not empty */
> +	if (is_vmpck_empty(snp_dev)) {
> +		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> +		return -ENOTTY;
> +	}
> +
>  	/* Get message sequence and verify that its a non-zero */
>  	seqno = snp_get_msg_seqno(snp_dev);
>  	if (!seqno)
> @@ -401,8 +409,6 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
>  	struct snp_guest_req req = {};
>  	int rc, resp_len;
>  
> -	lockdep_assert_held(&snp_cmd_mutex);
> -
>  	if (!arg->req_data || !arg->resp_data)
>  		return -EINVAL;
>  
> @@ -449,8 +455,6 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
>  	/* Response data is 64 bytes and max authsize for GCM is 16 bytes. */
>  	u8 buf[64 + 16];
>  
> -	lockdep_assert_held(&snp_cmd_mutex);
> -
>  	if (!arg->req_data || !arg->resp_data)
>  		return -EINVAL;
>  
> @@ -501,8 +505,6 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
>  	int ret, npages = 0, resp_len;
>  	sockptr_t certs_address;
>  
> -	lockdep_assert_held(&snp_cmd_mutex);
> -
>  	if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data))
>  		return -EINVAL;
>  
> @@ -598,15 +600,6 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
>  	if (!input.msg_version)
>  		return -EINVAL;
>  
> -	mutex_lock(&snp_cmd_mutex);
> -
> -	/* Check if the VMPCK is not empty */
> -	if (is_vmpck_empty(snp_dev)) {
> -		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> -		mutex_unlock(&snp_cmd_mutex);
> -		return -ENOTTY;
> -	}
> -
>  	switch (ioctl) {
>  	case SNP_GET_REPORT:
>  		ret = get_report(snp_dev, &input);
> @@ -628,8 +621,6 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
>  		break;
>  	}
>  
> -	mutex_unlock(&snp_cmd_mutex);
> -
>  	if (input.exitinfo2 && copy_to_user(argp, &input, sizeof(input)))
>  		return -EFAULT;
>  
> @@ -744,8 +735,6 @@ static int sev_svsm_report_new(struct tsm_report *report, void *data)
>  	man_len = SZ_4K;
>  	certs_len = SEV_FW_BLOB_MAX_SIZE;
>  
> -	guard(mutex)(&snp_cmd_mutex);
> -
>  	if (guid_is_null(&desc->service_guid)) {
>  		call_id = SVSM_ATTEST_CALL(SVSM_ATTEST_SERVICES);
>  	} else {
> @@ -880,14 +869,6 @@ static int sev_report_new(struct tsm_report *report, void *data)
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	guard(mutex)(&snp_cmd_mutex);
> -
> -	/* Check if the VMPCK is not empty */
> -	if (is_vmpck_empty(snp_dev)) {
> -		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> -		return -ENOTTY;
> -	}
> -
>  	cert_table = buf + report_size;
>  	struct snp_ext_report_req ext_req = {
>  		.data = { .vmpl = desc->privlevel },