[PATCH] Add memory clobber to hypercalls (v2)

[PATCH] Add memory clobber to hypercalls (v2)

[Anthony Liguori]

Hypercalls can modify arbitrary regions of memory.  Make sure to indicate this
in the clobber list.  This fixes a hang when using KVM_GUEST kernel built with
GCC 4.3.0.

This was originally spotted and analyzed by Marcelo.

Since v1, I've also added a "m" constraint for the inputs to the hypercall.
This was suggested by Christian since it's not entirely clear whether a memory
clobber will force the data to be in memory before the asm statement.  In the
very least, it helps to be more conservative.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/include/asm-x86/kvm_para.h b/include/asm-x86/kvm_para.h
index bfd9900..a621f10 100644
--- a/include/asm-x86/kvm_para.h
+++ b/include/asm-x86/kvm_para.h
@@ -71,7 +71,8 @@ static inline long kvm_hypercall0(unsigned int nr)
 	long ret;
 	asm volatile(KVM_HYPERCALL
 		     : "=a"(ret)
-		     : "a"(nr));
+		     : "a"(nr)
+		     : "memory");
 	return ret;
 }
 
@@ -80,7 +81,9 @@ static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
 	long ret;
 	asm volatile(KVM_HYPERCALL
 		     : "=a"(ret)
-		     : "a"(nr), "b"(p1));
+		     : "a"(nr), "b"(p1),
+		       "m"(*(char *)p1)
+		     : "memory");
 	return ret;
 }
 
@@ -90,7 +93,9 @@ static inline long kvm_hypercall2(unsigned int nr, unsigned long p1,
 	long ret;
 	asm volatile(KVM_HYPERCALL
 		     : "=a"(ret)
-		     : "a"(nr), "b"(p1), "c"(p2));
+		     : "a"(nr), "b"(p1), "c"(p2),
+		       "m"(*(char *)p1), "m"(*(char *)p2)
+		     : "memory");
 	return ret;
 }
 
@@ -100,7 +105,9 @@ static inline long kvm_hypercall3(unsigned int nr, unsigned long p1,
 	long ret;
 	asm volatile(KVM_HYPERCALL
 		     : "=a"(ret)
-		     : "a"(nr), "b"(p1), "c"(p2), "d"(p3));
+		     : "a"(nr), "b"(p1), "c"(p2), "d"(p3),
+		       "m"(*(char *)p1), "m"(*(char *)p2), "m"(*(char *)p3)
+		     : "memory");
 	return ret;
 }
 
@@ -111,7 +118,10 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
 	long ret;
 	asm volatile(KVM_HYPERCALL
 		     : "=a"(ret)
-		     : "a"(nr), "b"(p1), "c"(p2), "d"(p3), "S"(p4));
+		     : "a"(nr), "b"(p1), "c"(p2), "d"(p3), "S"(p4),
+		       "m"(*(char *)p1), "m"(*(char *)p2), "m"(*(char *)p3),
+		       "m"(*(char *)p4)
+		     : "memory");
 	return ret;
 }
 

Re: [PATCH] Add memory clobber to hypercalls (v2)

[Hollis Blanchard]

On Thu, 2008-06-26 at 13:25 -0500, Anthony Liguori wrote:
> Hypercalls can modify arbitrary regions of memory.  Make sure to indicate this
> in the clobber list.  This fixes a hang when using KVM_GUEST kernel built with
> GCC 4.3.0.
> 
> This was originally spotted and analyzed by Marcelo.
> 
> Since v1, I've also added a "m" constraint for the inputs to the hypercall.
> This was suggested by Christian since it's not entirely clear whether a memory
> clobber will force the data to be in memory before the asm statement.  In the
> very least, it helps to be more conservative.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Acked-by: Hollis Blanchard <hollisb@us.ibm.com>

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] Add memory clobber to hypercalls (v2)

[Avi Kivity]

Anthony Liguori wrote:
> Hypercalls can modify arbitrary regions of memory.  Make sure to indicate this
> in the clobber list.  This fixes a hang when using KVM_GUEST kernel built with
> GCC 4.3.0.
>
> This was originally spotted and analyzed by Marcelo.
>
> Since v1, I've also added a "m" constraint for the inputs to the hypercall.
> This was suggested by Christian since it's not entirely clear whether a memory
> clobber will force the data to be in memory before the asm statement.  In the
> very least, it helps to be more conservative.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> @@ -80,7 +81,9 @@ static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
>  	long ret;
>  	asm volatile(KVM_HYPERCALL
>  		     : "=a"(ret)
> -		     : "a"(nr), "b"(p1));
> +		     : "a"(nr), "b"(p1),
> +		       "m"(*(char *)p1)
> +		     : "memory");
>  	return ret;
>  }
>  
>   

Those are physical addresses, not virtual, and on i386 the addresses are 
split across multiple registers.

However a small test program shows that the memory clobber does work 
with gcc 4.3, so I'll pick the earlier patch.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] Add memory clobber to hypercalls (v2)

[Hollis Blanchard]

On Sat, 2008-06-28 at 06:43 +0300, Avi Kivity wrote:
> Anthony Liguori wrote:
> > Hypercalls can modify arbitrary regions of memory.  Make sure to indicate this
> > in the clobber list.  This fixes a hang when using KVM_GUEST kernel built with
> > GCC 4.3.0.
> >
> > This was originally spotted and analyzed by Marcelo.
> >
> > Since v1, I've also added a "m" constraint for the inputs to the hypercall.
> > This was suggested by Christian since it's not entirely clear whether a memory
> > clobber will force the data to be in memory before the asm statement.  In the
> > very least, it helps to be more conservative.
> >
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >
> > @@ -80,7 +81,9 @@ static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
> >  	long ret;
> >  	asm volatile(KVM_HYPERCALL
> >  		     : "=a"(ret)
> > -		     : "a"(nr), "b"(p1));
> > +		     : "a"(nr), "b"(p1),
> > +		       "m"(*(char *)p1)
> > +		     : "memory");
> >  	return ret;
> >  }
> >  
> >   
> 
> Those are physical addresses, not virtual, and on i386 the addresses are 
> split across multiple registers.
> 
> However a small test program shows that the memory clobber does work 
> with gcc 4.3, so I'll pick the earlier patch.

What about gcc 4.4? 4.5? 5.0?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] Add memory clobber to hypercalls (v2)

[Avi Kivity]

Hollis Blanchard wrote:
> On Sat, 2008-06-28 at 06:43 +0300, Avi Kivity wrote:
>   
>> Anthony Liguori wrote:
>>     
>>> Hypercalls can modify arbitrary regions of memory.  Make sure to indicate this
>>> in the clobber list.  This fixes a hang when using KVM_GUEST kernel built with
>>> GCC 4.3.0.
>>>
>>> This was originally spotted and analyzed by Marcelo.
>>>
>>> Since v1, I've also added a "m" constraint for the inputs to the hypercall.
>>> This was suggested by Christian since it's not entirely clear whether a memory
>>> clobber will force the data to be in memory before the asm statement.  In the
>>> very least, it helps to be more conservative.
>>>
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>>
>>> @@ -80,7 +81,9 @@ static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
>>>  	long ret;
>>>  	asm volatile(KVM_HYPERCALL
>>>  		     : "=a"(ret)
>>> -		     : "a"(nr), "b"(p1));
>>> +		     : "a"(nr), "b"(p1),
>>> +		       "m"(*(char *)p1)
>>> +		     : "memory");
>>>  	return ret;
>>>  }
>>>  
>>>   
>>>       
>> Those are physical addresses, not virtual, and on i386 the addresses are 
>> split across multiple registers.
>>
>> However a small test program shows that the memory clobber does work 
>> with gcc 4.3, so I'll pick the earlier patch.
>>     
>
> What about gcc 4.4? 4.5? 5.0?
>
>   

Alexandre Oliva's idea of adding a constraint in __pa() to tell gcc that 
the memory there must be written seems to be the best option here.  
Though perhaps gcc should consider all memory pointed to by a pointer 
that is cast to an integer as escaped.


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

Re: [PATCH] Add memory clobber to hypercalls (v2)

[Hollis Blanchard]

On Mon, 2008-06-30 at 18:59 +0300, Avi Kivity wrote:
> Hollis Blanchard wrote:
> > On Sat, 2008-06-28 at 06:43 +0300, Avi Kivity wrote:
> >   
> >> Anthony Liguori wrote:
> >>     
> >>> Hypercalls can modify arbitrary regions of memory.  Make sure to indicate this
> >>> in the clobber list.  This fixes a hang when using KVM_GUEST kernel built with
> >>> GCC 4.3.0.
> >>>
> >>> This was originally spotted and analyzed by Marcelo.
> >>>
> >>> Since v1, I've also added a "m" constraint for the inputs to the hypercall.
> >>> This was suggested by Christian since it's not entirely clear whether a memory
> >>> clobber will force the data to be in memory before the asm statement.  In the
> >>> very least, it helps to be more conservative.
> >>>
> >>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >>>
> >>> @@ -80,7 +81,9 @@ static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
> >>>  	long ret;
> >>>  	asm volatile(KVM_HYPERCALL
> >>>  		     : "=a"(ret)
> >>> -		     : "a"(nr), "b"(p1));
> >>> +		     : "a"(nr), "b"(p1),
> >>> +		       "m"(*(char *)p1)
> >>> +		     : "memory");
> >>>  	return ret;
> >>>  }
> >>>  
> >>>   
> >>>       
> >> Those are physical addresses, not virtual, and on i386 the addresses are 
> >> split across multiple registers.
> >>
> >> However a small test program shows that the memory clobber does work 
> >> with gcc 4.3, so I'll pick the earlier patch.
> >>     
> >
> > What about gcc 4.4? 4.5? 5.0?
> >
> >   
> 
> Alexandre Oliva's idea of adding a constraint in __pa() to tell gcc that 
> the memory there must be written seems to be the best option here.  

Makes sense to me.

-- 
Hollis Blanchard
IBM Linux Technology Center