[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends |
Date: |
Wed, 29 Apr 2015 22:19:12 +0200 |
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 <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?
> >>>
> >>> 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 <address@hidden>
> 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 <address@hidden>
> Reviewed-by: Cornelia Huck <address@hidden>
> Signed-off-by: Rusty Russell <address@hidden>
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
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, (continued)
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Michael S. Tsirkin, 2015/04/28
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Michael S. Tsirkin, 2015/04/28
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Christian Borntraeger, 2015/04/29
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Cornelia Huck, 2015/04/29
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Michael S. Tsirkin, 2015/04/29
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Cornelia Huck, 2015/04/29
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Michael S. Tsirkin, 2015/04/29
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Christian Borntraeger, 2015/04/29
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Michael S. Tsirkin, 2015/04/29
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Christian Borntraeger, 2015/04/29
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Christian Borntraeger, 2015/04/30
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Peter Maydell, 2015/04/28
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Michael S. Tsirkin, 2015/04/28
- Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends, Shannon Zhao, 2015/04/29