Messages in this thread | | | Date | Wed, 8 Feb 2023 15:50:53 -0600 | Subject | Re: [PATCH kernel] KVM: SVM: Fix SVM_VMGEXIT_EXT_GUEST_REQUEST to follow the rest of API | From | "Kalra, Ashish" <> |
| |
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
| |