[Qemu-devel] testandset asm fix

[Qemu-devel] testandset asm fix

[Paul Brook]

The inline assembly used for the x86/x86-64 host testandset routine is bogus.
The operand constraints are wrong (Fails to compile at -O0).
Also the return value is incorrect. It should return 0 if the lock was 
successfully acquired. 

Patch below fixes it. The additional sete test is unnecessary, we can just use 
the comparison/writeback value.

Paul

Index: exec-all.h
===================================================================
RCS file: /cvsroot/qemu/qemu/exec-all.h,v
retrieving revision 1.26
diff -u -p -r1.26 exec-all.h
--- exec-all.h 10 Jan 2005 23:23:48 -0000 1.26
+++ exec-all.h 6 Feb 2005 14:35:43 -0000
@@ -392,28 +392,24 @@ static inline int testandset (int *p)
 #ifdef __i386__
 static inline int testandset (int *p)
 {
-    char ret;
-    long int readval;
-    
-    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
-                          : "=q" (ret), "=m" (*p), "=a" (readval)
-                          : "r" (1), "m" (*p), "a" (0)
-                          : "memory");
-    return ret;
+    long int readval = 0;
+
+    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
+                          : "+m" (*p), "+a" (readval)
+                          : "r" (1));
+    return readval;
 }
 #endif
 
 #ifdef __x86_64__
 static inline int testandset (int *p)
 {
-    char ret;
-    int readval;
-    
-    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
-                          : "=q" (ret), "=m" (*p), "=a" (readval)
-                          : "r" (1), "m" (*p), "a" (0)
-                          : "memory");
-    return ret;
+    long int readval = 0;
+
+    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
+                          : "+m" (*p), "+a" (readval)
+                          : "r" (1));
+    return readval;
 }
 #endif
 

Re: [Qemu-devel] testandset asm fix

[Piotras]

m68k has similar problem

http://lists.gnu.org/archive/html/qemu-devel/2004-08/msg00122.html


Regards,

Piotrek

On Sun, 6 Feb 2005 14:43:38 +0000, Paul Brook <paul@codesourcery.com> wrote:
> The inline assembly used for the x86/x86-64 host testandset routine is bogus.
> The operand constraints are wrong (Fails to compile at -O0).
> Also the return value is incorrect. It should return 0 if the lock was
> successfully acquired.
> 
> Patch below fixes it. The additional sete test is unnecessary, we can just use
> the comparison/writeback value.
> 
> Paul
> 
> Index: exec-all.h
> ===================================================================
> RCS file: /cvsroot/qemu/qemu/exec-all.h,v
> retrieving revision 1.26
> diff -u -p -r1.26 exec-all.h
> --- exec-all.h 10 Jan 2005 23:23:48 -0000 1.26
> +++ exec-all.h 6 Feb 2005 14:35:43 -0000
> @@ -392,28 +392,24 @@ static inline int testandset (int *p)
> #ifdef __i386__
> static inline int testandset (int *p)
> {
> -    char ret;
> -    long int readval;
> -
> -    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
> -                          : "=q" (ret), "=m" (*p), "=a" (readval)
> -                          : "r" (1), "m" (*p), "a" (0)
> -                          : "memory");
> -    return ret;
> +    long int readval = 0;
> +
> +    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
> +                          : "+m" (*p), "+a" (readval)
> +                          : "r" (1));
> +    return readval;
> }
> #endif
> 
> #ifdef __x86_64__
> static inline int testandset (int *p)
> {
> -    char ret;
> -    int readval;
> -
> -    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
> -                          : "=q" (ret), "=m" (*p), "=a" (readval)
> -                          : "r" (1), "m" (*p), "a" (0)
> -                          : "memory");
> -    return ret;
> +    long int readval = 0;
> +
> +    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
> +                          : "+m" (*p), "+a" (readval)
> +                          : "r" (1));
> +    return readval;
> }
> #endif
> 
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
>

Re: [Qemu-devel] testandset asm fix

[Fabrice Bellard]

OK. Anyway, the locking in QEMU is mostly boggus. If SMP is implemented 
someday with host threads, then it will be the right time to correct it !

Fabrice.

Paul Brook wrote:
> The inline assembly used for the x86/x86-64 host testandset routine is bogus.
> The operand constraints are wrong (Fails to compile at -O0).
> Also the return value is incorrect. It should return 0 if the lock was 
> successfully acquired. 
> 
> Patch below fixes it. The additional sete test is unnecessary, we can just use 
> the comparison/writeback value.
> 
> Paul
> 
> Index: exec-all.h
> ===================================================================
> RCS file: /cvsroot/qemu/qemu/exec-all.h,v
> retrieving revision 1.26
> diff -u -p -r1.26 exec-all.h
> --- exec-all.h 10 Jan 2005 23:23:48 -0000 1.26
> +++ exec-all.h 6 Feb 2005 14:35:43 -0000
> @@ -392,28 +392,24 @@ static inline int testandset (int *p)
>  #ifdef __i386__
>  static inline int testandset (int *p)
>  {
> -    char ret;
> -    long int readval;
> -    
> -    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
> -                          : "=q" (ret), "=m" (*p), "=a" (readval)
> -                          : "r" (1), "m" (*p), "a" (0)
> -                          : "memory");
> -    return ret;
> +    long int readval = 0;
> +
> +    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
> +                          : "+m" (*p), "+a" (readval)
> +                          : "r" (1));
> +    return readval;
>  }
>  #endif
>  
>  #ifdef __x86_64__
>  static inline int testandset (int *p)
>  {
> -    char ret;
> -    int readval;
> -    
> -    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
> -                          : "=q" (ret), "=m" (*p), "=a" (readval)
> -                          : "r" (1), "m" (*p), "a" (0)
> -                          : "memory");
> -    return ret;
> +    long int readval = 0;
> +
> +    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
> +                          : "+m" (*p), "+a" (readval)
> +                          : "r" (1));
> +    return readval;
>  }
>  #endif
>  
> 
> 
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
> 
> 

Re: [Qemu-devel] testandset asm fix

[Piotras]

What about IRQ latency?


Piotrek

On Mon, 07 Feb 2005 14:47:29 +0100, Fabrice Bellard <fabrice@bellard.org> wrote:
> OK. Anyway, the locking in QEMU is mostly boggus. If SMP is implemented
> someday with host threads, then it will be the right time to correct it !
> 
> Fabrice.
> 
> Paul Brook wrote:
> > The inline assembly used for the x86/x86-64 host testandset routine is bogus.
> > The operand constraints are wrong (Fails to compile at -O0).
> > Also the return value is incorrect. It should return 0 if the lock was
> > successfully acquired.
> >
> > Patch below fixes it. The additional sete test is unnecessary, we can just use
> > the comparison/writeback value.
> >
> > Paul
> >
> > Index: exec-all.h
> > ===================================================================
> > RCS file: /cvsroot/qemu/qemu/exec-all.h,v
> > retrieving revision 1.26
> > diff -u -p -r1.26 exec-all.h
> > --- exec-all.h 10 Jan 2005 23:23:48 -0000 1.26
> > +++ exec-all.h 6 Feb 2005 14:35:43 -0000
> > @@ -392,28 +392,24 @@ static inline int testandset (int *p)
> >  #ifdef __i386__
> >  static inline int testandset (int *p)
> >  {
> > -    char ret;
> > -    long int readval;
> > -
> > -    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
> > -                          : "=q" (ret), "=m" (*p), "=a" (readval)
> > -                          : "r" (1), "m" (*p), "a" (0)
> > -                          : "memory");
> > -    return ret;
> > +    long int readval = 0;
> > +
> > +    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
> > +                          : "+m" (*p), "+a" (readval)
> > +                          : "r" (1));
> > +    return readval;
> >  }
> >  #endif
> >
> >  #ifdef __x86_64__
> >  static inline int testandset (int *p)
> >  {
> > -    char ret;
> > -    int readval;
> > -
> > -    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
> > -                          : "=q" (ret), "=m" (*p), "=a" (readval)
> > -                          : "r" (1), "m" (*p), "a" (0)
> > -                          : "memory");
> > -    return ret;
> > +    long int readval = 0;
> > +
> > +    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
> > +                          : "+m" (*p), "+a" (readval)
> > +                          : "r" (1));
> > +    return readval;
> >  }
> >  #endif
> >
> >
> >
> > _______________________________________________
> > Qemu-devel mailing list
> > Qemu-devel@nongnu.org
> > http://lists.nongnu.org/mailman/listinfo/qemu-devel
> >
> >
> 
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
>

Re: [Qemu-devel] testandset asm fix

[Piotras]

I didn't noticed that the patch was committed.


Piotrek

On Mon, 7 Feb 2005 14:26:36 +0000, Piotras <piotras@gmail.com> wrote:
> What about IRQ latency?
> 
> Piotrek
> 
> On Mon, 07 Feb 2005 14:47:29 +0100, Fabrice Bellard <fabrice@bellard.org> wrote:
> > OK. Anyway, the locking in QEMU is mostly boggus. If SMP is implemented
> > someday with host threads, then it will be the right time to correct it !
> >
> > Fabrice.
> >
> > Paul Brook wrote:
> > > The inline assembly used for the x86/x86-64 host testandset routine is bogus.
> > > The operand constraints are wrong (Fails to compile at -O0).
> > > Also the return value is incorrect. It should return 0 if the lock was
> > > successfully acquired.
> > >
> > > Patch below fixes it. The additional sete test is unnecessary, we can just use
> > > the comparison/writeback value.
> > >
> > > Paul
> > >
> > > Index: exec-all.h
> > > ===================================================================
> > > RCS file: /cvsroot/qemu/qemu/exec-all.h,v
> > > retrieving revision 1.26
> > > diff -u -p -r1.26 exec-all.h
> > > --- exec-all.h 10 Jan 2005 23:23:48 -0000 1.26
> > > +++ exec-all.h 6 Feb 2005 14:35:43 -0000
> > > @@ -392,28 +392,24 @@ static inline int testandset (int *p)
> > >  #ifdef __i386__
> > >  static inline int testandset (int *p)
> > >  {
> > > -    char ret;
> > > -    long int readval;
> > > -
> > > -    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
> > > -                          : "=q" (ret), "=m" (*p), "=a" (readval)
> > > -                          : "r" (1), "m" (*p), "a" (0)
> > > -                          : "memory");
> > > -    return ret;
> > > +    long int readval = 0;
> > > +
> > > +    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
> > > +                          : "+m" (*p), "+a" (readval)
> > > +                          : "r" (1));
> > > +    return readval;
> > >  }
> > >  #endif
> > >
> > >  #ifdef __x86_64__
> > >  static inline int testandset (int *p)
> > >  {
> > > -    char ret;
> > > -    int readval;
> > > -
> > > -    __asm__ __volatile__ ("lock; cmpxchgl %3, %1; sete %0"
> > > -                          : "=q" (ret), "=m" (*p), "=a" (readval)
> > > -                          : "r" (1), "m" (*p), "a" (0)
> > > -                          : "memory");
> > > -    return ret;
> > > +    long int readval = 0;
> > > +
> > > +    __asm__ __volatile__ ("lock; cmpxchgl %2, %0"
> > > +                          : "+m" (*p), "+a" (readval)
> > > +                          : "r" (1));
> > > +    return readval;
> > >  }
> > >  #endif
> > >
> > >
> > >
> > > _______________________________________________
> > > Qemu-devel mailing list
> > > Qemu-devel@nongnu.org
> > > http://lists.nongnu.org/mailman/listinfo/qemu-devel
> > >
> > >
> >
> > _______________________________________________
> > Qemu-devel mailing list
> > Qemu-devel@nongnu.org
> > http://lists.nongnu.org/mailman/listinfo/qemu-devel
> >
>

Re: [Qemu-devel] testandset asm fix

[Lennert Buytenhek]

On Mon, Feb 07, 2005 at 02:47:29PM +0100, Fabrice Bellard wrote:

> OK. Anyway, the locking in QEMU is mostly boggus. If SMP is
> implemented someday with host threads, then it will be the right
> time to correct it !

Differences in strict/loose memory ordering between different
processor types will be a bigger problem for SMP emulation, IMHO.
What are your plans on that?


cheers,
Lennert

Re: [Qemu-devel] testandset asm fix

[Fabrice Bellard]

Lennert Buytenhek wrote:
> On Mon, Feb 07, 2005 at 02:47:29PM +0100, Fabrice Bellard wrote:
> 
> 
>>OK. Anyway, the locking in QEMU is mostly boggus. If SMP is
>>implemented someday with host threads, then it will be the right
>>time to correct it !
> 
> 
> Differences in strict/loose memory ordering between different
> processor types will be a bigger problem for SMP emulation, IMHO.
> What are your plans on that?

For x86 on x86 it won't be a problem if we use the same locking 
instructions. For the rest, using high level locks might suffice at the 
expense of efficiency. In the worst case we can simply not use threads 
and do explicit scheduling based on a cycle counter.

Fabrice.