Message-ID: <45487a87-764a-7ff3-292b-4a55fe29f7ba@amd.com>
Date:   Mon, 2 Jan 2023 20:50:23 +0530
From:   "Nikunj A. Dadhania" <nikunj@....com>
To:     David Rientjes <rientjes@...gle.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
        bp@...en8.de, mingo@...hat.com, tglx@...utronix.de,
        dave.hansen@...ux.intel.com, seanjc@...gle.com,
        pbonzini@...hat.com, thomas.lendacky@....com, michael.roth@....com,
        stable@...nel.org
Subject: Re: [PATCH v3] x86/sev: Add SEV-SNP guest feature negotiation support



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@...en8.de>
>> CC: Michael Roth <michael.roth@....com>
>> CC: Tom Lendacky <thomas.lendacky@....com>
>> CC: <stable@...nel.org>
>> Signed-off-by: Nikunj A Dadhania <nikunj@....com>
>>
>> ---
>>
>> 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

Powered by blists - more mailing lists