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

From: Borislav Petkov
Date: Mon Dec 05 2022 - 09:31:00 EST

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.

> 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 ... "

> + * 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. :-)

> + * 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.

> + 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.

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

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette