Re: [PATCH 2/2] KVM: SVM: Make stepping out of NMI handlers more robust

[Gleb Natapov <gleb@redhat.com>]

On Wed, Feb 17, 2010 at 08:16:45PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Tue, Feb 16, 2010 at 12:08:58PM +0200, Gleb Natapov wrote:
> >>>>> Besides this, proper #DB forwarding to the guest was missing.
> >>>> During NMI injection? How to reproduce?
> >>> Inject, e.g., an NMI over code with TF set. A bit harder is placing a
> >>> guest HW breakpoint at the spot the NMI handler returns to.
> >>>
> >> Will try to reproduce.
> >>
> > How can I make gdb to run debugged process with TF set? Is this patch
> > fixes it:
> > 
> > 
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 52f78dd..b85b200 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -109,6 +109,7 @@ struct vcpu_svm {
> >  	struct nested_state nested;
> >  
> >  	bool nmi_singlestep;
> > +	bool nmi_singlestep_tf;
> >  };
> >  
> >  /* enable NPT for AMD64 and X86 with PAE */
> > @@ -1221,9 +1222,14 @@ static int db_interception(struct vcpu_svm *svm)
> >  
> >  	if (svm->nmi_singlestep) {
> >  		svm->nmi_singlestep = false;
> > -		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP))
> > +		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
> >  			svm->vmcb->save.rflags &=
> >  				~(X86_EFLAGS_TF | X86_EFLAGS_RF);
> > +			if (svm->nmi_singlestep_tf) {
> > +				svm->vmcb->save.rflags |= X86_EFLAGS_TF;
> > +				kvm_queue_exception(&svm->vcpu, DB_VECTOR);
> > +			}
> > +		}
> >  		update_db_intercept(&svm->vcpu);
> >  	}
> >  
> > @@ -2586,6 +2592,7 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
> >  	   possible problem (IRET or exception injection or interrupt
> >  	   shadow) */
> >  	svm->nmi_singlestep = true;
> > +	svm->nmi_singlestep_tf = (svm->vmcb->save.rflags | X86_EFLAGS_TF);
> >  	svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
> >  	update_db_intercept(vcpu);
> >  }
> 
> That's closer. However, I've a version here that restores TF&RF only if
> you did not execute an IRET but stepped over the shadow (which is still
> not correct either, e.g. when stepping popf). I will break up my patch
> into parts that fix the issues separately so that we can decide what to
> merge.
> 
I am not sure what do you mean here. Why should we restore RF? It is
cleared after each instruction execution and popf is not special in this
regards and SDM explicitly says so.

--
			Gleb.