On 7/31/24 10:08, 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>
> ---
> drivers/virt/coco/sev-guest/sev-guest.c | 17 ++---------------
> 1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 92734a2345a6..42f7126f1718 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -345,6 +345,8 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
> u64 seqno;
> int rc;
>
> + guard(mutex)(&snp_cmd_mutex);
> +
> /* Get message sequence and verify that its a non-zero */
> seqno = snp_get_msg_seqno(snp_dev);
> if (!seqno)
> @@ -419,8 +421,6 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
> struct snp_report_resp *report_resp;
> int rc, resp_len;
>
> - lockdep_assert_held(&snp_cmd_mutex);
> -
> if (!arg->req_data || !arg->resp_data)
> return -EINVAL;
>
> @@ -458,8 +458,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 +499,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;
>
> @@ -590,12 +586,9 @@ 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)) {
Are we ok with this being outside of the lock now?
I believe is_vmpck_empty() can get a false and then be waiting on the
mutex while snp_disable_vmpck() is called. Suddenly the code thinks the
VMPCK is valid when it isn't anymore. Not sure if that matters, as the
guest request will fail anyway?
Thanks,
Tom
> dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> - mutex_unlock(&snp_cmd_mutex);
> return -ENOTTY;
> }
>
> @@ -620,8 +613,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;
>
> @@ -736,8 +727,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 {
> @@ -872,8 +861,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");
