| Message ID | 20220516143116.28602-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/flushtlb: remove flush_area check on system state | expand |
On 16/05/2022 15:31, Roger Pau Monne wrote: > Booting with Shadow Stacks leads to the following assert on a debug > hypervisor: > > (XEN) [ 11.625166] Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265 > (XEN) [ 11.629410] ----[ Xen-4.17.0-10.24-d x86_64 debug=y Not tainted ]---- > (XEN) [ 11.633679] CPU: 0 > (XEN) [ 11.637834] RIP: e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e > [...] > (XEN) [ 11.806158] Xen call trace: > (XEN) [ 11.811255] [<ffff82d040345300>] R flush_area_mask+0x40/0x13e > (XEN) [ 11.816459] [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958 > (XEN) [ 11.821689] [<ffff82d0404474f9>] F arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9 > (XEN) [ 11.827053] [<ffff82d0404476cc>] F alternative_branches+0xf/0x12 > (XEN) [ 11.832416] [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776 > (XEN) [ 11.837809] [<ffff82d040203344>] F __high_start+0x94/0xa0 > > > This is due to SYS_STATE_smp_boot being set before calling > alternative_branches(), and the flush in modify_xen_mappings() then > using flush_area_all() with interrupts disabled. Note that > alternative_branches() is called before APs are started, so the flush > must be a local one (and indeed the cpumask passed to > flush_area_mask() just contains one CPU). > > Take the opportunity to simplify a bit the logic and make flush_area() > an alias for flush_area_mask(&cpu_online_map...), taking into account > that cpu_online_map just contains the BSP before APs are started. > This requires widening the assert in flush_area_mask() to allow > being called with interrupts disabled as long as it's strictly a local > only flush. > > The overall result is that a conditional can be removed from > flush_area(). > > Fixes: (78e072bc37 'x86/mm: avoid inadvertently degrading a TLB flush to local only') > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Tentatively Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> This seems like the least bad option of a lot of bad options. I'd say it's more than just removing a conditional from flush_area(); it's removing a runtime special case for init-time code. ~Andrew
On 16.05.2022 16:31, Roger Pau Monne wrote: > --- a/xen/arch/x86/include/asm/flushtlb.h > +++ b/xen/arch/x86/include/asm/flushtlb.h > @@ -146,7 +146,8 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags); > #define flush_mask(mask, flags) flush_area_mask(mask, NULL, flags) > > /* Flush all CPUs' TLBs/caches */ > -#define flush_area_all(va, flags) flush_area_mask(&cpu_online_map, va, flags) > +#define flush_area(va, flags) \ > + flush_area_mask(&cpu_online_map, (const void *)(va), flags) I have to admit that I would prefer if we kept the "_all" name suffix, to continue to clearly express the scope of the flush. I'm also not really happy to see the cast being added globally now. > --- a/xen/arch/x86/smp.c > +++ b/xen/arch/x86/smp.c > @@ -262,7 +262,8 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags) > { > unsigned int cpu = smp_processor_id(); > > - ASSERT(local_irq_is_enabled()); > + /* Local flushes can be performed with interrupts disabled. */ > + ASSERT(local_irq_is_enabled() || cpumask_equal(mask, cpumask_of(cpu))); Further down we use cpumask_subset(mask, cpumask_of(cpu)), apparently to also cover the case where mask is empty. I think you want to do so here as well. > if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) && > cpumask_test_cpu(cpu, mask) ) I suppose we want a further precaution here: Despite the !cpumask_subset(mask, cpumask_of(cpu)) below I think we want to extend what c64bf2d2a625 ("x86: make CPU state flush requests explicit") and later changes (isolating uses of FLUSH_VCPU_STATE from other FLUSH_*) did and exclude the use of FLUSH_VCPU_STATE for the local CPU altogether. That's because if such somehow made it into the conditional below here, it would still involve an IPI. E.g. ASSERT(local_irq_is_enabled() || (cpumask_subset(mask, cpumask_of(cpu)) && !(flags & FLUSH_VCPU_STATE))); Jan
On Wed, May 18, 2022 at 10:49:22AM +0200, Jan Beulich wrote: > On 16.05.2022 16:31, Roger Pau Monne wrote: > > --- a/xen/arch/x86/include/asm/flushtlb.h > > +++ b/xen/arch/x86/include/asm/flushtlb.h > > @@ -146,7 +146,8 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags); > > #define flush_mask(mask, flags) flush_area_mask(mask, NULL, flags) > > > > /* Flush all CPUs' TLBs/caches */ > > -#define flush_area_all(va, flags) flush_area_mask(&cpu_online_map, va, flags) > > +#define flush_area(va, flags) \ > > + flush_area_mask(&cpu_online_map, (const void *)(va), flags) > > I have to admit that I would prefer if we kept the "_all" name suffix, > to continue to clearly express the scope of the flush. I'm also not > really happy to see the cast being added globally now. But there where no direct callers of flush_area_all(), so the name was just relevant for it's use in flush_area(). With that now gone I don't see a need for a flush_area_all(), as flush_area_mask() is more appropriate. > > --- a/xen/arch/x86/smp.c > > +++ b/xen/arch/x86/smp.c > > @@ -262,7 +262,8 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags) > > { > > unsigned int cpu = smp_processor_id(); > > > > - ASSERT(local_irq_is_enabled()); > > + /* Local flushes can be performed with interrupts disabled. */ > > + ASSERT(local_irq_is_enabled() || cpumask_equal(mask, cpumask_of(cpu))); > > Further down we use cpumask_subset(mask, cpumask_of(cpu)), > apparently to also cover the case where mask is empty. I think > you want to do so here as well. Hm, yes. I guess that's cheaper than adding an extra: if ( cpumask_empty() ) return; check at the start of the function. > > if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) && > > cpumask_test_cpu(cpu, mask) ) > > I suppose we want a further precaution here: Despite the > !cpumask_subset(mask, cpumask_of(cpu)) below I think we want to > extend what c64bf2d2a625 ("x86: make CPU state flush requests > explicit") and later changes (isolating uses of FLUSH_VCPU_STATE > from other FLUSH_*) did and exclude the use of FLUSH_VCPU_STATE > for the local CPU altogether. If we really want to exclude the use of FLUSH_VCPU_STATE for the local CPU, we might wish to add this as a separate ASSERT, so that such checking doesn't depend on !local_irq_is_enabled(): ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)); ASSERT(!cpumask_subset(mask, cpumask_of(cpu)) || !(flags & FLUSH_VCPU_STATE)); > That's because if such somehow made > it into the conditional below here, it would still involve an IPI. Sorry, I'm confused by this: if the mask is empty there should be no IPI involved at all? And we shouldn't even get into the second conditional on the function. Thanks, Roger.
On 23.05.2022 16:37, Roger Pau Monné wrote: > On Wed, May 18, 2022 at 10:49:22AM +0200, Jan Beulich wrote: >> On 16.05.2022 16:31, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/include/asm/flushtlb.h >>> +++ b/xen/arch/x86/include/asm/flushtlb.h >>> @@ -146,7 +146,8 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags); >>> #define flush_mask(mask, flags) flush_area_mask(mask, NULL, flags) >>> >>> /* Flush all CPUs' TLBs/caches */ >>> -#define flush_area_all(va, flags) flush_area_mask(&cpu_online_map, va, flags) >>> +#define flush_area(va, flags) \ >>> + flush_area_mask(&cpu_online_map, (const void *)(va), flags) >> >> I have to admit that I would prefer if we kept the "_all" name suffix, >> to continue to clearly express the scope of the flush. I'm also not >> really happy to see the cast being added globally now. > > But there where no direct callers of flush_area_all(), so the name was > just relevant for it's use in flush_area(). With that now gone I > don't see a need for a flush_area_all(), as flush_area_mask() is more > appropriate. And flush_area_all() is shorthand for flush_area_mask(&cpu_online_map, ...). That's more clearly distinguished from flush_area_local() than simply flush_area(); the latter was okay-ish with its mm.c-only exposure, but imo isn't anymore when put in a header. >>> --- a/xen/arch/x86/smp.c >>> +++ b/xen/arch/x86/smp.c >>> @@ -262,7 +262,8 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags) >>> { >>> unsigned int cpu = smp_processor_id(); >>> >>> - ASSERT(local_irq_is_enabled()); >>> + /* Local flushes can be performed with interrupts disabled. */ >>> + ASSERT(local_irq_is_enabled() || cpumask_equal(mask, cpumask_of(cpu))); >> >> Further down we use cpumask_subset(mask, cpumask_of(cpu)), >> apparently to also cover the case where mask is empty. I think >> you want to do so here as well. > > Hm, yes. I guess that's cheaper than adding an extra: > > if ( cpumask_empty() ) > return; > > check at the start of the function. > >>> if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) && >>> cpumask_test_cpu(cpu, mask) ) >> >> I suppose we want a further precaution here: Despite the >> !cpumask_subset(mask, cpumask_of(cpu)) below I think we want to >> extend what c64bf2d2a625 ("x86: make CPU state flush requests >> explicit") and later changes (isolating uses of FLUSH_VCPU_STATE >> from other FLUSH_*) did and exclude the use of FLUSH_VCPU_STATE >> for the local CPU altogether. > > If we really want to exclude the use of FLUSH_VCPU_STATE for the local > CPU, we might wish to add this as a separate ASSERT, so that such > checking doesn't depend on !local_irq_is_enabled(): > > ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)); > ASSERT(!cpumask_subset(mask, cpumask_of(cpu)) || !(flags & FLUSH_VCPU_STATE)); > > >> That's because if such somehow made >> it into the conditional below here, it would still involve an IPI. > > Sorry, I'm confused by this: if the mask is empty there should be no > IPI involved at all? And we shouldn't even get into the second > conditional on the function. Should perhaps have made more explicit that "somehow" means a hypothetical way, perhaps even as a result of some further breakage somewhere. Jan
On Mon, May 23, 2022 at 05:13:43PM +0200, Jan Beulich wrote: > On 23.05.2022 16:37, Roger Pau Monné wrote: > > On Wed, May 18, 2022 at 10:49:22AM +0200, Jan Beulich wrote: > >> On 16.05.2022 16:31, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/include/asm/flushtlb.h > >>> +++ b/xen/arch/x86/include/asm/flushtlb.h > >>> @@ -146,7 +146,8 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags); > >>> #define flush_mask(mask, flags) flush_area_mask(mask, NULL, flags) > >>> > >>> /* Flush all CPUs' TLBs/caches */ > >>> -#define flush_area_all(va, flags) flush_area_mask(&cpu_online_map, va, flags) > >>> +#define flush_area(va, flags) \ > >>> + flush_area_mask(&cpu_online_map, (const void *)(va), flags) > >> > >> I have to admit that I would prefer if we kept the "_all" name suffix, > >> to continue to clearly express the scope of the flush. I'm also not > >> really happy to see the cast being added globally now. > > > > But there where no direct callers of flush_area_all(), so the name was > > just relevant for it's use in flush_area(). With that now gone I > > don't see a need for a flush_area_all(), as flush_area_mask() is more > > appropriate. > > And flush_area_all() is shorthand for flush_area_mask(&cpu_online_map, ...). > That's more clearly distinguished from flush_area_local() than simply > flush_area(); the latter was okay-ish with its mm.c-only exposure, but imo > isn't anymore when put in a header. OK, so you would prefer to replace callers to use flush_area_all() and drop flush_area() altogether. I can do that. > >>> --- a/xen/arch/x86/smp.c > >>> +++ b/xen/arch/x86/smp.c > >>> @@ -262,7 +262,8 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags) > >>> { > >>> unsigned int cpu = smp_processor_id(); > >>> > >>> - ASSERT(local_irq_is_enabled()); > >>> + /* Local flushes can be performed with interrupts disabled. */ > >>> + ASSERT(local_irq_is_enabled() || cpumask_equal(mask, cpumask_of(cpu))); > >> > >> Further down we use cpumask_subset(mask, cpumask_of(cpu)), > >> apparently to also cover the case where mask is empty. I think > >> you want to do so here as well. > > > > Hm, yes. I guess that's cheaper than adding an extra: > > > > if ( cpumask_empty() ) > > return; > > > > check at the start of the function. > > > >>> if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) && > >>> cpumask_test_cpu(cpu, mask) ) > >> > >> I suppose we want a further precaution here: Despite the > >> !cpumask_subset(mask, cpumask_of(cpu)) below I think we want to > >> extend what c64bf2d2a625 ("x86: make CPU state flush requests > >> explicit") and later changes (isolating uses of FLUSH_VCPU_STATE > >> from other FLUSH_*) did and exclude the use of FLUSH_VCPU_STATE > >> for the local CPU altogether. > > > > If we really want to exclude the use of FLUSH_VCPU_STATE for the local > > CPU, we might wish to add this as a separate ASSERT, so that such > > checking doesn't depend on !local_irq_is_enabled(): > > > > ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)); > > ASSERT(!cpumask_subset(mask, cpumask_of(cpu)) || !(flags & FLUSH_VCPU_STATE)); > > > > > >> That's because if such somehow made > >> it into the conditional below here, it would still involve an IPI. > > > > Sorry, I'm confused by this: if the mask is empty there should be no > > IPI involved at all? And we shouldn't even get into the second > > conditional on the function. > > Should perhaps have made more explicit that "somehow" means a hypothetical > way, perhaps even as a result of some further breakage somewhere. Oh, OK, then I wasn't so confused after all :). Given your lack of comments I assume you are fine with the addition of a separate ASSERT to cover the usage of FLUSH_VCPU_STATE. Thanks, Roger.
(trying to send again, as I've replied yesterday but the email never reached xen-devel). On Mon, May 23, 2022 at 05:13:43PM +0200, Jan Beulich wrote: > On 23.05.2022 16:37, Roger Pau Monné wrote: > > On Wed, May 18, 2022 at 10:49:22AM +0200, Jan Beulich wrote: > >> On 16.05.2022 16:31, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/include/asm/flushtlb.h > >>> +++ b/xen/arch/x86/include/asm/flushtlb.h > >>> @@ -146,7 +146,8 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags); > >>> #define flush_mask(mask, flags) flush_area_mask(mask, NULL, flags) > >>> > >>> /* Flush all CPUs' TLBs/caches */ > >>> -#define flush_area_all(va, flags) flush_area_mask(&cpu_online_map, va, flags) > >>> +#define flush_area(va, flags) \ > >>> + flush_area_mask(&cpu_online_map, (const void *)(va), flags) > >> > >> I have to admit that I would prefer if we kept the "_all" name suffix, > >> to continue to clearly express the scope of the flush. I'm also not > >> really happy to see the cast being added globally now. > > > > But there where no direct callers of flush_area_all(), so the name was > > just relevant for it's use in flush_area(). With that now gone I > > don't see a need for a flush_area_all(), as flush_area_mask() is more > > appropriate. > > And flush_area_all() is shorthand for flush_area_mask(&cpu_online_map, ...). > That's more clearly distinguished from flush_area_local() than simply > flush_area(); the latter was okay-ish with its mm.c-only exposure, but imo > isn't anymore when put in a header. OK, so you would prefer to replace callers to use flush_area_all() and drop flush_area() altogether. I can do that. > >>> --- a/xen/arch/x86/smp.c > >>> +++ b/xen/arch/x86/smp.c > >>> @@ -262,7 +262,8 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags) > >>> { > >>> unsigned int cpu = smp_processor_id(); > >>> > >>> - ASSERT(local_irq_is_enabled()); > >>> + /* Local flushes can be performed with interrupts disabled. */ > >>> + ASSERT(local_irq_is_enabled() || cpumask_equal(mask, cpumask_of(cpu))); > >> > >> Further down we use cpumask_subset(mask, cpumask_of(cpu)), > >> apparently to also cover the case where mask is empty. I think > >> you want to do so here as well. > > > > Hm, yes. I guess that's cheaper than adding an extra: > > > > if ( cpumask_empty() ) > > return; > > > > check at the start of the function. > > > >>> if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) && > >>> cpumask_test_cpu(cpu, mask) ) > >> > >> I suppose we want a further precaution here: Despite the > >> !cpumask_subset(mask, cpumask_of(cpu)) below I think we want to > >> extend what c64bf2d2a625 ("x86: make CPU state flush requests > >> explicit") and later changes (isolating uses of FLUSH_VCPU_STATE > >> from other FLUSH_*) did and exclude the use of FLUSH_VCPU_STATE > >> for the local CPU altogether. > > > > If we really want to exclude the use of FLUSH_VCPU_STATE for the local > > CPU, we might wish to add this as a separate ASSERT, so that such > > checking doesn't depend on !local_irq_is_enabled(): > > > > ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)); > > ASSERT(!cpumask_subset(mask, cpumask_of(cpu)) || !(flags & FLUSH_VCPU_STATE)); > > > > > >> That's because if such somehow made > >> it into the conditional below here, it would still involve an IPI. > > > > Sorry, I'm confused by this: if the mask is empty there should be no > > IPI involved at all? And we shouldn't even get into the second > > conditional on the function. > > Should perhaps have made more explicit that "somehow" means a hypothetical > way, perhaps even as a result of some further breakage somewhere. Oh, OK, then I wasn't so confused after all :). Given your lack of comments I assume you are fine with the addition of a separate ASSERT to cover the usage of FLUSH_VCPU_STATE. Thanks, Roger.
On Mon, May 23, 2022 at 06:24:48PM +0200, Roger Pau Monné wrote: > On Mon, May 23, 2022 at 05:13:43PM +0200, Jan Beulich wrote: > > On 23.05.2022 16:37, Roger Pau Monné wrote: > > > On Wed, May 18, 2022 at 10:49:22AM +0200, Jan Beulich wrote: > > >> On 16.05.2022 16:31, Roger Pau Monne wrote: > > >>> --- a/xen/arch/x86/smp.c > > >>> +++ b/xen/arch/x86/smp.c > > >>> @@ -262,7 +262,8 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags) > > >>> { > > >>> unsigned int cpu = smp_processor_id(); > > >>> > > >>> - ASSERT(local_irq_is_enabled()); > > >>> + /* Local flushes can be performed with interrupts disabled. */ > > >>> + ASSERT(local_irq_is_enabled() || cpumask_equal(mask, cpumask_of(cpu))); > > >> > > >> Further down we use cpumask_subset(mask, cpumask_of(cpu)), > > >> apparently to also cover the case where mask is empty. I think > > >> you want to do so here as well. > > > > > > Hm, yes. I guess that's cheaper than adding an extra: > > > > > > if ( cpumask_empty() ) > > > return; > > > > > > check at the start of the function. > > > > > >>> if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) && > > >>> cpumask_test_cpu(cpu, mask) ) > > >> > > >> I suppose we want a further precaution here: Despite the > > >> !cpumask_subset(mask, cpumask_of(cpu)) below I think we want to > > >> extend what c64bf2d2a625 ("x86: make CPU state flush requests > > >> explicit") and later changes (isolating uses of FLUSH_VCPU_STATE > > >> from other FLUSH_*) did and exclude the use of FLUSH_VCPU_STATE > > >> for the local CPU altogether. > > > > > > If we really want to exclude the use of FLUSH_VCPU_STATE for the local > > > CPU, we might wish to add this as a separate ASSERT, so that such > > > checking doesn't depend on !local_irq_is_enabled(): > > > > > > ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)); > > > ASSERT(!cpumask_subset(mask, cpumask_of(cpu)) || !(flags & FLUSH_VCPU_STATE)); Actually, it would seem even more accurate to use: ASSERT(!cpumask_test_cpu(cpu, mask) || !(flags & FLUSH_VCPU_STATE)); Thanks, Roger.
diff --git a/xen/arch/x86/include/asm/flushtlb.h b/xen/arch/x86/include/asm/flushtlb.h index 18777f1d4c..a97b3a2327 100644 --- a/xen/arch/x86/include/asm/flushtlb.h +++ b/xen/arch/x86/include/asm/flushtlb.h @@ -146,7 +146,8 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags); #define flush_mask(mask, flags) flush_area_mask(mask, NULL, flags) /* Flush all CPUs' TLBs/caches */ -#define flush_area_all(va, flags) flush_area_mask(&cpu_online_map, va, flags) +#define flush_area(va, flags) \ + flush_area_mask(&cpu_online_map, (const void *)(va), flags) #define flush_all(flags) flush_mask(&cpu_online_map, flags) /* Flush local TLBs */ diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 67c0427963..45235c5aa6 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5066,14 +5066,6 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v) #define l1f_to_lNf(f) (((f) & _PAGE_PRESENT) ? ((f) | _PAGE_PSE) : (f)) #define lNf_to_l1f(f) (((f) & _PAGE_PRESENT) ? ((f) & ~_PAGE_PSE) : (f)) -/* - * map_pages_to_xen() can be called early in boot before any other - * CPUs are online. Use flush_area_local() in this case. - */ -#define flush_area(v,f) (system_state < SYS_STATE_smp_boot ? \ - flush_area_local((const void *)v, f) : \ - flush_area_all((const void *)v, f)) - #define L3T_INIT(page) (page) = ZERO_BLOCK_PTR #define L3T_LOCK(page) \ diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c index 0a02086966..2f4e98cec9 100644 --- a/xen/arch/x86/smp.c +++ b/xen/arch/x86/smp.c @@ -262,7 +262,8 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags) { unsigned int cpu = smp_processor_id(); - ASSERT(local_irq_is_enabled()); + /* Local flushes can be performed with interrupts disabled. */ + ASSERT(local_irq_is_enabled() || cpumask_equal(mask, cpumask_of(cpu))); if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) && cpumask_test_cpu(cpu, mask) )
Booting with Shadow Stacks leads to the following assert on a debug hypervisor: (XEN) [ 11.625166] Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265 (XEN) [ 11.629410] ----[ Xen-4.17.0-10.24-d x86_64 debug=y Not tainted ]---- (XEN) [ 11.633679] CPU: 0 (XEN) [ 11.637834] RIP: e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e [...] (XEN) [ 11.806158] Xen call trace: (XEN) [ 11.811255] [<ffff82d040345300>] R flush_area_mask+0x40/0x13e (XEN) [ 11.816459] [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958 (XEN) [ 11.821689] [<ffff82d0404474f9>] F arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9 (XEN) [ 11.827053] [<ffff82d0404476cc>] F alternative_branches+0xf/0x12 (XEN) [ 11.832416] [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776 (XEN) [ 11.837809] [<ffff82d040203344>] F __high_start+0x94/0xa0 This is due to SYS_STATE_smp_boot being set before calling alternative_branches(), and the flush in modify_xen_mappings() then using flush_area_all() with interrupts disabled. Note that alternative_branches() is called before APs are started, so the flush must be a local one (and indeed the cpumask passed to flush_area_mask() just contains one CPU). Take the opportunity to simplify a bit the logic and make flush_area() an alias for flush_area_mask(&cpu_online_map...), taking into account that cpu_online_map just contains the BSP before APs are started. This requires widening the assert in flush_area_mask() to allow being called with interrupts disabled as long as it's strictly a local only flush. The overall result is that a conditional can be removed from flush_area(). Fixes: (78e072bc37 'x86/mm: avoid inadvertently degrading a TLB flush to local only') Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/include/asm/flushtlb.h | 3 ++- xen/arch/x86/mm.c | 8 -------- xen/arch/x86/smp.c | 3 ++- 3 files changed, 4 insertions(+), 10 deletions(-)