ACPI global lock macros

* ACPI global lock macros
@ 2003-12-09  9:22 Paul Menage
  2003-12-09  9:36 ` Arjan van de Ven
  2003-12-10  7:45 ` [ACPI] " Andi Kleen
  0 siblings, 2 replies; 6+ messages in thread
From: Paul Menage @ 2003-12-09  9:22 UTC (permalink / raw)
  To: agrover; +Cc: linux-kernel, acpi-devel

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.

Paul

^ permalink raw reply	[flat|nested] 6+ messages in thread