RE: ACPI global lock macros

RE: ACPI global lock macros

[Yu, Luming]

>>#define ACPI_ACQUIRE_GLOBAL_LOCK(GLptr, Acq) \
>>     do { \
>>        asm volatile("1:movl   (%1),%%eax;" \
>>             "movl   %%eax,%%edx;" \
>>             "andl   %2,%%edx;" \
>>             "btsl   $0x1,%%edx;" \
>>             "adcl   $0x0,%%edx;" \
>>             "lock;  cmpxchgl %%edx,(%1);" \
>>             "jnz    1b;" \
>>             "cmpb   $0x3,%%dl;" \
>>             "sbbl   %0,%0" \
>>             :"=r"(Acq):"r"(GLptr),"i"(~1L):"dx", "ax"); \
>>     } while(0)

Above code have a bug! Considering below code:

u8	acquired = FALSE;

ACPI_ACQUIRE_GLOBAL_LOC(acpi_gbl_common_fACS.global_lock, acquired);
if(acquired) {
....
}

Gcc will complain " ERROR: '%cl' not allowed with sbbl ". And I think any other compiler will
complain that  too !

How about  below changes to your proposal code.

<             "sbbl   %0,%0" \
<             :"=r"(Acq):"r"(GLptr),"i"(~1L):"dx","ax"); \
---
>             "sbbl   %%eax,%%eax" \
>             :"=a"(Acq):"r"(GLptr),"i"(~1L):"dx"); \

PS. I'm very curious about how could you find this bug.  

Thanks
Luming



-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

Re: [ACPI] ACPI global lock macros

[Andi Kleen]

On Thu, 11 Dec 2003 15:06:10 +0800
"Yu, Luming" <luming.yu@intel.com> wrote:

> 
> Above code have a bug! Considering below code:
> 
> u8	acquired = FALSE;
> 
> ACPI_ACQUIRE_GLOBAL_LOC(acpi_gbl_common_fACS.global_lock, acquired);
> if(acquired) {
> ....
> }
> 
> Gcc will complain " ERROR: '%cl' not allowed with sbbl ". And I think any other compiler will
> complain that  too !

It has even more bugs, e.g. it doesn't tell gcc that GLptr is modified (this hurts with
newer versions that optimize more aggressively) 

I tried to fix it on x86-64, but eventually gave up and adopted the IA64 C version.

-Andi

Re: ACPI global lock macros

[Paul Menage]

Andi Kleen wrote:
> 
> It has even more bugs, e.g. it doesn't tell gcc that GLptr is modified (this hurts with
> newer versions that optimize more aggressively) 

GLptr itself isn't modified - *GLptr is, but none of the actual C code 
accesses *GLptr, so how does this affect the compiler's optimization 
efforts? Is it because gcc treats the call as a pure function that maps 
a pointer to an acquired value, and hence believes that it can move it 
around? There aren't any instances of ACPI_ACQUIRE_GLOBAL_LOCK being 
called twice in the same function, so it wouldn't be able to cache a 
result. But I agree that it ought to declare that it clobbers memory.

Paul




-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

Re: ACPI global lock macros

[Paul Menage]

Yu, Luming wrote:
> 
> 
> Above code have a bug! Considering below code:
> 
> u8	acquired = FALSE;
> 
> ACPI_ACQUIRE_GLOBAL_LOC(acpi_gbl_common_fACS.global_lock, acquired);
> if(acquired) {
> ....
> }
> 
> Gcc will complain " ERROR: '%cl' not allowed with sbbl ". And I think any other compiler will
> complain that  too !

Oops - the version I posted differed in one character from the version 
that I'd compiled - I'd just used "sbb" in the working version and let 
the compiler figure out which version to use.

> 
> How about  below changes to your proposal code.
> 
> <             "sbbl   %0,%0" \
> <             :"=r"(Acq):"r"(GLptr),"i"(~1L):"dx","ax"); \
> ---
> 
>>            "sbbl   %%eax,%%eax" \
>>            :"=a"(Acq):"r"(GLptr),"i"(~1L):"dx"); \

Yes, that would work too, but I don't like forcing the asm to use 
particular registers when there's no good reason to do so.

> 
> 
> PS. I'm very curious about how could you find this bug.  

It was mainly down to the differences between the i386 and x86_64 
versions, and trying to understand the reasons for those differences, 
and indeed how the entire set of macros worked at all.

Paul



-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

RE: [ACPI] ACPI global lock macros

[Grover, Andrew]

Hi Paul,

Len Brown (len.brown@intel.com) is now the guy for ACPI patch
submissions, but let me just comment that historically this was a "get
it working and leave it alone" area, so if you've found potential bugs
and fixed them, then great.

BTW, i386, x86_64 and ia64 all have this macro, so these all might need
to be looked at.

Regards -- Andy

PS the question that Arjan brought up about why ACPI needs its own lock
has come up before. Maybe we should add this reason to the comment above
these macros in include/asm-*/acpi.h.

> -----Original Message-----
> From: acpi-devel-admin@lists.sourceforge.net 
> [mailto:acpi-devel-admin@lists.sourceforge.net] On Behalf Of 
> Paul Menage

> Hi Andy,
> 
> The ACPI_ACQUIRE_GLOBAL_LOCK() macro in 
> include/asm-i386/acpi.h looks a 
> little odd:
> 
> #define ACPI_ACQUIRE_GLOBAL_LOCK(GLptr, Acq) \
>      do { \
>          int dummy; \
>          asm("1:     movl (%1),%%eax;" \
>              "movl   %%eax,%%edx;" \
>              "andl   %2,%%edx;" \
>              "btsl   $0x1,%%edx;" \
>              "adcl   $0x0,%%edx;" \
>              "lock;  cmpxchgl %%edx,(%1);" \
>              "jnz    1b;" \
>              "cmpb   $0x3,%%dl;" \
>              "sbbl   %%eax,%%eax" \
>              :"=a"(Acq),"=c"(dummy):"c"(GLptr),"i"(~1L):"dx"); \
>      } while(0)
> 
> 
> When compiled, it results in:
> 
>   266:   mov    0x0,%ecx
>                          268: R_386_32   acpi_gbl_common_fACS
>   26c:   mov    (%ecx),%eax
>   26e:   mov    %eax,%edx
>   270:   and    %ecx,%edx
>   272:   bts    $0x1,%edx
>   276:   adc    $0x0,%edx
>   279:   lock cmpxchg %edx,(%ecx)
>   27d:   jne    26c <acpi_ev_acquire_global_lock+0x2f>
>   27f:   cmp    $0x3,%dl
>   282:   sbb    %eax,%eax
> 
> So at location 270 we mask %edx with %ecx, which is the 
> address of the 
> global lock. Unless the global lock is aligned on a 2-byte but not 
> 4-byte boundary, which seems a little unlikely, then this is going to 
> clear both the owned and the pending bits in %edx, so we'll 
> always think 
> that the lock is not owned. Shouldn't the andl be masking 
> with %3 (which 
> is initialised as ~1) rather than %2 (the address of the lock)?
> 
> Given the comments above the definition, I'm guessing that 
> the "dummy" 
> parameter was added later for some reason (to tell gcc that ecx would 
> get clobbered? - but it doesn't seem to be clobbered), and 
> the parameter 
> substitutions in the asm weren't updated. Unless I'm missing 
> something 
> fundamental, shouldn't the definition be something more like this:
> 
> 
> #define ACPI_ACQUIRE_GLOBAL_LOCK(GLptr, Acq) \
>      do { \
>          asm volatile("1:movl   (%1),%%eax;" \
>              "movl   %%eax,%%edx;" \
>              "andl   %2,%%edx;" \
>              "btsl   $0x1,%%edx;" \
>              "adcl   $0x0,%%edx;" \
>              "lock;  cmpxchgl %%edx,(%1);" \
>              "jnz    1b;" \
>              "cmpb   $0x3,%%dl;" \
>              "sbbl   %0,%0" \
>              :"=r"(Acq):"r"(GLptr),"i"(~1L):"dx", "ax"); \
>      } while(0)
> 
> which compiles to:
> 
>   2e5:   mov    0x0,%ecx
>                          2e7: R_386_32   acpi_gbl_common_fACS
>   2eb:   mov    (%ecx),%eax
>   2ed:   mov    %eax,%edx
>   2ef:   and    $0xfffffffe,%edx
>   2f2:   bts    $0x1,%edx
>   2f6:   adc    $0x0,%edx
>   2f9:   lock cmpxchg %edx,(%ecx)
>   2fd:   jne    2eb <acpi_ev_acquire_global_lock+0x37>
>   2ff:   cmp    $0x3,%dl
>   302:   sbb    %cl,%cl
> 
> 
> which is identical to the ACPI spec reference implementation, 
> apart from 
> returning the result in %cl rather than %al (since we're cleanly 
> separating clobbered registers from input/output params, and 
> letting gcc 
> choose the param registers).
> 
> Alternatively it could be defined in C (as in ia64) which 
> would reduce 
> the likelihood of asm bugs. (Although it wouldn't be safe to use 
> __cmpxchg(), as that uses LOCK_PREFIX which is empty on UP, 
> rather than 
> an explicit "lock").
> 
> ACPI_RELEASE_GLOBAL_LOCK(), and the x86_64 variants of these, seem to 
> have similar issues.

Re: ACPI global lock macros

[Paul Menage]

Grover, Andrew wrote:
> 
> BTW, i386, x86_64 and ia64 all have this macro, so these all might need
> to be looked at.
> 

Yes, it was the differences between the i386 and x86_64 versions that 
made me notice this problem. The ia64 version is in C, so looks safer. 
Ideally there would be a common C definition - the only arch-specific 
part should be the locked cmpxchg, unless this lock is likely to be 
taken/released so often that it's performance critical.

Paul



-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

Re: [ACPI] ACPI global lock macros

[David Mosberger]

>>>>> On Tue, 09 Dec 2003 11:04:05 -0800, Paul Menage <menage@google.com> said:

  Paul> Grover, Andrew wrote:

  >> BTW, i386, x86_64 and ia64 all have this macro, so these all might need
  >> to be looked at.


  Paul> Yes, it was the differences between the i386 and x86_64
  Paul> versions that made me notice this problem. The ia64 version is
  Paul> in C, so looks safer.  Ideally there would be a common C
  Paul> definition - the only arch-specific part should be the locked
  Paul> cmpxchg, unless this lock is likely to be taken/released so
  Paul> often that it's performance critical.

As far as ia64 is concerned, you could replace ia64_cmpxchg4_acq()
with cmpxchg().  That should just work.  Of course, as you point out,
that wouldn't work on x86 UP because of the missing "lock" prefix.
Sounds like you'd need to add something along the lines of
device_atomic_cmpxchg() or something like that...

	--david