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

[Qemu-devel] [PATCH V3 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 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: set VIRTIO_F_ANY_LAYOUT when 1.0 is supported

 hw/9pfs/virtio-9p-device.c  |  3 ++-
 hw/block/virtio-blk.c       | 13 +++++++++++--
 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, 33 insertions(+), 13 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH V3 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 6aefda4..4c27974 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 78c73e5..90bdc31 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -499,7 +499,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 e3c2db3..a56bcab 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -438,7 +438,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 f7d3c7c..701efeb 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 740ed31..63f35cb 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -98,7 +98,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 473fb75..1cd824f 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -97,7 +97,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 V3 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 4c27974..4716c3e 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -731,7 +731,14 @@ 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);
+    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;
+        }
+    } 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 V3 3/3] virtio-blk: set VIRTIO_F_ANY_LAYOUT when 1.0 is supported

[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 sets VIRTIO_F_ANY_LAYOUT when 1.0 is supported.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 4716c3e..2e7a190 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -736,6 +736,7 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
             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);
     }
-- 
2.1.4

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

[Cornelia Huck]

On Wed, 22 Jul 2015 13:59:51 +0800
Jason Wang <jasowang@redhat.com> wrote:

> 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 | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 4c27974..4716c3e 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -731,7 +731,14 @@ 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);
> +    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;
> +        }
> +    } else {
> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> +    }
> 
>      if (s->conf.config_wce) {
>          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);

Do we advertise F_SCSI even if scsi is not configured in order to keep
the same bits as before? I'm afraid I don't remember, that thread was
long :/

I'm asking because I'd like to depend on that bit to decide whether I
can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
would be an easy thing to do, and I'd like to avoid mucking around with
device-specific configuration from the transport.

To illustrate what I'm talking about, my current patchset for virtio-1
on ccw is here:

git://github.com/cohuck/qemu virtio-1-ccw-2.5

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> On Wed, 22 Jul 2015 13:59:51 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
> > 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 | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 4c27974..4716c3e 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -731,7 +731,14 @@ 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);
> > +    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;
> > +        }
> > +    } else {
> > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > +    }
> > 
> >      if (s->conf.config_wce) {
> >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> 
> Do we advertise F_SCSI even if scsi is not configured in order to keep
> the same bits as before? I'm afraid I don't remember, that thread was
> long :/
> 
> I'm asking because I'd like to depend on that bit to decide whether I
> can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> would be an easy thing to do, and I'd like to avoid mucking around with
> device-specific configuration from the transport.
> 
> To illustrate what I'm talking about, my current patchset for virtio-1
> on ccw is here:
> 
> git://github.com/cohuck/qemu virtio-1-ccw-2.5


I still think you are over-engineering it.

Just add a property to disable the modern interface.
Anyone using scsi passthrough will have to set that,
if not - above patch will make initialization fail.

-- 
MST

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote:
> 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 | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 4c27974..4716c3e 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -731,7 +731,14 @@ 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);
> +    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> +        if (s->conf.scsi) {
> +            error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");

A bit better:
"Virtio modern does not support scsi passthrough - please set disable-modern=on".

It might be even better if we could specify the device that failed the
initialization, but I don't know how to do that in a way that's
helpful to the user.

Markus?


> +            return 0;
> +        }
> +    } 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

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

[Daniel P. Berrange]

On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote:
> 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.

Why is SCSI passthrough support not available in virtio 1.0 ? This
will cause a regression for any users of that as & when QEMU changes
to use virtio 1.0 by default. Can we not fix this regression instead.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Jason Wang]

On 07/22/2015 04:58 PM, Cornelia Huck wrote:
> On Wed, 22 Jul 2015 13:59:51 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> 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 | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 4c27974..4716c3e 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -731,7 +731,14 @@ 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);
>> +    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;
>> +        }
>> +    } else {
>> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>> +    }
>>
>>      if (s->conf.config_wce) {
>>          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> Do we advertise F_SCSI even if scsi is not configured in order to keep
> the same bits as before? I'm afraid I don't remember, that thread was
> long :/
>
> I'm asking because I'd like to depend on that bit to decide whether I
> can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> would be an easy thing to do, and I'd like to avoid mucking around with
> device-specific configuration from the transport.

I don't see much difference. Both of our patches needs to set scsi to
off to have 1.0 device. And I don't see advantages that did it from
transport. If it has, we probably need something similar in virtio-pci.

>
> To illustrate what I'm talking about, my current patchset for virtio-1
> on ccw is here:
>
> git://github.com/cohuck/qemu virtio-1-ccw-2.5
>

Looks like patch "virtio-blk: scsi is legacy only" breaks backward
compatibility because VIRTIO_BLK_F_SCSI was not notified when scsi is off.

Thanks

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

[Jason Wang]

On 07/22/2015 05:31 PM, Daniel P. Berrange wrote:
> On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote:
>> 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.
> Why is SCSI passthrough support not available in virtio 1.0 ? This
> will cause a regression for any users of that as & when QEMU changes
> to use virtio 1.0 by default. Can we not fix this regression instead.
>
> Regards,
> Daniel


I can find some discussion here:

http://comments.gmane.org/gmane.comp.emulators.virtio.devel/865

Paolo may know more about this.

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

[Jason Wang]

On 07/22/2015 05:25 PM, Michael S. Tsirkin wrote:
> On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote:
>> 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 | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 4c27974..4716c3e 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -731,7 +731,14 @@ 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);
>> +    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
>> +        if (s->conf.scsi) {
>> +            error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
> A bit better:
> "Virtio modern does not support scsi passthrough - please set disable-modern=on".
>
> It might be even better if we could specify the device that failed the
> initialization, but I don't know how to do that in a way that's
> helpful to the user.

Looks like current error prompt has contained the device?

qemu-system-x86_64: -device
virtio-blk-pci,id=v0,scsi=on,disable-modern=off,drive=img1: Virtio 1.0
does not support scsi passthrough!

>
> Markus?
>
>
>> +            return 0;
>> +        }
>> +    } 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

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 05:52:29PM +0800, Jason Wang wrote:
> 
> 
> On 07/22/2015 05:25 PM, Michael S. Tsirkin wrote:
> > On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote:
> >> 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 | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 4c27974..4716c3e 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -731,7 +731,14 @@ 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);
> >> +    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> >> +        if (s->conf.scsi) {
> >> +            error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
> > A bit better:
> > "Virtio modern does not support scsi passthrough - please set disable-modern=on".
> >
> > It might be even better if we could specify the device that failed the
> > initialization, but I don't know how to do that in a way that's
> > helpful to the user.
> 
> Looks like current error prompt has contained the device?
> 
> qemu-system-x86_64: -device
> virtio-blk-pci,id=v0,scsi=on,disable-modern=off,drive=img1: Virtio 1.0
> does not support scsi passthrough!

Cool, then the message I suggested will contain all the
necessary information.

> >
> > Markus?
> >
> >
> >> +            return 0;
> >> +        }
> >> +    } 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

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 01:12:45PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 22, 2015 at 05:52:29PM +0800, Jason Wang wrote:
> > 
> > 
> > On 07/22/2015 05:25 PM, Michael S. Tsirkin wrote:
> > > On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote:
> > >> 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 | 9 ++++++++-
> > >>  1 file changed, 8 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > >> index 4c27974..4716c3e 100644
> > >> --- a/hw/block/virtio-blk.c
> > >> +++ b/hw/block/virtio-blk.c
> > >> @@ -731,7 +731,14 @@ 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);
> > >> +    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> > >> +        if (s->conf.scsi) {
> > >> +            error_setg(errp, "Virtio 1.0 does not support scsi passthrough!");
> > > A bit better:
> > > "Virtio modern does not support scsi passthrough - please set disable-modern=on".
> > >
> > > It might be even better if we could specify the device that failed the
> > > initialization, but I don't know how to do that in a way that's
> > > helpful to the user.
> > 
> > Looks like current error prompt has contained the device?
> > 
> > qemu-system-x86_64: -device
> > virtio-blk-pci,id=v0,scsi=on,disable-modern=off,drive=img1: Virtio 1.0
> > does not support scsi passthrough!
> 
> Cool, then the message I suggested will contain all the
> necessary information.

Maybe add "or switch to virtio-scsi".

> > >
> > > Markus?
> > >
> > >
> > >> +            return 0;
> > >> +        }
> > >> +    } 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

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 10:31:45AM +0100, Daniel P. Berrange wrote:
> On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote:
> > 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.
> 
> Why is SCSI passthrough support not available in virtio 1.0 ? This
> will cause a regression for any users of that as & when QEMU changes
> to use virtio 1.0 by default. Can we not fix this regression instead.
> 
> Regards,
> Daniel

If we wanted to, we might be able to fix this but not for 2.4: we'd have
to extend the spec and guest drivers, in some way TBD.

Paolo would be best placed to answer whether this feature is desirable
in the future, I think the argument made when the spec was written was that
the feature is not widely used, and virtio scsi is available as
a replacement for people who need it.


> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Cornelia Huck]

On Wed, 22 Jul 2015 17:35:07 +0800
Jason Wang <jasowang@redhat.com> wrote:

> 
> 
> On 07/22/2015 04:58 PM, Cornelia Huck wrote:
> > On Wed, 22 Jul 2015 13:59:51 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >
> >> 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 | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 4c27974..4716c3e 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -731,7 +731,14 @@ 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);
> >> +    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;
> >> +        }
> >> +    } else {
> >> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> >> +    }
> >>
> >>      if (s->conf.config_wce) {
> >>          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > the same bits as before? I'm afraid I don't remember, that thread was
> > long :/
> >
> > I'm asking because I'd like to depend on that bit to decide whether I
> > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > would be an easy thing to do, and I'd like to avoid mucking around with
> > device-specific configuration from the transport.
> 
> I don't see much difference. Both of our patches needs to set scsi to
> off to have 1.0 device. And I don't see advantages that did it from
> transport. If it has, we probably need something similar in virtio-pci.

The crux of the problem seems to be that pci and ccw have different
approaches on supporting legacy and modern devices...

> 
> >
> > To illustrate what I'm talking about, my current patchset for virtio-1
> > on ccw is here:
> >
> > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> >
> 
> Looks like patch "virtio-blk: scsi is legacy only" breaks backward
> compatibility because VIRTIO_BLK_F_SCSI was not notified when scsi is off.

Yeah, that's what I feared.

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

[Cornelia Huck]

On Wed, 22 Jul 2015 12:21:32 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > On Wed, 22 Jul 2015 13:59:51 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> > 
> > > 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 | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index 4c27974..4716c3e 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -731,7 +731,14 @@ 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);
> > > +    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;
> > > +        }
> > > +    } else {
> > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > +    }
> > > 
> > >      if (s->conf.config_wce) {
> > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > 
> > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > the same bits as before? I'm afraid I don't remember, that thread was
> > long :/
> > 
> > I'm asking because I'd like to depend on that bit to decide whether I
> > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > would be an easy thing to do, and I'd like to avoid mucking around with
> > device-specific configuration from the transport.
> > 
> > To illustrate what I'm talking about, my current patchset for virtio-1
> > on ccw is here:
> > 
> > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> 
> 
> I still think you are over-engineering it.
> 
> Just add a property to disable the modern interface.
> Anyone using scsi passthrough will have to set that,
> if not - above patch will make initialization fail.

And I still think requiring user action and not having this work
transparently is a bad idea...

Moreover, I will need a revision-fencing mechanism in any case, when we
introduce further revisions.

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote:
> On Wed, 22 Jul 2015 12:21:32 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > > On Wed, 22 Jul 2015 13:59:51 +0800
> > > Jason Wang <jasowang@redhat.com> wrote:
> > > 
> > > > 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 | 9 ++++++++-
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > index 4c27974..4716c3e 100644
> > > > --- a/hw/block/virtio-blk.c
> > > > +++ b/hw/block/virtio-blk.c
> > > > @@ -731,7 +731,14 @@ 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);
> > > > +    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;
> > > > +        }
> > > > +    } else {
> > > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > > +    }
> > > > 
> > > >      if (s->conf.config_wce) {
> > > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > > 
> > > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > > the same bits as before? I'm afraid I don't remember, that thread was
> > > long :/
> > > 
> > > I'm asking because I'd like to depend on that bit to decide whether I
> > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > > would be an easy thing to do, and I'd like to avoid mucking around with
> > > device-specific configuration from the transport.
> > > 
> > > To illustrate what I'm talking about, my current patchset for virtio-1
> > > on ccw is here:
> > > 
> > > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> > 
> > 
> > I still think you are over-engineering it.
> > 
> > Just add a property to disable the modern interface.
> > Anyone using scsi passthrough will have to set that,
> > if not - above patch will make initialization fail.
> 
> And I still think requiring user action and not having this work
> transparently is a bad idea...

Having what work transparently? SCSI passthrough?
Look, either you agree with Paolo or not.
Paolo thinks it's a deprecated hack not really worth supporting long term.
If you agree, I don't see why is asking for an extra property
such a bit deal. If you don't agree - please open a new thread
and argue about that.


> Moreover, I will need a revision-fencing mechanism in any case, when we
> introduce further revisions.

Why? Assuming we drop more features in the future?

-- 
MST

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

[Cornelia Huck]

On Wed, 22 Jul 2015 13:32:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote:
> > On Wed, 22 Jul 2015 12:21:32 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > > > On Wed, 22 Jul 2015 13:59:51 +0800
> > > > Jason Wang <jasowang@redhat.com> wrote:
> > > > 
> > > > > 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 | 9 ++++++++-
> > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > index 4c27974..4716c3e 100644
> > > > > --- a/hw/block/virtio-blk.c
> > > > > +++ b/hw/block/virtio-blk.c
> > > > > @@ -731,7 +731,14 @@ 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);
> > > > > +    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;
> > > > > +        }
> > > > > +    } else {
> > > > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > > > +    }
> > > > > 
> > > > >      if (s->conf.config_wce) {
> > > > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > > > 
> > > > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > > > the same bits as before? I'm afraid I don't remember, that thread was
> > > > long :/
> > > > 
> > > > I'm asking because I'd like to depend on that bit to decide whether I
> > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > > > would be an easy thing to do, and I'd like to avoid mucking around with
> > > > device-specific configuration from the transport.
> > > > 
> > > > To illustrate what I'm talking about, my current patchset for virtio-1
> > > > on ccw is here:
> > > > 
> > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> > > 
> > > 
> > > I still think you are over-engineering it.
> > > 
> > > Just add a property to disable the modern interface.
> > > Anyone using scsi passthrough will have to set that,
> > > if not - above patch will make initialization fail.
> > 
> > And I still think requiring user action and not having this work
> > transparently is a bad idea...
> 
> Having what work transparently? SCSI passthrough?
> Look, either you agree with Paolo or not.
> Paolo thinks it's a deprecated hack not really worth supporting long term.
> If you agree, I don't see why is asking for an extra property
> such a bit deal. If you don't agree - please open a new thread
> and argue about that.

I sometimes wonder whether we're arguing about the same thing :(

Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is
not. If I upgrade the host, I want the guests to be able to continue
using scsi without needing to fence virtio-1 off manually.

> 
> 
> > Moreover, I will need a revision-fencing mechanism in any case, when we
> > introduce further revisions.
> 
> Why? Assuming we drop more features in the future?

Revisions != features. Think new or changed channel commands, for
example.

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote:
> On Wed, 22 Jul 2015 13:32:17 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote:
> > > On Wed, 22 Jul 2015 12:21:32 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > > > > On Wed, 22 Jul 2015 13:59:51 +0800
> > > > > Jason Wang <jasowang@redhat.com> wrote:
> > > > > 
> > > > > > 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 | 9 ++++++++-
> > > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > > index 4c27974..4716c3e 100644
> > > > > > --- a/hw/block/virtio-blk.c
> > > > > > +++ b/hw/block/virtio-blk.c
> > > > > > @@ -731,7 +731,14 @@ 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);
> > > > > > +    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;
> > > > > > +        }
> > > > > > +    } else {
> > > > > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > > > > +    }
> > > > > > 
> > > > > >      if (s->conf.config_wce) {
> > > > > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > > > > 
> > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > > > > the same bits as before? I'm afraid I don't remember, that thread was
> > > > > long :/
> > > > > 
> > > > > I'm asking because I'd like to depend on that bit to decide whether I
> > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > > > > would be an easy thing to do, and I'd like to avoid mucking around with
> > > > > device-specific configuration from the transport.
> > > > > 
> > > > > To illustrate what I'm talking about, my current patchset for virtio-1
> > > > > on ccw is here:
> > > > > 
> > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> > > > 
> > > > 
> > > > I still think you are over-engineering it.
> > > > 
> > > > Just add a property to disable the modern interface.
> > > > Anyone using scsi passthrough will have to set that,
> > > > if not - above patch will make initialization fail.
> > > 
> > > And I still think requiring user action and not having this work
> > > transparently is a bad idea...
> > 
> > Having what work transparently? SCSI passthrough?
> > Look, either you agree with Paolo or not.
> > Paolo thinks it's a deprecated hack not really worth supporting long term.
> > If you agree, I don't see why is asking for an extra property
> > such a bit deal. If you don't agree - please open a new thread
> > and argue about that.
> 
> I sometimes wonder whether we're arguing about the same thing :(
> 
> Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is
> not. If I upgrade the host, I want the guests to be able to continue
> using scsi without needing to fence virtio-1 off manually.

Paolo's argument is that no one should be using scsi passthrough.

If the feature has users, we should bring it back into virtio 1.

If almost one uses it, then no one will suffer too much from getting
an error message saying "please set disable-modern=on".

And there's no reason to make it behave differently
between ccw and pci.


> > 
> > 
> > > Moreover, I will need a revision-fencing mechanism in any case, when we
> > > introduce further revisions.
> > 
> > Why? Assuming we drop more features in the future?
> 
> Revisions != features. Think new or changed channel commands, for
> example.

You likely can just add these unconditionally.
How is this related to virtio blk at all?

-- 
MST

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

[Cornelia Huck]

On Wed, 22 Jul 2015 13:44:14 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote:
> > On Wed, 22 Jul 2015 13:32:17 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote:
> > > > On Wed, 22 Jul 2015 12:21:32 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > > > > > On Wed, 22 Jul 2015 13:59:51 +0800
> > > > > > Jason Wang <jasowang@redhat.com> wrote:
> > > > > > 
> > > > > > > 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 | 9 ++++++++-
> > > > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > > > index 4c27974..4716c3e 100644
> > > > > > > --- a/hw/block/virtio-blk.c
> > > > > > > +++ b/hw/block/virtio-blk.c
> > > > > > > @@ -731,7 +731,14 @@ 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);
> > > > > > > +    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;
> > > > > > > +        }
> > > > > > > +    } else {
> > > > > > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > > > > > +    }
> > > > > > > 
> > > > > > >      if (s->conf.config_wce) {
> > > > > > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > > > > > 
> > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > > > > > the same bits as before? I'm afraid I don't remember, that thread was
> > > > > > long :/
> > > > > > 
> > > > > > I'm asking because I'd like to depend on that bit to decide whether I
> > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > > > > > would be an easy thing to do, and I'd like to avoid mucking around with
> > > > > > device-specific configuration from the transport.
> > > > > > 
> > > > > > To illustrate what I'm talking about, my current patchset for virtio-1
> > > > > > on ccw is here:
> > > > > > 
> > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> > > > > 
> > > > > 
> > > > > I still think you are over-engineering it.
> > > > > 
> > > > > Just add a property to disable the modern interface.
> > > > > Anyone using scsi passthrough will have to set that,
> > > > > if not - above patch will make initialization fail.
> > > > 
> > > > And I still think requiring user action and not having this work
> > > > transparently is a bad idea...
> > > 
> > > Having what work transparently? SCSI passthrough?
> > > Look, either you agree with Paolo or not.
> > > Paolo thinks it's a deprecated hack not really worth supporting long term.
> > > If you agree, I don't see why is asking for an extra property
> > > such a bit deal. If you don't agree - please open a new thread
> > > and argue about that.
> > 
> > I sometimes wonder whether we're arguing about the same thing :(
> > 
> > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is
> > not. If I upgrade the host, I want the guests to be able to continue
> > using scsi without needing to fence virtio-1 off manually.
> 
> Paolo's argument is that no one should be using scsi passthrough.
> 
> If the feature has users, we should bring it back into virtio 1.
> 
> If almost one uses it, then no one will suffer too much from getting
> an error message saying "please set disable-modern=on".

And here's where we disagree. Even if it's exotic, I don't want to
break existing users.

> And there's no reason to make it behave differently
> between ccw and pci.
> 
> 
> > > 
> > > 
> > > > Moreover, I will need a revision-fencing mechanism in any case, when we
> > > > introduce further revisions.
> > > 
> > > Why? Assuming we drop more features in the future?
> > 
> > Revisions != features. Think new or changed channel commands, for
> > example.
> 
> You likely can just add these unconditionally.

Backwards compatibility?

> How is this related to virtio blk at all?

It isn't, revision handling is generic. It's just the scsi bit that
triggered it.

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

[Paolo Bonzini]

On 22/07/2015 12:19, Michael S. Tsirkin wrote:
> > > 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.
> > 
> > Why is SCSI passthrough support not available in virtio 1.0 ? This
> > will cause a regression for any users of that as & when QEMU changes
> > to use virtio 1.0 by default. Can we not fix this regression instead.
>
> If we wanted to, we might be able to fix this but not for 2.4: we'd have
> to extend the spec and guest drivers, in some way TBD.
> 
> Paolo would be best placed to answer whether this feature is desirable
> in the future, I think the argument made when the spec was written was that
> the feature is not widely used, and virtio scsi is available as
> a replacement for people who need it.

No, the feature is not desirable in the future. There is no reason
really not to use virtio-scsi passthrough instead, since virtio-scsi has
been out for about 3 years now and is stable.

In addition, the implementation would either not be compatible with
virtio 0.9, or would be different from everything else in the spec
because it requires a particular framing for the buffers.

Paolo

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

[Daniel P. Berrange]

On Wed, Jul 22, 2015 at 01:40:25PM +0200, Paolo Bonzini wrote:
> 
> 
> On 22/07/2015 12:19, Michael S. Tsirkin wrote:
> > > > 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.
> > > 
> > > Why is SCSI passthrough support not available in virtio 1.0 ? This
> > > will cause a regression for any users of that as & when QEMU changes
> > > to use virtio 1.0 by default. Can we not fix this regression instead.
> >
> > If we wanted to, we might be able to fix this but not for 2.4: we'd have
> > to extend the spec and guest drivers, in some way TBD.
> > 
> > Paolo would be best placed to answer whether this feature is desirable
> > in the future, I think the argument made when the spec was written was that
> > the feature is not widely used, and virtio scsi is available as
> > a replacement for people who need it.
> 
> No, the feature is not desirable in the future. There is no reason
> really not to use virtio-scsi passthrough instead, since virtio-scsi has
> been out for about 3 years now and is stable.
> 
> In addition, the implementation would either not be compatible with
> virtio 0.9, or would be different from everything else in the spec
> because it requires a particular framing for the buffers.

IIUC, the SCSI passthrough feature for virtio-blk is enabled by
setting the 'scsi=on' property on the virtio-blk device, which is
exposed by libvirt with XML:

    <disk type='block' device='lun'>
      <driver name='qemu' type='raw'/>
      <source dev='/dev/sda'/>
      <target dev='vda' bus='virtio'/>
    </disk>

(For use with virtio-scsi you'd just change the <target> element)

So if the guest is using virtio-1.0, then this will now fail to boot, or
cause an error from monitor hotplug. This is not too bad, but I'm just
wondering if there's anything else we ought to think about doing in libvirt
in this situation. Normally we'd try to detect unsupported things upfront
so we can report VIR_ERR_CONFIG_UNSUPPORTED, instead of the generic error
code VIR_ERR_INTERNAL_ERROR, but perhaps this is sufficiently niche to
not worry about it and its fine to just delegate error reporting to QEMU ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Paolo Bonzini]

On 22/07/2015 13:46, Daniel P. Berrange wrote:
> IIUC, the SCSI passthrough feature for virtio-blk is enabled by
> setting the 'scsi=on' property on the virtio-blk device, which is
> exposed by libvirt with XML:
> 
>     <disk type='block' device='lun'>
>       <driver name='qemu' type='raw'/>
>       <source dev='/dev/sda'/>
>       <target dev='vda' bus='virtio'/>
>     </disk>
> 
> (For use with virtio-scsi you'd just change the <target> element)
> 
> So if the guest is using virtio-1.0, then this will now fail to boot, or
> cause an error from monitor hotplug. This is not too bad, but I'm just
> wondering if there's anything else we ought to think about doing in libvirt
> in this situation. Normally we'd try to detect unsupported things upfront
> so we can report VIR_ERR_CONFIG_UNSUPPORTED, instead of the generic error
> code VIR_ERR_INTERNAL_ERROR, but perhaps this is sufficiently niche to
> not worry about it and its fine to just delegate error reporting to QEMU ?

Probably.  Note that it will be a long time before the default is
changed to 1.0 (if it ever will).  Perhaps you can start warning now
about <disk type='block' device='lun'>, and suggest using virtio-scsi
instead?

Paolo

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 12:55:22PM +0200, Cornelia Huck wrote:
> On Wed, 22 Jul 2015 13:44:14 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote:
> > > On Wed, 22 Jul 2015 13:32:17 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote:
> > > > > On Wed, 22 Jul 2015 12:21:32 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > > > > > > On Wed, 22 Jul 2015 13:59:51 +0800
> > > > > > > Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > 
> > > > > > > > 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 | 9 ++++++++-
> > > > > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > > > > index 4c27974..4716c3e 100644
> > > > > > > > --- a/hw/block/virtio-blk.c
> > > > > > > > +++ b/hw/block/virtio-blk.c
> > > > > > > > @@ -731,7 +731,14 @@ 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);
> > > > > > > > +    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;
> > > > > > > > +        }
> > > > > > > > +    } else {
> > > > > > > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > > > > > > +    }
> > > > > > > > 
> > > > > > > >      if (s->conf.config_wce) {
> > > > > > > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > > > > > > 
> > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > > > > > > the same bits as before? I'm afraid I don't remember, that thread was
> > > > > > > long :/
> > > > > > > 
> > > > > > > I'm asking because I'd like to depend on that bit to decide whether I
> > > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > > > > > > would be an easy thing to do, and I'd like to avoid mucking around with
> > > > > > > device-specific configuration from the transport.
> > > > > > > 
> > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1
> > > > > > > on ccw is here:
> > > > > > > 
> > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> > > > > > 
> > > > > > 
> > > > > > I still think you are over-engineering it.
> > > > > > 
> > > > > > Just add a property to disable the modern interface.
> > > > > > Anyone using scsi passthrough will have to set that,
> > > > > > if not - above patch will make initialization fail.
> > > > > 
> > > > > And I still think requiring user action and not having this work
> > > > > transparently is a bad idea...
> > > > 
> > > > Having what work transparently? SCSI passthrough?
> > > > Look, either you agree with Paolo or not.
> > > > Paolo thinks it's a deprecated hack not really worth supporting long term.
> > > > If you agree, I don't see why is asking for an extra property
> > > > such a bit deal. If you don't agree - please open a new thread
> > > > and argue about that.
> > > 
> > > I sometimes wonder whether we're arguing about the same thing :(
> > > 
> > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is
> > > not. If I upgrade the host, I want the guests to be able to continue
> > > using scsi without needing to fence virtio-1 off manually.
> > 
> > Paolo's argument is that no one should be using scsi passthrough.
> > 
> > If the feature has users, we should bring it back into virtio 1.
> > 
> > If almost one uses it, then no one will suffer too much from getting
> > an error message saying "please set disable-modern=on".
> 
> And here's where we disagree. Even if it's exotic, I don't want to
> break existing users.

You should take this disagreement to the virtio TC.  QEMU merely
implements what the spec voted by TC says.

> > And there's no reason to make it behave differently
> > between ccw and pci.
> > 
> > 
> > > > 
> > > > 
> > > > > Moreover, I will need a revision-fencing mechanism in any case, when we
> > > > > introduce further revisions.
> > > > 
> > > > Why? Assuming we drop more features in the future?
> > > 
> > > Revisions != features. Think new or changed channel commands, for
> > > example.
> > 
> > You likely can just add these unconditionally.
> 
> Backwards compatibility?

Compatibility is built-in to revision negotiation, isn't it?

> > How is this related to virtio blk at all?
> 
> It isn't, revision handling is generic. It's just the scsi bit that
> triggered it.

-- 
MST

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 01:53:14PM +0200, Paolo Bonzini wrote:
> 
> 
> On 22/07/2015 13:46, Daniel P. Berrange wrote:
> > IIUC, the SCSI passthrough feature for virtio-blk is enabled by
> > setting the 'scsi=on' property on the virtio-blk device, which is
> > exposed by libvirt with XML:
> > 
> >     <disk type='block' device='lun'>
> >       <driver name='qemu' type='raw'/>
> >       <source dev='/dev/sda'/>
> >       <target dev='vda' bus='virtio'/>
> >     </disk>
> > 
> > (For use with virtio-scsi you'd just change the <target> element)
> > 
> > So if the guest is using virtio-1.0, then this will now fail to boot, or
> > cause an error from monitor hotplug. This is not too bad, but I'm just
> > wondering if there's anything else we ought to think about doing in libvirt
> > in this situation. Normally we'd try to detect unsupported things upfront
> > so we can report VIR_ERR_CONFIG_UNSUPPORTED, instead of the generic error
> > code VIR_ERR_INTERNAL_ERROR, but perhaps this is sufficiently niche to
> > not worry about it and its fine to just delegate error reporting to QEMU ?
> 
> Probably.  Note that it will be a long time before the default is
> changed to 1.0 (if it ever will).

I fully expect we'll enable modern by default in 2.5.  We'll also
disable legacy if the bus used is pcie.  We can disable modern if bus is
pci and scsi passthrough was requested.

>  Perhaps you can start warning now
> about <disk type='block' device='lun'>, and suggest using virtio-scsi
> instead?
> 
> Paolo

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

[Cornelia Huck]

On Wed, 22 Jul 2015 17:53:47 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 22, 2015 at 12:55:22PM +0200, Cornelia Huck wrote:
> > On Wed, 22 Jul 2015 13:44:14 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote:
> > > > On Wed, 22 Jul 2015 13:32:17 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote:
> > > > > > On Wed, 22 Jul 2015 12:21:32 +0300
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > 
> > > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > > > > > > > On Wed, 22 Jul 2015 13:59:51 +0800
> > > > > > > > Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > 
> > > > > > > > > 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 | 9 ++++++++-
> > > > > > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > > > > > index 4c27974..4716c3e 100644
> > > > > > > > > --- a/hw/block/virtio-blk.c
> > > > > > > > > +++ b/hw/block/virtio-blk.c
> > > > > > > > > @@ -731,7 +731,14 @@ 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);
> > > > > > > > > +    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;
> > > > > > > > > +        }
> > > > > > > > > +    } else {
> > > > > > > > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > > > > > > > +    }
> > > > > > > > > 
> > > > > > > > >      if (s->conf.config_wce) {
> > > > > > > > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > > > > > > > 
> > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > > > > > > > the same bits as before? I'm afraid I don't remember, that thread was
> > > > > > > > long :/
> > > > > > > > 
> > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I
> > > > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > > > > > > > would be an easy thing to do, and I'd like to avoid mucking around with
> > > > > > > > device-specific configuration from the transport.
> > > > > > > > 
> > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1
> > > > > > > > on ccw is here:
> > > > > > > > 
> > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> > > > > > > 
> > > > > > > 
> > > > > > > I still think you are over-engineering it.
> > > > > > > 
> > > > > > > Just add a property to disable the modern interface.
> > > > > > > Anyone using scsi passthrough will have to set that,
> > > > > > > if not - above patch will make initialization fail.
> > > > > > 
> > > > > > And I still think requiring user action and not having this work
> > > > > > transparently is a bad idea...
> > > > > 
> > > > > Having what work transparently? SCSI passthrough?
> > > > > Look, either you agree with Paolo or not.
> > > > > Paolo thinks it's a deprecated hack not really worth supporting long term.
> > > > > If you agree, I don't see why is asking for an extra property
> > > > > such a bit deal. If you don't agree - please open a new thread
> > > > > and argue about that.
> > > > 
> > > > I sometimes wonder whether we're arguing about the same thing :(
> > > > 
> > > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is
> > > > not. If I upgrade the host, I want the guests to be able to continue
> > > > using scsi without needing to fence virtio-1 off manually.
> > > 
> > > Paolo's argument is that no one should be using scsi passthrough.
> > > 
> > > If the feature has users, we should bring it back into virtio 1.
> > > 
> > > If almost one uses it, then no one will suffer too much from getting
> > > an error message saying "please set disable-modern=on".
> > 
> > And here's where we disagree. Even if it's exotic, I don't want to
> > break existing users.
> 
> You should take this disagreement to the virtio TC.  QEMU merely
> implements what the spec voted by TC says.

I fail to see why this is a spec issue. It sounds like a qemu
implementation question to me.

> 
> > > And there's no reason to make it behave differently
> > > between ccw and pci.
> > > 
> > > 
> > > > > 
> > > > > 
> > > > > > Moreover, I will need a revision-fencing mechanism in any case, when we
> > > > > > introduce further revisions.
> > > > > 
> > > > > Why? Assuming we drop more features in the future?
> > > > 
> > > > Revisions != features. Think new or changed channel commands, for
> > > > example.
> > > 
> > > You likely can just add these unconditionally.
> > 
> > Backwards compatibility?
> 
> Compatibility is built-in to revision negotiation, isn't it?

Yes, but we still want to support migration to older releases.

> 
> > > How is this related to virtio blk at all?
> > 
> > It isn't, revision handling is generic. It's just the scsi bit that
> > triggered it.
> 

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 01:40:25PM +0200, Paolo Bonzini wrote:
> 
> 
> On 22/07/2015 12:19, Michael S. Tsirkin wrote:
> > > > 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.
> > > 
> > > Why is SCSI passthrough support not available in virtio 1.0 ? This
> > > will cause a regression for any users of that as & when QEMU changes
> > > to use virtio 1.0 by default. Can we not fix this regression instead.
> >
> > If we wanted to, we might be able to fix this but not for 2.4: we'd have
> > to extend the spec and guest drivers, in some way TBD.
> > 
> > Paolo would be best placed to answer whether this feature is desirable
> > in the future, I think the argument made when the spec was written was that
> > the feature is not widely used, and virtio scsi is available as
> > a replacement for people who need it.
> 
> No, the feature is not desirable in the future. There is no reason
> really not to use virtio-scsi passthrough instead, since virtio-scsi has
> been out for about 3 years now and is stable.
> 

Given the amount of work we are spending handling this corner case,
I'm beginning to think we should just bring it back in.

> In addition, the implementation would either not be compatible with
> virtio 0.9, or would be different from everything else in the spec
> because it requires a particular framing for the buffers.
> 
> Paolo
Of course we'll need some kind of change to avoid depending on framing
of buffers, but how hard is it? Just stick the header size
somewhere in the buffer or in the config space.

Not compatible is probably not the right term to use here,
since when using the legacy interface we can make the old
framing assumption.


-- 
MST

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

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 06:11:16PM +0200, Cornelia Huck wrote:
> On Wed, 22 Jul 2015 17:53:47 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 22, 2015 at 12:55:22PM +0200, Cornelia Huck wrote:
> > > On Wed, 22 Jul 2015 13:44:14 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote:
> > > > > On Wed, 22 Jul 2015 13:32:17 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote:
> > > > > > > On Wed, 22 Jul 2015 12:21:32 +0300
> > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > 
> > > > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > > > > > > > > On Wed, 22 Jul 2015 13:59:51 +0800
> > > > > > > > > Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > > 
> > > > > > > > > > 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 | 9 ++++++++-
> > > > > > > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > > > > > > index 4c27974..4716c3e 100644
> > > > > > > > > > --- a/hw/block/virtio-blk.c
> > > > > > > > > > +++ b/hw/block/virtio-blk.c
> > > > > > > > > > @@ -731,7 +731,14 @@ 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);
> > > > > > > > > > +    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;
> > > > > > > > > > +        }
> > > > > > > > > > +    } else {
> > > > > > > > > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > > > > > > > > +    }
> > > > > > > > > > 
> > > > > > > > > >      if (s->conf.config_wce) {
> > > > > > > > > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > > > > > > > > 
> > > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > > > > > > > > the same bits as before? I'm afraid I don't remember, that thread was
> > > > > > > > > long :/
> > > > > > > > > 
> > > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I
> > > > > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > > > > > > > > would be an easy thing to do, and I'd like to avoid mucking around with
> > > > > > > > > device-specific configuration from the transport.
> > > > > > > > > 
> > > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1
> > > > > > > > > on ccw is here:
> > > > > > > > > 
> > > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> > > > > > > > 
> > > > > > > > 
> > > > > > > > I still think you are over-engineering it.
> > > > > > > > 
> > > > > > > > Just add a property to disable the modern interface.
> > > > > > > > Anyone using scsi passthrough will have to set that,
> > > > > > > > if not - above patch will make initialization fail.
> > > > > > > 
> > > > > > > And I still think requiring user action and not having this work
> > > > > > > transparently is a bad idea...
> > > > > > 
> > > > > > Having what work transparently? SCSI passthrough?
> > > > > > Look, either you agree with Paolo or not.
> > > > > > Paolo thinks it's a deprecated hack not really worth supporting long term.
> > > > > > If you agree, I don't see why is asking for an extra property
> > > > > > such a bit deal. If you don't agree - please open a new thread
> > > > > > and argue about that.
> > > > > 
> > > > > I sometimes wonder whether we're arguing about the same thing :(
> > > > > 
> > > > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is
> > > > > not. If I upgrade the host, I want the guests to be able to continue
> > > > > using scsi without needing to fence virtio-1 off manually.
> > > > 
> > > > Paolo's argument is that no one should be using scsi passthrough.
> > > > 
> > > > If the feature has users, we should bring it back into virtio 1.
> > > > 
> > > > If almost one uses it, then no one will suffer too much from getting
> > > > an error message saying "please set disable-modern=on".
> > > 
> > > And here's where we disagree. Even if it's exotic, I don't want to
> > > break existing users.
> > 
> > You should take this disagreement to the virtio TC.  QEMU merely
> > implements what the spec voted by TC says.
> 
> I fail to see why this is a spec issue. It sounds like a qemu
> implementation question to me.

Pls re-read the discussion around removing this issue then.


virtio 1 should be a super-set of virtio functionality-wise.


Paolo said this specific feature has been deprecated for years, no
one should be using it, so we dropped it from spec.


> > 
> > > > And there's no reason to make it behave differently
> > > > between ccw and pci.
> > > > 
> > > > 
> > > > > > 
> > > > > > 
> > > > > > > Moreover, I will need a revision-fencing mechanism in any case, when we
> > > > > > > introduce further revisions.
> > > > > > 
> > > > > > Why? Assuming we drop more features in the future?
> > > > > 
> > > > > Revisions != features. Think new or changed channel commands, for
> > > > > example.
> > > > 
> > > > You likely can just add these unconditionally.
> > > 
> > > Backwards compatibility?
> > 
> > Compatibility is built-in to revision negotiation, isn't it?
> 
> Yes, but we still want to support migration to older releases.

I don't think you can do anything in virtio to make this work
for PPC.  As long as you don't version machine types, cross version
migration will be broken.



> > 
> > > > How is this related to virtio blk at all?
> > > 
> > > It isn't, revision handling is generic. It's just the scsi bit that
> > > triggered it.
> > 

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

[Paolo Bonzini]

On 22/07/2015 18:15, Michael S. Tsirkin wrote:
> > No, the feature is not desirable in the future. There is no reason
> > really not to use virtio-scsi passthrough instead, since virtio-scsi has
> > been out for about 3 years now and is stable.
> 
> Given the amount of work we are spending handling this corner case,
> I'm beginning to think we should just bring it back in.

Please don't.  Perhaps it made sense when Rusty was thinking of
virtio-blk as simply "struct request over virtio", but I'm not even sure
what the use case is.

For virtio-scsi, for example, one reason to use passthrough is that you
can use the same /dev/disk/by-id path on physical and virtual machines.
 But that doesn't work with virtio-blk's pseudo passthrough.

Another is to pass "weird" devices (tapes, media changers, etc.) to
backup appliances, but that also doesn't work with virtio-blk.

So what is it used for?  That's what is needed to make an informed decision.

> > In addition, the implementation would either not be compatible with
> > virtio 0.9, or would be different from everything else in the spec
> > because it requires a particular framing for the buffers.
> 
> Of course we'll need some kind of change to avoid depending on framing
> of buffers, but how hard is it? Just stick the header size
> somewhere in the buffer or in the config space.
> 
> Not compatible is probably not the right term to use here,
> since when using the legacy interface we can make the old
> framing assumption.

Not compatible in the sense that it requires work in the drivers too
(unlike e.g. CONFIG_WCE, where the old code just works with the proposed
modifications to 1.0).

Paolo

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

[Cornelia Huck]

On Wed, 22 Jul 2015 19:34:31 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 22, 2015 at 06:11:16PM +0200, Cornelia Huck wrote:
> > On Wed, 22 Jul 2015 17:53:47 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 22, 2015 at 12:55:22PM +0200, Cornelia Huck wrote:
> > > > On Wed, 22 Jul 2015 13:44:14 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote:
> > > > > > On Wed, 22 Jul 2015 13:32:17 +0300
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > 
> > > > > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote:
> > > > > > > > On Wed, 22 Jul 2015 12:21:32 +0300
> > > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > > 
> > > > > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > > > > > > > > > On Wed, 22 Jul 2015 13:59:51 +0800
> > > > > > > > > > Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > > > 
> > > > > > > > > > > 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 | 9 ++++++++-
> > > > > > > > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > > > > > > > index 4c27974..4716c3e 100644
> > > > > > > > > > > --- a/hw/block/virtio-blk.c
> > > > > > > > > > > +++ b/hw/block/virtio-blk.c
> > > > > > > > > > > @@ -731,7 +731,14 @@ 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);
> > > > > > > > > > > +    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;
> > > > > > > > > > > +        }
> > > > > > > > > > > +    } else {
> > > > > > > > > > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > > > > > > > > > +    }
> > > > > > > > > > > 
> > > > > > > > > > >      if (s->conf.config_wce) {
> > > > > > > > > > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > > > > > > > > > 
> > > > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > > > > > > > > > the same bits as before? I'm afraid I don't remember, that thread was
> > > > > > > > > > long :/
> > > > > > > > > > 
> > > > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I
> > > > > > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > > > > > > > > > would be an easy thing to do, and I'd like to avoid mucking around with
> > > > > > > > > > device-specific configuration from the transport.
> > > > > > > > > > 
> > > > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1
> > > > > > > > > > on ccw is here:
> > > > > > > > > > 
> > > > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I still think you are over-engineering it.
> > > > > > > > > 
> > > > > > > > > Just add a property to disable the modern interface.
> > > > > > > > > Anyone using scsi passthrough will have to set that,
> > > > > > > > > if not - above patch will make initialization fail.
> > > > > > > > 
> > > > > > > > And I still think requiring user action and not having this work
> > > > > > > > transparently is a bad idea...
> > > > > > > 
> > > > > > > Having what work transparently? SCSI passthrough?
> > > > > > > Look, either you agree with Paolo or not.
> > > > > > > Paolo thinks it's a deprecated hack not really worth supporting long term.
> > > > > > > If you agree, I don't see why is asking for an extra property
> > > > > > > such a bit deal. If you don't agree - please open a new thread
> > > > > > > and argue about that.
> > > > > > 
> > > > > > I sometimes wonder whether we're arguing about the same thing :(
> > > > > > 
> > > > > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is
> > > > > > not. If I upgrade the host, I want the guests to be able to continue
> > > > > > using scsi without needing to fence virtio-1 off manually.
> > > > > 
> > > > > Paolo's argument is that no one should be using scsi passthrough.
> > > > > 
> > > > > If the feature has users, we should bring it back into virtio 1.
> > > > > 
> > > > > If almost one uses it, then no one will suffer too much from getting
> > > > > an error message saying "please set disable-modern=on".
> > > > 
> > > > And here's where we disagree. Even if it's exotic, I don't want to
> > > > break existing users.
> > > 
> > > You should take this disagreement to the virtio TC.  QEMU merely
> > > implements what the spec voted by TC says.
> > 
> > I fail to see why this is a spec issue. It sounds like a qemu
> > implementation question to me.
> 
> Pls re-read the discussion around removing this issue then.
> 
> 
> virtio 1 should be a super-set of virtio functionality-wise.

Perhaps this assumption is the problem then? scsi seems to be the only
odd one, but still...

> 
> 
> Paolo said this specific feature has been deprecated for years, no
> one should be using it, so we dropped it from spec.

Nothing wrong with that. But "it's deprecated" does not mean "nobody's
using it", I fear. The question is: Should qemu accommodate those users?

> 
> 
> > > 
> > > > > And there's no reason to make it behave differently
> > > > > between ccw and pci.
> > > > > 
> > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > Moreover, I will need a revision-fencing mechanism in any case, when we
> > > > > > > > introduce further revisions.
> > > > > > > 
> > > > > > > Why? Assuming we drop more features in the future?
> > > > > > 
> > > > > > Revisions != features. Think new or changed channel commands, for
> > > > > > example.
> > > > > 
> > > > > You likely can just add these unconditionally.
> > > > 
> > > > Backwards compatibility?
> > > 
> > > Compatibility is built-in to revision negotiation, isn't it?
> > 
> > Yes, but we still want to support migration to older releases.
> 
> I don't think you can do anything in virtio to make this work
> for PPC.  As long as you don't version machine types, cross version
> migration will be broken.

ccw is s390x :)

And we added versioned machines for 2.4, so yes, we care.

Which brings me to the next problem:

Assuming we want to make blocking VERSION_1 devices user-configurable,
we'll need a max_revision property or something like that. (pci's
legacy/modern approach just won't cut it, I fear, since we'll need to
handle higher revisions for later cross-version migration purposes as
well.) This implies we need to add this max_rev field to virtio-ccw's
migration stream _now_ (for 2.4).

I've started hacking up a patch, but I'm no way finished.

So:
- checking scsi flag in features for revision fencing won't work (we
  always need to advertise it for !VERSION_1)
- checking if it's a blk device with scsi configured for revision
  fencing is so ugly that I won't even try to code it
- which leaves setting VERSION_1 from the start and have virtio-blk
  fence VERSION_1 + scsi. For which I need the max_revision property
  with the issues I outlined above.

Not sure how to get out of this pickle.

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

[Paolo Bonzini]

On 23/07/2015 11:07, Cornelia Huck wrote:
> > Paolo said this specific feature has been deprecated for years, no
> > one should be using it, so we dropped it from spec.
>
> Nothing wrong with that. But "it's deprecated" does not mean "nobody's
> using it", I fear. The question is: Should qemu accommodate those users?

"It's deprecated" does mean (or at least can mean) "it's going to go
away in relatively exceptional circumstances".  virtio 1 counts as
relatively exceptional, I think.

Paolo

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

[Cornelia Huck]

On Thu, 23 Jul 2015 11:37:44 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 23/07/2015 11:07, Cornelia Huck wrote:
> > > Paolo said this specific feature has been deprecated for years, no
> > > one should be using it, so we dropped it from spec.
> >
> > Nothing wrong with that. But "it's deprecated" does not mean "nobody's
> > using it", I fear. The question is: Should qemu accommodate those users?
> 
> "It's deprecated" does mean (or at least can mean) "it's going to go
> away in relatively exceptional circumstances".  virtio 1 counts as
> relatively exceptional, I think.

OK, if the consensus is "you should not be using it; but if you really
need it, you can get it back with a config switch", I won't oppose that.