On 11/01/23 21:11, Tom Lendacky wrote: > On 1/11/23 00:45, Nikunj A Dadhania wrote: >> The hypervisor can enable various new features (SEV_FEATURES[1:63]) >> and start the SNP guest. Some of these features need guest side >> implementation. If any of these features are enabled without guest >> side implementation, the behavior of the SNP guest will be undefined. >> The SNP guest boot may fail in a non-obvious way making it difficult >> to debug. >> >> Instead of allowing the guest to continue and have it fail randomly >> later, detect this early and fail gracefully. >> >> SEV_STATUS MSR indicates features which the hypervisor has enabled. >> While booting, SNP guests should ascertain that all the enabled >> features have guest side implementation. In case any feature is not >> implemented in the guest, the guest terminates booting with GHCB >> protocol Non-Automatic Exit(NAE) termination request event[1]. Populate >> SW_EXITINFO2 with mask of unsupported features that the hypervisor >> can easily report to the user. >> >> More details in AMD64 APM[2] Vol 2: 15.34.10 SEV_STATUS MSR >> >> [1] https://developer.amd.com/wp-content/resources/56421.pdf >> 4.1.13 Termination Request >> >> [2] https://www.amd.com/system/files/TechDocs/40332_4.05.pdf >> >> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support") >> CC: Borislav Petkov <bp@xxxxxxxxx> >> CC: David Rientjes <rientjes@xxxxxxxxxx> >> CC: Michael Roth <michael.roth@xxxxxxx> >> CC: Tom Lendacky <thomas.lendacky@xxxxxxx> >> CC: <stable@xxxxxxxxxx> >> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx> >> >> --- >> >> Changes: >> v3: >> * Use GHCB protocol NAE termination event SEV-SNP feature(s) >> not supported along with SW_EXITINFO2 containing mask of the >> unsupported features. Need handling of this event on the HV. >> * Add the SNP features check initialize_identity_maps() when the >> boot GHCB page can be initialized and used. >> * Fixed sphinx warnings in documentation >> >> v2: >> * Updated Documentation/x86/amd-memory-encryption.rst >> * Address review feedback from Boris/Tom >> >> v1: >> * Dropped _ENABLED from the feature bits >> * Use approprate macro/function names and move closer to the function where >> it is used. >> * More details added to the commit message and comments >> * Fixed compilation issue >> --- >> Documentation/x86/amd-memory-encryption.rst | 36 +++++++++++++ >> arch/x86/boot/compressed/head_64.S | 10 ++++ >> arch/x86/boot/compressed/misc.h | 2 + >> arch/x86/boot/compressed/sev.c | 59 +++++++++++++++++++++ >> arch/x86/include/asm/msr-index.h | 20 +++++++ >> arch/x86/include/asm/sev-common.h | 1 + >> arch/x86/include/uapi/asm/svm.h | 10 ++++ >> 7 files changed, 138 insertions(+) >> >> diff --git a/Documentation/x86/amd-memory-encryption.rst b/Documentation/x86/amd-memory-encryption.rst >> index a1940ebe7be5..b3adc39d7735 100644 >> --- a/Documentation/x86/amd-memory-encryption.rst >> +++ b/Documentation/x86/amd-memory-encryption.rst >> @@ -95,3 +95,39 @@ by supplying mem_encrypt=on on the kernel command line. However, if BIOS does >> not enable SME, then Linux will not be able to activate memory encryption, even >> if configured to do so by default or the mem_encrypt=on command line parameter >> is specified. >> + >> +Secure Nested Paging (SNP) >> +========================== >> + >> +SEV-SNP introduces new features (SEV_FEATURES[1:63]) which can be enabled >> +by the hypervisor for security enhancements. Some of these features need >> +guest side implementation to function correctly. The below table lists the >> +expected guest behavior with various possible scenarios of guest/hypervisor >> +SNP feature support. >> + >> ++-----------------+---------------+---------------+------------------+ >> +| Feature Enabled | Guest needs | Guest has | Guest boot | >> +| by the HV | implementation| implementation| behaviour | >> ++=================+===============+===============+==================+ >> +| No | No | No | Boot | >> +| | | | | >> ++-----------------+---------------+---------------+------------------+ >> +| No | Yes | No | Boot | >> +| | | | | >> ++-----------------+---------------+---------------+------------------+ >> +| No | Yes | Yes | Boot | >> +| | | | | >> ++-----------------+---------------+---------------+------------------+ >> +| Yes | No | No | Boot with | >> +| | | | feature enabled | >> ++-----------------+---------------+---------------+------------------+ >> +| Yes | Yes | No | Graceful boot | >> +| | | | failure | >> ++-----------------+---------------+---------------+------------------+ >> +| Yes | Yes | Yes | Boot with | >> +| | | | feature enabled | >> ++-----------------+---------------+---------------+------------------+ >> + >> +More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR >> + >> +[1] https://www.amd.com/system/files/TechDocs/40332_4.05.pdf >> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S >> index a75712991df3..22037443e958 100644 >> --- a/arch/x86/boot/compressed/head_64.S >> +++ b/arch/x86/boot/compressed/head_64.S >> @@ -557,6 +557,16 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated) >> /* Pass boot_params to initialize_identity_maps() */ >> movq (%rsp), %rdi >> call initialize_identity_maps >> + >> +#ifdef CONFIG_AMD_MEM_ENCRYPT >> + /* >> + * Now that the required page table and mappings are done, early boot ghcb >> + * page can be setup and used. Check for SNP guest/HV feature compatibility >> + * and terminate the guest providing exit information in boot ghcb. >> + */ > > How about a more concise comment...> > /* > * Now that the required page table mappings are established and a > * GHCB can be used, check for SNP guest/HV feature compatibility. > */ Yes, better. >> + call snp_check_features >> +#endif >> + >> popq %rsi >> /* >> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h >> index 62208ec04ca4..0bc3639be1f8 100644 >> --- a/arch/x86/boot/compressed/misc.h >> +++ b/arch/x86/boot/compressed/misc.h >> @@ -126,6 +126,7 @@ static inline void console_init(void) >> #ifdef CONFIG_AMD_MEM_ENCRYPT >> void sev_enable(struct boot_params *bp); >> +void snp_check_features(void); >> void sev_es_shutdown_ghcb(void); >> extern bool sev_es_check_ghcb_fault(unsigned long address); >> void snp_set_page_private(unsigned long paddr); >> @@ -143,6 +144,7 @@ static inline void sev_enable(struct boot_params *bp) >> if (bp) >> bp->cc_blob_address = 0; >> } >> +static void snp_check_features(void) { } > > Unneeded since you're wrapping the call in a #ifdef check. Will drop it. > >> static inline void sev_es_shutdown_ghcb(void) { } >> static inline bool sev_es_check_ghcb_fault(unsigned long address) >> { >> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c >> index c93930d5ccbd..a26a5d6949c3 100644 >> --- a/arch/x86/boot/compressed/sev.c >> +++ b/arch/x86/boot/compressed/sev.c >> @@ -270,6 +270,65 @@ static void enforce_vmpl0(void) >> sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0); >> } >> +/* >> + * SNP_FEATURES_IMPL_REQ is the mask of SNP features that will need >> + * guest side implementation for proper functioning of the guest. If any >> + * of these features are enabled in the hypervisor but are lacking guest >> + * side implementation, the behavior of the guest will be undefined. The >> + * guest could fail in non-obvious way making it difficult to debug. >> + * >> + * As the behavior of reserved feature bits is unknown to be on the >> + * safe side add them to the required features mask. >> + */ >> +#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_DEBUG_SWAP | \ >> + MSR_AMD64_SNP_VMPL_SSS | \ >> + MSR_AMD64_SNP_SECURE_TSC | \ >> + MSR_AMD64_SNP_VMGEXIT_PARAM | \ >> + MSR_AMD64_SNP_VMSA_REG_PROTECTION | \ >> + MSR_AMD64_SNP_RESERVED_BIT13 | \ >> + MSR_AMD64_SNP_RESERVED_BIT15 | \ >> + MSR_AMD64_SNP_RESERVED_MASK) > > Should these be indented one extra space to line up with MSR_AMD64_SNP_VTOM? Boris in his comment on v2 had it indented till tab, I had used same intendation. >> + >> +/* >> + * SNP_FEATURES_PRESENT is the mask of SNP features that are implemented >> + * by the guest kernel. As and when a new feature is implemented in the >> + * guest kernel, a corresponding bit should be added to the mask. >> + */ >> +#define SNP_FEATURES_PRESENT (0) >> + >> +void snp_check_features(void) >> +{ >> + u64 unsupported_features; >> + >> + if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED)) >> + return; >> + >> + /* >> + * Terminate the boot if hypervisor has enabled any feature >> + * lacking guest side implementation. >> + */ >> + unsupported_features = sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT; >> + if (unsupported_features) { >> + u64 exit_info_1 = SVM_VMGEXIT_TERM_REASON(SVM_VMGEXIT_TERM_REASON_SET, > > This should be SEV_TERM_SET_GEN (or see below). Yes, will use reason set as SEV_TERM_SET_GEN and reason code as GHCB_SNP_UNSUPPORTED. > >> + SVM_VMGEXIT_TERM_SNP_FEAT_UNSUPPORTED); >> + >> + if (!boot_ghcb && !early_setup_ghcb()) >> + sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_FEAT_NOT_IMPLEMENTED); >> + > > You need to call vc_ghcb_invalidate() before doing any ghcb_set*() calls. Ack. > >> + ghcb_set_sw_exit_code(boot_ghcb, SVM_VMGEXIT_TERM_REQUEST); >> + ghcb_set_sw_exit_info_1(boot_ghcb, exit_info_1); >> + ghcb_set_sw_exit_info_2(boot_ghcb, unsupported_features); > > Add a blank line here. Ok > >> + sev_es_wr_ghcb_msr(__pa(boot_ghcb)); >> + VMGEXIT(); > > Add a blank line here. > Ok >> + while (true) >> + asm volatile("hlt\n" : : : "memory"); >> + } >> +} >> + >> void sev_enable(struct boot_params *bp) >> { >> unsigned int eax, ebx, ecx, edx; >> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h >> index 37ff47552bcb..d3fe82c5d6b6 100644 >> --- a/arch/x86/include/asm/msr-index.h >> +++ b/arch/x86/include/asm/msr-index.h >> @@ -566,6 +566,26 @@ >> #define MSR_AMD64_SEV_ES_ENABLED BIT_ULL(MSR_AMD64_SEV_ES_ENABLED_BIT) >> #define MSR_AMD64_SEV_SNP_ENABLED BIT_ULL(MSR_AMD64_SEV_SNP_ENABLED_BIT) >> +/* SNP feature bits enabled by the hypervisor */ >> +#define MSR_AMD64_SNP_VTOM BIT_ULL(3) >> +#define MSR_AMD64_SNP_REFLECT_VC BIT_ULL(4) >> +#define MSR_AMD64_SNP_RESTRICTED_INJ BIT_ULL(5) >> +#define MSR_AMD64_SNP_ALT_INJ BIT_ULL(6) >> +#define MSR_AMD64_SNP_DEBUG_SWAP BIT_ULL(7) >> +#define MSR_AMD64_SNP_PREVENT_HOST_IBS BIT_ULL(8) >> +#define MSR_AMD64_SNP_BTB_ISOLATION BIT_ULL(9) >> +#define MSR_AMD64_SNP_VMPL_SSS BIT_ULL(10) >> +#define MSR_AMD64_SNP_SECURE_TSC BIT_ULL(11) >> +#define MSR_AMD64_SNP_VMGEXIT_PARAM BIT_ULL(12) >> +#define MSR_AMD64_SNP_IBS_VIRT BIT_ULL(14) >> +#define MSR_AMD64_SNP_VMSA_REG_PROTECTION BIT_ULL(16) >> +#define MSR_AMD64_SNP_SMT_PROTECTION BIT_ULL(17) >> + >> +/* SNP feature bits reserved for future use. */ >> +#define MSR_AMD64_SNP_RESERVED_BIT13 BIT_ULL(13) >> +#define MSR_AMD64_SNP_RESERVED_BIT15 BIT_ULL(15) >> +#define MSR_AMD64_SNP_RESERVED_MASK GENMASK_ULL(63, 18) >> + >> #define MSR_AMD64_VIRT_SPEC_CTRL 0xc001011f >> /* AMD Collaborative Processor Performance Control MSRs */ >> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h >> index b8357d6ecd47..db60cbb01b31 100644 >> --- a/arch/x86/include/asm/sev-common.h >> +++ b/arch/x86/include/asm/sev-common.h >> @@ -148,6 +148,7 @@ struct snp_psc_desc { >> #define GHCB_SEV_ES_GEN_REQ 0 >> #define GHCB_SEV_ES_PROT_UNSUPPORTED 1 >> #define GHCB_SNP_UNSUPPORTED 2 >> +#define GHCB_SNP_FEAT_NOT_IMPLEMENTED 3 > > No, you can't create a new value to the SEV_TERM_SET_GEN without modifying the GHCB spec. So please use GHCB_SNP_UNSUPPORTED if using the SEV_TERM_SET_GEN set or else add a new value to be used with the SEV_TERM_SET_LINUX set. Agree, this anyways is not used now. > >> /* Linux-specific reason codes (used with reason set 1) */ >> #define SEV_TERM_SET_LINUX 1 >> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h >> index f69c168391aa..5bd81adfb114 100644 >> --- a/arch/x86/include/uapi/asm/svm.h >> +++ b/arch/x86/include/uapi/asm/svm.h >> @@ -116,6 +116,16 @@ >> #define SVM_VMGEXIT_AP_CREATE 1 >> #define SVM_VMGEXIT_AP_DESTROY 2 >> #define SVM_VMGEXIT_HV_FEATURES 0x8000fffd >> +#define SVM_VMGEXIT_TERM_REQUEST 0x8000fffe >> +#define SVM_VMGEXIT_TERM_REASON_SET 0 >> +#define SVM_VMGEXIT_TERM_GENERAL 0 >> +#define SVM_VMGEXIT_TERM_SEVES 1 >> +#define SVM_VMGEXIT_TERM_SNP_FEAT_UNSUPPORTED 2 > > This NAE event uses the same reason code set information as the MSR protocol, so the above 4 definitions are not needed or the definitions in sev-common.h should be redefined to use these defines, e.g.: > > #define SEV_TERM_SET_GEN SVM_VMGEXIT_TERM_REASON_SET > #define GHCB_SEV_ES_GEN_REQ SVM_VMGEXIT_TERM_GENERAL > ... Will use the defines from sev-common.h Regards Nikunj
