* [PATCH v1] page_alloc: assert IRQs are enabled in heap alloc/free
@ 2022-04-19 15:01 David Vrabel
2022-04-20 6:26 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: David Vrabel @ 2022-04-19 15:01 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall,
Stefano Stabellini, Wei Liu, David Vrabel
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.
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, during early boot when only 1 PCPU is online,
allocations are permitted with interrupts disabled.
Signed-off-by: David Vrabel <dvrabel@amazon.co.uk>
---
xen/common/page_alloc.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 319029140f..e1ce38df13 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -162,6 +162,14 @@
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() \
+ || system_state < SYS_STATE_smp_boot))
+
/*
* no-bootscrub -> Free pages are not zeroed during boot.
*/
@@ -2160,7 +2168,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 +2181,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 +2210,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 +2232,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 +2257,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 +2377,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 +2427,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 +2746,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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] page_alloc: assert IRQs are enabled in heap alloc/free
2022-04-19 15:01 [PATCH v1] page_alloc: assert IRQs are enabled in heap alloc/free David Vrabel
@ 2022-04-20 6:26 ` Jan Beulich
2022-04-20 8:13 ` David Vrabel
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2022-04-20 6:26 UTC (permalink / raw)
To: David Vrabel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, David Vrabel, xen-devel
On 19.04.2022 17:01, David Vrabel wrote:
> 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.
>
> 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, during early boot when only 1 PCPU is online,
> allocations are permitted with interrupts disabled.
This exception is tightly coupled with spin lock checking, i.e. the
point in time when spin_debug_enable() is called. I think this wants
making explicit at least in the code comment, but as a result I also
wonder in how far the extended assertions are really worthwhile: Any
violation would be detected in check_lock() anyway. Thoughts?
Furthermore I'm concerned of Arm not using either SYS_STATE_smp_boot
or spin_debug_enable().
Jan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] page_alloc: assert IRQs are enabled in heap alloc/free
2022-04-20 6:26 ` Jan Beulich
@ 2022-04-20 8:13 ` David Vrabel
2022-04-20 8:44 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: David Vrabel @ 2022-04-20 8:13 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, David Vrabel, xen-devel
On 20/04/2022 07:26, Jan Beulich wrote:
> On 19.04.2022 17:01, David Vrabel wrote:
>> 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.
>>
>> 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, during early boot when only 1 PCPU is online,
>> allocations are permitted with interrupts disabled.
>
> This exception is tightly coupled with spin lock checking, i.e. the
> point in time when spin_debug_enable() is called. I think this wants
> making explicit at least in the code comment, but as a result I also
> wonder in how far the extended assertions are really worthwhile: Any
> violation would be detected in check_lock() anyway. Thoughts?
I was caught out by stop_machine_run() disabling both interrupts and
spin lock debugging when running the action function, so check_lock()
didn't help in this (admittedly) narrow use case.
> Furthermore I'm concerned of Arm not using either SYS_STATE_smp_boot
> or spin_debug_enable().
David
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] page_alloc: assert IRQs are enabled in heap alloc/free
2022-04-20 8:13 ` David Vrabel
@ 2022-04-20 8:44 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2022-04-20 8:44 UTC (permalink / raw)
To: David Vrabel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, David Vrabel, xen-devel
On 20.04.2022 10:13, David Vrabel wrote:
>
>
> On 20/04/2022 07:26, Jan Beulich wrote:
>> On 19.04.2022 17:01, David Vrabel wrote:
>>> 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.
>>>
>>> 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, during early boot when only 1 PCPU is online,
>>> allocations are permitted with interrupts disabled.
>>
>> This exception is tightly coupled with spin lock checking, i.e. the
>> point in time when spin_debug_enable() is called. I think this wants
>> making explicit at least in the code comment, but as a result I also
>> wonder in how far the extended assertions are really worthwhile: Any
>> violation would be detected in check_lock() anyway. Thoughts?
>
> I was caught out by stop_machine_run() disabling both interrupts and
> spin lock debugging when running the action function, so check_lock()
> didn't help in this (admittedly) narrow use case.
Oh, I see - fair point.
Jan
>> Furthermore I'm concerned of Arm not using either SYS_STATE_smp_boot
>> or spin_debug_enable().
>
> David
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-20 8:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-19 15:01 [PATCH v1] page_alloc: assert IRQs are enabled in heap alloc/free David Vrabel
2022-04-20 6:26 ` Jan Beulich
2022-04-20 8:13 ` David Vrabel
2022-04-20 8:44 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.