Skip to contentSkip to conversation

Alternate Injection: Secure Interrupt Delivery for SEV-SNP Guests - Guest Support

PATCH Superseded Melody Wang 3 participants 22 messages Sep 20 – Sep 28

Subsystems (3)

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v3 cover Alternate Injection: Secure Interrupt Delivery for SEV-SNP Guests - Guest Support

Hi all,

This revision addresses all review comments from version 2.

The changes include:

1. Moved sev_prepare() from decompressor to EFI stub.

2. Changed "Register Alternate Injection" and "configure all interrupt vectors" as two APIs located in compressed/sev.c since they are self-contained, avoiding exporting functions.

3. Added more comments for better understanding.

Thanks,
Melody

Changelog:
v2
--

This revision fixes all the review comments from version 1.

The changes include:

1. Changed a few comments for better understanding, deleted some unnecessary code, corrected some typos, and some minor code fixes from the review comments by Sashiko and Boris.

2. Unified sev_apic_ghcb_msr_write() and sev_apic_ghcb_msr_read().

3. Move x2apic_svsm.o after x2apic_savic.o so that the secure AVIC gets preferred on machines supporting it.

4. Reworked svsm_apic_probe() as Boris suggested.

5. Updated tools with renamed MSR_AMD64_SNP_ALTERNATE_INJ_BIT and MSR_AMD64_SNP_ALT_INJ for completeness' sake.

6. Changed svsm_apic_send_IPI_self() with writing to the dedicated APIC_SELF_IPI instead of writing to APIC_ICR.

v1
--

The revision fixes all of the review comments from version 0.

The changes include:

1. Changed a few commit messages, new file names, variables name and order, function names and more comments for better understanding.

2. Added a pre-patch to fix the problem that in two functions in the the SVSM vTPM guest implementation do not disable preemption when fetching CAA. It is moving the CAA fetching operation inside svsm_perform_call_protocol().

This series relies on this fix too.

3. Removed svsm_do_call() helper function and some intermediary layer functions in SVSM APIC driver.

4. Removed the Secure AVIC check in SVSM APIC driver since it will never be touched.

5. Terminate boot when Alternate Injection is enabled in vmpl0.

6. Added a common function for SVSM APIC read and write.

7. Added a new SEV_TERM_SET_LINUX value for Alternate Injection and APIC ghcb msr read/write.

8. Reworked sev_prepare() to reflect the error conditions correctly.

9. Moved allowing all of interrupts right after registering the SVSM APIC protocol.

v0
--

Alternate Injection is a method to provide secure interrupt delivery for SEV-SNP guests against malicious injection attacks. By handing over the control of the interrupt injection to the guest itself, the security is applied. Alternate Injection use Secure VM Service Module (SVSM), and APIC emulation in the SVSM to secure interrupt delivery.

This is the guest side patches. The patch set includes the following:
1. Add support for enabling Alternate Injection.
2. Add support for the SVSM APIC protocol which uses a subset of the X2APIC MSRs.
3. Add support to allow the guest OS to request Alternate Injection.

Melody Wang (8):
x86/sev: Make SVSM calls preemption-safe
x86/sev: Add support for Alternate Injection
x86/apic: Add an SVSM APIC driver
x86/sev: Route unsupported APIC register accesses to the hypervisor
APIC emulation
x86/sev: Add a function to contain all SEV-specific setup operations x86/sev: Register the guest with the SVSM APIC protocol x86/sev: Allow the guest to configure interrupt vectors for the
hypervisor
x86/sev: Indicate that Alternate Injection is supported in the guest

 arch/x86/Kconfig                        |  14 ++
 arch/x86/boot/compressed/sev.c          |  58 +++++-
 arch/x86/boot/compressed/sev.h          |  13 ++
 arch/x86/coco/core.c                    |   3 +
 arch/x86/coco/sev/core.c                |  48 ++---
 arch/x86/coco/sev/svsm.c                |  10 +-
 arch/x86/include/asm/msr-index.h        |   4 +-
 arch/x86/include/asm/sev-common.h       |   2 +
 arch/x86/include/asm/sev.h              |  21 +-
 arch/x86/kernel/apic/Makefile           |   1 +
 arch/x86/kernel/apic/x2apic_savic.c     |   8 +-
 arch/x86/kernel/apic/x2apic_svsm.c      | 243 ++++++++++++++++++++++++
 drivers/firmware/efi/libstub/x86-stub.c |   9 +-
 include/linux/cc_platform.h             |   8 +
 tools/arch/x86/include/asm/msr-index.h  |   4 +-
 15 files changed, 398 insertions(+), 48 deletions(-)
 create mode 100644 arch/x86/kernel/apic/x2apic_svsm.c
Signature · 1 line
2.43.0
#1 lore raw

PATCH v3 1/8 x86/sev: Make SVSM calls preemption-safe

Two functions in the the SVSM vTPM guest implementation do not disable preemption when fetching the SVSM Calling Area Address (CAA).

The SVSM CAA is a per-CPU structure. When a thread is preempted and migrated to a different CPU after fetching the per-CPU CAA, the SVSM call will execute on the new CPU with the original CPU's CAA. Which is wrong.

Move the CAA fetching operation inside svsm_perform_call_protocol() which disables interrupts around the SVSM call and thus runs preemption-safe.

Fixes 770de678bc28 ("x86/sev: Add SVSM vTPM probe/send_command functions")
---
 arch/x86/coco/sev/svsm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
arch/x86/coco/sev/svsm.c +8 -2
index 916d62cd17dc..2d493d55ff2e 100644
@@ -74,6 +74,14 @@ int svsm_perform_call_protocol(struct svsm_call *call)
 
 	flags = native_local_irq_save();
 
+	/*
+	 * 'caa' is a per-CPU variable. To avoid using a stale or incorrect
+	 * 'caa' if the task is preempted or migrated to another CPU after it
+	 * is fetched, always fetch 'caa' and then issue the SVSM call with
+	 * interrupts disabled. This ensures the correct 'caa' is used.
+	 */
+	call->caa = svsm_get_caa();
+
 	ghcb = __sev_get_ghcb(&state);
 
 	do {
@@ -321,7 +329,6 @@ int snp_svsm_vtpm_send_command(u8 *buffer)
 {
 	struct svsm_call call = {};
 
-	call.caa = svsm_get_caa();
 	call.rax = SVSM_VTPM_CALL(SVSM_VTPM_CMD);
 	call.rcx = __pa(buffer);
 
@@ -345,7 +352,6 @@ bool snp_svsm_vtpm_probe(void)
 	if (!snp_vmpl)
 		return false;
 
-	call.caa = svsm_get_caa();
 	call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
 
 	if (svsm_perform_call_protocol(&call))
Signature · 1 line
2.43.0
#2 lore raw

PATCH v3 2/8 x86/sev: Add support for Alternate Injection

In reply to Melody Wang

The Alternate Injection feature is a method to protect a SNP guest from malicious injection attacks. It allows the guest to control the interrupt injection.

When this feature is enabled, interrupts are injected with the help of an agent called a Secure VM Service Module (SVSM) which executes in the security realm of the guest and uses Restricted Injection as the sole method to receive interrupts from the hypervisor.

Add support for Alternate Injection.

---
 arch/x86/Kconfig                       | 14 ++++++++++++++
 arch/x86/boot/compressed/sev.c         |  2 +-
 arch/x86/coco/core.c                   |  3 +++
 arch/x86/coco/sev/core.c               |  2 +-
 arch/x86/include/asm/msr-index.h       |  4 ++--
 include/linux/cc_platform.h            |  8 ++++++++
 tools/arch/x86/include/asm/msr-index.h |  4 ++--
 7 files changed, 31 insertions(+), 6 deletions(-)
arch/x86/Kconfig +14 -0
index 5a84998b6021..330cf9245342 100644
@@ -496,6 +496,20 @@ config AMD_SECURE_AVIC
 
 	  If you don't know what to do here, say N.
 
+config AMD_ALTERNATE_INJ
+	bool "Support Alternate Injection"
+	depends on X86_X2APIC && AMD_MEM_ENCRYPT
+	help
+	  Enable AMD Alternate Injection support for SNP guests.
+
+	  The Alternate Injection feature of SEV-SNP enhances the security of
+	  a confidential VM by preventing the untrusted host from presenting
+	  unexpected interrupts or exceptions, while still preserving the
+	  standard interrupt dispatch semantics inherent to the x86
+	  architecture.
+
+	  If you don't know what to do here, say N.
+
 config X86_POSTED_MSI
 	bool "Enable MSI and MSI-x delivery by posted interrupts"
 	depends on X86_64 && IRQ_REMAP
arch/x86/boot/compressed/sev.c +1 -1
index c6512f2ea31e..fc2029746c50 100644
@@ -179,7 +179,7 @@ bool sev_es_check_ghcb_fault(unsigned long address)
 #define SNP_FEATURES_IMPL_REQ	(MSR_AMD64_SNP_VTOM |			\
 				 MSR_AMD64_SNP_REFLECT_VC |		\
 				 MSR_AMD64_SNP_RESTRICTED_INJ |		\
-				 MSR_AMD64_SNP_ALT_INJ |		\
+				 MSR_AMD64_SNP_ALTERNATE_INJ |		\
 				 MSR_AMD64_SNP_DEBUG_SWAP |		\
 				 MSR_AMD64_SNP_VMPL_SSS |		\
 				 MSR_AMD64_SNP_SECURE_TSC |		\
arch/x86/coco/core.c +3 -0
index 989ca9f72ba3..aabda3ddc0e2 100644
@@ -107,6 +107,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
 	case CC_ATTR_SNP_SECURE_AVIC:
 		return sev_status & MSR_AMD64_SNP_SECURE_AVIC;
 
+	case CC_ATTR_SNP_ALTERNATE_INJECTION:
+		return sev_status & MSR_AMD64_SNP_ALTERNATE_INJ;
+
 	default:
 		return false;
 	}
arch/x86/coco/sev/core.c +1 -1
index cc292d7c6fd1..c54269366278 100644
@@ -78,7 +78,7 @@ static const char * const sev_status_feat_names[] = {
 	[MSR_AMD64_SNP_VTOM_BIT]		= "vTom",
 	[MSR_AMD64_SNP_REFLECT_VC_BIT]		= "ReflectVC",
 	[MSR_AMD64_SNP_RESTRICTED_INJ_BIT]	= "RI",
-	[MSR_AMD64_SNP_ALT_INJ_BIT]		= "AI",
+	[MSR_AMD64_SNP_ALTERNATE_INJ_BIT]	= "AI",
 	[MSR_AMD64_SNP_DEBUG_SWAP_BIT]		= "DebugSwap",
 	[MSR_AMD64_SNP_PREVENT_HOST_IBS_BIT]	= "NoHostIBS",
 	[MSR_AMD64_SNP_BTB_ISOLATION_BIT]	= "BTBIsol",
arch/x86/include/asm/msr-index.h +2 -2
index 428eb44106a9..bedc9a62d0b4 100644
@@ -728,8 +728,8 @@
 #define MSR_AMD64_SNP_REFLECT_VC	BIT_ULL(MSR_AMD64_SNP_REFLECT_VC_BIT)
 #define MSR_AMD64_SNP_RESTRICTED_INJ_BIT 5
 #define MSR_AMD64_SNP_RESTRICTED_INJ	BIT_ULL(MSR_AMD64_SNP_RESTRICTED_INJ_BIT)
-#define MSR_AMD64_SNP_ALT_INJ_BIT	6
-#define MSR_AMD64_SNP_ALT_INJ		BIT_ULL(MSR_AMD64_SNP_ALT_INJ_BIT)
+#define MSR_AMD64_SNP_ALTERNATE_INJ_BIT	6
+#define MSR_AMD64_SNP_ALTERNATE_INJ	BIT_ULL(MSR_AMD64_SNP_ALTERNATE_INJ_BIT)
 #define MSR_AMD64_SNP_DEBUG_SWAP_BIT	7
 #define MSR_AMD64_SNP_DEBUG_SWAP	BIT_ULL(MSR_AMD64_SNP_DEBUG_SWAP_BIT)
 #define MSR_AMD64_SNP_PREVENT_HOST_IBS_BIT 8
include/linux/cc_platform.h +8 -0
index 559353ad64ac..80d9f1de2ae8 100644
@@ -104,6 +104,14 @@ enum cc_attr {
 	 * to run SEV-SNP guests with full Secure AVIC capabilities.
 	 */
 	CC_ATTR_SNP_SECURE_AVIC,
+
+	/**
+	 * @CC_ATTR_SNP_ALTERNATE_INJECTION: AMD Alternate Injection enabled.
+	 *
+	 * A Secure VM Service Module (SVSM) has started the guest with
+	 * Alternate Injection enabled.
+	 */
+	CC_ATTR_SNP_ALTERNATE_INJECTION,
 };
 
 #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
tools/arch/x86/include/asm/msr-index.h +2 -2
index 18c4be75e927..e4eab2210518 100644
@@ -721,8 +721,8 @@
 #define MSR_AMD64_SNP_REFLECT_VC	BIT_ULL(MSR_AMD64_SNP_REFLECT_VC_BIT)
 #define MSR_AMD64_SNP_RESTRICTED_INJ_BIT 5
 #define MSR_AMD64_SNP_RESTRICTED_INJ	BIT_ULL(MSR_AMD64_SNP_RESTRICTED_INJ_BIT)
-#define MSR_AMD64_SNP_ALT_INJ_BIT	6
-#define MSR_AMD64_SNP_ALT_INJ		BIT_ULL(MSR_AMD64_SNP_ALT_INJ_BIT)
+#define MSR_AMD64_SNP_ALTERNATE_INJ_BIT	6
+#define MSR_AMD64_SNP_ALTERNATE_INJ	BIT_ULL(MSR_AMD64_SNP_ALTERNATE_INJ_BIT)
 #define MSR_AMD64_SNP_DEBUG_SWAP_BIT	7
 #define MSR_AMD64_SNP_DEBUG_SWAP	BIT_ULL(MSR_AMD64_SNP_DEBUG_SWAP_BIT)
 #define MSR_AMD64_SNP_PREVENT_HOST_IBS_BIT 8
Signature · 1 line
2.43.0
#3 lore raw

PATCH v3 3/8 x86/apic: Add an SVSM APIC driver

In reply to Melody Wang

Alternate Injection replaces hypervisor-based interrupt queuing and event injection, requiring guest-controlled queuing and injection. In order to perform this guest-controlled queuing and injection, an SVSM is used to update the guest VMSA to perform the required actions.

The guest uses the SVSM APIC protocol to communicate with the SVSM to perform selected APIC related operations instead of using standard APIC MSR access.

Add such a SVSM APIC driver (which implements a subset of an X2APIC), for the APIC emulation supported by the SVSM.

---
 arch/x86/include/asm/sev-common.h  |   1 +
 arch/x86/include/asm/sev.h         |   9 ++
 arch/x86/kernel/apic/Makefile      |   1 +
 arch/x86/kernel/apic/x2apic_svsm.c | 236 +++++++++++++++++++++++++++++
 4 files changed, 247 insertions(+)
 create mode 100644 arch/x86/kernel/apic/x2apic_svsm.c
arch/x86/include/asm/sev-common.h +1 -0
index 01a6e4dbe423..095fbafe8a2c 100644
@@ -209,6 +209,7 @@ struct snp_psc_desc {
 #define GHCB_TERM_SECURE_TSC		10	/* Secure TSC initialization failed */
 #define GHCB_TERM_SVSM_CA_REMAP_FAIL	11	/* SVSM is present but CA could not be remapped */
 #define GHCB_TERM_SAVIC_FAIL		12	/* Secure AVIC-specific failure */
+#define GHCB_TERM_ALT_INJ_FAIL		13	/* Alternate Injection-specific failure */
 
 #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
 
arch/x86/include/asm/sev.h +9 -0
index 9e7a077c445d..557c4d1ef4be 100644
@@ -435,6 +435,13 @@ struct svsm_call {
 #define SVSM_VTPM_QUERY			0
 #define SVSM_VTPM_CMD			1
 
+#define SVSM_APIC_CALL(x)		((3ULL << 32) | (x))
+#define SVSM_APIC_QUERY_FEATURES	0
+#define SVSM_APIC_CONFIG_EMULATION	1
+#define SVSM_APIC_READ_REGISTER		2
+#define SVSM_APIC_WRITE_REGISTER	3
+#define SVSM_APIC_CONFIG_VECTOR		4
+
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 
 extern u8 snp_vmpl;
@@ -528,6 +535,7 @@ void snp_msg_free(struct snp_msg_desc *mdesc);
 int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req);
 
 int snp_svsm_vtpm_send_command(u8 *buffer);
+int svsm_perform_call_protocol(struct svsm_call *call);
 
 void __init snp_secure_tsc_prepare(void);
 void __init snp_secure_tsc_init(void);
@@ -636,6 +644,7 @@ static inline void snp_msg_free(struct snp_msg_desc *mdesc) { }
 static inline int snp_send_guest_request(struct snp_msg_desc *mdesc,
 					 struct snp_guest_req *req) { return -ENODEV; }
 static inline int snp_svsm_vtpm_send_command(u8 *buffer) { return -ENODEV; }
+static inline int svsm_perform_call_protocol(struct svsm_call *call) { return -ENODEV; }
 static inline void __init snp_secure_tsc_prepare(void) { }
 static inline void __init snp_secure_tsc_init(void) { }
 static inline void sev_evict_cache(void *va, int npages) {}
arch/x86/kernel/apic/Makefile +1 -0
index 581db89477f9..fef51024025b 100644
@@ -19,6 +19,7 @@ ifeq ($(CONFIG_X86_64),y)
 obj-$(CONFIG_X86_NUMACHIP)	+= apic_numachip.o
 obj-$(CONFIG_X86_UV)		+= x2apic_uv_x.o
 obj-$(CONFIG_AMD_SECURE_AVIC)	+= x2apic_savic.o
+obj-$(CONFIG_AMD_ALTERNATE_INJ)	+= x2apic_svsm.o
 obj-$(CONFIG_X86_X2APIC)	+= x2apic_phys.o
 obj-$(CONFIG_X86_X2APIC)	+= x2apic_cluster.o
 obj-y				+= apic_flat_64.o
arch/x86/kernel/apic/x2apic_svsm.c new file +236 -0

236 lines not on this page · raw message

Signature · 1 line
2.43.0
#4 lore raw

Re: [PATCH v3 3/8] x86/apic: Add an SVSM APIC driver

Hi all,

Quoted 20 lines · Melody Wang
arch/x86/kernel/apic/x2apic_svsm.c new file +20 -0
+
+static u32 __svsm_apic_msr_rw(u32 reg, u32 v, bool write)
+{
+	u32 msr = APIC_BASE_MSR + (reg >> 4);
+	struct svsm_call call = {};
+	const char *call_reg_str;
+	unsigned int call_reg;
+	int ret;
+
+	call_reg = write ? SVSM_APIC_WRITE_REGISTER
+			 : SVSM_APIC_READ_REGISTER;
+
+	call_reg_str = write ? "SVSM_APIC_WRITE_REGISTER"
+			     : "SVSM_APIC_READ_REGISTER";
+
+	switch (reg) {
+	case APIC_ID:
+	case APIC_TASKPRI:
+	case APIC_PROCPRI:
+	case APIC_EOI:

arch/x86/kernel/apic/x2apic_svsm.c line 77

Quoted 10 lines
arch/x86/kernel/apic/x2apic_svsm.c new file +10 -0
+	case APIC_ISR ... APIC_ISR + 0x70:
+	case APIC_TMR ... APIC_TMR + 0x70:
+	case APIC_IRR ... APIC_IRR + 0x70:
+	case APIC_ICR:
+	case APIC_SELF_IPI:
+		call.rax = SVSM_APIC_CALL(call_reg);
+		call.rcx = msr;
+		call.rdx = v;
+
+		ret = svsm_perform_call_protocol(&call);

Sashiko said this:

"Can this SVSM APIC call be interrupted by an NMI, causing CAA page corruption and guest termination?
If a normal thread or IRQ handler executes an SVSM APIC operation and an NMI fires exactly between setting call_pending to 1 and the vmmcall instruction inside svsm_issue_call(), the NMI handler might execute another APIC operation.
The nested SVSM call from the NMI would use the exact same per-CPU Calling Area Address (CAA) page, clearing call_pending to 0. When the interrupted call resumes, svsm_process_result_codes() would misinterpret the unmodified input registers as an error code because call_pending is 0. Does this result in ret being non-zero and triggering sev_es_terminate() fatally crashing the guest?"

This will happen when there is APIC operation happening in the NMI handler, there is no such case so I would keep the code as it is for now unless there is some use case that needs us to do an APIC operation in the NMI handler in the future.

Thanks,
Melody

maintainer
#5 lore raw

Re: [PATCH v3 3/8] x86/apic: Add an SVSM APIC driver

In reply to Melody Wang
Quoted 11 lines · Melody Wang

Alternate Injection replaces hypervisor-based interrupt queuing and event injection, requiring guest-controlled queuing and injection. In order to perform this guest-controlled queuing and injection, an SVSM is used to update the guest VMSA to perform the required actions.

The guest uses the SVSM APIC protocol to communicate with the SVSM to perform selected APIC related operations instead of using standard APIC MSR access.

Add such a SVSM APIC driver (which implements a subset of an X2APIC), for the APIC emulation supported by the SVSM.

Simplified the commit message a bit:

"Alternate Injection replaces hypervisor-based interrupt queuing and event injection in favor of guest-controlled queuing and injection. In order to accomplish that, an SVSM is used to update the guest VMSA to perform the required actions.

The guest uses the SVSM APIC protocol to communicate with the SVSM to perform selected APIC-related operations instead of using standard APIC MSR accesses.

Add a such SVSM APIC driver (which implements a subset of the X2APIC interface), for the APIC emulation supported by the SVSM."

Quoted 9 lines
---
 arch/x86/include/asm/sev-common.h  |   1 +
 arch/x86/include/asm/sev.h         |   9 ++
 arch/x86/kernel/apic/Makefile      |   1 +
 arch/x86/kernel/apic/x2apic_svsm.c | 236 +++++++++++++++++++++++++++++
 4 files changed, 247 insertions(+)
 create mode 100644 arch/x86/kernel/apic/x2apic_svsm.c

...

Quoted 28 lines
arch/x86/kernel/apic/x2apic_svsm.c new file +28 -0
+static inline u64 svsm_apic_icr_read(void)
+{
+	struct svsm_call call = {};
+	u32 reg;
+	int ret;
+
+	reg = APIC_ICR;
+
+	call.rax = SVSM_APIC_CALL(SVSM_APIC_READ_REGISTER);
+	call.rcx = APIC_BASE_MSR + (reg >> 4);
+
+	ret = svsm_perform_call_protocol(&call);
+	if (ret) {
+		pr_err("%s: error: %d\n", __func__, ret);
+		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
+	}
+
+	return call.rdx_out;
+}
+
+static void svsm_apic_icr_write(u32 low, u32 id)
+{
+	struct svsm_call call = {};
+	u32 reg;
+	int ret;
+
+	reg = APIC_ICR;
+

arch/x86/kernel/apic/x2apic_svsm.c line 144

Quoted 10 lines
arch/x86/kernel/apic/x2apic_svsm.c new file +10 -0
+	call.rax = SVSM_APIC_CALL(SVSM_APIC_WRITE_REGISTER);
+	call.rcx = APIC_BASE_MSR + (reg >> 4);
+	call.rdx = ((u64)id) << 32 | low;
+
+	ret = svsm_perform_call_protocol(&call);
+	if (ret) {
+		pr_err("%s: error: %d\n", __func__, ret);
+		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
+	}
+}

Those two look very similar now and since we're unifying them, you probably should unify those two too.

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
#6 lore raw

PATCH v3 4/8 x86/sev: Route unsupported APIC register accesses to the hypervisor APIC emulation

In reply to Melody Wang

The SVSM APIC emulation supports only a subset of the X2APIC MSRs. Right now only basic APIC functionality related to interrupt delivery is supported. Therefore, route the unsupported ones to the hypervisor's X2APIC emulation.

---
 arch/x86/coco/sev/core.c            | 46 +++++++++++------------------
 arch/x86/include/asm/sev-common.h   |  1 +
 arch/x86/include/asm/sev.h          |  8 ++---
 arch/x86/kernel/apic/x2apic_savic.c |  8 ++---
 arch/x86/kernel/apic/x2apic_svsm.c  | 11 +++++--
 5 files changed, 35 insertions(+), 39 deletions(-)
arch/x86/coco/sev/core.c +17 -29
index c54269366278..22956fab1581 100644
@@ -972,7 +972,7 @@ int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd)
 	return 0;
 }
 
-u64 savic_ghcb_msr_read(u32 reg)
+static u64 __sev_apic_ghcb_msr_rw(u32 reg, u64 value, bool write)
 {
 	u64 msr = APIC_BASE_MSR + (reg >> 4);
 	struct pt_regs regs = { .cx = msr };
@@ -981,49 +981,37 @@ u64 savic_ghcb_msr_read(u32 reg)
 	enum es_result res;
 	struct ghcb *ghcb;
 
+	if (write) {
+		regs.ax = lower_32_bits(value);
+		regs.dx = upper_32_bits(value);
+	}
+
 	guard(irqsave)();
 
 	ghcb = __sev_get_ghcb(&state);
 	vc_ghcb_invalidate(ghcb);
 
-	res = __vc_handle_msr(ghcb, &ctxt, false);
+	res = __vc_handle_msr(ghcb, &ctxt, write);
 	if (res != ES_OK) {
-		pr_err("Secure AVIC MSR (0x%llx) read returned error (%d)\n", msr, res);
-		/* MSR read failures are treated as fatal errors */
-		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SAVIC_FAIL);
+		pr_err("APIC MSR via GHCB (0x%llx) %s returned error (%d)\n", msr, write ? "write" : "read", res);
+		/* MSR read/write failures are treated as fatal errors */
+		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_APIC_MSR_FAIL);
 	}
 
 	__sev_put_ghcb(&state);
 
+	/* The return value will be ignored by the caller for a write */
 	return regs.ax | regs.dx << 32;
 }
 
-void savic_ghcb_msr_write(u32 reg, u64 value)
+u64 sev_apic_ghcb_msr_read(u32 reg)
 {
-	u64 msr = APIC_BASE_MSR + (reg >> 4);
-	struct pt_regs regs = {
-		.cx = msr,
-		.ax = lower_32_bits(value),
-		.dx = upper_32_bits(value)
-	};
-	struct es_em_ctxt ctxt = { .regs = &regs };
-	struct ghcb_state state;
-	enum es_result res;
-	struct ghcb *ghcb;
-
-	guard(irqsave)();
-
-	ghcb = __sev_get_ghcb(&state);
-	vc_ghcb_invalidate(ghcb);
-
-	res = __vc_handle_msr(ghcb, &ctxt, true);
-	if (res != ES_OK) {
-		pr_err("Secure AVIC MSR (0x%llx) write returned error (%d)\n", msr, res);
-		/* MSR writes should never fail. Any failure is fatal error for SNP guest */
-		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SAVIC_FAIL);
-	}
+	return __sev_apic_ghcb_msr_rw(reg, 0, false);
+}
 
-	__sev_put_ghcb(&state);
+void sev_apic_ghcb_msr_write(u32 reg, u64 value)
+{
+	__sev_apic_ghcb_msr_rw(reg, value, true);
 }
 
 enum es_result savic_register_gpa(u64 gpa)
arch/x86/include/asm/sev-common.h +1 -0
index 095fbafe8a2c..585ddc44b6b4 100644
@@ -210,6 +210,7 @@ struct snp_psc_desc {
 #define GHCB_TERM_SVSM_CA_REMAP_FAIL	11	/* SVSM is present but CA could not be remapped */
 #define GHCB_TERM_SAVIC_FAIL		12	/* Secure AVIC-specific failure */
 #define GHCB_TERM_ALT_INJ_FAIL		13	/* Alternate Injection-specific failure */
+#define GHCB_TERM_APIC_MSR_FAIL		14	/* APIC MSR failure */
 
 #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
 
arch/x86/include/asm/sev.h +4 -4
index 557c4d1ef4be..3cb6c5d6a6e0 100644
@@ -541,8 +541,8 @@ void __init snp_secure_tsc_prepare(void);
 void __init snp_secure_tsc_init(void);
 enum es_result savic_register_gpa(u64 gpa);
 enum es_result savic_unregister_gpa(u64 *gpa);
-u64 savic_ghcb_msr_read(u32 reg);
-void savic_ghcb_msr_write(u32 reg, u64 value);
+u64 sev_apic_ghcb_msr_read(u32 reg);
+void sev_apic_ghcb_msr_write(u32 reg, u64 value);
 
 static __always_inline void vc_ghcb_invalidate(struct ghcb *ghcb)
 {
@@ -650,8 +650,8 @@ static inline void __init snp_secure_tsc_init(void) { }
 static inline void sev_evict_cache(void *va, int npages) {}
 static inline enum es_result savic_register_gpa(u64 gpa) { return ES_UNSUPPORTED; }
 static inline enum es_result savic_unregister_gpa(u64 *gpa) { return ES_UNSUPPORTED; }
-static inline void savic_ghcb_msr_write(u32 reg, u64 value) { }
-static inline u64 savic_ghcb_msr_read(u32 reg) { return 0; }
+static inline void sev_apic_ghcb_msr_write(u32 reg, u64 value) { }
+static inline u64 sev_apic_ghcb_msr_read(u32 reg) { return 0; }
 
 #endif	/* CONFIG_AMD_MEM_ENCRYPT */
 
arch/x86/kernel/apic/x2apic_savic.c +4 -4
index 4bc6d7e018a5..990e4a379e07 100644
@@ -72,7 +72,7 @@ static u32 savic_read(u32 reg)
 	case APIC_LVT0:
 	case APIC_LVT1:
 	case APIC_LVTERR:
-		return savic_ghcb_msr_read(reg);
+		return sev_apic_ghcb_msr_read(reg);
 	case APIC_ID:
 	case APIC_LVR:
 	case APIC_TASKPRI:
@@ -193,7 +193,7 @@ static void savic_icr_write(u32 icr_low, u32 icr_high)
 
 	icr_data = ((u64)icr_high) << 32 | icr_low;
 	if (dsh != APIC_DEST_SELF)
-		savic_ghcb_msr_write(APIC_ICR, icr_data);
+		sev_apic_ghcb_msr_write(APIC_ICR, icr_data);
 	apic_set_reg64(this_cpu_ptr(savic_page), APIC_ICR, icr_data);
 }
 
@@ -210,7 +210,7 @@ static void savic_write(u32 reg, u32 data)
 	case APIC_LVTTHMR:
 	case APIC_LVTPC:
 	case APIC_LVTERR:
-		savic_ghcb_msr_write(reg, data);
+		sev_apic_ghcb_msr_write(reg, data);
 		break;
 	case APIC_TASKPRI:
 	case APIC_EOI:
@@ -316,7 +316,7 @@ static void savic_eoi(void)
 		 * interrupts. Return to the guest from GHCB protocol event takes
 		 * care of re-evaluating interrupt state.
 		 */
-		savic_ghcb_msr_write(APIC_EOI, 0);
+		sev_apic_ghcb_msr_write(APIC_EOI, 0);
 	} else {
 		/*
 		 * Hardware clears APIC_ISR and re-evaluates the interrupt state
arch/x86/kernel/apic/x2apic_svsm.c +9 -2
index d72bc7962a66..9ab49d360219 100644
@@ -81,8 +81,15 @@ static u32 __svsm_apic_msr_rw(u32 reg, u32 v, bool write)
 		}
 		break;
 	default:
-		pr_err("%s 0x%x not supported\n", call_reg_str, reg);
-		return 0;
+		if (write) {
+			/*
+			 * Even if this will end up returning call.rdx_out,
+			 * that is being ignored by the caller for a write.
+			 */
+			sev_apic_ghcb_msr_write(reg, v);
+		} else {
+			return sev_apic_ghcb_msr_read(reg);
+		}
 	}
 
 	return call.rdx_out;
Signature · 1 line
2.43.0
#7 lore raw

PATCH v3 5/8 x86/sev: Add a function to contain all SEV-specific setup operations

In reply to Melody Wang

To make the code clean in the boot phase, add a sev_prepare() wrapper which contains early SEV-specific checks in order to have all that code in a single place.

No functional changes.

---
 drivers/firmware/efi/libstub/x86-stub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/firmware/efi/libstub/x86-stub.c +2 -2
index cef32e2c82d8..49388524a61f 100644
@@ -783,7 +783,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
 	return EFI_SUCCESS;
 }
 
-static bool have_unsupported_snp_features(void)
+static bool sev_prepare(void)
 {
 	u64 unsupported;
 
@@ -933,7 +933,7 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 
 	hdr = &boot_params->hdr;
 
-	if (have_unsupported_snp_features())
+	if (sev_prepare())
 		efi_exit(handle, EFI_UNSUPPORTED);
 
 	if (IS_ENABLED(CONFIG_EFI_DXE_MEM_ATTRIBUTES)) {
Signature · 1 line
2.43.0
#8 lore raw

PATCH v3 6/8 x86/sev: Register the guest with the SVSM APIC protocol

In reply to Melody Wang

The SVSM APIC protocol supports 5 calls. SVSM_APIC_CONFIGURE_EMULATION (shortened to SVSM_APIC_CONFIG_EMULATION for brevity), call 1, provides the controls whether the guest can make use of the SVSM APIC protocol.

Implement this call, and register Alternate Injection for the guest by default.

---
 arch/x86/boot/compressed/sev.c          | 23 +++++++++++++++++++++++
 arch/x86/boot/compressed/sev.h          |  6 ++++++
 arch/x86/include/asm/sev.h              |  2 ++
 drivers/firmware/efi/libstub/x86-stub.c |  3 +++
 4 files changed, 34 insertions(+)
arch/x86/boot/compressed/sev.c +23 -0
index fc2029746c50..9f776ba7f123 100644
@@ -511,3 +511,26 @@ bool early_is_sevsnp_guest(void)
 	}
 	return true;
 }
+
+/*
+ * When the guest is running at VMPL2 with Alternate Injection enabled,
+ * register Alternate Injection before ExitBootServices().
+ */
+void svsm_register_alt_inj(void)
+{
+	if (early_is_sevsnp_guest() && snp_vmpl) {
+		struct svsm_call call = {};
+		int ret;
+
+		if (!(sev_get_status() & MSR_AMD64_SNP_ALTERNATE_INJ))
+			return;
+
+		call.caa = (struct svsm_ca *)boot_svsm_caa_pa;
+		call.rax = SVSM_APIC_CALL(SVSM_APIC_CONFIG_EMULATION);
+		call.rcx = SVSM_AI_REGISTER;
+
+		ret = svsm_call_msr_protocol(&call);
+		if (ret)
+			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
+	}
+}
arch/x86/boot/compressed/sev.h +6 -0
index 22637b416b46..dd058af2e7aa 100644
@@ -12,6 +12,12 @@
 
 #include <asm/shared/msr.h>
 
+enum svsm_ai_ctrl {
+	SVSM_AI_DISABLE,
+	SVSM_AI_DEREGISTER,
+	SVSM_AI_REGISTER,
+};
+
 void snp_accept_memory(phys_addr_t start, phys_addr_t end);
 u64 sev_get_status(void);
 bool early_is_sevsnp_guest(void);
arch/x86/include/asm/sev.h +2 -0
index 3cb6c5d6a6e0..c1e31fc9c918 100644
@@ -543,6 +543,7 @@ enum es_result savic_register_gpa(u64 gpa);
 enum es_result savic_unregister_gpa(u64 *gpa);
 u64 sev_apic_ghcb_msr_read(u32 reg);
 void sev_apic_ghcb_msr_write(u32 reg, u64 value);
+void svsm_register_alt_inj(void);
 
 static __always_inline void vc_ghcb_invalidate(struct ghcb *ghcb)
 {
@@ -652,6 +653,7 @@ static inline enum es_result savic_register_gpa(u64 gpa) { return ES_UNSUPPORTED
 static inline enum es_result savic_unregister_gpa(u64 *gpa) { return ES_UNSUPPORTED; }
 static inline void sev_apic_ghcb_msr_write(u32 reg, u64 value) { }
 static inline u64 sev_apic_ghcb_msr_read(u32 reg) { return 0; }
+static inline void svsm_register_alt_inj(void) { }
 
 #endif	/* CONFIG_AMD_MEM_ENCRYPT */
 
drivers/firmware/efi/libstub/x86-stub.c +3 -0
index 49388524a61f..35b8641119b9 100644
@@ -793,6 +793,9 @@ static bool sev_prepare(void)
 			unsupported);
 		return true;
 	}
+
+	svsm_register_alt_inj();
+
 	return false;
 }
 
Signature · 1 line
2.43.0
#9 lore raw

Re: [PATCH v3 6/8] x86/sev: Register the guest with the SVSM APIC protocol

Hi all,

drivers/firmware/efi/libstub/x86-stub.c static bool sev_prepare(void) · line 798

Quoted 11 lines · Melody Wang
drivers/firmware/efi/libstub/x86-stub.c +3 -0
index 49388524a61f..35b8641119b9 100644
@@ -793,6 +793,9 @@ static bool sev_prepare(void)
  			unsupported);
  		return true;
  	}
+
+	svsm_register_alt_inj();
+

Sashiko said this:

"Does this code handle non-EFI direct kernel boots correctly? When an SEV-SNP guest is booted directly and bypasses the EFI stub (such as when using QEMU's -kernel with a setup_data CC blob), sev_prepare() is skipped entirely.
This completely bypasses the svsm_register_alt_inj() call.
The main kernel checks for Alternate Injection support via cc_platform_has(CC_ATTR_SNP_ALTERNATE_INJECTION). This check relies strictly on the sev_status MSR hardware bit set by the SVSM and ignores whether the guest successfully registered the APIC emulation. Could this cause the APIC probe to issue SVSM APIC calls via __svsm_apic_msr_rw() that are rejected by the SVSM, leading to an unconditional crash via sev_es_terminate() early in the boot sequence?"

Alternate Injection does not work without OVMF support so no need to handle the non-EFI direct kernel boots.

Thanks,
Melody

Quoted 3 lines
drivers/firmware/efi/libstub/x86-stub.c +0 -0
  	return false;
  }
 
Signature · 2 lines
Thanks,
Melody
#10 lore raw

PATCH v3 7/8 x86/sev: Allow the guest to configure interrupt vectors for the hypervisor

In reply to Melody Wang

The SVSM APIC protocol supports 5 API calls. SVSM_APIC_CONFIGURE_VECTOR (shortened to SVSM_APIC_CONFIG_VECTOR for brevity), call 4, provides for the guest to configure an interrupt vector which the guest allows and the hypervisor can use to signal interrupts for it.

Implement this call, and make the default interrupt setting permissive to allow all interrupts when detecting an SVSM and Alternate Injection is enabled.

---
 arch/x86/boot/compressed/sev.c          | 24 ++++++++++++++++++++++++
 arch/x86/boot/compressed/sev.h          |  7 +++++++
 arch/x86/include/asm/sev.h              |  2 ++
 drivers/firmware/efi/libstub/x86-stub.c |  2 ++
 4 files changed, 35 insertions(+)
arch/x86/boot/compressed/sev.c +24 -0
index 9f776ba7f123..6ab6276f731f 100644
@@ -534,3 +534,27 @@ void svsm_register_alt_inj(void)
 			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
 	}
 }
+
+/*
+ * Configure all interrupts as permissive by default for Alternate Injection
+ * enabled SNP guest.
+ */
+void svsm_config_alt_inj_vectors(void)
+{
+	if (early_is_sevsnp_guest() && snp_vmpl) {
+		struct svsm_call call = {};
+		int ret;
+
+		if (!(sev_get_status() & MSR_AMD64_SNP_ALTERNATE_INJ))
+			return;
+
+		call.caa = (struct svsm_ca *)boot_svsm_caa_pa;
+		call.rax = SVSM_APIC_CALL(SVSM_APIC_CONFIG_VECTOR);
+		call.rcx = SVSM_IRQ_ENABLE_ALL;
+
+		ret = svsm_call_msr_protocol(&call);
+		if (ret)
+			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
+	}
+
+}
arch/x86/boot/compressed/sev.h +7 -0
index dd058af2e7aa..035a7b46085b 100644
@@ -18,6 +18,13 @@ enum svsm_ai_ctrl {
 	SVSM_AI_REGISTER,
 };
 
+enum svsm_vec_enable {
+	SVSM_IRQ_DISABLE_SINGLE = (0u << 8),
+	SVSM_IRQ_ENABLE_SINGLE = (1u << 8),
+	SVSM_IRQ_DISABLE_ALL = (2u << 8),
+	SVSM_IRQ_ENABLE_ALL = (3u << 8),
+};
+
 void snp_accept_memory(phys_addr_t start, phys_addr_t end);
 u64 sev_get_status(void);
 bool early_is_sevsnp_guest(void);
arch/x86/include/asm/sev.h +2 -0
index c1e31fc9c918..a6d4e5ea0913 100644
@@ -544,6 +544,7 @@ enum es_result savic_unregister_gpa(u64 *gpa);
 u64 sev_apic_ghcb_msr_read(u32 reg);
 void sev_apic_ghcb_msr_write(u32 reg, u64 value);
 void svsm_register_alt_inj(void);
+void svsm_config_alt_inj_vectors(void);
 
 static __always_inline void vc_ghcb_invalidate(struct ghcb *ghcb)
 {
@@ -654,6 +655,7 @@ static inline enum es_result savic_unregister_gpa(u64 *gpa) { return ES_UNSUPPOR
 static inline void sev_apic_ghcb_msr_write(u32 reg, u64 value) { }
 static inline u64 sev_apic_ghcb_msr_read(u32 reg) { return 0; }
 static inline void svsm_register_alt_inj(void) { }
+static inline void svsm_config_alt_inj_vectors(void) { }
 
 #endif	/* CONFIG_AMD_MEM_ENCRYPT */
 
drivers/firmware/efi/libstub/x86-stub.c +2 -0
index 35b8641119b9..7082fa10d5e6 100644
@@ -796,6 +796,8 @@ static bool sev_prepare(void)
 
 	svsm_register_alt_inj();
 
+	svsm_config_alt_inj_vectors();
+
 	return false;
 }
 
Signature · 1 line
2.43.0
#11 lore raw

Re: [PATCH v3 7/8] x86/sev: Allow the guest to configure interrupt vectors for the hypervisor

Hi all,

Quoted 92 lines · Melody Wang

The SVSM APIC protocol supports 5 API calls. SVSM_APIC_CONFIGURE_VECTOR (shortened to SVSM_APIC_CONFIG_VECTOR for brevity), call 4, provides for the guest to configure an interrupt vector which the guest allows and the hypervisor can use to signal interrupts for it.

Implement this call, and make the default interrupt setting permissive to allow all interrupts when detecting an SVSM and Alternate Injection is enabled.

---
  arch/x86/boot/compressed/sev.c          | 24 ++++++++++++++++++++++++
  arch/x86/boot/compressed/sev.h          |  7 +++++++
  arch/x86/include/asm/sev.h              |  2 ++
  drivers/firmware/efi/libstub/x86-stub.c |  2 ++
  4 files changed, 35 insertions(+)
arch/x86/boot/compressed/sev.c +24 -0
index 9f776ba7f123..6ab6276f731f 100644
@@ -534,3 +534,27 @@ void svsm_register_alt_inj(void)
  			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
  	}
  }
+
+/*
+ * Configure all interrupts as permissive by default for Alternate Injection
+ * enabled SNP guest.
+ */
+void svsm_config_alt_inj_vectors(void)
+{
+	if (early_is_sevsnp_guest() && snp_vmpl) {
+		struct svsm_call call = {};
+		int ret;
+
+		if (!(sev_get_status() & MSR_AMD64_SNP_ALTERNATE_INJ))
+			return;
+
+		call.caa = (struct svsm_ca *)boot_svsm_caa_pa;
+		call.rax = SVSM_APIC_CALL(SVSM_APIC_CONFIG_VECTOR);
+		call.rcx = SVSM_IRQ_ENABLE_ALL;
+
+		ret = svsm_call_msr_protocol(&call);
+		if (ret)
+			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
+	}
+
+}
arch/x86/boot/compressed/sev.h +7 -0
index dd058af2e7aa..035a7b46085b 100644
@@ -18,6 +18,13 @@ enum svsm_ai_ctrl {
  	SVSM_AI_REGISTER,
  };
 
+enum svsm_vec_enable {
+	SVSM_IRQ_DISABLE_SINGLE = (0u << 8),
+	SVSM_IRQ_ENABLE_SINGLE = (1u << 8),
+	SVSM_IRQ_DISABLE_ALL = (2u << 8),
+	SVSM_IRQ_ENABLE_ALL = (3u << 8),
+};
+
  void snp_accept_memory(phys_addr_t start, phys_addr_t end);
  u64 sev_get_status(void);
  bool early_is_sevsnp_guest(void);
arch/x86/include/asm/sev.h +2 -0
index c1e31fc9c918..a6d4e5ea0913 100644
@@ -544,6 +544,7 @@ enum es_result savic_unregister_gpa(u64 *gpa);
  u64 sev_apic_ghcb_msr_read(u32 reg);
  void sev_apic_ghcb_msr_write(u32 reg, u64 value);
  void svsm_register_alt_inj(void);
+void svsm_config_alt_inj_vectors(void);
 
  static __always_inline void vc_ghcb_invalidate(struct ghcb *ghcb)
  {
@@ -654,6 +655,7 @@ static inline enum es_result savic_unregister_gpa(u64 *gpa) { return ES_UNSUPPOR
  static inline void sev_apic_ghcb_msr_write(u32 reg, u64 value) { }
  static inline u64 sev_apic_ghcb_msr_read(u32 reg) { return 0; }
  static inline void svsm_register_alt_inj(void) { }
+static inline void svsm_config_alt_inj_vectors(void) { }
 
  #endif	/* CONFIG_AMD_MEM_ENCRYPT */
 
drivers/firmware/efi/libstub/x86-stub.c +0 -0
index 35b8641119b9..7082fa10d5e6 100644

drivers/firmware/efi/libstub/x86-stub.c static bool sev_prepare(void) · line 800

Quoted 5 lines
drivers/firmware/efi/libstub/x86-stub.c +2 -0
@@ -796,6 +796,8 @@ static bool sev_prepare(void)
 
  	svsm_register_alt_inj();
 
+	svsm_config_alt_inj_vectors();
+

Sashiko said this:

"Does this configuration need to happen for non-EFI direct boots as well? Since svsm_register_alt_inj() and svsm_config_alt_inj_vectors() are exclusively called from sev_prepare() inside the EFI stub, non-EFI direct boots (e.g., passing the CC blob via find_cc_blob_setup_data()) will bypass this code.
In a non-EFI boot, Alternate Injection is never registered with the SVSM. However, svsm_apic_probe() will still successfully probe and enable svsm_apic because MSR_AMD64_SNP_ALTERNATE_INJ remains set in the hardware status MSR provided by SVSM.
Will this cause the first APIC operation by the kernel (e.g., svsm_apic_msr_write()) to be routed to SVSM, where the unregistered call is rejected and triggers sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL), leading to a deterministic boot crash?"

Alternate Injection does not work without OVMF support so no need to handle the non-EFI direct kernel boots.

Thanks,
Melody

Quoted 3 lines
drivers/firmware/efi/libstub/x86-stub.c +0 -0
  	return false;
  }
 
Signature · 2 lines
Thanks,
Melody
#12 lore raw

PATCH v3 8/8 x86/sev: Indicate that Alternate Injection is supported in the guest

In reply to Melody Wang

Now the Alternate Injection support is complete, add it to the present SNP features.

---
 arch/x86/boot/compressed/sev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/x86/boot/compressed/sev.c +8 -1
index 6ab6276f731f..62b1e54988ac 100644
@@ -197,6 +197,12 @@ bool sev_es_check_ghcb_fault(unsigned long address)
 #define SNP_FEATURE_SECURE_AVIC		0
 #endif
 
+#ifdef CONFIG_AMD_ALTERNATE_INJ
+#define SNP_FEATURE_ALTERNATE_INJ	MSR_AMD64_SNP_ALTERNATE_INJ
+#else
+#define SNP_FEATURE_ALTERNATE_INJ	0
+#endif
+
 /*
  * SNP_FEATURES_IMPL is the mask of SNP features that are implemented
  * by the guest kernel. As and when a new feature is implemented in the
@@ -204,7 +210,8 @@ bool sev_es_check_ghcb_fault(unsigned long address)
  */
 #define SNP_FEATURES_IMPL	(MSR_AMD64_SNP_DEBUG_SWAP |	\
 				 MSR_AMD64_SNP_SECURE_TSC |	\
-				 SNP_FEATURE_SECURE_AVIC)
+				 SNP_FEATURE_SECURE_AVIC |	\
+				 SNP_FEATURE_ALTERNATE_INJ)
 
 u64 snp_get_unsupported_features(u64 status)
 {
Signature · 1 line
2.43.0
maintainer
#13 lore raw
In reply to Melody Wang

Hi all,

Quoted 8 lines · Melody Wang

Hi all,

This revision addresses all review comments from version 2.

The changes include:

1. Moved sev_prepare() from decompressor to EFI stub.

This may be a bit of a 'back to the drawing board' remark, but if opting in to this alternate injection scheme can only be done from the EFI firmware before ExitBootServices(), couldn't we wrap all this logic into an OVMF protocol instead? That way, the EFI stub can just invoke it if it exists, and what happens under the hood as a result is entirely under the control of the firmware.

Quoted 117 lines

2. Changed "Register Alternate Injection" and "configure all interrupt vectors" as two APIs located in compressed/sev.c since they are self-contained, avoiding exporting functions.

3. Added more comments for better understanding.

Thanks,
Melody

Changelog:
v2
--

This revision fixes all the review comments from version 1.

The changes include:

1. Changed a few comments for better understanding, deleted some unnecessary code, corrected some typos, and some minor code fixes from the review comments by Sashiko and Boris.

2. Unified sev_apic_ghcb_msr_write() and sev_apic_ghcb_msr_read().

3. Move x2apic_svsm.o after x2apic_savic.o so that the secure AVIC gets preferred on machines supporting it.

4. Reworked svsm_apic_probe() as Boris suggested.

5. Updated tools with renamed MSR_AMD64_SNP_ALTERNATE_INJ_BIT and MSR_AMD64_SNP_ALT_INJ for completeness' sake.

6. Changed svsm_apic_send_IPI_self() with writing to the dedicated APIC_SELF_IPI instead of writing to APIC_ICR.

v1
--

The revision fixes all of the review comments from version 0.

The changes include:

1. Changed a few commit messages, new file names, variables name and order, function names and more comments for better understanding.

2. Added a pre-patch to fix the problem that in two functions in the the SVSM vTPM guest implementation do not disable preemption when fetching CAA. It is moving the CAA fetching operation inside svsm_perform_call_protocol().

This series relies on this fix too.

3. Removed svsm_do_call() helper function and some intermediary layer functions in SVSM APIC driver.

4. Removed the Secure AVIC check in SVSM APIC driver since it will never be touched.

5. Terminate boot when Alternate Injection is enabled in vmpl0.

6. Added a common function for SVSM APIC read and write.

7. Added a new SEV_TERM_SET_LINUX value for Alternate Injection and APIC ghcb msr read/write.

8. Reworked sev_prepare() to reflect the error conditions correctly.

9. Moved allowing all of interrupts right after registering the SVSM APIC protocol.

v0
--

Alternate Injection is a method to provide secure interrupt delivery for SEV-SNP guests against malicious injection attacks. By handing over the control of the interrupt injection to the guest itself, the security is applied. Alternate Injection use Secure VM Service Module (SVSM), and APIC emulation in the SVSM to secure interrupt delivery.

This is the guest side patches. The patch set includes the following:
1. Add support for enabling Alternate Injection.
2. Add support for the SVSM APIC protocol which uses a subset of the X2APIC MSRs.
3. Add support to allow the guest OS to request Alternate Injection.

Melody Wang (8):
x86/sev: Make SVSM calls preemption-safe
x86/sev: Add support for Alternate Injection
x86/apic: Add an SVSM APIC driver
x86/sev: Route unsupported APIC register accesses to the hypervisor
APIC emulation
x86/sev: Add a function to contain all SEV-specific setup operations x86/sev: Register the guest with the SVSM APIC protocol x86/sev: Allow the guest to configure interrupt vectors for the
hypervisor
x86/sev: Indicate that Alternate Injection is supported in the guest

 arch/x86/Kconfig                        |  14 ++
 arch/x86/boot/compressed/sev.c          |  58 +++++-
 arch/x86/boot/compressed/sev.h          |  13 ++
 arch/x86/coco/core.c                    |   3 +
 arch/x86/coco/sev/core.c                |  48 ++---
 arch/x86/coco/sev/svsm.c                |  10 +-
 arch/x86/include/asm/msr-index.h        |   4 +-
 arch/x86/include/asm/sev-common.h       |   2 +
 arch/x86/include/asm/sev.h              |  21 +-
 arch/x86/kernel/apic/Makefile           |   1 +
 arch/x86/kernel/apic/x2apic_savic.c     |   8 +-
 arch/x86/kernel/apic/x2apic_svsm.c      | 243 ++++++++++++++++++++++++
 drivers/firmware/efi/libstub/x86-stub.c |   9 +-
 include/linux/cc_platform.h             |   8 +
 tools/arch/x86/include/asm/msr-index.h  |   4 +-
 15 files changed, 398 insertions(+), 48 deletions(-)
 create mode 100644 arch/x86/kernel/apic/x2apic_svsm.c
Signature · 1 line
2.43.0
#14 lore raw

Hi Ard,

Quoted 6 lines · Ard Biesheuvel

This may be a bit of a 'back to the drawing board' remark, but if opting in to this alternate injection scheme can only be done from the EFI firmware before ExitBootServices(), couldn't we wrap all this logic into an OVMF protocol instead? That way, the EFI stub can just invoke it if it exists, and what happens under the hood as a result is entirely under the control of the firmware.

Let me try to explain and please ask if anything's not clear because this is a complex topic:

When a SEV-SNP guest starts, a Secure VM Service Module running at VMPL0 (higher privilege level than the guest's) is executed first. That's a paravisor, see here for a possible explanation: https://ksayid.github.io/confidential-computing-notes/docs/core/paravisor/.

That paravisor will boot OVMF, and then OVMF will boot the guest kernel.

The SVSM must know the Alternate Injection choice of the first component which is the OVMF, so it will be able to enable Alternate Injection in the guest VMSA and record OVMF's registration.

The guest kernel should be able to make its own independent decision about whether to use Alternate Injection or not. If the kernel wants it, the kernel must register itself — this registration is performed in guest kernel code so that it remains under the guest kernel's control.

The guest kernel needs to register Alternate Injection before ExitBootServices() so the SVSM registration counter always reflects the correct, current state of Alternate Injection across the OVMF-to-kernel hand-off.

But if we leave the OVMF to decide for the guest kernel, that is violating the confidential computing principles of the guest being in control.

I hope this makes more sense.

Thanks,
Melody.

maintainer
#15 lore raw
Quoted 10 lines · Melody Wang

Hi Ard,

Quoted 6 lines · Ard Biesheuvel

This may be a bit of a 'back to the drawing board' remark, but if opting in to this alternate injection scheme can only be done from the EFI firmware before ExitBootServices(), couldn't we wrap all this logic into an OVMF protocol instead? That way, the EFI stub can just invoke it if it exists, and what happens under the hood as a result is entirely under the control of the firmware.

...

Quoted 5 lines

But if we leave the OVMF to decide for the guest kernel, that is violating the confidential computing principles of the guest being in control.

That is not what I am suggesting.

What I would like to see is an abstraction implemented in OVMF that encapsulates the logic that you are adding here. All the EFI stub would have to do is call the protocol, nothing more.

After ExitBootServices() is a different matter, and actually, I think doing the memory acceptance at that point was a mistake, and I'd like to fix that but that is a separate discussion.

Before ExitBootServices(), we should not be poking MSRs directly. We should be relying on the abstractions exposed by the firmware.

maintainer
#16 lore raw
Quoted 3 lines · Ard Biesheuvel

What I would like to see is an abstraction implemented in OVMF that encapsulates the logic that you are adding here. All the EFI stub would have to do is call the protocol, nothing more.

Out of pure curiosity, why?

Is the answer something along the lines of, before ExitBootServices(), the firmware owns the machine, the kernel should not poke at anything but should call the fw and latter is supposed to do the init and synchronization and setup of resources, yadda yadda...

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
maintainer
#17 lore raw
Quoted 5 lines · Borislav Petkov
Quoted 3 lines · Ard Biesheuvel

What I would like to see is an abstraction implemented in OVMF that encapsulates the logic that you are adding here. All the EFI stub would have to do is call the protocol, nothing more.

Quoted 7 lines

Out of pure curiosity, why?

Is the answer something along the lines of, before ExitBootServices(), the firmware owns the machine, the kernel should not poke at anything but should call the fw and latter is supposed to do the init and synchronization and setup of resources, yadda yadda...

Essentially. It is a layering violation.

Before ExitBootServices(), the firmware runs with the timer interrupt enabled. So poking at MSRs to reconfigure this behind the back of the firmware while it thinks it is still in full control is not a great idea.

Instead, the EFI stub should inform the firmware that it wants alternate injection, and the firmware can take care of that at EBS() time.

maintainer
#18 lore raw
Quoted 8 lines · Ard Biesheuvel

Essentially. It is a layering violation.

Before ExitBootServices(), the firmware runs with the timer interrupt enabled. So poking at MSRs to reconfigure this behind the back of the firmware while it thinks it is still in full control is not a great idea.

Instead, the EFI stub should inform the firmware that it wants alternate injection, and the firmware can take care of that at EBS() time.

Ack, makes sense.

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
#19 lore raw
In reply to Ard Biesheuvel

Hi Ard,

Quoted 7 lines · Ard Biesheuvel

That is not what I am suggesting.

What I would like to see is an abstraction implemented in OVMF that encapsulates the logic that you are adding here. All the EFI stub would have to do is call the protocol, nothing more.

Quoted 7 lines

After ExitBootServices() is a different matter, and actually, I think doing the memory acceptance at that point was a mistake, and I'd like to fix that but that is a separate discussion.

Before ExitBootServices(), we should not be poking MSRs directly. We should be relying on the abstractions exposed by the firmware.

ok, see below what I did, I think the following is perhaps what you had in mind but please let me know if that is ok this way.

In there, the guest kernel will register Alternate Injection through OVMF if Alternate Injection is enabled in the special sev_status MSR.

The OVMF will check if there is an SVSM present and Alternate Injection is enabled or not, if yes, it will register Alternate Injection for the guest kernel. Otherwise, it will not. A failure of registering Alternate Injection will terminate the guest.

Cc Jon Lange, the Alternate Injection spec author as an FYI.

-----------------------------------------------------------------------

Kernel code:

arch/x86/boot/compressed/sev.h +0 -1
index 22637b416b46..62e50c2e71ed 100644
@@ -14,7 +14,6 @@
 
 void snp_accept_memory(phys_addr_t start, phys_addr_t end);
 u64 sev_get_status(void);
-bool early_is_sevsnp_guest(void);
 
 static inline u64 sev_es_rd_ghcb_msr(void)
 {
@@ -37,7 +36,6 @@ static inline void sev_es_wr_ghcb_msr(u64 val)
 
 static inline void snp_accept_memory(phys_addr_t start, phys_addr_t 

end) { }
static inline u64 sev_get_status(void) { return 0; }
-static inline bool early_is_sevsnp_guest(void) { return false; }

#endif

arch/x86/include/asm/sev.h +2 -0
index 93161b663d9b..13f2e8c009b4 100644
@@ -471,6 +471,8 @@ static __always_inline void sev_es_nmi_complete(void)
 extern int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd);
 extern void sev_enable(struct boot_params *bp);
 
+bool early_is_sevsnp_guest(void);
+
 /*
  * RMPADJUST modifies the RMP permissions of a page of a lesser-
  * privileged (numerically higher) VMPL.
drivers/firmware/efi/libstub/efistub.h +11 -0
index fd91fc15ec81..d65cdc8f2acd 100644
@@ -177,6 +177,17 @@ void efi_set_u64_split(u64 data, u32 *lo, u32 *hi)
  */
 #define EFI_MMAP_NR_SLACK_SLOTS        32
 
+typedef union sev_alt_inj_register_protocol sev_alt_inj_register_protocol_t;
+union sev_alt_inj_register_protocol {
+       struct {
+               efi_status_t (__efiapi * sev_register_alt_inj)
+                       (sev_alt_inj_register_protocol_t *);
+       };
+       struct {
+               u32 sev_register_alt_inj;
+       } mixed_mode;
+};
+
 typedef struct efi_generic_dev_path efi_device_path_protocol_t;
 
 union efi_device_path_to_text_protocol {
drivers/firmware/efi/libstub/x86-stub.c +34 -0
index 2a1fa773d241..b6e58ade4162 100644
@@ -783,6 +783,36 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
        return EFI_SUCCESS;
 }
 
+/*
+ * When the guest is running at VMPL2 with Alternate Injection enabled,
+ * register Alternate Injection before ExitBootServices().
+ */
+static int svsm_register_alt_inj(void)
+{
+       efi_guid_t alt_inj_proto = OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL_GUID;
+       sev_alt_inj_register_protocol_t *proto;
+       efi_status_t status;
+
+       if (early_is_sevsnp_guest() && snp_vmpl) {
+               if (!(sev_get_status() & MSR_AMD64_SNP_ALTERNATE_INJ))
+                       return 0;
+       }
+
+       status = efi_bs_call(locate_protocol, &alt_inj_proto, NULL, (void **)&proto);
+       if (status != EFI_SUCCESS) {
+               efi_err("Alternate Injection registration protocol not exist\n");
+               return 1;
+       }
+
+       status = efi_call_proto(proto, sev_register_alt_inj);
+       if (status != EFI_SUCCESS) {
+               efi_err("Alternate Injection registration protocol failed\n");
+               return 2;
+       }
+
+       return 0;
+}
+
 static bool sev_prepare(void)
 {
        u64 unsupported;
@@ -793,6 +823,10 @@ static bool sev_prepare(void)
                        unsupported);
                return false;
        }
+
+       if (svsm_register_alt_inj())
+               return false;
+
        return true;
 }
 
include/linux/efi.h +0 -0
index aa15ff88539b..ebf7b4b9a292 100644
@@ -444,6 +444,8 @@ void efi_native_runtime_setup(void);
 #define OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL_GUID 

EFI_GUID(0xc5a010fe, 0x38a7, 0x4531, 0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49)
#define OVMF_MEMORY_LOG_TABLE_GUID EFI_GUID(0x95305139,
0xb20f, 0x4723, 0x84, 0x25, 0x62, 0x7c, 0x88, 0x8f, 0xf1, 0x21)

+#define OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL_GUID EFI_GUID(0x3cf00587, 0x2329, 0x4c44, 0xb1, 0x84, 0x8f, 0xd9, 0x87, 0x00, 0x42, 0xe0) +

 typedef struct {
        efi_guid_t guid;
        u64 table;

---------------------------------------------------------------------

OVMF code:

OvmfPkg/AmdSevDxe/AmdSevDxe.c +39 -0
index f10f37719527..c7f471edd838 100644
@@ -24,6 +24,7 @@
 #include <Library/PcdLib.h>
 #include <Pi/PiDxeCis.h>
 #include <Protocol/SevMemoryAcceptance.h>
+#include <Protocol/SevAltInjRegister.h>^M
 #include <Protocol/MemoryAccept.h>
 #include <Uefi/UefiSpec.h>
 
@@ -193,6 +194,42 @@ STATIC EDKII_MEMORY_ACCEPT_PROTOCOL mMemoryAcceptProtocol = {
   AmdSevMemoryAccept
 };
 
+VOID^M
+EFIAPI^M
+SvsmRegisterAltInj (^M
+  VOID^M
+  )^M
+{^M
+  UINT8 vector = 0x02;^M
+  AmdSvsmSnpApicConfigEmulation(vector);^M
+}^M
+^M
+/**^M
+  Register the guest kernel with Alternate Injection.^M
+**/^M
+STATIC^M
+EFI_STATUS^M
+EFIAPI^M
+SevRegisterAltInj (^M
+  IN OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL  *This^M
+  )^M
+{^M
+  if (This == NULL) {^M
+    return EFI_INVALID_PARAMETER;^M
+  }^M
+^M
+  if (!(AmdSvsmIsSvsmPresent() && AlternateInjectionEnabled())) {^M
+    return EFI_UNSUPPORTED;^M
+  }^M
+^M
+  SvsmRegisterAltInj ();^M
+^M
+  return EFI_SUCCESS;^M
+}^M
+^M
+STATIC^M
+OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL^M
+  mSevAltInjRegisterProtocol = { SevRegisterAltInj };^M
 
 EFI_EVENT  mDeregisterAlternateInjectionEvent = NULL;
 /**
@@ -355,6 +392,8 @@ AmdSevDxeEntryPoint (
                     &mMemoryAcceptProtocol,
                     &gOvmfSevMemoryAcceptanceProtocolGuid,
                     &mMemoryAcceptanceProtocol,
+                    &gOvmfSevAltInjRegisterProtocolGuid,^M
+                    &mSevAltInjRegisterProtocol,^M
                     NULL
                     );
     ASSERT_EFI_ERROR (Status);
OvmfPkg/AmdSevDxe/AmdSevDxe.inf +1 -0
index 0db38471020a..bc6c1827e649 100644
@@ -53,6 +53,7 @@
 [Protocols]
   gEdkiiMemoryAcceptProtocolGuid
   gOvmfSevMemoryAcceptanceProtocolGuid
+  gOvmfSevAltInjRegisterProtocolGuid^M
 
 [Guids]
   gConfidentialComputingSevSnpBlobGuid
OvmfPkg/Include/Protocol/SevAltInjRegister.h new file +28 -0
new file mode 100644
index 000000000000..cd6c32384a51
@@ -0,0 +1,28 @@
+#pragma once
+
+#define OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL_GUID \
+  { 0x3cf00587, \
+    0x2329, \
+    0x4c44, \
+    {0xb1, 0x84, 0x8f, 0xd9, 0x87, 0x00, 0x42, 0xe0}}
+
+typedef struct _OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL
+    OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL;
+
+/**
+  Register the caller (guest kernel) as an Alternate Injection consumer.
+  Wraps the SVSM registration; increments the SVSM registration counter.
+
+  @retval EFI_SUCCESS           Registration succeeded.
+  @retval EFI_UNSUPPORTED       Alt-Injection not available on this platform.
+  @retval EFI_DEVICE_ERROR      SVSM call failed.
+**/
+typedef
+EFI_STATUS
+(EFIAPI *OVMF_SEV_REGISTER_ALT_INJ)(
+  IN OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL  *This
+  );
+
+struct _OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL {
+  OVMF_SEV_REGISTER_ALT_INJ  SevRegisterAltInj;
+};
OvmfPkg/OvmfPkg.dec +0 -0
index 50de41b7fcc8..cc3fa86a30d4 100644
@@ -213,6 +213,7 @@
   gQemuAcpiTableNotifyProtocolGuid      = {0x928939b2, 0x4235, 0x462f, 

{0x95, 0x80, 0xf6, 0xa2, 0xb2, 0xc2, 0x1a, 0x4f}}
gEfiMpInitLibMpDepProtocolGuid = {0xbb00a5ca, 0x8ce, 0x462f,
{0xa5, 0x37, 0x43, 0xc7, 0x4a, 0x82, 0x5c, 0xa4}}
gEfiMpInitLibUpDepProtocolGuid = {0xa9e7cef1, 0x5682, 0x42cc,
{0xb1, 0x23, 0x99, 0x30, 0x97, 0x3f, 0x4a, 0x9f}}
+ gOvmfSevAltInjRegisterProtocolGuid = {0x3cf00587, 0x2329, 0x4c44, {0xb1, 0x84, 0x8f, 0xd9, 0x87, 0x00, 0x42, 0xe0}}^M

[PcdsFixedAtBuild]

   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0
Signature · 2 lines
Thanks,
Melody
maintainer
#20 lore raw

Hi Melody,

Quoted 24 lines · Melody Wang

Hi Ard,

Quoted 7 lines · Ard Biesheuvel

That is not what I am suggesting.

What I would like to see is an abstraction implemented in OVMF that encapsulates the logic that you are adding here. All the EFI stub would have to do is call the protocol, nothing more.

Quoted 7 lines

After ExitBootServices() is a different matter, and actually, I think doing the memory acceptance at that point was a mistake, and I'd like to fix that but that is a separate discussion.

Before ExitBootServices(), we should not be poking MSRs directly. We should be relying on the abstractions exposed by the firmware.

ok, see below what I did, I think the following is perhaps what you had in mind but please let me know if that is ok this way.

In there, the guest kernel will register Alternate Injection through OVMF if Alternate Injection is enabled in the special sev_status MSR.

Quoted 7 lines

The OVMF will check if there is an SVSM present and Alternate Injection is enabled or not, if yes, it will register Alternate Injection for the guest kernel. Otherwise, it will not. A failure of registering Alternate Injection will terminate the guest.

Cc Jon Lange, the Alternate Injection spec author as an FYI.

This looks much better thanks.

Quoted 77 lines

-----------------------------------------------------------------------

Kernel code:

arch/x86/boot/compressed/sev.h +0 -2
index 22637b416b46..62e50c2e71ed 100644
@@ -14,7 +14,6 @@
 
  void snp_accept_memory(phys_addr_t start, phys_addr_t end);
  u64 sev_get_status(void);
-bool early_is_sevsnp_guest(void);
 
  static inline u64 sev_es_rd_ghcb_msr(void)
  {
@@ -37,7 +36,6 @@ static inline void sev_es_wr_ghcb_msr(u64 val)
 
  static inline void snp_accept_memory(phys_addr_t start, phys_addr_t 
 end) { }
  static inline u64 sev_get_status(void) { return 0; }
-static inline bool early_is_sevsnp_guest(void) { return false; }
 
  #endif
arch/x86/include/asm/sev.h +2 -0
index 93161b663d9b..13f2e8c009b4 100644
@@ -471,6 +471,8 @@ static __always_inline void sev_es_nmi_complete(void)
  extern int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd);
  extern void sev_enable(struct boot_params *bp);
 
+bool early_is_sevsnp_guest(void);
+
  /*
   * RMPADJUST modifies the RMP permissions of a page of a lesser-
   * privileged (numerically higher) VMPL.
drivers/firmware/efi/libstub/efistub.h +11 -0
index fd91fc15ec81..d65cdc8f2acd 100644
@@ -177,6 +177,17 @@ void efi_set_u64_split(u64 data, u32 *lo, u32 *hi)
   */
  #define EFI_MMAP_NR_SLACK_SLOTS        32
 
+typedef union sev_alt_inj_register_protocol 
 sev_alt_inj_register_protocol_t;
+union sev_alt_inj_register_protocol {
+       struct {
+               efi_status_t (__efiapi * sev_register_alt_inj)
+                       (sev_alt_inj_register_protocol_t *);
+       };
+       struct {
+               u32 sev_register_alt_inj;
+       } mixed_mode;
+};
+
  typedef struct efi_generic_dev_path efi_device_path_protocol_t;
 
  union efi_device_path_to_text_protocol {
drivers/firmware/efi/libstub/x86-stub.c +5 -0
index 2a1fa773d241..b6e58ade4162 100644
@@ -783,6 +783,36 @@ static efi_status_t exit_boot(struct boot_params 
 *boot_params, void *handle)
         return EFI_SUCCESS;
  }
 
+/*
+ * When the guest is running at VMPL2 with Alternate Injection enabled,
+ * register Alternate Injection before ExitBootServices().
+ */
+static int svsm_register_alt_inj(void)

drivers/firmware/efi/libstub/x86-stub.c static efi_status_t exit_boot(struct boot_params · line 801

Quoted 10 lines
drivers/firmware/efi/libstub/x86-stub.c +10 -0
+{
+       efi_guid_t alt_inj_proto = OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL_GUID;
+       sev_alt_inj_register_protocol_t *proto;
+       efi_status_t status;
+
+       if (early_is_sevsnp_guest() && snp_vmpl) {
+               if (!(sev_get_status() & MSR_AMD64_SNP_ALTERNATE_INJ))
+                       return 0;
+       }
+

No need to test this - the protocol should deal with this.

drivers/firmware/efi/libstub/x86-stub.c static efi_status_t exit_boot(struct boot_params *boot_params, void *handle) · line 806

Quoted 8 lines
drivers/firmware/efi/libstub/x86-stub.c +6 -0
+       status = efi_bs_call(locate_protocol, &alt_inj_proto, NULL, 
 (void **)&proto);
+       if (status != EFI_SUCCESS) {
+               efi_err("Alternate Injection registration protocol not 
 exist\n");
+               return 1;
+       }
+

Just ignore the error

drivers/firmware/efi/libstub/x86-stub.c static efi_status_t exit_boot(struct boot_params *boot_params, void *handle) · line 812

Quoted 7 lines
drivers/firmware/efi/libstub/x86-stub.c +3 -0
+       status = efi_call_proto(proto, sev_register_alt_inj);
+       if (status != EFI_SUCCESS) {
+               efi_err("Alternate Injection registration protocol 

failed\n");

drivers/firmware/efi/libstub/x86-stub.c +3 -0
+               return 2;
+       }
+

Distinguish here between EFI_UNSUPPORTED (which can be ignored) and other errors.

Only question is whether EFI stub and OVMF are guaranteed to be in sync wrt snp_vmpl versus AmdSvsmIsSvsmPresent(), but I guess things would not work at all if that is not the case?

Quoted 52 lines
drivers/firmware/efi/libstub/x86-stub.c +7 -0
+       return 0;
+}
+
  static bool sev_prepare(void)
  {
         u64 unsupported;
@@ -793,6 +823,10 @@ static bool sev_prepare(void)
                         unsupported);
                 return false;
         }
+
+       if (svsm_register_alt_inj())
+               return false;
+
         return true;
  }
 
include/linux/efi.h +1 -0
index aa15ff88539b..ebf7b4b9a292 100644
@@ -444,6 +444,8 @@ void efi_native_runtime_setup(void);
  #define OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL_GUID 
 EFI_GUID(0xc5a010fe, 0x38a7, 0x4531,  0x8a, 0x4a, 0x05, 0x00, 0xd2, 
 0xfd, 0x16, 0x49)
  #define OVMF_MEMORY_LOG_TABLE_GUID             EFI_GUID(0x95305139, 
 0xb20f, 0x4723,  0x84, 0x25, 0x62, 0x7c, 0x88, 0x8f, 0xf1, 0x21)
 
+#define OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL_GUID 

EFI_GUID(0x3cf00587, 0x2329, 0x4c44, 0xb1, 0x84, 0x8f, 0xd9, 0x87, 0x00, 0x42, 0xe0)
+

  typedef struct {
         efi_guid_t guid;
         u64 table;

---------------------------------------------------------------------

OVMF code:

OvmfPkg/AmdSevDxe/AmdSevDxe.c +1 -0
index f10f37719527..c7f471edd838 100644
@@ -24,6 +24,7 @@
  #include <Library/PcdLib.h>
  #include <Pi/PiDxeCis.h>
  #include <Protocol/SevMemoryAcceptance.h>
+#include <Protocol/SevAltInjRegister.h>^M
  #include <Protocol/MemoryAccept.h>
  #include <Uefi/UefiSpec.h>
 

OvmfPkg/AmdSevDxe/AmdSevDxe.c STATIC EDKII_MEMORY_ACCEPT_PROTOCOL · line 197

Quoted 4 lines
OvmfPkg/AmdSevDxe/AmdSevDxe.c +0 -0
@@ -193,6 +194,42 @@ STATIC EDKII_MEMORY_ACCEPT_PROTOCOL 
 mMemoryAcceptProtocol = {
    AmdSevMemoryAccept
  };
 

Add STATIC here

Quoted 16 lines
OvmfPkg/AmdSevDxe/AmdSevDxe.c +15 -0
+VOID^M
 
+EFIAPI^M
+SvsmRegisterAltInj (^M
+  VOID^M
+  )^M
+{^M
+  UINT8 vector = 0x02;^M
+  AmdSvsmSnpApicConfigEmulation(vector);^M
+}^M
+^M
+/**^M
+  Register the guest kernel with Alternate Injection.^M
+**/^M
+STATIC^M
+EFI_STATUS^M

OvmfPkg/AmdSevDxe/AmdSevDxe.c STATIC EDKII_MEMORY_ACCEPT_PROTOCOL mMemoryAcceptProtocol = { · line 221

Quoted 10 lines
OvmfPkg/AmdSevDxe/AmdSevDxe.c +10 -0
+EFIAPI^M
+SevRegisterAltInj (^M
+  IN OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL  *This^M
+  )^M
+{^M
+  if (This == NULL) {^M
+    return EFI_INVALID_PARAMETER;^M
+  }^M
+^M
+  if (!(AmdSvsmIsSvsmPresent() && AlternateInjectionEnabled())) {^M

Please make this !cond || !cond

Quoted 92 lines
OvmfPkg/AmdSevDxe/AmdSevDxe.c +11 -0
+    return EFI_UNSUPPORTED;^M
+  }^M
+^M
+  SvsmRegisterAltInj ();^M
+^M
+  return EFI_SUCCESS;^M
+}^M
+^M
+STATIC^M
+OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL^M
+  mSevAltInjRegisterProtocol = { SevRegisterAltInj };^M
 
  EFI_EVENT  mDeregisterAlternateInjectionEvent = NULL;
  /**
OvmfPkg/AmdSevDxe/AmdSevDxe.c +2 -0
@@ -355,6 +392,8 @@ AmdSevDxeEntryPoint (
                      &mMemoryAcceptProtocol,
                      &gOvmfSevMemoryAcceptanceProtocolGuid,
                      &mMemoryAcceptanceProtocol,
+                    &gOvmfSevAltInjRegisterProtocolGuid,^M
+                    &mSevAltInjRegisterProtocol,^M
                      NULL
                      );
      ASSERT_EFI_ERROR (Status);
OvmfPkg/AmdSevDxe/AmdSevDxe.inf +1 -0
index 0db38471020a..bc6c1827e649 100644
@@ -53,6 +53,7 @@
  [Protocols]
    gEdkiiMemoryAcceptProtocolGuid
    gOvmfSevMemoryAcceptanceProtocolGuid
+  gOvmfSevAltInjRegisterProtocolGuid^M
 
  [Guids]
    gConfidentialComputingSevSnpBlobGuid
OvmfPkg/Include/Protocol/SevAltInjRegister.h new file +17 -0
new file mode 100644
index 000000000000..cd6c32384a51
@@ -0,0 +1,28 @@
+#pragma once
+
+#define OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL_GUID \
+  { 0x3cf00587, \
+    0x2329, \
+    0x4c44, \
+    {0xb1, 0x84, 0x8f, 0xd9, 0x87, 0x00, 0x42, 0xe0}}
+
+typedef struct _OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL
+    OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL;
+
+/**
+  Register the caller (guest kernel) as an Alternate Injection consumer.
+  Wraps the SVSM registration; increments the SVSM registration counter.
+
+  @retval EFI_SUCCESS           Registration succeeded.
+  @retval EFI_UNSUPPORTED       Alt-Injection not available on this 

platform.

OvmfPkg/Include/Protocol/SevAltInjRegister.h new file +11 -0
+  @retval EFI_DEVICE_ERROR      SVSM call failed.
+**/
+typedef
+EFI_STATUS
+(EFIAPI *OVMF_SEV_REGISTER_ALT_INJ)(
+  IN OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL  *This
+  );
+
+struct _OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL {
+  OVMF_SEV_REGISTER_ALT_INJ  SevRegisterAltInj;
+};
OvmfPkg/OvmfPkg.dec +1 -0
index 50de41b7fcc8..cc3fa86a30d4 100644
@@ -213,6 +213,7 @@
    gQemuAcpiTableNotifyProtocolGuid      = {0x928939b2, 0x4235, 0x462f, 
 {0x95, 0x80, 0xf6, 0xa2, 0xb2, 0xc2, 0x1a, 0x4f}}
    gEfiMpInitLibMpDepProtocolGuid        = {0xbb00a5ca, 0x8ce,  0x462f, 
 {0xa5, 0x37, 0x43, 0xc7, 0x4a, 0x82, 0x5c, 0xa4}}
    gEfiMpInitLibUpDepProtocolGuid        = {0xa9e7cef1, 0x5682, 0x42cc, 
 {0xb1, 0x23, 0x99, 0x30, 0x97, 0x3f, 0x4a, 0x9f}}
+  gOvmfSevAltInjRegisterProtocolGuid    = {0x3cf00587, 0x2329, 0x4c44, 

{0xb1, 0x84, 0x8f, 0xd9, 0x87, 0x00, 0x42, 0xe0}}^M

[PcdsFixedAtBuild]

    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0
Signature · 2 lines
Thanks,
Melody
#21 lore raw

Hi Ard,

Quoted 8 lines · Ard Biesheuvel
Quoted 8 lines

+ * When the guest is running at VMPL2 with Alternate Injection enabled,
+ * register Alternate Injection before ExitBootServices().
+ */

drivers/firmware/efi/libstub/x86-stub.c +5 -0
+static int svsm_register_alt_inj(void)
+{
+       efi_guid_t alt_inj_proto = OVMF_SEV_ALT_INJ_REGISTER_PROTOCOL_GUID;
+       sev_alt_inj_register_protocol_t *proto;
+       efi_status_t status;
Quoted 8 lines

drivers/firmware/efi/libstub/x86-stub.c static efi_status_t exit_boot(struct boot_params *boot_params, void *handle) · line 800

Quoted 6 lines
drivers/firmware/efi/libstub/x86-stub.c +6 -0
+
+       if (early_is_sevsnp_guest() && snp_vmpl) {
+               if (!(sev_get_status() & MSR_AMD64_SNP_ALTERNATE_INJ))
+                       return 0;
+       }
+

No need to test this - the protocol should deal with this.

I think we need to test this because the guest kernel should not try to register Alternate Injection if it is not a sev-snp guest, and running at VMPL0, and Alternate Injection is not supported.

Only if the three are satisfied, then it should try to register Alternate Injection.

This way, when the registration failed for whatever reason, such as the protocol is not supported by OVMF, etc, the guest should terminate. This way, the registration decision is still controlled by the guest.

All in all, it is the guest's decision whether to enable Alternate Injection or not and that check is done here. OVMF should not be responsible for checking that for the guest - it should simply do what it has been requested to do.

I hope this makes sense.

Quoted 10 lines

drivers/firmware/efi/libstub/x86-stub.c static efi_status_t exit_boot(struct boot_params *boot_params, void *handle) · line 806

Quoted 8 lines

+ status = efi_bs_call(locate_protocol, &alt_inj_proto, NULL,
(void **)&proto);

(quoted diff) +2 -0
+       if (status != EFI_SUCCESS) {
+               efi_err("Alternate Injection registration protocol not

exist\n");

drivers/firmware/efi/libstub/x86-stub.c +3 -0
+               return 1;
+       }
+

Just ignore the error

I think I need to check this error, as mentioned above, so the guest will terminate, that means the OVMF must support the protocol, when Alternate Injection is supported, and the guest kernel is trying to register it.

Quoted 9 lines

drivers/firmware/efi/libstub/x86-stub.c static efi_status_t exit_boot(struct boot_params *boot_params, void *handle) · line 812

Quoted 8 lines
drivers/firmware/efi/libstub/x86-stub.c +3 -0
+       status = efi_call_proto(proto, sev_register_alt_inj);
+       if (status != EFI_SUCCESS) {
+               efi_err("Alternate Injection registration protocol

failed\n");

drivers/firmware/efi/libstub/x86-stub.c +3 -0
+               return 2;
+       }
+
Quoted 7 lines

Distinguish here between EFI_UNSUPPORTED (which can be ignored) and other errors.

Only question is whether EFI stub and OVMF are guaranteed to be in sync wrt snp_vmpl versus AmdSvsmIsSvsmPresent(), but I guess things would not work at all if that is not the case?

yes if a SVSM is present, that means the SVSM is running at VMPL0, OVMF and the guest kernel will run at the lower priority level.

Signature · 2 lines
Thanks,
Melody