[PATCH] xen: arm: clear the exclusive monitor on exception return

[PATCH] xen: arm: clear the exclusive monitor on exception return

[Ian Campbell]

Otherwise context switching between two vcpus which are contending the same
lock can result in a spurious success.

This is not required on ARMv8 since eret implicitly clears the monitor.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/arm32/entry.S |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 76814dd..1c26835 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -117,6 +117,7 @@ ENTRY(return_to_hypervisor)
         msr SPSR_hyp, r11
         pop {r0-r12}
         add sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */
+        clrex
         eret
 
 /*
-- 
1.7.2.5

Re: [PATCH] xen: arm: clear the exclusive monitor on exception return

[Stefano Stabellini]

On Wed, 17 Jul 2013, Ian Campbell wrote:
> Otherwise context switching between two vcpus which are contending the same
> lock can result in a spurious success.
> 
> This is not required on ARMv8 since eret implicitly clears the monitor.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  xen/arch/arm/arm32/entry.S |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 76814dd..1c26835 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -117,6 +117,7 @@ ENTRY(return_to_hypervisor)
>          msr SPSR_hyp, r11
>          pop {r0-r12}
>          add sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */
> +        clrex
>          eret
>  
>  /*
> -- 
> 1.7.2.5
> 

Re: [PATCH] xen: arm: clear the exclusive monitor on exception return

[Tim Deegan]

At 12:18 +0100 on 17 Jul (1374063531), Ian Campbell wrote:
> Otherwise context switching between two vcpus which are contending the same
> lock can result in a spurious success.

Shouldn't this go in ctxt_swicth_to(), then?  I think any use that Xen
itself makes of the monitor should end with it cleared.

Tim.

> This is not required on ARMv8 since eret implicitly clears the monitor.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/arm32/entry.S |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 76814dd..1c26835 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -117,6 +117,7 @@ ENTRY(return_to_hypervisor)
>          msr SPSR_hyp, r11
>          pop {r0-r12}
>          add sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */
> +        clrex
>          eret
>  
>  /*
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] xen: arm: clear the exclusive monitor on exception return

[Ian Campbell]

On Wed, 2013-07-17 at 20:48 +0100, Tim Deegan wrote:
> At 12:18 +0100 on 17 Jul (1374063531), Ian Campbell wrote:
> > Otherwise context switching between two vcpus which are contending the same
> > lock can result in a spurious success.
> 
> Shouldn't this go in ctxt_swicth_to(), then?  I think any use that Xen
> itself makes of the monitor should end with it cleared.

Our atomic read/set which we stole^Winherited from Linux relies on this
behaviour (interlocking with atomic_add_and_blah stuff).
        /*
         * On ARM, ordinary assignment (str instruction) doesn't clear the local
         * strex/ldrex monitor on some implementations. The reason we can use it for
         * atomic_set() is the clrex or dummy strex done on every exception return.
         */
As well as atomics our spin unlock (also inherited) is just a plain
store.

If nothing else it makes v7 and v8 consistent since the clrex is
implicit in the eret on v8.

> 
> Tim.
> 
> > This is not required on ARMv8 since eret implicitly clears the monitor.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/arm32/entry.S |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> > index 76814dd..1c26835 100644
> > --- a/xen/arch/arm/arm32/entry.S
> > +++ b/xen/arch/arm/arm32/entry.S
> > @@ -117,6 +117,7 @@ ENTRY(return_to_hypervisor)
> >          msr SPSR_hyp, r11
> >          pop {r0-r12}
> >          add sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */
> > +        clrex
> >          eret
> >  
> >  /*
> > -- 
> > 1.7.2.5
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: [PATCH] xen: arm: clear the exclusive monitor on exception return

[Tim Deegan]

At 09:31 +0100 on 18 Jul (1374139884), Ian Campbell wrote:
> On Wed, 2013-07-17 at 20:48 +0100, Tim Deegan wrote:
> > At 12:18 +0100 on 17 Jul (1374063531), Ian Campbell wrote:
> > > Otherwise context switching between two vcpus which are contending the same
> > > lock can result in a spurious success.
> > 
> > Shouldn't this go in ctxt_swicth_to(), then?  I think any use that Xen
> > itself makes of the monitor should end with it cleared.
> 
> Our atomic read/set which we stole^Winherited from Linux relies on this
> behaviour (interlocking with atomic_add_and_blah stuff).
>         /*
>          * On ARM, ordinary assignment (str instruction) doesn't clear the local
>          * strex/ldrex monitor on some implementations. The reason we can use it for
>          * atomic_set() is the clrex or dummy strex done on every exception return.
>          */

Ah, OK.  So, Ack, but if it's not already committed can you edit the
commit message to refer to this?

> As well as atomics our spin unlock (also inherited) is just a plain
> store.

Well, we already avoid having locks shared between interrupt/exception
handlers and plain code, but I guess it can't hurt.

Tim.

Re: [PATCH] xen: arm: clear the exclusive monitor on exception return

[Ian Campbell]

On Thu, 2013-07-18 at 17:48 +0100, Tim Deegan wrote:
> At 09:31 +0100 on 18 Jul (1374139884), Ian Campbell wrote:
> > On Wed, 2013-07-17 at 20:48 +0100, Tim Deegan wrote:
> > > At 12:18 +0100 on 17 Jul (1374063531), Ian Campbell wrote:
> > > > Otherwise context switching between two vcpus which are contending the same
> > > > lock can result in a spurious success.
> > > 
> > > Shouldn't this go in ctxt_swicth_to(), then?  I think any use that Xen
> > > itself makes of the monitor should end with it cleared.
> > 
> > Our atomic read/set which we stole^Winherited from Linux relies on this
> > behaviour (interlocking with atomic_add_and_blah stuff).
> >         /*
> >          * On ARM, ordinary assignment (str instruction) doesn't clear the local
> >          * strex/ldrex monitor on some implementations. The reason we can use it for
> >          * atomic_set() is the clrex or dummy strex done on every exception return.
> >          */
> 
> Ah, OK.  So, Ack, but if it's not already committed can you edit the
> commit message to refer to this?

Thanks and Yes Sure.

> > As well as atomics our spin unlock (also inherited) is just a plain
> > store.
> 
> Well, we already avoid having locks shared between interrupt/exception
> handlers and plain code,

We do? Plain code can use the irqsave/restore variants if it wants to
coexist with irq handlers which take the same locks, can't they? e.g.
the gic lock is handled this way... If that's not valid then we might
have a problem ;-)

>  but I guess it can't hurt.
> 
> Tim.

Re: [PATCH] xen: arm: clear the exclusive monitor on exception return

[Ian Campbell]

On Thu, 2013-07-18 at 17:48 +0100, Tim Deegan wrote:
> At 09:31 +0100 on 18 Jul (1374139884), Ian Campbell wrote:
> > On Wed, 2013-07-17 at 20:48 +0100, Tim Deegan wrote:
> > > At 12:18 +0100 on 17 Jul (1374063531), Ian Campbell wrote:
> > > > Otherwise context switching between two vcpus which are contending the same
> > > > lock can result in a spurious success.
> > > 
> > > Shouldn't this go in ctxt_swicth_to(), then?  I think any use that Xen
> > > itself makes of the monitor should end with it cleared.
> > 
> > Our atomic read/set which we stole^Winherited from Linux relies on this
> > behaviour (interlocking with atomic_add_and_blah stuff).
> >         /*
> >          * On ARM, ordinary assignment (str instruction) doesn't clear the local
> >          * strex/ldrex monitor on some implementations. The reason we can use it for
> >          * atomic_set() is the clrex or dummy strex done on every exception return.
> >          */
> 
> Ah, OK.  So, Ack, but if it's not already committed can you edit the
> commit message to refer to this?

Applied, thanks. Commit message is now:

        xen: arm: clear the exclusive monitor on exception return
        
        Otherwise context switching between two vcpus which are contending the same
        lock can result in a spurious success.
        
        Our spinlock and atomics code (which we get from Linux) rely on this behaviour
        because they use non-exclusive stores for single instruction operations (e.g. 
        spin_unlock or atomic_set).
        
        This is not required on ARMv8 since eret implicitly clears the monitor.
        
        Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
        Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
        Acked-by: Tim Deegan <tim@xen.org>

Re: [PATCH] xen: arm: clear the exclusive monitor on exception return

[Tim Deegan]

At 08:53 +0100 on 19 Jul (1374224014), Ian Campbell wrote:
> On Thu, 2013-07-18 at 17:48 +0100, Tim Deegan wrote:
> > At 09:31 +0100 on 18 Jul (1374139884), Ian Campbell wrote:
> > > As well as atomics our spin unlock (also inherited) is just a plain
> > > store.
> > 
> > Well, we already avoid having locks shared between interrupt/exception
> > handlers and plain code,
> 
> We do? Plain code can use the irqsave/restore variants if it wants to
> coexist with irq handlers which take the same locks, can't they? e.g.
> the gic lock is handled this way... If that's not valid then we might
> have a problem ;-)

Yes, but the irqsave variants are there explicitly to avoid the case
where irq-disabled code touches a spinlock that plain code might be
spinning on.  I guess officially there's nothing stopping irq-disabled
code from _unlocking_ a plain-code spinlock, so this covers that case. :)

Tim.