[PATCH 0/2] KVM: x86: RSM emulation DR6/DR7 cleanups

[PATCH 0/2] KVM: x86: RSM emulation DR6/DR7 cleanups

[Sean Christopherson]

A cleanup and a bug fix (technically) in the RSM loading of DR6 and DR7 I
stumbled on when looking at the bus lock detect DR6 changes.

Sean Christopherson (2):
  KVM: x86: Remove misleading DR6/DR7 adjustments from RSM emulation
  KVM: x86: Restore all 64 bits of DR6 and DR7 during RSM on x86-64

 arch/x86/kvm/emulate.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.30.0.365.g02bc693789-goog

[PATCH 1/2] KVM: x86: Remove misleading DR6/DR7 adjustments from RSM emulation

[Sean Christopherson]

Drop the DR6/7 volatile+fixed bits adjustments in RSM emulation, which
are redundant and misleading.  The necessary adjustments are made by
kvm_set_dr(), which properly sets the fixed bits that are conditional
on the vCPU model.

Note, KVM incorrectly reads only bits 31:0 of the DR6/7 fields when
emulating RSM on x86-64.  On the plus side for this change, that bug
makes removing "& DRx_VOLATILE" a nop.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 9641cff06722..2e6e6c39922f 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2506,12 +2506,12 @@ static int rsm_load_state_32(struct x86_emulate_ctxt *ctxt,
 
 	val = GET_SMSTATE(u32, smstate, 0x7fcc);
 
-	if (ctxt->ops->set_dr(ctxt, 6, (val & DR6_VOLATILE) | DR6_FIXED_1))
+	if (ctxt->ops->set_dr(ctxt, 6, val))
 		return X86EMUL_UNHANDLEABLE;
 
 	val = GET_SMSTATE(u32, smstate, 0x7fc8);
 
-	if (ctxt->ops->set_dr(ctxt, 7, (val & DR7_VOLATILE) | DR7_FIXED_1))
+	if (ctxt->ops->set_dr(ctxt, 7, val))
 		return X86EMUL_UNHANDLEABLE;
 
 	selector =                 GET_SMSTATE(u32, smstate, 0x7fc4);
@@ -2566,12 +2566,12 @@ static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
 
 	val = GET_SMSTATE(u32, smstate, 0x7f68);
 
-	if (ctxt->ops->set_dr(ctxt, 6, (val & DR6_VOLATILE) | DR6_FIXED_1))
+	if (ctxt->ops->set_dr(ctxt, 6, val))
 		return X86EMUL_UNHANDLEABLE;
 
 	val = GET_SMSTATE(u32, smstate, 0x7f60);
 
-	if (ctxt->ops->set_dr(ctxt, 7, (val & DR7_VOLATILE) | DR7_FIXED_1))
+	if (ctxt->ops->set_dr(ctxt, 7, val))
 		return X86EMUL_UNHANDLEABLE;
 
 	cr0 =                       GET_SMSTATE(u64, smstate, 0x7f58);
-- 
2.30.0.365.g02bc693789-goog

[PATCH 2/2] KVM: x86: Restore all 64 bits of DR6 and DR7 during RSM on x86-64

[Sean Christopherson]

Restore the full 64-bit values of DR6 and DR7 when emulating RSM on
x86-64, as defined by both Intel's SDM and AMD's APM.

Note, bits 63:32 of DR6 and DR7 are reserved, so this is a glorified nop
unless the SMM handler is poking into SMRAM, which it most definitely
shouldn't be doing since both Intel and AMD list the DR6 and DR7 fields
as read-only.

Fixes: 660a5d517aaa ("KVM: x86: save/load state on SMM switch")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 2e6e6c39922f..72a1bd04dfe1 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2564,12 +2564,12 @@ static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
 	ctxt->_eip   = GET_SMSTATE(u64, smstate, 0x7f78);
 	ctxt->eflags = GET_SMSTATE(u32, smstate, 0x7f70) | X86_EFLAGS_FIXED;
 
-	val = GET_SMSTATE(u32, smstate, 0x7f68);
+	val = GET_SMSTATE(u64, smstate, 0x7f68);
 
 	if (ctxt->ops->set_dr(ctxt, 6, val))
 		return X86EMUL_UNHANDLEABLE;
 
-	val = GET_SMSTATE(u32, smstate, 0x7f60);
+	val = GET_SMSTATE(u64, smstate, 0x7f60);
 
 	if (ctxt->ops->set_dr(ctxt, 7, val))
 		return X86EMUL_UNHANDLEABLE;
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH 2/2] KVM: x86: Restore all 64 bits of DR6 and DR7 during RSM on x86-64

[Paolo Bonzini]

On 05/02/21 02:24, Sean Christopherson wrote:
> Restore the full 64-bit values of DR6 and DR7 when emulating RSM on
> x86-64, as defined by both Intel's SDM and AMD's APM.
> 
> Note, bits 63:32 of DR6 and DR7 are reserved, so this is a glorified nop
> unless the SMM handler is poking into SMRAM, which it most definitely
> shouldn't be doing since both Intel and AMD list the DR6 and DR7 fields
> as read-only.
> 
> Fixes: 660a5d517aaa ("KVM: x86: save/load state on SMM switch")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/emulate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 2e6e6c39922f..72a1bd04dfe1 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2564,12 +2564,12 @@ static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
>   	ctxt->_eip   = GET_SMSTATE(u64, smstate, 0x7f78);
>   	ctxt->eflags = GET_SMSTATE(u32, smstate, 0x7f70) | X86_EFLAGS_FIXED;
>   
> -	val = GET_SMSTATE(u32, smstate, 0x7f68);
> +	val = GET_SMSTATE(u64, smstate, 0x7f68);
>   
>   	if (ctxt->ops->set_dr(ctxt, 6, val))
>   		return X86EMUL_UNHANDLEABLE;
>   
> -	val = GET_SMSTATE(u32, smstate, 0x7f60);
> +	val = GET_SMSTATE(u64, smstate, 0x7f60);
>   
>   	if (ctxt->ops->set_dr(ctxt, 7, val))
>   		return X86EMUL_UNHANDLEABLE;
> 

Queued, thanks.

Paolo