Xen devel - Aug 2013 - [Patch 0/4] Xen stack trace printing improvements

This series consists of improvements to Xen''s ability to print traces
of its
own stack, and specifically for the stack overflow case to be able to use
frame pointers in a debug build.

I have dev tested the series in debug and non-debug cases, with and without
memory guards, and I believe that all the stack traces look correct.  However,
I would greatly appreciate a second opinion on the boundary conditions.

The 4th patch is included for anyone wishing to easily test the series; It is
not intended for committing.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

Before, show_trace() had two implementations depending on
CONFIG_FRAME_POINTER.  Some parts were common, while the loops to wander up
the stack were different.

The version aided by frame pointers had a special case for function calls on
wild function pointers, but this doesn''t need to be a special case.

After the refactoring, there are now two implementations of __show_trace()
which differ depending on CONFIG_FRAME_POINTER, and a single show_trace()
with the common bits, including the logic for wild function pointers.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---

The new wild pointer logic is rather larger than its pre-refactor version, but
is rather more legible.  I cant think of a way to compact it without making it
substantially less legible.
---
 xen/arch/x86/traps.c |   66 +++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 57dbd0c..b686177 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -192,14 +192,13 @@ static void show_guest_stack(struct vcpu *v, struct
cpu_user_regs *regs)
 
 #if !defined(CONFIG_FRAME_POINTER)
 
-static void show_trace(struct cpu_user_regs *regs)
+/* Stack trace from pointers found in stack, unaided by frame pointers.  For
+ * caller convenience, this has the same prototype as its alternative, and
+ * simply ignores the rbp parameter.
+ */
+static void __show_trace(unsigned long sp, unsigned long __maybe_unused bp)
 {
-    unsigned long *stack = ESP_BEFORE_EXCEPTION(regs), addr;
-
-    printk("Xen call trace:\n   ");
-
-    printk("[<%p>]", _p(regs->eip));
-    print_symbol(" %s\n   ", regs->eip);
+    unsigned long *stack = (unsigned long *)sp, addr;
 
     while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
     {
@@ -210,36 +209,22 @@ static void show_trace(struct cpu_user_regs *regs)
             print_symbol(" %s\n   ", addr);
         }
     }
-
-    printk("\n");
 }
 
 #else
 
-static void show_trace(struct cpu_user_regs *regs)
+/* Stack trace from frames in the stack, using frame pointers */
+static void __show_trace(unsigned long sp, unsigned long bp)
 {
     unsigned long *frame, next, addr, low, high;
 
-    printk("Xen call trace:\n   ");
-
-    /*
-     * If RIP is not pointing into hypervisor code then someone may have
-     * called into oblivion. Peek to see if they left a return address at
-     * top of stack.
-     */
-    addr = is_active_kernel_text(regs->eip) ||
-           !is_active_kernel_text(*ESP_BEFORE_EXCEPTION(regs)) ?
-           regs->eip : *ESP_BEFORE_EXCEPTION(regs);
-    printk("[<%p>]", _p(addr));
-    print_symbol(" %s\n   ", addr);
-
     /* Bounds for range of valid frame pointer. */
-    low  = (unsigned long)(ESP_BEFORE_EXCEPTION(regs) - 2);
+    low  = sp - 2*sizeof(unsigned long);
     high = (low & ~(STACK_SIZE - 1)) + 
         (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long));
 
     /* The initial frame pointer. */
-    next = regs->ebp;
+    next = bp;
 
     for ( ; ; )
     {
@@ -272,12 +257,39 @@ static void show_trace(struct cpu_user_regs *regs)
 
         low = (unsigned long)&frame[2];
     }
-
-    printk("\n");
 }
 
 #endif
 
+static void show_trace(const struct cpu_user_regs *regs)
+{
+    unsigned long sp = regs->rsp;
+    printk("Xen call trace:\n   ");
+
+    /* If RIP looks sensible, or the top of the stack doesn''t look
sensible,
+     * print RIP at the top of the stack trace. */
+    if ( is_active_kernel_text(regs->rip) ||
+         !is_active_kernel_text(regs->rsp) )
+    {
+        printk("[<%p>]", _p(regs->rip));
+        print_symbol(" %s\n   ", regs->rip);
+    }
+    /* else RIP looks bad but the top of the stack looks ok.  Perhaps we
+     * followed a wild function pointer, so lets assume the top of the stack is
+     * a return address.  Skip past it so__show_trace() doesn''t print
it
+     * again. */
+    else
+    {
+        printk("[<%p>]", _p(sp));
+        print_symbol(" %s\n   ", sp);
+        sp += sizeof (unsigned long);
+    }
+
+    __show_trace(sp, regs->rbp);
+
+    printk("\n");
+}
+
 void show_stack(struct cpu_user_regs *regs)
 {
     unsigned long *stack = ESP_BEFORE_EXCEPTION(regs), addr;
-- 
1.7.10.4

Move the boundary into current.h along with the other stack manipulation code,
and apply the same bottom boundary to both the frame pointer and non-frame
pointer case.  This will prevent the non-frame pointer case from finding
certainly-spurious values on the stack in the guest_cpu_user_regs section.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/traps.c          |    9 ++++-----
 xen/include/asm-x86/current.h |    9 +++++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index b686177..2bcfc7b 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -199,8 +199,9 @@ static void show_guest_stack(struct vcpu *v, struct
cpu_user_regs *regs)
 static void __show_trace(unsigned long sp, unsigned long __maybe_unused bp)
 {
     unsigned long *stack = (unsigned long *)sp, addr;
+    unsigned long *bottom = (unsigned long *)get_printable_stack_bottom(sp);
 
-    while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
+    while ( stack <= bottom )
     {
         addr = *stack++;
         if ( is_active_kernel_text(addr) )
@@ -216,12 +217,10 @@ static void __show_trace(unsigned long sp, unsigned long
__maybe_unused bp)
 /* Stack trace from frames in the stack, using frame pointers */
 static void __show_trace(unsigned long sp, unsigned long bp)
 {
-    unsigned long *frame, next, addr, low, high;
+    unsigned long *frame, next, addr;
 
     /* Bounds for range of valid frame pointer. */
-    low  = sp - 2*sizeof(unsigned long);
-    high = (low & ~(STACK_SIZE - 1)) + 
-        (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long));
+    unsigned long low = sp, high = get_printable_stack_bottom(sp);
 
     /* The initial frame pointer. */
     next = bp;
diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
index bec4dbe..da2d1bf 100644
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -50,6 +50,15 @@ static inline struct cpu_info *get_cpu_info(void)
 #define get_stack_bottom()                      \
     ((unsigned long)&get_cpu_info()->guest_cpu_user_regs.es)
 
+/*
+ * Get the bottom-of-stack, as useful for printing stack traces. This is an
+ * equivalent place on the stack as guest_cpu_user_regs(), but works on an
+ * arbitrary stack pointer rather than the current stack.
+ */
+#define get_printable_stack_bottom(sp)          \
+    ((sp & (~(STACK_SIZE-1))) +                 \
+     (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long)))
+
 #define reset_stack_and_jump(__fn)              \
     __asm__ __volatile__ (                      \
         "mov %0,%%"__OP"sp; jmp %c1"            \
-- 
1.7.10.4

Pass a full set of cpu_user_regs, and defer the hand-coded stack printing to
__show_trace(), which will correctly use frame pointers if available.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/traps.c            |   15 +++------------
 xen/arch/x86/x86_64/traps.c     |    2 +-
 xen/include/asm-x86/processor.h |    2 +-
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 2bcfc7b..17255d5 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -315,11 +315,11 @@ void show_stack(struct cpu_user_regs *regs)
     show_trace(regs);
 }
 
-void show_stack_overflow(unsigned int cpu, unsigned long esp)
+void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
 {
 #ifdef MEMORY_GUARD
+    unsigned long esp = regs->rsp;
     unsigned long esp_top, esp_bottom;
-    unsigned long *stack, addr;
 
     esp_bottom = (esp | (STACK_SIZE - 1)) + 1;
     esp_top    = esp_bottom - PRIMARY_STACK_SIZE;
@@ -342,16 +342,7 @@ void show_stack_overflow(unsigned int cpu, unsigned long
esp)
     printk("Xen stack overflow (dumping trace %p-%p):\n   ",
            (void *)esp, (void *)esp_bottom);
 
-    stack = (unsigned long *)esp;
-    while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
-    {
-        addr = *stack++;
-        if ( is_active_kernel_text(addr) )
-        {
-            printk("%p: [<%p>]", stack, _p(addr));
-            print_symbol(" %s\n   ", addr);
-        }
-    }
+    __show_trace(esp, regs->rbp);
 
     printk("\n");
 #endif
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index bcd7609..385b366 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -247,7 +247,7 @@ void do_double_fault(struct cpu_user_regs *regs)
 
     printk("CPU:    %d\n", cpu);
     _show_registers(regs, crs, CTXT_hypervisor, NULL);
-    show_stack_overflow(cpu, regs->rsp);
+    show_stack_overflow(cpu, regs);
 
     panic("DOUBLE FAULT -- system shutdown\n");
 }
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 5cdacc7..d050bac 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -507,7 +507,7 @@ extern always_inline void prefetchw(const void *x)
 #endif
 
 void show_stack(struct cpu_user_regs *regs);
-void show_stack_overflow(unsigned int cpu, unsigned long esp);
+void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs);
 void show_registers(struct cpu_user_regs *regs);
 void show_execution_state(struct cpu_user_regs *regs);
 #define dump_execution_state() run_in_exception_handler(show_execution_state)
-- 
1.7.10.4

---
 xen/arch/x86/traps.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 17255d5..f5e99c1 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -34,6 +34,7 @@
 #include <xen/console.h>
 #include <xen/shutdown.h>
 #include <xen/guest_access.h>
+#include <xen/keyhandler.h>
 #include <asm/regs.h>
 #include <xen/delay.h>
 #include <xen/event.h>
@@ -3742,6 +3743,76 @@ unsigned long do_get_debugreg(int reg)
     return -EINVAL;
 }
 
+static noinline void recursion(int depth)
+{
+    /* junk on stack to fool naive algorithm */
+    volatile unsigned long addr1 = (unsigned long)&do_get_debugreg;
+    volatile unsigned long tsc;
+    volatile unsigned long addr2 = (unsigned long)&do_set_debugreg;
+
+    if ( depth == 0 )
+    {
+        printk("Did you mean recursion()?\n");
+        run_in_exception_handler(show_stack);
+        return;
+    }
+    else
+    {
+        rdtscll(tsc);
+        printk("depth %d, addr1 0x%016"PRIx64", addr2
0x%016"PRIx64", tsc %"PRIu64"\n",
+               depth, addr1, addr2, tsc);
+        recursion(depth-1);
+        printk("done\n"); /* So GCC cant perform tailcall
optimisation */
+    }
+}
+
+static void stacktest1(unsigned char key)
+{
+    printk("In %s()\n", __func__);
+    recursion(5);
+}
+
+static void stacktest2(unsigned char key, struct cpu_user_regs * regs)
+{
+    printk("In %s()\n", __func__);
+    recursion(5);
+}
+
+static void stacktest3(unsigned char key)
+{
+    printk("In %s()\n", __func__);
+    recursion(500);
+}
+
+static struct keyhandler stacktest_handler[] = {
+    {
+        .irq_callback = 0,
+        .u.fn = stacktest1,
+        .desc = "Xen stack #1 - Recurse a little and see trace."
+    },
+
+    {
+        .irq_callback = 1,
+        .u.irq_fn = stacktest2,
+        .desc = "Xen stack #2 - Recurse a little and see trace in
irq."
+    },
+
+    {
+        .irq_callback = 0,
+        .u.fn = stacktest3,
+        .desc = "Xen stack #3 - Cause stack overflow."
+    },
+ };
+
+static int __init initialize_crashtable(void)
+{
+    register_keyhandler(''1'', &stacktest_handler[0]);
+    register_keyhandler(''2'', &stacktest_handler[1]);
+    register_keyhandler(''3'', &stacktest_handler[2]);
+    return 1;
+}
+__initcall(initialize_crashtable);
+
 /*
  * Local variables:
  * mode: C
-- 
1.7.10.4

>>> On 09.08.13 at 21:55, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -50,6 +50,15 @@ static inline struct cpu_info *get_cpu_info(void)
>  #define get_stack_bottom()                      \
>      ((unsigned long)&get_cpu_info()->guest_cpu_user_regs.es)
>  
> +/*
> + * Get the bottom-of-stack, as useful for printing stack traces. This is
an
> + * equivalent place on the stack as guest_cpu_user_regs(), but works on an
> + * arbitrary stack pointer rather than the current stack.
> + */
> +#define get_printable_stack_bottom(sp)          \
> +    ((sp & (~(STACK_SIZE-1))) +                 \
> +     (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long)))
Iirc in the prior RFC version of these patches you were convinced
that the adjustment by 2 stack slots was wrong (and that was the
primary reason why you had asked for comments). Now you''re
keeping it, without making clear what made you change your
opinion.

Jan

On 12/08/13 09:46, Jan Beulich wrote:
>>>> On 09.08.13 at 21:55, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/asm-x86/current.h
>> +++ b/xen/include/asm-x86/current.h
>> @@ -50,6 +50,15 @@ static inline struct cpu_info *get_cpu_info(void)
>>  #define get_stack_bottom()                      \
>>      ((unsigned long)&get_cpu_info()->guest_cpu_user_regs.es)
>>  
>> +/*
>> + * Get the bottom-of-stack, as useful for printing stack traces. This
is an
>> + * equivalent place on the stack as guest_cpu_user_regs(), but works
on an
>> + * arbitrary stack pointer rather than the current stack.
>> + */
>> +#define get_printable_stack_bottom(sp)          \
>> +    ((sp & (~(STACK_SIZE-1))) +                 \
>> +     (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long)))
> Iirc in the prior RFC version of these patches you were convinced
> that the adjustment by 2 stack slots was wrong (and that was the
> primary reason why you had asked for comments). Now you''re
> keeping it, without making clear what made you change your
> opinion.
>
> Jan
>
I was concerned about the -2 adjustment to ''low'', which could
end up
causing a triple fault.

The adjustment here covers call instructions out of assembly code, which
lack a traditional stack layout.  Specifically, removing the -2 causes
the frame pointer code to find a frame pointer of value 0x0...001 as an
unknown text symbol.

I suppose one side effect would be that the non-frame pointer case will
miss the bottom return address; That could be worked around by having
the -2 conditional on the frame pointer case.  This would still have the
advantage of the non-frame pointer case not attempting to interpret the
guest_cpu_user_regs for xen return addresses.

~Andrew

>>> On 12.08.13 at 11:43, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 12/08/13 09:46, Jan Beulich wrote:
>>>>> On 09.08.13 at 21:55, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/include/asm-x86/current.h
>>> +++ b/xen/include/asm-x86/current.h
>>> @@ -50,6 +50,15 @@ static inline struct cpu_info
*get_cpu_info(void)
>>>  #define get_stack_bottom()                      \
>>>      ((unsigned
long)&get_cpu_info()->guest_cpu_user_regs.es)
>>>  
>>> +/*
>>> + * Get the bottom-of-stack, as useful for printing stack traces.
This is an
>>> + * equivalent place on the stack as guest_cpu_user_regs(), but
works on an
>>> + * arbitrary stack pointer rather than the current stack.
>>> + */
>>> +#define get_printable_stack_bottom(sp)          \
>>> +    ((sp & (~(STACK_SIZE-1))) +                 \
>>> +     (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned
long)))
>> Iirc in the prior RFC version of these patches you were convinced
>> that the adjustment by 2 stack slots was wrong (and that was the
>> primary reason why you had asked for comments). Now you''re
>> keeping it, without making clear what made you change your
>> opinion.
> 
> I was concerned about the -2 adjustment to ''low'', which
could end up
> causing a triple fault.
> 
> The adjustment here covers call instructions out of assembly code, which
> lack a traditional stack layout.  Specifically, removing the -2 causes
> the frame pointer code to find a frame pointer of value 0x0...001 as an
> unknown text symbol.
> 
> I suppose one side effect would be that the non-frame pointer case will
> miss the bottom return address; That could be worked around by having
> the -2 conditional on the frame pointer case.  This would still have the
> advantage of the non-frame pointer case not attempting to interpret the
> guest_cpu_user_regs for xen return addresses.
I think not dropping potentially useful information is more important
than avoiding the displaying of useless/bogus information. So the
primary goal ought to be to not leave out any valid entry on the
stack. Only then we would ideally also never print a garbage entry.

Jan

Move the boundary into current.h along with the other stack manipulation code,
and apply the same bottom boundary to both the frame pointer and non-frame
pointer case.  This will prevent the non-frame pointer case from finding
certainly-spurious values on the stack in the guest_cpu_user_regs section.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---

Changes since v1:
 * Change printable bottom depending on frame pointers
---
 xen/arch/x86/traps.c          |    9 ++++-----
 xen/include/asm-x86/current.h |   22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index b686177..2bcfc7b 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -199,8 +199,9 @@ static void show_guest_stack(struct vcpu *v, struct
cpu_user_regs *regs)
 static void __show_trace(unsigned long sp, unsigned long __maybe_unused bp)
 {
     unsigned long *stack = (unsigned long *)sp, addr;
+    unsigned long *bottom = (unsigned long *)get_printable_stack_bottom(sp);
 
-    while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
+    while ( stack <= bottom )
     {
         addr = *stack++;
         if ( is_active_kernel_text(addr) )
@@ -216,12 +217,10 @@ static void __show_trace(unsigned long sp, unsigned long
__maybe_unused bp)
 /* Stack trace from frames in the stack, using frame pointers */
 static void __show_trace(unsigned long sp, unsigned long bp)
 {
-    unsigned long *frame, next, addr, low, high;
+    unsigned long *frame, next, addr;
 
     /* Bounds for range of valid frame pointer. */
-    low  = sp - 2*sizeof(unsigned long);
-    high = (low & ~(STACK_SIZE - 1)) + 
-        (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long));
+    unsigned long low = sp, high = get_printable_stack_bottom(sp);
 
     /* The initial frame pointer. */
     next = bp;
diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
index bec4dbe..02da2ec 100644
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -50,6 +50,28 @@ static inline struct cpu_info *get_cpu_info(void)
 #define get_stack_bottom()                      \
     ((unsigned long)&get_cpu_info()->guest_cpu_user_regs.es)
 
+/*
+ * Get the bottom-of-stack, as useful for printing stack traces.  This is
+ * similar position as is returned by get_cpu_info(), but works on arbitrary
+ * stack pointer, rather than just the current.
+ *
+ * In the non-frame pointer case, the boundary is exactly at the cpu_info
+ * structure, which prevents the stack trace walker from mis-interpreting
+ * guest register as Xen return addresses.
+ *
+ * For the frame pointer case, the boundary is further adjusted by two words,
+ * as the call out of assembly does not contain a traditional C stack with
+ * frame pointers.
+ */
+#ifdef CONFIG_FRAME_POINTER
+#define get_printable_stack_bottom(sp)          \
+    ((sp & (~(STACK_SIZE-1))) +                 \
+     (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long)))
+#else
+#define get_printable_stack_bottom(sp)          \
+    ((sp & (~(STACK_SIZE-1))) + (STACK_SIZE - sizeof(struct cpu_info)))
+#endif
+
 #define reset_stack_and_jump(__fn)              \
     __asm__ __volatile__ (                      \
         "mov %0,%%"__OP"sp; jmp %c1"            \
-- 
1.7.10.4

>>> On 12.08.13 at 14:15, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> +/*
> + * Get the bottom-of-stack, as useful for printing stack traces.  This is
> + * similar position as is returned by get_cpu_info(), but works on
arbitrary
> + * stack pointer, rather than just the current.
> + *
> + * In the non-frame pointer case, the boundary is exactly at the cpu_info
> + * structure, which prevents the stack trace walker from mis-interpreting
> + * guest register as Xen return addresses.
> + *
> + * For the frame pointer case, the boundary is further adjusted by two
words,
> + * as the call out of assembly does not contain a traditional C stack with
> + * frame pointers.
> + */
> +#ifdef CONFIG_FRAME_POINTER
> +#define get_printable_stack_bottom(sp)          \
> +    ((sp & (~(STACK_SIZE-1))) +                 \
> +     (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long)))
> +#else
> +#define get_printable_stack_bottom(sp)          \
> +    ((sp & (~(STACK_SIZE-1))) + (STACK_SIZE - sizeof(struct
cpu_info)))
> +#endif
Hmm, the delta being 2 slots makes no sense to me: The only
difference ought to be the saved frame pointer, i.e. one slot.
With what you propose here I''d suspect that the return address
pointing into assembly code might be lost now with frame pointers.

Jan

On 12/08/13 14:44, Jan Beulich wrote:
>>>> On 12.08.13 at 14:15, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> +/*
>> + * Get the bottom-of-stack, as useful for printing stack traces.  This
is
>> + * similar position as is returned by get_cpu_info(), but works on
arbitrary
>> + * stack pointer, rather than just the current.
>> + *
>> + * In the non-frame pointer case, the boundary is exactly at the
cpu_info
>> + * structure, which prevents the stack trace walker from
mis-interpreting
>> + * guest register as Xen return addresses.
>> + *
>> + * For the frame pointer case, the boundary is further adjusted by two
words,
>> + * as the call out of assembly does not contain a traditional C stack
with
>> + * frame pointers.
>> + */
>> +#ifdef CONFIG_FRAME_POINTER
>> +#define get_printable_stack_bottom(sp)          \
>> +    ((sp & (~(STACK_SIZE-1))) +                 \
>> +     (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long)))
>> +#else
>> +#define get_printable_stack_bottom(sp)          \
>> +    ((sp & (~(STACK_SIZE-1))) + (STACK_SIZE - sizeof(struct
cpu_info)))
>> +#endif
> Hmm, the delta being 2 slots makes no sense to me: The only
> difference ought to be the saved frame pointer, i.e. one slot.
> With what you propose here I''d suspect that the return address
> pointing into assembly code might be lost now with frame pointers.
>
> Jan
>
It was never available, and I cant find a way of getting at it without
special casing the frame-pointer code, or faking up a frame pointer in
the assembly.

~Andrew

On 09/08/2013 20:55, "Andrew Cooper" <andrew.cooper3@citrix.com>
wrote:
> This series consists of improvements to Xen''s ability to print
traces of its
> own stack, and specifically for the stack overflow case to be able to use
> frame pointers in a debug build.
> 
> I have dev tested the series in debug and non-debug cases, with and without
> memory guards, and I believe that all the stack traces look correct. 
However,
> I would greatly appreciate a second opinion on the boundary conditions.
> 
> The 4th patch is included for anyone wishing to easily test the series; It
is
> not intended for committing.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
Acked-by: Keir Fraser <keir@xen.org>

On 09/09/13 12:09, Keir Fraser wrote:
> On 09/08/2013 20:55, "Andrew Cooper"
<andrew.cooper3@citrix.com> wrote:
>
>> This series consists of improvements to Xen''s ability to print
traces of its
>> own stack, and specifically for the stack overflow case to be able to
use
>> frame pointers in a debug build.
>>
>> I have dev tested the series in debug and non-debug cases, with and
without
>> memory guards, and I believe that all the stack traces look correct. 
However,
>> I would greatly appreciate a second opinion on the boundary conditions.
>>
>> The 4th patch is included for anyone wishing to easily test the series;
It is
>> not intended for committing.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
> Acked-by: Keir Fraser <keir@xen.org>
>
>
Thanks for the ack, but I would like to hold off on committing this
until I have fixed the bug for following frame pointers through
exception frames.

~Andrew

>>> On 09.09.13 at 14:28, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 09/09/13 12:09, Keir Fraser wrote:
>> On 09/08/2013 20:55, "Andrew Cooper"
<andrew.cooper3@citrix.com> wrote:
>>
>>> This series consists of improvements to Xen''s ability to
print traces of its
>>> own stack, and specifically for the stack overflow case to be able
to use
>>> frame pointers in a debug build.
>>>
>>> I have dev tested the series in debug and non-debug cases, with and
without
>>> memory guards, and I believe that all the stack traces look
correct.
> However,
>>> I would greatly appreciate a second opinion on the boundary
conditions.
>>>
>>> The 4th patch is included for anyone wishing to easily test the
series; It
> is
>>> not intended for committing.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Keir Fraser <keir@xen.org>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Tim Deegan <tim@xen.org>
>> Acked-by: Keir Fraser <keir@xen.org>
>>
>>
> 
> Thanks for the ack, but I would like to hold off on committing this
> until I have fixed the bug for following frame pointers through
> exception frames.
Ah, that''s why you didn''t ping for an ack yourself? I had this
in my
to-be-committed-eventually list of things, but with your comment
above I assume you''ll re-submit once you sorted out that remaining
item, and I can therefore drop it from that list of mine.

Jan

On 09/09/13 13:44, Jan Beulich wrote:
>>>> On 09.09.13 at 14:28, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> On 09/09/13 12:09, Keir Fraser wrote:
>>> On 09/08/2013 20:55, "Andrew Cooper"
<andrew.cooper3@citrix.com> wrote:
>>>
>>>> This series consists of improvements to Xen''s ability
to print traces of its
>>>> own stack, and specifically for the stack overflow case to be
able to use
>>>> frame pointers in a debug build.
>>>>
>>>> I have dev tested the series in debug and non-debug cases, with
and without
>>>> memory guards, and I believe that all the stack traces look
correct.
>> However,
>>>> I would greatly appreciate a second opinion on the boundary
conditions.
>>>>
>>>> The 4th patch is included for anyone wishing to easily test the
series; It
>> is
>>>> not intended for committing.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> CC: Keir Fraser <keir@xen.org>
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Tim Deegan <tim@xen.org>
>>> Acked-by: Keir Fraser <keir@xen.org>
>>>
>>>
>> Thanks for the ack, but I would like to hold off on committing this
>> until I have fixed the bug for following frame pointers through
>> exception frames.
> Ah, that''s why you didn''t ping for an ack yourself? I had
this in my
> to-be-committed-eventually list of things, but with your comment
> above I assume you''ll re-submit once you sorted out that remaining
> item, and I can therefore drop it from that list of mine.
>
> Jan
>
Yes - it is high up my todo list, but there are some regressions moving
to 4.3 which are higher up.

~Andrew