[PATCH] x86: make compat_iret() domain crash cases distinguishable

[PATCH] x86: make compat_iret() domain crash cases distinguishable

[Jan Beulich]

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

Rather than issuing a (mostly) useless separate message, rely on
domain_crash() providing enough data, and leverage the line number
information it prints.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_64/compat/traps.c
+++ b/xen/arch/x86/x86_64/compat/traps.c
@@ -77,19 +77,28 @@ unsigned int compat_iret(void)
 
     /* Restore EAX (clobbered by hypercall). */
     if ( unlikely(__get_user(regs->_eax, (u32 *)regs->rsp)) )
-        goto exit_and_crash;
+    {
+        domain_crash(v->domain);
+        return 0;
+    }
 
     /* Restore CS and EIP. */
     if ( unlikely(__get_user(regs->_eip, (u32 *)regs->rsp + 1)) ||
         unlikely(__get_user(regs->cs, (u32 *)regs->rsp + 2)) )
-        goto exit_and_crash;
+    {
+        domain_crash(v->domain);
+        return 0;
+    }
 
     /*
      * Fix up and restore EFLAGS. We fix up in a local staging area
      * to avoid firing the BUG_ON(IOPL) check in arch_get_info_guest.
      */
     if ( unlikely(__get_user(eflags, (u32 *)regs->rsp + 3)) )
-        goto exit_and_crash;
+    {
+        domain_crash(v->domain);
+        return 0;
+    }
     regs->_eflags = (eflags & ~X86_EFLAGS_IOPL) | X86_EFLAGS_IF;
 
     if ( unlikely(eflags & X86_EFLAGS_VM) )
@@ -126,7 +135,10 @@ unsigned int compat_iret(void)
             }
         }
         if ( rc )
-            goto exit_and_crash;
+        {
+            domain_crash(v->domain);
+            return 0;
+        }
         regs->_esp = ksp;
         regs->ss = v->arch.pv_vcpu.kernel_ss;
 
@@ -136,21 +148,27 @@ unsigned int compat_iret(void)
         regs->_eflags &= ~(X86_EFLAGS_VM|X86_EFLAGS_RF|
                            X86_EFLAGS_NT|X86_EFLAGS_TF);
         if ( unlikely(__put_user(0, (u32 *)regs->rsp)) )
-            goto exit_and_crash;
+        {
+            domain_crash(v->domain);
+            return 0;
+        }
         regs->_eip = ti->address;
         regs->cs = ti->cs;
     }
     else if ( unlikely(ring_0(regs)) )
-        goto exit_and_crash;
-    else if ( !ring_1(regs) )
     {
-        /* Return to ring 2/3: restore ESP and SS. */
-        if ( __get_user(regs->ss, (u32 *)regs->rsp + 5)
-            || __get_user(regs->_esp, (u32 *)regs->rsp + 4))
-            goto exit_and_crash;
+        domain_crash(v->domain);
+        return 0;
     }
-    else
+    else if ( ring_1(regs) )
         regs->_esp += 16;
+    /* Return to ring 2/3: restore ESP and SS. */
+    else if ( __get_user(regs->ss, (u32 *)regs->rsp + 5) ||
+              __get_user(regs->_esp, (u32 *)regs->rsp + 4) )
+    {
+        domain_crash(v->domain);
+        return 0;
+    }
 
     /* Restore upcall mask from supplied EFLAGS.IF. */
     vcpu_info(v, evtchn_upcall_mask) = !(eflags & X86_EFLAGS_IF);
@@ -162,11 +180,6 @@ unsigned int compat_iret(void)
      * value.
      */
     return regs->_eax;
-
- exit_and_crash:
-    gprintk(XENLOG_ERR, "Fatal IRET error\n");
-    domain_crash(v->domain);
-    return 0;
 }
 
 static long compat_register_guest_callback(




[-- Attachment #2: hcall-printk-3.patch --]
[-- Type: text/plain, Size: 3179 bytes --]

x86: make compat_iret() domain crash cases distinguishable

Rather than issuing a (mostly) useless separate message, rely on
domain_crash() providing enough data, and leverage the line number
information it prints.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_64/compat/traps.c
+++ b/xen/arch/x86/x86_64/compat/traps.c
@@ -77,19 +77,28 @@ unsigned int compat_iret(void)
 
     /* Restore EAX (clobbered by hypercall). */
     if ( unlikely(__get_user(regs->_eax, (u32 *)regs->rsp)) )
-        goto exit_and_crash;
+    {
+        domain_crash(v->domain);
+        return 0;
+    }
 
     /* Restore CS and EIP. */
     if ( unlikely(__get_user(regs->_eip, (u32 *)regs->rsp + 1)) ||
         unlikely(__get_user(regs->cs, (u32 *)regs->rsp + 2)) )
-        goto exit_and_crash;
+    {
+        domain_crash(v->domain);
+        return 0;
+    }
 
     /*
      * Fix up and restore EFLAGS. We fix up in a local staging area
      * to avoid firing the BUG_ON(IOPL) check in arch_get_info_guest.
      */
     if ( unlikely(__get_user(eflags, (u32 *)regs->rsp + 3)) )
-        goto exit_and_crash;
+    {
+        domain_crash(v->domain);
+        return 0;
+    }
     regs->_eflags = (eflags & ~X86_EFLAGS_IOPL) | X86_EFLAGS_IF;
 
     if ( unlikely(eflags & X86_EFLAGS_VM) )
@@ -126,7 +135,10 @@ unsigned int compat_iret(void)
             }
         }
         if ( rc )
-            goto exit_and_crash;
+        {
+            domain_crash(v->domain);
+            return 0;
+        }
         regs->_esp = ksp;
         regs->ss = v->arch.pv_vcpu.kernel_ss;
 
@@ -136,21 +148,27 @@ unsigned int compat_iret(void)
         regs->_eflags &= ~(X86_EFLAGS_VM|X86_EFLAGS_RF|
                            X86_EFLAGS_NT|X86_EFLAGS_TF);
         if ( unlikely(__put_user(0, (u32 *)regs->rsp)) )
-            goto exit_and_crash;
+        {
+            domain_crash(v->domain);
+            return 0;
+        }
         regs->_eip = ti->address;
         regs->cs = ti->cs;
     }
     else if ( unlikely(ring_0(regs)) )
-        goto exit_and_crash;
-    else if ( !ring_1(regs) )
     {
-        /* Return to ring 2/3: restore ESP and SS. */
-        if ( __get_user(regs->ss, (u32 *)regs->rsp + 5)
-            || __get_user(regs->_esp, (u32 *)regs->rsp + 4))
-            goto exit_and_crash;
+        domain_crash(v->domain);
+        return 0;
     }
-    else
+    else if ( ring_1(regs) )
         regs->_esp += 16;
+    /* Return to ring 2/3: restore ESP and SS. */
+    else if ( __get_user(regs->ss, (u32 *)regs->rsp + 5) ||
+              __get_user(regs->_esp, (u32 *)regs->rsp + 4) )
+    {
+        domain_crash(v->domain);
+        return 0;
+    }
 
     /* Restore upcall mask from supplied EFLAGS.IF. */
     vcpu_info(v, evtchn_upcall_mask) = !(eflags & X86_EFLAGS_IF);
@@ -162,11 +180,6 @@ unsigned int compat_iret(void)
      * value.
      */
     return regs->_eax;
-
- exit_and_crash:
-    gprintk(XENLOG_ERR, "Fatal IRET error\n");
-    domain_crash(v->domain);
-    return 0;
 }
 
 static long compat_register_guest_callback(

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: make compat_iret() domain crash cases distinguishable

[Andrew Cooper]

On 30/10/15 17:46, Jan Beulich wrote:
> Rather than issuing a (mostly) useless separate message, rely on
> domain_crash() providing enough data, and leverage the line number
> information it prints.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

The messages are not completely useless, and they do save a round-trip
to the debug symbols (which can be a very long RTT when dealing with
customers).

I agree that moving the domain_crash()'s is a good idea, but can we get
away with just demoing the gprintk()s to gdprintk()s ?

~Andrew

Re: [PATCH] x86: make compat_iret() domain crash cases distinguishable

[Andrew Cooper]

On 30/10/15 19:05, Andrew Cooper wrote:
> On 30/10/15 17:46, Jan Beulich wrote:
>> Rather than issuing a (mostly) useless separate message, rely on
>> domain_crash() providing enough data, and leverage the line number
>> information it prints.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> The messages are not completely useless, and they do save a round-trip
> to the debug symbols (which can be a very long RTT when dealing with
> customers).
>
> I agree that moving the domain_crash()'s is a good idea, but can we get
> away with just demoing the gprintk()s to gdprintk()s ?

Apologies - this review text was intended for "[PATCH] x86: simplify
do_iret() domain crashing".  I replied to the wrong email.

This patch "[PATCH] x86: make compat_iret() domain crash cases
distinguishable" is

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] x86: make compat_iret() domain crash cases distinguishable

[Jan Beulich]

>>> On 30.10.15 at 20:05, <andrew.cooper3@citrix.com> wrote:
> On 30/10/15 17:46, Jan Beulich wrote:
>> Rather than issuing a (mostly) useless separate message, rely on
>> domain_crash() providing enough data, and leverage the line number
>> information it prints.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> The messages are not completely useless, and they do save a round-trip
> to the debug symbols (which can be a very long RTT when dealing with
> customers).
> 
> I agree that moving the domain_crash()'s is a good idea, but can we get
> away with just demoing the gprintk()s to gdprintk()s ?

How would that aid with the customer case (who wouldn't normally run
debug hypervisors)?

Jan

Re: [PATCH] x86: make compat_iret() domain crash cases distinguishable

[Jan Beulich]

>>> On 30.10.15 at 20:05, <andrew.cooper3@citrix.com> wrote:
> On 30/10/15 17:46, Jan Beulich wrote:
>> Rather than issuing a (mostly) useless separate message, rely on
>> domain_crash() providing enough data, and leverage the line number
>> information it prints.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> The messages are not completely useless, and they do save a round-trip
> to the debug symbols (which can be a very long RTT when dealing with
> customers).

Oh, also: Why would looking at the debug symbols be needed? You
get file name and line number printed.

Jan

Re: [PATCH] x86: make compat_iret() domain crash cases distinguishable

[Andrew Cooper]

On 02/11/15 10:56, Jan Beulich wrote:
>>>> On 30.10.15 at 20:05, <andrew.cooper3@citrix.com> wrote:
>> On 30/10/15 17:46, Jan Beulich wrote:
>>> Rather than issuing a (mostly) useless separate message, rely on
>>> domain_crash() providing enough data, and leverage the line number
>>> information it prints.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> The messages are not completely useless, and they do save a round-trip
>> to the debug symbols (which can be a very long RTT when dealing with
>> customers).
>>
>> I agree that moving the domain_crash()'s is a good idea, but can we get
>> away with just demoing the gprintk()s to gdprintk()s ?
> How would that aid with the customer case (who wouldn't normally run
> debug hypervisors)?

I suppose it wouldn't in the general case.

XenServer now ships both regular and debug hypervisors to short circuit
the first RTT when dealing with host crashes.  A side effect is that
guest crashes can be diagnosed more easily by switching to the debug
hypervisor.

~Andrew

Re: [PATCH] x86: make compat_iret() domain crash cases distinguishable

[Andrew Cooper]

On 02/11/15 10:57, Jan Beulich wrote:
>>>> On 30.10.15 at 20:05, <andrew.cooper3@citrix.com> wrote:
>> On 30/10/15 17:46, Jan Beulich wrote:
>>> Rather than issuing a (mostly) useless separate message, rely on
>>> domain_crash() providing enough data, and leverage the line number
>>> information it prints.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> The messages are not completely useless, and they do save a round-trip
>> to the debug symbols (which can be a very long RTT when dealing with
>> customers).
> Oh, also: Why would looking at the debug symbols be needed? You
> get file name and line number printed.

Any file patched will end up having a slide on the line number.  The use
of the debug symbols is necessary to confirm, especially in cases like
this where there are multiple possibilities close to each other.

~Andrew

Re: [PATCH] x86: make compat_iret() domain crash cases distinguishable

[Jan Beulich]

>>> On 02.11.15 at 12:10, <andrew.cooper3@citrix.com> wrote:
> On 02/11/15 10:57, Jan Beulich wrote:
>>>>> On 30.10.15 at 20:05, <andrew.cooper3@citrix.com> wrote:
>>> On 30/10/15 17:46, Jan Beulich wrote:
>>>> Rather than issuing a (mostly) useless separate message, rely on
>>>> domain_crash() providing enough data, and leverage the line number
>>>> information it prints.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> The messages are not completely useless, and they do save a round-trip
>>> to the debug symbols (which can be a very long RTT when dealing with
>>> customers).
>> Oh, also: Why would looking at the debug symbols be needed? You
>> get file name and line number printed.
> 
> Any file patched will end up having a slide on the line number.  The use
> of the debug symbols is necessary to confirm, especially in cases like
> this where there are multiple possibilities close to each other.

But with version control and proper release numbering you ought to
be able to tell the exact state of the sources at the time the package
got built?

Jan