| Message ID | 20220419150134.64704-1-dvrabel@cantab.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] page_alloc: assert IRQs are enabled in heap alloc/free | expand |
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
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
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 >
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 )