KVM: pvmmu breakage with gcc 4.3.0

Some pvmmu functions store their commands on stack, and newer GCC versions conclude that these commands are unused.

So stick an inline asm statement to convince the compiler otherwise.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kernel/kvm.c +6 -1

index 8b7a3cf..c892752 100644
@@ -55,6 +55,12 @@ static void kvm_mmu_op(void *buffer, unsigned len)
 	int r;
 	unsigned long a1, a2;
 
+	/*
+   	 * GCC 4.3.0 concludes that on-stack kvm_mmu_op* is unused and
+   	 * optimizes its initialization away.
+ 	 */
+        asm ("" : : "p" (buffer));
+
 	do {
 		a1 = __pa(buffer);
 		a2 = 0;   /* on i386 __pa() always returns <4G */
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Marcelo Tosatti

Some pvmmu functions store their commands on stack, and newer GCC versions conclude that these commands are unused.

So stick an inline asm statement to convince the compiler otherwise.

I think a better fix is to add a "memory " clobber to the hypercalls. This isn't really a GCC bug since it doesn't realize that hypercalls can touch memory.

I think a better fix is to add a "memory " clobber to the hypercalls.

I thought a better approach
was to have __pa do something along the lines of

({ char *__buf = (char*)buffer; asm ("" : : "o"(*__buf)); ... })

to make it clear that physical addresses are used.

Some pvmmu functions store their commands on stack, and newer GCC versions conclude that these commands are unused.

So stick an inline asm statement to convince the compiler otherwise.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kernel/kvm.c +0 -0

index 8b7a3cf..c892752 100644

Quoted 13 lines · Marcelo Tosatti

Some pvmmu functions store their commands on stack, and newer GCC versions conclude that these commands are unused.

So stick an inline asm statement to convince the compiler otherwise.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kernel/kvm.c +0 -0

index 8b7a3cf..c892752 100644

arch/x86/kernel/kvm.c static void kvm_mmu_op(void *buffer, unsigned len) · line 64

Quoted 10 lines

arch/x86/kernel/kvm.c +6 -0

@@ -55,6 +55,12 @@ static void kvm_mmu_op(void *buffer, unsigned len)
     int r;
     unsigned long a1, a2;
 
+    /*
+        * GCC 4.3.0 concludes that on-stack kvm_mmu_op* is unused and
+        * optimizes its initialization away.
+      */
+        asm ("" : : "p" (buffer));
+
 

I don't think "p" should force the contents into memory? Perhaps "m"(*(char *)buffer)?

Anthony, I don't see why a memory clobber would tell gcc that the variables is actually used.

The problem is with the void * -> unsigned long cast (__pa), once
that happens gcc loses track. It's probably needed anyway since hypercalls _do_ clobber memory.

Quoted 27 lines · Avi Kivity

Quoted 13 lines · Marcelo Tosatti

Some pvmmu functions store their commands on stack, and newer GCC versions conclude that these commands are unused.

So stick an inline asm statement to convince the compiler otherwise.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kernel/kvm.c +0 -0

index 8b7a3cf..c892752 100644

arch/x86/kernel/kvm.c static void kvm_mmu_op(void *buffer, unsigned len) · line 64

Quoted 10 lines

arch/x86/kernel/kvm.c +6 -0

@@ -55,6 +55,12 @@ static void kvm_mmu_op(void *buffer, unsigned len)
     int r;
     unsigned long a1, a2;
 
+    /*
+        * GCC 4.3.0 concludes that on-stack kvm_mmu_op* is unused and
+        * optimizes its initialization away.
+      */
+        asm ("" : : "p" (buffer));
+
 

I don't think "p" should force the contents into memory? Perhaps "m"(*(char *)buffer)?

Quoted 1 line

Anthony, I don't see why a memory clobber would tell gcc that the variables is actually used.

It doesn't, but it seems to me that it should force GCC to assume everything is live. It's a big stick to hit the problem with but it seems like the right thing semantically.

Quoted 2 lines

The problem is with the void * -> unsigned long cast (__pa), once
that happens gcc loses track. It's probably needed anyway since hypercalls _do_ clobber memory.

Right, it's not telling GCC that we touch a particular variable, but rather that we may have touched any variable.

I don't think "p" should force the contents into memory? Perhaps "m"(*(char *)buffer)?

Anthony, I don't see why a memory clobber would tell gcc that the variables is actually used. The problem is with the void * -> unsigned long cast (__pa), once that happens gcc loses track. It's probably needed anyway since hypercalls _do_ clobber memory.

Am Donnerstag, 26. Juni 2008 schrieb Avi Kivity:

Quoted 4 lines

I don't think "p" should force the contents into memory? Perhaps "m"(*(char *)buffer)?

Anthony, I don't see why a memory clobber would tell gcc that the variables is actually used. The problem is with the void * -> unsigned long cast (__pa), once that happens gcc loses track. It's probably needed anyway since hypercalls _do_ clobber memory.

I I think about that again, the correct solution should be to use 2 input constraints for parameters together with the memory clobber on hypercall.

I think something like the following covers all cases:

static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)

{
	long ret;
	asm volatile(KVM_HYPERCALL
		     : "=a"(ret)
		     : "a"(nr), "b"(p1), "m"(*(char *) p1)
		     : "memory" );
	return ret;
}

The address and the memory content of p1 (if it is a pointer) is not ommitted by gcc. Furthermore, the memory clobbering nature of a hypercall is specified as well.

Christian

Am Donnerstag, 26. Juni 2008 schrieb Avi Kivity:

Quoted 4 lines

I don't think "p" should force the contents into memory? Perhaps "m"(*(char *)buffer)?

Anthony, I don't see why a memory clobber would tell gcc that the variables is actually used. The problem is with the void * -> unsigned long cast (__pa), once that happens gcc loses track. It's probably needed anyway since hypercalls _do_ clobber memory.

I I think about that again, the correct solution should be to use 2 input constraints for parameters together with the memory clobber on hypercall.

I think something like the following covers all cases:

static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)

{
	long ret;
	asm volatile(KVM_HYPERCALL
		     : "=a"(ret)
		     : "a"(nr), "b"(p1), "m"(*(char *) p1)
		     : "memory" );
	return ret;
}

The address and the memory content of p1 (if it is a pointer) is not ommitted by gcc. Furthermore, the memory clobbering nature of a hypercall is specified as well.

Christian
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 17 lines · Christian Borntraeger

Am Donnerstag, 26. Juni 2008 schrieb Avi Kivity:

Quoted 8 lines

I don't think "p" should force the contents into memory? Perhaps "m"(*(char *)buffer)?

Anthony, I don't see why a memory clobber would tell gcc that the variables is actually used. The problem is with the void * -> unsigned long cast (__pa), once that happens gcc loses track. It's probably needed anyway since hypercalls _do_ clobber memory.

I I think about that again, the correct solution should be to use 2 input constraints for parameters together with the memory clobber on hypercall.

I think something like the following covers all cases:

static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)

Quoted 9 lines

{
	long ret;
	asm volatile(KVM_HYPERCALL
		     : "=a"(ret)
		     : "a"(nr), "b"(p1), "m"(*(char *) p1)
		     : "memory" );
	return ret;
}

I don't know exactly what the rules are when you cast from pointer to unsigned long. What I'm concerned about is that if there are a few layers of indirection, GCC won't be able to propagate the liveness of the pointer p1. It has to be able to figure out that p1 here was really that point on the stack. From the perspective of being conservative, it certainly can't hurt, but I'm not sure it solves the problem by itself.

This is why I think the "memory" constraint is really the right solution. That should force GCC to be very conservative about syncing everything to memory.

Am Donnerstag, 26. Juni 2008 schrieb Avi Kivity:

Quoted 4 lines

I don't think "p" should force the contents into memory? Perhaps "m"(*(char *)buffer)?

Anthony, I don't see why a memory clobber would tell gcc that the variables is actually used. The problem is with the void * -> unsigned long cast (__pa), once that happens gcc loses track. It's probably needed anyway since hypercalls _do_ clobber memory.

I I think about that again, the correct solution should be to use 2 input constraints for parameters together with the memory clobber on hypercall.

I think something like the following covers all cases:

static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)

{
	long ret;
	asm volatile(KVM_HYPERCALL
		     : "=a"(ret)
		     : "a"(nr), "b"(p1), "m"(*(char *) p1)
		     : "memory" );
	return ret;
}

The address and the memory content of p1 (if it is a pointer) is not ommitted by gcc. Furthermore, the memory clobbering nature of a hypercall is specified as well.