Xen devel - Aug 2013 - [PATCH] x86/boot: Explicitly clean pcpu stacks in debug builds

This reduces confusion when looking at a hexdump of the pcpu stacks and
wondering were on earth some of the junk was coming from.  Also leave some
grep fodder for finding where the BSP switches stack (because it took me far
longer to find than I care to admit to)

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

---

I came across this while developing my pcpu stack hexdumping capability into
the crashdump analyser.  While I know it is not neccessary for Xen to function
correctly, I do feel that it is quite important when getting to this level of
debugging.

I guess this patch is partly RFC, but it does successfully boot on my test
box, and fix stack junk seen on all CPUs.
---
 xen/arch/x86/boot/x86_64.S |    9 +++++++++
 xen/arch/x86/smpboot.c     |    4 ++++
 2 files changed, 13 insertions(+)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index ed3888d..a45549b 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -13,6 +13,15 @@
         mov     mmu_cr4_features(%rip),%rcx
         mov     %rcx,%cr4
 
+#ifndef NDEBUG
+        /* Clean stack. */
+        mov     stack_start(%rip),%rdi
+        mov     $(STACK_SIZE>>8),%rcx
+        xor     %eax,%eax
+        rep stosq
+#endif
+
+        /* Move to cpu0_stack. */
         mov     stack_start(%rip),%rsp
         or      $(STACK_SIZE-CPUINFO_sizeof),%rsp
 
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 3df4e88..eced9ba 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -662,6 +662,10 @@ static int cpu_smpboot_alloc(unsigned int cpu)
     stack_base[cpu] = alloc_xenheap_pages(STACK_ORDER, 0);
     if ( stack_base[cpu] == NULL )
         goto oom;
+#ifndef NDEBUG
+    for ( order = 0 ; order < 1<<STACK_ORDER ; ++order )
+        clear_page(stack_base[cpu] + order * PAGE_SIZE);
+#endif
     memguard_guard_stack(stack_base[cpu]);
 
     order = get_order_from_pages(NR_RESERVED_GDT_PAGES);
-- 
1.7.10.4

>>> On 15.08.13 at 22:49, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -13,6 +13,15 @@
>          mov     mmu_cr4_features(%rip),%rcx
>          mov     %rcx,%cr4
>  
> +#ifndef NDEBUG
> +        /* Clean stack. */
> +        mov     stack_start(%rip),%rdi
> +        mov     $(STACK_SIZE>>8),%rcx
Probably you meant STACK_SIZE / 8?
> +#ifndef NDEBUG
> +    for ( order = 0 ; order < 1<<STACK_ORDER ; ++order )
Please parenthesize the shift operation.

Jan

This reduces confusion when looking at a hexdump of the pcpu stacks and
wondering were on earth some of the junk was coming from.  Also leave some
grep fodder for finding where the BSP switches stack (because it took me far
longer to find than I care to admit to)

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

---

Changes since v1: Brackets around shift operation, and correct the calculation
for cpu0_stack, both noticed by Jan Beulich.
---
 xen/arch/x86/boot/x86_64.S |    9 +++++++++
 xen/arch/x86/smpboot.c     |    4 ++++
 2 files changed, 13 insertions(+)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index ed3888d..399541d 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -13,6 +13,15 @@
         mov     mmu_cr4_features(%rip),%rcx
         mov     %rcx,%cr4
 
+#ifndef NDEBUG
+        /* Clean stack. */
+        mov     stack_start(%rip),%rdi
+        mov     $(STACK_SIZE / 8),%rcx
+        xor     %eax,%eax
+        rep stosq
+#endif
+
+        /* Switch to cpu0_stack. */
         mov     stack_start(%rip),%rsp
         or      $(STACK_SIZE-CPUINFO_sizeof),%rsp
 
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 9f81c7b..f6b42f5 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -659,6 +659,10 @@ static int cpu_smpboot_alloc(unsigned int cpu)
     stack_base[cpu] = alloc_xenheap_pages(STACK_ORDER, 0);
     if ( stack_base[cpu] == NULL )
         goto oom;
+#ifndef NDEBUG
+    for ( order = 0 ; order < (1<<STACK_ORDER) ; ++order )
+        clear_page(stack_base[cpu] + order * PAGE_SIZE);
+#endif
     memguard_guard_stack(stack_base[cpu]);
 
     order = get_order_from_pages(NR_RESERVED_GDT_PAGES);
-- 
1.7.10.4