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();
>
