Re: [PATCH Part1 v5 13/38] x86/sev: Register GHCB memory when SEV-SNP is active

[Borislav Petkov <bp@alien8.de>]

On Fri, Aug 20, 2021 at 10:19:08AM -0500, Brijesh Singh wrote:
> The SEV-SNP guest is required to perform GHCB GPA registration. This is
> because the hypervisor may prefer that a guest use a consistent and/or
> specific GPA for the GHCB associated with a vCPU. For more information,
> see the GHCB specification section GHCB GPA Registration.
> 
> During the boot, init_ghcb() allocates a per-cpu GHCB page. On very first
> VC exception, the exception handler switch to using the per-cpu GHCB page
> allocated during the init_ghcb(). The GHCB page must be registered in
> the current vcpu context.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/kernel/sev-internal.h | 12 ++++++++++++
>  arch/x86/kernel/sev.c          | 28 ++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
>  create mode 100644 arch/x86/kernel/sev-internal.h
> 
> diff --git a/arch/x86/kernel/sev-internal.h b/arch/x86/kernel/sev-internal.h
> new file mode 100644
> index 000000000000..0fb7324803b4
> --- /dev/null
> +++ b/arch/x86/kernel/sev-internal.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Forward declarations for sev-shared.c
> + *
> + * Author: Brijesh Singh <brijesh.singh@amd.com>
> + */
> +
> +#ifndef __X86_SEV_INTERNAL_H__
> +
> +static void snp_register_ghcb_early(unsigned long paddr);
> +
> +#endif	/* __X86_SEV_INTERNAL_H__ */

I believe you don't need that header if you move __sev_get_ghcb()
and snp_register_ghcb() under the #include "sev-shared.c" so that
snp_register_ghcb_early() is visible by then.

diff --git a/arch/x86/kernel/sev-internal.h b/arch/x86/kernel/sev-internal.h
deleted file mode 100644
index 0fb7324803b4..000000000000
--- a/arch/x86/kernel/sev-internal.h
+++ /dev/null
@@ -1,12 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Forward declarations for sev-shared.c
- *
- * Author: Brijesh Singh <brijesh.singh@amd.com>
- */
-
-#ifndef __X86_SEV_INTERNAL_H__
-
-static void snp_register_ghcb_early(unsigned long paddr);
-
-#endif	/* __X86_SEV_INTERNAL_H__ */
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 9ab541b893c2..0ec0602e4bed 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -31,8 +31,6 @@
 #include <asm/smp.h>
 #include <asm/cpu.h>
 
-#include "sev-internal.h"
-
 #define DR7_RESET_VALUE        0x400
 
 /* For early boot hypervisor communication in SEV-ES enabled guests */
@@ -200,69 +198,6 @@ void noinstr __sev_es_ist_exit(void)
 	this_cpu_write(cpu_tss_rw.x86_tss.ist[IST_INDEX_VC], *(unsigned long *)ist);
 }
 
-static void snp_register_ghcb(struct sev_es_runtime_data *data, unsigned long paddr)
-{
-	if (data->snp_ghcb_registered)
-		return;
-
-	snp_register_ghcb_early(paddr);
-
-	data->snp_ghcb_registered = true;
-}
-
-/*
- * Nothing shall interrupt this code path while holding the per-CPU
- * GHCB. The backup GHCB is only for NMIs interrupting this path.
- *
- * Callers must disable local interrupts around it.
- */
-static noinstr struct ghcb *__sev_get_ghcb(struct ghcb_state *state)
-{
-	struct sev_es_runtime_data *data;
-	struct ghcb *ghcb;
-
-	WARN_ON(!irqs_disabled());
-
-	data = this_cpu_read(runtime_data);
-	ghcb = &data->ghcb_page;
-
-	if (unlikely(data->ghcb_active)) {
-		/* GHCB is already in use - save its contents */
-
-		if (unlikely(data->backup_ghcb_active)) {
-			/*
-			 * Backup-GHCB is also already in use. There is no way
-			 * to continue here so just kill the machine. To make
-			 * panic() work, mark GHCBs inactive so that messages
-			 * can be printed out.
-			 */
-			data->ghcb_active        = false;
-			data->backup_ghcb_active = false;
-
-			instrumentation_begin();
-			panic("Unable to handle #VC exception! GHCB and Backup GHCB are already in use");
-			instrumentation_end();
-		}
-
-		/* Mark backup_ghcb active before writing to it */
-		data->backup_ghcb_active = true;
-
-		state->ghcb = &data->backup_ghcb;
-
-		/* Backup GHCB content */
-		*state->ghcb = *ghcb;
-	} else {
-		state->ghcb = NULL;
-		data->ghcb_active = true;
-	}
-
-	/* SEV-SNP guest requires that GHCB must be registered. */
-	if (sev_feature_enabled(SEV_SNP))
-		snp_register_ghcb(data, __pa(ghcb));
-
-	return ghcb;
-}
-
 /* Needed in vc_early_forward_exception */
 void do_early_exception(struct pt_regs *regs, int trapnr);
 
@@ -518,6 +453,69 @@ 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 void snp_register_ghcb(struct sev_es_runtime_data *data, unsigned long paddr)
+{
+	if (data->snp_ghcb_registered)
+		return;
+
+	snp_register_ghcb_early(paddr);
+
+	data->snp_ghcb_registered = true;
+}
+
+/*
+ * Nothing shall interrupt this code path while holding the per-CPU
+ * GHCB. The backup GHCB is only for NMIs interrupting this path.
+ *
+ * Callers must disable local interrupts around it.
+ */
+static noinstr struct ghcb *__sev_get_ghcb(struct ghcb_state *state)
+{
+	struct sev_es_runtime_data *data;
+	struct ghcb *ghcb;
+
+	WARN_ON(!irqs_disabled());
+
+	data = this_cpu_read(runtime_data);
+	ghcb = &data->ghcb_page;
+
+	if (unlikely(data->ghcb_active)) {
+		/* GHCB is already in use - save its contents */
+
+		if (unlikely(data->backup_ghcb_active)) {
+			/*
+			 * Backup-GHCB is also already in use. There is no way
+			 * to continue here so just kill the machine. To make
+			 * panic() work, mark GHCBs inactive so that messages
+			 * can be printed out.
+			 */
+			data->ghcb_active        = false;
+			data->backup_ghcb_active = false;
+
+			instrumentation_begin();
+			panic("Unable to handle #VC exception! GHCB and Backup GHCB are already in use");
+			instrumentation_end();
+		}
+
+		/* Mark backup_ghcb active before writing to it */
+		data->backup_ghcb_active = true;
+
+		state->ghcb = &data->backup_ghcb;
+
+		/* Backup GHCB content */
+		*state->ghcb = *ghcb;
+	} else {
+		state->ghcb = NULL;
+		data->ghcb_active = true;
+	}
+
+	/* SEV-SNP guest requires that GHCB must be registered. */
+	if (sev_feature_enabled(SEV_SNP))
+		snp_register_ghcb(data, __pa(ghcb));
+
+	return ghcb;
+}
+
 static noinstr void __sev_put_ghcb(struct ghcb_state *state)
 {
 	struct sev_es_runtime_data *data;

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette