1. Patchew
  2. Xen
  3. View series

[PATCH v3] page_alloc: assert IRQs are enabled in heap alloc/free

David Vrabel posted 1 patch 2 years ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220422153601.967318-1-dvrabel@cantab.net
There is a newer version of this series
xen/common/page_alloc.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
[PATCH v3] page_alloc: assert IRQs are enabled in heap alloc/free
Posted by David Vrabel 2 years ago 
From: David Vrabel <dvrabel@amazon.co.uk>

Heap pages can only be safely allocated and freed with interuupts
enabled as they may require a TLB flush which will send IPIs.

Normally spinlock debugging would catch calls from the incorrect
context, but not from stop_machine_run() action functions as these are
called with spin lock debugging disabled.

Enhance the assertions in alloc_xenheap_pages() and
alloc_domheap_pages() to check interrupts are enabled. For consistency
the same asserts are used when freeing heap pages.

As an exception, when only 1 PCPU is online, allocations are permitted
with interrupts disabled as any TLB flushes would be local only. This
is necessary during early boot.

Signed-off-by: David Vrabel <dvrabel@amazon.co.uk>
---
Changes in v3:
- Use num_online_cpus() in assert.

Changes in v2:
- Set SYS_STATE_smp_boot on arm.
---
 xen/common/page_alloc.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 319029140f..516ffa2a97 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -162,6 +162,13 @@
 static char __initdata opt_badpage[100] = "";
 string_param("badpage", opt_badpage);
 
+/*
+ * Heap allocations may need TLB flushes which require IRQs to be
+ * enabled (except during early boot 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.
  */
@@ -2160,7 +2167,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
 {
     struct page_info *pg;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     pg = alloc_heap_pages(MEMZONE_XEN, MEMZONE_XEN,
                           order, memflags | MEMF_no_scrub, NULL);
@@ -2173,7 +2180,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
 
 void free_xenheap_pages(void *v, unsigned int order)
 {
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     if ( v == NULL )
         return;
@@ -2202,7 +2209,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
     struct page_info *pg;
     unsigned int i;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     if ( xenheap_bits && (memflags >> _MEMF_bits) > xenheap_bits )
         memflags &= ~MEMF_bits(~0U);
@@ -2224,7 +2231,7 @@ void free_xenheap_pages(void *v, unsigned int order)
     struct page_info *pg;
     unsigned int i;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     if ( v == NULL )
         return;
@@ -2249,7 +2256,7 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
 {
     mfn_t smfn, emfn;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     smfn = maddr_to_mfn(round_pgup(ps));
     emfn = maddr_to_mfn(round_pgdown(pe));
@@ -2369,7 +2376,7 @@ struct page_info *alloc_domheap_pages(
     unsigned int bits = memflags >> _MEMF_bits, zone_hi = NR_ZONES - 1;
     unsigned int dma_zone;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     bits = domain_clamp_alloc_bitsize(memflags & MEMF_no_owner ? NULL : d,
                                       bits ? : (BITS_PER_LONG+PAGE_SHIFT));
@@ -2419,7 +2426,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
     unsigned int i;
     bool drop_dom_ref;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     if ( unlikely(is_xen_heap_page(pg)) )
     {
@@ -2738,7 +2745,7 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
 {
     struct page_info *pg;
 
-    ASSERT(!in_irq());
+    ASSERT_ALLOC_CONTEXT();
 
     pg = acquire_staticmem_pages(smfn, nr_mfns, memflags);
     if ( !pg )
-- 
2.30.2
Re: [PATCH v3] page_alloc: assert IRQs are enabled in heap alloc/free
Posted by Julien Grall 2 years ago 
Hi David,

On 22/04/2022 16:36, David Vrabel wrote:
> From: David Vrabel <dvrabel@amazon.co.uk>
> 
> Heap pages can only be safely allocated and freed with interuupts

typo: s/interuupts/interrupts/

> enabled as they may require a TLB flush which will send IPIs.

We don't have such requirements on Arm. Given this is common code, I 
think we should write "which may send IPIs on some architectures (such 
as x86).

That said, I think the change is still a good move on Arm because I 
don't think it is sane to do allocation with interrupts disabled.

> 
> Normally spinlock debugging would catch calls from the incorrect
> context, but not from stop_machine_run() action functions as these are
> called with spin lock debugging disabled.
> 
> Enhance the assertions in alloc_xenheap_pages() and
> alloc_domheap_pages() to check interrupts are enabled. For consistency
> the same asserts are used when freeing heap pages.
> 
> As an exception, when only 1 PCPU is online, allocations are permitted
> with interrupts disabled as any TLB flushes would be local only. This
> is necessary during early boot.
> 
> Signed-off-by: David Vrabel <dvrabel@amazon.co.uk>
> ---
> Changes in v3:
> - Use num_online_cpus() in assert.
> 
> Changes in v2:
> - Set SYS_STATE_smp_boot on arm.
> ---
>   xen/common/page_alloc.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 319029140f..516ffa2a97 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -162,6 +162,13 @@
>   static char __initdata opt_badpage[100] = "";
>   string_param("badpage", opt_badpage);
>   
> +/*
> + * Heap allocations may need TLB flushes which require IRQs to be
> + * enabled (except during early boot when only 1 PCPU is online).

Same remark as above. Also, I think there are other cases where 
num_online_cpus() == 1:
   - Xen is only using one core (it will not be a useful system but 
technically supported)
   - During suspend/resume

So I think we should either relax the comment or restrict the assert 
below. I don't have any preference.

> + */ > +#define ASSERT_ALLOC_CONTEXT() \
> +    ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() == 1))
> +
>   /*
>    * no-bootscrub -> Free pages are not zeroed during boot.
>    */
> @@ -2160,7 +2167,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>   {
>       struct page_info *pg;
>   
> -    ASSERT(!in_irq());
> +    ASSERT_ALLOC_CONTEXT();
>   
>       pg = alloc_heap_pages(MEMZONE_XEN, MEMZONE_XEN,
>                             order, memflags | MEMF_no_scrub, NULL);
> @@ -2173,7 +2180,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>   
>   void free_xenheap_pages(void *v, unsigned int order)
>   {
> -    ASSERT(!in_irq());
> +    ASSERT_ALLOC_CONTEXT();
>   
>       if ( v == NULL )
>           return;
> @@ -2202,7 +2209,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>       struct page_info *pg;
>       unsigned int i;
>   
> -    ASSERT(!in_irq());
> +    ASSERT_ALLOC_CONTEXT();
>   
>       if ( xenheap_bits && (memflags >> _MEMF_bits) > xenheap_bits )
>           memflags &= ~MEMF_bits(~0U);
> @@ -2224,7 +2231,7 @@ void free_xenheap_pages(void *v, unsigned int order)
>       struct page_info *pg;
>       unsigned int i;
>   
> -    ASSERT(!in_irq());
> +    ASSERT_ALLOC_CONTEXT();
>   
>       if ( v == NULL )
>           return;
> @@ -2249,7 +2256,7 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>   {
>       mfn_t smfn, emfn;
>   
> -    ASSERT(!in_irq());
> +    ASSERT_ALLOC_CONTEXT();
>   
>       smfn = maddr_to_mfn(round_pgup(ps));
>       emfn = maddr_to_mfn(round_pgdown(pe));
> @@ -2369,7 +2376,7 @@ struct page_info *alloc_domheap_pages(
>       unsigned int bits = memflags >> _MEMF_bits, zone_hi = NR_ZONES - 1;
>       unsigned int dma_zone;
>   
> -    ASSERT(!in_irq());
> +    ASSERT_ALLOC_CONTEXT();
>   
>       bits = domain_clamp_alloc_bitsize(memflags & MEMF_no_owner ? NULL : d,
>                                         bits ? : (BITS_PER_LONG+PAGE_SHIFT));
> @@ -2419,7 +2426,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>       unsigned int i;
>       bool drop_dom_ref;
>   
> -    ASSERT(!in_irq());
> +    ASSERT_ALLOC_CONTEXT();
>   
>       if ( unlikely(is_xen_heap_page(pg)) )
>       {
> @@ -2738,7 +2745,7 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>   {
>       struct page_info *pg;
>   
> -    ASSERT(!in_irq());
> +    ASSERT_ALLOC_CONTEXT();
>   
>       pg = acquire_staticmem_pages(smfn, nr_mfns, memflags);
>       if ( !pg )

Cheers,

-- 
Julien Grall
Re: [PATCH v3] page_alloc: assert IRQs are enabled in heap alloc/free
Posted by Jan Beulich 2 years ago 
On 24.04.2022 17:52, Julien Grall wrote:
>> Changes in v3:
>> - Use num_online_cpus() in assert.

With this ...

>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -162,6 +162,13 @@
>>   static char __initdata opt_badpage[100] = "";
>>   string_param("badpage", opt_badpage);
>>   
>> +/*
>> + * Heap allocations may need TLB flushes which require IRQs to be
>> + * enabled (except during early boot when only 1 PCPU is online).
> 
> Same remark as above. Also, I think there are other cases where 
> num_online_cpus() == 1:
>    - Xen is only using one core (it will not be a useful system but 
> technically supported)
>    - During suspend/resume
> 
> So I think we should either relax the comment or restrict the assert 
> below. I don't have any preference.

... I think it is the comment which wants bringing back in sync.

> + */ > +#define ASSERT_ALLOC_CONTEXT() \
> +    ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() == 1))

While by the time calls here can legitimately occur the online map
should be initialized, I wonder whether it wouldn't be better to use
"<= 1" here nevertheless.

Jan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.