* 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; 15+ 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] 15+ messages in thread
* Re: ACPI global lock macros
2003-12-09 9:22 ACPI global lock macros Paul Menage
@ 2003-12-09 9:36 ` Arjan van de Ven
2003-12-09 9:42 ` Paul Menage
2003-12-10 7:45 ` [ACPI] " Andi Kleen
1 sibling, 1 reply; 15+ messages in thread
From: Arjan van de Ven @ 2003-12-09 9:36 UTC (permalink / raw)
To: Paul Menage; +Cc: agrover, linux-kernel, acpi-devel
[-- Attachment #1: Type: text/plain, Size: 269 bytes --]
On Tue, 2003-12-09 at 10:22, Paul Menage wrote:
> Hi Andy,
>
> The ACPI_ACQUIRE_GLOBAL_LOCK() macro in include/asm-i386/acpi.h looks a
> little odd:
maybe the odd thing is that it exists at all?
(eg why does ACPI need to have it's own locking primitives...)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: ACPI global lock macros
2003-12-09 9:36 ` Arjan van de Ven
@ 2003-12-09 9:42 ` Paul Menage
2003-12-09 9:43 ` Arjan van de Ven
0 siblings, 1 reply; 15+ messages in thread
From: Paul Menage @ 2003-12-09 9:42 UTC (permalink / raw)
To: arjanv; +Cc: agrover, linux-kernel, acpi-devel
Arjan van de Ven wrote:
>
> maybe the odd thing is that it exists at all?
> (eg why does ACPI need to have it's own locking primitives...)
Because the ACPI spec defines its own locking protocol for
synchronization between the OS and the BIOS.
Paul
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: ACPI global lock macros
2003-12-09 9:42 ` Paul Menage
@ 2003-12-09 9:43 ` Arjan van de Ven
2003-12-09 9:50 ` Paul Menage
0 siblings, 1 reply; 15+ messages in thread
From: Arjan van de Ven @ 2003-12-09 9:43 UTC (permalink / raw)
To: Paul Menage; +Cc: arjanv, agrover, linux-kernel, acpi-devel
On Tue, Dec 09, 2003 at 01:42:34AM -0800, Paul Menage wrote:
> Arjan van de Ven wrote:
> >
> >maybe the odd thing is that it exists at all?
> >(eg why does ACPI need to have it's own locking primitives...)
>
> Because the ACPI spec defines its own locking protocol for
> synchronization between the OS and the BIOS.
... which can't be written based on linux locks ?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: ACPI global lock macros
2003-12-09 9:43 ` Arjan van de Ven
@ 2003-12-09 9:50 ` Paul Menage
0 siblings, 0 replies; 15+ messages in thread
From: Paul Menage @ 2003-12-09 9:50 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: agrover, linux-kernel, acpi-devel
Arjan van de Ven wrote:
>>>maybe the odd thing is that it exists at all?
>>>(eg why does ACPI need to have it's own locking primitives...)
>>
>>Because the ACPI spec defines its own locking protocol for
>>synchronization between the OS and the BIOS.
>
>
> ... which can't be written based on linux locks ?
I assume (hope!) there's already a higher-level linux lock serializing
access to acpi_acquire_global_lock() although I've not delved deeply
into the code. This is the lock described on p112 of
http://www.acpi.info/DOWNLOADS/ACPIspec-2-0c.pdf, which has the
semantics that if the OS wants to take the lock while the BIOS holds it,
it sets a bit and waits for an interrupt from the BIOS. I don't see that
it could be naturally implemented using a linux lock.
Paul
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [ACPI] ACPI global lock macros
2003-12-09 9:22 ACPI global lock macros Paul Menage
2003-12-09 9:36 ` Arjan van de Ven
@ 2003-12-10 7:45 ` Andi Kleen
1 sibling, 0 replies; 15+ messages in thread
From: Andi Kleen @ 2003-12-10 7:45 UTC (permalink / raw)
To: Paul Menage; +Cc: agrover, linux-kernel, acpi-devel
On Tue, 09 Dec 2003 01:22:09 -0800
Paul Menage <menage@google.com> wrote:
> Hi Andy,
>
> The ACPI_ACQUIRE_GLOBAL_LOCK() macro in include/asm-i386/acpi.h looks a
> little odd:
[...]
Thanks. I fixed the x86-64 version. RELEASE also needed similar treatment.
> 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:
Numeric asm parameters are always evil and cause such bugs all the time.
gcc 3.2+ has named asm parameters which makes this much cleaner and less error prone.
Unfortunately they cannot be used in i386 because there are still people who insist
on using ancient compilers :-(
-Andi
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [ACPI] ACPI global lock macros
2003-12-11 7:27 Yu, Luming
@ 2003-12-11 17:46 ` Nate Lawson
0 siblings, 0 replies; 15+ messages in thread
From: Nate Lawson @ 2003-12-11 17:46 UTC (permalink / raw)
To: Yu, Luming; +Cc: Paul Menage, agrover, linux-kernel, acpi-devel
On Thu, 11 Dec 2003, Yu, Luming wrote:
> -----Original Message-----
> From: acpi-devel-admin@lists.sourceforge.net [mailto:acpi-devel-admin@lists.sourceforge.net]On Behalf Of Yu, Luming
> Sent: 2003?12?11? 15:06
> To: Paul Menage; agrover@groveronline.com
> Cc: linux-kernel@vger.kernel.org; acpi-devel@lists.sourceforge.net
> Subject: RE: [ACPI] ACPI global lock macros
>
>
> >>#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"); \
FYI, that's what we do in FreeBSD also. The only difference after your
patch is that we use +m for GLptr.
#define ACPI_ACQUIRE_GLOBAL_LOCK(GLptr, Acq) \
do { \
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), "+m" (GLptr) : "i" (~1L) : "edx"); \
} while(0)
-Nate
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [ACPI] ACPI global lock macros
2003-12-11 8:07 ` Andi Kleen
@ 2003-12-11 8:27 ` Paul Menage
0 siblings, 0 replies; 15+ messages in thread
From: Paul Menage @ 2003-12-11 8:27 UTC (permalink / raw)
To: Andi Kleen; +Cc: Yu, Luming, linux-kernel, acpi-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [ACPI] ACPI global lock macros
2003-12-11 7:06 Yu, Luming
2003-12-11 8:07 ` Andi Kleen
@ 2003-12-11 8:20 ` Paul Menage
1 sibling, 0 replies; 15+ messages in thread
From: Paul Menage @ 2003-12-11 8:20 UTC (permalink / raw)
To: Yu, Luming; +Cc: linux-kernel, acpi-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [ACPI] ACPI global lock macros
2003-12-11 7:06 Yu, Luming
@ 2003-12-11 8:07 ` Andi Kleen
2003-12-11 8:27 ` Paul Menage
2003-12-11 8:20 ` Paul Menage
1 sibling, 1 reply; 15+ messages in thread
From: Andi Kleen @ 2003-12-11 8:07 UTC (permalink / raw)
To: Yu, Luming; +Cc: menage, agrover, linux-kernel, acpi-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [ACPI] ACPI global lock macros
@ 2003-12-11 7:27 Yu, Luming
2003-12-11 17:46 ` Nate Lawson
0 siblings, 1 reply; 15+ messages in thread
From: Yu, Luming @ 2003-12-11 7:27 UTC (permalink / raw)
To: Paul Menage, agrover; +Cc: linux-kernel, acpi-devel
I have filed a tracker http://bugme.osdl.org/show_bug.cgi?id=1669 , And a proposal patch based on Paul's proposal filed there.
--Luming
-----Original Message-----
From: acpi-devel-admin@lists.sourceforge.net [mailto:acpi-devel-admin@lists.sourceforge.net]On Behalf Of Yu, Luming
Sent: 2003?12?11? 15:06
To: Paul Menage; agrover@groveronline.com
Cc: linux-kernel@vger.kernel.org; acpi-devel@lists.sourceforge.net
Subject: RE: [ACPI] ACPI global lock macros
>>#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/
_______________________________________________
Acpi-devel mailing list
Acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/acpi-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [ACPI] ACPI global lock macros
@ 2003-12-11 7:06 Yu, Luming
2003-12-11 8:07 ` Andi Kleen
2003-12-11 8:20 ` Paul Menage
0 siblings, 2 replies; 15+ messages in thread
From: Yu, Luming @ 2003-12-11 7:06 UTC (permalink / raw)
To: Paul Menage, agrover; +Cc: linux-kernel, acpi-devel
>>#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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [ACPI] ACPI global lock macros
2003-12-09 18:20 Grover, Andrew
@ 2003-12-09 19:04 ` Paul Menage
2003-12-09 19:03 ` David Mosberger
0 siblings, 1 reply; 15+ messages in thread
From: Paul Menage @ 2003-12-09 19:04 UTC (permalink / raw)
To: Grover, Andrew; +Cc: linux-kernel, acpi-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [ACPI] ACPI global lock macros
2003-12-09 19:04 ` Paul Menage
@ 2003-12-09 19:03 ` David Mosberger
0 siblings, 0 replies; 15+ messages in thread
From: David Mosberger @ 2003-12-09 19:03 UTC (permalink / raw)
To: Paul Menage; +Cc: Grover, Andrew, linux-kernel, acpi-devel
>>>>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [ACPI] ACPI global lock macros
@ 2003-12-09 18:20 Grover, Andrew
2003-12-09 19:04 ` Paul Menage
0 siblings, 1 reply; 15+ messages in thread
From: Grover, Andrew @ 2003-12-09 18:20 UTC (permalink / raw)
To: Paul Menage; +Cc: linux-kernel, acpi-devel
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2003-12-11 17:46 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-12-09 9:22 ACPI global lock macros Paul Menage
2003-12-09 9:36 ` Arjan van de Ven
2003-12-09 9:42 ` Paul Menage
2003-12-09 9:43 ` Arjan van de Ven
2003-12-09 9:50 ` Paul Menage
2003-12-10 7:45 ` [ACPI] " Andi Kleen
2003-12-09 18:20 Grover, Andrew
2003-12-09 19:04 ` Paul Menage
2003-12-09 19:03 ` David Mosberger
2003-12-11 7:06 Yu, Luming
2003-12-11 8:07 ` Andi Kleen
2003-12-11 8:27 ` Paul Menage
2003-12-11 8:20 ` Paul Menage
2003-12-11 7:27 Yu, Luming
2003-12-11 17:46 ` Nate Lawson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).