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|
+---------------+---------------+---------------+---------------+
More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR
[1] https://www.amd.com/system/files/TechDocs/40332_4.05.pdf
Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
CC: Borislav Petkov <[email protected]>
CC: Michael Roth <[email protected]>
CC: Tom Lendacky <[email protected]>
CC: <[email protected]>
Signed-off-by: Nikunj A Dadhania <[email protected]>
---
Changes:
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
---
arch/x86/boot/compressed/sev.c | 51 +++++++++++++++++++++++++++++++
arch/x86/include/asm/msr-index.h | 20 ++++++++++++
arch/x86/include/asm/sev-common.h | 1 +
3 files changed, 72 insertions(+)
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,
+ * 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.
+ */
+#define SNP_FEATURES_HAS_GUEST_IMPLEMENTATION (0)
+
+/*
+ * 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.
+ */
+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);
+}
+
void sev_enable(struct boot_params *bp)
{
unsigned int eax, ebx, ecx, edx;
@@ -335,6 +379,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
+ * missing guest side implementation.
+ */
+ if (!snp_guest_has_features_implemented())
+ sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_FEAT_NOT_IMPLEMENTED);
+
enforce_vmpl0();
}
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 4a2af82553e4..91447f018f6e 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -571,6 +571,26 @@
#define MSR_AMD64_SEV_ES_ENABLED BIT_ULL(MSR_AMD64_SEV_ES_ENABLED_BIT)
#define MSR_AMD64_SEV_SNP_ENABLED BIT_ULL(MSR_AMD64_SEV_SNP_ENABLED_BIT)
+/* SNP feature bits enabled by the hypervisor */
+#define MSR_AMD64_SNP_VTOM BIT_ULL(3)
+#define MSR_AMD64_SNP_REFLECT_VC BIT_ULL(4)
+#define MSR_AMD64_SNP_RESTRICTED_INJ BIT_ULL(5)
+#define MSR_AMD64_SNP_ALT_INJ BIT_ULL(6)
+#define MSR_AMD64_SNP_DEBUG_SWAP BIT_ULL(7)
+#define MSR_AMD64_SNP_PREVENT_HOST_IBS BIT_ULL(8)
+#define MSR_AMD64_SNP_BTB_ISOLATION BIT_ULL(9)
+#define MSR_AMD64_SNP_VMPL_SSS BIT_ULL(10)
+#define MSR_AMD64_SNP_SECURE_TSC BIT_ULL(11)
+#define MSR_AMD64_SNP_VMGEXIT_PARAM BIT_ULL(12)
+#define MSR_AMD64_SNP_IBS_VIRT BIT_ULL(14)
+#define MSR_AMD64_SNP_VMSA_REG_PROTECTION BIT_ULL(16)
+#define MSR_AMD64_SNP_SMT_PROTECTION BIT_ULL(17)
+
+/* SNP feature bits reserved for future use. */
+#define MSR_AMD64_SNP_RESERVED_BIT13 BIT_ULL(13)
+#define MSR_AMD64_SNP_RESERVED_BIT15 BIT_ULL(15)
+#define MSR_AMD64_SNP_RESERVED_MASK GENMASK_ULL(63, 18)
+
#define MSR_AMD64_VIRT_SPEC_CTRL 0xc001011f
/* AMD Collaborative Processor Performance Control MSRs */
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index b8357d6ecd47..db60cbb01b31 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -148,6 +148,7 @@ struct snp_psc_desc {
#define GHCB_SEV_ES_GEN_REQ 0
#define GHCB_SEV_ES_PROT_UNSUPPORTED 1
#define GHCB_SNP_UNSUPPORTED 2
+#define GHCB_SNP_FEAT_NOT_IMPLEMENTED 3
/* Linux-specific reason codes (used with reason set 1) */
#define SEV_TERM_SET_LINUX 1
--
2.32.0
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
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();
>
On Mon, Dec 05, 2022 at 08:32:52PM +0530, Nikunj A. Dadhania wrote:
> Looks good. Do you want me to send v3 with these changes ?
Sure, pls do, after testing it again.
But no hurry because the merge window opens next week and that one will
have to wait for after it.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 12/5/22 08:30, 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.
>
>> 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
This makes it sound like these features are required to run SNP, which
they're not. It's that we can't run SNP with these features without
support in the guest. So while the patch name is long, it is accurate.
Maybe SNP_FEATURES_NEED_IMPLEMENTATION if you want to shorten it a little?
Thanks,
Tom
>
> 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();
>
On Mon, Dec 05, 2022 at 09:23:17AM -0600, Tom Lendacky wrote:
> This makes it sound like these features are required to run SNP, which
> they're not. It's that we can't run SNP with these features without support
> in the guest. So while the patch name is long, it is accurate.
Maybe, but it is a mouthful this way. And unreadable. And there's plenty
of comments explaining what it is. And it is used in one place only.
> Maybe SNP_FEATURES_NEED_IMPLEMENTATION if you want to shorten it a little?
"need implementation" where? Host? Guest?
I.e., we're relying on comments to explain what the name is, one way or
the other.
How about a short and sweet:
if (sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT)
along with the explanation in a comment above it. Comment which is a
must regardless.
Hmm.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette