On Wed, Apr 29, 2015 at 09:49:53PM +0200, Christian Borntraeger wrote: > Am 29.04.2015 um 20:35 schrieb Michael S. Tsirkin: > > On Wed, Apr 29, 2015 at 04:43:19PM +0200, Christian Borntraeger wrote: > >> Am 29.04.2015 um 12:32 schrieb Michael S. Tsirkin: > >>> On Wed, Apr 29, 2015 at 10:52:15AM +0200, Cornelia Huck wrote: > >>>> On Wed, 29 Apr 2015 10:17:55 +0200 > >>>> Christian Borntraeger <borntrae...@de.ibm.com> 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 <peter.mayd...@linaro.org> wrote: > >>>>>>>>> > >>>>>>>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <m...@redhat.com> > >>>>>>>>>> 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? > >>> > >>> Or a compiler change? Try compiling some old release, see what happens. > >>> Anyway, let's move DEFINE_VIRTIO_COMMON_FEATURES into the base class > >>> now. Can you send a patch pls? > >> > >> 3.17 as guest fails, 3.18 as guest works. Not sure yet why. > >> > > > > Fascinating. block core changes? bisect will tell. > > > > This commit made it work. > > commit 7a11370e5e6c26566904bb7f08281093a3002ff2 > Author: Michael S. Tsirkin <m...@redhat.com> > Date: Wed Oct 15 10:22:30 2014 +1030 > > virtio_blk: enable VQs early > > virtio spec requires drivers to set DRIVER_OK before using VQs. > This is set automatically after probe returns, virtio block violated this > rule by calling add_disk, which causes the VQ to be used directly within > probe. > > To fix, call virtio_device_ready before using VQs. > > Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > Reviewed-by: Cornelia Huck <cornelia.h...@de.ibm.com> > Signed-off-by: Rusty Russell <ru...@rustcorp.com.au>
I guess this means s390 code somehow lost kicks that happened before DRIVER_OK. Without event index you would typically get another one on the next request. -- MST
