KVM: pvmmu breakage with gcc 4.3.0

KVM: pvmmu breakage with gcc 4.3.0

[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 <mtosatti@redhat.com>


diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 8b7a3cf..c892752 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -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 */

Re: KVM: pvmmu breakage with gcc 4.3.0

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

Marcelo Tosatti wrote:
> 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.

See the attached patch.

Avi: please push this for 2.6.26 if possible.

Regards,

Anthony Liguori

> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8b7a3cf..c892752 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


[-- Attachment #2: hypercall-memory-clobber.patch --]
[-- Type: application/mbox, Size: 1866 bytes --]

Re: KVM: pvmmu breakage with gcc 4.3.0

[Avi Kivity]

Marcelo Tosatti wrote:
> 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 <mtosatti@redhat.com>
>
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8b7a3cf..c892752 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -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.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: KVM: pvmmu breakage with gcc 4.3.0

[Anthony Liguori]

Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> 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 <mtosatti@redhat.com>
>>
>>
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 8b7a3cf..c892752 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -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.

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.

>   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.

Regards,

Anthony Liguori

Re: KVM: pvmmu breakage with gcc 4.3.0

[Christian Borntraeger]

Am Donnerstag, 26. Juni 2008 schrieb Anthony Liguori:
> Marcelo Tosatti wrote:
> > 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.

Thats what we do on asm-s390/kvm_para.h. Our hypercalls do have a memory 
clobber.

Re: KVM: pvmmu breakage with gcc 4.3.0

[Christian Borntraeger]

Am Donnerstag, 26. Juni 2008 schrieb Avi Kivity:
> 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

Re: KVM: pvmmu breakage with gcc 4.3.0

[Anthony Liguori]

Christian Borntraeger wrote:
> Am Donnerstag, 26. Juni 2008 schrieb Avi Kivity:
>   
>> 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.
>   

Isn't that redundant though?  Shouldn't a memory clobber force GCC to 
assume that anything that's reachable is live?

Regards,

Anthony Liguori

> Christian
>   

Re: KVM: pvmmu breakage with gcc 4.3.0

[Anthony Liguori]

Christian Borntraeger wrote:
> Am Donnerstag, 26. Juni 2008 schrieb Avi Kivity:
>   
>> 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;
> }
>   

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.

Regards,

Anthony Liguori

> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: KVM: pvmmu breakage with gcc 4.3.0

[Christian Borntraeger]

Am Donnerstag, 26. Juni 2008 schrieb Anthony Liguori:
> Christian Borntraeger wrote:
> > Am Donnerstag, 26. Juni 2008 schrieb Avi Kivity:
> >   
> >> 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;
> > }
> >   
> 
> 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.

The thing is, that your patch has "memory" in the clobber list.  I dont know 
if clobber constraints give any guarantees about input constraints. Thats why 
I think that "b" plus "m" as input constraints are better. Does gcc actually 
accept "memory" as input contraint? 

I think I have to ask our compiler developers, but in 5 minutes I will be 
offline until monday...

Re: KVM: pvmmu breakage with gcc 4.3.0

[Alexandre Oliva]

On Jun 26, 2008, Anthony Liguori <anthony@codemonkey.ws> wrote:

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

This should definitely be done, but I don't see that this is a sure
way to get that initializers that are otherwise unreferenced to be
stored in their locations.  IOW, it appears to me that GCC might still
decide to optimize them away, because it doesn't see that the values
can be used.

Now, Marcelo's patch appears to have been the result of a
misunderstanding.  I meant to tell him that a simpler "p" notation was
*not* enough to make the problem go away.  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.

Anyhow, for now, it appears that a "memory" clobber is not only
required for correct semantics, but also enough to fix the problem.
Let's just hope GCC doesn't get too smart.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}
FSFLA Board Member       ¡Sé Libre! => http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}

Re: KVM: pvmmu breakage with gcc 4.3.0

[Bill Davidsen]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Marcelo Tosatti wrote:
>>> 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 <mtosatti@redhat.com>
>>>
>>>
>>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>>> index 8b7a3cf..c892752 100644
>>> --- a/arch/x86/kernel/kvm.c
>>> +++ b/arch/x86/kernel/kvm.c
>>> @@ -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.
> 
> 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.
> 
In general the right stick to hit a compiler with is 'volatile,' which 
should tell the compiler that outside forces are in play. Without really 
digging into the code I don't want to suggest just where this should be 
used, but clearly Christian had the same thought.

>>   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.
> 

-- 
Bill Davidsen <davidsen@tmr.com>
   "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot

Re: KVM: pvmmu breakage with gcc 4.3.0

[Avi Kivity]

Alexandre Oliva wrote:
> 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.
>
>   

I agree, I think such a patch is important for future-proofing Linux 
against increasing gcc optimization capabilities.

-- 
error compiling committee.c: too many arguments to function

Re: KVM: pvmmu breakage with gcc 4.3.0

[Avi Kivity]

Christian Borntraeger wrote:
> Am Donnerstag, 26. Juni 2008 schrieb Avi Kivity:
>   
>> 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.
>   

That only works if p1 is a virtual address.  x86 (and ppc) hypercalls 
use physical addresses.


-- 
error compiling committee.c: too many arguments to function