* [PATCH] KVM: x86: Fix guest debug across vcpu INIT reset
@ 2012-09-19 19:38 Jan Kiszka
2012-09-20 11:39 ` Avi Kivity
0 siblings, 1 reply; 4+ messages in thread
From: Jan Kiszka @ 2012-09-19 19:38 UTC (permalink / raw)
To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, Roedel, Joerg
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 <jan.kiszka@siemens.com>
---
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(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 611c728..4163ce2 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index d62b413..37f68f7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c4d451e..7c8c2b8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;
--
1.7.3.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: x86: Fix guest debug across vcpu INIT reset
2012-09-19 19:38 [PATCH] KVM: x86: Fix guest debug across vcpu INIT reset Jan Kiszka
@ 2012-09-20 11:39 ` Avi Kivity
2012-09-21 3:42 ` [PATCH v2] " Jan Kiszka
0 siblings, 1 reply; 4+ messages in thread
From: Avi Kivity @ 2012-09-20 11:39 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Marcelo Tosatti, kvm, Roedel, Joerg
On 09/19/2012 10:38 PM, Jan Kiszka wrote:
> 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.
>
> @@ -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)
> kvm_set_rflags(&svm->vcpu, 2);
> save->rip = 0x0000fff0;
> svm->vcpu.arch.regs[VCPU_REGS_RIP] = save->rip;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index d62b413..37f68f7 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -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().
> vmcs_writel(GUEST_GDTR_BASE, 0);
> vmcs_write32(GUEST_GDTR_LIMIT, 0xffff);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c4d451e..7c8c2b8 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -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).
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] KVM: x86: Fix guest debug across vcpu INIT reset
2012-09-20 11:39 ` Avi Kivity
@ 2012-09-21 3:42 ` Jan Kiszka
2012-09-23 13:01 ` Avi Kivity
0 siblings, 1 reply; 4+ messages in thread
From: Jan Kiszka @ 2012-09-21 3:42 UTC (permalink / raw)
To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, Roedel, Joerg
From: Jan Kiszka <jan.kiszka@siemens.com>
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 <jan.kiszka@siemens.com>
---
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(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 64adb61..e8baa1c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 611c728..076064b 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index d62b413..b0f7b34 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c4d451e..355d7a8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;
--
1.7.3.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] KVM: x86: Fix guest debug across vcpu INIT reset
2012-09-21 3:42 ` [PATCH v2] " Jan Kiszka
@ 2012-09-23 13:01 ` Avi Kivity
0 siblings, 0 replies; 4+ messages in thread
From: Avi Kivity @ 2012-09-23 13:01 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Marcelo Tosatti, kvm, Roedel, Joerg
On 09/21/2012 06:42 AM, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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.
>
Thanks, applied.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-09-23 13:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-19 19:38 [PATCH] KVM: x86: Fix guest debug across vcpu INIT reset Jan Kiszka
2012-09-20 11:39 ` Avi Kivity
2012-09-21 3:42 ` [PATCH v2] " Jan Kiszka
2012-09-23 13:01 ` Avi Kivity
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).