Skip to contentSkip to conversation

KVM: SVM: Fix reading of DR6

PATCH In review Jan Kiszka 4 participants 6 messages Dec 19, 2013 – Jan 2, 2014

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 KVM: SVM: Fix reading of DR6

In contrast to VMX, SVM dose not automatically transfer DR6 into the VCPU's arch.dr6. So if we face a DR6 read, we must consult a new vendor hook to obtain the current value.

Fixes a regression of 020df0794f.

Signed-off-by Jan Kiszka <[email protected]>

---

Should go to stable as well.

 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/svm.c              | 9 +++++++++
 arch/x86/kvm/vmx.c              | 6 ++++++
 arch/x86/kvm/x86.c              | 2 +-
 4 files changed, 17 insertions(+), 1 deletion(-)
arch/x86/include/asm/kvm_host.h +1 -0
index ae5d783..f115f46 100644
@@ -699,6 +699,7 @@ struct kvm_x86_ops {
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
+	u64 (*get_dr6)(struct kvm_vcpu *vcpu);
 	void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
arch/x86/kvm/svm.c +9 -0
index c7168a5..48fa63e 100644
@@ -1671,6 +1671,14 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
 	mark_dirty(svm->vmcb, VMCB_ASID);
 }
 
+static u64 svm_get_dr6(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+		return vcpu->arch.dr6;
+	else
+		return to_svm(vcpu)->vmcb->save.dr6;
+}
+
 static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);

@@ -4286,6 +4294,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_idt = svm_set_idt,
 	.get_gdt = svm_get_gdt,
 	.set_gdt = svm_set_gdt,
+	.get_dr6 = svm_get_dr6,
 	.set_dr7 = svm_set_dr7,
 	.cache_reg = svm_cache_reg,
 	.get_rflags = svm_get_rflags,
arch/x86/kvm/vmx.c +6 -0
index ee3bf54..c5c7e62 100644
@@ -5153,6 +5153,11 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.dr6;
+}
+
 static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	vmcs_writel(GUEST_DR7, val);

@@ -8573,6 +8578,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.set_idt = vmx_set_idt,
 	.get_gdt = vmx_get_gdt,
 	.set_gdt = vmx_set_gdt,
+	.get_dr6 = vmx_get_dr6,
 	.set_dr7 = vmx_set_dr7,
 	.cache_reg = vmx_cache_reg,
 	.get_rflags = vmx_get_rflags,
arch/x86/kvm/x86.c +1 -1
index 1dc0359..8fe227c 100644
@@ -788,7 +788,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
 			return 1;
 		/* fall through */
 	case 6:
-		*val = vcpu->arch.dr6;
+		*val = kvm_x86_ops->get_dr6(vcpu);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
Signature · 1 line
1.8.1.1.298.ge7eed54
#1 lore raw
Quoted 95 lines · Jan Kiszka

In contrast to VMX, SVM dose not automatically transfer DR6 into the VCPU's arch.dr6. So if we face a DR6 read, we must consult a new vendor hook to obtain the current value.

Fixes a regression of 020df0794f.

Signed-off-by Jan Kiszka <[email protected]>

---

Should go to stable as well.

 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/svm.c              | 9 +++++++++
 arch/x86/kvm/vmx.c              | 6 ++++++
 arch/x86/kvm/x86.c              | 2 +-
 4 files changed, 17 insertions(+), 1 deletion(-)
arch/x86/include/asm/kvm_host.h +1 -0
index ae5d783..f115f46 100644
@@ -699,6 +699,7 @@ struct kvm_x86_ops {
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
+	u64 (*get_dr6)(struct kvm_vcpu *vcpu);
 	void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
arch/x86/kvm/svm.c +9 -0
index c7168a5..48fa63e 100644
@@ -1671,6 +1671,14 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
 	mark_dirty(svm->vmcb, VMCB_ASID);
 }
 
+static u64 svm_get_dr6(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+		return vcpu->arch.dr6;
+	else
+		return to_svm(vcpu)->vmcb->save.dr6;
+}
+
 static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);

@@ -4286,6 +4294,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_idt = svm_set_idt,
 	.get_gdt = svm_get_gdt,
 	.set_gdt = svm_set_gdt,
+	.get_dr6 = svm_get_dr6,
 	.set_dr7 = svm_set_dr7,
 	.cache_reg = svm_cache_reg,
 	.get_rflags = svm_get_rflags,
arch/x86/kvm/vmx.c +6 -0
index ee3bf54..c5c7e62 100644
@@ -5153,6 +5153,11 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.dr6;
+}
+
 static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	vmcs_writel(GUEST_DR7, val);

@@ -8573,6 +8578,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.set_idt = vmx_set_idt,
 	.get_gdt = vmx_get_gdt,
 	.set_gdt = vmx_set_gdt,
+	.get_dr6 = vmx_get_dr6,
 	.set_dr7 = vmx_set_dr7,
 	.cache_reg = vmx_cache_reg,
 	.get_rflags = vmx_get_rflags,
arch/x86/kvm/x86.c +1 -1
index 1dc0359..8fe227c 100644
@@ -788,7 +788,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
 			return 1;
 		/* fall through */
 	case 6:
-		*val = vcpu->arch.dr6;
+		*val = kvm_x86_ops->get_dr6(vcpu);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
Signature · 1 line
1.8.1.1.298.ge7eed54

It allows

kvm_set_dr(a)
val = kvm_get_dr()

to have 'val' different than a.

Is this OK ? (its certainly counter intuitive).

#2 lore raw

PATCH v2 1/1 KVM: SVM: Fix reading of DR6

Quoted 99 lines · Marcelo Tosatti
Quoted 95 lines · Jan Kiszka

In contrast to VMX, SVM dose not automatically transfer DR6 into the VCPU's arch.dr6. So if we face a DR6 read, we must consult a new vendor hook to obtain the current value.

Fixes a regression of 020df0794f.

Signed-off-by Jan Kiszka <[email protected]>

---

Should go to stable as well.

 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/svm.c              | 9 +++++++++
 arch/x86/kvm/vmx.c              | 6 ++++++
 arch/x86/kvm/x86.c              | 2 +-
 4 files changed, 17 insertions(+), 1 deletion(-)
arch/x86/include/asm/kvm_host.h +1 -0
index ae5d783..f115f46 100644
@@ -699,6 +699,7 @@ struct kvm_x86_ops {
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
+	u64 (*get_dr6)(struct kvm_vcpu *vcpu);
 	void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
arch/x86/kvm/svm.c +9 -0
index c7168a5..48fa63e 100644
@@ -1671,6 +1671,14 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
 	mark_dirty(svm->vmcb, VMCB_ASID);
 }
 
+static u64 svm_get_dr6(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+		return vcpu->arch.dr6;
+	else
+		return to_svm(vcpu)->vmcb->save.dr6;
+}
+
 static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);

@@ -4286,6 +4294,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_idt = svm_set_idt,
 	.get_gdt = svm_get_gdt,
 	.set_gdt = svm_set_gdt,
+	.get_dr6 = svm_get_dr6,
 	.set_dr7 = svm_set_dr7,
 	.cache_reg = svm_cache_reg,
 	.get_rflags = svm_get_rflags,
arch/x86/kvm/vmx.c +6 -0
index ee3bf54..c5c7e62 100644
@@ -5153,6 +5153,11 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.dr6;
+}
+
 static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	vmcs_writel(GUEST_DR7, val);

@@ -8573,6 +8578,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.set_idt = vmx_set_idt,
 	.get_gdt = vmx_get_gdt,
 	.set_gdt = vmx_set_gdt,
+	.get_dr6 = vmx_get_dr6,
 	.set_dr7 = vmx_set_dr7,
 	.cache_reg = vmx_cache_reg,
 	.get_rflags = vmx_get_rflags,
arch/x86/kvm/x86.c +1 -1
index 1dc0359..8fe227c 100644
@@ -788,7 +788,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
 			return 1;
 		/* fall through */
 	case 6:
-		*val = vcpu->arch.dr6;
+		*val = kvm_x86_ops->get_dr6(vcpu);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
Signature · 1 line
1.8.1.1.298.ge7eed54

It allows

Quoted 6 lines

kvm_set_dr(a)
val = kvm_get_dr()

to have 'val' different than a.

Is this OK ? (its certainly counter intuitive).

No, it's not ok. We also need to sync the guest-visible state to the VMCB on updates.

----8<----

From: Jan Kiszka <[email protected]>

In contrast to VMX, SVM dose not automatically transfer DR6 into the VCPU's arch.dr6. So if we face a DR6 read, we must consult a new vendor hook to obtain the current value. And as SVM now picks the DR6 state from its VMCB, we also need a set callback in order to write updates of DR6 back.

Fixes a regression of 020df0794f.

Signed-off-by Jan Kiszka <[email protected]>
---
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/svm.c              | 18 ++++++++++++++++++
 arch/x86/kvm/vmx.c              | 11 +++++++++++
 arch/x86/kvm/x86.c              |  3 ++-
 4 files changed, 33 insertions(+), 1 deletion(-)
arch/x86/include/asm/kvm_host.h +2 -0
index ae5d783..e73651b 100644
@@ -699,6 +699,8 @@ struct kvm_x86_ops {
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
+	u64 (*get_dr6)(struct kvm_vcpu *vcpu);
+	void (*set_dr6)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
arch/x86/kvm/svm.c +18 -0
index c7168a5..5987414 100644
@@ -1671,6 +1671,22 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
 	mark_dirty(svm->vmcb, VMCB_ASID);
 }
 
+static u64 svm_get_dr6(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+		return vcpu->arch.dr6;
+	else
+		return to_svm(vcpu)->vmcb->save.dr6;
+}
+
+static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	svm->vmcb->save.dr6 = value;
+	mark_dirty(svm->vmcb, VMCB_DR);
+}
+
 static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);

@@ -4286,6 +4302,8 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_idt = svm_set_idt,
 	.get_gdt = svm_get_gdt,
 	.set_gdt = svm_set_gdt,
+	.get_dr6 = svm_get_dr6,
+	.set_dr6 = svm_set_dr6,
 	.set_dr7 = svm_set_dr7,
 	.cache_reg = svm_cache_reg,
 	.get_rflags = svm_get_rflags,
arch/x86/kvm/vmx.c +11 -0
index ee3bf54..1d9b0ec 100644
@@ -5153,6 +5153,15 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.dr6;
+}
+
+static void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val)
+{
+}
+
 static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	vmcs_writel(GUEST_DR7, val);

@@ -8573,6 +8582,8 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.set_idt = vmx_set_idt,
 	.get_gdt = vmx_get_gdt,
 	.set_gdt = vmx_set_gdt,
+	.get_dr6 = vmx_get_dr6,
+	.set_dr6 = vmx_set_dr6,
 	.set_dr7 = vmx_set_dr7,
 	.cache_reg = vmx_cache_reg,
 	.get_rflags = vmx_get_rflags,
arch/x86/kvm/x86.c +2 -1
index 1dc0359..be000ec 100644
@@ -747,6 +747,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
 		if (val & 0xffffffff00000000ULL)
 			return -1; /* #GP */
 		vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
+		kvm_x86_ops->set_dr6(vcpu, vcpu->arch.dr6);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))

@@ -788,7 +789,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
 			return 1;
 		/* fall through */
 	case 6:
-		*val = vcpu->arch.dr6;
+		*val = kvm_x86_ops->get_dr6(vcpu);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
Signature · 1 line
1.8.1.1.298.ge7eed54
maintainer
#3 lore raw

Il 27/12/2013 19:17, Jan Kiszka ha scritto:

Quoted 44 lines

From: Jan Kiszka <[email protected]>

In contrast to VMX, SVM dose not automatically transfer DR6 into the VCPU's arch.dr6. So if we face a DR6 read, we must consult a new vendor hook to obtain the current value. And as SVM now picks the DR6 state from its VMCB, we also need a set callback in order to write updates of DR6 back.

Fixes a regression of 020df0794f.

Signed-off-by Jan Kiszka <[email protected]>
---
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/svm.c              | 18 ++++++++++++++++++
 arch/x86/kvm/vmx.c              | 11 +++++++++++
 arch/x86/kvm/x86.c              |  3 ++-
 4 files changed, 33 insertions(+), 1 deletion(-)
arch/x86/include/asm/kvm_host.h +2 -0
index ae5d783..e73651b 100644
@@ -699,6 +699,8 @@ struct kvm_x86_ops {
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
+	u64 (*get_dr6)(struct kvm_vcpu *vcpu);
+	void (*set_dr6)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
arch/x86/kvm/svm.c +4 -0
index c7168a5..5987414 100644
@@ -1671,6 +1671,22 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
 	mark_dirty(svm->vmcb, VMCB_ASID);
 }
 
+static u64 svm_get_dr6(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+		return vcpu->arch.dr6;

arch/x86/kvm/svm.c static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd) · line 1687

Quoted 10 lines
arch/x86/kvm/svm.c +10 -0
+	else
+		return to_svm(vcpu)->vmcb->save.dr6;
+}
+
+static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	svm->vmcb->save.dr6 = value;
+	mark_dirty(svm->vmcb, VMCB_DR);

The code here is a bit different from the existing DR7 hooks. Why isn't get_dr7 needed? I cannot find anything in the AMD manuals that suggests a difference between DR6 and DR7.

Also, set_dr7 is only called when !(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP). I think it makes sense to do the same for all the new hooks you need to introduce (get_dr6, set_dr6 and possibly get_dr7).

And finally, is it necessary to write to both vcpu->arch.dr6/7 and svm->vmcb->save.dr6/7, or is it simply okay to do that?

Paolo

Quoted 64 lines
arch/x86/kvm/svm.c +4 -0
+}
+
 static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);

@@ -4286,6 +4302,8 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_idt = svm_set_idt,
 	.get_gdt = svm_get_gdt,
 	.set_gdt = svm_set_gdt,
+	.get_dr6 = svm_get_dr6,
+	.set_dr6 = svm_set_dr6,
 	.set_dr7 = svm_set_dr7,
 	.cache_reg = svm_cache_reg,
 	.get_rflags = svm_get_rflags,
arch/x86/kvm/vmx.c +11 -0
index ee3bf54..1d9b0ec 100644
@@ -5153,6 +5153,15 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.dr6;
+}
+
+static void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val)
+{
+}
+
 static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	vmcs_writel(GUEST_DR7, val);

@@ -8573,6 +8582,8 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.set_idt = vmx_set_idt,
 	.get_gdt = vmx_get_gdt,
 	.set_gdt = vmx_set_gdt,
+	.get_dr6 = vmx_get_dr6,
+	.set_dr6 = vmx_set_dr6,
 	.set_dr7 = vmx_set_dr7,
 	.cache_reg = vmx_cache_reg,
 	.get_rflags = vmx_get_rflags,
arch/x86/kvm/x86.c +2 -1
index 1dc0359..be000ec 100644
@@ -747,6 +747,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
 		if (val & 0xffffffff00000000ULL)
 			return -1; /* #GP */
 		vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
+		kvm_x86_ops->set_dr6(vcpu, vcpu->arch.dr6);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))

@@ -788,7 +789,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
 			return 1;
 		/* fall through */
 	case 6:
-		*val = vcpu->arch.dr6;
+		*val = kvm_x86_ops->get_dr6(vcpu);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
#4 lore raw
Quoted 51 lines · Paolo Bonzini

Il 27/12/2013 19:17, Jan Kiszka ha scritto:

Quoted 50 lines

From: Jan Kiszka <[email protected]>

In contrast to VMX, SVM dose not automatically transfer DR6 into the VCPU's arch.dr6. So if we face a DR6 read, we must consult a new vendor hook to obtain the current value. And as SVM now picks the DR6 state from its VMCB, we also need a set callback in order to write updates of DR6 back.

Fixes a regression of 020df0794f.

Signed-off-by Jan Kiszka <[email protected]>
---
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/svm.c              | 18 ++++++++++++++++++
 arch/x86/kvm/vmx.c              | 11 +++++++++++
 arch/x86/kvm/x86.c              |  3 ++-
 4 files changed, 33 insertions(+), 1 deletion(-)
arch/x86/include/asm/kvm_host.h +2 -0
index ae5d783..e73651b 100644
@@ -699,6 +699,8 @@ struct kvm_x86_ops {
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
+	u64 (*get_dr6)(struct kvm_vcpu *vcpu);
+	void (*set_dr6)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
arch/x86/kvm/svm.c +10 -0
index c7168a5..5987414 100644
@@ -1671,6 +1671,22 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
 	mark_dirty(svm->vmcb, VMCB_ASID);
 }
 
+static u64 svm_get_dr6(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+		return vcpu->arch.dr6;
+	else
+		return to_svm(vcpu)->vmcb->save.dr6;
+}
+
+static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value)
+{
Quoted 8 lines

arch/x86/kvm/svm.c static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd) · line 1687

Quoted 4 lines
arch/x86/kvm/svm.c +4 -0
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	svm->vmcb->save.dr6 = value;
+	mark_dirty(svm->vmcb, VMCB_DR);

The code here is a bit different from the existing DR7 hooks. Why isn't get_dr7 needed? I cannot find anything in the AMD manuals that suggests a difference between DR6 and DR7.

DR7 doesn't change while the guest is running, only when the guest writes to it, and we intercept that.

Quoted 4 lines

Also, set_dr7 is only called when !(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP). I think it makes sense to do the same for all the new hooks you need to introduce (get_dr6, set_dr6 and possibly get_dr7).

We could make set_dr6 conditional, but it doesn't make a difference in practice. If guest debugging is enabled, we effectively only use DR6 when leaving the guest, i.e. when the hardware defined DR6.

Hmm, SVM is not updating kvm_run::debug.arch.dr6/7... There might be more broken, need to check. But that would not be guest visible then.

Quoted 3 lines

And finally, is it necessary to write to both vcpu->arch.dr6/7 and svm->vmcb->save.dr6/7, or is it simply okay to do that?

arch.dr6/7 holds the guest visible state, save.dr6/7 the effective one.

Jan

PS: I'll look into a kvm unit test for hardware debugging features these days. Guest-side at least, no idea yet how to implement host-side debugging with unit tests.

maintainer
#5 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 31/12/2013 18:29, Jan Kiszka ha scritto:

Quoted 7 lines
Quoted 4 lines

Also, set_dr7 is only called when !(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP). I think it makes sense to do the same for all the new hooks you need to introduce (get_dr6, set_dr6 and possibly get_dr7).

We could make set_dr6 conditional, but it doesn't make a difference in practice.

No, it doesn't indeed. It's just a matter of consistency.

Quoted 11 lines

If guest debugging is enabled, we effectively only use DR6 when leaving the guest, i.e. when the hardware defined DR6.

Hmm, SVM is not updating kvm_run::debug.arch.dr6/7... There might be more broken, need to check. But that would not be guest visible then.

Quoted 3 lines

And finally, is it necessary to write to both vcpu->arch.dr6/7 and svm->vmcb->save.dr6/7, or is it simply okay to do that?

Quoted 8 lines

arch.dr6/7 holds the guest visible state, save.dr6/7 the effective one.

Jan

PS: I'll look into a kvm unit test for hardware debugging features these days. Guest-side at least, no idea yet how to implement host-side debugging with unit tests.

It would be possible to do it with infrastructure like qtest or autotest. You could speak the gdbserver protocol yourself, or open a pty to gdb. But guest-side is already an improvement over nothing!

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSxSI0AAoJEBvWZb6bTYbyoiYP/REKgPL/dUwJoSA8AEm7h6Ip wkXlaxBcyBiKlIzfiN3abBXBOZvDy7dSzNfXUZ4bY6DitQIJZYSVc93XfvWYRToY ORC6qZej+P/bD/rNwATpvVhGG6RbRpmihmVgkoX5w5J3GWZ52QREcyoW7mZEhIoR fCMJmNCq2l9vxa5hhXLAuiLYtq7mto6hQpFE8TXG81PyzE+c3WWK1D5vy/2cpv48 qabaVsJs7aj0KV2rVDvyQkO+GSdm+8SDpf4GyJcLo1Y1yYjPW97xC0Ey4ZyEXXWf 37zb3oOoH9NPPXtt1oJhUFCFZ34v1MxW2j0IIGvtUfRPSjphW+T/nQ5y17GG+efQ TbKnSlibNP0L7fX/xa/sytaYcslIJVIKvvq0Wz9a3Ka1OHNufgtBIreUjtAPvSmy TLxTm2w2s0S/mD3yMDK62uAtQMV/FW7qV6WHTHHZzj8VJpgfAAO0j4gkN0PlLB91 XfuR8ANmYoAKm9yafOSlr6Xw4uzHh41ZODMIY9w/pwB6nGB8SMRVBUWAILkyiyoN btJdVCFdmxTlUmszAeHGTkd4VMtOMqva8MmgnV5isVEUy5A6gJZwNSZ8KJCdny+J N+jRLj/j3SUlxGFnsK4FPBlMQzK1DDYS3C2QKd+Z5No42kZg4w2g4n0zQamDi2XR 2lcuexvRePHTZglNkP82
=dDQc
-----END PGP SIGNATURE-----