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

[Christian Borntraeger <borntraeger@de.ibm.com>]

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 <borntraeger@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.maydell@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@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 <mst@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 <mst@redhat.com>
    Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>