[PATCH v3 0/2] Adjustment after introducing ASSERT_ALLOC_CONTEXT

[PATCH v3 0/2] Adjustment after introducing ASSERT_ALLOC_CONTEXT

[Henry Wang]

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

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

[PATCH v3 1/2] xen/arm: Defer request_irq on secondary CPUs after local_irq_enable

[Henry Wang]

With the enhanced ASSERT_ALLOC_CONTEXT, calling request_irq before
local_irq_enable on secondary cores 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.

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 7bfd0a73a7..9bb32a301a 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -361,9 +361,6 @@ void start_secondary(void)
 
     init_secondary_IRQ();
 
-    init_maintenance_interrupt();
-    init_timer_interrupt();
-
     set_current(idle_vcpu[cpuid]);
 
     setup_cpu_sibling_map(cpuid);
@@ -380,6 +377,15 @@ 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

[PATCH v3 2/2] xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in xmalloc()

[Henry Wang]

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

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 e866e0d864..ea59cd1a4a 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -162,13 +162,6 @@
 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 d2ad909502..75bdf18c4e 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -378,6 +378,8 @@ 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
@@ -456,6 +458,8 @@ 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;
 
@@ -594,7 +598,7 @@ void *_xmalloc(unsigned long size, unsigned long align)
 {
     void *p = NULL;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     if ( !size )
         return ZERO_BLOCK_PTR;
@@ -697,11 +701,11 @@ 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 d8beadd16b..300625e56d 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -10,6 +10,13 @@
 #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

Re: [PATCH v3 1/2] xen/arm: Defer request_irq on secondary CPUs after local_irq_enable

[Julien Grall]

Hi Henry,

On 07/05/2022 03:54, Henry Wang wrote:
> With the enhanced ASSERT_ALLOC_CONTEXT, calling request_irq before
> local_irq_enable on secondary cores 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.
> 
> 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>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 2/2] xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in xmalloc()

[Julien Grall]

Hi,

On 07/05/2022 03:54, Henry Wang wrote:
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index e866e0d864..ea59cd1a4a 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -162,13 +162,6 @@
>   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))
> -
FYI, the patch introducing ASSERT_ALLOC_CONTEXT() has been reverted. I 
intend to re-introduce it once your previous patch and the one fixing 
the ITS (not yet formally sent) have been committed.

I have also checked that none of the ASSERTs() would be triggered on my 
x86 setup. So:

Tested-by: Julien Grall <jgrall@amazon.com>
Acked-by: Julien Grall <jgrall@amazon.com>

On a side note (no action expected for you), I noticed that the 
ASSERT()s would only trigger from CPU2 and onwards at least for Arm. 
This is because num_online_cpus() would still be 1 when bringing-up CPU1.

I went through the original discussion and I am not sure why we switched 
from < SYS_STATE_smp_boot to num_online_cpus() (aside that Arm doesn't 
set it). Aynway, this is not a major issue here as this is an ASSERT().

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/2] xen/arm: Defer request_irq on secondary CPUs after local_irq_enable

[Julien Grall]

On 15/05/2022 11:58, Julien Grall wrote:
> On 07/05/2022 03:54, Henry Wang wrote:
>> With the enhanced ASSERT_ALLOC_CONTEXT, calling request_irq before
>> local_irq_enable on secondary cores 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.
>>
>> 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>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

I have committed this patch. The second patch will go in once 
"page_alloc: assert IRQs are enabled in heap alloc/free" has been 
re-committed.

Cheers,

-- 
Julien Grall

RE: [PATCH v3 2/2] xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in xmalloc()

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v3 2/2] xen/common: Use enhanced
> ASSERT_ALLOC_CONTEXT in xmalloc()
> 
> Hi,
> 
> On 07/05/2022 03:54, Henry Wang wrote:
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index e866e0d864..ea59cd1a4a 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -162,13 +162,6 @@
> >   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))
> > -
> FYI, the patch introducing ASSERT_ALLOC_CONTEXT() has been reverted. I
> intend to re-introduce it once your previous patch and the one fixing
> the ITS (not yet formally sent) have been committed.

Thanks for the information! IIUC the patch:
"xen/arm: gic-v3-lpi: Allocate the pending table while preparing the CPU"
is merged. So I guess both "page_alloc: assert IRQs are enabled in heap alloc/free"
and this patch can be re-introduced if everyone is happy with the patch?

> 
> I have also checked that none of the ASSERTs() would be triggered on my
> x86 setup. So:
> 
> Tested-by: Julien Grall <jgrall@amazon.com>
> Acked-by: Julien Grall <jgrall@amazon.com>

Thank you very much for both testing and acking this patch!

Kind regards,
Henry

> 
> On a side note (no action expected for you), I noticed that the
> ASSERT()s would only trigger from CPU2 and onwards at least for Arm.
> This is because num_online_cpus() would still be 1 when bringing-up CPU1.
> 
> I went through the original discussion and I am not sure why we switched
> from < SYS_STATE_smp_boot to num_online_cpus() (aside that Arm
> doesn't
> set it). Aynway, this is not a major issue here as this is an ASSERT().
> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v3 2/2] xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in xmalloc()

[Julien Grall]

Hi Henry,

On 24/05/2022 02:53, Henry Wang wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH v3 2/2] xen/common: Use enhanced
>> ASSERT_ALLOC_CONTEXT in xmalloc()
>>
>> Hi,
>>
>> On 07/05/2022 03:54, Henry Wang wrote:
>>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>>> index e866e0d864..ea59cd1a4a 100644
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -162,13 +162,6 @@
>>>    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))
>>> -
>> FYI, the patch introducing ASSERT_ALLOC_CONTEXT() has been reverted. I
>> intend to re-introduce it once your previous patch and the one fixing
>> the ITS (not yet formally sent) have been committed.
> 
> Thanks for the information! IIUC the patch:
> "xen/arm: gic-v3-lpi: Allocate the pending table while preparing the CPU"
> is merged. So I guess both "page_alloc: assert IRQs are enabled in heap alloc/free"
> and this patch can be re-introduced if everyone is happy with the patch?

I have re-committed David's patch and committed yours.

Cheers,

-- 
Julien Grall