[Qemu-devel] [PATCH V2 0/5] Set correct blk feature for virtio 1.0

[Qemu-devel] [PATCH V2 0/5] 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 by, 1) disable
  scsi by defautl for 2.4 machine type and fail the initialization
  when both scsi and 1.0 were set.
- any layout must be set for transitional device. This is done by set
  any layout when 1.0 is supported.

Please review

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 (5):
  virtio: get_features() can fail
  virtio-blk: advertise scsi only when scsi is set
  virtio-blk: disable scsi passthrough by default
  virtio-blk: fail the init when both 1.0 and scsi is 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       | 16 +++++++++++++---
 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/compat.h         |  6 +++++-
 include/hw/virtio/virtio.h  |  4 +++-
 13 files changed, 40 insertions(+), 15 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH V2 1/5] 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 V2 2/5] virtio-blk: advertise scsi only when scsi is set

[Jason Wang]

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 4c27974..761d763 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -731,7 +731,9 @@ 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 (s->conf.scsi) {
+        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 V2 3/5] virtio-blk: disable scsi passthrough by default

[Jason Wang]

Disable scsi passthrough by default since it was incompatible with
virtio 1.0. For legacy machine types, keep this on by default.

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 +-
 include/hw/compat.h   | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 761d763..362fe53 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -964,7 +964,7 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial),
     DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true),
 #ifdef __linux__
-    DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, true),
+    DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, false),
 #endif
     DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
                     true),
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 4a43466..56039d8 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,11 @@
 #define HW_COMPAT_H
 
 #define HW_COMPAT_2_3 \
-        /* empty */
+        {\
+            .driver   = "virtio-blk-pci",\
+            .property = "scsi",\
+            .value    = "on",\
+        },
 
 #define HW_COMPAT_2_2 \
         /* empty */
-- 
2.1.4

[Qemu-devel] [PATCH V2 4/5] virtio-blk: fail the init when both 1.0 and scsi is set

[Jason Wang]

Scsi passthrough was no longer supported in 1.0, so fail the
initialization when user want both features.

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 362fe53..4a0ef68 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -732,6 +732,10 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
     virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
     virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
     if (s->conf.scsi) {
+        if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
+            error_setg(errp, "Virtio 1.0 does not support scsi passthrough");
+            return 0;
+        }
         virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
     }
 
-- 
2.1.4

[Qemu-devel] [PATCH V2 5/5] 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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 4a0ef68..78b1f1e 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -738,6 +738,9 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
         }
         virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
     }
+    if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
+        virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
+    }
 
     if (s->conf.config_wce) {
         virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
-- 
2.1.4

Re: [Qemu-devel] [PATCH V2 2/5] virtio-blk: advertise scsi only when scsi is set

[Paolo Bonzini]

On 15/07/2015 07:29, Jason Wang wrote:
> 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 4c27974..761d763 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -731,7 +731,9 @@ 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 (s->conf.scsi) {
> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> +    }

This must only be done for newer machine types only, or you change guest
ABI for scsi=off.  Effectively you have to split it in two properties,
"scsi" and "always_set_f_scsi".

Paolo

Re: [Qemu-devel] [PATCH V2 2/5] virtio-blk: advertise scsi only when scsi is set

[Jason Wang]

On 07/15/2015 03:57 PM, Paolo Bonzini wrote:
>
> On 15/07/2015 07:29, Jason Wang wrote:
>> 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 | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 4c27974..761d763 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -731,7 +731,9 @@ 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 (s->conf.scsi) {
>> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>> +    }
> This must only be done for newer machine types only, or you change guest
> ABI for scsi=off.  Effectively you have to split it in two properties,
> "scsi" and "always_set_f_scsi".
>
> Paolo

And always_set_f_scsi is true only for legacy machine types?

Re: [Qemu-devel] [PATCH V2 2/5] virtio-blk: advertise scsi only when scsi is set

[Paolo Bonzini]

On 15/07/2015 10:31, Jason Wang wrote:
> > This must only be done for newer machine types only, or you change guest
> > ABI for scsi=off.  Effectively you have to split it in two properties,
> > "scsi" and "always_set_f_scsi".
>
> And always_set_f_scsi is true only for legacy machine types?

s/legacy/older/ :)

It's also ignored for modern devices.  So perhaps you could name it to
transitional_force_f_scsi or something like that.

Paolo

Re: [Qemu-devel] [PATCH V2 1/5] virtio: get_features() can fail

[Cornelia Huck]

On Wed, 15 Jul 2015 13:29:57 +0800
Jason Wang <jasowang@redhat.com> wrote:

> 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/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 */

Don't you need to propagate the error instead of passing it through? Or
am I just confused by error handling? :)

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

[Cornelia Huck]

On Wed, 15 Jul 2015 13:29:56 +0800
Jason Wang <jasowang@redhat.com> 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 by, 1) disable
>   scsi by defautl for 2.4 machine type and fail the initialization
>   when both scsi and 1.0 were set.
> - any layout must be set for transitional device. This is done by set
>   any layout when 1.0 is supported.
> 
> Please review
> 
> 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.

Hm, this seems confusing to me mainly due to the different way
transitional devices are handled by pci and ccw.

For virtio-pci: (please correct me if I misunderstood)
- devices (except input) are transitional by default
- user can disable legacy or modern support
- drivers can use method they prefer, depending on VERSION_1

For virtio-ccw:
- transitional means "in limbo" regarding legacy or modern
- devices become legacy if features are read without negotiation of a
  revision, or if revision 0 is negotiated
- they become modern if revision 1 is negotiated

That implies that for ccw, a transitional device does not offer any
features: It either transitions to the legacy or modern state if a
revision was negotiated, or it becomes a legacy device by reading
features (which are the legacy features, then). While pci has "real"
transitional devices needing to offer a certain feature set.

I'm not sure what the solution is here: basically, ccw needs a dynamic
feature set, while pci does not.

Re: [Qemu-devel] [PATCH V2 1/5] virtio: get_features() can fail

[Jason Wang]

On 07/15/2015 05:01 PM, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 13:29:57 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> 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/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 */
> Don't you need to propagate the error instead of passing it through? Or
> am I just confused by error handling? :)
>

If I understand the code correctly. The caller (virtio_device_realize())
will propagate the error.

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

[Jason Wang]

On 07/15/2015 05:11 PM, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 13:29:56 +0800
> Jason Wang <jasowang@redhat.com> 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 by, 1) disable
>>   scsi by defautl for 2.4 machine type and fail the initialization
>>   when both scsi and 1.0 were set.
>> - any layout must be set for transitional device. This is done by set
>>   any layout when 1.0 is supported.
>>
>> Please review
>>
>> 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.
> Hm, this seems confusing to me mainly due to the different way
> transitional devices are handled by pci and ccw.
>
> For virtio-pci: (please correct me if I misunderstood)
> - devices (except input) are transitional by default
> - user can disable legacy or modern support
> - drivers can use method they prefer, depending on VERSION_1
>
> For virtio-ccw:
> - transitional means "in limbo" regarding legacy or modern
> - devices become legacy if features are read without negotiation of a
>   revision, or if revision 0 is negotiated
> - they become modern if revision 1 is negotiated
>
> That implies that for ccw, a transitional device does not offer any
> features: It either transitions to the legacy or modern state if a
> revision was negotiated, or it becomes a legacy device by reading
> features (which are the legacy features, then). While pci has "real"
> transitional devices needing to offer a certain feature set.
>
> I'm not sure what the solution is here: basically, ccw needs a dynamic
> feature set, while pci does not.
>

So, if I understand correctly. All virtio-ccw devices are transitional
since user can disable neither legacy nor modern. It looks to me then we
can set VIRTIO_F_VERSION_1 unconditionally for ccw. Then there's no
issue left?

Re: [Qemu-devel] [PATCH V2 1/5] virtio: get_features() can fail

[Paolo Bonzini]

On 15/07/2015 11:01, Cornelia Huck wrote:
> > 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 */
>
> Don't you need to propagate the error instead of passing it through? Or
> am I just confused by error handling? :)

Explicit propagation is only needed if you need to look at the error
(because errp could be NULL).  Here you don't need to do that, so
passing it through is fine.

Error management is not hard if you know the rules (and the rules are
not hard, just important).  Someone needs to dig up Markus's latest
explanation and put it in docs/.

Paolo

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

[Cornelia Huck]

On Wed, 15 Jul 2015 17:39:53 +0800
Jason Wang <jasowang@redhat.com> wrote:

> 
> 
> On 07/15/2015 05:11 PM, Cornelia Huck wrote:
> > On Wed, 15 Jul 2015 13:29:56 +0800
> > Jason Wang <jasowang@redhat.com> 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 by, 1) disable
> >>   scsi by defautl for 2.4 machine type and fail the initialization
> >>   when both scsi and 1.0 were set.
> >> - any layout must be set for transitional device. This is done by set
> >>   any layout when 1.0 is supported.
> >>
> >> Please review
> >>
> >> 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.
> > Hm, this seems confusing to me mainly due to the different way
> > transitional devices are handled by pci and ccw.
> >
> > For virtio-pci: (please correct me if I misunderstood)
> > - devices (except input) are transitional by default
> > - user can disable legacy or modern support
> > - drivers can use method they prefer, depending on VERSION_1
> >
> > For virtio-ccw:
> > - transitional means "in limbo" regarding legacy or modern
> > - devices become legacy if features are read without negotiation of a
> >   revision, or if revision 0 is negotiated
> > - they become modern if revision 1 is negotiated
> >
> > That implies that for ccw, a transitional device does not offer any
> > features: It either transitions to the legacy or modern state if a
> > revision was negotiated, or it becomes a legacy device by reading
> > features (which are the legacy features, then). While pci has "real"
> > transitional devices needing to offer a certain feature set.
> >
> > I'm not sure what the solution is here: basically, ccw needs a dynamic
> > feature set, while pci does not.
> >
> 
> So, if I understand correctly. All virtio-ccw devices are transitional
> since user can disable neither legacy nor modern. It looks to me then we
> can set VIRTIO_F_VERSION_1 unconditionally for ccw. Then there's no
> issue left?

I think the base problem is that "transitional" for pci means something
slightly different than "transitional" for ccw - it's more transient
for ccw.

Setting VERSION_1 is not really the problem (as legacy guests will only
retrieve the first 32 bits of features anyway), but any decicions about
other feature bits that are made based upon the presence of VERSION_1.

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

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 11:11:18AM +0200, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 13:29:56 +0800
> Jason Wang <jasowang@redhat.com> 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 by, 1) disable
> >   scsi by defautl for 2.4 machine type and fail the initialization
> >   when both scsi and 1.0 were set.
> > - any layout must be set for transitional device. This is done by set
> >   any layout when 1.0 is supported.
> > 
> > Please review
> > 
> > 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.
> 
> Hm, this seems confusing to me mainly due to the different way
> transitional devices are handled by pci and ccw.
> 
> For virtio-pci: (please correct me if I misunderstood)
> - devices (except input) are transitional by default
> - user can disable legacy or modern support
> - drivers can use method they prefer, depending on VERSION_1
> 
> For virtio-ccw:
> - transitional means "in limbo" regarding legacy or modern
> - devices become legacy if features are read without negotiation of a
>   revision, or if revision 0 is negotiated
> - they become modern if revision 1 is negotiated
> 
> That implies that for ccw, a transitional device does not offer any
> features: It either transitions to the legacy or modern state if a
> revision was negotiated, or it becomes a legacy device by reading
> features (which are the legacy features, then). While pci has "real"
> transitional devices needing to offer a certain feature set.
> 
> I'm not sure what the solution is here: basically, ccw needs a dynamic
> feature set, while pci does not.

I think the issue is theoretical since linux guests
do not read feature bits > 31 unless revision 1
is negotiated.

In any case, won't the following solve it nicely?

if (revision >= 1)
		return d->host_features
else
		return d->host_features & 0xffffffff;


-- 
MST

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

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 01:38:42PM +0200, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 17:39:53 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
> > 
> > 
> > On 07/15/2015 05:11 PM, Cornelia Huck wrote:
> > > On Wed, 15 Jul 2015 13:29:56 +0800
> > > Jason Wang <jasowang@redhat.com> 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 by, 1) disable
> > >>   scsi by defautl for 2.4 machine type and fail the initialization
> > >>   when both scsi and 1.0 were set.
> > >> - any layout must be set for transitional device. This is done by set
> > >>   any layout when 1.0 is supported.
> > >>
> > >> Please review
> > >>
> > >> 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.
> > > Hm, this seems confusing to me mainly due to the different way
> > > transitional devices are handled by pci and ccw.
> > >
> > > For virtio-pci: (please correct me if I misunderstood)
> > > - devices (except input) are transitional by default
> > > - user can disable legacy or modern support
> > > - drivers can use method they prefer, depending on VERSION_1
> > >
> > > For virtio-ccw:
> > > - transitional means "in limbo" regarding legacy or modern
> > > - devices become legacy if features are read without negotiation of a
> > >   revision, or if revision 0 is negotiated
> > > - they become modern if revision 1 is negotiated
> > >
> > > That implies that for ccw, a transitional device does not offer any
> > > features: It either transitions to the legacy or modern state if a
> > > revision was negotiated, or it becomes a legacy device by reading
> > > features (which are the legacy features, then). While pci has "real"
> > > transitional devices needing to offer a certain feature set.
> > >
> > > I'm not sure what the solution is here: basically, ccw needs a dynamic
> > > feature set, while pci does not.
> > >
> > 
> > So, if I understand correctly. All virtio-ccw devices are transitional
> > since user can disable neither legacy nor modern. It looks to me then we
> > can set VIRTIO_F_VERSION_1 unconditionally for ccw. Then there's no
> > issue left?
> 
> I think the base problem is that "transitional" for pci means something
> slightly different than "transitional" for ccw - it's more transient
> for ccw.
> 
> Setting VERSION_1 is not really the problem (as legacy guests will only
> retrieve the first 32 bits of features anyway), but any decicions about
> other feature bits that are made based upon the presence of VERSION_1.

Are we talking about the scsi bit here, again?
Or are there other examples?

-- 
MST

Re: [Qemu-devel] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 01:29:59PM +0800, Jason Wang wrote:
> Disable scsi passthrough by default since it was incompatible with
> virtio 1.0. For legacy machine types, keep this on by default.
> 
> 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>

Seems risky for 2.4.  modern is off by default for now. Can't we limit
the change to when modern is enabled?

I suggested changing this from bool to on/off/auto, and
make auto mean !modern.


> ---
>  hw/block/virtio-blk.c | 2 +-
>  include/hw/compat.h   | 6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 761d763..362fe53 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -964,7 +964,7 @@ static Property virtio_blk_properties[] = {
>      DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial),
>      DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true),
>  #ifdef __linux__
> -    DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, true),
> +    DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, false),
>  #endif
>      DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
>                      true),
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 4a43466..56039d8 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -2,7 +2,11 @@
>  #define HW_COMPAT_H
>  
>  #define HW_COMPAT_2_3 \
> -        /* empty */
> +        {\
> +            .driver   = "virtio-blk-pci",\
> +            .property = "scsi",\
> +            .value    = "on",\
> +        },
>  
>  #define HW_COMPAT_2_2 \
>          /* empty */
> -- 
> 2.1.4

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

[Cornelia Huck]

On Wed, 15 Jul 2015 14:52:11 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 15, 2015 at 01:38:42PM +0200, Cornelia Huck wrote:
> > On Wed, 15 Jul 2015 17:39:53 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> > 
> > > 
> > > 
> > > On 07/15/2015 05:11 PM, Cornelia Huck wrote:
> > > > On Wed, 15 Jul 2015 13:29:56 +0800
> > > > Jason Wang <jasowang@redhat.com> 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 by, 1) disable
> > > >>   scsi by defautl for 2.4 machine type and fail the initialization
> > > >>   when both scsi and 1.0 were set.
> > > >> - any layout must be set for transitional device. This is done by set
> > > >>   any layout when 1.0 is supported.
> > > >>
> > > >> Please review
> > > >>
> > > >> 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.
> > > > Hm, this seems confusing to me mainly due to the different way
> > > > transitional devices are handled by pci and ccw.
> > > >
> > > > For virtio-pci: (please correct me if I misunderstood)
> > > > - devices (except input) are transitional by default
> > > > - user can disable legacy or modern support
> > > > - drivers can use method they prefer, depending on VERSION_1
> > > >
> > > > For virtio-ccw:
> > > > - transitional means "in limbo" regarding legacy or modern
> > > > - devices become legacy if features are read without negotiation of a
> > > >   revision, or if revision 0 is negotiated
> > > > - they become modern if revision 1 is negotiated
> > > >
> > > > That implies that for ccw, a transitional device does not offer any
> > > > features: It either transitions to the legacy or modern state if a
> > > > revision was negotiated, or it becomes a legacy device by reading
> > > > features (which are the legacy features, then). While pci has "real"
> > > > transitional devices needing to offer a certain feature set.
> > > >
> > > > I'm not sure what the solution is here: basically, ccw needs a dynamic
> > > > feature set, while pci does not.
> > > >
> > > 
> > > So, if I understand correctly. All virtio-ccw devices are transitional
> > > since user can disable neither legacy nor modern. It looks to me then we
> > > can set VIRTIO_F_VERSION_1 unconditionally for ccw. Then there's no
> > > issue left?
> > 
> > I think the base problem is that "transitional" for pci means something
> > slightly different than "transitional" for ccw - it's more transient
> > for ccw.
> > 
> > Setting VERSION_1 is not really the problem (as legacy guests will only
> > retrieve the first 32 bits of features anyway), but any decicions about
> > other feature bits that are made based upon the presence of VERSION_1.
> 
> Are we talking about the scsi bit here, again?
> Or are there other examples?
> 

The scsi bit is the one I thought about here; I'd need to check the spec
whether there are others (for other device types).

Re: [Qemu-devel] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Paolo Bonzini]

On 15/07/2015 14:21, Michael S. Tsirkin wrote:
>> > Disable scsi passthrough by default since it was incompatible with
>> > virtio 1.0. For legacy machine types, keep this on by default.
>> > 
>> > 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>
> Seems risky for 2.4.  modern is off by default for now. Can't we limit
> the change to when modern is enabled?

That would have the effect of disabling a feature when you turn on modern.

> I suggested changing this from bool to on/off/auto, and
> make auto mean !modern.

No, please do it like Jason did.  The SCSI feature effectively had to be
enabled explicitly already, the requests were marked as unsupported.

Paolo

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

[Cornelia Huck]

On Wed, 15 Jul 2015 14:49:43 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 15, 2015 at 11:11:18AM +0200, Cornelia Huck wrote:
> > On Wed, 15 Jul 2015 13:29:56 +0800
> > Jason Wang <jasowang@redhat.com> 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 by, 1) disable
> > >   scsi by defautl for 2.4 machine type and fail the initialization
> > >   when both scsi and 1.0 were set.
> > > - any layout must be set for transitional device. This is done by set
> > >   any layout when 1.0 is supported.
> > > 
> > > Please review
> > > 
> > > 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.
> > 
> > Hm, this seems confusing to me mainly due to the different way
> > transitional devices are handled by pci and ccw.
> > 
> > For virtio-pci: (please correct me if I misunderstood)
> > - devices (except input) are transitional by default
> > - user can disable legacy or modern support
> > - drivers can use method they prefer, depending on VERSION_1
> > 
> > For virtio-ccw:
> > - transitional means "in limbo" regarding legacy or modern
> > - devices become legacy if features are read without negotiation of a
> >   revision, or if revision 0 is negotiated
> > - they become modern if revision 1 is negotiated
> > 
> > That implies that for ccw, a transitional device does not offer any
> > features: It either transitions to the legacy or modern state if a
> > revision was negotiated, or it becomes a legacy device by reading
> > features (which are the legacy features, then). While pci has "real"
> > transitional devices needing to offer a certain feature set.
> > 
> > I'm not sure what the solution is here: basically, ccw needs a dynamic
> > feature set, while pci does not.
> 
> I think the issue is theoretical since linux guests
> do not read feature bits > 31 unless revision 1
> is negotiated.

Again, VERSION_1 is not really the problem. (qemu clears it for
revisions < 1, and I plan to send a patch that effectively clears all
bits > 31.) I'm caring about the legacy bits (scsi, in that case) that
are not send to legacy drivers.

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

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 02:46:19PM +0200, Cornelia Huck wrote:
> The scsi bit is the one I thought about here; I'd need to check the spec
> whether there are others (for other device types).

So it's mostly the same pain for pci as well, I don't
think ccw is special.

-- 
MST

Re: [Qemu-devel] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote:
> 
> 
> On 15/07/2015 14:21, Michael S. Tsirkin wrote:
> >> > Disable scsi passthrough by default since it was incompatible with
> >> > virtio 1.0. For legacy machine types, keep this on by default.
> >> > 
> >> > 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>
> > Seems risky for 2.4.  modern is off by default for now. Can't we limit
> > the change to when modern is enabled?
> 
> That would have the effect of disabling a feature when you turn on modern.

What's wrong with that?

> > I suggested changing this from bool to on/off/auto, and
> > make auto mean !modern.
> 
> No, please do it like Jason did.  The SCSI feature effectively had to be
> enabled explicitly already, the requests were marked as unsupported.
> 
> Paolo

I didn't know. How is it enabled?

-- 
MST

Re: [Qemu-devel] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Paolo Bonzini]

On 15/07/2015 16:14, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 15/07/2015 14:21, Michael S. Tsirkin wrote:
>>>>> Disable scsi passthrough by default since it was incompatible with
>>>>> virtio 1.0. For legacy machine types, keep this on by default.
>>>>>
>>>>> 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>
>>> Seems risky for 2.4.  modern is off by default for now. Can't we limit
>>> the change to when modern is enabled?
>>
>> That would have the effect of disabling a feature when you turn on modern.
> 
> What's wrong with that?

Weren't you complaining about it a few hours ago? :)

>>> I suggested changing this from bool to on/off/auto, and
>>> make auto mean !modern.
>>
>> No, please do it like Jason did.  The SCSI feature effectively had to be
>> enabled explicitly already, the requests were marked as unsupported.
> 
> I didn't know. How is it enabled?

It's enabled by default in QEMU, but disabled by default in libvirt.
And it only works if you pass a whole _disk_ (not a partition or logical
volume) to QEMU, which is definitely not the common case.

It can just be documented in the release notes; the feature is still
available, and libvirt won't be broken because it adds explicitly both
scsi=on and scsi=off.

Paolo

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

[Cornelia Huck]

On Wed, 15 Jul 2015 17:08:02 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 15, 2015 at 02:46:19PM +0200, Cornelia Huck wrote:
> > The scsi bit is the one I thought about here; I'd need to check the spec
> > whether there are others (for other device types).
> 
> So it's mostly the same pain for pci as well, I don't
> think ccw is special.

I don't think there's any feature bit (other than BAD_FEATURE) that is
transport-dependant.

Re: [Qemu-devel] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 04:18:49PM +0200, Paolo Bonzini wrote:
> 
> 
> On 15/07/2015 16:14, Michael S. Tsirkin wrote:
> > On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 15/07/2015 14:21, Michael S. Tsirkin wrote:
> >>>>> Disable scsi passthrough by default since it was incompatible with
> >>>>> virtio 1.0. For legacy machine types, keep this on by default.
> >>>>>
> >>>>> 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>
> >>> Seems risky for 2.4.  modern is off by default for now. Can't we limit
> >>> the change to when modern is enabled?
> >>
> >> That would have the effect of disabling a feature when you turn on modern.
> > 
> > What's wrong with that?
> 
> Weren't you complaining about it a few hours ago? :)

No, I complained about guest driver update disabling it.

> >>> I suggested changing this from bool to on/off/auto, and
> >>> make auto mean !modern.
> >>
> >> No, please do it like Jason did.  The SCSI feature effectively had to be
> >> enabled explicitly already, the requests were marked as unsupported.
> > 
> > I didn't know. How is it enabled?
> 
> It's enabled by default in QEMU, but disabled by default in libvirt.
> And it only works if you pass a whole _disk_ (not a partition or logical
> volume) to QEMU, which is definitely not the common case.
> 
> It can just be documented in the release notes; the feature is still
> available, and libvirt won't be broken because it adds explicitly both
> scsi=on and scsi=off.
> 
> Paolo

So for libvirt, we don't really care about the default, right?
For command line, would it not be friendlier to make it follow the
modern flag automatically?

Re: [Qemu-devel] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Paolo Bonzini]

On 15/07/2015 16:28, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2015 at 04:18:49PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 15/07/2015 16:14, Michael S. Tsirkin wrote:
>>> On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 15/07/2015 14:21, Michael S. Tsirkin wrote:
>>>>>>> Disable scsi passthrough by default since it was incompatible with
>>>>>>> virtio 1.0. For legacy machine types, keep this on by default.
>>>>>>>
>>>>>>> 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>
>>>>> Seems risky for 2.4.  modern is off by default for now. Can't we limit
>>>>> the change to when modern is enabled?
>>>>
>>>> That would have the effect of disabling a feature when you turn on modern.
>>>
>>> What's wrong with that?
>>
>> Weren't you complaining about it a few hours ago? :)
> 
> No, I complained about guest driver update disabling it.

Ah, sorry for the confusion.

>>>>> I suggested changing this from bool to on/off/auto, and
>>>>> make auto mean !modern.
>>>>
>>>> No, please do it like Jason did.  The SCSI feature effectively had to be
>>>> enabled explicitly already, the requests were marked as unsupported.
>>>
>>> I didn't know. How is it enabled?
>>
>> It's enabled by default in QEMU, but disabled by default in libvirt.
>> And it only works if you pass a whole _disk_ (not a partition or logical
>> volume) to QEMU, which is definitely not the common case.
>>
>> It can just be documented in the release notes; the feature is still
>> available, and libvirt won't be broken because it adds explicitly both
>> scsi=on and scsi=off.
> 
> So for libvirt, we don't really care about the default, right?
> For command line, would it not be friendlier to make it follow the
> modern flag automatically?

I wouldn't mind if we grabbed the occasion to disable it altogether.
However, that would indeed work as well.

Paolo

Re: [Qemu-devel] [PATCH V2 1/5] virtio: get_features() can fail

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 15/07/2015 11:01, Cornelia Huck wrote:
>> > 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 */
>>
>> Don't you need to propagate the error instead of passing it through? Or
>> am I just confused by error handling? :)
>
> Explicit propagation is only needed if you need to look at the error
> (because errp could be NULL).  Here you don't need to do that, so
> passing it through is fine.
>
> Error management is not hard if you know the rules (and the rules are
> not hard, just important).  Someone needs to dig up Markus's latest
> explanation and put it in docs/.

I got a patch pending that puts it in include/qapi/error.h:

    [PATCH 6/7] error: Revamp interface documentation

part of

    error: On abort, report where the error was created

Intend to respin soonish to address Eric's and Laszlo's review.

Re: [Qemu-devel] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Cornelia Huck]

<dragging up an old thread again>

On Wed, 15 Jul 2015 16:45:52 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 15/07/2015 16:28, Michael S. Tsirkin wrote:
> > On Wed, Jul 15, 2015 at 04:18:49PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 15/07/2015 16:14, Michael S. Tsirkin wrote:
> >>> On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote:
> >>>>
> >>>>
> >>>> On 15/07/2015 14:21, Michael S. Tsirkin wrote:
> >>>>>>> Disable scsi passthrough by default since it was incompatible with
> >>>>>>> virtio 1.0. For legacy machine types, keep this on by default.
> >>>>>>>
> >>>>>>> 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>
> >>>>> Seems risky for 2.4.  modern is off by default for now. Can't we limit
> >>>>> the change to when modern is enabled?
> >>>>
> >>>> That would have the effect of disabling a feature when you turn on modern.
> >>>
> >>> What's wrong with that?
> >>
> >> Weren't you complaining about it a few hours ago? :)
> > 
> > No, I complained about guest driver update disabling it.
> 
> Ah, sorry for the confusion.
> 
> >>>>> I suggested changing this from bool to on/off/auto, and
> >>>>> make auto mean !modern.
> >>>>
> >>>> No, please do it like Jason did.  The SCSI feature effectively had to be
> >>>> enabled explicitly already, the requests were marked as unsupported.
> >>>
> >>> I didn't know. How is it enabled?
> >>
> >> It's enabled by default in QEMU, but disabled by default in libvirt.
> >> And it only works if you pass a whole _disk_ (not a partition or logical
> >> volume) to QEMU, which is definitely not the common case.
> >>
> >> It can just be documented in the release notes; the feature is still
> >> available, and libvirt won't be broken because it adds explicitly both
> >> scsi=on and scsi=off.
> > 
> > So for libvirt, we don't really care about the default, right?
> > For command line, would it not be friendlier to make it follow the
> > modern flag automatically?
> 
> I wouldn't mind if we grabbed the occasion to disable it altogether.
> However, that would indeed work as well.

Do we want to change anything for 2.5 about the default?

Currently, we still default scsi to true, and you have to disable it
explicitly if you want to use virtio-1 compliant virtio-blk devices
(which is a bit annoying, as scsi passthrough is not something people
usually want).

Re: [Qemu-devel] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Paolo Bonzini]

On 14/10/2015 12:29, Cornelia Huck wrote:
> Do we want to change anything for 2.5 about the default?
> 
> Currently, we still default scsi to true, and you have to disable it
> explicitly if you want to use virtio-1 compliant virtio-blk devices
> (which is a bit annoying, as scsi passthrough is not something people
> usually want).

I think disabling it is best.  Can you send a patch?

Paolo