[Qemu-devel] [PATCH 0/3] decouple VIRTIO_BLK_F_SCSI from SG_IO support

[Qemu-devel] [PATCH 0/3] decouple VIRTIO_BLK_F_SCSI from SG_IO support

[Paolo Bonzini]

VIRTIO_BLK_F_SCSI is supposed to mean whether the host can *parse*
SCSI requests, not *execute* them.  So it should always be enabled,
and the scsi=on/off property tied to a separate configuration variable
that is not guest visible.

With this change, Linux has problems understanding failed requests, so
patch 1 works around the Linux bugs.

Important: because we need to do this to fix a migration compatibility
problem when QEMU might be invoked with an old machine type, we must do
this unconditionally.  This more or less assumes that no one ever invoked
QEMU with scsi=off, as it breaks migration from new QEMU, scsi=off to
old QEMU, also scsi=off.  However new->old is not supported upstream.
If any downstream cares about new->old migration, they can apply v1 of
the patches instead of this.

Paolo Bonzini (3):
  virtio-blk: report non-zero status when failing SG_IO requests
  virtio-blk: define VirtIOBlkConf
  virtio-blk: always enable VIRTIO_BLK_F_SCSI

 hw/s390-virtio-bus.c |   10 ++++--
 hw/s390-virtio-bus.h |    4 +-
 hw/virtio-blk.c      |   77 ++++++++++++++++++++++++-------------------------
 hw/virtio-blk.h      |   14 +++++----
 hw/virtio-pci.c      |   11 ++++---
 hw/virtio-pci.h      |    4 +-
 hw/virtio.h          |    4 +-
 7 files changed, 64 insertions(+), 60 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/3] virtio-blk: report non-zero status when failing SG_IO requests

[Paolo Bonzini]

Linux really looks only at scsi->errors.  Arguably it is their bug,
but we can make it safe for older guests now.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-blk.c |   48 +++++++++++++++++++++++-------------------------
 1 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 49990f8..b7e510d 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -145,20 +145,12 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
     return req;
 }
 
-#ifdef __linux__
 static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 {
-    struct sg_io_hdr hdr;
-    int ret;
+    int ret = -1;
     int status;
     int i;
 
-    if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) {
-        virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
-        g_free(req);
-        return;
-    }
-
     /*
      * We require at least one output segment each for the virtio_blk_outhdr
      * and the SCSI command block.
@@ -173,20 +165,26 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
     }
 
     /*
-     * No support for bidirection commands yet.
+     * The scsi inhdr is placed in the second-to-last input segment, just
+     * before the regular inhdr.
      */
-    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
-        virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
-        g_free(req);
-        return;
+    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
+
+    if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) {
+        status = VIRTIO_BLK_S_UNSUPP;
+        goto fail;
     }
 
     /*
-     * The scsi inhdr is placed in the second-to-last input segment, just
-     * before the regular inhdr.
+     * No support for bidirection commands yet.
      */
-    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
+    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
+        status = VIRTIO_BLK_S_UNSUPP;
+        goto fail;
+    }
 
+#ifdef __linux__
+    struct sg_io_hdr hdr;
     memset(&hdr, 0, sizeof(struct sg_io_hdr));
     hdr.interface_id = 'S';
     hdr.cmd_len = req->elem.out_sg[1].iov_len;
@@ -229,9 +227,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 
     ret = bdrv_ioctl(req->dev->bs, SG_IO, &hdr);
     if (ret) {
-        status = VIRTIO_BLK_S_UNSUPP;
-        hdr.status = ret;
-        hdr.resid = hdr.dxfer_len;
+        goto fail;
     } else if (hdr.status) {
         status = VIRTIO_BLK_S_IOERR;
     } else {
@@ -258,14 +254,16 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 
     virtio_blk_req_complete(req, status);
     g_free(req);
-}
 #else
-static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
-{
-    virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
+    abort();
+#endif
+
+fail:
+    /* Just put anything nonzero so that the ioctl fails in the guest.  */
+    stl_p(&req->scsi->errors, 255);
+    virtio_blk_req_complete(req, status);
     g_free(req);
 }
-#endif /* __linux__ */
 
 typedef struct MultiReqBuffer {
     BlockRequest        blkreq[32];
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 1/3] virtio-blk: report non-zero status when failing SG_IO requests

[Orit Wasserman]

On 03/07/2012 08:01 PM, Paolo Bonzini wrote:
> Linux really looks only at scsi->errors.  Arguably it is their bug,
> but we can make it safe for older guests now.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio-blk.c |   48 +++++++++++++++++++++++-------------------------
>  1 files changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 49990f8..b7e510d 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -145,20 +145,12 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
>      return req;
>  }
>  
> -#ifdef __linux__
>  static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>  {
> -    struct sg_io_hdr hdr;
> -    int ret;
> +    int ret = -1;
>      int status;
>      int i;
>  
> -    if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) {
> -        virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> -        g_free(req);
> -        return;
> -    }
> -
>      /*
>       * We require at least one output segment each for the virtio_blk_outhdr
>       * and the SCSI command block.
> @@ -173,20 +165,26 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>      }
>  
>      /*
> -     * No support for bidirection commands yet.
> +     * The scsi inhdr is placed in the second-to-last input segment, just
> +     * before the regular inhdr.
>       */
> -    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
> -        virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> -        g_free(req);
> -        return;
> +    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
> +
> +    if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) {
> +        status = VIRTIO_BLK_S_UNSUPP;
> +        goto fail;
>      }
>  
>      /*
> -     * The scsi inhdr is placed in the second-to-last input segment, just
> -     * before the regular inhdr.
> +     * No support for bidirection commands yet.
>       */
> -    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
> +    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
> +        status = VIRTIO_BLK_S_UNSUPP;
> +        goto fail;
> +    }
>  
> +#ifdef __linux__
> +    struct sg_io_hdr hdr;
>      memset(&hdr, 0, sizeof(struct sg_io_hdr));
>      hdr.interface_id = 'S';
>      hdr.cmd_len = req->elem.out_sg[1].iov_len;
> @@ -229,9 +227,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>  
>      ret = bdrv_ioctl(req->dev->bs, SG_IO, &hdr);
>      if (ret) {
> -        status = VIRTIO_BLK_S_UNSUPP;
> -        hdr.status = ret;
> -        hdr.resid = hdr.dxfer_len;
> +        goto fail;
>      } else if (hdr.status) {
>          status = VIRTIO_BLK_S_IOERR;
>      } else {
> @@ -258,14 +254,16 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>  
>      virtio_blk_req_complete(req, status);
>      g_free(req);
> -}
>  #else
> -static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
> -{
> -    virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> +    abort();
> +#endif
> +
> +fail:
> +    /* Just put anything nonzero so that the ioctl fails in the guest.  */
> +    stl_p(&req->scsi->errors, 255);
> +    virtio_blk_req_complete(req, status);

I get to following compile error:
In function ‘virtio_blk_handle_request’:
virtio-blk.c:264:28: error: ‘status’ may be used uninitialized in this function [-Werror=uninitialized]
virtio-blk.c:151:9: note: ‘status’ was declared here
cc1: all warnings being treated as errors

Are you using  -disable-werror ?

Orit
>      g_free(req);
>  }
> -#endif /* __linux__ */
>  
>  typedef struct MultiReqBuffer {
>      BlockRequest        blkreq[32];

Re: [Qemu-devel] [PATCH 1/3] virtio-blk: report non-zero status when failing SG_IO requests

[Paolo Bonzini]

Il 13/03/2012 14:36, Orit Wasserman ha scritto:
> I get to following compile error:
> In function ‘virtio_blk_handle_request’:
> virtio-blk.c:264:28: error: ‘status’ may be used uninitialized in this function [-Werror=uninitialized]
> virtio-blk.c:151:9: note: ‘status’ was declared here
> cc1: all warnings being treated as errors
> 
> Are you using  -disable-werror ?

No, perhaps a different compiler though.

Paolo

Re: [Qemu-devel] [PATCH 1/3] virtio-blk: report non-zero status when failing SG_IO requests

[Orit Wasserman]

On 03/13/2012 03:39 PM, Paolo Bonzini wrote:
> Il 13/03/2012 14:36, Orit Wasserman ha scritto:
>> I get to following compile error:
>> In function ‘virtio_blk_handle_request’:
>> virtio-blk.c:264:28: error: ‘status’ may be used uninitialized in this function [-Werror=uninitialized]
>> virtio-blk.c:151:9: note: ‘status’ was declared here
>> cc1: all warnings being treated as errors
>>
>> Are you using  -disable-werror ?
> 
> No, perhaps a different compiler though.

could be I'm using  (GCC) 4.6.1 20110908 (Red Hat 4.6.1-9)

> 
> Paolo

Re: [Qemu-devel] [PATCH 1/3] virtio-blk: report non-zero status when failing SG_IO requests

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On 03/13/2012 07:39 AM, Paolo Bonzini wrote:
> Il 13/03/2012 14:36, Orit Wasserman ha scritto:
>> I get to following compile error:
>> In function ‘virtio_blk_handle_request’:
>> virtio-blk.c:264:28: error: ‘status’ may be used uninitialized in this function [-Werror=uninitialized]
>> virtio-blk.c:151:9: note: ‘status’ was declared here
>> cc1: all warnings being treated as errors
>>
>> Are you using  -disable-werror ?
> 
> No, perhaps a different compiler though.

Or a difference in -O level.  gcc is notoriously bad at missing
-Wuninitialized at -O0, and not warning until -O1 or -O2.  [I'm still
impressed at how the Java language was able to mandate uninitialized
detection into the compiler as a required part of the language.]

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[Qemu-devel] [PATCH 2/3] virtio-blk: define VirtIOBlkConf

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/s390-virtio-bus.c |    7 +++----
 hw/s390-virtio-bus.h |    4 ++--
 hw/virtio-blk.c      |   28 ++++++++++++++--------------
 hw/virtio-blk.h      |    7 +++++++
 hw/virtio-pci.c      |    8 +++-----
 hw/virtio-pci.h      |    4 ++--
 hw/virtio.h          |    4 ++--
 7 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index c450e4b..300454b 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -139,8 +139,7 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev)
 {
     VirtIODevice *vdev;
 
-    vdev = virtio_blk_init((DeviceState *)dev, &dev->block,
-                           &dev->block_serial);
+    vdev = virtio_blk_init((DeviceState *)dev, &dev->blk);
     if (!vdev) {
         return -1;
     }
@@ -376,8 +375,8 @@ static TypeInfo s390_virtio_net = {
 };
 
 static Property s390_virtio_blk_properties[] = {
-    DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block),
-    DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial),
+    DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, blk.conf),
+    DEFINE_PROP_STRING("serial", VirtIOS390Device, blk.serial),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
index 0e60bc0..ae07045 100644
--- a/hw/s390-virtio-bus.h
+++ b/hw/s390-virtio-bus.h
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "virtio-blk.h"
 #include "virtio-net.h"
 #include "virtio-serial.h"
 #include "virtio-scsi.h"
@@ -62,8 +63,7 @@ struct VirtIOS390Device {
     ram_addr_t feat_offs;
     uint8_t feat_len;
     VirtIODevice *vdev;
-    BlockConf block;
-    char *block_serial;
+    VirtIOBlkConf blk;
     NICConf nic;
     uint32_t host_features;
     virtio_serial_conf serial;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index b7e510d..10d84d5 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -29,7 +29,7 @@ typedef struct VirtIOBlock
     void *rq;
     QEMUBH *bh;
     BlockConf *conf;
-    char *serial;
+    VirtIOBlkConf *blk;
     unsigned short sector_mask;
     DeviceState *qdev;
 } VirtIOBlock;
@@ -392,7 +392,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
          * terminated by '\0' only when shorter than buffer.
          */
         strncpy(req->elem.in_sg[0].iov_base,
-                s->serial ? s->serial : "",
+                s->blk->serial ? s->blk->serial : "",
                 MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
         virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
         g_free(req);
@@ -566,28 +566,27 @@ static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
 };
 
-VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
-                              char **serial)
+VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
 {
     VirtIOBlock *s;
     int cylinders, heads, secs;
     static int virtio_blk_id;
     DriveInfo *dinfo;
 
-    if (!conf->bs) {
+    if (!blk->conf.bs) {
         error_report("drive property not set");
         return NULL;
     }
-    if (!bdrv_is_inserted(conf->bs)) {
+    if (!bdrv_is_inserted(blk->conf.bs)) {
         error_report("Device needs media, but drive is empty");
         return NULL;
     }
 
-    if (!*serial) {
+    if (!blk->serial) {
         /* try to fall back to value set with legacy -drive serial=... */
-        dinfo = drive_get_by_blockdev(conf->bs);
+        dinfo = drive_get_by_blockdev(blk->conf.bs);
         if (*dinfo->serial) {
-            *serial = strdup(dinfo->serial);
+            blk->serial = strdup(dinfo->serial);
         }
     }
 
@@ -598,9 +597,9 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.get_features = virtio_blk_get_features;
     s->vdev.reset = virtio_blk_reset;
-    s->bs = conf->bs;
-    s->conf = conf;
-    s->serial = *serial;
+    s->bs = blk->conf.bs;
+    s->conf = &blk->conf;
+    s->blk = blk;
     s->rq = NULL;
     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
@@ -612,10 +611,10 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
     register_savevm(dev, "virtio-blk", virtio_blk_id++, 2,
                     virtio_blk_save, virtio_blk_load, s);
     bdrv_set_dev_ops(s->bs, &virtio_block_ops, s);
-    bdrv_set_buffer_alignment(s->bs, conf->logical_block_size);
+    bdrv_set_buffer_alignment(s->bs, s->conf->logical_block_size);
 
     bdrv_iostatus_enable(s->bs);
-    add_boot_device_path(conf->bootindex, dev, "/disk@0,0");
+    add_boot_device_path(s->conf->bootindex, dev, "/disk@0,0");
 
     return &s->vdev;
 }
@@ -624,5 +623,6 @@ void virtio_blk_exit(VirtIODevice *vdev)
 {
     VirtIOBlock *s = to_virtio_blk(vdev);
     unregister_savevm(s->qdev, "virtio-blk", s);
+    blockdev_mark_auto_del(s->bs);
     virtio_cleanup(vdev);
 }
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 244dce4..70564a1 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -97,6 +97,12 @@ struct virtio_scsi_inhdr
     uint32_t residual;
 };
 
+struct VirtIOBlkConf
+{
+    BlockConf conf;
+    char *serial;
+};
+
 #ifdef __linux__
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
@@ -105,4 +111,5 @@ struct virtio_scsi_inhdr
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
 #endif
+
 #endif
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a0fb7c1..956926b 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -697,8 +697,7 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev)
         proxy->class_code != PCI_CLASS_STORAGE_OTHER)
         proxy->class_code = PCI_CLASS_STORAGE_SCSI;
 
-    vdev = virtio_blk_init(&pci_dev->qdev, &proxy->block,
-                           &proxy->block_serial);
+    vdev = virtio_blk_init(&pci_dev->qdev, &proxy->blk);
     if (!vdev) {
         return -1;
     }
@@ -726,7 +725,6 @@ static int virtio_blk_exit_pci(PCIDevice *pci_dev)
 
     virtio_pci_stop_ioeventfd(proxy);
     virtio_blk_exit(proxy->vdev);
-    blockdev_mark_auto_del(proxy->block.bs);
     return virtio_exit_pci(pci_dev);
 }
 
@@ -809,8 +807,8 @@ static int virtio_balloon_exit_pci(PCIDevice *pci_dev)
 
 static Property virtio_blk_properties[] = {
     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
-    DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, block),
-    DEFINE_PROP_STRING("serial", VirtIOPCIProxy, block_serial),
+    DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
+    DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index e560428..889e59e 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -15,6 +15,7 @@
 #ifndef QEMU_VIRTIO_PCI_H
 #define QEMU_VIRTIO_PCI_H
 
+#include "virtio-blk.h"
 #include "virtio-net.h"
 #include "virtio-serial.h"
 #include "virtio-scsi.h"
@@ -32,8 +33,7 @@ typedef struct {
     uint32_t flags;
     uint32_t class_code;
     uint32_t nvectors;
-    BlockConf block;
-    char *block_serial;
+    VirtIOBlkConf blk;
     NICConf nic;
     uint32_t host_features;
 #ifdef CONFIG_LINUX
diff --git a/hw/virtio.h b/hw/virtio.h
index 400c092..75686d2 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -191,8 +191,8 @@ void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
                         void *opaque);
 
 /* Base devices.  */
-VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
-                              char **serial);
+typedef struct VirtIOBlkConf VirtIOBlkConf;
+VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk);
 struct virtio_net_conf;
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
                               struct virtio_net_conf *net);
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/3] virtio-blk: always enable VIRTIO_BLK_F_SCSI

[Paolo Bonzini]

VIRTIO_BLK_F_SCSI is supposed to mean whether the host can *parse* SCSI
requests, not *execute* them.  We need to change this to fix a migration
compatibility problem related to the version of *management* (hence
unrelated to the QEMU machine type), so we must do this unconditionally
even on older machine types.

This more or less assumes that no one ever invoked QEMU with scsi=off.
Here is how migration works/fails with various combinations:

- old QEMU, scsi=on -> new QEMU, scsi=on
- new QEMU, scsi=on -> old QEMU, scsi=on
- old QEMU, scsi=off -> new QEMU, scsi=on
- new QEMU, scsi=off -> old QEMU, scsi=on
        ok (new QEMU has VIRTIO_BLK_F_SCSI, adding host features is fine)

- old QEMU, scsi=off -> new QEMU, scsi=off
        ok (new QEMU has VIRTIO_BLK_F_SCSI, adding host features is fine)

- old QEMU, scsi=on -> new QEMU, scsi=off
        ok, bug fixed

- new QEMU, scsi=on -> old QEMU, scsi=off
        doesn't work (same as: old QEMU, scsi=on -> old QEMU, scsi=off)

- new QEMU, scsi=off -> old QEMU, scsi=off
        broken by the patch (new->old not supported upstream)

If any downstream cares about new->old migration, they can apply v1 of
the patches instead of this (since the problem is in the destination,
applying both series would not help), and carry those until they can
break downward-compatibility.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/s390-virtio-bus.c |    3 +++
 hw/virtio-blk.c      |    3 ++-
 hw/virtio-blk.h      |    7 +------
 hw/virtio-pci.c      |    3 +++
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index 300454b..a7f0a95 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -377,6 +377,9 @@ static TypeInfo s390_virtio_net = {
 static Property s390_virtio_blk_properties[] = {
     DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, blk.conf),
     DEFINE_PROP_STRING("serial", VirtIOS390Device, blk.serial),
+#ifdef __linux__
+    DEFINE_PROP_BIT("scsi", VirtIOS390Device, blk.scsi, 0, true),
+#endif
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 10d84d5..4735ded 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -170,7 +170,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
      */
     req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
 
-    if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) {
+    if (!req->dev->blk->scsi) {
         status = VIRTIO_BLK_S_UNSUPP;
         goto fail;
     }
@@ -507,6 +507,7 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
     features |= (1 << VIRTIO_BLK_F_GEOMETRY);
     features |= (1 << VIRTIO_BLK_F_TOPOLOGY);
     features |= (1 << VIRTIO_BLK_F_BLK_SIZE);
+    features |= (1 << VIRTIO_BLK_F_SCSI);
 
     if (bdrv_enable_write_cache(s->bs))
         features |= (1 << VIRTIO_BLK_F_WCACHE);
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 70564a1..d785001 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -101,15 +101,10 @@ struct VirtIOBlkConf
 {
     BlockConf conf;
     char *serial;
+    uint32_t scsi;
 };
 
-#ifdef __linux__
-#define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
-        DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
-        DEFINE_PROP_BIT("scsi", _state, _field, VIRTIO_BLK_F_SCSI, true)
-#else
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
-#endif
 
 #endif
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 956926b..6169d99 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -809,6 +809,9 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
     DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
     DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
+#ifdef __linux__
+    DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
+#endif
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 0/3] decouple VIRTIO_BLK_F_SCSI from SG_IO support

[Paolo Bonzini]

Il 07/03/2012 19:01, Paolo Bonzini ha scritto:
> VIRTIO_BLK_F_SCSI is supposed to mean whether the host can *parse*
> SCSI requests, not *execute* them.  So it should always be enabled,
> and the scsi=on/off property tied to a separate configuration variable
> that is not guest visible.
> 
> With this change, Linux has problems understanding failed requests, so
> patch 1 works around the Linux bugs.
> 
> Important: because we need to do this to fix a migration compatibility
> problem when QEMU might be invoked with an old machine type, we must do
> this unconditionally.  This more or less assumes that no one ever invoked
> QEMU with scsi=off, as it breaks migration from new QEMU, scsi=off to
> old QEMU, also scsi=off.  However new->old is not supported upstream.
> If any downstream cares about new->old migration, they can apply v1 of
> the patches instead of this.
> 
> Paolo Bonzini (3):
>   virtio-blk: report non-zero status when failing SG_IO requests
>   virtio-blk: define VirtIOBlkConf
>   virtio-blk: always enable VIRTIO_BLK_F_SCSI
> 
>  hw/s390-virtio-bus.c |   10 ++++--
>  hw/s390-virtio-bus.h |    4 +-
>  hw/virtio-blk.c      |   77 ++++++++++++++++++++++++-------------------------
>  hw/virtio-blk.h      |   14 +++++----
>  hw/virtio-pci.c      |   11 ++++---
>  hw/virtio-pci.h      |    4 +-
>  hw/virtio.h          |    4 +-
>  7 files changed, 64 insertions(+), 60 deletions(-)
> 

Ping.

Paolo