Re: [patch 0/2] Handle multiple exceptions (fixes Win2003 reboot by triple fault)

[Gleb Natapov <gleb@redhat.com>]

On Thu, Nov 12, 2009 at 02:07:09PM -0200, Marcelo Tosatti wrote:
> On Thu, Nov 12, 2009 at 02:21:24PM +0200, Gleb Natapov wrote:
> > On Wed, Nov 11, 2009 at 05:29:47PM -0200, Marcelo Tosatti wrote:
> > > I suppose a complete fix would be to follow the "Conditions for
> > > Generating a Double Fault" with support for handling exceptions
> > > serially.
> > > 
> > > But this works for me.
> > > 
> > I prefer proper solution. Like one attached (this is combination of ths
> > patch by Eddie Dong and my fix):
> > 
> > Move Double-Fault generation logic out of page fault
> > exception generating function to cover more generic case.
> > 
> > Signed-off-by: Eddie Dong <eddie.dong@intel.com>
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> 
> Nice.
> 
> Tested-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 76c8375..88c4490 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -248,12 +248,61 @@ void kvm_set_apic_base(struct kvm_vcpu *vcpu, u64 data)
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_set_apic_base);
> >  
> > +#define EXCPT_BENIGN		0
> > +#define EXCPT_CONTRIBUTORY	1
> > +#define EXCPT_PF		2
> > +
> > +static int exception_class(int vector)
> > +{
> > +	if (vector == 14)
> > +		return EXCPT_PF;
> > +	else if (vector == 0 || (vector >= 10 && vector <= 13))
> > +		return EXCPT_CONTRIBUTORY;
> > +	else
> > +		return EXCPT_BENIGN;
> > +}
> > +
> > +static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
> > +		unsigned nr, bool has_error, u32 error_code)
> > +{
> > +	u32 prev_nr;
> > +	int class1, class2;
> > +
> > +	if (!vcpu->arch.exception.pending) {
> > +	queue:
> > +		vcpu->arch.exception.pending = true;
> > +		vcpu->arch.exception.has_error_code = has_error;
> > +		vcpu->arch.exception.nr = nr;
> > +		vcpu->arch.exception.error_code = error_code;
> > +		return;
> > +	}
> > +
> > +	/* to check exception */
> > +	prev_nr = vcpu->arch.exception.nr;
> > +	if (prev_nr == DF_VECTOR) {
> > +		/* triple fault -> shutdown */
> > +		set_bit(KVM_REQ_TRIPLE_FAULT, &vcpu->requests);
> > +		return;
> > +	}
> > +	class1 = exception_class(prev_nr);
> > +	class2 = exception_class(nr);
> > +	if ((class1 == EXCPT_CONTRIBUTORY && class2 == EXCPT_CONTRIBUTORY)
> > +		|| (class1 == EXCPT_PF && class2 != EXCPT_BENIGN)) {
> > +		/* generate double fault per SDM Table 5-5 */
> > +		vcpu->arch.exception.pending = true;
> > +		vcpu->arch.exception.has_error_code = true;
> > +		vcpu->arch.exception.nr = DF_VECTOR;
> > +		vcpu->arch.exception.error_code = 0;
> 
> > +	} else
> > +		/* replace previous exception with a new one in a hope
> > +		   that instruction re-execution will regenerate lost
> > +		   exception */
> 
> Out of curiosity, why not an exception queue? 
> 
It seems unnecessary. Lost exception will be regenerated after
instruction that caused it will be re-executed.

> > +		goto queue;
> 
> This goto seems unnecessary.
It is very important. It replaces previous exception with the new one.
This allows CPU to proceed in situation where exception injection causes
another exception serially. Think about #DE that causes #PF (because
stack is not mapped for instance). If we'll drop #PF and re-inject #DE it
will generate #PF once again. If we'll drop #DE and inject #PF then #PF
handler with hopefully fix #PF condition and #DE will be regenerated
when devision will be retried.

--
			Gleb.