Since the VMGEXIT instruction can be issued from userspace, invalidate
the GHCB after performing VMGEXIT processing in the kernel.
Invalidation is only required after userspace is available, so call
vc_ghcb_invalidate() from sev_es_put_ghcb(). Update vc_ghcb_invalidate()
to additionally clear the GHCB exit code, so that a value of 0 is always
present outside VMGEXIT processing in the kernel.
Since vc_ghcb_invalidate() is part of sev-shared.c, move sev_es_put_ghcb()
down to after where sev-shared.c is included.
Fixes: 0786138c78e79 ("x86/sev-es: Add a Runtime #VC Exception Handler")
Signed-off-by: Tom Lendacky <[email protected]>
---
arch/x86/kernel/sev-shared.c | 1 +
arch/x86/kernel/sev.c | 37 ++++++++++++++++++------------------
2 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 6ec8b3bfd76e..9f90f460a28c 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -63,6 +63,7 @@ static bool sev_es_negotiate_protocol(void)
static __always_inline void vc_ghcb_invalidate(struct ghcb *ghcb)
{
+ ghcb->save.sw_exit_code = 0;
memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
}
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 9578c82832aa..5ccb0218c885 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -221,24 +221,6 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state)
return ghcb;
}
-static __always_inline void sev_es_put_ghcb(struct ghcb_state *state)
-{
- struct sev_es_runtime_data *data;
- struct ghcb *ghcb;
-
- data = this_cpu_read(runtime_data);
- ghcb = &data->ghcb_page;
-
- if (state->ghcb) {
- /* Restore GHCB from Backup */
- *ghcb = *state->ghcb;
- data->backup_ghcb_active = false;
- state->ghcb = NULL;
- } else {
- data->ghcb_active = false;
- }
-}
-
/* Needed in vc_early_forward_exception */
void do_early_exception(struct pt_regs *regs, int trapnr);
@@ -461,6 +443,25 @@ static enum es_result vc_slow_virt_to_phys(struct ghcb *ghcb, struct es_em_ctxt
/* Include code shared with pre-decompression boot stage */
#include "sev-shared.c"
+static __always_inline void sev_es_put_ghcb(struct ghcb_state *state)
+{
+ struct sev_es_runtime_data *data;
+ struct ghcb *ghcb;
+
+ data = this_cpu_read(runtime_data);
+ ghcb = &data->ghcb_page;
+
+ if (state->ghcb) {
+ /* Restore GHCB from Backup */
+ *ghcb = *state->ghcb;
+ data->backup_ghcb_active = false;
+ state->ghcb = NULL;
+ } else {
+ vc_ghcb_invalidate(ghcb);
+ data->ghcb_active = false;
+ }
+}
+
void noinstr __sev_es_nmi_complete(void)
{
struct ghcb_state state;
--
2.31.0
On 5/14/21 2:12 PM, Tom Lendacky wrote:
> Since the VMGEXIT instruction can be issued from userspace, invalidate
> the GHCB after performing VMGEXIT processing in the kernel.
>
> Invalidation is only required after userspace is available, so call
> vc_ghcb_invalidate() from sev_es_put_ghcb(). Update vc_ghcb_invalidate()
> to additionally clear the GHCB exit code, so that a value of 0 is always
> present outside VMGEXIT processing in the kernel.
>
> Since vc_ghcb_invalidate() is part of sev-shared.c, move sev_es_put_ghcb()
> down to after where sev-shared.c is included.
>
> Fixes: 0786138c78e79 ("x86/sev-es: Add a Runtime #VC Exception Handler")
> Signed-off-by: Tom Lendacky <[email protected]>
> ---
> arch/x86/kernel/sev-shared.c | 1 +
> arch/x86/kernel/sev.c | 37 ++++++++++++++++++------------------
> 2 files changed, 20 insertions(+), 18 deletions(-)
I was debating whether to do this as two patches, with the first patch
moving sev_es_put_ghcb() and then the second patch would more clearly show
the changes related to the invalidation.
Let me know if that would be preferred and I'll re-submit this as a
two-patch series.
Thanks,
Tom
>
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 6ec8b3bfd76e..9f90f460a28c 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -63,6 +63,7 @@ static bool sev_es_negotiate_protocol(void)
>
> static __always_inline void vc_ghcb_invalidate(struct ghcb *ghcb)
> {
> + ghcb->save.sw_exit_code = 0;
> memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
> }
>
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 9578c82832aa..5ccb0218c885 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -221,24 +221,6 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state)
> return ghcb;
> }
>
> -static __always_inline void sev_es_put_ghcb(struct ghcb_state *state)
> -{
> - struct sev_es_runtime_data *data;
> - struct ghcb *ghcb;
> -
> - data = this_cpu_read(runtime_data);
> - ghcb = &data->ghcb_page;
> -
> - if (state->ghcb) {
> - /* Restore GHCB from Backup */
> - *ghcb = *state->ghcb;
> - data->backup_ghcb_active = false;
> - state->ghcb = NULL;
> - } else {
> - data->ghcb_active = false;
> - }
> -}
> -
> /* Needed in vc_early_forward_exception */
> void do_early_exception(struct pt_regs *regs, int trapnr);
>
> @@ -461,6 +443,25 @@ static enum es_result vc_slow_virt_to_phys(struct ghcb *ghcb, struct es_em_ctxt
> /* Include code shared with pre-decompression boot stage */
> #include "sev-shared.c"
>
> +static __always_inline void sev_es_put_ghcb(struct ghcb_state *state)
> +{
> + struct sev_es_runtime_data *data;
> + struct ghcb *ghcb;
> +
> + data = this_cpu_read(runtime_data);
> + ghcb = &data->ghcb_page;
> +
> + if (state->ghcb) {
> + /* Restore GHCB from Backup */
> + *ghcb = *state->ghcb;
> + data->backup_ghcb_active = false;
> + state->ghcb = NULL;
> + } else {
> + vc_ghcb_invalidate(ghcb);
> + data->ghcb_active = false;
> + }
> +}
> +
> void noinstr __sev_es_nmi_complete(void)
> {
> struct ghcb_state state;
>
Hi Tom,
On Fri, May 14, 2021 at 02:12:33PM -0500, Tom Lendacky wrote:
> arch/x86/kernel/sev-shared.c | 1 +
> arch/x86/kernel/sev.c | 37 ++++++++++++++++++------------------
> 2 files changed, 20 insertions(+), 18 deletions(-)
Having this change in one patch is okay. No need to split it up.
> +static __always_inline void sev_es_put_ghcb(struct ghcb_state *state)
> +{
> + struct sev_es_runtime_data *data;
> + struct ghcb *ghcb;
> +
> + data = this_cpu_read(runtime_data);
> + ghcb = &data->ghcb_page;
> +
> + if (state->ghcb) {
> + /* Restore GHCB from Backup */
> + *ghcb = *state->ghcb;
> + data->backup_ghcb_active = false;
> + state->ghcb = NULL;
> + } else {
> + vc_ghcb_invalidate(ghcb);
A comment would be good to explain why the invalidate here is
necessary.
Regards,
Joerg
On 5/17/21 7:33 AM, Joerg Roedel wrote:
> Hi Tom,
>
> On Fri, May 14, 2021 at 02:12:33PM -0500, Tom Lendacky wrote:
>> arch/x86/kernel/sev-shared.c | 1 +
>> arch/x86/kernel/sev.c | 37 ++++++++++++++++++------------------
>> 2 files changed, 20 insertions(+), 18 deletions(-)
>
> Having this change in one patch is okay. No need to split it up.
>
>> +static __always_inline void sev_es_put_ghcb(struct ghcb_state *state)
>> +{
>> + struct sev_es_runtime_data *data;
>> + struct ghcb *ghcb;
>> +
>> + data = this_cpu_read(runtime_data);
>> + ghcb = &data->ghcb_page;
>> +
>> + if (state->ghcb) {
>> + /* Restore GHCB from Backup */
>> + *ghcb = *state->ghcb;
>> + data->backup_ghcb_active = false;
>> + state->ghcb = NULL;
>> + } else {
>> + vc_ghcb_invalidate(ghcb);
>
> A comment would be good to explain why the invalidate here is
> necessary.
Ah, good point. I'll add that and send a v2, but I'll wait for further
feedback before sending the next version.
Thanks,
Tom
>
> Regards,
>
> Joerg
>