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

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..b8f838ae74 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -594,7 +594,7 @@ void *_xmalloc(unsigned long size, unsigned long align)
 {
     void *p = NULL;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     if ( !size )
         return ZERO_BLOCK_PTR;
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

On 05.05.2022 04:54, Henry Wang wrote:
> 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

Just two formal remarks for starters: What's this last tag? And why am
I on the To: list of this patch, when ...

> ---
>  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 this diffstat you should instead have _Cc_-ed REST maintainers?

Jan

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> > Change-Id: Ia463d2241e80e8a78d7dbb5b2318694d3ca5ed67
> 
> Just two formal remarks for starters: What's this last tag? And why am
> I on the To: list of this patch, when ...

I forgot to remove the Change-Id before sending the patch, sorry about that.
This will be removed in v2.

> 
> > ---
> >  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 this diffstat you should instead have _Cc_-ed REST maintainers?

I think this is because by switching to add-maintainers.pl I didn't change
my git send-email command line where originally it has a --suppress-cc=all.
Since you are the maintainer of common code I thought I should directly
"To" you.

Kind regards,
Henry

> 
> Jan

On 05.05.2022 09:26, Henry Wang wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>>> Change-Id: Ia463d2241e80e8a78d7dbb5b2318694d3ca5ed67
>>
>> Just two formal remarks for starters: What's this last tag? And why am
>> I on the To: list of this patch, when ...
> 
> I forgot to remove the Change-Id before sending the patch, sorry about that.
> This will be removed in v2.
> 
>>
>>> ---
>>>  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 this diffstat you should instead have _Cc_-ed REST maintainers?
> 
> I think this is because by switching to add-maintainers.pl I didn't change
> my git send-email command line where originally it has a --suppress-cc=all.
> Since you are the maintainer of common code I thought I should directly
> "To" you.

I'm sorry, but no: For one I am not "the" maintainer, but one of several.
And then patches are generally only sent _To_ the list, with _Cc_ to
maintainers.

Jan