| Message ID | 20170912003726.368-14-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Konrad, On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote: > Under ARM64 the vmap calls were all done with IRQs disabled which > didn't trip the spinlock debug check (as seen on x86): Well, I think it does not happen because spin_debug_enable() is never called on ARM at boot. So atomic_read(&spin_debug) can never return a positive value and hence the check will not be performed. We should probably enable spin_debug on ARM. > livepatch.c:1330: livepatch: xen_hello_world: timeout is 30000000ns > livepatch.c:1437: livepatch: xen_hello_world: CPU3 - IPIing the other 3 CPUs > Applying xen_hello_world... (XEN) livepatch: xen_hello_world: Applying 1 functions > Xen BUG at spinlock.c:47 > ..snip.. > [<ffff82d0802379b1>] spinlock.c#check_lock+0x3e/0x44 > [<ffff82d080237a70>] _spin_lock+0x11/0x4a > [<ffff82d08023e2de>] __vmap+0x78/0x381 > [<ffff82d080219c13>] livepatch.c#livepatch_quiesce+0xc4/0x1bf > [<ffff82d080219ee3>] livepatch.c#apply_payload+0x3a/0x102 > [<ffff82d08021a19e>] check_for_livepatch_work+0x1f3/0x390 > [<ffff82d08027b7f4>] domain.c#continue_idle_domain+0x1b/0x22 > > But are definitly the case on x86 - so we expand the scope s/definitly/definitely/ > of the spin_debug_disable. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/common/livepatch.c | 30 ++++++++++++++++-------------- > 1 file changed, 16 insertions(+), 14 deletions(-) > > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > index 93083cda1a..2f5ee1ae75 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -1154,22 +1154,22 @@ static int apply_payload(struct payload *data) > printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n", > data->name, data->nfuncs); > > + /* > + * Since we are running with IRQs disabled and the hooks may call common > + * code - which expects certain spinlocks to run with IRQs enabled - we > + * temporarily disable the spin locks IRQ state checks. > + */ > + spin_debug_disable(); > rc = livepatch_quiesce(data->funcs, data->nfuncs); > if ( rc ) > { > + spin_debug_enable(); > printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); > return rc; > } > > - /* > - * Since we are running with IRQs disabled and the hooks may call common > - * code - which expects certain spinlocks to run with IRQs enabled - we > - * temporarily disable the spin locks IRQ state checks. > - */ > - spin_debug_disable(); > for ( i = 0; i < data->n_load_funcs; i++ ) > data->load_funcs[i](); > - spin_debug_enable(); > > ASSERT(!local_irq_is_enabled()); > > @@ -1177,6 +1177,7 @@ static int apply_payload(struct payload *data) > arch_livepatch_apply(&data->funcs[i]); > > livepatch_revive(); > + spin_debug_enable(); > > /* > * We need RCU variant (which has barriers) in case we crash here. > @@ -1195,9 +1196,16 @@ static int revert_payload(struct payload *data) > > printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name); > > + /* > + * Since we are running with IRQs disabled and the hooks may call common > + * code - which expects certain spinlocks to run with IRQs enabled - we > + * temporarily disable the spin locks IRQ state checks. > + */ > + spin_debug_disable(); > rc = livepatch_quiesce(data->funcs, data->nfuncs); > if ( rc ) > { > + spin_debug_enable(); > printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); > return rc; > } > @@ -1205,19 +1213,13 @@ static int revert_payload(struct payload *data) > for ( i = 0; i < data->nfuncs; i++ ) > livepatch_revert(&data->funcs[i]); > > - /* > - * Since we are running with IRQs disabled and the hooks may call common > - * code - which expects certain spinlocks to run with IRQs enabled - we > - * temporarily disable the spin locks IRQ state checks. > - */ > - spin_debug_disable(); > for ( i = 0; i < data->n_unload_funcs; i++ ) > data->unload_funcs[i](); > - spin_debug_enable(); > > ASSERT(!local_irq_is_enabled()); > > livepatch_revive(); > + spin_debug_enable(); > > /* > * We need RCU variant (which has barriers) in case we crash here. > Cheers,
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 93083cda1a..2f5ee1ae75 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -1154,22 +1154,22 @@ static int apply_payload(struct payload *data) printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n", data->name, data->nfuncs); + /* + * Since we are running with IRQs disabled and the hooks may call common + * code - which expects certain spinlocks to run with IRQs enabled - we + * temporarily disable the spin locks IRQ state checks. + */ + spin_debug_disable(); rc = livepatch_quiesce(data->funcs, data->nfuncs); if ( rc ) { + spin_debug_enable(); printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); return rc; } - /* - * Since we are running with IRQs disabled and the hooks may call common - * code - which expects certain spinlocks to run with IRQs enabled - we - * temporarily disable the spin locks IRQ state checks. - */ - spin_debug_disable(); for ( i = 0; i < data->n_load_funcs; i++ ) data->load_funcs[i](); - spin_debug_enable(); ASSERT(!local_irq_is_enabled()); @@ -1177,6 +1177,7 @@ static int apply_payload(struct payload *data) arch_livepatch_apply(&data->funcs[i]); livepatch_revive(); + spin_debug_enable(); /* * We need RCU variant (which has barriers) in case we crash here. @@ -1195,9 +1196,16 @@ static int revert_payload(struct payload *data) printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name); + /* + * Since we are running with IRQs disabled and the hooks may call common + * code - which expects certain spinlocks to run with IRQs enabled - we + * temporarily disable the spin locks IRQ state checks. + */ + spin_debug_disable(); rc = livepatch_quiesce(data->funcs, data->nfuncs); if ( rc ) { + spin_debug_enable(); printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); return rc; } @@ -1205,19 +1213,13 @@ static int revert_payload(struct payload *data) for ( i = 0; i < data->nfuncs; i++ ) livepatch_revert(&data->funcs[i]); - /* - * Since we are running with IRQs disabled and the hooks may call common - * code - which expects certain spinlocks to run with IRQs enabled - we - * temporarily disable the spin locks IRQ state checks. - */ - spin_debug_disable(); for ( i = 0; i < data->n_unload_funcs; i++ ) data->unload_funcs[i](); - spin_debug_enable(); ASSERT(!local_irq_is_enabled()); livepatch_revive(); + spin_debug_enable(); /* * We need RCU variant (which has barriers) in case we crash here.
Under ARM64 the vmap calls were all done with IRQs disabled which didn't trip the spinlock debug check (as seen on x86): livepatch.c:1330: livepatch: xen_hello_world: timeout is 30000000ns livepatch.c:1437: livepatch: xen_hello_world: CPU3 - IPIing the other 3 CPUs Applying xen_hello_world... (XEN) livepatch: xen_hello_world: Applying 1 functions Xen BUG at spinlock.c:47 ..snip.. [<ffff82d0802379b1>] spinlock.c#check_lock+0x3e/0x44 [<ffff82d080237a70>] _spin_lock+0x11/0x4a [<ffff82d08023e2de>] __vmap+0x78/0x381 [<ffff82d080219c13>] livepatch.c#livepatch_quiesce+0xc4/0x1bf [<ffff82d080219ee3>] livepatch.c#apply_payload+0x3a/0x102 [<ffff82d08021a19e>] check_for_livepatch_work+0x1f3/0x390 [<ffff82d08027b7f4>] domain.c#continue_idle_domain+0x1b/0x22 But are definitly the case on x86 - so we expand the scope of the spin_debug_disable. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/common/livepatch.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)