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

From: "Kalra, Ashish" <ashish.kalra@amd.com>
To: Alexey Kardashevskiy <aik@amd.com>, kvm@vger.kernel.org
Cc: x86@kernel.org, linux-kernel@vger.kernel.org,
	linux-crypto@vger.kernel.org,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Michael Roth <michael.roth@amd.com>,
	John Allen <john.allen@amd.com>, Ingo Molnar <mingo@redhat.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Dionna Glaze <dionnaglaze@google.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Borislav Petkov <bp@alien8.de>, "H. Peter Anvin" <hpa@zytor.com>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH kernel] KVM: SVM: Fix SVM_VMGEXIT_EXT_GUEST_REQUEST to follow the rest of API
Date: Wed, 8 Feb 2023 15:50:53 -0600	[thread overview]
Message-ID: <8ad433cf-36bc-63e8-4f8c-13e2a283c61c@amd.com> (raw)
In-Reply-To: <ff0ffddf-9a4b-e1cd-7c34-73412c89ded6@amd.com>

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