Commit 709f744f18ebc3a810d29c8d5502bf20c3cecc70

Authored by Jan Beulich
Committed by Ingo Molnar
1 parent 6e908947b4

x86: bitops asm constraint fixes

This (simplified) piece of code didn't behave as expected due to
incorrect constraints in some of the bitops functions, when
X86_FEATURE_xxx is referring to other than the first long:

int test(struct cpuinfo_x86 *c) {
	if (cpu_has(c, X86_FEATURE_xxx))
		clear_cpu_cap(c, X86_FEATURE_xxx);
	return cpu_has(c, X86_FEATURE_xxx);
}

I'd really like understand, though, what the policy of (not) having a
"memory" clobber in these operations is - currently, this appears to
be totally inconsistent. Also, many comments of the non-atomic
functions say those may also be re-ordered - this contradicts the use
of "asm volatile" in there, which again I'd like to understand.

As much as all of these, using 'int' for the 'nr' parameter and
'void *' for the 'addr' one is in conflict with
Documentation/atomic_ops.txt, especially because bt{,c,r,s} indeed
take the bit index as signed (which hence would really need special
precaution) and access the full 32 bits (if 'unsigned long' was used
properly here, 64 bits for x86-64) pointed at, so invalid uses like
referencing a 'char' array cannot currently be caught.

Finally, the code with and without this patch relies heavily on the
-fno-strict-aliasing compiler switch and I'm not certain this really
is a good idea.

In the light of all of this I'm sending this as RFC, as fixing the
above might warrant a much bigger patch...

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

Showing 1 changed file with 24 additions and 19 deletions Side-by-side Diff

include/asm-x86/bitops.h
... ... @@ -24,9 +24,12 @@
24 24 /* Technically wrong, but this avoids compilation errors on some gcc
25 25 versions. */
26 26 #define ADDR "=m" (*(volatile long *) addr)
  27 +#define BIT_ADDR "=m" (((volatile int *) addr)[nr >> 5])
27 28 #else
28 29 #define ADDR "+m" (*(volatile long *) addr)
  30 +#define BIT_ADDR "+m" (((volatile int *) addr)[nr >> 5])
29 31 #endif
  32 +#define BASE_ADDR "m" (*(volatile int *) addr)
30 33  
31 34 /**
32 35 * set_bit - Atomically set a bit in memory
... ... @@ -79,9 +82,8 @@
79 82 */
80 83 static inline void clear_bit(int nr, volatile void *addr)
81 84 {
82   - asm volatile(LOCK_PREFIX "btr %1,%0"
83   - : ADDR
84   - : "Ir" (nr));
  85 + asm volatile(LOCK_PREFIX "btr %1,%2"
  86 + : BIT_ADDR : "Ir" (nr), BASE_ADDR);
85 87 }
86 88  
87 89 /*
... ... @@ -100,7 +102,7 @@
100 102  
101 103 static inline void __clear_bit(int nr, volatile void *addr)
102 104 {
103   - asm volatile("btr %1,%0" : ADDR : "Ir" (nr));
  105 + asm volatile("btr %1,%2" : BIT_ADDR : "Ir" (nr), BASE_ADDR);
104 106 }
105 107  
106 108 /*
... ... @@ -135,7 +137,7 @@
135 137 */
136 138 static inline void __change_bit(int nr, volatile void *addr)
137 139 {
138   - asm volatile("btc %1,%0" : ADDR : "Ir" (nr));
  140 + asm volatile("btc %1,%2" : BIT_ADDR : "Ir" (nr), BASE_ADDR);
139 141 }
140 142  
141 143 /**
... ... @@ -149,8 +151,8 @@
149 151 */
150 152 static inline void change_bit(int nr, volatile void *addr)
151 153 {
152   - asm volatile(LOCK_PREFIX "btc %1,%0"
153   - : ADDR : "Ir" (nr));
  154 + asm volatile(LOCK_PREFIX "btc %1,%2"
  155 + : BIT_ADDR : "Ir" (nr), BASE_ADDR);
154 156 }
155 157  
156 158 /**
... ... @@ -198,10 +200,10 @@
198 200 {
199 201 int oldbit;
200 202  
201   - asm("bts %2,%1\n\t"
202   - "sbb %0,%0"
203   - : "=r" (oldbit), ADDR
204   - : "Ir" (nr));
  203 + asm volatile("bts %2,%3\n\t"
  204 + "sbb %0,%0"
  205 + : "=r" (oldbit), BIT_ADDR
  206 + : "Ir" (nr), BASE_ADDR);
205 207 return oldbit;
206 208 }
207 209  
208 210  
... ... @@ -238,10 +240,10 @@
238 240 {
239 241 int oldbit;
240 242  
241   - asm volatile("btr %2,%1\n\t"
  243 + asm volatile("btr %2,%3\n\t"
242 244 "sbb %0,%0"
243   - : "=r" (oldbit), ADDR
244   - : "Ir" (nr));
  245 + : "=r" (oldbit), BIT_ADDR
  246 + : "Ir" (nr), BASE_ADDR);
245 247 return oldbit;
246 248 }
247 249  
248 250  
... ... @@ -250,10 +252,10 @@
250 252 {
251 253 int oldbit;
252 254  
253   - asm volatile("btc %2,%1\n\t"
  255 + asm volatile("btc %2,%3\n\t"
254 256 "sbb %0,%0"
255   - : "=r" (oldbit), ADDR
256   - : "Ir" (nr) : "memory");
  257 + : "=r" (oldbit), BIT_ADDR
  258 + : "Ir" (nr), BASE_ADDR);
257 259  
258 260 return oldbit;
259 261 }
260 262  
... ... @@ -288,10 +290,11 @@
288 290 {
289 291 int oldbit;
290 292  
291   - asm volatile("bt %2,%1\n\t"
  293 + asm volatile("bt %2,%3\n\t"
292 294 "sbb %0,%0"
293 295 : "=r" (oldbit)
294   - : "m" (*(unsigned long *)addr), "Ir" (nr));
  296 + : "m" (((volatile const int *)addr)[nr >> 5]),
  297 + "Ir" (nr), BASE_ADDR);
295 298  
296 299 return oldbit;
297 300 }
... ... @@ -310,6 +313,8 @@
310 313 constant_test_bit((nr),(addr)) : \
311 314 variable_test_bit((nr),(addr)))
312 315  
  316 +#undef BASE_ADDR
  317 +#undef BIT_ADDR
313 318 #undef ADDR
314 319  
315 320 #ifdef CONFIG_X86_32