Re: [PATCH v2] x86/sev: Add SEV-SNP guest feature negotiation support

[Nikunj A. Dadhania]

On 05/12/22 20:00, Borislav Petkov wrote:
> On Thu, Dec 01, 2022 at 03:34:23PM +0530, 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.
>>
>> 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|
>> +---------------+---------------+---------------+---------------+
> 
> I like this table and I wouldn't want for it to go under in some commit
> message which is not that easy to retrieve so I'm thinking you should
> add it along with some blurb to
> 
>   Documentation/x86/amd-memory-encryption.rst
> 
> instead where it belongs.

Sure will do.

> 
>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>> index c93930d5ccbd..571eb2576475 100644
>> --- a/arch/x86/boot/compressed/sev.c
>> +++ b/arch/x86/boot/compressed/sev.c
>> @@ -270,6 +270,50 @@ static void enforce_vmpl0(void)
>>  		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
>>  }
>>  
>> +/*
>> + * SNP_FEATURES_NEED_GUEST_IMPLEMENTATION 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 without guest side implementation,
> 
> "... are enabled in the hypervisor ... "
> 

Sure

>> + * the behavior of the guest will be undefined. The guest may fail in
>> + * non-obvious way making it difficult to debug.
>> + *
>> + * SNP reserved feature bits may or may not need guest side implementation.
> 
> Yah, get rid of that PPR formulation. If you see the verb "may" always
> run away. :-)
>

True, had to add "may" as the I don't know what the bit will be used for.
 
>> + * As the behavior of reserved feature bits are unknown, to be on the safer
>> + * side add them to the NEED_GUEST_IMPLEMENTATION mask.
> 
> Yah, that makes sense - you want to protect those for future use. Ack.
> 
>> + */
>> +#define SNP_FEATURES_NEED_GUEST_IMPLEMENTATION (MSR_AMD64_SNP_VTOM |			\
> 
> SNP_FEATURES_REQUIRED
> 
> Simpler and shorter.

Yes, much better.

> 
>> +						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_HAS_GUEST_IMPLEMENTATION 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.
> 
> And there's no way we won't notice that we've forgotten to do so because
> it'll terminate with the proper error code.
> 
>> + */
>> +#define SNP_FEATURES_HAS_GUEST_IMPLEMENTATION (0)
> 
> SNP_FEATURES_PRESENT
> 
> And so I've done a couple of changes ontop, here's a diff as it explains
> a lot better what I mean.
> 
> Have a look and let me know if something's wrong.

Looks good. Do you want me to send v3 with these changes ?

Regards
Nikunj

> 
> Thx.
> 
> ---
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 10272835dfe9..f023d37e2c41 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -271,48 +271,35 @@ static void enforce_vmpl0(void)
>  }
>  
>  /*
> - * SNP_FEATURES_NEED_GUEST_IMPLEMENTATION 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 without guest side implementation,
> - * the behavior of the guest will be undefined. The guest may fail in
> - * non-obvious way making it difficult to debug.
> - *
> - * SNP reserved feature bits may or may not need guest side implementation.
> - * As the behavior of reserved feature bits are unknown, to be on the safer
> - * side add them to the NEED_GUEST_IMPLEMENTATION mask.
> - */
> -#define SNP_FEATURES_NEED_GUEST_IMPLEMENTATION (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_HAS_GUEST_IMPLEMENTATION 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.
> + * SNP_FEATURES_REQUIRED 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_HAS_GUEST_IMPLEMENTATION (0)
> +#define SNP_FEATURES_REQUIRED		(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)
>  
>  /*
> - * The hypervisor can enable various features flags (in SEV_FEATURES[1:63]) and
> - * start the SNP guest. Certain SNP features need guest side implementation.
> - * Check if the SNP guest has implementation for those features.
> + * 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.
>   */
> -static bool snp_guest_has_features_implemented(void)
> -{
> -	u64 guest_features_not_implemented = SNP_FEATURES_NEED_GUEST_IMPLEMENTATION &
> -		~SNP_FEATURES_HAS_GUEST_IMPLEMENTATION;
> -
> -	return !(sev_status & guest_features_not_implemented);
> -}
> +#define SNP_FEATURES_PRESENT (0)
>  
>  void sev_enable(struct boot_params *bp)
>  {
> @@ -383,7 +370,7 @@ void sev_enable(struct boot_params *bp)
>  		 * Terminate the boot if hypervisor has enabled any feature
>  		 * missing guest side implementation.
>  		 */
> -		if (!snp_guest_has_features_implemented())
> +		if (sev_status & SNP_FEATURES_REQUIRED & ~SNP_FEATURES_PRESENT)
>  			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_FEAT_NOT_IMPLEMENTED);
>  
>  		enforce_vmpl0();
>