[SeaBIOS] [PATCH] virtio: clean up memory barrier usage

[Gleb Natapov]

On Mon, May 24, 2010 at 01:01:34PM +0300, Michael S. Tsirkin wrote:
> On Mon, May 24, 2010 at 12:55:30PM +0300, Gleb Natapov wrote:
> > On Mon, May 24, 2010 at 12:39:41PM +0300, Michael S. Tsirkin wrote:
> > > On Mon, May 24, 2010 at 12:35:21PM +0300, Gleb Natapov wrote:
> > > > On Thu, May 20, 2010 at 04:36:32PM +0300, Michael S. Tsirkin wrote:
> > > > > diff --git a/src/virtio-ring.h b/src/virtio-ring.h
> > > > > index 3fb86fe..8b546f4 100644
> > > > > --- a/src/virtio-ring.h
> > > > > +++ b/src/virtio-ring.h
> > > > > @@ -9,8 +9,9 @@
> > > > >  
> > > > >  #define virt_to_phys(v) (unsigned long)(v)
> > > > >  #define phys_to_virt(p) (void*)(p)
> > > > > -#define wmb() barrier()
> > > > > -#define mb() barrier()
> > > > > +/* Compiler barrier is enough as an x86 CPU does not reorder reads or writes */
> > > > > +#define smp_rmb() barrier()
> > > > > +#define smp_wmb() barrier()
> > > > >  
> > > > I thought you were going to use real memory barriers (although we
> > > > concluded compiler barrier should be enough, but it's better to be safe
> > > > then sorry).
> > > > 
> > > > --
> > > > 			Gleb.
> > > 
> > > On x86, ops of the same kind are ordered by CPU, so all we need to do is
> > > 1. avoid depending on order between reads and writes
> > > 2. prevent the compiler from reordering memory accesses
> > > 
> > So on Linux smp_rmb() and smp_wmb() are just compiler barriers too? (look
> > like they are).
> 
> On x86, yes.
> 
Of course x86. We are talking about x86 BIOS code here ;)

> > In that case lets use barrier() directly no need to
> > obfuscate the code.
> 
> I think it's better to use smp_rmb/smp_wmb because this code
> tends to get copied around, it might end up on non-x86 platforms.
> For these, barrier() would be wrong.
> 
So they will have a bug. It will teach them to not blindly copy code.

> Another reason is that it makes it easier to audit code comparing
> it with linux virtio guest.
> 
If you know that on x86 smp_(w|r)mb() are just barrier() it's still
easy. To be honest though this is the reason I left
virt_to_phys()/phys_to_virt(), so Kevin this is your call.

--
			Gleb.