KVM: x86: Fix guest debug across vcpu INIT reset

PATCH In review Jan Kiszka 3 participants 4 messages Sep 19, 2012 – Sep 23, 2012

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jan Kiszka

author

2012-09-19 19:38 UTC #0 lore raw

PATCH 1/1 KVM: x86: Fix guest debug across vcpu INIT reset

If we reset a vcpu on INIT, make sure to not touch dr7 as stored in the VMCS/VMCB and also switch_db_regs if guest debugging is using hardware breakpoints. Otherwise, the vcpu will not trigger hardware breakpoints until userspace issues another KVM_SET_GUEST_DEBUG IOCTL for it.

Found while trying to stop on start_secondary.

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

---

Nice to have for stable as well.

 arch/x86/kvm/svm.c |    3 ++-
 arch/x86/kvm/vmx.c |    3 ++-
 arch/x86/kvm/x86.c |    3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

arch/x86/kvm/svm.c +2 -1

index 611c728..4163ce2 100644
@@ -1146,7 +1146,8 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm_set_efer(&svm->vcpu, 0);
 	save->dr6 = 0xffff0ff0;
-	save->dr7 = 0x400;
+	if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_USE_HW_BP))
+		save->dr7 = 0x400;
 	kvm_set_rflags(&svm->vcpu, 2);
 	save->rip = 0x0000fff0;
 	svm->vcpu.arch.regs[VCPU_REGS_RIP] = save->rip;

arch/x86/kvm/vmx.c +2 -1

index d62b413..37f68f7 100644
@@ -3959,7 +3959,8 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		kvm_rip_write(vcpu, 0);
 	kvm_register_write(vcpu, VCPU_REGS_RSP, 0);
 
-	vmcs_writel(GUEST_DR7, 0x400);
+	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP))
+		vmcs_writel(GUEST_DR7, 0x400);
 
 	vmcs_writel(GUEST_GDTR_BASE, 0);
 	vmcs_write32(GUEST_GDTR_LIMIT, 0xffff);

arch/x86/kvm/x86.c +2 -1

index c4d451e..7c8c2b8 100644
@@ -6043,7 +6043,8 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
 	vcpu->arch.nmi_pending = 0;
 	vcpu->arch.nmi_injected = false;
 
-	vcpu->arch.switch_db_regs = 0;
+	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP))
+		vcpu->arch.switch_db_regs = 0;
 	memset(vcpu->arch.db, 0, sizeof(vcpu->arch.db));
 	vcpu->arch.dr6 = DR6_FIXED_1;
 	vcpu->arch.dr7 = DR7_FIXED_1;

Signature · 1 line

1.7.3.4

Avi Kivity

2012-09-20 11:39 UTC #1 lore raw

Quoted 8 lines · Jan Kiszka

If we reset a vcpu on INIT, make sure to not touch dr7 as stored in the VMCS/VMCB and also switch_db_regs if guest debugging is using hardware breakpoints. Otherwise, the vcpu will not trigger hardware breakpoints until userspace issues another KVM_SET_GUEST_DEBUG IOCTL for it.
Found while trying to stop on start_secondary.

arch/x86/kvm/svm.c static void init_vmcb(struct vcpu_svm *svm) · line 1150

Quoted 6 lines

arch/x86/kvm/svm.c +2 -1

@@ -1146,7 +1146,8 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm_set_efer(&svm->vcpu, 0);
 	save->dr6 = 0xffff0ff0;
-	save->dr7 = 0x400;
+	if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_USE_HW_BP))
+		save->dr7 = 0x400;

Whenever we multiplex a resource foo for several users, we should have a function update_foo() that recalculates foo from all its sources:

        vcpu->arch.dr7 = 0x400;
        update_dr7(&svm->vcpu);

(don't know if the first line really belongs here)

Quoted 8 lines

arch/x86/kvm/svm.c +0 -0

 	kvm_set_rflags(&svm->vcpu, 2);
 	save->rip = 0x0000fff0;
 	svm->vcpu.arch.regs[VCPU_REGS_RIP] = save->rip;

arch/x86/kvm/vmx.c +0 -0

index d62b413..37f68f7 100644

arch/x86/kvm/vmx.c static int vmx_vcpu_reset(struct kvm_vcpu *vcpu) · line 3964

Quoted 7 lines

arch/x86/kvm/vmx.c +2 -1

@@ -3959,7 +3959,8 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		kvm_rip_write(vcpu, 0);
 	kvm_register_write(vcpu, VCPU_REGS_RSP, 0);
 
-	vmcs_writel(GUEST_DR7, 0x400);
+	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP))
+		vmcs_writel(GUEST_DR7, 0x400);

Ditto. update_dr7() could even be generic and use ->set_dr7().

Quoted 8 lines

arch/x86/kvm/vmx.c +0 -0

 	vmcs_writel(GUEST_GDTR_BASE, 0);
 	vmcs_write32(GUEST_GDTR_LIMIT, 0xffff);

arch/x86/kvm/x86.c +0 -0

index c4d451e..7c8c2b8 100644

arch/x86/kvm/x86.c int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu) · line 6050

Quoted 9 lines

arch/x86/kvm/x86.c +2 -1

@@ -6043,7 +6043,8 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
 	vcpu->arch.nmi_pending = 0;
 	vcpu->arch.nmi_injected = false;
 
-	vcpu->arch.switch_db_regs = 0;
+	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP))
+		vcpu->arch.switch_db_regs = 0;
 	memset(vcpu->arch.db, 0, sizeof(vcpu->arch.db));
 	vcpu->arch.dr6 = DR6_FIXED_1;
 	vcpu->arch.dr7 = DR7_FIXED_1;

You could move the switch_db_regs calculation into update_dr7(), and move the update_dr7() call after assigning it here. This fixes everything neatly (and obviates the need for the first line in the snippet above).

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2012-09-21 03:43 UTC #2 lore raw

PATCH v2 1/1 KVM: x86: Fix guest debug across vcpu INIT reset

From: Jan Kiszka <[email protected]>

If we reset a vcpu on INIT, we so far overwrote dr7 as provided by KVM_SET_GUEST_DEBUG, and we also cleared switch_db_regs unconditionally.

Fix this by saving the dr7 used for guest debugging and calculating the effective register value as well as switch_db_regs on any potential change. This will change to focus of the set_guest_debug vendor op to update_dp_bp_intercept.

Found while trying to stop on start_secondary.

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

---
 arch/x86/include/asm/kvm_host.h |    4 ++--
 arch/x86/kvm/svm.c              |   23 ++++-------------------
 arch/x86/kvm/vmx.c              |   14 +-------------
 arch/x86/kvm/x86.c              |   26 +++++++++++++++++---------
 4 files changed, 24 insertions(+), 43 deletions(-)

arch/x86/include/asm/kvm_host.h +2 -2

index 64adb61..e8baa1c 100644
@@ -457,6 +457,7 @@ struct kvm_vcpu_arch {
 	unsigned long dr6;
 	unsigned long dr7;
 	unsigned long eff_db[KVM_NR_DB_REGS];
+	unsigned long guest_debug_dr7;
 
 	u64 mcg_cap;
 	u64 mcg_status;

@@ -621,8 +622,7 @@ struct kvm_x86_ops {
 	void (*vcpu_load)(struct kvm_vcpu *vcpu, int cpu);
 	void (*vcpu_put)(struct kvm_vcpu *vcpu);
 
-	void (*set_guest_debug)(struct kvm_vcpu *vcpu,
-				struct kvm_guest_debug *dbg);
+	void (*update_db_bp_intercept)(struct kvm_vcpu *vcpu);
 	int (*get_msr)(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata);
 	int (*set_msr)(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
 	u64 (*get_segment_base)(struct kvm_vcpu *vcpu, int seg);

arch/x86/kvm/svm.c +4 ~~-19~~

index 611c728..076064b 100644
@@ -1146,7 +1146,6 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm_set_efer(&svm->vcpu, 0);
 	save->dr6 = 0xffff0ff0;
-	save->dr7 = 0x400;
 	kvm_set_rflags(&svm->vcpu, 2);
 	save->rip = 0x0000fff0;
 	svm->vcpu.arch.regs[VCPU_REGS_RIP] = save->rip;

@@ -1643,7 +1642,7 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
 	mark_dirty(svm->vmcb, VMCB_SEG);
 }
 
-static void update_db_intercept(struct kvm_vcpu *vcpu)
+static void update_db_bp_intercept(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);

@@ -1663,20 +1662,6 @@ static void update_db_intercept(struct kvm_vcpu *vcpu)
 		vcpu->guest_debug = 0;
 }
 
-static void svm_guest_debug(struct kvm_vcpu *vcpu, struct kvm_guest_debug *dbg)
-{
-	struct vcpu_svm *svm = to_svm(vcpu);
-
-	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
-		svm->vmcb->save.dr7 = dbg->arch.debugreg[7];
-	else
-		svm->vmcb->save.dr7 = vcpu->arch.dr7;
-
-	mark_dirty(svm->vmcb, VMCB_DR);
-
-	update_db_intercept(vcpu);
-}
-
 static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
 {
 	if (sd->next_asid > sd->max_asid) {

@@ -1748,7 +1733,7 @@ static int db_interception(struct vcpu_svm *svm)
 		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP))
 			svm->vmcb->save.rflags &=
 				~(X86_EFLAGS_TF | X86_EFLAGS_RF);
-		update_db_intercept(&svm->vcpu);
+		update_db_bp_intercept(&svm->vcpu);
 	}
 
 	if (svm->vcpu.guest_debug &

@@ -3659,7 +3644,7 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
 	 */
 	svm->nmi_singlestep = true;
 	svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
-	update_db_intercept(vcpu);
+	update_db_bp_intercept(vcpu);
 }
 
 static int svm_set_tss_addr(struct kvm *kvm, unsigned int addr)

@@ -4259,7 +4244,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.vcpu_load = svm_vcpu_load,
 	.vcpu_put = svm_vcpu_put,
 
-	.set_guest_debug = svm_guest_debug,
+	.update_db_bp_intercept = update_db_bp_intercept,
 	.get_msr = svm_get_msr,
 	.set_msr = svm_set_msr,
 	.get_segment_base = svm_get_segment_base,

arch/x86/kvm/vmx.c +1 ~~-13~~

index d62b413..b0f7b34 100644
@@ -2286,16 +2286,6 @@ static void vmx_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
 	}
 }
 
-static void set_guest_debug(struct kvm_vcpu *vcpu, struct kvm_guest_debug *dbg)
-{
-	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
-		vmcs_writel(GUEST_DR7, dbg->arch.debugreg[7]);
-	else
-		vmcs_writel(GUEST_DR7, vcpu->arch.dr7);
-
-	update_exception_bitmap(vcpu);
-}
-
 static __init int cpu_has_kvm_support(void)
 {
 	return cpu_has_vmx();

@@ -3959,8 +3949,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		kvm_rip_write(vcpu, 0);
 	kvm_register_write(vcpu, VCPU_REGS_RSP, 0);
 
-	vmcs_writel(GUEST_DR7, 0x400);
-
 	vmcs_writel(GUEST_GDTR_BASE, 0);
 	vmcs_write32(GUEST_GDTR_LIMIT, 0xffff);

@@ -7241,7 +7229,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.vcpu_load = vmx_vcpu_load,
 	.vcpu_put = vmx_vcpu_put,
 
-	.set_guest_debug = set_guest_debug,
+	.update_db_bp_intercept = update_exception_bitmap,
 	.get_msr = vmx_get_msr,
 	.set_msr = vmx_set_msr,
 	.get_segment_base = vmx_get_segment_base,

arch/x86/kvm/x86.c +17 -9

index c4d451e..355d7a8 100644
@@ -692,6 +692,18 @@ unsigned long kvm_get_cr8(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_get_cr8);
 
+static void kvm_update_dr7(struct kvm_vcpu *vcpu)
+{
+	unsigned long dr7;
+
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+		dr7 = vcpu->arch.guest_debug_dr7;
+	else
+		dr7 = vcpu->arch.dr7;
+	kvm_x86_ops->set_dr7(vcpu, dr7);
+	vcpu->arch.switch_db_regs = (dr7 & DR7_BP_EN_MASK);
+}
+
 static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
 {
 	switch (dr) {

@@ -717,10 +729,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
 		if (val & 0xffffffff00000000ULL)
 			return -1; /* #GP */
 		vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1;
-		if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) {
-			kvm_x86_ops->set_dr7(vcpu, vcpu->arch.dr7);
-			vcpu->arch.switch_db_regs = (val & DR7_BP_EN_MASK);
-		}
+		kvm_update_dr7(vcpu);
 		break;
 	}

@@ -5853,13 +5862,12 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
 		for (i = 0; i < KVM_NR_DB_REGS; ++i)
 			vcpu->arch.eff_db[i] = dbg->arch.debugreg[i];
-		vcpu->arch.switch_db_regs =
-			(dbg->arch.debugreg[7] & DR7_BP_EN_MASK);
+		vcpu->arch.guest_debug_dr7 = dbg->arch.debugreg[7];
 	} else {
 		for (i = 0; i < KVM_NR_DB_REGS; i++)
 			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
-		vcpu->arch.switch_db_regs = (vcpu->arch.dr7 & DR7_BP_EN_MASK);
 	}
+	kvm_update_dr7(vcpu);
 
 	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
 		vcpu->arch.singlestep_rip = kvm_rip_read(vcpu) +

@@ -5871,7 +5879,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 	 */
 	kvm_set_rflags(vcpu, rflags);
 
-	kvm_x86_ops->set_guest_debug(vcpu, dbg);
+	kvm_x86_ops->update_db_bp_intercept(vcpu);
 
 	r = 0;

@@ -6043,10 +6051,10 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
 	vcpu->arch.nmi_pending = 0;
 	vcpu->arch.nmi_injected = false;
 
-	vcpu->arch.switch_db_regs = 0;
 	memset(vcpu->arch.db, 0, sizeof(vcpu->arch.db));
 	vcpu->arch.dr6 = DR6_FIXED_1;
 	vcpu->arch.dr7 = DR7_FIXED_1;
+	kvm_update_dr7(vcpu);
 
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	vcpu->arch.apf.msr_val = 0;

Signature · 1 line

1.7.3.4

Avi Kivity

Applied

2012-09-23 13:01 UTC #3 lore raw

Quoted 5 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
If we reset a vcpu on INIT, we so far overwrote dr7 as provided by KVM_SET_GUEST_DEBUG, and we also cleared switch_db_regs unconditionally.

Quoted 7 lines

Fix this by saving the dr7 used for guest debugging and calculating the effective register value as well as switch_db_regs on any potential change. This will change to focus of the set_guest_debug vendor op to update_dp_bp_intercept.
Found while trying to stop on start_secondary.

Thanks, applied.

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.