The GHCB specification declares that the firmware error value for a
guest request will be stored in the lower 32 bits of EXIT_INFO_2.
The upper 32 bits are for the VMM's own error code. The fw_err argument
is thus a misnomer, and callers will need access to all 64 bits.
The type of unsigned long also causes problems, since sw_exit_info2 is
u64 (unsigned long long) vs the argument's unsigned long*. A temporary
typedef is introduced for the err argument so it can be changed in a
later patch more cleanly.
The firmware might not even be called, so the call is bookended with
the no firmware call error and clearing the error.
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Peter Gonda <pgonda@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <Borislav.Petkov@amd.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Venu Busireddy <venu.busireddy@oracle.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
Cc: Michael Sterritt <sterritt@google.com>
Fixes: d5af44dde546 ("x86/sev: Provide support for SNP guest request NAEs")
Reviewed-by: Tom Lendacky <Thomas.Lendacky@amd.com>
Reviewed-by: Borislav Petkov <Borislav.Petkov@amd.com>
Reviewed-by: Peter Gonda <pgonda@google.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
arch/x86/include/asm/sev.h | 10 ++++++++--
arch/x86/kernel/sev.c | 10 ++++++----
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index ebc271bb6d8e..5b03ba18fee7 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -133,6 +133,12 @@ struct snp_secrets_page_layout {
u8 rsvd3[3840];
} __packed;
+/*
+ * Use a type alias temporarily to cleanly change the snp_issue_guest_request
+ * signature cleanly over multiple patches.
+ */
+typedef unsigned long sev_guestreq_err_t;
+
#ifdef CONFIG_AMD_MEM_ENCRYPT
extern struct static_key_false sev_es_enable_key;
extern void __sev_es_ist_enter(struct pt_regs *regs);
@@ -196,7 +202,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned int npages);
void snp_set_wakeup_secondary_cpu(void);
bool snp_init(struct boot_params *bp);
void __init __noreturn snp_abort(void);
-int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err);
+int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, sev_guestreq_err_t *exitinfo2);
#else
static inline void sev_es_ist_enter(struct pt_regs *regs) { }
static inline void sev_es_ist_exit(void) { }
@@ -217,7 +223,7 @@ static inline void snp_set_wakeup_secondary_cpu(void) { }
static inline bool snp_init(struct boot_params *bp) { return false; }
static inline void snp_abort(void) { }
static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input,
- unsigned long *fw_err)
+ sev_guestreq_err_t *exitinfo2)
{
return -ENOTTY;
}
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 679026a640ef..d1a6092b1e03 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -22,6 +22,7 @@
#include <linux/efi.h>
#include <linux/platform_device.h>
#include <linux/io.h>
+#include <linux/psp-sev.h>
#include <asm/cpu_entry_area.h>
#include <asm/stacktrace.h>
@@ -2175,7 +2176,7 @@ static int __init init_sev_config(char *str)
}
__setup("sev=", init_sev_config);
-int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err)
+int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, sev_guestreq_err_t *exitinfo2)
{
struct ghcb_state state;
struct es_em_ctxt ctxt;
@@ -2186,9 +2187,11 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned
if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
return -ENODEV;
- if (!fw_err)
+ if (!exitinfo2)
return -EINVAL;
+ *exitinfo2 = SEV_RET_NO_FW_CALL;
+
/*
* __sev_get_ghcb() needs to run with IRQs disabled because it is using
* a per-CPU GHCB.
@@ -2212,14 +2215,13 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned
if (ret)
goto e_put;
+ *exitinfo2 = ghcb->save.sw_exit_info_2;
if (ghcb->save.sw_exit_info_2) {
/* Number of expected pages are returned in RBX */
if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST &&
ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN)
input->data_npages = ghcb_get_rbx(ghcb);
- *fw_err = ghcb->save.sw_exit_info_2;
-
ret = -EIO;
}
--
2.39.0.314.g84b9a713c41-googOn Wed, Jan 11, 2023 at 07:39:56PM +0000, Dionna Glaze wrote:
> The type of unsigned long also causes problems, since sw_exit_info2 is
> u64 (unsigned long long) vs the argument's unsigned long*. A temporary
> typedef is introduced for the err argument so it can be changed in a
> later patch more cleanly.
Why?
Why not make it a u64 directly and be done with it?
---
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index ebc271bb6d8e..61b0bfc7f131 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -196,7 +196,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned int npages);
void snp_set_wakeup_secondary_cpu(void);
bool snp_init(struct boot_params *bp);
void __init __noreturn snp_abort(void);
-int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err);
+int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, u64 *exitinfo2);
#else
static inline void sev_es_ist_enter(struct pt_regs *regs) { }
static inline void sev_es_ist_exit(void) { }
@@ -216,8 +216,7 @@ static inline void snp_set_memory_private(unsigned long vaddr, unsigned int npag
static inline void snp_set_wakeup_secondary_cpu(void) { }
static inline bool snp_init(struct boot_params *bp) { return false; }
static inline void snp_abort(void) { }
-static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input,
- unsigned long *fw_err)
+static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, u64 *exitinfo2)
{
return -ENOTTY;
}
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 679026a640ef..70b4cbd33c45 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -22,6 +22,7 @@
#include <linux/efi.h>
#include <linux/platform_device.h>
#include <linux/io.h>
+#include <linux/psp-sev.h>
#include <asm/cpu_entry_area.h>
#include <asm/stacktrace.h>
@@ -2175,7 +2176,7 @@ static int __init init_sev_config(char *str)
}
__setup("sev=", init_sev_config);
-int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err)
+int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, u64 *exitinfo2)
{
struct ghcb_state state;
struct es_em_ctxt ctxt;
@@ -2186,9 +2187,11 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned
if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
return -ENODEV;
- if (!fw_err)
+ if (!exitinfo2)
return -EINVAL;
+ *exitinfo2 = SEV_RET_NO_FW_CALL;
+
/*
* __sev_get_ghcb() needs to run with IRQs disabled because it is using
* a per-CPU GHCB.
@@ -2212,14 +2215,13 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned
if (ret)
goto e_put;
+ *exitinfo2 = ghcb->save.sw_exit_info_2;
if (ghcb->save.sw_exit_info_2) {
/* Number of expected pages are returned in RBX */
if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST &&
ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN)
input->data_npages = ghcb_get_rbx(ghcb);
- *fw_err = ghcb->save.sw_exit_info_2;
-
ret = -EIO;
}
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 4ec4174e05a3..e15d94785761 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -322,8 +322,7 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
u8 type, void *req_buf, size_t req_sz, void *resp_buf,
u32 resp_sz, __u64 *fw_err)
{
- unsigned long err;
- u64 seqno;
+ u64 seqno, err;
int rc;
/* Get message sequence and verify that its a non-zero */
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
> > Why not make it a u64 directly and be done with it? > > --- I was operating under the assumption that kvm: sev: patches should only touch kvm, and virt: coco: sev-guest should only touch sev-guest. If you're okay with a patch that changes both areas at the same time, then I could do that. -- -Dionna Glaze, PhD (she/her)
On Fri, Jan 20, 2023 at 10:03:33AM -0800, Dionna Amalie Glaze wrote:
> I was operating under the assumption that kvm: sev: patches should
> only touch kvm, and virt: coco: sev-guest should only touch sev-guest.
No, we pretty-much never do that. If a patch touches multiple subsystems, the
relevant maintainers agree on its path upstream.
> If you're okay with a patch that changes both areas at the same time,
> then I could do that.
Yes pls.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
© 2016 - 2025 Red Hat, Inc.