mark read_crX() asm code as volatile

Some gcc versions (I checked at least 4.1.1 from RHEL5 & 4.1.2 from gentoo) can generate incorrect code with read_crX()/write_crX() functions mix up, due to cached results of read_crX().

The small app for x8664 below compiled with -O2 demonstrates this (i686 does the same thing):

------------------- cut -----------------------
static inline unsigned long read_cr3(void)

{
        unsigned long cr3;
        asm("movq %%cr3,%0" : "=r" (cr3));
        return cr3;
}

static inline void write_cr3(unsigned long val)

{
        asm volatile("movq %0,%%cr3" :: "r" (val) : "memory");
}

void main()

{
        unsigned long c;
        c = read_cr3();
        write_cr3(c | 0x80);
        c = read_cr3();
        write_cr3(c | 0x100);
}

------------------- cut -----------------------

# objdump -dr tst
                ....
0000000000400430 <main>:
  400430:       0f 20 d8                mov    %cr3,%rax
  400433:       48 89 c2                mov    %rax,%rdx
  400436:       80 ca 80                or     $0x80,%dl
  400439:       0f 22 da                mov    %rdx,%cr3
  40043c:       80 cc 01                or     $0x1,%ah
  40043f:       0f 22 d8                mov    %rax,%cr3
  400442:       c3                      retq
                ....

As one can notice, cr3 value is incorrectly read only once, and finally updated with only one bit set.

So better be on the safe side and mark all asm statements in read_crX() functions as volatile which helps.
i686 already has most of these functions marked as volatile already.

I faced this bug myself in i686 arch code when did code rearrangement in 2.6.18.

---

 asm-i386/system.h   |    2 +-
 asm-x86_64/system.h |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

@@ -141,7 +141,7 @@ static inline unsigned long native_read_
 {
 	unsigned long val;
 	/* This could fault if %cr4 does not exist */
-	asm("1: movl %%cr4, %0		\n"
+	asm volatile("1: movl %%cr4, %0		\n"
 		"2:				\n"
 		".section __ex_table,\"a\"	\n"
 		".long 1b,2b			\n"

@@ -85,7 +85,7 @@ static inline void write_cr0(unsigned lo
 static inline unsigned long read_cr2(void)
 {
 	unsigned long cr2;
-	asm("movq %%cr2,%0" : "=r" (cr2));
+	asm volatile("movq %%cr2,%0" : "=r" (cr2));
 	return cr2;
 }
 

@@ -97,7 +97,7 @@ static inline void write_cr2(unsigned lo
 static inline unsigned long read_cr3(void)
 { 
 	unsigned long cr3;
-	asm("movq %%cr3,%0" : "=r" (cr3));
+	asm volatile("movq %%cr3,%0" : "=r" (cr3));
 	return cr3;
 }
 

@@ -109,7 +109,7 @@ static inline void write_cr3(unsigned lo
 static inline unsigned long read_cr4(void)
 { 
 	unsigned long cr4;
-	asm("movq %%cr4,%0" : "=r" (cr4));
+	asm volatile("movq %%cr4,%0" : "=r" (cr4));
 	return cr4;
 }
 

@@ -121,7 +121,7 @@ static inline void write_cr4(unsigned lo
 static inline unsigned long read_cr8(void)
 {
 	unsigned long cr8;
-	asm("movq %%cr8,%0" : "=r" (cr8));
+	asm volatile("movq %%cr8,%0" : "=r" (cr8));
 	return cr8;
 }
 

added thanks

-Andi

I'm not so sure volatile is the right answer, as compared to giving the asm more strict contraints....

asm volatile tends to mean something else than "the result has changed"....

It means "don't eliminate this code if it's reachable" which should be just enough for this case. But it could still be reordered in some cases that could break, I think.

This should work because the result gets used before reading again:

read_cr3(a);
write_cr3(a | 1);
read_cr3(a);

But this might be reordered so that b gets read before the write:

read_cr3(a);
write_cr3(a | 1);
read_cr3(b);

?

I don't see how, as write_cr3 clobbers memory.

Because read_cr3() doesn't depend on memory, and b could be stored in a register.

-hpa

How does the compiler know it doesn't depend on memory?

How do you say it depends on memory? You really need something as heavy as volatile?

When it has no m (or equivalent like g) constrained argument and no memory clobber.

You add any of the above.

You could do a memory clobber, but it would be heavier than the volatile because the memory clobber clobbers all cached variables. volatile essentially just says "don't remove; has side effects". Normally gcc does that automatically for something without outputs, but this one has.

Besides a CRx access does not actually clobber memory.

-Andi

One of the aspect of volatility is "the result will change in ways you (gcc) just don't understand."

-hpa

Arjan,

I can experiment with any constraints if you suggest which one.

to be merged! Sigh...
Below are 2 programs which differ in one space in read_cr3_b() asm statement. The first one compiles incorrectly, while 2nd one - correctly.

My personal feeling is that comparing asm strings is simply a "misfeature".

-------------------------- cut ---------------------------- static inline unsigned long read_cr3_a(void)

{
        unsigned long cr3;
        asm("movq %%cr3,%0" : "=r" (cr3));
        return cr3;
}

static inline unsigned long read_cr3_b(void)

{
        unsigned long cr3;
        asm("movq %%cr3,%0" : "=r" (cr3));
        return cr3;
}

static inline void write_cr3(unsigned long val)

{
        asm volatile("movq %0,%%cr3" :: "r" (val) : "memory");
}

void main()

{
        unsigned long c;
        c = read_cr3_a();
        write_cr3(c | 0x80);
        c = read_cr3_b();
        write_cr3(c | 0x100);
}

-------------------------- cut ----------------------------

-------------------------- cut ---------------------------- static inline unsigned long read_cr3_a(void)

{
        unsigned long cr3;
        asm("movq %%cr3,%0" : "=r" (cr3));
        return cr3;
}

static inline unsigned long read_cr3_b(void)

{
        unsigned long cr3;
        asm("movq  %%cr3,%0" : "=r" (cr3));
        return cr3;
}

static inline void write_cr3(unsigned long val)

{
        asm volatile("movq %0,%%cr3" :: "r" (val) : "memory");
}

void main()

{
        unsigned long c;
        c = read_cr3_a();
        write_cr3(c | 0x80);
        c = read_cr3_b();
        write_cr3(c | 0x100);
}

-------------------------- cut ----------------------------

Kirill