Alternate Injection: Secure Interrupt Delivery for SEV-SNP Guests - Guest Support
cover Alternate Injection: Secure Interrupt Delivery for SEV-SNP Guests - Guest Support
Hi all,
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 confitions correctly.
9. Moved allowing all of interrupts right after registering the SVSM APIC protocol.
Thanks,
Melody
Changelog:
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 | 49 ++++- arch/x86/boot/compressed/sev.h | 13 ++ arch/x86/coco/core.c | 3 + arch/x86/coco/sev/core.c | 14 +- arch/x86/coco/sev/svsm.c | 11 +- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 4 +- arch/x86/include/asm/sev-common.h | 2 + arch/x86/include/asm/sev.h | 20 +- arch/x86/kernel/apic/Makefile | 1 + arch/x86/kernel/apic/x2apic_savic.c | 8 +- arch/x86/kernel/apic/x2apic_svsm.c | 236 ++++++++++++++++++++++++ drivers/firmware/efi/libstub/x86-stub.c | 17 +- include/linux/cc_platform.h | 8 + 15 files changed, 366 insertions(+), 35 deletions(-) create mode 100644 arch/x86/kernel/apic/x2apic_svsm.c
Signature · 1 line
2.43.0
PATCH 1/8 x86/sev: Make SVSM calls preemption-safe
Two functions in the the SVSM vTPM guest implementation do not disable preemption when fetching CAA.
The SVSM calling area is a per-CPU structure. When a thread is preempted and migrated to a different CPU after fetching the per-CPU Calling Area Address (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.
--- arch/x86/coco/sev/svsm.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
arch/x86/coco/sev/svsm.c +9 -2
@@ -74,6 +74,15 @@ 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 migrates 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 even +* under preemption or CPU migration. +*/ + call->caa =svsm_get_caa (); + ghcb =__sev_get_ghcb (&state); do {
@@ -321,7 +330,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 +353,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
Borislav Petkov
maintainerRe: [PATCH v1 1/8] x86/sev: Make SVSM calls preemption-safe
+ folks who added the vtpm stuff.
Quoted 2 lines · Melody Wang
Two functions in the the SVSM vTPM guest implementation do not disable preemption when fetching CAA.
" ... when fetching the SVSM Calling Area Address (CAA).
The SVSM CAA..."
This way, you introduce what CAA means and then use the abbreviation.
Quoted 3 lines
The SVSM calling area is a per-CPU structure. When a thread is preempted and migrated to a different CPU after fetching the per-CPU Calling Area Address (CAA), the SVSM call will execute on the new CPU with the
and here you can use "CAA" directly because you've explained it above.
Quoted 7 lines
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.Signed-off-by Melody Wang <[email protected]>
Should this be CC:stable so that it gets backported?
I think we want to do that and mark it with
Quoted 9 lines
--- arch/x86/coco/sev/svsm.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)arch/x86/coco/sev/svsm.c +0
-0
arch/x86/coco/sev/svsm.c int svsm_perform_call_protocol(struct svsm_call *call) · line 79
Quoted 6 lines
arch/x86/coco/sev/svsm.c +3
-0@@ -74,6 +74,15 @@ 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 migrates to another CPU after it
"... or migrated to another CPU after the CAA has been fetched..."
arch/x86/coco/sev/svsm.c int svsm_perform_call_protocol(struct svsm_call *call) · line 82
Quoted 3 lines
arch/x86/coco/sev/svsm.c +3
-0+* is fetched, always fetch 'caa' and then issue the SVSM call with +* interrupts disabled. This ensures the correct 'caa' is used even +* under preemption or CPU migration.
s/even under preemption or CPU migration//
That's kinda clear.
Leaving in the rest for the newly CCed.
Quoted 25 lines
arch/x86/coco/sev/svsm.c +3
-2+*/ + call->caa =svsm_get_caa (); + ghcb =__sev_get_ghcb (&state); do {@@ -321,7 +330,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 +353,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
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteStefano Garzarella
Reviewed-byRe: [PATCH v1 1/8] x86/sev: Make SVSM calls preemption-safe
Quoted 1 line · Borislav Petkov
+ folks who added the vtpm stuff.
Thanks.
Quoted 55 lines · Melody Wang
Quoted 2 lines
Two functions in the the SVSM vTPM guest implementation do not disable preemption when fetching CAA.
" ... when fetching the SVSM Calling Area Address (CAA).
The SVSM CAA..."
This way, you introduce what CAA means and then use the abbreviation.
Quoted 3 lines
The SVSM calling area is a per-CPU structure. When a thread is preempted and migrated to a different CPU after fetching the per-CPU Calling Area Address (CAA), the SVSM call will execute on the new CPU with the
and here you can use "CAA" directly because you've explained it above.
Quoted 7 lines
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.Signed-off-by Melody Wang <[email protected]>Should this be CC:stable so that it gets backported?
I think we want to do that and mark it with
Fixes 770de678bc28 ("x86/sev: Add SVSM vTPM probe/send_command functions")Quoted 9 lines
--- arch/x86/coco/sev/svsm.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)arch/x86/coco/sev/svsm.c +0
-0arch/x86/coco/sev/svsm.c int svsm_perform_call_protocol(struct svsm_call *call) · line 79
Quoted 6 lines
arch/x86/coco/sev/svsm.c +3
-0@@ -74,6 +74,15 @@ 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 migrates to another CPU after it "... or migrated to another CPU after the CAA has been fetched..."
arch/x86/coco/sev/svsm.c int svsm_perform_call_protocol(struct svsm_call *call) · line 82
Quoted 3 lines
arch/x86/coco/sev/svsm.c +3
-0+* is fetched, always fetch 'caa' and then issue the SVSM call with +* interrupts disabled. This ensures the correct 'caa' is used even +* under preemption or CPU migration. s/even under preemption or CPU migration//
Quoted 6 lines
That's kinda clear.
Leaving in the rest for the newly CCed.
arch/x86/coco/sev/svsm.c int svsm_perform_call_protocol(struct svsm_call *call) · line 84
Quoted 2 lines
arch/x86/coco/sev/svsm.c +2
-0+*/ + call->caa =svsm_get_caa ();
svsm_perform_call_protocol() is called also by svsm_pval_pages() and snp_issue_svsm_attest_req(). After this change call->caa is set unconditionally inside svsm_perform_call_protocol(), so the call->caa = svsm_get_caa() there becomes somewhat redundant. That said we can't just drop it though, so at most it could become a local variable. Probably not worth the churn, just wanted to point it out.
That said, I agree that in those functions we don't have the issue since both of them are disabling irqs.
Thanks,
Stefano
Borislav Petkov
maintainerRe: [PATCH v1 1/8] x86/sev: Make SVSM calls preemption-safe
Quoted 4 lines · Stefano Garzarella
svsm_perform_call_protocol()is called also bysvsm_pval_pages()andsnp_issue_svsm_attest_req(). After this changecall->caais set unconditionally insidesvsm_perform_call_protocol(), so thecall->caa=svsm_get_caa()there becomes somewhat redundant.
Yeah, I believe Tom wanted here to be on the safe side and get the right caa, even if we get moved to another CPU. And yeah, it is redundant but it is a per-CPU access so meh, probably not that important.
But I guess a good cleanup ontop would be to remove all those svsm_get_caa() calls from the callers and have that happen solely in svsm_perform_call_protocol().
I guess Melody can do that ontop of this one after a proper audit of all call sites and testing. Because the current fix should probably go to stable and we would need the fix to be minimal.
Quoted 2 lines
That said we can't just drop it though, so at most it could become a local variable. Probably not worth the churn, just wanted to point it out.
Yeah.
Quoted 4 lines
That said, I agree that in those functions we don't have the issue since both of them are disabling irqs.
Reviewed-by Stefano Garzarella <[email protected]>a
I'll add your tag to the latest version:
https://lore.kernel.org/r/a5bc0d4a2c462a0089109e145c21626b244b2ff0.1789345277.git.huibo.wang@amd.com
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquettePATCH 2/8 x86/sev: Add support for Alternate Injection
In reply to Melody WangThe Alternate Injection feature is a method to protect an AMD confidential computing 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 enablement.
--- 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/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 4 ++-- include/linux/cc_platform.h | 8 ++++++++ 7 files changed, 30 insertions(+), 4 deletions(-)
arch/x86/Kconfig +14 -0
@@ -497,6 +497,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
@@ -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
@@ -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
@@ -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/cpufeatures.h +1 -0
@@ -466,6 +466,7 @@#define X86_FEATURE_SNP_SECURE_TSC (19*32+ 8)/* SEV-SNP Secure TSC */ #define X86_FEATURE_V_TSC_AUX (19*32+ 9)/* Virtual TSC_AUX */ #define X86_FEATURE_SME_COHERENT (19*32+10)/* hardware-enforced cache coherency */ +#define X86_FEATURE_ALTERNATE_INJECTION (19*32+13)/* SEV Alternate Injection */ #define X86_FEATURE_DEBUG_SWAP (19*32+14)/* "debug_swap" SEV-ES full debug state swap support */ #define X86_FEATURE_RMPREAD (19*32+21)/* RMPREAD instruction */ #define X86_FEATURE_SEGMENTED_RMP (19*32+23)/* Segmented RMP support */
arch/x86/include/asm/msr-index.h +2 -2
@@ -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
include/linux/cc_platform.h +8 -0
@@ -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 on the host. +* +* The host kernel is running with the necessary features +* needed to run Alternate Injection enabled guests. +*/ + CC_ATTR_SNP_ALTERNATE_INJECTION, };#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
Signature · 1 line
2.43.0
Borislav Petkov
maintainerRe: [PATCH v1 2/8] x86/sev: Add support for Alternate Injection
Quoted 2 lines · Melody Wang
The Alternate Injection feature is a method to protect an AMD confidential computing guest from malicious injection attacks. It allows
" ... a SNP guest... " - simpler.
Quoted 8 lines
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 enablement.
s/ enablement//
Quoted 10 lines
Signed-off-by Melody Wang <[email protected]>--- 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/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 4 ++-- include/linux/cc_platform.h | 8 ++++++++ 7 files changed, 30 insertions(+), 4 deletions(-)
...
arch/x86/coco/core.c static bool noinstr amd_cc_platform_has(enum cc_attr attr) · line 111
Quoted 10 lines
arch/x86/coco/core.c +2
-0@@ -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 ;
Sashiko is complaining here:
| Does this attribute evaluate the host status or the guest status? | | The implementation in amd_cc_platform_has() checks sev_status, which | represents the SEV capability of the executing guest. However, the doc comment | for CC_ATTR_SNP_ALTERNATE_INJECTION below states it checks if "The host kernel | is running with the necessary features". Could the documentation be updated to | match the actual implementation?
Because of the comment over CC_ATTR_SNP_ALTERNATE_INJECTION talking about the host running with the necessary features, blabla. Yeah, sev_status dumps guest bits and the alternate injection bit is set in the VMSA by the SVSM and so yeah, maybe we should tone down the "host" speech in the comment...
Quoted 18 lines
arch/x86/coco/sev/core.c +1
-1@@ -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/cpufeatures.h +0
-0
arch/x86/include/asm/cpufeatures.h line 469
Quoted 4 lines
arch/x86/include/asm/cpufeatures.h +1
-0@@ -466,6 +466,7 @@#define X86_FEATURE_SNP_SECURE_TSC (19*32+ 8)/* SEV-SNP Secure TSC */ #define X86_FEATURE_V_TSC_AUX (19*32+ 9)/* Virtual TSC_AUX */ #define X86_FEATURE_SME_COHERENT (19*32+10)/* hardware-enforced cache coherency */ +#define X86_FEATURE_ALTERNATE_INJECTION (19*32+13)/* SEV Alternate Injection */
Sashiko: Does this new feature addition need to be synchronized with the kernel tools directory?
Answer: no. We sync those only when needed.
Also, make the comment /* SEV-SNP Alternate ... */
as it is a SNP feature.
Quoted 8 lines
arch/x86/include/asm/cpufeatures.h +0
-0#define X86_FEATURE_DEBUG_SWAP (19*32+14)/* "debug_swap" SEV-ES full debug state swap support */ #define X86_FEATURE_RMPREAD (19*32+21)/* RMPREAD instruction */ #define X86_FEATURE_SEGMENTED_RMP (19*32+23)/* Segmented RMP support */ arch/x86/include/asm/msr-index.h +0
-0
arch/x86/include/asm/msr-index.h line 728
Quoted 10 lines
arch/x86/include/asm/msr-index.h +2
-2@@ -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
Sashiko: Is it necessary to update tools/arch/x86/include/asm/msr-index.h with this macro rename as well?
Yeah, we probably should for completeness' sake.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquettePATCH 3/8 x86/apic: Add an SVSM APIC driver
In reply to Melody WangAlternate Injection replaces hypervisor-based interrupt queuing and event injection, requiring guest-controlled queuing and injection. In order to perform this guest-controlled queuing and injectiion, 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 | 233 +++++++++++++++++++++++++++++ 4 files changed, 244 insertions(+) create mode 100644 arch/x86/kernel/apic/x2apic_svsm.c
arch/x86/include/asm/sev-common.h +1 -0
@@ -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
@@ -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
@@ -17,6 +17,7 @@ obj-$(CONFIG_SMP) += ipi.o ifeq ($(CONFIG_X86_64),y) # APIC probe will depend on the listing order here obj-$(CONFIG_X86_NUMACHIP) += apic_numachip.o +obj-$(CONFIG_AMD_ALTERNATE_INJ) += x2apic_svsm.o obj-$(CONFIG_X86_UV) += x2apic_uv_x.o obj-$(CONFIG_AMD_SECURE_AVIC) += x2apic_savic.o obj-$(CONFIG_X86_X2APIC) += x2apic_phys.o
arch/x86/kernel/apic/x2apic_svsm.c new file +233 -0
233 lines not on this page · raw message
Signature · 1 line
2.43.0
Borislav Petkov
maintainerRe: [PATCH v1 3/8] x86/apic: Add an SVSM APIC driver
In addition to the Sashiko review comments, some minor things below:
Quoted 3 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 injectiion, an SVSM
"injection"
Please use a spellchecker when writing commit messages.
Quoted 11 lines
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.
Signed-off-by Melody Wang <[email protected]>---
...
arch/x86/kernel/apic/Makefile obj-$(CONFIG_SMP) += ipi.o · line 20
Quoted 9 lines
arch/x86/kernel/apic/Makefile +1
-0@@ -17,6 +17,7 @@ obj-$(CONFIG_SMP) += ipi.o ifeq ($(CONFIG_X86_64),y) # APIC probe will depend on the listing order here obj-$(CONFIG_X86_NUMACHIP) += apic_numachip.o +obj-$(CONFIG_AMD_ALTERNATE_INJ) += x2apic_svsm.o
This should probably be after secure AVIC so that secure AVIC gets preferred on machines that support it.
Quoted 3 lines
arch/x86/kernel/apic/Makefile +0
-0obj-$(CONFIG_X86_UV) += x2apic_uv_x.o obj-$(CONFIG_AMD_SECURE_AVIC) += x2apic_savic.o obj-$(CONFIG_X86_X2APIC) += x2apic_phys.o
...
arch/x86/kernel/apic/Makefile
Quoted 9 lines
arch/x86/kernel/apic/Makefile +9
-0+static int svsm_apic_probe(void) +{ + if (cc_platform_has(CC_ATTR_SNP_ALTERNATE_INJECTION) && !snp_vmpl) { + pr_err("Alternate Injection in VMPL0 impossible. Terminating.\n"); + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL); + } + + if (!cc_platform_has(CC_ATTR_SNP_ALTERNATE_INJECTION)) + return 0;
This check should come first, ofc. IOW,
if (!cc...) {
...
} else {
...
}IOW, something like this:
static int svsm_apic_probe(void)
{
if (!cc_platform_has(CC_ATTR_SNP_ALTERNATE_INJECTION)) {
return 0;
} else {
if (!snp_vmpl) {
pr_err("Alternate Injection in VMPL0 impossible. Terminating.\n");
goto terminate;
}
}
if (!x2apic_mode) {
pr_err("Alternate Injection in non x2APIC mode impossible. Terminating.\n");
goto terminate;
}
pr_info("Alternate Injection SVSM APIC enabled\n");
return 1;terminate:
sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
return -EINVAL;
}...
Quoted 4 lines
arch/x86/kernel/apic/Makefile +4
-0+static inline u64 svsm_apic_icr_read(void) +{ + struct svsm_call call = {}; + u32 reg;
arch/x86/kernel/apic/Makefile
Quoted 10 lines
arch/x86/kernel/apic/Makefile +10
-0+ 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("svsm_apic_icr_read error: %d\n", ret);
Make that
pr_err("%s: error: %d\n", __func__, ret);Quoted 9 lines
arch/x86/kernel/apic/Makefile +9
-0+ 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 = {};
arch/x86/kernel/apic/Makefile
Quoted 10 lines
arch/x86/kernel/apic/Makefile +10
-0+ u64 icr_data; + u32 reg; + int ret; + + reg = APIC_ICR; + icr_data = ((u64)id) << 32 | low; + + call.rax = SVSM_APIC_CALL(SVSM_APIC_WRITE_REGISTER); + call.rcx = APIC_BASE_MSR + (reg >> 4); + call.rdx = icr_data;
You can do
call.rdx = ((u64)id) << 32 | low;
and get rid of icr_data.
arch/x86/kernel/apic/Makefile
Quoted 3 lines
arch/x86/kernel/apic/Makefile +3
-0+ ret = svsm_perform_call_protocol(&call); + if (ret) { + pr_err("svsm_apic_icr_write error: %d\n", ret);
Same as above:
pr_err("%s: error: %d\n", __func__, ret);Quoted 5 lines
arch/x86/kernel/apic/Makefile +5
-0+ sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL); + } +} + +static void svsm_apic_send_IPI(int cpu, int vector)
arch/x86/kernel/apic/Makefile
Quoted 10 lines
arch/x86/kernel/apic/Makefile +10
-0+{ + u32 dest = per_cpu(x86_cpu_to_apicid, cpu); + + svsm_apic_icr_write(__prepare_ICR(0, vector, APIC_DEST_PHYSICAL), dest); +} + +static void __svsm_apic_send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest) +{ + unsigned long query_cpu; + unsigned long this_cpu;
One line:
unsigned long query_cpu, this_cpu;
Quoted 11 lines
arch/x86/kernel/apic/Makefile +11
-0+ + guard(irqsave)(); + + this_cpu = smp_processor_id(); + for_each_cpu(query_cpu, mask) { + if (apic_dest == APIC_DEST_ALLBUT && this_cpu == query_cpu) + continue; + + svsm_apic_send_IPI(query_cpu, vector); + } +}
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquettePATCH 4/8 x86/sev: Route unsupported APIC register accesses to the hypervisor APIC emulation
In reply to Melody WangThe 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 | 12 ++++++------ 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 | 9 ++++++--- 5 files changed, 21 insertions(+), 17 deletions(-)
arch/x86/coco/sev/core.c +6 -6
@@ -972,7 +972,7 @@ int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd)return 0 ; } -u64 savic_ghcb_msr_read (u32 reg) +u64 sev_apic_ghcb_msr_read (u32 reg) {u64 msr =APIC_BASE_MSR + (reg >>4 );struct pt_regs regs = { .cx = msr };
@@ -988,9 +988,9 @@ u64 savic_ghcb_msr_read(u32 reg) res =__vc_handle_msr (ghcb, &ctxt, false);if (res !=ES_OK ) { -pr_err ("Secure AVIC MSR (0x%llx) read returned error (%d)\n" , msr, res); +pr_err ("APIC MSR via GHCB (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 ); +sev_es_terminate (SEV_TERM_SET_LINUX ,GHCB_TERM_APIC_MSR_FAIL ); }__sev_put_ghcb (&state);
@@ -998,7 +998,7 @@ u64 savic_ghcb_msr_read(u32 reg)return regs.ax | regs.dx <<32 ; } -void savic_ghcb_msr_write (u32 reg,u64 value) +void sev_apic_ghcb_msr_write (u32 reg,u64 value) {u64 msr =APIC_BASE_MSR + (reg >>4 );struct pt_regs regs = {
@@ -1018,9 +1018,9 @@ void savic_ghcb_msr_write(u32 reg, u64 value) 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); +pr_err ("APIC MSR via GHCB (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 ); +sev_es_terminate (SEV_TERM_SET_LINUX ,GHCB_TERM_APIC_MSR_FAIL ); }__sev_put_ghcb (&state);
arch/x86/include/asm/sev-common.h +1 -0
@@ -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
@@ -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
@@ -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 +6 -3
@@ -69,14 +69,17 @@ static u32 __svsm_apic_msr_rw(u32 reg, u32 v, bool write) call.rdx = v; ret =svsm_perform_call_protocol (&call); -if (ret) { +if (ret) {pr_err ("%s: 0x%x, error: %d\n" , call_reg_str, reg, ret);sev_es_terminate (SEV_TERM_SET_LINUX ,GHCB_TERM_ALT_INJ_FAIL ); }break ;default : -pr_err ("%s 0x%x not supported\n" , call_reg_str, reg); -return 0 ; +if (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
Borislav Petkov
maintainerRe: [PATCH v1 4/8] x86/sev: Route unsupported APIC register accesses to the hypervisor APIC emulation
Quoted 28 lines · Melody Wang
(quoted diff) +4
-4-u64 savic_ghcb_msr_read(u32 reg) +u64 sev_apic_ghcb_msr_read(u32 reg) { u64 msr = APIC_BASE_MSR + (reg >> 4); struct pt_regs regs = { .cx = msr };@@ -988,9 +988,9 @@ u64 savic_ghcb_msr_read(u32 reg) res = __vc_handle_msr(ghcb, &ctxt, false); if (res != ES_OK) { - pr_err("Secure AVIC MSR (0x%llx) read returned error (%d)\n", msr, res); + pr_err("APIC MSR via GHCB (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); + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_APIC_MSR_FAIL); } __sev_put_ghcb(&state);@@ -998,7 +998,7 @@ u64 savic_ghcb_msr_read(u32 reg) return regs.ax | regs.dx << 32; } -void savic_ghcb_msr_write(u32 reg, u64 value) +void sev_apic_ghcb_msr_write(u32 reg, u64 value) { u64 msr = APIC_BASE_MSR + (reg >> 4); struct pt_regs regs = {@@ -1018,9 +1018,9 @@ void savic_ghcb_msr_write(u32 reg, u64 value)
Review void savic_ghcb_msr_write(u32 reg, u64 value) · line 1026
Quoted 10 lines
(quoted diff) +2
-2res = __vc_handle_msr(ghcb, &ctxt, true); if (res != ES_OK) { - pr_err("Secure AVIC MSR (0x%llx) write returned error (%d)\n", msr, res); + pr_err("APIC MSR via GHCB (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); + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_APIC_MSR_FAIL); } __sev_put_ghcb(&state);
Those two functions look almost identical. I think you should unify them in a pre-patch exactly like you did __svsm_apic_msr_rw().
...
arch/x86/kernel/apic/x2apic_svsm.c static u32 __svsm_apic_msr_rw(u32 reg, u32 v, bool write) · line 72
Quoted 10 lines
arch/x86/kernel/apic/x2apic_svsm.c +1
-1@@ -69,14 +69,17 @@ static u32 __svsm_apic_msr_rw(u32 reg, u32 v, bool write) call.rdx = v; ret =svsm_perform_call_protocol (&call); -if (ret) { +if (ret) {
Applying: x86/sev: Route unsupported APIC register accesses to the hypervisor APIC emulation .git/rebase-apply/patch:142: trailing whitespace.
if (ret) { warning: 1 line adds whitespace errors.
This looks like a change you made by mistake while reworking.
arch/x86/kernel/apic/x2apic_svsm.c static u32 __svsm_apic_msr_rw(u32 reg, u32 v, bool write) · line 82
Quoted 12 lines
arch/x86/kernel/apic/x2apic_svsm.c +5
-2pr_err ("%s: 0x%x, error: %d\n" , call_reg_str, reg, ret);sev_es_terminate (SEV_TERM_SET_LINUX ,GHCB_TERM_ALT_INJ_FAIL ); }break ;default : -pr_err ("%s 0x%x not supported\n" , call_reg_str, reg); -return 0 ; +if (write) { +sev_apic_ghcb_msr_write (reg, v); + }else { +return sev_apic_ghcb_msr_read (reg); + }
You don't need the { } braces for single statements. And you can do:
arch/x86/kernel/apic/x2apic_svsm.c +4 -0
@@ -76,6 +76,10 @@ static u32 __svsm_apic_msr_rw(u32 reg, u32 v, bool write)break ;default :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);
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquettePATCH 5/8 x86/sev: Add a function to contain all SEV-specific setup operations
In reply to Melody WangTo 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.
--- arch/x86/boot/compressed/sev.c | 11 +++++++++++ arch/x86/include/asm/sev.h | 3 +++ drivers/firmware/efi/libstub/x86-stub.c | 17 +++-------------- 3 files changed, 17 insertions(+), 14 deletions(-)
arch/x86/boot/compressed/sev.c +11 -0
@@ -511,3 +511,14 @@ bool early_is_sevsnp_guest(void) }return true; } + +bool sev_prepare (void ) +{ +u64 unsupported =snp_get_unsupported_features (sev_get_status ()); +if (unsupported) { +error ("Unsupported SEV-SNP features detected\n" ); +return true; + } + +return false; +}
arch/x86/include/asm/sev.h +3 -0
@@ -605,6 +605,8 @@ static inline void sev_evict_cache(void *va, int npages) } } +bool sev_prepare (void ); +#else /* !CONFIG_AMD_MEM_ENCRYPT */ #define snp_vmpl 0
@@ -652,6 +654,7 @@ static inline enum es_result savic_register_gpa(u64 gpa) { return ES_UNSUPPORTEDstatic 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 bool sev_prepare (void ) {return false; }#endif /* CONFIG_AMD_MEM_ENCRYPT */
drivers/firmware/efi/libstub/x86-stub.c +3 -14
@@ -783,19 +783,6 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)return EFI_SUCCESS ; } -static bool have_unsupported_snp_features (void ) -{ -u64 unsupported; - - unsupported =snp_get_unsupported_features (sev_get_status ()); -if (unsupported) { -efi_err ("Unsupported SEV-SNP features detected: 0x%llx\n" , - unsupported); -return true; - } -return false; -} -static void efi_get_seed (void *seed,int size) {efi_get_random_bytes (size, seed);
@@ -919,6 +906,7 @@ void __noreturn efi_stub_entry(efi_handle_t handle,unsigned long kernel_entry;struct setup_header *hdr;efi_status_t status; +bool err; efi_system_table = sys_table_arg;/* Check if we were booted by the EFI firmware */
@@ -933,7 +921,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle, hdr = &boot_params->hdr; -if (have_unsupported_snp_features ()) + err =sev_prepare (); +if (err)efi_exit (handle,EFI_UNSUPPORTED );if (IS_ENABLED (CONFIG_EFI_DXE_MEM_ATTRIBUTES )) {
Signature · 1 line
2.43.0
Borislav Petkov
maintainerRe: [PATCH v1 5/8] x86/sev: Add a function to contain all SEV-specific setup operations
arch/x86/boot/compressed/sev.c bool early_is_sevsnp_guest(void) · line 520
Quoted 6 lines · Melody Wang
arch/x86/boot/compressed/sev.c +6
-0+bool sev_prepare (void ) +{ +u64 unsupported =snp_get_unsupported_features (sev_get_status ()); +if (unsupported) { +error ("Unsupported SEV-SNP features detected\n" ); +return true;
Sashiko says here:
| Does calling error() here inadvertently cause a silent system hang?
| Since error() is a __noreturn function that halts the system via an infinite
| asm("hlt") loop, the return statement directly below it becomes unreachable
| dead code.Yes dear Sashiko, it used to do that before too with
efi_exit(handle, EFI_UNSUPPORTED);
| Additionally, the early diagnostic output from error() might fail because the | console lines and cols are not yet initialized in the EFI stub context before | extract_kernel() runs, resulting in dropped diagnostic information.
Not a big surprise there.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquettePATCH 6/8 x86/sev: Register the guest with the SVSM APIC protocol
In reply to Melody WangThe 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 | 17 +++++++++++++++++ arch/x86/boot/compressed/sev.h | 6 ++++++ 2 files changed, 23 insertions(+)
arch/x86/boot/compressed/sev.c +17 -0
@@ -520,5 +520,22 @@ bool sev_prepare(void)return true; } +/* Register Alternate Injection */ +if (early_is_sevsnp_guest () && snp_vmpl) { +struct svsm_call call = {}; +int ret; + +if (!(sev_get_status () &MSR_AMD64_SNP_ALTERNATE_INJ )) +return 0 ; + + 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 ); + } +return false; }
arch/x86/boot/compressed/sev.h +6 -0
@@ -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 );
Signature · 1 line
2.43.0
Borislav Petkov
maintainerRe: [PATCH v1 6/8] x86/sev: Register the guest with the SVSM APIC protocol
arch/x86/boot/compressed/sev.c bool sev_prepare(void) · line 529
Quoted 11 lines · Melody Wang
arch/x86/boot/compressed/sev.c +7
-0@@ -520,5 +520,22 @@ bool sev_prepare(void)return true; } +/* Register Alternate Injection */ +if (early_is_sevsnp_guest () && snp_vmpl) { +struct svsm_call call = {}; +int ret; + +if (!(sev_get_status () &MSR_AMD64_SNP_ALTERNATE_INJ )) +return 0 ;
Function is a bool - this needs to be
return false;
Quoted 12 lines
arch/x86/boot/compressed/sev.c +10
-0+ + 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 ); + } +return false; }
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquettePATCH 7/8 x86/sev: Allow the guest to configure interrupt vectors for the hypervisor
In reply to Melody WangThe 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 | 12 +++++++++++- arch/x86/boot/compressed/sev.h | 7 +++++++ 2 files changed, 18 insertions(+), 1 deletion(-)
arch/x86/boot/compressed/sev.c +11 -1
@@ -520,7 +520,10 @@ bool sev_prepare(void)return true; } -/* Register Alternate Injection */ +/* When the guest is running at VMPL2 with Alternate Injection enabled, +* register Alternate Injection, and configure all of interrupts as +* permissive by default. +*/ if (early_is_sevsnp_guest () && snp_vmpl) {struct svsm_call call = {};int ret;
@@ -535,6 +538,13 @@ bool sev_prepare(void) ret =svsm_call_msr_protocol (&call);if (ret)sev_es_terminate (SEV_TERM_SET_LINUX ,GHCB_TERM_ALT_INJ_FAIL ); + + call.rax =SVSM_APIC_CALL (SVSM_APIC_CONFIG_VECTOR ); + call.rcx =SVSM_IRQ_ENABLE_ALL ; + + ret =svsm_call_msr_protocol (&call); +if (svsm_call_msr_protocol (&call)) +sev_es_terminate (SEV_TERM_SET_LINUX ,GHCB_TERM_ALT_INJ_FAIL ); }return false;
arch/x86/boot/compressed/sev.h +7 -0
@@ -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 );
Signature · 1 line
2.43.0
Borislav Petkov
maintainerRe: [PATCH v1 7/8] x86/sev: Allow the guest to configure interrupt vectors for the hypervisor
Quoted 20 lines · Melody Wang
The SVSM APIC protocol supports 5 API calls.
SVSM_APIC_CONFIGURE_VECTOR(shortened toSVSM_APIC_CONFIG_VECTORfor 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.
Signed-off-by Melody Wang <[email protected]>--- arch/x86/boot/compressed/sev.c | 12 +++++++++++- arch/x86/boot/compressed/sev.h | 7 +++++++ 2 files changed, 18 insertions(+), 1 deletion(-)arch/x86/boot/compressed/sev.c +0
-0
arch/x86/boot/compressed/sev.c bool sev_prepare(void) · line 526
Quoted 8 lines
arch/x86/boot/compressed/sev.c +4
-1@@ -520,7 +520,10 @@ bool sev_prepare(void)return true; } -/* Register Alternate Injection */ +/* When the guest is running at VMPL2 with Alternate Injection enabled, +* register Alternate Injection, and configure all of interrupts as +* permissive by default. +*/
Kernel comments style format is:
/*
* A sentence ending with a full-stop.
* Another sentence. ...
* More sentences. ...
*/
arch/x86/boot/compressed/sev.c bool sev_prepare(void) · line 546
Quoted 13 lines
arch/x86/boot/compressed/sev.c +6
-0if (early_is_sevsnp_guest () && snp_vmpl) {struct svsm_call call = {};int ret;@@ -535,6 +538,13 @@ bool sev_prepare(void) ret =svsm_call_msr_protocol (&call);if (ret)sev_es_terminate (SEV_TERM_SET_LINUX ,GHCB_TERM_ALT_INJ_FAIL ); + + call.rax =SVSM_APIC_CALL (SVSM_APIC_CONFIG_VECTOR ); + call.rcx =SVSM_IRQ_ENABLE_ALL ; + + ret =svsm_call_msr_protocol (&call); +if (svsm_call_msr_protocol (&call))
That looks like a typo here and Sashiko is correct:
|Does this code inadvertently execute svsm_call_msr_protocol() twice when |booting an SEV-SNP guest at VMPL2 with Alternate Injection enabled?
IOW:
if (ret)
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquettePATCH 8/8 x86/sev: Indicate that Alternate Injection is supported in the guest
In reply to Melody WangNow 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
@@ -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