[kernel] KVM: SVM: Fix SVM_VMGEXIT_EXT_GUEST_REQUEST to follow the rest of API

  When SVM VM is up, KVM uses sev_issue_cmd_external_user() with an open
/dev/sev fd which ensures that the SVM initialization was done correctly.
The only helper not following the scheme is snp_guest_ext_guest_request()
which bypasses the fd check.

Change the SEV API to require passing a file.

Handle errors with care in the SNP Extended Guest Request handler
(snp_handle_ext_guest_request()) as there are actually 3 types of errors:
- @rc: return code SEV device's sev_issue_cmd() which is int==int32;
- @err: a psp return code in sev_issue_cmd(), also int==int32 (probably
a mistake but kvm_sev_cmd::error uses __u32 for some time now);
- (added by this) @exitcode: GHCB's exit code sw_exit_info_2, uint64.

Use the right types, remove cast to int* and return ENOSPC from SEV
device for converting it to the GHCB's exit code
SNP_GUEST_REQ_INVALID_LEN==BIT(32).

Fixes: 17f1d0c995ac ("KVM: SVM: Provide support for SNP_GUEST_REQUEST NAE event")
While at this, preserve the original error in snp_cleanup_guest_buf().

Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---

This can easily be squashed into what it fixes.

The patch is made for
https://github.com/AMDESE/linux/commits/upmv10-host-snp-v7-rfc
---
 include/linux/psp-sev.h      | 62 +++++++++++---------
 arch/x86/kvm/svm/sev.c       | 50 +++++++++++-----
 drivers/crypto/ccp/sev-dev.c | 11 ++--
 3 files changed, 73 insertions(+), 50 deletions(-)
  

On 2/5/2023 9:13 PM, Alexey Kardashevskiy wrote:
> When SVM VM is up, KVM uses sev_issue_cmd_external_user() with an open
> /dev/sev fd which ensures that the SVM initialization was done correctly.
> The only helper not following the scheme is snp_guest_ext_guest_request()
> which bypasses the fd check.
> 
> Change the SEV API to require passing a file.
> 
> Handle errors with care in the SNP Extended Guest Request handler
> (snp_handle_ext_guest_request()) as there are actually 3 types of errors:
> - @rc: return code SEV device's sev_issue_cmd() which is int==int32;
> - @err: a psp return code in sev_issue_cmd(), also int==int32 (probably
> a mistake but kvm_sev_cmd::error uses __u32 for some time now);
> - (added by this) @exitcode: GHCB's exit code sw_exit_info_2, uint64.
> 
> Use the right types, remove cast to int* and return ENOSPC from SEV
> device for converting it to the GHCB's exit code
> SNP_GUEST_REQ_INVALID_LEN==BIT(32).
> 
> Fixes: 17f1d0c995ac ("KVM: SVM: Provide support for SNP_GUEST_REQUEST NAE event")
> While at this, preserve the original error in snp_cleanup_guest_buf().
> 
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> ---
> 
> This can easily be squashed into what it fixes.
> 
> The patch is made for
> https://github.com/AMDESE/linux/commits/upmv10-host-snp-v7-rfc
> ---
>   include/linux/psp-sev.h      | 62 +++++++++++---------
>   arch/x86/kvm/svm/sev.c       | 50 +++++++++++-----
>   drivers/crypto/ccp/sev-dev.c | 11 ++--
>   3 files changed, 73 insertions(+), 50 deletions(-)
> 
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 970a9de0ed20..466b1a6e7d7b 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -848,6 +848,36 @@ int sev_platform_status(struct sev_user_data_status *status, int *error);
>   int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
>   				void *data, int *error);
>   
> +/**
> + * sev_issue_cmd_external_user_cert - issue SEV command by other driver with a file
> + * handle and return certificates set onto SEV device via SNP_SET_EXT_CONFIG;
> + * intended for use by the SNP extended guest request command defined
> + * in the GHCB specification.
> + *
> + * @filep - SEV device file pointer
> + * @cmd - command to issue
> + * @data - command buffer
> + * @vaddr: address where the certificate blob need to be copied.
> + * @npages: number of pages for the certificate blob.
> + *    If the specified page count is less than the certificate blob size, then the
> + *    required page count is returned with ENOSPC error code.
> + *    If the specified page count is more than the certificate blob size, then
> + *    page count is updated to reflect the amount of valid data copied in the
> + *    vaddr.
> + *
> + * @error: SEV command return code
> + *
> + * Returns:
> + * 0 if the sev successfully processed the command
> + * -%ENODEV    if the sev device is not available
> + * -%ENOTSUPP  if the sev does not support SEV
> + * -%ETIMEDOUT if the sev command timed out
> + * -%EIO       if the sev returned a non-zero return code
> + * -%ENOSPC    if the specified page count is too small
> + */
> +int sev_issue_cmd_external_user_cert(struct file *filep, unsigned int cmd, void *data,
> +				     unsigned long vaddr, unsigned long *npages, int *error);
> +
>   /**
>    * sev_guest_deactivate - perform SEV DEACTIVATE command
>    *
> @@ -945,32 +975,6 @@ void snp_free_firmware_page(void *addr);
>    */
>   void snp_mark_pages_offline(unsigned long pfn, unsigned int npages);
>   
> -/**
> - * snp_guest_ext_guest_request - perform the SNP extended guest request command
> - *  defined in the GHCB specification.
> - *
> - * @data: the input guest request structure
> - * @vaddr: address where the certificate blob need to be copied.
> - * @npages: number of pages for the certificate blob.
> - *    If the specified page count is less than the certificate blob size, then the
> - *    required page count is returned with error code defined in the GHCB spec.
> - *    If the specified page count is more than the certificate blob size, then
> - *    page count is updated to reflect the amount of valid data copied in the
> - *    vaddr.
> - *
> - * @sev_ret: sev command return code
> - *
> - * Returns:
> - * 0 if the sev successfully processed the command
> - * -%ENODEV    if the sev device is not available
> - * -%ENOTSUPP  if the sev does not support SEV
> - * -%ETIMEDOUT if the sev command timed out
> - * -%EIO       if the sev returned a non-zero return code
> - */
> -int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
> -				unsigned long vaddr, unsigned long *npages,
> -				unsigned long *error);
> -
>   #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
>   
>   static inline int
> @@ -1013,9 +1017,9 @@ static inline void *snp_alloc_firmware_page(gfp_t mask)
>   
>   static inline void snp_free_firmware_page(void *addr) { }
>   
> -static inline int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
> -					      unsigned long vaddr, unsigned long *n,
> -					      unsigned long *error)
> +static inline int sev_issue_cmd_external_user_cert(struct file *filep, unsigned int cmd,
> +						   void *data, unsigned long vaddr,
> +						   unsigned long *npages, int *error)
>   {
>   	return -ENODEV;
>   }
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index d0e58cffd1ed..b268c35efab4 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -394,6 +394,23 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
>   	return __sev_issue_cmd(sev->fd, id, data, error);
>   }
>   
> +static int sev_issue_cmd_cert(struct kvm *kvm, int id, void *data,
> +			      unsigned long vaddr, unsigned long *npages, int *error)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	struct fd f;
> +	int ret;
> +
> +	f = fdget(sev->fd);
> +	if (!f.file)
> +		return -EBADF;
> +
> +	ret = sev_issue_cmd_external_user_cert(f.file, id, data, vaddr, npages, error);
> +
> +	fdput(f);
> +	return ret;
> +}
> +
>   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   {
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> @@ -3587,11 +3604,11 @@ static void snp_cleanup_guest_buf(struct sev_data_snp_guest_request *data, unsig
>   	int ret;
>   
>   	ret = snp_page_reclaim(pfn);
> -	if (ret)
> +	if (ret && (*rc == SEV_RET_SUCCESS))
>   		*rc = SEV_RET_INVALID_ADDRESS;
>   
>   	ret = rmp_make_shared(pfn, PG_LEVEL_4K);
> -	if (ret)
> +	if (ret && (*rc == SEV_RET_SUCCESS))
>   		*rc = SEV_RET_INVALID_ADDRESS;
>   }

I believe we need to fix this as per the GHCB specifications.

As per GHCB 2.0 specifications:

SW_EXITINFO2
...
State from Hypervisor: Upper
32-bits (63:32) will contain the
return code from the hypervisor.
Lower 32-bits (31:0) will contain
the return code from the firmware
call (0 = success)

So i believe the FW error code (which is the FW error code from 
SNP_GUEST_REQUEST or *rc here) should be contained in the lower 32-bits 
and the error code being returned back due to response buffer pages 
reclaim failure and/or failure to transisition these pages back to 
shared state is basically hypervisor (error) return code and that should 
be returned in the upper 32-bit of the exitinfo.

There is work in progress to check conformance of SNP v7 patches to GHCB 
2.0 specifications, so probably this fix can be included as part of 
those patches.

>   
> @@ -3638,8 +3655,9 @@ static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
>   	struct kvm *kvm = vcpu->kvm;
>   	unsigned long data_npages;
>   	struct kvm_sev_info *sev;
> -	unsigned long rc, err;

This needs to be looked at more carefully. The SEV firmware status code 
is defined as 32-bit, but is being handled as unsigned long in the 
KVM/SNP code and as int in the CCP driver. So this needs to be fixed 
consistently across, snp_setup_guest_buf() return value will need to be 
fixed accordingly.

> +	unsigned long exitcode;
>   	u64 data_gpa;
> +	int err, rc;
>   
>   	if (!sev_snp_guest(vcpu->kvm)) {
>   		rc = SEV_RET_INVALID_GUEST;
> @@ -3669,17 +3687,16 @@ static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
>   	 */
>   	if (sev->snp_certs_len) {
>   		if ((data_npages << PAGE_SHIFT) < sev->snp_certs_len) {
> -			rc = -EINVAL;
> -			err = SNP_GUEST_REQ_INVALID_LEN;
> +			rc = -ENOSPC;

Why do we need to introduce ENOSPC error code?

If we continue to use SNP_GUEST_REQ_INVALID_LEN we don't need to map 
ENOSPC to SNP_GUEST_REQ_INVALID_LEN below.

And the CCP driver can return SNP_GUEST_REQ_INVALID_LEN as earlier via 
the fw_err parameter.

Thanks,
Ashish

>   			goto datalen;
>   		}
> -		rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
> -				   (int *)&err);
> +		rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req, &err);
>   	} else {
> -		rc = snp_guest_ext_guest_request(&req,
> -						 (unsigned long)sev->snp_certs_data,
> -						 &data_npages, &err);
> +		rc = sev_issue_cmd_cert(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
> +					(unsigned long)sev->snp_certs_data,
> +					&data_npages, &err);
>   	}
> +
>   datalen:
>   	if (sev->snp_certs_len)
>   		data_npages = sev->snp_certs_len >> PAGE_SHIFT;
> @@ -3689,27 +3706,30 @@ static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
>   		 * If buffer length is small then return the expected
>   		 * length in rbx.
>   		 */
> -		if (err == SNP_GUEST_REQ_INVALID_LEN)
> +		if (rc == -ENOSPC) {
>   			vcpu->arch.regs[VCPU_REGS_RBX] = data_npages;
> +			exitcode = SNP_GUEST_REQ_INVALID_LEN;
> +			goto cleanup;
> +		}
>   
>   		/* pass the firmware error code */
> -		rc = err;
> +		exitcode = err;
>   		goto cleanup;
>   	}
>   
>   	/* Copy the certificate blob in the guest memory */
>   	if (data_npages &&
>   	    kvm_write_guest(kvm, data_gpa, sev->snp_certs_data, data_npages << PAGE_SHIFT))
> -		rc = SEV_RET_INVALID_ADDRESS;
> +		exitcode = SEV_RET_INVALID_ADDRESS;
>   
>   cleanup:
> -	snp_cleanup_guest_buf(&req, &rc);
> +	snp_cleanup_guest_buf(&req, &exitcode);
>   
>   unlock:
>   	mutex_unlock(&sev->guest_req_lock);
>   
>   e_fail:
> -	svm_set_ghcb_sw_exit_info_2(vcpu, rc);
> +	svm_set_ghcb_sw_exit_info_2(vcpu, exitcode);
>   }
>   
>   static kvm_pfn_t gfn_to_pfn_restricted(struct kvm *kvm, gfn_t gfn)
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 6c4fdcaed72b..73f56c20255c 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -2070,8 +2070,8 @@ int snp_guest_dbg_decrypt_page(u64 gctx_pfn, u64 src_pfn, u64 dst_pfn, int *erro
>   }
>   EXPORT_SYMBOL_GPL(snp_guest_dbg_decrypt_page);
>   
> -int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
> -				unsigned long vaddr, unsigned long *npages, unsigned long *fw_err)
> +int sev_issue_cmd_external_user_cert(struct file *filep, unsigned int cmd, void *data,
> +				     unsigned long vaddr, unsigned long *npages, int *error)
>   {
>   	unsigned long expected_npages;
>   	struct sev_device *sev;
> @@ -2093,12 +2093,11 @@ int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
>   	expected_npages = sev->snp_certs_len >> PAGE_SHIFT;
>   	if (*npages < expected_npages) {
>   		*npages = expected_npages;
> -		*fw_err = SNP_GUEST_REQ_INVALID_LEN;
>   		mutex_unlock(&sev->snp_certs_lock);
> -		return -EINVAL;
> +		return -ENOSPC;
>   	}
>   
> -	rc = sev_do_cmd(SEV_CMD_SNP_GUEST_REQUEST, data, (int *)fw_err);
> +	rc = sev_issue_cmd_external_user(filep, cmd, data, error);
>   	if (rc) {
>   		mutex_unlock(&sev->snp_certs_lock);
>   		return rc;
> @@ -2115,7 +2114,7 @@ int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
>   	mutex_unlock(&sev->snp_certs_lock);
>   	return rc;
>   }
> -EXPORT_SYMBOL_GPL(snp_guest_ext_guest_request);
> +EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user_cert);
>   
>   static void sev_exit(struct kref *ref)
>   {
>
  

On 07/02/2023 08:57, Kalra, Ashish wrote:
> On 2/5/2023 9:13 PM, Alexey Kardashevskiy wrote:
>> When SVM VM is up, KVM uses sev_issue_cmd_external_user() with an open
>> /dev/sev fd which ensures that the SVM initialization was done correctly.
>> The only helper not following the scheme is snp_guest_ext_guest_request()
>> which bypasses the fd check.
>>
>> Change the SEV API to require passing a file.
>>
>> Handle errors with care in the SNP Extended Guest Request handler
>> (snp_handle_ext_guest_request()) as there are actually 3 types of errors:
>> - @rc: return code SEV device's sev_issue_cmd() which is int==int32;
>> - @err: a psp return code in sev_issue_cmd(), also int==int32 (probably
>> a mistake but kvm_sev_cmd::error uses __u32 for some time now);
>> - (added by this) @exitcode: GHCB's exit code sw_exit_info_2, uint64.
>>
>> Use the right types, remove cast to int* and return ENOSPC from SEV
>> device for converting it to the GHCB's exit code
>> SNP_GUEST_REQ_INVALID_LEN==BIT(32).
>>
>> Fixes: 17f1d0c995ac ("KVM: SVM: Provide support for SNP_GUEST_REQUEST 
>> NAE event")
>> While at this, preserve the original error in snp_cleanup_guest_buf().
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>> ---
>>
>> This can easily be squashed into what it fixes.
>>
>> The patch is made for
>> https://github.com/AMDESE/linux/commits/upmv10-host-snp-v7-rfc
>> ---
>>   include/linux/psp-sev.h      | 62 +++++++++++---------
>>   arch/x86/kvm/svm/sev.c       | 50 +++++++++++-----
>>   drivers/crypto/ccp/sev-dev.c | 11 ++--
>>   3 files changed, 73 insertions(+), 50 deletions(-)
>>
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index 970a9de0ed20..466b1a6e7d7b 100644
>> --- a/include/linux/psp-sev.h
>> +++ b/include/linux/psp-sev.h
>> @@ -848,6 +848,36 @@ int sev_platform_status(struct 
>> sev_user_data_status *status, int *error);
>>   int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
>>                   void *data, int *error);
>> +/**
>> + * sev_issue_cmd_external_user_cert - issue SEV command by other 
>> driver with a file
>> + * handle and return certificates set onto SEV device via 
>> SNP_SET_EXT_CONFIG;
>> + * intended for use by the SNP extended guest request command defined
>> + * in the GHCB specification.
>> + *
>> + * @filep - SEV device file pointer
>> + * @cmd - command to issue
>> + * @data - command buffer
>> + * @vaddr: address where the certificate blob need to be copied.
>> + * @npages: number of pages for the certificate blob.
>> + *    If the specified page count is less than the certificate blob 
>> size, then the
>> + *    required page count is returned with ENOSPC error code.
>> + *    If the specified page count is more than the certificate blob 
>> size, then
>> + *    page count is updated to reflect the amount of valid data 
>> copied in the
>> + *    vaddr.
>> + *
>> + * @error: SEV command return code
>> + *
>> + * Returns:
>> + * 0 if the sev successfully processed the command
>> + * -%ENODEV    if the sev device is not available
>> + * -%ENOTSUPP  if the sev does not support SEV
>> + * -%ETIMEDOUT if the sev command timed out
>> + * -%EIO       if the sev returned a non-zero return code
>> + * -%ENOSPC    if the specified page count is too small
>> + */
>> +int sev_issue_cmd_external_user_cert(struct file *filep, unsigned int 
>> cmd, void *data,
>> +                     unsigned long vaddr, unsigned long *npages, int 
>> *error);
>> +
>>   /**
>>    * sev_guest_deactivate - perform SEV DEACTIVATE command
>>    *
>> @@ -945,32 +975,6 @@ void snp_free_firmware_page(void *addr);
>>    */
>>   void snp_mark_pages_offline(unsigned long pfn, unsigned int npages);
>> -/**
>> - * snp_guest_ext_guest_request - perform the SNP extended guest 
>> request command
>> - *  defined in the GHCB specification.
>> - *
>> - * @data: the input guest request structure
>> - * @vaddr: address where the certificate blob need to be copied.
>> - * @npages: number of pages for the certificate blob.
>> - *    If the specified page count is less than the certificate blob 
>> size, then the
>> - *    required page count is returned with error code defined in the 
>> GHCB spec.
>> - *    If the specified page count is more than the certificate blob 
>> size, then
>> - *    page count is updated to reflect the amount of valid data 
>> copied in the
>> - *    vaddr.
>> - *
>> - * @sev_ret: sev command return code
>> - *
>> - * Returns:
>> - * 0 if the sev successfully processed the command
>> - * -%ENODEV    if the sev device is not available
>> - * -%ENOTSUPP  if the sev does not support SEV
>> - * -%ETIMEDOUT if the sev command timed out
>> - * -%EIO       if the sev returned a non-zero return code
>> - */
>> -int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
>> -                unsigned long vaddr, unsigned long *npages,
>> -                unsigned long *error);
>> -
>>   #else    /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>   static inline int
>> @@ -1013,9 +1017,9 @@ static inline void 
>> *snp_alloc_firmware_page(gfp_t mask)
>>   static inline void snp_free_firmware_page(void *addr) { }
>> -static inline int snp_guest_ext_guest_request(struct 
>> sev_data_snp_guest_request *data,
>> -                          unsigned long vaddr, unsigned long *n,
>> -                          unsigned long *error)
>> +static inline int sev_issue_cmd_external_user_cert(struct file 
>> *filep, unsigned int cmd,
>> +                           void *data, unsigned long vaddr,
>> +                           unsigned long *npages, int *error)
>>   {
>>       return -ENODEV;
>>   }
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index d0e58cffd1ed..b268c35efab4 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -394,6 +394,23 @@ static int sev_issue_cmd(struct kvm *kvm, int id, 
>> void *data, int *error)
>>       return __sev_issue_cmd(sev->fd, id, data, error);
>>   }
>> +static int sev_issue_cmd_cert(struct kvm *kvm, int id, void *data,
>> +                  unsigned long vaddr, unsigned long *npages, int 
>> *error)
>> +{
>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +    struct fd f;
>> +    int ret;
>> +
>> +    f = fdget(sev->fd);
>> +    if (!f.file)
>> +        return -EBADF;
>> +
>> +    ret = sev_issue_cmd_external_user_cert(f.file, id, data, vaddr, 
>> npages, error);
>> +
>> +    fdput(f);
>> +    return ret;
>> +}
>> +
>>   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   {
>>       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> @@ -3587,11 +3604,11 @@ static void snp_cleanup_guest_buf(struct 
>> sev_data_snp_guest_request *data, unsig
>>       int ret;
>>       ret = snp_page_reclaim(pfn);
>> -    if (ret)
>> +    if (ret && (*rc == SEV_RET_SUCCESS))
>>           *rc = SEV_RET_INVALID_ADDRESS;
>>       ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>> -    if (ret)
>> +    if (ret && (*rc == SEV_RET_SUCCESS))
>>           *rc = SEV_RET_INVALID_ADDRESS;
>>   }
> 
> I believe we need to fix this as per the GHCB specifications.
> 
> As per GHCB 2.0 specifications:
> 
> SW_EXITINFO2
> ...
> State from Hypervisor: Upper
> 32-bits (63:32) will contain the
> return code from the hypervisor.
> Lower 32-bits (31:0) will contain
> the return code from the firmware
> call (0 = success)
> 
> So i believe the FW error code (which is the FW error code from 
> SNP_GUEST_REQUEST or *rc here) should be contained in the lower 32-bits 
> and the error code being returned back due to response buffer pages 
> reclaim failure and/or failure to transisition these pages back to 
> shared state is basically hypervisor (error) return code and that should 
> be returned in the upper 32-bit of the exitinfo.
> 
> There is work in progress to check conformance of SNP v7 patches to GHCB 
> 2.0 specifications, so probably this fix can be included as part of 
> those patches.

Yes, please :)


> 
>> @@ -3638,8 +3655,9 @@ static void snp_handle_ext_guest_request(struct 
>> vcpu_svm *svm, gpa_t req_gpa, gp
>>       struct kvm *kvm = vcpu->kvm;
>>       unsigned long data_npages;
>>       struct kvm_sev_info *sev;
>> -    unsigned long rc, err;
> 
> This needs to be looked at more carefully. The SEV firmware status code 
> is defined as 32-bit, but is being handled as unsigned long in the 
> KVM/SNP code and as int in the CCP driver. So this needs to be fixed 
> consistently across,

Ultimately it should be explicit u32 in SEV and u64 in GHCB because PSP 
and GHCB are binary interfaces and the sizes should be explicit. Error 
codes between KVM and CCP can be anything (unsigned long, u64) as it is 
the same binary.

 > snp_setup_guest_buf() return value will need to be
> fixed accordingly.
> 
>> +    unsigned long exitcode;
>>       u64 data_gpa;
>> +    int err, rc;
>>       if (!sev_snp_guest(vcpu->kvm)) {
>>           rc = SEV_RET_INVALID_GUEST;
>> @@ -3669,17 +3687,16 @@ static void 
>> snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
>>        */
>>       if (sev->snp_certs_len) {
>>           if ((data_npages << PAGE_SHIFT) < sev->snp_certs_len) {
>> -            rc = -EINVAL;
>> -            err = SNP_GUEST_REQ_INVALID_LEN;
>> +            rc = -ENOSPC;
> 
> Why do we need to introduce ENOSPC error code?

To distinguish it from other errors and return SNP_GUEST_REQ_INVALID_LEN 
when needed (the commit log mentions this).


> If we continue to use SNP_GUEST_REQ_INVALID_LEN we don't need to map 
> ENOSPC to SNP_GUEST_REQ_INVALID_LEN below.
> And the CCP driver can return SNP_GUEST_REQ_INVALID_LEN as earlier via 
> the fw_err parameter.

imho this is a bad idea.

SNP_GUEST_REQ_INVALID_LEN is defined in the GHCB spec and GHCB is 
between KVM and VM, /dev/sev is neither GHCB nor KVM. err here is for 
the firmware errors but SNP_GUEST_REQ_INVALID_LEN is not from the 
firmware and for not-from-the-firmware-errors we already have "return 
rc" so lets just use that. Also err is 32bit across the place, in things 
like sev_issue_cmd() and then there is this ugly cast to int*. Thanks,


> 
> Thanks,
> Ashish
> 
>>               goto datalen;
>>           }
>> -        rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
>> -                   (int *)&err);
>> +        rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req, &err);
>>       } else {
>> -        rc = snp_guest_ext_guest_request(&req,
>> -                         (unsigned long)sev->snp_certs_data,
>> -                         &data_npages, &err);
>> +        rc = sev_issue_cmd_cert(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
>> +                    (unsigned long)sev->snp_certs_data,
>> +                    &data_npages, &err);
>>       }
>> +
>>   datalen:
>>       if (sev->snp_certs_len)
>>           data_npages = sev->snp_certs_len >> PAGE_SHIFT;
>> @@ -3689,27 +3706,30 @@ static void 
>> snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
>>            * If buffer length is small then return the expected
>>            * length in rbx.
>>            */
>> -        if (err == SNP_GUEST_REQ_INVALID_LEN)
>> +        if (rc == -ENOSPC) {
>>               vcpu->arch.regs[VCPU_REGS_RBX] = data_npages;
>> +            exitcode = SNP_GUEST_REQ_INVALID_LEN;
>> +            goto cleanup;
>> +        }
>>           /* pass the firmware error code */
>> -        rc = err;
>> +        exitcode = err;
>>           goto cleanup;
>>       }
>>       /* Copy the certificate blob in the guest memory */
>>       if (data_npages &&
>>           kvm_write_guest(kvm, data_gpa, sev->snp_certs_data, 
>> data_npages << PAGE_SHIFT))
>> -        rc = SEV_RET_INVALID_ADDRESS;
>> +        exitcode = SEV_RET_INVALID_ADDRESS;
>>   cleanup:
>> -    snp_cleanup_guest_buf(&req, &rc);
>> +    snp_cleanup_guest_buf(&req, &exitcode);
>>   unlock:
>>       mutex_unlock(&sev->guest_req_lock);
>>   e_fail:
>> -    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>> +    svm_set_ghcb_sw_exit_info_2(vcpu, exitcode);
>>   }
>>   static kvm_pfn_t gfn_to_pfn_restricted(struct kvm *kvm, gfn_t gfn)
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 6c4fdcaed72b..73f56c20255c 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -2070,8 +2070,8 @@ int snp_guest_dbg_decrypt_page(u64 gctx_pfn, u64 
>> src_pfn, u64 dst_pfn, int *erro
>>   }
>>   EXPORT_SYMBOL_GPL(snp_guest_dbg_decrypt_page);
>> -int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
>> -                unsigned long vaddr, unsigned long *npages, unsigned 
>> long *fw_err)
>> +int sev_issue_cmd_external_user_cert(struct file *filep, unsigned int 
>> cmd, void *data,
>> +                     unsigned long vaddr, unsigned long *npages, int 
>> *error)
>>   {
>>       unsigned long expected_npages;
>>       struct sev_device *sev;
>> @@ -2093,12 +2093,11 @@ int snp_guest_ext_guest_request(struct 
>> sev_data_snp_guest_request *data,
>>       expected_npages = sev->snp_certs_len >> PAGE_SHIFT;
>>       if (*npages < expected_npages) {
>>           *npages = expected_npages;
>> -        *fw_err = SNP_GUEST_REQ_INVALID_LEN;
>>           mutex_unlock(&sev->snp_certs_lock);
>> -        return -EINVAL;
>> +        return -ENOSPC;
>>       }
>> -    rc = sev_do_cmd(SEV_CMD_SNP_GUEST_REQUEST, data, (int *)fw_err);
>> +    rc = sev_issue_cmd_external_user(filep, cmd, data, error);
>>       if (rc) {
>>           mutex_unlock(&sev->snp_certs_lock);
>>           return rc;
>> @@ -2115,7 +2114,7 @@ int snp_guest_ext_guest_request(struct 
>> sev_data_snp_guest_request *data,
>>       mutex_unlock(&sev->snp_certs_lock);
>>       return rc;
>>   }
>> -EXPORT_SYMBOL_GPL(snp_guest_ext_guest_request);
>> +EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user_cert);
>>   static void sev_exit(struct kref *ref)
>>   {
>>
  

Hello Alexey,

On 2/6/2023 7:24 PM, Alexey Kardashevskiy wrote:
> 
> 
> On 07/02/2023 08:57, Kalra, Ashish wrote:
>> On 2/5/2023 9:13 PM, Alexey Kardashevskiy wrote:
>>> When SVM VM is up, KVM uses sev_issue_cmd_external_user() with an open
>>> /dev/sev fd which ensures that the SVM initialization was done 
>>> correctly.
>>> The only helper not following the scheme is 
>>> snp_guest_ext_guest_request()
>>> which bypasses the fd check.
>>>
>>> Change the SEV API to require passing a file.
>>>
>>> Handle errors with care in the SNP Extended Guest Request handler
>>> (snp_handle_ext_guest_request()) as there are actually 3 types of 
>>> errors:
>>> - @rc: return code SEV device's sev_issue_cmd() which is int==int32;
>>> - @err: a psp return code in sev_issue_cmd(), also int==int32 (probably
>>> a mistake but kvm_sev_cmd::error uses __u32 for some time now);
>>> - (added by this) @exitcode: GHCB's exit code sw_exit_info_2, uint64.
>>>
>>> Use the right types, remove cast to int* and return ENOSPC from SEV
>>> device for converting it to the GHCB's exit code
>>> SNP_GUEST_REQ_INVALID_LEN==BIT(32).
>>>
>>> Fixes: 17f1d0c995ac ("KVM: SVM: Provide support for SNP_GUEST_REQUEST 
>>> NAE event")
>>> While at this, preserve the original error in snp_cleanup_guest_buf().
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>>> ---
>>>
>>> This can easily be squashed into what it fixes.
>>>
>>> The patch is made for
>>> https://github.com/AMDESE/linux/commits/upmv10-host-snp-v7-rfc
>>> ---
>>>   include/linux/psp-sev.h      | 62 +++++++++++---------
>>>   arch/x86/kvm/svm/sev.c       | 50 +++++++++++-----
>>>   drivers/crypto/ccp/sev-dev.c | 11 ++--
>>>   3 files changed, 73 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>>> index 970a9de0ed20..466b1a6e7d7b 100644
>>> --- a/include/linux/psp-sev.h
>>> +++ b/include/linux/psp-sev.h
>>> @@ -848,6 +848,36 @@ int sev_platform_status(struct 
>>> sev_user_data_status *status, int *error);
>>>   int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
>>>                   void *data, int *error);
>>> +/**
>>> + * sev_issue_cmd_external_user_cert - issue SEV command by other 
>>> driver with a file
>>> + * handle and return certificates set onto SEV device via 
>>> SNP_SET_EXT_CONFIG;
>>> + * intended for use by the SNP extended guest request command defined
>>> + * in the GHCB specification.
>>> + *
>>> + * @filep - SEV device file pointer
>>> + * @cmd - command to issue
>>> + * @data - command buffer
>>> + * @vaddr: address where the certificate blob need to be copied.
>>> + * @npages: number of pages for the certificate blob.
>>> + *    If the specified page count is less than the certificate blob 
>>> size, then the
>>> + *    required page count is returned with ENOSPC error code.
>>> + *    If the specified page count is more than the certificate blob 
>>> size, then
>>> + *    page count is updated to reflect the amount of valid data 
>>> copied in the
>>> + *    vaddr.
>>> + *
>>> + * @error: SEV command return code
>>> + *
>>> + * Returns:
>>> + * 0 if the sev successfully processed the command
>>> + * -%ENODEV    if the sev device is not available
>>> + * -%ENOTSUPP  if the sev does not support SEV
>>> + * -%ETIMEDOUT if the sev command timed out
>>> + * -%EIO       if the sev returned a non-zero return code
>>> + * -%ENOSPC    if the specified page count is too small
>>> + */
>>> +int sev_issue_cmd_external_user_cert(struct file *filep, unsigned 
>>> int cmd, void *data,
>>> +                     unsigned long vaddr, unsigned long *npages, int 
>>> *error);
>>> +
>>>   /**
>>>    * sev_guest_deactivate - perform SEV DEACTIVATE command
>>>    *
>>> @@ -945,32 +975,6 @@ void snp_free_firmware_page(void *addr);
>>>    */
>>>   void snp_mark_pages_offline(unsigned long pfn, unsigned int npages);
>>> -/**
>>> - * snp_guest_ext_guest_request - perform the SNP extended guest 
>>> request command
>>> - *  defined in the GHCB specification.
>>> - *
>>> - * @data: the input guest request structure
>>> - * @vaddr: address where the certificate blob need to be copied.
>>> - * @npages: number of pages for the certificate blob.
>>> - *    If the specified page count is less than the certificate blob 
>>> size, then the
>>> - *    required page count is returned with error code defined in the 
>>> GHCB spec.
>>> - *    If the specified page count is more than the certificate blob 
>>> size, then
>>> - *    page count is updated to reflect the amount of valid data 
>>> copied in the
>>> - *    vaddr.
>>> - *
>>> - * @sev_ret: sev command return code
>>> - *
>>> - * Returns:
>>> - * 0 if the sev successfully processed the command
>>> - * -%ENODEV    if the sev device is not available
>>> - * -%ENOTSUPP  if the sev does not support SEV
>>> - * -%ETIMEDOUT if the sev command timed out
>>> - * -%EIO       if the sev returned a non-zero return code
>>> - */
>>> -int snp_guest_ext_guest_request(struct sev_data_snp_guest_request 
>>> *data,
>>> -                unsigned long vaddr, unsigned long *npages,
>>> -                unsigned long *error);
>>> -
>>>   #else    /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>>   static inline int
>>> @@ -1013,9 +1017,9 @@ static inline void 
>>> *snp_alloc_firmware_page(gfp_t mask)
>>>   static inline void snp_free_firmware_page(void *addr) { }
>>> -static inline int snp_guest_ext_guest_request(struct 
>>> sev_data_snp_guest_request *data,
>>> -                          unsigned long vaddr, unsigned long *n,
>>> -                          unsigned long *error)
>>> +static inline int sev_issue_cmd_external_user_cert(struct file 
>>> *filep, unsigned int cmd,
>>> +                           void *data, unsigned long vaddr,
>>> +                           unsigned long *npages, int *error)
>>>   {
>>>       return -ENODEV;
>>>   }
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index d0e58cffd1ed..b268c35efab4 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -394,6 +394,23 @@ static int sev_issue_cmd(struct kvm *kvm, int 
>>> id, void *data, int *error)
>>>       return __sev_issue_cmd(sev->fd, id, data, error);
>>>   }
>>> +static int sev_issue_cmd_cert(struct kvm *kvm, int id, void *data,
>>> +                  unsigned long vaddr, unsigned long *npages, int 
>>> *error)
>>> +{
>>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>> +    struct fd f;
>>> +    int ret;
>>> +
>>> +    f = fdget(sev->fd);
>>> +    if (!f.file)
>>> +        return -EBADF;
>>> +
>>> +    ret = sev_issue_cmd_external_user_cert(f.file, id, data, vaddr, 
>>> npages, error);
>>> +
>>> +    fdput(f);
>>> +    return ret;
>>> +}
>>> +
>>>   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>   {
>>>       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>> @@ -3587,11 +3604,11 @@ static void snp_cleanup_guest_buf(struct 
>>> sev_data_snp_guest_request *data, unsig
>>>       int ret;
>>>       ret = snp_page_reclaim(pfn);
>>> -    if (ret)
>>> +    if (ret && (*rc == SEV_RET_SUCCESS))
>>>           *rc = SEV_RET_INVALID_ADDRESS;
>>>       ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>>> -    if (ret)
>>> +    if (ret && (*rc == SEV_RET_SUCCESS))
>>>           *rc = SEV_RET_INVALID_ADDRESS;
>>>   }
>>
>> I believe we need to fix this as per the GHCB specifications.
>>
>> As per GHCB 2.0 specifications:
>>
>> SW_EXITINFO2
>> ...
>> State from Hypervisor: Upper
>> 32-bits (63:32) will contain the
>> return code from the hypervisor.
>> Lower 32-bits (31:0) will contain
>> the return code from the firmware
>> call (0 = success)
>>
>> So i believe the FW error code (which is the FW error code from 
>> SNP_GUEST_REQUEST or *rc here) should be contained in the lower 
>> 32-bits and the error code being returned back due to response buffer 
>> pages reclaim failure and/or failure to transisition these pages back 
>> to shared state is basically hypervisor (error) return code and that 
>> should be returned in the upper 32-bit of the exitinfo.
>>
>> There is work in progress to check conformance of SNP v7 patches to 
>> GHCB 2.0 specifications, so probably this fix can be included as part 
>> of those patches.
> 
> Yes, please :)
> 

Yes, will address this in the GHCB specs conformance patch-set for SNP 
as per the following revisions of the GHCB specs:

The SNP Guest Request and SNP Extended Guest Request have been updated 
to expand on the use of the SW_EXITINFO2 return value to better allow 
for the hypervisor to return error codes.

> 
>>
>>> @@ -3638,8 +3655,9 @@ static void snp_handle_ext_guest_request(struct 
>>> vcpu_svm *svm, gpa_t req_gpa, gp
>>>       struct kvm *kvm = vcpu->kvm;
>>>       unsigned long data_npages;
>>>       struct kvm_sev_info *sev;
>>> -    unsigned long rc, err;
>>
>> This needs to be looked at more carefully. The SEV firmware status 
>> code is defined as 32-bit, but is being handled as unsigned long in 
>> the KVM/SNP code and as int in the CCP driver. So this needs to be 
>> fixed consistently across,
> 
> Ultimately it should be explicit u32 in SEV and u64 in GHCB because PSP 
> and GHCB are binary interfaces and the sizes should be explicit. Error 
> codes between KVM and CCP can be anything (unsigned long, u64) as it is 
> the same binary.
> 

Again, as lower 32-bits (31:0) of SW_EXITINFO2 is supposed to be set
to the return code from the firmware, so this should also be u32 in GHCB 
and same in KVM/SNP code.

>  > snp_setup_guest_buf() return value will need to be
>> fixed accordingly.
>>
>>> +    unsigned long exitcode;
>>>       u64 data_gpa;
>>> +    int err, rc;
>>>       if (!sev_snp_guest(vcpu->kvm)) {
>>>           rc = SEV_RET_INVALID_GUEST;
>>> @@ -3669,17 +3687,16 @@ static void 
>>> snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
>>>        */
>>>       if (sev->snp_certs_len) {
>>>           if ((data_npages << PAGE_SHIFT) < sev->snp_certs_len) {
>>> -            rc = -EINVAL;
>>> -            err = SNP_GUEST_REQ_INVALID_LEN;
>>> +            rc = -ENOSPC;
>>
>> Why do we need to introduce ENOSPC error code?
> 
> To distinguish it from other errors and return SNP_GUEST_REQ_INVALID_LEN 
> when needed (the commit log mentions this).
> 
> 
>> If we continue to use SNP_GUEST_REQ_INVALID_LEN we don't need to map 
>> ENOSPC to SNP_GUEST_REQ_INVALID_LEN below.
>> And the CCP driver can return SNP_GUEST_REQ_INVALID_LEN as earlier via 
>> the fw_err parameter.
> 
> imho this is a bad idea.
> 
> SNP_GUEST_REQ_INVALID_LEN is defined in the GHCB spec and GHCB is 
> between KVM and VM, /dev/sev is neither GHCB nor KVM. err here is for 
> the firmware errors but SNP_GUEST_REQ_INVALID_LEN is not from the 
> firmware and for not-from-the-firmware-errors we already have "return 
> rc" so lets just use that. Also err is 32bit across the place, in things 
> like sev_issue_cmd() and then there is this ugly cast to int*. Thanks,
> 

Ok, that does make sense.

Thanks,
Ashish