On 02/01/23 16:53, David Rientjes wrote: > On Mon, 2 Jan 2023, 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 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 SNP feature >> unsupported exit code. >> >> More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR >> >> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F40332_4.05.pdf&data=05%7C01%7Cnikunj.dadhania%40amd.com%7Cd4d286d55435487f7d4f08daecb3d3e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638082554474351848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=TLBCkg20WCaF77YrOzGC%2Bn9V3JFgVl2vs1XMPmSWHcM%3D&reserved=0 >> >> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support") >> CC: Borislav Petkov <bp@xxxxxxxxx> >> CC: Michael Roth <michael.roth@xxxxxxx> >> CC: Tom Lendacky <thomas.lendacky@xxxxxxx> >> CC: <stable@xxxxxxxxxx> >> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx> >> >> --- >> >> Changes: >> 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 | 35 +++++++++++++++++++ >> arch/x86/boot/compressed/sev.c | 37 +++++++++++++++++++++ >> arch/x86/include/asm/msr-index.h | 20 +++++++++++ >> arch/x86/include/asm/sev-common.h | 1 + >> 4 files changed, 93 insertions(+) >> >> diff --git a/Documentation/x86/amd-memory-encryption.rst b/Documentation/x86/amd-memory-encryption.rst >> index a1940ebe7be5..b8b6b87be995 100644 >> --- a/Documentation/x86/amd-memory-encryption.rst >> +++ b/Documentation/x86/amd-memory-encryption.rst >> @@ -95,3 +95,38 @@ 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 HV |implementation |implementation | behavior | >> ++---------------+---------------+---------------+---------------+ >> +| 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| >> ++---------------+---------------+---------------+---------------+ >> + > > Couple things: > > - I'd assume that the documentation would refer the reader to the > SNP_FEATURES_IMPL_REQ mask that defines whether the guest is required > to have a specific feature or not.> > - Don't hate me, but I feel wanting more from this, such as a rationale > for why certain features are required in the guest. The guest can boot without any of these features provided the hypervisor has not enabled any of the SNP_FEATURES_IMPL_REQ features. But in case the hypervisor does enable them, the guest needs to react in a predictable manner. These are optional security features provided for SNP guests which the hypervisor can enable. > When a guest fails to boot and the reasoning provided by a cloud provider > is "your guest doesn't support ${feature}", the natural question to ask > will be "why do I need ${feature}?" I think the "why" part depends on the user. Whether or not the user needs a certain feature enabled for the confidential guest. If the cloud provider(hypervisor) enables the feature on user request, the guest terminates with GHCB_SNP_FEAT_NOT_IMPLEMENTED when guest kernel does have corresponding code/implementation. > >> +More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR >> + >> +[1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F40332_4.05.pdf&data=05%7C01%7Cnikunj.dadhania%40amd.com%7Cd4d286d55435487f7d4f08daecb3d3e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638082554474351848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=TLBCkg20WCaF77YrOzGC%2Bn9V3JFgVl2vs1XMPmSWHcM%3D&reserved=0 >> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c >> index c93930d5ccbd..43793f46cfc1 100644 >> --- a/arch/x86/boot/compressed/sev.c >> +++ b/arch/x86/boot/compressed/sev.c >> @@ -270,6 +270,36 @@ 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. > > If one or more of the reserved feature bits turns out to not be required > for the SNP guest to function correctly, is this the correct choice? Yes, I think so. As they are undefined at the moment, it is safe to assume that a guest implementation is required. > > IIUC, legacy guests would fail to boot correctly (and unnecessarily, > because of this change) if they do not have an implentation for a > non-required feature. Absent this change, however, they would proceed > just fine. True, but the other way around guest behaviour may be undefined. > >> + */ >> +#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) >> + >> +/* >> + * 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 sev_enable(struct boot_params *bp) >> { >> unsigned int eax, ebx, ecx, edx; >> @@ -335,6 +365,13 @@ void sev_enable(struct boot_params *bp) >> if (!(get_hv_features() & GHCB_HV_FT_SNP)) >> sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED); >> >> + /* >> + * Terminate the boot if hypervisor has enabled any feature >> + * lacking guest side implementation. >> + */ >> + if (sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT) >> + sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_FEAT_NOT_IMPLEMENTED); > > We can't help out by specifying which feature(s)? The purpose of SNP_FEATURES_PRESENT is just that, at present no features that need guest implementation is part of the kernel. For e.g. I will be posting patches with SecureTSC enabled, that will make the following change. diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c index 43793f46cfc1..cd0948c6db50 100644 --- a/arch/x86/boot/compressed/sev.c +++ b/arch/x86/boot/compressed/sev.c @@ -298,7 +298,7 @@ static void enforce_vmpl0(void) * 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) +#define SNP_FEATURES_PRESENT (MSR_AMD64_SNP_SECURE_TSC) void sev_enable(struct boot_params *bp) { Regards, Nikunj
