Re: [patch 1/2] KVM: x86: handle double and triple faults for every exception

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Nov 11, 2009 at 10:02:19PM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > On Wed, Nov 11, 2009 at 09:07:08PM +0100, Jan Kiszka wrote:
> >> Marcelo Tosatti wrote:
> >>> From: Joerg Roedel <joerg.roedel@amd.com>
> >>>
> >>> The current KVM x86 exception code handles double and triple faults only for
> >>> page fault exceptions. This patch extends this detection for every exception
> >>> that gets queued for the guest.
> >>>
> >>> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> >>> CC: Jan Kiszka <jan.kiszka@web.de>
> >> For a moment I felt like I was time traveling - back in '08. :)
> >>
> >> Reading the archive I noticed that someone posted a fix-up for this patch:
> >>
> >> http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/16931
> >>
> >> Why don't we need this anymore?
> > 
> > I suppose qemu-kvm's call to set_sregs (via system_reset) will end up
> > clearing pending exception?
> 
> Right, forgot for the moment that triple fault implies user space.
> 
> > 
> >> Moreover, are we sure to not regress /wrt to the cases that shall be
> >> handled serially? So far they should have triggered the WARN_ON, right?
> > 
> > Right. 
> > 
> > How can it regress though, given that serially handled exceptions are
> > not supported at the moment (you get a WARN_ON and lose the previously
> > queued anyway).
> 
> The guest so far sees the second exception as the result, now it sees
> DF. So the behavior changes from broken to broken, but I wondered if the
> current state is already so broken that this change doesn't matter.

I see your point. I suppose the WARN_ON is there to catch any code paths
that could trigger (unsupported) multiple exceptions, and apparently no
path does that now (other than pagefault which is handled separately) ?

> Another micro difference is this:
> 
> > @@ -184,24 +196,6 @@ void kvm_inject_page_fault(struct kvm_vc
> >  {
> >  	++vcpu->stat.pf_guest;
> >  
> > -	if (vcpu->arch.exception.pending) {
> > -		switch(vcpu->arch.exception.nr) {
> > -		case DF_VECTOR:
> > -			/* triple fault -> shutdown */
> > -			set_bit(KVM_REQ_TRIPLE_FAULT, &vcpu->requests);
> > -			return;
> > -		case PF_VECTOR:
> > -			vcpu->arch.exception.nr = DF_VECTOR;
> > -			vcpu->arch.exception.error_code = 0;
> > -			return;
> > -		default:
> > -			/* replace previous exception with a new one in a hope
> > -			   that instruction re-execution will regenerate lost
> > -			   exception */
> > -			vcpu->arch.exception.pending = false;
> > -			break;
> > -		}
> > -	}
> >  	vcpu->arch.cr2 = addr;
> >  	kvm_queue_exception_e(vcpu, PF_VECTOR, error_code);
> >  }
> 
> So far cr2 was not touched on DF, now it is.

Yep. The PF was overwritten with DF, which means the cr2 value will not
be interpreted by the guest?

> 
> Jan
>