Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores

[Aurelien Jarno <aurelien@aurel32.net>]

On Fri, Nov 07, 2008 at 03:19:08PM +0100, Laurent Desnogues wrote:
> On Fri, Nov 7, 2008 at 3:00 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Fri, Nov 07, 2008 at 12:34:29PM +0100, Laurent Desnogues wrote:
> >> Hello,
> >>
> >> this patch is based on Vince Weaver patch for locked loads/stores.
> >> It was checked against Alpha architecture manual.
> >>
> >> Two fixes were done to Vince's patch:
> >>
> >>    - use a comparison to 1 for lock instead of 0 to be closer to the
> >>      Alpha spec
> >
> > I don't agree with this part. The current code use a single variable for
> > both address and lock_bit to spare a few tests. Basically it sets
> > cpu_lock to -1 when not locked and stores the address when locked. Your
> > patch does not compare the address, so it will break multi-threading.
> 
> My understanding of the Alpha architecture manual is that
> if the addresses don't meet certain criteria (which you
> simplify to addresses comparison) then failure or success
> of st_c is UNPREDICTABLE (I am not shouting, it's the way
> they write it :-) unless some lock clearing occurred (cf
> section 4.2.5).

The manual is actually not really clear. Section 4.2.5 does not really
speak about storing the locked address, while Section 3.1.4 explicitly
mentions a "locked_physical_address register".

The current implementation, now that it is fixed (can someone confirms
that the problem is actually fixed?), matches the pre-TCG
implementation. I am not sure it is the correct one, but if it works for
now and until someone comes with more arguments, I think we should let
the code as now.

Aurelien 

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net