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 previous unsigned long*.
The signature change requires the follow-up change to
drivers/virt/coco/sev-guest to use the new expected type in order to
compile.
The firmware might not even be called, so we bookend the call with the
no firmware call error and clearing the error.
Cc: Tom Lendacky <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Peter Gonda <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Venu Busireddy <[email protected]>
Cc: Michael Roth <[email protected]>
Cc: "Kirill A. Shutemov" <[email protected]>
Cc: Michael Sterritt <[email protected]>
Fixes: d5af44dde546 ("x86/sev: Provide support for SNP guest request NAEs")
Signed-off-by: Dionna Glaze <[email protected]>
---
arch/x86/include/asm/sev.h | 4 ++--
arch/x86/kernel/sev.c | 10 ++++++----
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index ebc271bb6d8e..05de34d10d89 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) { }
@@ -217,7 +217,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)
+ u64 *exitinfo2)
{
return -ENOTTY;
}
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index a428c62330d3..148f17cb07b5 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;
}
--
2.38.1.431.g37b22c650d-goog
On Fri, Nov 4, 2022 at 5:01 PM Dionna Glaze <[email protected]> wrote:
>
> 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 previous unsigned long*.
> The signature change requires the follow-up change to
> drivers/virt/coco/sev-guest to use the new expected type in order to
> compile.
>
> The firmware might not even be called, so we bookend the call with the
> no firmware call error and clearing the error.
>
> Cc: Tom Lendacky <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Peter Gonda <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Venu Busireddy <[email protected]>
> Cc: Michael Roth <[email protected]>
> Cc: "Kirill A. Shutemov" <[email protected]>
> Cc: Michael Sterritt <[email protected]>
>
> Fixes: d5af44dde546 ("x86/sev: Provide support for SNP guest request NAEs")
> Signed-off-by: Dionna Glaze <[email protected]>
Reviewed-by: Peter Gonda <[email protected]
On 11/4/22 18:00, Dionna Glaze wrote:
> 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 previous unsigned long*.
> The signature change requires the follow-up change to
> drivers/virt/coco/sev-guest to use the new expected type in order to
> compile.
>
> The firmware might not even be called, so we bookend the call with the
> no firmware call error and clearing the error.
>
> Cc: Tom Lendacky <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Peter Gonda <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Venu Busireddy <[email protected]>
> Cc: Michael Roth <[email protected]>
> Cc: "Kirill A. Shutemov" <[email protected]>
> Cc: Michael Sterritt <[email protected]>
>
> Fixes: d5af44dde546 ("x86/sev: Provide support for SNP guest request NAEs")
> Signed-off-by: Dionna Glaze <[email protected]>
Reviewed-by: Tom Lendacky <[email protected]>
> ---
> arch/x86/include/asm/sev.h | 4 ++--
> arch/x86/kernel/sev.c | 10 ++++++----
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index ebc271bb6d8e..05de34d10d89 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) { }
> @@ -217,7 +217,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)
> + u64 *exitinfo2)
> {
> return -ENOTTY;
> }
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index a428c62330d3..148f17cb07b5 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;
> }
>
On Fri, Nov 04, 2022 at 11:00:38PM +0000, Dionna Glaze wrote:
> 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 previous unsigned long*.
> The signature change requires the follow-up change to
> drivers/virt/coco/sev-guest to use the new expected type in order to
> compile.
>
> The firmware might not even be called, so we bookend the call with the
Please use passive voice in your commit message: no "we" or "I",
etc, and describe your changes in imperative mood.
Personal pronouns are ambiguous in text, especially with so many
parties/companies/etc developing the kernel so let's avoid them please.
Otherwise, LGTM.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette