With MSR_AMD64_SEV_DEBUG_SWAP enabled, the VM should not get #VC
events for DR7 read/write which it rather avoided.
Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
Reviewed-by: Borislav Petkov (AMD) <bp@alien8.de>
---
Changes:
v2:
* use new bit definition
---
arch/x86/include/asm/msr-index.h | 1 +
tools/arch/x86/include/asm/msr-index.h | 1 +
arch/x86/kernel/sev.c | 6 ++++++
3 files changed, 8 insertions(+)
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index cb3d0f6e6ac2..e15afe3500ff 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -574,6 +574,7 @@
#define MSR_AMD64_SEV_ENABLED BIT_ULL(MSR_AMD64_SEV_ENABLED_BIT)
#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)
+#define MSR_AMD64_SEV_DEBUG_SWAP BIT_ULL(7)
#define MSR_AMD64_VIRT_SPEC_CTRL 0xc001011f
diff --git a/tools/arch/x86/include/asm/msr-index.h b/tools/arch/x86/include/asm/msr-index.h
index 37ff47552bcb..27c1c349e49b 100644
--- a/tools/arch/x86/include/asm/msr-index.h
+++ b/tools/arch/x86/include/asm/msr-index.h
@@ -565,6 +565,7 @@
#define MSR_AMD64_SEV_ENABLED BIT_ULL(MSR_AMD64_SEV_ENABLED_BIT)
#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)
+#define MSR_AMD64_SEV_DEBUG_SWAP BIT_ULL(7)
#define MSR_AMD64_VIRT_SPEC_CTRL 0xc001011f
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 679026a640ef..8184f8ba4edc 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1618,6 +1618,9 @@ static enum es_result vc_handle_dr7_write(struct ghcb *ghcb,
long val, *reg = vc_insn_get_rm(ctxt);
enum es_result ret;
+ if (sev_status & MSR_AMD64_SEV_DEBUG_SWAP)
+ return ES_VMM_ERROR;
+
if (!reg)
return ES_DECODE_FAILED;
@@ -1655,6 +1658,9 @@ static enum es_result vc_handle_dr7_read(struct ghcb *ghcb,
struct sev_es_runtime_data *data = this_cpu_read(runtime_data);
long *reg = vc_insn_get_rm(ctxt);
+ if (sev_status & MSR_AMD64_SEV_DEBUG_SWAP)
+ return ES_VMM_ERROR;
+
if (!reg)
return ES_DECODE_FAILED;
--
2.38.1On 20/01/23 08:40, Alexey Kardashevskiy wrote: > With MSR_AMD64_SEV_DEBUG_SWAP enabled, the VM should not get #VC > events for DR7 read/write which it rather avoided. > SNP guest feature negotiation patch is part of tip now: https://lore.kernel.org/lkml/167414649850.4906.1693185384677559889.tip-bot2@tip-bot2/ MSR_AMD64_SNP_DEBUG_SWAP is already defined. As this requires guest side changes, please add MSR_AMD64_SNP_DEBUG_SWAP as part of SNP_FEATURES_PRESENT bit mask. Regards Nikunj
On 20/1/23 16:12, Nikunj A. Dadhania wrote: > On 20/01/23 08:40, Alexey Kardashevskiy wrote: >> With MSR_AMD64_SEV_DEBUG_SWAP enabled, the VM should not get #VC >> events for DR7 read/write which it rather avoided. >> > > SNP guest feature negotiation patch is part of tip now: https://lore.kernel.org/lkml/167414649850.4906.1693185384677559889.tip-bot2@tip-bot2/ Worth mentioning it is tip/x86/urgent (which does not have X86_FEATURE_NO_NESTED_DATA_BP), not tip/master (which has X86_FEATURE_NO_NESTED_DATA_BP). > > MSR_AMD64_SNP_DEBUG_SWAP is already defined. As this requires guest side changes, please add MSR_AMD64_SNP_DEBUG_SWAP as part of SNP_FEATURES_PRESENT bit mask. It is MSR_AMD64_SEV_DEBUG_SWAP (SEV, not SNP), it is an SEV-ES thing. Why is that feature negotiation SNP-only and not SEV? -- Alexey
On 20/01/23 15:53, Alexey Kardashevskiy wrote: > It is MSR_AMD64_SEV_DEBUG_SWAP (SEV, not SNP), it is an SEV-ES thing. Yes, noticed that, earlier analysis was that Debug Swap shouldn't need any guest side changes, but it does need it. > Why is that feature negotiation SNP-only and not SEV? As per the spec, GHCB termination request: reason code: 0x2 is SNP features specific. Regards Nikunj
On 24/1/23 21:37, Nikunj A. Dadhania wrote:>> It is MSR_AMD64_SEV_DEBUG_SWAP (SEV, not SNP), it is an SEV-ES thing.> Yes, noticed that, earlier analysis was that Debug Swap shouldn't need any guest side changes, but it does need it.>>> Why is that feature negotiation SNP-only and not SEV?> > As per the spec, GHCB termination request: reason code: 0x2 is SNP features specific. Does the guest really need to terminate in such case? A VM could just not do the GHCB thing if it does not want to. -- Alexey
On 24/01/23 18:07, Alexey Kardashevskiy wrote: > > > On 24/1/23 21:37, Nikunj A. Dadhania wrote: >> It is MSR_AMD64_SEV_DEBUG_SWAP (SEV, not SNP), it is an SEV-ES thing. > Yes, noticed that, earlier analysis was that Debug Swap shouldn't need any guest side changes, but it does need it. >>> Why is that feature negotiation SNP-only and not SEV? >> As per the spec, GHCB termination request: reason code: 0x2 is SNP features specific. > Does the guest really need to terminate in such case? The termination is from the guest that do not have implementation for the hypervisor enabled feature, in this case DebugSwap. If DebugSwap is enabled by the hypervisor and not handled in guest #VC, then DR7 read/write can be intercepted by the malicious hypervisor, which can return unexpected values. > A VM could just not do the GHCB thing if it does not want to. In that case, the VM can have unexpected failures. Regards Nikunj
On Fri, Jan 20, 2023 at 09:23:48PM +1100, Alexey Kardashevskiy wrote:
> Worth mentioning it is tip/x86/urgent (which does not have
> X86_FEATURE_NO_NESTED_DATA_BP), not tip/master (which has
> X86_FEATURE_NO_NESTED_DATA_BP).
Yeah, when you submit patches for tip, you can always use tip/master which has
the latest lineup of all branches and should have all the required bits.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 20/1/23 23:06, Borislav Petkov wrote: > On Fri, Jan 20, 2023 at 09:23:48PM +1100, Alexey Kardashevskiy wrote: >> Worth mentioning it is tip/x86/urgent (which does not have >> X86_FEATURE_NO_NESTED_DATA_BP), not tip/master (which has >> X86_FEATURE_NO_NESTED_DATA_BP). > > Yeah, when you submit patches for tip, you can always use tip/master which has > the latest lineup of all branches and should have all the required bits. The tip/master's head just a few days ago was 195df42eb64dcb "Merge branch into tip/master: 'x86/platform'" and did have dcf67f724b8ada "x86/cpu, kvm: Add the NO_NESTED_DATA_BP feature" but now tip/master does not have it - 8272720be044 "Merge x86/cache into tip/master". /me confused. What tree is the tree? > > Thx. > -- Alexey
On Wed, Jan 25, 2023 at 02:11:27PM +1100, Alexey Kardashevskiy wrote:
> The tip/master's head just a few days ago was 195df42eb64dcb "Merge branch
> into tip/master: 'x86/platform'" and did have dcf67f724b8ada "x86/cpu, kvm:
> Add the NO_NESTED_DATA_BP feature" but now tip/master does not have it -
Yeah, it had to be backed out. A new version will be queued soon.
> 8272720be044 "Merge x86/cache into tip/master". /me confused. What tree is
> the tree?
It still is *the* tree - just there were some issues with the KVM side of Kim's
series so it had to be dropped but he sent a new version which I need to queue
soon. Then I'll have a look at yours.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
With MSR_AMD64_SEV_DEBUG_SWAP enabled, the VM should not get #VC
events for DR7 read/write which it rather avoided.
Update the SNP_FEATURES_PRESENT mask with MSR_AMD64_SNP_DEBUG_SWAP so
an SNP guest can gracefully terminate during SNP feature negotiation.
For SEV-ES (which does not negotiate features) and enabled DebugSwap,
fail to handle DR7's #VC and return en error which in turn causes
panic() as there is no goot reason for the HV to keep intercepting DR7.
Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---
1/3 and 2/3 are the same as in v3 and fairly independent from this one.
The question now is what should SNP-feature-negotiation-aware guest do
when KVM enables DebugSwap.
"x86/sev: Add SEV-SNP guest feature negotiation support" is going to
reach the upstream long before any of these three from this thread.
It does not matter now as there is no SNP in upstream KVM.
---
Changes:
v4:
* rebased on top of SNP feature negotiation
v2:
* use new bit definition
---
arch/x86/boot/compressed/sev.c | 2 +-
arch/x86/kernel/sev.c | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index d63ad8f99f83..ac86f458951d 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -315,7 +315,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_DEBUG_SWAP
void snp_check_features(void)
{
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 679026a640ef..f29e60c496d7 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1618,6 +1618,9 @@ static enum es_result vc_handle_dr7_write(struct ghcb *ghcb,
long val, *reg = vc_insn_get_rm(ctxt);
enum es_result ret;
+ if (sev_status & MSR_AMD64_SNP_DEBUG_SWAP)
+ return ES_VMM_ERROR;
+
if (!reg)
return ES_DECODE_FAILED;
@@ -1655,6 +1658,9 @@ static enum es_result vc_handle_dr7_read(struct ghcb *ghcb,
struct sev_es_runtime_data *data = this_cpu_read(runtime_data);
long *reg = vc_insn_get_rm(ctxt);
+ if (sev_status & MSR_AMD64_SNP_DEBUG_SWAP)
+ return ES_VMM_ERROR;
+
if (!reg)
return ES_DECODE_FAILED;
--
2.39.1© 2016 - 2025 Red Hat, Inc.