Series comparison

With the enhanced ASSERT_ALLOC_CONTEXT, calling request_irq before
local_irq_enable on secondary cores on Arm will lead to

(XEN) Xen call trace:
(XEN) [<000000000021d86c>] alloc_xenheap_pages+0x74/0x194 (PC)
(XEN) [<000000000021d864>] alloc_xenheap_pages+0x6c/0x194 (LR)
(XEN) [<0000000000229e90>] xmalloc_tlsf.c#xmalloc_pool_get+0x1c/0x28
(XEN) [<000000000022a270>] xmem_pool_alloc+0x21c/0x448
(XEN) [<000000000022a8dc>] _xmalloc+0x8c/0x290
(XEN) [<000000000026b57c>] request_irq+0x40/0xb8
(XEN) [<0000000000272780>] init_timer_interrupt+0x74/0xcc
(XEN) [<000000000027212c>] start_secondary+0x1b4/0x238
(XEN) [<0000000084000200>] 0000000084000200
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 4:
(XEN) Assertion '!in_irq() && (local_irq_is_enabled() ||
num_online_cpus() <= 1)' failed at common/page_alloc.c:2212
(XEN) ****************************************

on systems without a big enough pool for xmalloc() to cater the
requested size. To solve this issue, this series introduces two
patches. The first one defers the calling of request_irq on
secondary CPUs after local_irq_enable on Arm. The second one
moves the definition of ASSERT_ALLOC_CONTEXT to header and uses
the ASSERT_ALLOC_CONTEXT to replace the original assertion in
xmalloc().

Henry Wang (2):
  xen/arm: Defer request_irq on secondary CPUs after local_irq_enable
  xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in xmalloc()

xen/arch/arm/smpboot.c    | 7 ++++---
 xen/common/page_alloc.c   | 7 -------
 xen/common/xmalloc_tlsf.c | 2 +-
 xen/include/xen/irq.h     | 7 +++++++
 4 files changed, 12 insertions(+), 11 deletions(-)

-- 
2.25.1

With the enhanced ASSERT_ALLOC_CONTEXT, calling request_irq before
local_irq_enable on secondary cores will lead to

on systems without a big enough pool for xmalloc() to cater the
requested size.

Reported-by: Wei Chen <Wei.Chen@arm.com>
Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Change-Id: Iebdde81f52785b0c6e037c981ff68922db016d08
---
 xen/arch/arm/smpboot.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index XXXXXXX..XXXXXXX 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -XXX,XX +XXX,XX @@ void start_secondary(void)
 
     init_secondary_IRQ();
 
-    init_maintenance_interrupt();
-    init_timer_interrupt();
-
     set_current(idle_vcpu[cpuid]);
 
     setup_cpu_sibling_map(cpuid);
@@ -XXX,XX +XXX,XX @@ void start_secondary(void)
     cpumask_set_cpu(cpuid, &cpu_online_map);
 
     local_irq_enable();
+
+    init_maintenance_interrupt();
+    init_timer_interrupt();
+
     local_abort_enable();
 
     check_local_cpu_errata();
-- 
2.25.1

xmalloc() will use a pool for allocation smaller than a page.
The pool is extended only when there are no more space. At which
point, alloc_xenheap_pages() is called to add more memory.

xmalloc() must be protected by ASSERT_ALLOC_CONTEXT. It should not
rely on pool expanding to trigger the ASSERT_ALLOC_CONTEXT in
alloc_xenheap_pages(). Hence, this commit moves the definition of
ASSERT_ALLOC_CONTEXT to header and uses the ASSERT_ALLOC_CONTEXT
to replace the original assertion in xmalloc().

Reported-by: Wei Chen <Wei.Chen@arm.com>
Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Change-Id: Ia463d2241e80e8a78d7dbb5b2318694d3ca5ed67
---
 xen/common/page_alloc.c   | 7 -------
 xen/common/xmalloc_tlsf.c | 2 +-
 xen/include/xen/irq.h     | 7 +++++++
 3 files changed, 8 insertions(+), 8 deletions(-)

With the enhanced ASSERT_ALLOC_CONTEXT, calling request_irq before
local_irq_enable on secondary cores on Arm will lead to

Also take the chance to enhance the assertion in xmalloc_tlsf.c.

Henry Wang (2):
  xen/arm: Defer request_irq on secondary CPUs after local_irq_enable
  xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in xmalloc()

xen/arch/arm/smpboot.c    | 12 +++++++++---
 xen/common/page_alloc.c   |  7 -------
 xen/common/xmalloc_tlsf.c | 10 +++++++---
 xen/include/xen/irq.h     |  7 +++++++
 4 files changed, 23 insertions(+), 13 deletions(-)

-- 
2.25.1

With the enhanced ASSERT_ALLOC_CONTEXT, calling request_irq before
local_irq_enable on secondary cores will lead to

on systems without a big enough pool for xmalloc() to cater the
requested size.

Moving the call of request_irq() past local_irq_enable() on
secondary cores will make sure the assertion condition in
alloc_xenheap_pages(), i.e. !in_irq && local_irq_enabled() is
satisfied. It is also safe because the timer and GIC maintenance
interrupt will not be used until the CPU is fully online.

Reported-by: Wei Chen <Wei.Chen@arm.com>
Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
v2 -> v3:
- No changes.
v1 -> v2:
- Explain why the moving of code is safe in the commit message and
add comments.
---
 xen/arch/arm/smpboot.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index XXXXXXX..XXXXXXX 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -XXX,XX +XXX,XX @@ void start_secondary(void)
 
     init_secondary_IRQ();
 
-    init_maintenance_interrupt();
-    init_timer_interrupt();
-
     set_current(idle_vcpu[cpuid]);
 
     setup_cpu_sibling_map(cpuid);
@@ -XXX,XX +XXX,XX @@ void start_secondary(void)
     cpumask_set_cpu(cpuid, &cpu_online_map);
 
     local_irq_enable();
+
+    /*
+     * Calling request_irq() after local_irq_enable() on secondary cores
+     * will make sure the assertion condition in alloc_xenheap_pages(),
+     * i.e. !in_irq && local_irq_enabled() is satisfied.
+     */
+    init_maintenance_interrupt();
+    init_timer_interrupt();
+
     local_abort_enable();
 
     check_local_cpu_errata();
-- 
2.25.1

xmalloc() will use a pool for allocation smaller than a page.
The pool is extended only when there are no more space. At which
point, alloc_xenheap_pages() is called to add more memory.

For consistency, the same assertion should be used in xfree(),
and the position of the assertion should be at the beginning of
the xfree().

Also take the opportunity to enhance the non-static functions
xmem_pool_{alloc,free}() with the same assertion so that future
callers of these two functions can be benefited.

Reported-by: Wei Chen <Wei.Chen@arm.com>
Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
v2 -> v3:
- Add ASSERT_ALLOC_CONTEXT in xmem_pool_{alloc,free}()
- Also change the assertion in xfree to ASSERT_ALLOC_CONTEXT and
move the position of assertion to the beginning of the function.
v1 -> v2:
- No changes
---
 xen/common/page_alloc.c   |  7 -------
 xen/common/xmalloc_tlsf.c | 10 +++++++---
 xen/include/xen/irq.h     |  7 +++++++
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index XXXXXXX..XXXXXXX 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -XXX,XX +XXX,XX @@
 static char __initdata opt_badpage[100] = "";
 string_param("badpage", opt_badpage);
 
-/*
- * Heap allocations may need TLB flushes which may require IRQs to be
- * enabled (except when only 1 PCPU is online).
- */
-#define ASSERT_ALLOC_CONTEXT() \
-    ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1))
-
 /*
  * no-bootscrub -> Free pages are not zeroed during boot.
  */
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index XXXXXXX..XXXXXXX 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -XXX,XX +XXX,XX @@ void *xmem_pool_alloc(unsigned long size, struct xmem_pool *pool)
     int fl, sl;
     unsigned long tmp_size;
 
+    ASSERT_ALLOC_CONTEXT();
+
     if ( size < MIN_BLOCK_SIZE )
         size = MIN_BLOCK_SIZE;
     else
@@ -XXX,XX +XXX,XX @@ void xmem_pool_free(void *ptr, struct xmem_pool *pool)
     struct bhdr *b, *tmp_b;
     int fl = 0, sl = 0;
 
+    ASSERT_ALLOC_CONTEXT();
+
     if ( unlikely(ptr == NULL) )
         return;
 
@@ -XXX,XX +XXX,XX @@ void *_xmalloc(unsigned long size, unsigned long align)
 {
     void *p = NULL;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     if ( !size )
         return ZERO_BLOCK_PTR;
@@ -XXX,XX +XXX,XX @@ void *_xrealloc(void *ptr, unsigned long size, unsigned long align)
 
 void xfree(void *p)
 {
+    ASSERT_ALLOC_CONTEXT();
+
     if ( p == NULL || p == ZERO_BLOCK_PTR )
         return;
 
-    ASSERT(!in_irq());
-
     if ( !((unsigned long)p & (PAGE_SIZE - 1)) )
     {
         unsigned long size = PFN_ORDER(virt_to_page(p));
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index XXXXXXX..XXXXXXX 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -XXX,XX +XXX,XX @@
 #include <asm/hardirq.h>
 #include <public/event_channel.h>
 
+/*
+ * Heap allocations may need TLB flushes which may require IRQs to be
+ * enabled (except when only 1 PCPU is online).
+ */
+#define ASSERT_ALLOC_CONTEXT() \
+    ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1))
+
 struct irqaction {
     void (*handler)(int, void *, struct cpu_user_regs *);
     const char *name;
-- 
2.25.1