Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Cornelia Huck]

On Wed, 29 Apr 2015 10:17:55 +0200
Christian Borntraeger <address@hidden> wrote:

> Am 28.04.2015 um 20:32 schrieb Michael S. Tsirkin:
> > On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
> >> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
> >>> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> >>>> On Tue, 28 Apr 2015 14:16:40 +0100
> >>>> Peter Maydell <address@hidden> wrote:
> >>>>
> >>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <address@hidden> wrote:
> >>>>>> The patches look correct to me too, but I want s390
> >>>>>> cleaned up so it does not include COMMON_FEATURES
> >>>>>> in 100 places, and I prefer merging it all together.
> >>>>>
> >>>>> It seems a bit harsh to ask Shannon to do s390 cleanup when
> >>>>> he doesn't have any access to s390 guests or test cases...
> >>>>> Making S390 put COMMON_FEATURES in the right places seems
> >>>>> to me like a separate bit of s390-specific cleanup.
> >>>>
> >>>> Yep, see my other reply... I'm not quite sure what's wrong with
> >>>> event_idx on virtio-blk for s390-virtio, or I would gladly make this
> >>>> consistent with the other transports. Any hints appreciated :)
> >>>
> >>> Is this still happening?
> >>>
> >>> It is possible that what was missing was
> >>> 92045d80badc43c9f95897aad675dc7ef17a3b3f
> >>> and/or
> >>> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
> >>>
> >>
> >> Found this:
> >> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
> >> so it's unlikely: these commits are from 2012, you saw
> >> issues in 2014.
> >>
> >> We really need to fix it. virtio 1 work will be much easier if
> >> we can just move features into virtio dev.
> 
> Yes, we have to understand why event_idx breaks for the s390-virtio transport.
> > 
> > I'm beginning to suspect this is a wrong implementation of barriers.
> > Questions:
> >     - which compiler to you use?
> >     - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
> >       They all must do br %r14 I think, and this is what
> >       s390x-linux-gnu-gcc generated for me:
> >         s390x-linux-gnu-gcc (GCC) 4.9.1
> 
> s390 has strong memory ordering. Reads are in order, writes are in order. 
> bcr 14,0 or bcr 15,0 then only serialize the reads against the writes.
> So smp_rmb and smp_wmb can be implemented as no-ops like QEMU.
> If your change "fixes" the issue then we have a problem somewhere else

And (surprise, surprise) virtio-blk now works - but it also works when
I back out the atomic.h change again. No barrier problems :)

Good news is that we can change s390-virtio to be just like the other
transports. Although I'd like to understand why it was broken before.
Maybe a guest change?