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

[Hollis Blanchard <hollisb@us.ibm.com>]

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