From: "Jan Beulich" <jbeulich@novell.com>
To: xen-devel@lists.xensource.com
Subject: [PATCH] x86: fix variable_test_bit() asm constraints
Date: Fri, 14 Mar 2008 11:23:47 +0000 [thread overview]
Message-ID: <47DA6E53.76E4.0078.0@novell.com> (raw)
I just sent a (much bigger, see below) patch to the same effect to the
x86 Linux maintainers - in Xen, all the operations modifying bits do
have "memory" clobbers, so it's just the test_bit() constraint that's
wrong.
However, I wonder whether the non-atomic ops aren't limiting things too
much by having "memory" clobbers, they would much better be restricted
to indicate just the changing memory location. This, however, would
probably require some additional consideration given that Xen (other
than Linux) isn't using -fno-strict-aliasing. Furthermore, these
non-atomic operations, according to their comments, can be re-ordered,
which contradicts the use of __asm__ __volatile__ (but note that
removing this would probably require extra precautions to meet strict
aliasing rules).
Further, using 'void *' for the 'addr' parameter appears dangerous,
since bt{,c,r,s} 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 I find the leading 'd' constraints in the 'nr' assembly
operands quite odd - what is the purpose of that? Linux is using just
"Ir" here...
Signed-off-by: Jan Beulich <jbeulich@novell.com>
Index: 2008-03-05/xen/include/asm-x86/bitops.h
===================================================================
--- 2008-03-05.orig/xen/include/asm-x86/bitops.h 2007-09-14 11:03:32.000000000 +0200
+++ 2008-03-05/xen/include/asm-x86/bitops.h 2008-03-13 10:20:34.000000000 +0100
@@ -254,7 +254,8 @@ static __inline__ int variable_test_bit(
__asm__ __volatile__(
"btl %2,%1\n\tsbbl %0,%0"
:"=r" (oldbit)
- :"m" (CONST_ADDR),"dIr" (nr));
+ :"m" (CONST_ADDR), "dIr" (nr),
+ "m" (((const volatile int *)addr)[nr >> 5]));
return oldbit;
}
next reply other threads:[~2008-03-14 11:23 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-03-14 11:23 Jan Beulich [this message]
2008-03-14 11:55 ` [PATCH] x86: fix variable_test_bit() asm constraints Keir Fraser
2008-03-14 11:59 ` Keir Fraser
2008-03-14 13:46 ` [PATCH] x86: fix variable_test_bit() asmconstraints Jan Beulich
2008-03-14 12:06 ` [PATCH] x86: fix variable_test_bit() asm constraints Keir Fraser
2008-03-14 13:51 ` [PATCH] x86: fix variable_test_bit() asmconstraints Jan Beulich
2008-03-14 13:57 ` Keir Fraser
2008-03-14 14:11 ` [PATCH] x86: fix variable_test_bit()asmconstraints Jan Beulich
2008-03-14 15:37 ` Keir Fraser
2008-03-14 16:42 ` Jan Beulich
2008-03-14 17:08 ` Keir Fraser
2008-03-16 14:08 ` Keir Fraser
2008-03-14 13:59 ` [PATCH] x86: fix variable_test_bit() asmconstraints Samuel Thibault
2008-03-14 14:04 ` Keir Fraser
2008-03-14 14:18 ` Jan Beulich
2008-03-14 15:17 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=47DA6E53.76E4.0078.0@novell.com \
--to=jbeulich@novell.com \
--cc=xen-devel@lists.xensource.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.