Re: [Xen-devel] [PATCH v3 13/17] livepatch: Expand spin_debug_disable in [apply|revert]_payload

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxx>
> ---
>   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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel