Hi all,
x86 memory-mapped IO register accessors cast the memory mapped address
parameter to a one with the 'volatile' type qualifier. For example, here
is readb() after cpp processing
--> arch/x86/include/asm/io.h:
static inline unsigned char readb(const volatile void __iomem *addr) {
unsigned char ret;
asm volatile("movb %1, %0"
:"=q" (ret)
:"m" (*(volatile unsigned char __force *)addr)
:"memory");
return ret;
}
I wonder if the volatile qualifiers in the parameter above and at the asm
statement operand were strictly necessary, or just added for extra safety.
AFAIK, the asm statement already functions as a compiler barrier, and the
compiler won't 'optimize' the statement away due to the 'asm volatile' part,
so shouldn't things be safe without those volatile qualifiers?
The only red-herring I found in the gcc manual was the fact that the
"volatile asm instruction can be moved relative to other code, including
across jump instructions."
I wonder if this was the reason a volatile-type data dependency was added
to the mov{b,w,l,q} asm statements; not to reorder the asm instruction
around non-memory-accessing instructions (we already have a barrier).
Thank you!
--
Darwish
> x86 memory-mapped IO register accessors cast the memory mapped address
> parameter to a one with the 'volatile' type qualifier. For example,
> here
> is readb() after cpp processing
>
> --> arch/x86/include/asm/io.h:
>
> static inline unsigned char readb(const volatile void __iomem *addr) {
This "volatile" is meaningless.
> unsigned char ret;
> asm volatile("movb %1, %0"
This "volatile" is required; without it, if "ret" isn't used (or can
be optimised away), the asm() could be optimised away.
> :"=q" (ret)
> :"m" (*(volatile unsigned char __force *)addr)
This "volatile" has no effect, since the asm has a "memory" clobber.
Without that clobber, this "volatile" would prevent moving the asm
over other memory accesses.
If you want to get all language-lawyery, if the object pointed to by
"addr" is volatile, the volatile here _is_ needed: accessing volatile
objects via a not volatile-qualified lvalue is undefined. But since
this is GCC-specific code anyway, do you care? :-)
> :"memory");
> return ret;
> }
Segher
On Friday 04 December 2009, Segher Boessenkool wrote:
> If you want to get all language-lawyery, if the object pointed to by
> "addr" is volatile, the volatile here is needed: accessing volatile
> objects via a not volatile-qualified lvalue is undefined. But since
> this is GCC-specific code anyway, do you care? :-)
I think the real reason for having it is to avoid a warning when
device drivers pass volatile objects. Not sure if that's a good
thing or if we should better actually warn about it.
Arnd <><
On 12/04/2009 06:39 AM, Segher Boessenkool wrote:
>> x86 memory-mapped IO register accessors cast the memory mapped address
>> parameter to a one with the 'volatile' type qualifier. For example, here
>> is readb() after cpp processing
>>
>> --> arch/x86/include/asm/io.h:
>>
>> static inline unsigned char readb(const volatile void __iomem *addr) {
>
> This "volatile" is meaningless.
Wrong. "volatile" here is an assertion that it is safe to pass pointer
to a volatile object to this function.
>> unsigned char ret;
>> asm volatile("movb %1, %0"
>
> This "volatile" is required; without it, if "ret" isn't used (or can
> be optimised away), the asm() could be optimised away.
>
>> :"=q" (ret)
>> :"m" (*(volatile unsigned char __force *)addr)
>
> This "volatile" has no effect, since the asm has a "memory" clobber.
> Without that clobber, this "volatile" would prevent moving the asm
> over other memory accesses.
>
> If you want to get all language-lawyery, if the object pointed to by
> "addr" is volatile, the volatile here _is_ needed: accessing volatile
> objects via a not volatile-qualified lvalue is undefined. But since
> this is GCC-specific code anyway, do you care? :-)
Again, this comes from the prototype being volatile.
Either way, it works, it is guaranteed to be safe, and removing it can
only introduce bugs, not remove them.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
>>> static inline unsigned char readb(const volatile void __iomem *addr) {
>>
>> This "volatile" is meaningless.
>
> Wrong. "volatile" here is an assertion that it is safe to pass pointer
> to a volatile object to this function.
Yes, sorry. What I meant is: this volatile has no effect on what
the rest of the function does.
> Either way, it works, it is guaranteed to be safe, and removing it can
> only introduce bugs, not remove them.
Oh definitely, I wasn't suggesting otherwise.
Segher