Re: [Qemu-devel] [PATCHv2 3/3] virtio: order index/descriptor reads

[Michael S. Tsirkin]

On Tue, Apr 24, 2012 at 05:01:07PM +0200, Paolo Bonzini wrote:
> Il 24/04/2012 16:38, Michael S. Tsirkin ha scritto:
> >>> I think it is prudent to address this theoretical race condition.
> >>
> >> I think your fix is right, but it is not needed on x86.  lfence is only
> >> required for weakly-ordered memory types, so says Intel,
> > 
> > I see this in spec:
> > 
> > The LFENCE instruction establishes a memory fence for loads. It
> > guarantees ordering between two loads and prevents speculative loads
> > from passing the load fence (that is, no speculative loads are allowed
> > until all loads specified before the load fence have been carried out).
> > 
> > Speculative load is exactly what we are handling here.
> 
> In my copy of the Intel manual "6.2.2 Memory Ordering in P6 and More
> Recent Processor Families" says very explicitly "Reads are not reordered
> with other reads".
> 
> At least this is the illusion that the processor gives; that is,
> speculative loads are discarded if another processor writes to the same
> location.

In my copy it's 8.2.2. Anyway - this explicitly talks about a single processor 
system.

> 
> But it's not just speculative loads, it can also be out-of-order
> execution as AMD says in "7.1.1 Read Ordering".  For cacheable memory
> types "Out-of-order reads are allowed to the extent that they can be
> performed transparently to software, such that the appearance of
> in-order execution is maintained. Out-of-order reads can occur as a
> result of out-of-order instruction execution or speculative execution.
> The processor can read memory and perform cache refills out-of-order to
> allow out-of-order execution to proceed".
> >> and AMD is even clearer: "Loads from differing memory types may be
> >> performed out of order, in particular between WC/WC+ and other
> >> memory types".
> > 
> > This does not say loads from same memory type are ordered.
> 
> That would really be a bad trick for AMD to play.  Luckily they say that
> in 7.1.1, as cited above.

Have not looked but I think they talk about a single processor here as well.
lfence is for controlling smp effects.

> >> I would be grateful if, instead of fixing the qemu-barrier.h parts of
> >> the patches, you picked up the (sole) patch in the atomics branch of
> >> git://github.com/bonzini/qemu.git.  The constructs there are more
> >> complete than what we have in qemu-barrier.h,
> > 
> > Sorry this is just a bugfix in virtio, don't see a reason to make
> > it depend on a wholesale rework of atomics.
> 
> The reason is that your fixes didn't work on PPC, and were suboptimal on
> x86.
> 
> Paolo

I'll fix PPC but I'll stick to the barriers the way Linux implements
them. They pairing rules for these are well documented so we
just need to stick to the rules.

Let's argue about C11 atomics in some other thread.