Ian Campbell
2013-Jul-17 11:18 UTC
[PATCH] 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.
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
Stefano Stabellini
2013-Jul-17 13:57 UTC
Re: [PATCH] xen: arm: clear the exclusive monitor on exception return
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 >
Tim Deegan
2013-Jul-17 19:48 UTC
Re: [PATCH] xen: arm: clear the exclusive monitor on exception return
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
Ian Campbell
2013-Jul-18 08:31 UTC
Re: [PATCH] xen: arm: clear the exclusive monitor on exception return
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
Tim Deegan
2013-Jul-18 16:48 UTC
Re: [PATCH] xen: arm: clear the exclusive monitor on exception return
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.
Ian Campbell
2013-Jul-19 07:53 UTC
Re: [PATCH] xen: arm: clear the exclusive monitor on exception return
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.
Ian Campbell
2013-Jul-19 14:17 UTC
Re: [PATCH] xen: arm: clear the exclusive monitor on exception return
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>
Tim Deegan
2013-Jul-19 17:06 UTC
Re: [PATCH] xen: arm: clear the exclusive monitor on exception return
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.