Re: [PATCH kernel] KVM: SVM: Fix SVM_VMGEXIT_EXT_GUEST_REQUEST to follow the rest of API
From: Kalra, Ashish
Date: Wed Feb 08 2023 - 16:51:13 EST
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@xxxxxxx>
---
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