[Qemu-devel] [PATCH V4 0/3] Set correct blk feature for virtio 1.0

[Qemu-devel] [PATCH V4 0/3] Set correct blk feature for virtio 1.0

[Jason Wang]

Hi all:

This series tries to set feature correctly for virtio-blk when virtio
1.0 is supported. Two isssues were addressed according to the spec:

- scsi passthrough was not support in 1.0. This is done through: 1)
  let get_features() can fail 2) fail the get_features() when both
  scsi and virtio 1.0 is enabled.
- any layout must be set for transitional device. This is done by set
  any layout when 1.0 is supported.

Changes from V3:
- rebase on top of Michael's any_layout fixes

Changes from V2:
- Keep scsi=on by default since virtio 1.0 is disabled by default
- Advertise VIRTIO_BLK_F_SCSI unconditionally if virtio 1.0 is
  disabled

Changes from V1:
- Split virtio-net changes out of the series
- Enable VIRTIO_BLK_F_SCSI only when scsi is set
- Disable scsi by default and compat it for legacy machine types
- Let get_features() can fail and fail the initialization of
  virito-blk when both 1.0 and scsi were supported.

Jason Wang (3):
  virtio: get_features() can fail
  virtio-blk: fail get_features when both scsi and 1.0 were set
  virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device

 hw/9pfs/virtio-9p-device.c  |  3 ++-
 hw/block/virtio-blk.c       | 15 ++++++++++++---
 hw/char/virtio-serial-bus.c |  3 ++-
 hw/display/virtio-gpu.c     |  3 ++-
 hw/input/virtio-input.c     |  3 ++-
 hw/net/virtio-net.c         |  3 ++-
 hw/scsi/vhost-scsi.c        |  3 ++-
 hw/scsi/virtio-scsi.c       |  3 ++-
 hw/virtio/virtio-balloon.c  |  3 ++-
 hw/virtio/virtio-bus.c      |  3 ++-
 hw/virtio/virtio-rng.c      |  2 +-
 include/hw/virtio/virtio.h  |  4 +++-
 12 files changed, 34 insertions(+), 14 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH V4 1/3] virtio: get_features() can fail

[Jason Wang]

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/9pfs/virtio-9p-device.c  | 3 ++-
 hw/block/virtio-blk.c       | 3 ++-
 hw/char/virtio-serial-bus.c | 3 ++-
 hw/display/virtio-gpu.c     | 3 ++-
 hw/input/virtio-input.c     | 3 ++-
 hw/net/virtio-net.c         | 3 ++-
 hw/scsi/vhost-scsi.c        | 3 ++-
 hw/scsi/virtio-scsi.c       | 3 ++-
 hw/virtio/virtio-balloon.c  | 3 ++-
 hw/virtio/virtio-bus.c      | 3 ++-
 hw/virtio/virtio-rng.c      | 2 +-
 include/hw/virtio/virtio.h  | 4 +++-
 12 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 3f4c9e7..93a407c 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -21,7 +21,8 @@
 #include "virtio-9p-coth.h"
 #include "hw/virtio/virtio-access.h"
 
-static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features)
+static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features,
+                                       Error **errp)
 {
     virtio_add_feature(&features, VIRTIO_9P_MOUNT_TAG);
     return features;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 015b9b5..a6cf008 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -722,7 +722,8 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
     aio_context_release(blk_get_aio_context(s->blk));
 }
 
-static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features)
+static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
+                                        Error **errp)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 929e49c..bc56f5d 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -500,7 +500,8 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
     }
 }
 
-static uint64_t get_features(VirtIODevice *vdev, uint64_t features)
+static uint64_t get_features(VirtIODevice *vdev, uint64_t features,
+                             Error **errp)
 {
     VirtIOSerial *vser;
 
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 990a26b..a67d927 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -89,7 +89,8 @@ static void virtio_gpu_set_config(VirtIODevice *vdev, const uint8_t *config)
     }
 }
 
-static uint64_t virtio_gpu_get_features(VirtIODevice *vdev, uint64_t features)
+static uint64_t virtio_gpu_get_features(VirtIODevice *vdev, uint64_t features,
+                                        Error **errp)
 {
     return features;
 }
diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
index 7f5b8d6..7b25d27 100644
--- a/hw/input/virtio-input.c
+++ b/hw/input/virtio-input.c
@@ -166,7 +166,8 @@ static void virtio_input_set_config(VirtIODevice *vdev,
     virtio_notify_config(vdev);
 }
 
-static uint64_t virtio_input_get_features(VirtIODevice *vdev, uint64_t f)
+static uint64_t virtio_input_get_features(VirtIODevice *vdev, uint64_t f,
+                                          Error **errp)
 {
     return f;
 }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index e203058..1510839 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -446,7 +446,8 @@ static void virtio_net_set_queues(VirtIONet *n)
 
 static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
 
-static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features)
+static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
+                                        Error **errp)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
     NetClientState *nc = qemu_get_queue(n->nic);
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 52549f8..a69918b 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -153,7 +153,8 @@ static void vhost_scsi_stop(VHostSCSI *s)
 }
 
 static uint64_t vhost_scsi_get_features(VirtIODevice *vdev,
-                                        uint64_t features)
+                                        uint64_t features,
+                                        Error **errp)
 {
     VHostSCSI *s = VHOST_SCSI(vdev);
 
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index d17698d..811c3da 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -629,7 +629,8 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
 }
 
 static uint64_t virtio_scsi_get_features(VirtIODevice *vdev,
-                                         uint64_t requested_features)
+                                         uint64_t requested_features,
+                                         Error **errp)
 {
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 2990f8d..3577b7a 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -310,7 +310,8 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
     trace_virtio_balloon_set_config(dev->actual, oldactual);
 }
 
-static uint64_t virtio_balloon_get_features(VirtIODevice *vdev, uint64_t f)
+static uint64_t virtio_balloon_get_features(VirtIODevice *vdev, uint64_t f,
+                                            Error **errp)
 {
     VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
     f |= dev->host_features;
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 3926f7e..febda76 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -54,7 +54,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
 
     /* Get the features of the plugged device. */
     assert(vdc->get_features != NULL);
-    vdev->host_features = vdc->get_features(vdev, vdev->host_features);
+    vdev->host_features = vdc->get_features(vdev, vdev->host_features,
+                                            errp);
 }
 
 /* Reset the virtio_bus */
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 6e5f022..97d1541 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -104,7 +104,7 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
     virtio_rng_process(vrng);
 }
 
-static uint64_t get_features(VirtIODevice *vdev, uint64_t f)
+static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
 {
     return f;
 }
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index ff91711..59f0763 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -101,7 +101,9 @@ typedef struct VirtioDeviceClass {
     /* This is what a VirtioDevice must implement */
     DeviceRealize realize;
     DeviceUnrealize unrealize;
-    uint64_t (*get_features)(VirtIODevice *vdev, uint64_t requested_features);
+    uint64_t (*get_features)(VirtIODevice *vdev,
+                             uint64_t requested_features,
+                             Error **errp);
     uint64_t (*bad_features)(VirtIODevice *vdev);
     void (*set_features)(VirtIODevice *vdev, uint64_t val);
     int (*validate_features)(VirtIODevice *vdev);
-- 
2.1.4

[Qemu-devel] [PATCH V4 2/3] virtio-blk: fail get_features when both scsi and 1.0 were set

[Jason Wang]

SCSI passthrough was no longer supported in virtio 1.0, so this patch
fail the get_features() when both 1.0 and scsi is set. And also only
advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/block/virtio-blk.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a6cf008..9acbc3a 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -731,8 +731,16 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
     virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
     virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
     virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
-    virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
     virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
+    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
+        if (s->conf.scsi) {
+            error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
+            return 0;
+        }
+        virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
+    } else {
+        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
+    }
 
     if (s->conf.config_wce) {
         virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
-- 
2.1.4

[Qemu-devel] [PATCH V4 3/3] virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device

[Jason Wang]

Chapter 6.3 of spec said

"
Transitional devices MUST offer, and if offered by the device
transitional drivers MUST accept the following:

VIRTIO_F_ANY_LAYOUT (27)
"

So this patch only clear VIRTIO_F_LAYOUT for legacy device.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9acbc3a..1d3f26c 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -731,7 +731,6 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
     virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
     virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
     virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
-    virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
     if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
         if (s->conf.scsi) {
             error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
@@ -739,6 +738,7 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
         }
         virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
     } else {
+        virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
         virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
     }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH V4 2/3] virtio-blk: fail get_features when both scsi and 1.0 were set

[Paolo Bonzini]

On 27/07/2015 11:49, Jason Wang wrote:
> +    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {

No double underscores in userspace code.  Longstanding so it can be
fixed after 2.4 is out---but please remember to do it.

> +        if (s->conf.scsi) {
> +            error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");

Unclear error message, as one would expect SCSI passthrough not to work
anyway for e.g. a disk backed by a file.

It's not a big deal as long as you will disable VIRTIO_BLK_F_SCSI by
default in the same release that enables VIRTIO_F_VERSION_1 by default.

Paolo

> +            return 0;
> +        }
> +        virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
> +    } else {
> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> +    }

Re: [Qemu-devel] [PATCH V4 3/3] virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device

[Paolo Bonzini]

On 27/07/2015 11:49, Jason Wang wrote:
> So this patch only clear VIRTIO_F_LAYOUT for legacy device.
> 
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: qemu-block@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/block/virtio-blk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 9acbc3a..1d3f26c 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -731,7 +731,6 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
>      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
>      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
>      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> -    virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
>      if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
>          if (s->conf.scsi) {
>              error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
> @@ -739,6 +738,7 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
>          }
>          virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
>      } else {
> +        virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
>          virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>      }

This patch is unnecessary, since the feature is added back below under
"if (__virtio_has_feature(features, VIRTIO_F_VERSION_1))".

Paolo

Re: [Qemu-devel] [PATCH V4 2/3] virtio-blk: fail get_features when both scsi and 1.0 were set

[Cornelia Huck]

On Mon, 27 Jul 2015 12:28:23 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 27/07/2015 11:49, Jason Wang wrote:
> > +    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> 
> No double underscores in userspace code.  Longstanding so it can be
> fixed after 2.4 is out---but please remember to do it.

There's https://marc.info/?l=qemu-devel&m=142599436726514&w=2
which seems to have been lost in all that virtio rebasing.

Re: [Qemu-devel] [PATCH V4 3/3] virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device

[Michael S. Tsirkin]

On Mon, Jul 27, 2015 at 12:30:19PM +0200, Paolo Bonzini wrote:
> 
> 
> On 27/07/2015 11:49, Jason Wang wrote:
> > So this patch only clear VIRTIO_F_LAYOUT for legacy device.
> > 
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: qemu-block@nongnu.org
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/block/virtio-blk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 9acbc3a..1d3f26c 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -731,7 +731,6 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
> >      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> >      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> >      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> > -    virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
> >      if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> >          if (s->conf.scsi) {
> >              error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
> > @@ -739,6 +738,7 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
> >          }
> >          virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
> >      } else {
> > +        virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
> >          virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> >      }
> 
> This patch is unnecessary, since the feature is added back below under
> "if (__virtio_has_feature(features, VIRTIO_F_VERSION_1))".
> 
> Paolo

It's needed so we can apply
    virtio: set any_layout in virtio core

Re: [Qemu-devel] [PATCH V4 2/3] virtio-blk: fail get_features when both scsi and 1.0 were set

[Michael S. Tsirkin]

On Mon, Jul 27, 2015 at 12:39:44PM +0200, Cornelia Huck wrote:
> On Mon, 27 Jul 2015 12:28:23 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > On 27/07/2015 11:49, Jason Wang wrote:
> > > +    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> > 
> > No double underscores in userspace code.  Longstanding so it can be
> > fixed after 2.4 is out---but please remember to do it.
> 
> There's https://marc.info/?l=qemu-devel&m=142599436726514&w=2
> which seems to have been lost in all that virtio rebasing.

Right.
Pls repost after 2.4.

-- 
MST

Re: [Qemu-devel] [PATCH V4 2/3] virtio-blk: fail get_features when both scsi and 1.0 were set

[Michael S. Tsirkin]

On Mon, Jul 27, 2015 at 12:28:23PM +0200, Paolo Bonzini wrote:
> 
> 
> On 27/07/2015 11:49, Jason Wang wrote:
> > +    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> 
> No double underscores in userspace code.  Longstanding so it can be
> fixed after 2.4 is out---but please remember to do it.
> 
> > +        if (s->conf.scsi) {
> > +            error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
> 
> Unclear error message, as one would expect SCSI passthrough not to work
> anyway for e.g. a disk backed by a file.

Right - so I suggested:
	Virtio modern does not support scsi passthrough - please set disable-modern=on or switch to virtio-scsi.
With that change - ACK?

> 
> It's not a big deal as long as you will disable VIRTIO_BLK_F_SCSI by
> default in the same release that enables VIRTIO_F_VERSION_1 by default.
> 
> Paolo
> > +            return 0;
> > +        }
> > +        virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > +    } else {
> > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > +    }

Re: [Qemu-devel] [PATCH V4 2/3] virtio-blk: fail get_features when both scsi and 1.0 were set

[Paolo Bonzini]

On 27/07/2015 13:26, Michael S. Tsirkin wrote:
>>> > > +        if (s->conf.scsi) {
>>> > > +            error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
>> > 
>> > Unclear error message, as one would expect SCSI passthrough not to work
>> > anyway for e.g. a disk backed by a file.
> Right - so I suggested:
> 	Virtio modern does not support scsi passthrough - please set disable-modern=on or switch to virtio-scsi.
> With that change - ACK?

scsi=on by default, so everyone is getting the message until they
disable virtio 1.0.  Suggesting a switch to virtio-scsi doesn't make sense.

Your proposal makes sense once scsi=off by default.  Until then, what
about "please set scsi=off for virtio-blk devices in order to use virtio
1.0"?

Paolo

Re: [Qemu-devel] [PATCH V4 3/3] virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device

[Paolo Bonzini]

On 27/07/2015 13:22, Michael S. Tsirkin wrote:
> > This patch is unnecessary, since the feature is added back below under
> > "if (__virtio_has_feature(features, VIRTIO_F_VERSION_1))".
> 
> It's needed so we can apply
>     virtio: set any_layout in virtio core

Ah, okay.

Paolo

Re: [Qemu-devel] [PATCH V4 0/3] Set correct blk feature for virtio 1.0

[Paolo Bonzini]

On 27/07/2015 11:49, Jason Wang wrote:
> Hi all:
> 
> This series tries to set feature correctly for virtio-blk when virtio
> 1.0 is supported. Two isssues were addressed according to the spec:
> 
> - scsi passthrough was not support in 1.0. This is done through: 1)
>   let get_features() can fail 2) fail the get_features() when both
>   scsi and virtio 1.0 is enabled.
> - any layout must be set for transitional device. This is done by set
>   any layout when 1.0 is supported.
> 
> Changes from V3:
> - rebase on top of Michael's any_layout fixes

With my fixup to the error message,

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> Changes from V2:
> - Keep scsi=on by default since virtio 1.0 is disabled by default
> - Advertise VIRTIO_BLK_F_SCSI unconditionally if virtio 1.0 is
>   disabled
> 
> Changes from V1:
> - Split virtio-net changes out of the series
> - Enable VIRTIO_BLK_F_SCSI only when scsi is set
> - Disable scsi by default and compat it for legacy machine types
> - Let get_features() can fail and fail the initialization of
>   virito-blk when both 1.0 and scsi were supported.
> 
> Jason Wang (3):
>   virtio: get_features() can fail
>   virtio-blk: fail get_features when both scsi and 1.0 were set
>   virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device
> 
>  hw/9pfs/virtio-9p-device.c  |  3 ++-
>  hw/block/virtio-blk.c       | 15 ++++++++++++---
>  hw/char/virtio-serial-bus.c |  3 ++-
>  hw/display/virtio-gpu.c     |  3 ++-
>  hw/input/virtio-input.c     |  3 ++-
>  hw/net/virtio-net.c         |  3 ++-
>  hw/scsi/vhost-scsi.c        |  3 ++-
>  hw/scsi/virtio-scsi.c       |  3 ++-
>  hw/virtio/virtio-balloon.c  |  3 ++-
>  hw/virtio/virtio-bus.c      |  3 ++-
>  hw/virtio/virtio-rng.c      |  2 +-
>  include/hw/virtio/virtio.h  |  4 +++-
>  12 files changed, 34 insertions(+), 14 deletions(-)
> 

Re: [Qemu-devel] [PATCH V4 3/3] virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device

[Cornelia Huck]

On Mon, 27 Jul 2015 14:22:37 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 27, 2015 at 12:30:19PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 27/07/2015 11:49, Jason Wang wrote:
> > > So this patch only clear VIRTIO_F_LAYOUT for legacy device.
> > > 
> > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > Cc: qemu-block@nongnu.org
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  hw/block/virtio-blk.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index 9acbc3a..1d3f26c 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -731,7 +731,6 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
> > >      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> > >      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> > >      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> > > -    virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > >      if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> > >          if (s->conf.scsi) {
> > >              error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
> > > @@ -739,6 +738,7 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
> > >          }
> > >          virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > >      } else {
> > > +        virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > >          virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > >      }
> > 
> > This patch is unnecessary, since the feature is added back below under
> > "if (__virtio_has_feature(features, VIRTIO_F_VERSION_1))".
> > 
> > Paolo
> 
> It's needed so we can apply
>     virtio: set any_layout in virtio core

So what's the plan on all those virtio feature patches? It's hard to
keep track about what is based upon what, and what the end result looks
like. I don't have a good feeling about doing this that late in the 2.4
cycle.

Re: [Qemu-devel] [PATCH V4 3/3] virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device

[Michael S. Tsirkin]

On Mon, Jul 27, 2015 at 03:28:51PM +0200, Cornelia Huck wrote:
> On Mon, 27 Jul 2015 14:22:37 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Jul 27, 2015 at 12:30:19PM +0200, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 27/07/2015 11:49, Jason Wang wrote:
> > > > So this patch only clear VIRTIO_F_LAYOUT for legacy device.
> > > > 
> > > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > > Cc: qemu-block@nongnu.org
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  hw/block/virtio-blk.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > index 9acbc3a..1d3f26c 100644
> > > > --- a/hw/block/virtio-blk.c
> > > > +++ b/hw/block/virtio-blk.c
> > > > @@ -731,7 +731,6 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
> > > >      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> > > >      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> > > >      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> > > > -    virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > > >      if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> > > >          if (s->conf.scsi) {
> > > >              error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
> > > > @@ -739,6 +738,7 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
> > > >          }
> > > >          virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > > >      } else {
> > > > +        virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > > >          virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > >      }
> > > 
> > > This patch is unnecessary, since the feature is added back below under
> > > "if (__virtio_has_feature(features, VIRTIO_F_VERSION_1))".
> > > 
> > > Paolo
> > 
> > It's needed so we can apply
> >     virtio: set any_layout in virtio core
> 
> So what's the plan on all those virtio feature patches? It's hard to
> keep track about what is based upon what, and what the end result looks
> like.

I pushed everything out to my pci branch, pls take a look.
This is what I have:

b787b35 acpi: fix pvpanic device is not shown in ui
49009db hw/acpi/ich9: clean up stale comment about KVM not supporting SMM
e513e9c hw/acpi/ich9: clear smi_en on reset
c9b11f9 virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device
efb8206 virtio-blk: fail get_features when both scsi and 1.0 were set
9d5b731 virtio: get_features() can fail
2746269 virtio-pci: fix memory MR cleanup for modern
09999a5 virtio: set any_layout in virtio core
cd4bfbb virtio-9p: fix any_layout
7882080 virtio-serial: fix ANY_LAYOUT
5f45607 virtio: hide legacy features from modern guests


> I don't have a good feeling about doing this that late in the 2.4
> cycle.

Well there will always be bugs. Given modern is disabled by default,
even if more bugs surface after release it's not the end of the
world.

-- 
MST

Re: [Qemu-devel] [PATCH V4 2/3] virtio-blk: fail get_features when both scsi and 1.0 were set

[Jason Wang]

On 07/27/2015 07:26 PM, Michael S. Tsirkin wrote:
> On Mon, Jul 27, 2015 at 12:28:23PM +0200, Paolo Bonzini wrote:
>>
>> On 27/07/2015 11:49, Jason Wang wrote:
>>> +    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
>> No double underscores in userspace code.  Longstanding so it can be
>> fixed after 2.4 is out---but please remember to do it.
>>
>>> +        if (s->conf.scsi) {
>>> +            error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
>> Unclear error message, as one would expect SCSI passthrough not to work
>> anyway for e.g. a disk backed by a file.
> Right - so I suggested:
> 	Virtio modern does not support scsi passthrough - please set disable-modern=on or switch to virtio-scsi.
> With that change - ACK?

I've considered this, but a little problem is 'disable-modern' only
works for pci.

>> It's not a big deal as long as you will disable VIRTIO_BLK_F_SCSI by
>> default in the same release that enables VIRTIO_F_VERSION_1 by default.
>>
>> Paolo
>>> +            return 0;
>>> +        }
>>> +        virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
>>> +    } else {
>>> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>>> +    }