[Qemu-devel] [PATCH for-2.4 0/3] scsi: fixes for failed requests

[Qemu-devel] [PATCH for-2.4 0/3] scsi: fixes for failed requests

[Stefan Hajnoczi]

When requests fail the error policy (-drive rerror=,werror=) determines what
happens.  The 'stop' policy pauses the guest and waits for the administrator to
resolve the storage problem.  It is possible to live migrate during this time
and the failed requests can be restarted on the destination host.

Two bugs:
1. Segfault due to missing sgs mapping when loading migrated failed requests.
2. Incorrect error action due to broken is_read logic.

I also noticed that the unaligned WRITE SAME test case in
tests/virtio-scsi-test.c is broken.  I've included a fix for that too.

Stefan Hajnoczi (3):
  virtio-scsi: use virtqueue_map_sg() when loading requests
  scsi-disk: fix cmd.mode field typo
  tests: virtio-scsi: clear unit attention after reset

 hw/scsi/scsi-disk.c      |  2 +-
 hw/scsi/virtio-scsi.c    |  5 +++
 tests/virtio-scsi-test.c | 90 +++++++++++++++++++++++++++++-------------------
 3 files changed, 60 insertions(+), 37 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 1/3] virtio-scsi: use virtqueue_map_sg() when loading requests

[Stefan Hajnoczi]

The VirtQueueElement struct is serialized during migration but the
in_sg[]/out_sg[] iovec arrays are not usable on the destination host
because the pointers are meaningless.

Use virtqueue_map_sg() to refresh in_sg[]/out_sg[] to valid pointers
based on in_addr[]/out_addr[] hwaddrs.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/virtio-scsi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 811c3da..a8bb1c6 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -217,6 +217,11 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
     assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));
 
+    virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
+                     req->elem.in_num, 1);
+    virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
+                     req->elem.out_num, 0);
+
     if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
                               sizeof(VirtIOSCSICmdResp) + vs->sense_size) < 0) {
         error_report("invalid SCSI request migration data");
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 2/3] scsi-disk: fix cmd.mode field typo

[Stefan Hajnoczi]

The cmd.xfer field is the data length.  The cmd.mode field is the data
transfer direction.

scsi_handle_rw_error() was using the wrong error policy for read
requests.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/scsi-disk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 64f0694..73fed3f 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -399,7 +399,7 @@ static void scsi_read_data(SCSIRequest *req)
  */
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error)
 {
-    bool is_read = (r->req.cmd.xfer == SCSI_XFER_FROM_DEV);
+    bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
     BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
                                                    is_read, error);
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 3/3] tests: virtio-scsi: clear unit attention after reset

[Stefan Hajnoczi]

The unit attention after reset (power on) prevents normal commands from
running.  The unaligned WRITE SAME test never executed its command!

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/virtio-scsi-test.c | 90 +++++++++++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 36 deletions(-)

diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 11ccdd6..6e91ca9 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -13,6 +13,7 @@
 #include "libqtest.h"
 #include "qemu/osdep.h"
 #include <stdio.h>
+#include "block/scsi.h"
 #include "libqos/virtio.h"
 #include "libqos/virtio-pci.h"
 #include "libqos/pci-pc.h"
@@ -71,40 +72,6 @@ static void qvirtio_scsi_stop(void)
     qtest_end();
 }
 
-static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot)
-{
-    QVirtIOSCSI *vs;
-    QVirtioPCIDevice *dev;
-    void *addr;
-    int i;
-
-    vs = g_new0(QVirtIOSCSI, 1);
-    vs->alloc = pc_alloc_init();
-    vs->bus = qpci_init_pc();
-
-    dev = qvirtio_pci_device_find(vs->bus, QVIRTIO_SCSI_DEVICE_ID);
-    vs->dev = (QVirtioDevice *)dev;
-    g_assert(dev != NULL);
-    g_assert_cmphex(vs->dev->device_type, ==, QVIRTIO_SCSI_DEVICE_ID);
-
-    qvirtio_pci_device_enable(dev);
-    qvirtio_reset(&qvirtio_pci, vs->dev);
-    qvirtio_set_acknowledge(&qvirtio_pci, vs->dev);
-    qvirtio_set_driver(&qvirtio_pci, vs->dev);
-
-    addr = dev->addr + QVIRTIO_PCI_DEVICE_SPECIFIC_NO_MSIX;
-    vs->num_queues = qvirtio_config_readl(&qvirtio_pci, vs->dev,
-                                          (uint64_t)(uintptr_t)addr);
-
-    g_assert_cmpint(vs->num_queues, <, MAX_NUM_QUEUES);
-
-    for (i = 0; i < vs->num_queues + 2; i++) {
-        vs->vq[i] = qvirtqueue_setup(&qvirtio_pci, vs->dev, vs->alloc, i);
-    }
-
-    return vs;
-}
-
 static void qvirtio_scsi_pci_free(QVirtIOSCSI *vs)
 {
     int i;
@@ -134,7 +101,8 @@ static uint64_t qvirtio_scsi_alloc(QVirtIOSCSI *vs, size_t alloc_size,
 static uint8_t virtio_scsi_do_command(QVirtIOSCSI *vs, const uint8_t *cdb,
                                       const uint8_t *data_in,
                                       size_t data_in_len,
-                                      uint8_t *data_out, size_t data_out_len)
+                                      uint8_t *data_out, size_t data_out_len,
+                                      QVirtIOSCSICmdResp *resp_out)
 {
     QVirtQueue *vq;
     QVirtIOSCSICmdReq req = { { 0 } };
@@ -174,6 +142,10 @@ static uint8_t virtio_scsi_do_command(QVirtIOSCSI *vs, const uint8_t *cdb,
 
     response = readb(resp_addr + offsetof(QVirtIOSCSICmdResp, response));
 
+    if (resp_out) {
+        memread(resp_addr, resp_out, sizeof(*resp_out));
+    }
+
     guest_free(vs->alloc, req_addr);
     guest_free(vs->alloc, resp_addr);
     guest_free(vs->alloc, data_in_addr);
@@ -181,6 +153,52 @@ static uint8_t virtio_scsi_do_command(QVirtIOSCSI *vs, const uint8_t *cdb,
     return response;
 }
 
+static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot)
+{
+    const uint8_t test_unit_ready_cdb[CDB_SIZE] = {};
+    QVirtIOSCSI *vs;
+    QVirtioPCIDevice *dev;
+    QVirtIOSCSICmdResp resp;
+    void *addr;
+    int i;
+
+    vs = g_new0(QVirtIOSCSI, 1);
+    vs->alloc = pc_alloc_init();
+    vs->bus = qpci_init_pc();
+
+    dev = qvirtio_pci_device_find(vs->bus, QVIRTIO_SCSI_DEVICE_ID);
+    vs->dev = (QVirtioDevice *)dev;
+    g_assert(dev != NULL);
+    g_assert_cmphex(vs->dev->device_type, ==, QVIRTIO_SCSI_DEVICE_ID);
+
+    qvirtio_pci_device_enable(dev);
+    qvirtio_reset(&qvirtio_pci, vs->dev);
+    qvirtio_set_acknowledge(&qvirtio_pci, vs->dev);
+    qvirtio_set_driver(&qvirtio_pci, vs->dev);
+
+    addr = dev->addr + QVIRTIO_PCI_DEVICE_SPECIFIC_NO_MSIX;
+    vs->num_queues = qvirtio_config_readl(&qvirtio_pci, vs->dev,
+                                          (uint64_t)(uintptr_t)addr);
+
+    g_assert_cmpint(vs->num_queues, <, MAX_NUM_QUEUES);
+
+    for (i = 0; i < vs->num_queues + 2; i++) {
+        vs->vq[i] = qvirtqueue_setup(&qvirtio_pci, vs->dev, vs->alloc, i);
+    }
+
+    /* Clear the POWER ON OCCURRED unit attention */
+    g_assert_cmpint(virtio_scsi_do_command(vs, test_unit_ready_cdb,
+                                           NULL, 0, NULL, 0, &resp),
+                    ==, 0);
+    g_assert_cmpint(resp.status, ==, CHECK_CONDITION);
+    g_assert_cmpint(resp.sense[0], ==, 0x70); /* Fixed format sense buffer */
+    g_assert_cmpint(resp.sense[2], ==, UNIT_ATTENTION);
+    g_assert_cmpint(resp.sense[12], ==, 0x29); /* POWER ON */
+    g_assert_cmpint(resp.sense[13], ==, 0x00);
+
+    return vs;
+}
+
 /* Tests only initialization so far. TODO: Replace with functional tests */
 static void pci_nop(void)
 {
@@ -231,7 +249,7 @@ static void test_unaligned_write_same(void)
     vs = qvirtio_scsi_pci_init(PCI_SLOT);
 
     g_assert_cmphex(0, ==,
-        virtio_scsi_do_command(vs, write_same_cdb, NULL, 0, buf, 512));
+        virtio_scsi_do_command(vs, write_same_cdb, NULL, 0, buf, 512, NULL));
 
     qvirtio_scsi_pci_free(vs);
     qvirtio_scsi_stop();
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.4 0/3] scsi: fixes for failed requests

[Paolo Bonzini]

On 30/07/2015 15:16, Stefan Hajnoczi wrote:
> When requests fail the error policy (-drive rerror=,werror=) determines what
> happens.  The 'stop' policy pauses the guest and waits for the administrator to
> resolve the storage problem.  It is possible to live migrate during this time
> and the failed requests can be restarted on the destination host.
> 
> Two bugs:
> 1. Segfault due to missing sgs mapping when loading migrated failed requests.
> 2. Incorrect error action due to broken is_read logic.
> 
> I also noticed that the unaligned WRITE SAME test case in
> tests/virtio-scsi-test.c is broken.  I've included a fix for that too.
> 
> Stefan Hajnoczi (3):
>   virtio-scsi: use virtqueue_map_sg() when loading requests
>   scsi-disk: fix cmd.mode field typo
>   tests: virtio-scsi: clear unit attention after reset
> 
>  hw/scsi/scsi-disk.c      |  2 +-
>  hw/scsi/virtio-scsi.c    |  5 +++
>  tests/virtio-scsi-test.c | 90 +++++++++++++++++++++++++++++-------------------
>  3 files changed, 60 insertions(+), 37 deletions(-)
> 

All good, thanks!  I'll send a pull request asap.

Paolo