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

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 08:32:51PM +0200, Michael S. Tsirkin wrote:
> 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.
> 
> 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


Also, what does the following do?

Signed-off-by: Michael S. Tsirkin <address@hidden>

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 98e05ca..1c8051b 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -43,7 +43,7 @@
 #define smp_read_barrier_depends()   asm volatile("mb":::"memory")
 #endif
 
-#if defined(__i386__) || defined(__x86_64__) || defined(__s390x__)
+#if defined(__i386__) || defined(__x86_64__)
 
 /*
  * Because of the strongly ordered storage model, wmb() and rmb() are nops
@@ -53,6 +53,10 @@
 #define smp_wmb()   barrier()
 #define smp_rmb()   barrier()
 
+#endif
+
+#if defined(__i386__) || defined(__x86_64__) || defined(__s390x__)
+
 /*
  * __sync_lock_test_and_set() is documented to be an acquire barrier only,
  * but it is a full barrier at the hardware level.  Add a compiler barrier
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index c8a78ba..0f31ae3 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -527,11 +527,17 @@ static const TypeInfo s390_virtio_net = {
     .class_init    = s390_virtio_net_class_init,
 };
 
+static Property s390_virtio_blk_properties[] = {
+    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
 {
     VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
 
     k->realize = s390_virtio_blk_realize;
+    dc->props = s390_virtio_blk_properties;
 }
 
 static const TypeInfo s390_virtio_blk = {