Use wbinvd() macro instead of raw inline assembly in .c files

This patch uses the already-existant wbinvd() macro to replace raw assembly to perform this very same task in some .c files

Signed-off-by Glauber de Oliveira Costa <[email protected]>

arch/x86_64/kernel/tce.c +0 -0

index f61fb8e..afbb951 100644

arch/x86_64/kernel/tce.c +0 -0

 }
 
 void tce_build(struct iommu_table *tbl, unsigned long index,

arch/x86_64/mm/pageattr.c +0 -0

index 9148f4a..0a75790 100644

Quoted 5 lines · On Friday 20 July 2007 00:55:40 Glauber de Oliveira Costa wrote:

This patch uses the already-existant wbinvd() macro to replace raw assembly to perform this very same task in some .c files

Signed-off-by Glauber de Oliveira Costa <[email protected]>

arch/x86_64/kernel/tce.c static inline void flush_tce(void* tceaddr) · line 45

Quoted 10 lines

arch/x86_64/kernel/tce.c +1 -1

index f61fb8e..afbb951 100644
@@ -42,7 +42,7 @@ static inline void flush_tce(void* tceaddr)
 	if (cpu_has_clflush)
 		asm volatile("clflush (%0)" :: "r" (tceaddr));
 	else
-		asm volatile("wbinvd":::"memory");
+		wbinvd();

I guess it can be just removed there. I don' think there are any calgary machines without clflush

Quoted 6 lines · Andi Kleen

Quoted 5 lines · On Friday 20 July 2007 00:55:40 Glauber de Oliveira Costa wrote:

This patch uses the already-existant wbinvd() macro to replace raw assembly to perform this very same task in some .c files

Signed-off-by Glauber de Oliveira Costa <[email protected]>

Quoted 14 lines

arch/x86_64/kernel/tce.c static inline void flush_tce(void* tceaddr) · line 45

Quoted 10 lines

arch/x86_64/kernel/tce.c +1 -1

index f61fb8e..afbb951 100644
@@ -42,7 +42,7 @@ static inline void flush_tce(void* tceaddr)
 	if (cpu_has_clflush)
 		asm volatile("clflush (%0)" :: "r" (tceaddr));
 	else
-		asm volatile("wbinvd":::"memory");
+		wbinvd();

I guess it can be just removed there. I don' think there are any calgary machines without clflush

That seems like an unsafe thing to do (unless we err out somewhere); lest there is, say, a microcode update disabling clflush due to an erratum.

Create an inline function for clflush(), with the proper arguments, and use it instead of hard-coding the instruction.

This also removes one instance of hard-coded wbinvd, based on a patch by Bauder de Oliveira Costa.

Quoted 5 lines · H. Peter Anvin

Create an inline function for clflush(), with the proper arguments, and use it instead of hard-coding the instruction.

This also removes one instance of hard-coded wbinvd, based on a patch by Bauder de Oliveira Costa.

I don't see much sense in it. CLFLUSH is not priviledged, paravirt doesn't need to change and this adds just an unnecessary layer of abstraction.

Quoted 4 lines · Andi Kleen

Quoted 3 lines · H. Peter Anvin

Create an inline function for clflush(), with the proper arguments, and use it instead of hard-coding the instruction.

Quoted 5 lines

Quoted 2 lines

This also removes one instance of hard-coded wbinvd, based on a patch by Bauder de Oliveira Costa.

I don't see much sense in it. CLFLUSH is not priviledged, paravirt doesn't need to change and this adds just an unnecessary layer of abstraction.

The main reason is that everyone seems to invoke it either incorrectly

Quoted 1 line

The main reason is that everyone seems to invoke it either incorrectly

Where is it incorrect?

Quoted 2 lines · Andi Kleen

Quoted 1 line

The main reason is that everyone seems to invoke it either incorrectly

Where is it incorrect?

Using "r" or "m" (as opposed as "+m") gives gcc the wrong dependency information, and it could, at least in theory, cause memory references to be moved around it -- especially when using "r".

Quoted 6 lines · H. Peter Anvin

Quoted 2 lines · Andi Kleen

Quoted 1 line

The main reason is that everyone seems to invoke it either incorrectly

Where is it incorrect?

Using "r" or "m" (as opposed as "+m") gives gcc the wrong dependency information, and it could, at least in theory, cause memory references to be moved around it -- especially when using "r".

That doesn't sound correct. Taking the address should be like an escaping pointer and equivalent to read/write and effectively disable optimizations on this memory.

Create an inline function for clflush(), with the proper arguments, and use it instead of hard-coding the instruction.

This also removes one instance of hard-coded wbinvd, based on a patch by Bauder de Oliveira Costa.

Cc Andi Kleen <[email protected]>

Cc Glauber de Oliveira Costa <[email protected]>

Signed-off-by H. Peter Anvin <[email protected]>

Quoted 5 lines · H. Peter Anvin

Create an inline function for clflush(), with the proper arguments, and use it instead of hard-coding the instruction.

This also removes one instance of hard-coded wbinvd, based on a patch by Bauder de Oliveira Costa.

Hey, Who's that guy that got a name so close to mine? ;-)

Quoted 2 lines

Cc Andi Kleen <[email protected]>

Cc Glauber de Oliveira Costa <[email protected]>

Create an inline function for clflush(), with the proper arguments, and use it instead of hard-coding the instruction.

This also removes one instance of hard-coded wbinvd, based on a patch by Bauder de Oliveira Costa.

Cc Andi Kleen <[email protected]>

Cc Glauber de Oliveira Costa <[email protected]>

Signed-off-by H. Peter Anvin <[email protected]>

Mostly looks good, but see below.

Quoted 7 lines

drivers/char/agp/efficeon-agp.c +0 -0

index df8da72..41f46df 100644
@@ -221,7 +221,7 @@ static int efficeon_create_gatt_table(struct agp_bridge_data *bridge)
 		SetPageReserved(virt_to_page((char *)page));
 

drivers/char/agp/efficeon-agp.c static int efficeon_create_gatt_table(struct agp_bridge_data *bridge) · line 224

Quoted 3 lines

drivers/char/agp/efficeon-agp.c +1 -1

 		for (offset = 0; offset < PAGE_SIZE; offset += clflush_chunk)
-			asm volatile("clflush %0" : : "m" (*(char *)(page+offset)));
+			clflush((char *)page+offset);

The original code flushes (*(page+offset)) whereas the new code flushes (pagee+offset). Assuming this is a bug-fix, it should go as a separate patch.

Quoted 10 lines · Muli Ben-Yehuda

Mostly looks good, but see below.

Quoted 7 lines

drivers/char/agp/efficeon-agp.c +0 -0

index df8da72..41f46df 100644
@@ -221,7 +221,7 @@ static int efficeon_create_gatt_table(struct agp_bridge_data *bridge)
 		SetPageReserved(virt_to_page((char *)page));
 

Quoted 7 lines

drivers/char/agp/efficeon-agp.c static int efficeon_create_gatt_table(struct agp_bridge_data *bridge) · line 224

Quoted 3 lines

drivers/char/agp/efficeon-agp.c +1 -1

 		for (offset = 0; offset < PAGE_SIZE; offset += clflush_chunk)
-			asm volatile("clflush %0" : : "m" (*(char *)(page+offset)));
+			clflush((char *)page+offset);

The original code flushes (*(page+offset)) whereas the new code flushes (pagee+offset). Assuming this is a bug-fix, it should go as a separate patch.

No, it doesn't. There is a * in the inline, as there should be.

So it looks like we have a purely syntactic patch that does something different than the original code in one of the above places. What am I missing?

Ok, let's try again:

You're changing this (pageattr.c)

                asm volatile("clflush (%0)" :: "r" (adr + i));

into this:

		asm volatile("clflush %0" : "+m" (*(char __force*)(adr + i)));

The original one calls clflush with (adr + i), the new one with (*(adr
+ i)). Are these calls equivalent?

Quoted 4 lines · Muli Ben-Yehuda

Ok, let's try again:

You're changing this (pageattr.c)

Quoted 8 lines

                asm volatile("clflush (%0)" :: "r" (adr + i));

into this:

		asm volatile("clflush %0" : "+m" (*(char __force*)(adr + i)));

The original one calls clflush with (adr + i), the new one with (*(adr
+ i)). Are these calls equivalent?

Yes, they are. The parentheses which are part of the old assembly string has the same effect as the asterisk operator in C.

The difference between the two is that the latter form allows the C compiler to select the addressing mode, which allows the full range of addressing modes, whereas the former forces it to use a single register indirect.

Quoted 9 lines · Muli Ben-Yehuda

Ok, let's try again:

You're changing this (pageattr.c)

                asm volatile("clflush (%0)" :: "r" (adr + i));

into this:

Quoted 4 lines

		asm volatile("clflush %0" : "+m" (*(char __force*)(adr + i)));

The original one calls clflush with (adr + i), the new one with (*(adr
+ i)). Are these calls equivalent?

Yes, they are. The parentheses which are part of the old assembly string has the same effect as the asterisk operator in C.