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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 4864 bytes --]

Avi Kivity wrote:
> On 10/03/2009 12:31 AM, Jan Kiszka wrote:
>> 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);
>>
>> +    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 <jan.kiszka@siemens.com>
---

 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(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 279a2ae..407e1a7 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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;
 }
 
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 70020e5..8e678ef 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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);
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index aa5d574..5b562dd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]