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

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

[Marcelo Tosatti]

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.

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

[Marcelo Tosatti]

[-- Attachment #1: handle-multiple-exceptions --]
[-- Type: text/plain, Size: 2276 bytes --]

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>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -170,9 +170,21 @@ void kvm_set_apic_base(struct kvm_vcpu *
 }
 EXPORT_SYMBOL_GPL(kvm_set_apic_base);
 
+static void handle_multiple_faults(struct kvm_vcpu *vcpu)
+{
+       if (vcpu->arch.exception.nr != DF_VECTOR) {
+               vcpu->arch.exception.nr = DF_VECTOR;
+               vcpu->arch.exception.error_code = 0;
+       } else
+               set_bit(KVM_REQ_TRIPLE_FAULT, &vcpu->requests);
+}
+
 void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr)
 {
-	WARN_ON(vcpu->arch.exception.pending);
+	if (vcpu->arch.exception.pending) {
+		handle_multiple_faults(vcpu);
+		return;
+	}
 	vcpu->arch.exception.pending = true;
 	vcpu->arch.exception.has_error_code = false;
 	vcpu->arch.exception.nr = nr;
@@ -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);
 }
@@ -214,7 +208,10 @@ EXPORT_SYMBOL_GPL(kvm_inject_nmi);
 
 void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
 {
-	WARN_ON(vcpu->arch.exception.pending);
+	if (vcpu->arch.exception.pending) {
+		handle_multiple_faults(vcpu);
+		return;
+	}
 	vcpu->arch.exception.pending = true;
 	vcpu->arch.exception.has_error_code = true;
 	vcpu->arch.exception.nr = nr;

-- 

[patch 2/2] KVM: x86: raise TSS exception for NULL CS and SS segments

[Marcelo Tosatti]

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

Windows 2003 uses task switch to triple fault and reboot (the other
exception being reserved pdptrs bits).

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -4091,6 +4091,15 @@ static int is_vm86_segment(struct kvm_vc
 		(kvm_x86_ops->get_rflags(vcpu) & X86_EFLAGS_VM);
 }
 
+static void kvm_check_segment_descriptor(struct kvm_vcpu *vcpu, int seg,
+					 u16 selector)
+{
+	/* NULL selector is not valid for CS and SS */
+	if (seg == VCPU_SREG_CS || seg == VCPU_SREG_SS)
+		if (!selector)
+			kvm_queue_exception_e(vcpu, TS_VECTOR, selector >> 3);
+}
+
 int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
 				int type_bits, int seg)
 {
@@ -4100,6 +4109,8 @@ int kvm_load_segment_descriptor(struct k
 		return kvm_load_realmode_segment(vcpu, selector, seg);
 	if (load_segment_descriptor_to_kvm_desct(vcpu, selector, &kvm_seg))
 		return 1;
+
+	kvm_check_segment_descriptor(vcpu, seg, selector);
 	kvm_seg.type |= type_bits;
 
 	if (seg != VCPU_SREG_SS && seg != VCPU_SREG_CS &&

-- 

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

[Jan Kiszka]

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

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?

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?
But maybe that went through unnoticed, and the guest was simply happy to
not have triggered a triple fault...

Jan


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

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

[Marcelo Tosatti]

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?

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

> But maybe that went through unnoticed, and the guest was simply happy to
> not have triggered a triple fault...

Can't parse. What went through unnoticed, and when/which guest was happy 
to not trigger a triple fault?

BTW, from my understanding of the documentation, the triple fault       
should happen only on an exception during execution of the double fault 
handler, which is not what the pagefault injection code does (or Joerg's
patch and the TSS exceptions in the task switch code).

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

[Jan Kiszka]

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

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.

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.

Jan


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

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

[Marcelo Tosatti]

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
> 

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

[Gleb Natapov]

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>
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 */
+		goto queue;
+}
+
 void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr)
 {
-	WARN_ON(vcpu->arch.exception.pending);
-	vcpu->arch.exception.pending = true;
-	vcpu->arch.exception.has_error_code = false;
-	vcpu->arch.exception.nr = nr;
+	kvm_multiple_exception(vcpu, nr, false, 0);
 }
 EXPORT_SYMBOL_GPL(kvm_queue_exception);
 
@@ -261,25 +310,6 @@ void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long addr,
 			   u32 error_code)
 {
 	++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);
 }
@@ -292,11 +322,7 @@ EXPORT_SYMBOL_GPL(kvm_inject_nmi);
 
 void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
 {
-	WARN_ON(vcpu->arch.exception.pending);
-	vcpu->arch.exception.pending = true;
-	vcpu->arch.exception.has_error_code = true;
-	vcpu->arch.exception.nr = nr;
-	vcpu->arch.exception.error_code = error_code;
+	kvm_multiple_exception(vcpu, nr, true, error_code);
 }
 EXPORT_SYMBOL_GPL(kvm_queue_exception_e);
 

--
			Gleb.

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

[Gleb Natapov]

On Wed, Nov 11, 2009 at 05:29:48PM -0200, 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>
> 
> Index: kvm/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/x86.c
> +++ kvm/arch/x86/kvm/x86.c
> @@ -170,9 +170,21 @@ void kvm_set_apic_base(struct kvm_vcpu *
>  }
>  EXPORT_SYMBOL_GPL(kvm_set_apic_base);
>  
> +static void handle_multiple_faults(struct kvm_vcpu *vcpu)
> +{
> +       if (vcpu->arch.exception.nr != DF_VECTOR) {
> +               vcpu->arch.exception.nr = DF_VECTOR;
> +               vcpu->arch.exception.error_code = 0;
> +       } else
> +               set_bit(KVM_REQ_TRIPLE_FAULT, &vcpu->requests);
> +}
> +
Making #DF from two bening exceptions is very wrong.

>  void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr)
>  {
> -	WARN_ON(vcpu->arch.exception.pending);
> +	if (vcpu->arch.exception.pending) {
> +		handle_multiple_faults(vcpu);
> +		return;
> +	}
>  	vcpu->arch.exception.pending = true;
>  	vcpu->arch.exception.has_error_code = false;
>  	vcpu->arch.exception.nr = nr;
> @@ -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;
When exceptions are handled serially previous exception have to be
replaced by new one. Think about #PF during #DE. #PF should be handled first
before #DE can proceed.

> -		}
> -	}
>  	vcpu->arch.cr2 = addr;
>  	kvm_queue_exception_e(vcpu, PF_VECTOR, error_code);
>  }
> @@ -214,7 +208,10 @@ EXPORT_SYMBOL_GPL(kvm_inject_nmi);
>  
>  void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
>  {
> -	WARN_ON(vcpu->arch.exception.pending);
> +	if (vcpu->arch.exception.pending) {
> +		handle_multiple_faults(vcpu);
> +		return;
> +	}
>  	vcpu->arch.exception.pending = true;
>  	vcpu->arch.exception.has_error_code = true;
>  	vcpu->arch.exception.nr = nr;
> 
> -- 

--
			Gleb.

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

[Jan Kiszka]

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

Nice, preferred here as well. I only have a minor comment below.

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

xx_VECTOR?

> +		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 */
> +		goto queue;
> +}
> +
>  void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr)
>  {
> -	WARN_ON(vcpu->arch.exception.pending);
> -	vcpu->arch.exception.pending = true;
> -	vcpu->arch.exception.has_error_code = false;
> -	vcpu->arch.exception.nr = nr;
> +	kvm_multiple_exception(vcpu, nr, false, 0);
>  }
>  EXPORT_SYMBOL_GPL(kvm_queue_exception);
>  
> @@ -261,25 +310,6 @@ void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long addr,
>  			   u32 error_code)
>  {
>  	++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);
>  }
> @@ -292,11 +322,7 @@ EXPORT_SYMBOL_GPL(kvm_inject_nmi);
>  
>  void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
>  {
> -	WARN_ON(vcpu->arch.exception.pending);
> -	vcpu->arch.exception.pending = true;
> -	vcpu->arch.exception.has_error_code = true;
> -	vcpu->arch.exception.nr = nr;
> -	vcpu->arch.exception.error_code = error_code;
> +	kvm_multiple_exception(vcpu, nr, true, error_code);
>  }
>  EXPORT_SYMBOL_GPL(kvm_queue_exception_e);
>  

Jan

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

[Gleb Natapov]

On Thu, Nov 12, 2009 at 01:41:31PM +0100, Jan Kiszka wrote:
> 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):
> 
> Nice, preferred here as well. I only have a minor comment below.
> 

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>
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 == DE_VECTOR || (vector >= TS_VECTOR && vector <= GP_VECTOR))
+		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 */
+		goto queue;
+}
+
 void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr)
 {
-	WARN_ON(vcpu->arch.exception.pending);
-	vcpu->arch.exception.pending = true;
-	vcpu->arch.exception.has_error_code = false;
-	vcpu->arch.exception.nr = nr;
+	kvm_multiple_exception(vcpu, nr, false, 0);
 }
 EXPORT_SYMBOL_GPL(kvm_queue_exception);
 
@@ -261,25 +310,6 @@ void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long addr,
 			   u32 error_code)
 {
 	++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);
 }
@@ -292,11 +322,7 @@ EXPORT_SYMBOL_GPL(kvm_inject_nmi);
 
 void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
 {
-	WARN_ON(vcpu->arch.exception.pending);
-	vcpu->arch.exception.pending = true;
-	vcpu->arch.exception.has_error_code = true;
-	vcpu->arch.exception.nr = nr;
-	vcpu->arch.exception.error_code = error_code;
+	kvm_multiple_exception(vcpu, nr, true, error_code);
 }
 EXPORT_SYMBOL_GPL(kvm_queue_exception_e);
 
--
			Gleb.

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

[Marcelo Tosatti]

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? 

> +		goto queue;

This goto seems unnecessary.

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

[Gleb Natapov]

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.

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

[Avi Kivity]

>> 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?
>    

The note under interrupt 14 documentation in 5.15 indicates that cr2 is 
updated when the page fault is detected, not delivered, so it is correct 
to update cr2 immediately.

-- 
error compiling committee.c: too many arguments to function

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

[Avi Kivity]

On 11/12/2009 02:26 PM, Gleb Natapov wrote:
>>
>> -	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;
>>      
> When exceptions are handled serially previous exception have to be
> replaced by new one. Think about #PF during #DE. #PF should be handled first
> before #DE can proceed.
>    

"replacing" exceptions is dangerous in the case of debug exceptions and 
machine checks, since restarting execution won't recover them.


-- 
error compiling committee.c: too many arguments to function

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

[Gleb Natapov]

On Sun, Nov 15, 2009 at 02:41:58PM +0200, Avi Kivity wrote:
> On 11/12/2009 02:26 PM, Gleb Natapov wrote:
> >>
> >>-	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;
> >When exceptions are handled serially previous exception have to be
> >replaced by new one. Think about #PF during #DE. #PF should be handled first
> >before #DE can proceed.
> 
> "replacing" exceptions is dangerous in the case of debug exceptions
> and machine checks, since restarting execution won't recover them.
> 
But not replacing them is not better. No point in re-injecting
exception that causes another exception.

--
			Gleb.

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

[Avi Kivity]

On 11/12/2009 03:05 PM, Gleb Natapov wrote:
> On Thu, Nov 12, 2009 at 01:41:31PM +0100, Jan Kiszka wrote:
>    
>> 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):
>>>        
>> Nice, preferred here as well. I only have a minor comment below.
>>
>>      
> 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>
> 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 == DE_VECTOR || (vector>= TS_VECTOR&&  vector<= GP_VECTOR))
> +		return EXCPT_CONTRIBUTORY;
> +	else
> +		return EXCPT_BENIGN;
> +}
>    

It's actually less readable.  I know 11 is between 10 and 13, but is 
NP_VECTOR between TS_VECTOR and GP_VECTOR?

This is better as a switch, or even:

u8 exception_class[] = {
    [PF_VECTOR] EXPT_PF,

etc.


-- 
error compiling committee.c: too many arguments to function

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

[Avi Kivity]

On 11/15/2009 02:51 PM, Gleb Natapov wrote:
>
>> "replacing" exceptions is dangerous in the case of debug exceptions
>> and machine checks, since restarting execution won't recover them.
>>
>>      
> But not replacing them is not better. No point in re-injecting
> exception that causes another exception.
>    

Right, just pointing out there's still a small hole left.  Replacing is 
much better ignoring.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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

Avi Kivity wrote:
> On 11/15/2009 02:51 PM, Gleb Natapov wrote:
>>
>>> "replacing" exceptions is dangerous in the case of debug exceptions
>>> and machine checks, since restarting execution won't recover them.
>>>
>>>      
>> But not replacing them is not better. No point in re-injecting
>> exception that causes another exception.
>>    
> 
> Right, just pointing out there's still a small hole left.  Replacing is
> much better ignoring.

Is the hardware doing some queuing in this case? Would it be overly
complicated to adopt the real behavior here?

Jan


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

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

[Avi Kivity]

On 11/15/2009 04:29 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 11/15/2009 02:51 PM, Gleb Natapov wrote:
>>      
>>>        
>>>> "replacing" exceptions is dangerous in the case of debug exceptions
>>>> and machine checks, since restarting execution won't recover them.
>>>>
>>>>
>>>>          
>>> But not replacing them is not better. No point in re-injecting
>>> exception that causes another exception.
>>>
>>>        
>> Right, just pointing out there's still a small hole left.  Replacing is
>> much better ignoring.
>>      
> Is the hardware doing some queuing in this case?

Gleb has verified by testing, and Intel has confirmed, that the hardware 
does not queue, and will in fact lose traps and NMIs in such a case.  So 
the small hole is actually present in the processor and we're better off 
not queueing!  I've forgotten about this but Gleb reminded me.

> Would it be overly
> complicated to adopt the real behavior here?
>    

Not only would it be difficult, it would also be incorrect.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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

Avi Kivity wrote:
> On 11/15/2009 04:29 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 11/15/2009 02:51 PM, Gleb Natapov wrote:
>>>     
>>>>       
>>>>> "replacing" exceptions is dangerous in the case of debug exceptions
>>>>> and machine checks, since restarting execution won't recover them.
>>>>>
>>>>>
>>>>>          
>>>> But not replacing them is not better. No point in re-injecting
>>>> exception that causes another exception.
>>>>
>>>>        
>>> Right, just pointing out there's still a small hole left.  Replacing is
>>> much better ignoring.
>>>      
>> Is the hardware doing some queuing in this case?
> 
> Gleb has verified by testing, and Intel has confirmed, that the hardware
> does not queue, and will in fact lose traps and NMIs in such a case.  So
> the small hole is actually present in the processor and we're better off
> not queueing!  I've forgotten about this but Gleb reminded me.
> 
>> Would it be overly
>> complicated to adopt the real behavior here?
>>    
> 
> Not only would it be difficult, it would also be incorrect.
> 

I'm all for correctness - so this correctly drop events. :)

Jan


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

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

[Gleb Natapov]

> It's actually less readable.  I know 11 is between 10 and 13, but is
> NP_VECTOR between TS_VECTOR and GP_VECTOR?
> 
> This is better as a switch, or even:
> 
> u8 exception_class[] = {
>    [PF_VECTOR] EXPT_PF,
> 
> etc.
OK what about this then:

From: Eddie Dong<eddie.dong@intel.com>

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>
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9b0602a..5e95ff9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -248,12 +248,68 @@ 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)
+{
+	switch (vector) {
+	case PF_VECTOR:
+		return EXCPT_PF;
+	case DE_VECTOR:
+	case TS_VECTOR:
+	case NP_VECTOR:
+	case SS_VECTOR:
+	case GP_VECTOR:
+		return EXCPT_CONTRIBUTORY;
+	default:
+		break;
+	}
+	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 */
+		goto queue;
+}
+
 void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr)
 {
-	WARN_ON(vcpu->arch.exception.pending);
-	vcpu->arch.exception.pending = true;
-	vcpu->arch.exception.has_error_code = false;
-	vcpu->arch.exception.nr = nr;
+	kvm_multiple_exception(vcpu, nr, false, 0);
 }
 EXPORT_SYMBOL_GPL(kvm_queue_exception);
 
@@ -261,25 +317,6 @@ void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long addr,
 			   u32 error_code)
 {
 	++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);
 }
@@ -292,11 +329,7 @@ EXPORT_SYMBOL_GPL(kvm_inject_nmi);
 
 void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
 {
-	WARN_ON(vcpu->arch.exception.pending);
-	vcpu->arch.exception.pending = true;
-	vcpu->arch.exception.has_error_code = true;
-	vcpu->arch.exception.nr = nr;
-	vcpu->arch.exception.error_code = error_code;
+	kvm_multiple_exception(vcpu, nr, true, error_code);
 }
 EXPORT_SYMBOL_GPL(kvm_queue_exception_e);
 
--
			Gleb.

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

[Ryan Harper]

* Gleb Natapov <gleb@redhat.com> [2009-11-19 09:55]:
> > It's actually less readable.  I know 11 is between 10 and 13, but is
> > NP_VECTOR between TS_VECTOR and GP_VECTOR?
> > 
> > This is better as a switch, or even:
> > 
> > u8 exception_class[] = {
> >    [PF_VECTOR] EXPT_PF,
> > 
> > etc.
> OK what about this then:

Is there a consensus on this yet?  I'm tracking this issue and didn't
see anything get committed yet.


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

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

[Marcelo Tosatti]

On Thu, Nov 19, 2009 at 05:54:07PM +0200, Gleb Natapov wrote:
> > It's actually less readable.  I know 11 is between 10 and 13, but is
> > NP_VECTOR between TS_VECTOR and GP_VECTOR?
> > 
> > This is better as a switch, or even:
> > 
> > u8 exception_class[] = {
> >    [PF_VECTOR] EXPT_PF,
> > 
> > etc.
> OK what about this then:

Looks good.

> 
> From: Eddie Dong<eddie.dong@intel.com>
> 
> 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>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9b0602a..5e95ff9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -248,12 +248,68 @@ 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)
> +{
> +	switch (vector) {
> +	case PF_VECTOR:
> +		return EXCPT_PF;
> +	case DE_VECTOR:
> +	case TS_VECTOR:
> +	case NP_VECTOR:
> +	case SS_VECTOR:
> +	case GP_VECTOR:
> +		return EXCPT_CONTRIBUTORY;
> +	default:
> +		break;
> +	}
> +	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 */
> +		goto queue;
> +}
> +
>  void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr)
>  {
> -	WARN_ON(vcpu->arch.exception.pending);
> -	vcpu->arch.exception.pending = true;
> -	vcpu->arch.exception.has_error_code = false;
> -	vcpu->arch.exception.nr = nr;
> +	kvm_multiple_exception(vcpu, nr, false, 0);
>  }
>  EXPORT_SYMBOL_GPL(kvm_queue_exception);
>  
> @@ -261,25 +317,6 @@ void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long addr,
>  			   u32 error_code)
>  {
>  	++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);
>  }
> @@ -292,11 +329,7 @@ EXPORT_SYMBOL_GPL(kvm_inject_nmi);
>  
>  void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
>  {
> -	WARN_ON(vcpu->arch.exception.pending);
> -	vcpu->arch.exception.pending = true;
> -	vcpu->arch.exception.has_error_code = true;
> -	vcpu->arch.exception.nr = nr;
> -	vcpu->arch.exception.error_code = error_code;
> +	kvm_multiple_exception(vcpu, nr, true, error_code);
>  }
>  EXPORT_SYMBOL_GPL(kvm_queue_exception_e);
>  
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Avi Kivity]

On 11/19/2009 05:54 PM, Gleb Natapov wrote:
>> It's actually less readable.  I know 11 is between 10 and 13, but is
>> NP_VECTOR between TS_VECTOR and GP_VECTOR?
>>
>> This is better as a switch, or even:
>>
>> u8 exception_class[] = {
>>     [PF_VECTOR] EXPT_PF,
>>
>> etc.
>>      
> OK what about this then:
>
> From: Eddie Dong<eddie.dong@intel.com>
>
> Move Double-Fault generation logic out of page fault
> exception generating function to cover more generic case.
>    

Much better.

-- 
error compiling committee.c: too many arguments to function

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

[Marcelo Tosatti]

On Thu, Nov 19, 2009 at 05:54:07PM +0200, Gleb Natapov wrote:
> > It's actually less readable.  I know 11 is between 10 and 13, but is
> > NP_VECTOR between TS_VECTOR and GP_VECTOR?
> > 
> > This is better as a switch, or even:
> > 
> > u8 exception_class[] = {
> >    [PF_VECTOR] EXPT_PF,
> > 
> > etc.
> OK what about this then:
> 
> From: Eddie Dong<eddie.dong@intel.com>
> 
> 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>

Applied this and TSS exceptions, thanks.