KVM: x86: Preserve guest single-stepping on register

PATCH In review Jan Kiszka 3 participants 5 messages Oct 2, 2009 – Oct 5, 2009 2 messages not in the archive

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jan Kiszka

author

2009-10-02 22:31 UTC #1 lore raw

PATCH 2/2 KVM: x86: Preserve guest single-stepping on register

Give user space more flexibility /wrt its IOCTL order. So far updating the rflags via KVM_SET_REGS ignored potentially set single-step flags. Now they will be kept.

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

---

 arch/x86/kvm/x86.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

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

index aa5d574..9fbb4c8 100644
@@ -3853,6 +3853,8 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
+	unsigned long rflags;
+
 	vcpu_load(vcpu);
 
 	kvm_register_write(vcpu, VCPU_REGS_RAX, regs->rax);

@@ -3876,8 +3878,11 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 #endif
 
 	kvm_rip_write(vcpu, regs->rip);
-	kvm_x86_ops->set_rflags(vcpu, regs->rflags);
 
+	rflags = regs->rflags;
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
+		rflags |= X86_EFLAGS_TF | X86_EFLAGS_RF;
+	kvm_x86_ops->set_rflags(vcpu, rflags);
 
 	vcpu->arch.exception.pending = false;

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jan Kiszka

author

2009-10-04 19:15 UTC #3 lore raw

Quoted 10 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka
Give user space more flexibility /wrt its IOCTL order. So far updating the rflags via KVM_SET_REGS ignored potentially set single-step flags. Now they will be kept.
      kvm_rip_write(vcpu, regs->rip);
- kvm_x86_ops->set_rflags(vcpu, regs->rflags);

Quoted 9 lines

Review
Quoted 6 lines
(quoted diff) +4 -0
+    rflags = regs->rflags;
+    if (vcpu->guest_debug&  KVM_GUESTDBG_SINGLESTEP)
+        rflags |= X86_EFLAGS_TF | X86_EFLAGS_RF;
+    kvm_x86_ops->set_rflags(vcpu, rflags);
I think we need same on popf instruction emulation.

Hmmmm, good point. Mind reverting 2/2 and applying this one instead?

Jan

--------->

KVM: x86: Rework guest single-step flag injection and filtering

Push TF and RF injection and filtering on guest single-stepping into the vender get/set_rflags callbacks. This makes the whole mechanism more robust /wrt user space IOTCTL order and instruction emulations.

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

---

 arch/x86/kvm/svm.c |    8 +++++++-
 arch/x86/kvm/vmx.c |    4 ++++
 arch/x86/kvm/x86.c |   24 +++++++++---------------
 3 files changed, 20 insertions(+), 16 deletions(-)

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

index 279a2ae..407e1a7 100644
@@ -797,11 +797,17 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
 
 static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
 {
-	return to_svm(vcpu)->vmcb->save.rflags;
+	unsigned long rflags = to_svm(vcpu)->vmcb->save.rflags;
+
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
+		rflags &= ~(unsigned long)(X86_EFLAGS_TF | X86_EFLAGS_RF);
+	return rflags;
 }
 
 static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 {
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
+		rflags |= X86_EFLAGS_TF | X86_EFLAGS_RF;
 	to_svm(vcpu)->vmcb->save.rflags = rflags;
 }

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

index 70020e5..8e678ef 100644
@@ -787,6 +787,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)
 	rflags = vmcs_readl(GUEST_RFLAGS);
 	if (to_vmx(vcpu)->rmode.vm86_active)
 		rflags &= ~(unsigned long)(X86_EFLAGS_IOPL | X86_EFLAGS_VM);
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
+		rflags &= ~(unsigned long)(X86_EFLAGS_TF | X86_EFLAGS_RF);
 	return rflags;
 }

@@ -794,6 +796,8 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 {
 	if (to_vmx(vcpu)->rmode.vm86_active)
 		rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
+		rflags |= X86_EFLAGS_TF | X86_EFLAGS_RF;
 	vmcs_writel(GUEST_RFLAGS, rflags);
 }

arch/x86/kvm/x86.c +9 ~~-15~~

index aa5d574..5b562dd 100644
@@ -3840,12 +3840,6 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	regs->rip = kvm_rip_read(vcpu);
 	regs->rflags = kvm_x86_ops->get_rflags(vcpu);
 
-	/*
-	 * Don't leak debug flags in case they were set for guest debugging
-	 */
-	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
-		regs->rflags &= ~(X86_EFLAGS_TF | X86_EFLAGS_RF);
-
 	vcpu_put(vcpu);
 
 	return 0;

@@ -3872,13 +3866,11 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	kvm_register_write(vcpu, VCPU_REGS_R13, regs->r13);
 	kvm_register_write(vcpu, VCPU_REGS_R14, regs->r14);
 	kvm_register_write(vcpu, VCPU_REGS_R15, regs->r15);
-
 #endif
 
 	kvm_rip_write(vcpu, regs->rip);
 	kvm_x86_ops->set_rflags(vcpu, regs->rflags);
 
-
 	vcpu->arch.exception.pending = false;
 
 	vcpu_put(vcpu);

@@ -4471,12 +4463,15 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 					struct kvm_guest_debug *dbg)
 {
 	unsigned long rflags;
-	int old_debug;
 	int i;
 
 	vcpu_load(vcpu);
 
-	old_debug = vcpu->guest_debug;
+	/*
+	 * Read rflags as long as potentially injected trace flags are still
+	 * filtered out.
+	 */
+	rflags = kvm_x86_ops->get_rflags(vcpu);
 
 	vcpu->guest_debug = dbg->control;
 	if (!(vcpu->guest_debug & KVM_GUESTDBG_ENABLE))

@@ -4493,11 +4488,10 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 		vcpu->arch.switch_db_regs = (vcpu->arch.dr7 & DR7_BP_EN_MASK);
 	}
 
-	rflags = kvm_x86_ops->get_rflags(vcpu);
-	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
-		rflags |= X86_EFLAGS_TF | X86_EFLAGS_RF;
-	else if (old_debug & KVM_GUESTDBG_SINGLESTEP)
-		rflags &= ~(X86_EFLAGS_TF | X86_EFLAGS_RF);
+	/*
+	 * Trigger an rflags update that will inject or remove the trace
+	 * flags.
+	 */
 	kvm_x86_ops->set_rflags(vcpu, rflags);
 
 	kvm_x86_ops->set_guest_debug(vcpu, dbg);

Avi Kivity

2009-10-05 10:44 UTC #4 lore raw

Quoted 34 lines · Jan Kiszka

Hmmmm, good point. Mind reverting 2/2 and applying this one instead?
Jan
--------->
KVM: x86: Rework guest single-step flag injection and filtering
Push TF and RF injection and filtering on guest single-stepping into the vender get/set_rflags callbacks. This makes the whole mechanism more robust /wrt user space IOTCTL order and instruction emulations.
Signed-off-by Jan Kiszka<[email protected]>
---
  arch/x86/kvm/svm.c |    8 +++++++-
  arch/x86/kvm/vmx.c |    4 ++++
  arch/x86/kvm/x86.c |   24 +++++++++---------------
  3 files changed, 20 insertions(+), 16 deletions(-)
arch/x86/kvm/svm.c +4 -1
index 279a2ae..407e1a7 100644
@@ -797,11 +797,17 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
 
  static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
  {
-	return to_svm(vcpu)->vmcb->save.rflags;
+	unsigned long rflags = to_svm(vcpu)->vmcb->save.rflags;
+
+	if (vcpu->guest_debug&  KVM_GUESTDBG_SINGLESTEP)
+		rflags&= ~(unsigned long)(X86_EFLAGS_TF | X86_EFLAGS_RF);

arch/x86/kvm/svm.c static void svm_vcpu_put(struct kvm_vcpu *vcpu) · line 813

Quoted 10 lines

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

+	return rflags;
  }
 
  static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
  {
+	if (vcpu->guest_debug&  KVM_GUESTDBG_SINGLESTEP)
+		rflags |= X86_EFLAGS_TF | X86_EFLAGS_RF;
  	to_svm(vcpu)->vmcb->save.rflags = rflags;
  }

This code is duplicated in vmx. How about kvm_[gs]et_rflags to contain it?

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2009-10-05 11:08 UTC #5 lore raw

PATCH v2 2/2 KVM: x86: Rework guest single-step flag injection and filtering

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

---

 arch/x86/include/asm/kvm_host.h |    3 ++
 arch/x86/kvm/x86.c              |   77 +++++++++++++++++++++++----------------
 2 files changed, 48 insertions(+), 32 deletions(-)

arch/x86/include/asm/kvm_host.h +3 -0

index e7f8708..179a919 100644
@@ -614,6 +614,9 @@ void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l);
 int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata);
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data);
 
+unsigned long kvm_get_rflags(struct kvm_vcpu *vcpu);
+void kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
+
 void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr);
 void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code);
 void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long cr2,

arch/x86/kvm/x86.c +45 ~~-32~~

186 lines not on this page · raw message

Avi Kivity

Applied

2009-10-05 12:49 UTC #6 lore raw

Re: [PATCH v2 2/2] KVM: x86: Rework guest single-step flag injection and filtering

Quoted 4 lines · Jan Kiszka

Push TF and RF injection and filtering on guest single-stepping into the vender get/set_rflags callbacks. This makes the whole mechanism more robust /wrt user space IOTCTL order and instruction emulations.

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function