All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/11] scsi-generic: error handling overhaul
@ 2021-02-24 18:24 Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 01/10] scsi-disk: move scsi_handle_rw_error earlier Paolo Bonzini
                   ` (10 more replies)
  0 siblings, 11 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

This series is mostly a refresh of Hannes's patches at
https://patchew.org/QEMU/20201116184041.60465-1-hare@suse.de/, which
haven't been applied yet because they broke the testsuite.

In addition to Hannes's introduction of host status in the SCSI
subsystem, which survives mostly in patches 3 and 6-10, I am also changing
scsi_handle_rw_error to always pass down guest-recoverable sense codes,
even if rerror=stop/werror=stop.  Without this change, [rw]error=stop are
mostly unusable because the guest will stop even for unit attention sense.

Paolo

Supersedes: <20201116184041.60465-1-hare@suse.de>

Hannes Reinecke (5):
  scsi: drop 'result' argument from command_complete callback
  scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error
    codes
  scsi: Add mapping for generic SCSI_HOST status to sense codes
  scsi: inline sg_io_sense_from_errno() into the callers.
  scsi: move host_status handling into SCSI drivers

Paolo Bonzini (5):
  scsi-disk: move scsi_handle_rw_error earlier
  scsi-disk: do not complete requests early for rerror/werror=ignore
  scsi: introduce scsi_sense_from_errno()
  scsi-disk: pass SCSI status to scsi_handle_rw_error
  scsi-disk: pass guest recoverable errors through even for rerror=stop

 hw/scsi/esp-pci.c        |   5 +-
 hw/scsi/esp.c            |   7 +-
 hw/scsi/lsi53c895a.c     |   6 +-
 hw/scsi/megasas.c        |   6 +-
 hw/scsi/mptsas.c         |   5 +-
 hw/scsi/scsi-bus.c       |  23 ++++-
 hw/scsi/scsi-disk.c      | 203 ++++++++++++++++++++++-----------------
 hw/scsi/scsi-generic.c   |  20 ++--
 hw/scsi/spapr_vscsi.c    |  10 +-
 hw/scsi/virtio-scsi.c    |  51 +++++++++-
 hw/scsi/vmw_pvscsi.c     |  43 ++++++++-
 hw/usb/dev-storage.c     |   6 +-
 hw/usb/dev-uas.c         |   7 +-
 include/hw/scsi/esp.h    |   2 +-
 include/hw/scsi/scsi.h   |   6 +-
 include/scsi/constants.h |   3 +
 include/scsi/utils.h     |  29 ++++--
 scsi/qemu-pr-helper.c    |  24 +++--
 scsi/utils.c             | 119 +++++++++++++++++------
 19 files changed, 395 insertions(+), 180 deletions(-)

-- 
2.29.2



^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH 01/10] scsi-disk: move scsi_handle_rw_error earlier
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 02/10] scsi-disk: do not complete requests early for rerror/werror=ignore Paolo Bonzini
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

Remove the forward declaration.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 168 ++++++++++++++++++++++----------------------
 1 file changed, 83 insertions(+), 85 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index a2716b26b4..18ab777017 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -111,8 +111,6 @@ struct SCSIDiskState {
     uint16_t rotation_rate;
 };
 
-static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed);
-
 static void scsi_free_request(SCSIRequest *req)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -182,6 +180,89 @@ static void scsi_disk_load_request(QEMUFile *f, SCSIRequest *req)
     qemu_iovec_init_external(&r->qiov, &r->iov, 1);
 }
 
+/*
+ * scsi_handle_rw_error has two return values.  False means that the error
+ * must be ignored, true means that the error has been processed and the
+ * caller should not do anything else for this request.  Note that
+ * scsi_handle_rw_error always manages its reference counts, independent
+ * of the return value.
+ */
+static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
+{
+    bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
+    BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
+                                                   is_read, error);
+
+    if (action == BLOCK_ERROR_ACTION_REPORT) {
+        if (acct_failed) {
+            block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
+        }
+        switch (error) {
+        case 0:
+            /* A passthrough command has run and has produced sense data; check
+             * whether the error has to be handled by the guest or should rather
+             * pause the host.
+             */
+            assert(r->status && *r->status);
+            if (scsi_sense_buf_is_guest_recoverable(r->req.sense, sizeof(r->req.sense))) {
+                /* These errors are handled by guest. */
+                sdc->update_sense(&r->req);
+                scsi_req_complete(&r->req, *r->status);
+                return true;
+            }
+            error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
+            break;
+#ifdef CONFIG_LINUX
+            /* These errno mapping are specific to Linux.  For more information:
+             * - scsi_decide_disposition in drivers/scsi/scsi_error.c
+             * - scsi_result_to_blk_status in drivers/scsi/scsi_lib.c
+             * - blk_errors[] in block/blk-core.c
+             */
+        case EBADE:
+            /* DID_NEXUS_FAILURE -> BLK_STS_NEXUS.  */
+            scsi_req_complete(&r->req, RESERVATION_CONFLICT);
+            break;
+        case ENODATA:
+            /* DID_MEDIUM_ERROR -> BLK_STS_MEDIUM.  */
+            scsi_check_condition(r, SENSE_CODE(READ_ERROR));
+            break;
+        case EREMOTEIO:
+            /* DID_TARGET_FAILURE -> BLK_STS_TARGET.  */
+            scsi_req_complete(&r->req, HARDWARE_ERROR);
+            break;
+#endif
+        case ENOMEDIUM:
+            scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
+            break;
+        case ENOMEM:
+            scsi_check_condition(r, SENSE_CODE(TARGET_FAILURE));
+            break;
+        case EINVAL:
+            scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
+            break;
+        case ENOSPC:
+            scsi_check_condition(r, SENSE_CODE(SPACE_ALLOC_FAILED));
+            break;
+        default:
+            scsi_check_condition(r, SENSE_CODE(IO_ERROR));
+            break;
+        }
+    }
+
+    blk_error_action(s->qdev.conf.blk, action, is_read, error);
+    if (action == BLOCK_ERROR_ACTION_IGNORE) {
+        scsi_req_complete(&r->req, 0);
+        return true;
+    }
+
+    if (action == BLOCK_ERROR_ACTION_STOP) {
+        scsi_req_retry(&r->req);
+    }
+    return true;
+}
+
 static bool scsi_disk_req_check_error(SCSIDiskReq *r, int ret, bool acct_failed)
 {
     if (r->req.io_canceled) {
@@ -428,89 +509,6 @@ static void scsi_read_data(SCSIRequest *req)
     }
 }
 
-/*
- * scsi_handle_rw_error has two return values.  False means that the error
- * must be ignored, true means that the error has been processed and the
- * caller should not do anything else for this request.  Note that
- * scsi_handle_rw_error always manages its reference counts, independent
- * of the return value.
- */
-static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
-{
-    bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
-    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-    SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
-    BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
-                                                   is_read, error);
-
-    if (action == BLOCK_ERROR_ACTION_REPORT) {
-        if (acct_failed) {
-            block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
-        }
-        switch (error) {
-        case 0:
-            /* A passthrough command has run and has produced sense data; check
-             * whether the error has to be handled by the guest or should rather
-             * pause the host.
-             */
-            assert(r->status && *r->status);
-            if (scsi_sense_buf_is_guest_recoverable(r->req.sense, sizeof(r->req.sense))) {
-                /* These errors are handled by guest. */
-                sdc->update_sense(&r->req);
-                scsi_req_complete(&r->req, *r->status);
-                return true;
-            }
-            error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
-            break;
-#ifdef CONFIG_LINUX
-            /* These errno mapping are specific to Linux.  For more information:
-             * - scsi_decide_disposition in drivers/scsi/scsi_error.c
-             * - scsi_result_to_blk_status in drivers/scsi/scsi_lib.c
-             * - blk_errors[] in block/blk-core.c
-             */
-        case EBADE:
-            /* DID_NEXUS_FAILURE -> BLK_STS_NEXUS.  */
-            scsi_req_complete(&r->req, RESERVATION_CONFLICT);
-            break;
-        case ENODATA:
-            /* DID_MEDIUM_ERROR -> BLK_STS_MEDIUM.  */
-            scsi_check_condition(r, SENSE_CODE(READ_ERROR));
-            break;
-        case EREMOTEIO:
-            /* DID_TARGET_FAILURE -> BLK_STS_TARGET.  */
-            scsi_req_complete(&r->req, HARDWARE_ERROR);
-            break;
-#endif
-        case ENOMEDIUM:
-            scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
-            break;
-        case ENOMEM:
-            scsi_check_condition(r, SENSE_CODE(TARGET_FAILURE));
-            break;
-        case EINVAL:
-            scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
-            break;
-        case ENOSPC:
-            scsi_check_condition(r, SENSE_CODE(SPACE_ALLOC_FAILED));
-            break;
-        default:
-            scsi_check_condition(r, SENSE_CODE(IO_ERROR));
-            break;
-        }
-    }
-
-    blk_error_action(s->qdev.conf.blk, action, is_read, error);
-    if (action == BLOCK_ERROR_ACTION_IGNORE) {
-        scsi_req_complete(&r->req, 0);
-        return true;
-    }
-
-    if (action == BLOCK_ERROR_ACTION_STOP) {
-        scsi_req_retry(&r->req);
-    }
-    return true;
-}
-
 static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
 {
     uint32_t n;
-- 
2.29.2




^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 02/10] scsi-disk: do not complete requests early for rerror/werror=ignore
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 01/10] scsi-disk: move scsi_handle_rw_error earlier Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 03/10] scsi: introduce scsi_sense_from_errno() Paolo Bonzini
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

When requested to ignore errors, just do nothing and let the
request complete normally.  This means that the request will
be accounted correctly.

This is what commit 40dce4ee61 ("scsi-disk: fix rerror/werror=ignore",
2018-10-19) was supposed to do:

Fixes: 40dce4ee61 ("scsi-disk: fix rerror/werror=ignore", 2018-10-19)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 18ab777017..36aa872445 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -253,8 +253,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
 
     blk_error_action(s->qdev.conf.blk, action, is_read, error);
     if (action == BLOCK_ERROR_ACTION_IGNORE) {
-        scsi_req_complete(&r->req, 0);
-        return true;
+        return false;
     }
 
     if (action == BLOCK_ERROR_ACTION_STOP) {
-- 
2.29.2




^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 03/10] scsi: introduce scsi_sense_from_errno()
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 01/10] scsi-disk: move scsi_handle_rw_error earlier Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 02/10] scsi-disk: do not complete requests early for rerror/werror=ignore Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 04/10] scsi-disk: pass SCSI status to scsi_handle_rw_error Paolo Bonzini
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

The new function is an extension of the switch statement in scsi-disk.c
which also includes the errno cases only found in sg_io_sense_from_errno.
This allows us to consolidate the errno handling.

Extracted from a patch by Hannes Reinecke <hare@suse.de>.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c  | 45 +++++++-------------------------------
 include/scsi/utils.h |  2 ++
 scsi/utils.c         | 51 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 51 insertions(+), 47 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 36aa872445..9c6099ffc4 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -194,13 +194,13 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
     SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
     BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
                                                    is_read, error);
+    SCSISense sense;
 
     if (action == BLOCK_ERROR_ACTION_REPORT) {
         if (acct_failed) {
             block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
         }
-        switch (error) {
-        case 0:
+        if (error == 0) {
             /* A passthrough command has run and has produced sense data; check
              * whether the error has to be handled by the guest or should rather
              * pause the host.
@@ -213,41 +213,12 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
                 return true;
             }
             error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
-            break;
-#ifdef CONFIG_LINUX
-            /* These errno mapping are specific to Linux.  For more information:
-             * - scsi_decide_disposition in drivers/scsi/scsi_error.c
-             * - scsi_result_to_blk_status in drivers/scsi/scsi_lib.c
-             * - blk_errors[] in block/blk-core.c
-             */
-        case EBADE:
-            /* DID_NEXUS_FAILURE -> BLK_STS_NEXUS.  */
-            scsi_req_complete(&r->req, RESERVATION_CONFLICT);
-            break;
-        case ENODATA:
-            /* DID_MEDIUM_ERROR -> BLK_STS_MEDIUM.  */
-            scsi_check_condition(r, SENSE_CODE(READ_ERROR));
-            break;
-        case EREMOTEIO:
-            /* DID_TARGET_FAILURE -> BLK_STS_TARGET.  */
-            scsi_req_complete(&r->req, HARDWARE_ERROR);
-            break;
-#endif
-        case ENOMEDIUM:
-            scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
-            break;
-        case ENOMEM:
-            scsi_check_condition(r, SENSE_CODE(TARGET_FAILURE));
-            break;
-        case EINVAL:
-            scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
-            break;
-        case ENOSPC:
-            scsi_check_condition(r, SENSE_CODE(SPACE_ALLOC_FAILED));
-            break;
-        default:
-            scsi_check_condition(r, SENSE_CODE(IO_ERROR));
-            break;
+        } else {
+            int status = scsi_sense_from_errno(error, &sense);
+            if (status == CHECK_CONDITION) {
+                scsi_req_build_sense(&r->req, sense);
+            }
+            scsi_req_complete(&r->req, status);
         }
     }
 
diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index 096489c6cd..ff7c7091b6 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -135,4 +135,6 @@ int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
                            SCSISense *sense);
 #endif
 
+int scsi_sense_from_errno(int errno_value, SCSISense *sense);
+
 #endif
diff --git a/scsi/utils.c b/scsi/utils.c
index 793c3a6b9c..6b56e01002 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -565,21 +565,52 @@ const char *scsi_command_name(uint8_t cmd)
     return names[cmd];
 }
 
+int scsi_sense_from_errno(int errno_value, SCSISense *sense)
+{
+    switch (errno_value) {
+    case 0:
+        return GOOD;
+    case EDOM:
+        return TASK_SET_FULL;
+#ifdef CONFIG_LINUX
+        /* These errno mapping are specific to Linux.  For more information:
+         * - scsi_decide_disposition in drivers/scsi/scsi_error.c
+         * - scsi_result_to_blk_status in drivers/scsi/scsi_lib.c
+         * - blk_errors[] in block/blk-core.c
+         */
+    case EBADE:
+        return RESERVATION_CONFLICT;
+    case ENODATA:
+        *sense = SENSE_CODE(READ_ERROR);
+        return CHECK_CONDITION;
+    case EREMOTEIO:
+        *sense = SENSE_CODE(LUN_COMM_FAILURE);
+        return CHECK_CONDITION;
+#endif
+    case ENOMEDIUM:
+        *sense = SENSE_CODE(NO_MEDIUM);
+        return CHECK_CONDITION;
+    case ENOMEM:
+        *sense = SENSE_CODE(TARGET_FAILURE);
+        return CHECK_CONDITION;
+    case EINVAL:
+        *sense = SENSE_CODE(INVALID_FIELD);
+        return CHECK_CONDITION;
+    case ENOSPC:
+        *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
+        return CHECK_CONDITION;
+    default:
+        *sense = SENSE_CODE(IO_ERROR);
+        return CHECK_CONDITION;
+    }
+}
+
 #ifdef CONFIG_LINUX
 int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
                            SCSISense *sense)
 {
     if (errno_value != 0) {
-        switch (errno_value) {
-        case EDOM:
-            return TASK_SET_FULL;
-        case ENOMEM:
-            *sense = SENSE_CODE(TARGET_FAILURE);
-            return CHECK_CONDITION;
-        default:
-            *sense = SENSE_CODE(IO_ERROR);
-            return CHECK_CONDITION;
-        }
+        return scsi_sense_from_errno(errno_value, sense);
     } else {
         if (io_hdr->host_status == SG_ERR_DID_NO_CONNECT ||
             io_hdr->host_status == SG_ERR_DID_BUS_BUSY ||
-- 
2.29.2




^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 04/10] scsi-disk: pass SCSI status to scsi_handle_rw_error
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
                   ` (2 preceding siblings ...)
  2021-02-24 18:24 ` [PATCH 03/10] scsi: introduce scsi_sense_from_errno() Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 05/10] scsi-disk: pass guest recoverable errors through even for rerror=stop Paolo Bonzini
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

Instead of fishing it from *r->status, just pass the SCSI status
as a positive value of the second parameter and an errno as a
negative value.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 9c6099ffc4..548a5297fa 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -187,34 +187,48 @@ static void scsi_disk_load_request(QEMUFile *f, SCSIRequest *req)
  * scsi_handle_rw_error always manages its reference counts, independent
  * of the return value.
  */
-static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
+static bool scsi_handle_rw_error(SCSIDiskReq *r, int ret, bool acct_failed)
 {
     bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
     SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
-    BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
-                                                   is_read, error);
-    SCSISense sense;
+    SCSISense sense = SENSE_CODE(NO_SENSE);
+    int error = 0;
+    bool req_has_sense = false;
+    BlockErrorAction action;
+    int status;
 
+    if (ret < 0) {
+        status = scsi_sense_from_errno(-ret, &sense);
+        error = -ret;
+    } else {
+        /* A passthrough command has completed with nonzero status.  */
+        status = ret;
+        if (status == CHECK_CONDITION) {
+            req_has_sense = true;
+            error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
+        } else {
+            error = EINVAL;
+        }
+    }
+
+    action = blk_get_error_action(s->qdev.conf.blk, is_read, error);
     if (action == BLOCK_ERROR_ACTION_REPORT) {
         if (acct_failed) {
             block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
         }
-        if (error == 0) {
+        if (req_has_sense) {
             /* A passthrough command has run and has produced sense data; check
              * whether the error has to be handled by the guest or should rather
              * pause the host.
              */
-            assert(r->status && *r->status);
             if (scsi_sense_buf_is_guest_recoverable(r->req.sense, sizeof(r->req.sense))) {
                 /* These errors are handled by guest. */
                 sdc->update_sense(&r->req);
-                scsi_req_complete(&r->req, *r->status);
+                scsi_req_complete(&r->req, status);
                 return true;
             }
-            error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
         } else {
-            int status = scsi_sense_from_errno(error, &sense);
             if (status == CHECK_CONDITION) {
                 scsi_req_build_sense(&r->req, sense);
             }
@@ -240,8 +254,10 @@ static bool scsi_disk_req_check_error(SCSIDiskReq *r, int ret, bool acct_failed)
         return true;
     }
 
-    if (ret < 0 || (r->status && *r->status)) {
-        return scsi_handle_rw_error(r, -ret, acct_failed);
+    if (ret < 0) {
+        return scsi_handle_rw_error(r, ret, acct_failed);
+    } else if (r->status && *r->status) {
+        return scsi_handle_rw_error(r, *r->status, acct_failed);
     }
 
     return false;
-- 
2.29.2




^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 05/10] scsi-disk: pass guest recoverable errors through even for rerror=stop
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
                   ` (3 preceding siblings ...)
  2021-02-24 18:24 ` [PATCH 04/10] scsi-disk: pass SCSI status to scsi_handle_rw_error Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 06/10] scsi: drop 'result' argument from command_complete callback Paolo Bonzini
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

Right now, recoverable sense values are only passed directly to the
guest only for rerror=report.  However, when rerror/werror are 'stop'
we still don't want the host to be involved on every UNIT ATTENTION
(especially considered that the QMP event will not have enough information
to act on the report).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 51 +++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 548a5297fa..a5a58d7db3 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -212,39 +212,44 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int ret, bool acct_failed)
         }
     }
 
-    action = blk_get_error_action(s->qdev.conf.blk, is_read, error);
-    if (action == BLOCK_ERROR_ACTION_REPORT) {
+    /*
+     * Check whether the error has to be handled by the guest or should
+     * rather follow the rerror=/werror= settings.  Guest-handled errors
+     * are usually retried immediately, so do not post them to QMP and
+     * do not account them as failed I/O.
+     */
+    if (req_has_sense &&
+        scsi_sense_buf_is_guest_recoverable(r->req.sense, sizeof(r->req.sense))) {
+        action = BLOCK_ERROR_ACTION_REPORT;
+        acct_failed = false;
+    } else {
+        action = blk_get_error_action(s->qdev.conf.blk, is_read, error);
+        blk_error_action(s->qdev.conf.blk, action, is_read, error);
+    }
+
+    switch (action) {
+    case BLOCK_ERROR_ACTION_REPORT:
         if (acct_failed) {
             block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
         }
         if (req_has_sense) {
-            /* A passthrough command has run and has produced sense data; check
-             * whether the error has to be handled by the guest or should rather
-             * pause the host.
-             */
-            if (scsi_sense_buf_is_guest_recoverable(r->req.sense, sizeof(r->req.sense))) {
-                /* These errors are handled by guest. */
-                sdc->update_sense(&r->req);
-                scsi_req_complete(&r->req, status);
-                return true;
-            }
-        } else {
-            if (status == CHECK_CONDITION) {
-                scsi_req_build_sense(&r->req, sense);
-            }
-            scsi_req_complete(&r->req, status);
+            sdc->update_sense(&r->req);
+        } else if (status == CHECK_CONDITION) {
+            scsi_req_build_sense(&r->req, sense);
         }
-    }
+        scsi_req_complete(&r->req, status);
+        return true;
 
-    blk_error_action(s->qdev.conf.blk, action, is_read, error);
-    if (action == BLOCK_ERROR_ACTION_IGNORE) {
+    case BLOCK_ERROR_ACTION_IGNORE:
         return false;
-    }
 
-    if (action == BLOCK_ERROR_ACTION_STOP) {
+    case BLOCK_ERROR_ACTION_STOP:
         scsi_req_retry(&r->req);
+        return true;
+
+    default:
+        g_assert_not_reached();
     }
-    return true;
 }
 
 static bool scsi_disk_req_check_error(SCSIDiskReq *r, int ret, bool acct_failed)
-- 
2.29.2




^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 06/10] scsi: drop 'result' argument from command_complete callback
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
                   ` (4 preceding siblings ...)
  2021-02-24 18:24 ` [PATCH 05/10] scsi-disk: pass guest recoverable errors through even for rerror=stop Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 07/10] scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error codes Paolo Bonzini
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

From: Hannes Reinecke <hare@suse.de>

The command complete callback has a SCSIRequest as the first argument,
and the status field of that structure is identical to the 'status'
argument. So drop the argument from the callback.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Message-Id: <20201116184041.60465-3-hare@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/esp-pci.c      |  5 ++---
 hw/scsi/esp.c          |  7 +++----
 hw/scsi/lsi53c895a.c   |  6 +++---
 hw/scsi/megasas.c      |  6 ++----
 hw/scsi/mptsas.c       |  5 +++--
 hw/scsi/scsi-bus.c     |  2 +-
 hw/scsi/spapr_vscsi.c  | 10 +++++-----
 hw/scsi/virtio-scsi.c  |  5 ++---
 hw/scsi/vmw_pvscsi.c   |  4 ++--
 hw/usb/dev-storage.c   |  6 +++---
 hw/usb/dev-uas.c       |  7 +++----
 include/hw/scsi/esp.h  |  2 +-
 include/hw/scsi/scsi.h |  2 +-
 13 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index 2ce96dc56e..4d7c2cab56 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -329,13 +329,12 @@ static const VMStateDescription vmstate_esp_pci_scsi = {
     }
 };
 
-static void esp_pci_command_complete(SCSIRequest *req, uint32_t status,
-                                     size_t resid)
+static void esp_pci_command_complete(SCSIRequest *req, size_t resid)
 {
     ESPState *s = req->hba_private;
     PCIESPState *pci = container_of(s, PCIESPState, esp);
 
-    esp_command_complete(req, status, resid);
+    esp_command_complete(req, resid);
     pci->dma_regs[DMA_WBC] = 0;
     pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
 }
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index b84e0fe33e..93d9c9c7b9 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -485,8 +485,7 @@ static void esp_report_command_complete(ESPState *s, uint32_t status)
     }
 }
 
-void esp_command_complete(SCSIRequest *req, uint32_t status,
-                          size_t resid)
+void esp_command_complete(SCSIRequest *req, size_t resid)
 {
     ESPState *s = req->hba_private;
 
@@ -495,11 +494,11 @@ void esp_command_complete(SCSIRequest *req, uint32_t status,
          * interrupt has been handled.
          */
         trace_esp_command_complete_deferred();
-        s->deferred_status = status;
+        s->deferred_status = req->status;
         s->deferred_complete = true;
         return;
     }
-    esp_report_command_complete(s, status);
+    esp_report_command_complete(s, req->status);
 }
 
 void esp_transfer_data(SCSIRequest *req, uint32_t len)
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 7d13c7dc1c..a4e58580e4 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -787,14 +787,14 @@ static int lsi_queue_req(LSIState *s, SCSIRequest *req, uint32_t len)
 }
 
  /* Callback to indicate that the SCSI layer has completed a command.  */
-static void lsi_command_complete(SCSIRequest *req, uint32_t status, size_t resid)
+static void lsi_command_complete(SCSIRequest *req, size_t resid)
 {
     LSIState *s = LSI53C895A(req->bus->qbus.parent);
     int out;
 
     out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
-    trace_lsi_command_complete(status);
-    s->status = status;
+    trace_lsi_command_complete(req->status);
+    s->status = req->status;
     s->command_complete = 2;
     if (s->waiting && s->dbc != 0) {
         /* Raise phase mismatch for short transfers.  */
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 5bfc92fca1..8f2389d2c6 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1852,13 +1852,12 @@ static void megasas_xfer_complete(SCSIRequest *req, uint32_t len)
     }
 }
 
-static void megasas_command_complete(SCSIRequest *req, uint32_t status,
-                                     size_t resid)
+static void megasas_command_complete(SCSIRequest *req, size_t resid)
 {
     MegasasCmd *cmd = req->hba_private;
     uint8_t cmd_status = MFI_STAT_OK;
 
-    trace_megasas_command_complete(cmd->index, status, resid);
+    trace_megasas_command_complete(cmd->index, req->status, resid);
 
     if (req->io_canceled) {
         return;
@@ -1873,7 +1872,6 @@ static void megasas_command_complete(SCSIRequest *req, uint32_t status,
             return;
         }
     } else {
-        req->status = status;
         trace_megasas_scsi_complete(cmd->index, req->status,
                                     cmd->iov_size, req->cmd.xfer);
         if (req->status != GOOD) {
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index f86616544b..7416e78706 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1133,7 +1133,7 @@ static QEMUSGList *mptsas_get_sg_list(SCSIRequest *sreq)
 }
 
 static void mptsas_command_complete(SCSIRequest *sreq,
-        uint32_t status, size_t resid)
+        size_t resid)
 {
     MPTSASRequest *req = sreq->hba_private;
     MPTSASState *s = req->dev;
@@ -1143,7 +1143,8 @@ static void mptsas_command_complete(SCSIRequest *sreq,
     hwaddr sense_buffer_addr = req->dev->sense_buffer_high_addr |
             req->scsi_io.SenseBufferLowAddr;
 
-    trace_mptsas_command_complete(s, req->scsi_io.MsgContext, status, resid);
+    trace_mptsas_command_complete(s, req->scsi_io.MsgContext,
+                                  sreq->status, resid);
 
     sense_len = scsi_req_get_sense(sreq, sense_buf, SCSI_SENSE_BUF_SIZE);
     if (sense_len > 0) {
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index c349fb7f2d..dc4141ec8d 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1483,7 +1483,7 @@ void scsi_req_complete(SCSIRequest *req, int status)
 
     scsi_req_ref(req);
     scsi_req_dequeue(req);
-    req->bus->info->complete(req, req->status, req->resid);
+    req->bus->info->complete(req, req->resid);
 
     /* Cancelled requests might end up being completed instead of cancelled */
     notifier_list_notify(&req->cancel_notifiers, req);
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 4aa0224c47..d653b5a6ad 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -551,19 +551,19 @@ static void vscsi_transfer_data(SCSIRequest *sreq, uint32_t len)
 }
 
 /* Callback to indicate that the SCSI layer has completed a transfer.  */
-static void vscsi_command_complete(SCSIRequest *sreq, uint32_t status, size_t resid)
+static void vscsi_command_complete(SCSIRequest *sreq, size_t resid)
 {
     VSCSIState *s = VIO_SPAPR_VSCSI_DEVICE(sreq->bus->qbus.parent);
     vscsi_req *req = sreq->hba_private;
     int32_t res_in = 0, res_out = 0;
 
-    trace_spapr_vscsi_command_complete(sreq->tag, status, req);
+    trace_spapr_vscsi_command_complete(sreq->tag, sreq->status, req);
     if (req == NULL) {
         fprintf(stderr, "VSCSI: Can't find request for tag 0x%x\n", sreq->tag);
         return;
     }
 
-    if (status == CHECK_CONDITION) {
+    if (sreq->status == CHECK_CONDITION) {
         req->senselen = scsi_req_get_sense(req->sreq, req->sense,
                                            sizeof(req->sense));
         trace_spapr_vscsi_command_complete_sense_data1(req->senselen,
@@ -574,8 +574,8 @@ static void vscsi_command_complete(SCSIRequest *sreq, uint32_t status, size_t re
                 req->sense[12], req->sense[13], req->sense[14], req->sense[15]);
     }
 
-    trace_spapr_vscsi_command_complete_status(status);
-    if (status == 0) {
+    trace_spapr_vscsi_command_complete_status(sreq->status);
+    if (sreq->status == 0) {
         /* We handle overflows, not underflows for normal commands,
          * but hopefully nobody cares
          */
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 99ff261cea..358c0e70b0 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -500,8 +500,7 @@ static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req)
     virtio_scsi_complete_req(req);
 }
 
-static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
-                                         size_t resid)
+static void virtio_scsi_command_complete(SCSIRequest *r, size_t resid)
 {
     VirtIOSCSIReq *req = r->hba_private;
     uint8_t sense[SCSI_SENSE_BUF_SIZE];
@@ -513,7 +512,7 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
     }
 
     req->resp.cmd.response = VIRTIO_SCSI_S_OK;
-    req->resp.cmd.status = status;
+    req->resp.cmd.status = r->status;
     if (req->resp.cmd.status == GOOD) {
         req->resp.cmd.resid = virtio_tswap32(vdev, resid);
     } else {
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index a63d25de48..0da378ed50 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -511,7 +511,7 @@ pvscsi_write_sense(PVSCSIRequest *r, uint8_t *sense, int len)
 }
 
 static void
-pvscsi_command_complete(SCSIRequest *req, uint32_t status, size_t resid)
+pvscsi_command_complete(SCSIRequest *req, size_t resid)
 {
     PVSCSIRequest *pvscsi_req = req->hba_private;
     PVSCSIState *s;
@@ -528,7 +528,7 @@ pvscsi_command_complete(SCSIRequest *req, uint32_t status, size_t resid)
         pvscsi_req->cmp.hostStatus = BTSTAT_DATARUN;
     }
 
-    pvscsi_req->cmp.scsiStatus = status;
+    pvscsi_req->cmp.scsiStatus = req->status;
     if (pvscsi_req->cmp.scsiStatus == CHECK_CONDITION) {
         uint8_t sense[SCSI_SENSE_BUF_SIZE];
         int sense_len =
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index c49e8b819e..a5f76fc001 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -277,17 +277,17 @@ static void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
     }
 }
 
-static void usb_msd_command_complete(SCSIRequest *req, uint32_t status, size_t resid)
+static void usb_msd_command_complete(SCSIRequest *req, size_t resid)
 {
     MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
     USBPacket *p = s->packet;
 
-    trace_usb_msd_cmd_complete(status, req->tag);
+    trace_usb_msd_cmd_complete(req->status, req->tag);
 
     s->csw.sig = cpu_to_le32(0x53425355);
     s->csw.tag = cpu_to_le32(req->tag);
     s->csw.residue = cpu_to_le32(s->data_len);
-    s->csw.status = status != 0;
+    s->csw.status = req->status != 0;
 
     if (s->packet) {
         if (s->data_len == 0 && s->mode == USB_MSDM_DATAOUT) {
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index a51402bc0b..d2bd85d3f6 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -598,17 +598,16 @@ static void usb_uas_scsi_transfer_data(SCSIRequest *r, uint32_t len)
     }
 }
 
-static void usb_uas_scsi_command_complete(SCSIRequest *r,
-                                          uint32_t status, size_t resid)
+static void usb_uas_scsi_command_complete(SCSIRequest *r, size_t resid)
 {
     UASRequest *req = r->hba_private;
 
-    trace_usb_uas_scsi_complete(req->uas->dev.addr, req->tag, status, resid);
+    trace_usb_uas_scsi_complete(req->uas->dev.addr, req->tag, r->status, resid);
     req->complete = true;
     if (req->data) {
         usb_uas_complete_data_packet(req);
     }
-    usb_uas_queue_sense(req, status);
+    usb_uas_queue_sense(req, r->status);
     scsi_req_unref(req->req);
 }
 
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 60cc3047a5..d8a6263c13 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -151,7 +151,7 @@ struct SysBusESPState {
 
 void esp_dma_enable(ESPState *s, int irq, int level);
 void esp_request_cancelled(SCSIRequest *req);
-void esp_command_complete(SCSIRequest *req, uint32_t status, size_t resid);
+void esp_command_complete(SCSIRequest *req, size_t resid);
 void esp_transfer_data(SCSIRequest *req, uint32_t len);
 void esp_hard_reset(ESPState *s);
 uint64_t esp_reg_read(ESPState *s, uint32_t saddr);
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index a8ef59c0f4..5d992e6e1d 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -123,7 +123,7 @@ struct SCSIBusInfo {
     int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
                      void *hba_private);
     void (*transfer_data)(SCSIRequest *req, uint32_t arg);
-    void (*complete)(SCSIRequest *req, uint32_t arg, size_t resid);
+    void (*complete)(SCSIRequest *req, size_t resid);
     void (*cancel)(SCSIRequest *req);
     void (*change)(SCSIBus *bus, SCSIDevice *dev, SCSISense sense);
     QEMUSGList *(*get_sg_list)(SCSIRequest *req);
-- 
2.29.2




^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 07/10] scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error codes
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
                   ` (5 preceding siblings ...)
  2021-02-24 18:24 ` [PATCH 06/10] scsi: drop 'result' argument from command_complete callback Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 08/10] scsi: Add mapping for generic SCSI_HOST status to sense codes Paolo Bonzini
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

From: Hannes Reinecke <hare@suse.de>

We really should make a distinction between legitimate sense codes
(ie if one is running against an emulated block device or for
pass-through sense codes), and the intermediate errors generated
during processing of the command, which really are not sense codes
but refer to some specific internal status. And this internal
state is not necessarily linux-specific, but rather can refer to
the qemu implementation itself.
So rename the linux-only SG_ERR codes to SCSI_HOST codes and make
them available generally.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Message-Id: <20201116184041.60465-5-hare@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/scsi/utils.h | 23 ++++++++++++++++-------
 scsi/utils.c         |  6 +++---
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index ff7c7091b6..ddb22b56df 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -16,6 +16,22 @@ enum SCSIXferMode {
     SCSI_XFER_TO_DEV,    /*  WRITE, MODE_SELECT, ...         */
 };
 
+enum SCSIHostStatus {
+    SCSI_HOST_OK,
+    SCSI_HOST_NO_LUN,
+    SCSI_HOST_BUSY,
+    SCSI_HOST_TIME_OUT,
+    SCSI_HOST_BAD_RESPONSE,
+    SCSI_HOST_ABORTED,
+    SCSI_HOST_ERROR = 0x07,
+    SCSI_HOST_RESET = 0x08,
+    SCSI_HOST_TRANSPORT_DISRUPTED = 0xe,
+    SCSI_HOST_TARGET_FAILURE = 0x10,
+    SCSI_HOST_RESERVATION_ERROR = 0x11,
+    SCSI_HOST_ALLOCATION_FAILURE = 0x12,
+    SCSI_HOST_MEDIUM_ERROR = 0x13,
+};
+
 typedef struct SCSICommand {
     uint8_t buf[SCSI_CMD_BUF_SIZE];
     int len;
@@ -124,13 +140,6 @@ int scsi_cdb_length(uint8_t *buf);
 #define SG_ERR_DRIVER_TIMEOUT  0x06
 #define SG_ERR_DRIVER_SENSE    0x08
 
-#define SG_ERR_DID_OK          0x00
-#define SG_ERR_DID_NO_CONNECT  0x01
-#define SG_ERR_DID_BUS_BUSY    0x02
-#define SG_ERR_DID_TIME_OUT    0x03
-
-#define SG_ERR_DRIVER_SENSE    0x08
-
 int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
                            SCSISense *sense);
 #endif
diff --git a/scsi/utils.c b/scsi/utils.c
index 6b56e01002..4d994b6d56 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -612,9 +612,9 @@ int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
     if (errno_value != 0) {
         return scsi_sense_from_errno(errno_value, sense);
     } else {
-        if (io_hdr->host_status == SG_ERR_DID_NO_CONNECT ||
-            io_hdr->host_status == SG_ERR_DID_BUS_BUSY ||
-            io_hdr->host_status == SG_ERR_DID_TIME_OUT ||
+        if (io_hdr->host_status == SCSI_HOST_NO_LUN ||
+            io_hdr->host_status == SCSI_HOST_BUSY ||
+            io_hdr->host_status == SCSI_HOST_TIME_OUT ||
             (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT)) {
             return BUSY;
         } else if (io_hdr->host_status) {
-- 
2.29.2




^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 08/10] scsi: Add mapping for generic SCSI_HOST status to sense codes
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
                   ` (6 preceding siblings ...)
  2021-02-24 18:24 ` [PATCH 07/10] scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error codes Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 09/10] scsi: inline sg_io_sense_from_errno() into the callers Paolo Bonzini
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

From: Hannes Reinecke <hare@suse.de>

As we don't have a driver-specific mapping (yet) we should provide
for a detailed mapping from host_status to SCSI sense codes.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Message-Id: <20201116184041.60465-6-hare@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/scsi/utils.h |  1 +
 scsi/utils.c         | 65 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index ddb22b56df..9080d65e27 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -145,5 +145,6 @@ int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
 #endif
 
 int scsi_sense_from_errno(int errno_value, SCSISense *sense);
+int scsi_sense_from_host_status(uint8_t host_status, SCSISense *sense);
 
 #endif
diff --git a/scsi/utils.c b/scsi/utils.c
index 4d994b6d56..28eb32746e 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -257,6 +257,21 @@ const struct SCSISense sense_code_LUN_COMM_FAILURE = {
     .key = ABORTED_COMMAND, .asc = 0x08, .ascq = 0x00
 };
 
+/* Command aborted, LUN does not respond to selection */
+const struct SCSISense sense_code_LUN_NOT_RESPONDING = {
+    .key = ABORTED_COMMAND, .asc = 0x05, .ascq = 0x00
+};
+
+/* Command aborted, Command Timeout during processing */
+const struct SCSISense sense_code_COMMAND_TIMEOUT = {
+    .key = ABORTED_COMMAND, .asc = 0x2e, .ascq = 0x02
+};
+
+/* Command aborted, Commands cleared by device server */
+const struct SCSISense sense_code_COMMAND_ABORTED = {
+    .key = ABORTED_COMMAND, .asc = 0x2f, .ascq = 0x02
+};
+
 /* Medium Error, Unrecovered read error */
 const struct SCSISense sense_code_READ_ERROR = {
     .key = MEDIUM_ERROR, .asc = 0x11, .ascq = 0x00
@@ -605,6 +620,45 @@ int scsi_sense_from_errno(int errno_value, SCSISense *sense)
     }
 }
 
+int scsi_sense_from_host_status(uint8_t host_status,
+                                SCSISense *sense)
+{
+    switch (host_status) {
+    case SCSI_HOST_NO_LUN:
+        *sense = SENSE_CODE(LUN_NOT_RESPONDING);
+        return CHECK_CONDITION;
+    case SCSI_HOST_BUSY:
+        return BUSY;
+    case SCSI_HOST_TIME_OUT:
+        *sense = SENSE_CODE(COMMAND_TIMEOUT);
+        return CHECK_CONDITION;
+    case SCSI_HOST_BAD_RESPONSE:
+        *sense = SENSE_CODE(LUN_COMM_FAILURE);
+        return CHECK_CONDITION;
+    case SCSI_HOST_ABORTED:
+        *sense = SENSE_CODE(COMMAND_ABORTED);
+        return CHECK_CONDITION;
+    case SCSI_HOST_RESET:
+        *sense = SENSE_CODE(RESET);
+        return CHECK_CONDITION;
+    case SCSI_HOST_TRANSPORT_DISRUPTED:
+        *sense = SENSE_CODE(I_T_NEXUS_LOSS);
+        return CHECK_CONDITION;
+    case SCSI_HOST_TARGET_FAILURE:
+        *sense = SENSE_CODE(TARGET_FAILURE);
+        return CHECK_CONDITION;
+    case SCSI_HOST_RESERVATION_ERROR:
+        return RESERVATION_CONFLICT;
+    case SCSI_HOST_ALLOCATION_FAILURE:
+        *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
+        return CHECK_CONDITION;
+    case SCSI_HOST_MEDIUM_ERROR:
+        *sense = SENSE_CODE(READ_ERROR);
+        return CHECK_CONDITION;
+    }
+    return GOOD;
+}
+
 #ifdef CONFIG_LINUX
 int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
                            SCSISense *sense)
@@ -612,14 +666,11 @@ int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
     if (errno_value != 0) {
         return scsi_sense_from_errno(errno_value, sense);
     } else {
-        if (io_hdr->host_status == SCSI_HOST_NO_LUN ||
-            io_hdr->host_status == SCSI_HOST_BUSY ||
-            io_hdr->host_status == SCSI_HOST_TIME_OUT ||
-            (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT)) {
+        int status = scsi_sense_from_host_status(io_hdr->host_status, sense);
+        if (status) {
+            return status;
+        } else if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
             return BUSY;
-        } else if (io_hdr->host_status) {
-            *sense = SENSE_CODE(I_T_NEXUS_LOSS);
-            return CHECK_CONDITION;
         } else if (io_hdr->status) {
             return io_hdr->status;
         } else if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
-- 
2.29.2




^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 09/10] scsi: inline sg_io_sense_from_errno() into the callers.
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
                   ` (7 preceding siblings ...)
  2021-02-24 18:24 ` [PATCH 08/10] scsi: Add mapping for generic SCSI_HOST status to sense codes Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 18:24 ` [PATCH 10/10] scsi: move host_status handling into SCSI drivers Paolo Bonzini
  2021-02-24 19:05 ` [PATCH 00/11] scsi-generic: error handling overhaul Philippe Mathieu-Daudé
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

From: Hannes Reinecke <hare@suse.de>

Currently sg_io_sense_from_errno() converts the two input parameters
'errno' and 'io_hdr' into sense code and SCSI status. Having
split the function off into scsi_sense_from_errno() and
scsi_sense_from_host_status(), both of which are available generically,
we now inline the logic in the callers so that scsi-disk and
scsi-generic will be able to pass host_status to the HBA.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Message-Id: <20201116184041.60465-7-hare@suse.de>
[Put together from "scsi-disk: Add sg_io callback to evaluate status"
 and what remains of "scsi: split sg_io_sense_from_errno() in two functions",
 with many other fixes. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c    | 47 +++++++++++++++++++++++++++++++++++++-----
 hw/scsi/scsi-generic.c | 22 ++++++++++++++------
 include/scsi/utils.h   |  3 ---
 scsi/qemu-pr-helper.c  | 24 ++++++++++++++-------
 scsi/utils.c           | 23 ---------------------
 5 files changed, 75 insertions(+), 44 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index a5a58d7db3..ceaf78b423 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -77,7 +77,6 @@ typedef struct SCSIDiskReq {
     struct iovec iov;
     QEMUIOVector qiov;
     BlockAcctCookie acct;
-    unsigned char *status;
 } SCSIDiskReq;
 
 #define SCSI_DISK_F_REMOVABLE             0
@@ -261,8 +260,6 @@ static bool scsi_disk_req_check_error(SCSIDiskReq *r, int ret, bool acct_failed)
 
     if (ret < 0) {
         return scsi_handle_rw_error(r, ret, acct_failed);
-    } else if (r->status && *r->status) {
-        return scsi_handle_rw_error(r, *r->status, acct_failed);
     }
 
     return false;
@@ -2697,8 +2694,47 @@ typedef struct SCSIBlockReq {
 
     /* CDB passed to SG_IO.  */
     uint8_t cdb[16];
+    BlockCompletionFunc *cb;
+    void *cb_opaque;
 } SCSIBlockReq;
 
+static void scsi_block_sgio_complete(void *opaque, int ret)
+{
+    SCSIBlockReq *req = (SCSIBlockReq *)opaque;
+    SCSIDiskReq *r = &req->req;
+    SCSIDevice *s = r->req.dev;
+    sg_io_hdr_t *io_hdr = &req->io_header;
+    SCSISense sense;
+
+    if (ret == 0) {
+        if (io_hdr->host_status != SCSI_HOST_OK) {
+            ret = scsi_sense_from_host_status(io_hdr->host_status, &sense);
+            if (ret == CHECK_CONDITION) {
+                scsi_req_build_sense(&r->req, sense);
+            }
+        } else if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
+            ret = BUSY;
+        } else {
+            ret = io_hdr->status;
+        }
+
+        if (ret > 0) {
+            aio_context_acquire(blk_get_aio_context(s->conf.blk));
+            if (scsi_handle_rw_error(r, ret, true)) {
+                aio_context_release(blk_get_aio_context(s->conf.blk));
+                scsi_req_unref(&r->req);
+                return;
+            }
+            aio_context_release(blk_get_aio_context(s->conf.blk));
+
+            /* Ignore error.  */
+            ret = 0;
+        }
+    }
+
+    req->cb(req->cb_opaque, ret);
+}
+
 static BlockAIOCB *scsi_block_do_sgio(SCSIBlockReq *req,
                                       int64_t offset, QEMUIOVector *iov,
                                       int direction,
@@ -2777,9 +2813,11 @@ static BlockAIOCB *scsi_block_do_sgio(SCSIBlockReq *req,
     io_header->timeout = s->qdev.io_timeout * 1000;
     io_header->usr_ptr = r;
     io_header->flags |= SG_FLAG_DIRECT_IO;
+    req->cb = cb;
+    req->cb_opaque = opaque;
     trace_scsi_disk_aio_sgio_command(r->req.tag, req->cdb[0], lba,
                                      nb_logical_blocks, io_header->timeout);
-    aiocb = blk_aio_ioctl(s->qdev.conf.blk, SG_IO, io_header, cb, opaque);
+    aiocb = blk_aio_ioctl(s->qdev.conf.blk, SG_IO, io_header, scsi_block_sgio_complete, req);
     assert(aiocb != NULL);
     return aiocb;
 }
@@ -2893,7 +2931,6 @@ static int32_t scsi_block_dma_command(SCSIRequest *req, uint8_t *buf)
         return 0;
     }
 
-    r->req.status = &r->io_header.status;
     return scsi_disk_dma_command(req, buf);
 }
 
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index cf7e11cf44..716caf0d22 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -75,6 +75,7 @@ static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
 {
     int status;
     SCSISense sense;
+    sg_io_hdr_t *io_hdr = &r->io_header;
 
     assert(r->req.aiocb == NULL);
 
@@ -82,15 +83,24 @@ static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
         scsi_req_cancel_complete(&r->req);
         goto done;
     }
-    status = sg_io_sense_from_errno(-ret, &r->io_header, &sense);
-    if (status == CHECK_CONDITION) {
-        if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
-            r->req.sense_len = r->io_header.sb_len_wr;
-        } else {
+    if (ret < 0) {
+        status = scsi_sense_from_errno(-ret, &sense);
+        if (status == CHECK_CONDITION) {
+            scsi_req_build_sense(&r->req, sense);
+        }
+    } else if (io_hdr->host_status != SCSI_HOST_OK) {
+        status = scsi_sense_from_host_status(io_hdr->host_status, &sense);
+        if (status == CHECK_CONDITION) {
             scsi_req_build_sense(&r->req, sense);
         }
+    } else if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
+        status = BUSY;
+    } else {
+        status = io_hdr->status;
+        if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
+            r->req.sense_len = io_hdr->sb_len_wr;
+        }
     }
-
     trace_scsi_generic_command_complete_noio(r, r->req.tag, status);
 
     scsi_req_complete(&r->req, status);
diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index 9080d65e27..d5c8efa16e 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -139,9 +139,6 @@ int scsi_cdb_length(uint8_t *buf);
 #ifdef CONFIG_LINUX
 #define SG_ERR_DRIVER_TIMEOUT  0x06
 #define SG_ERR_DRIVER_SENSE    0x08
-
-int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
-                           SCSISense *sense);
 #endif
 
 int scsi_sense_from_errno(int errno_value, SCSISense *sense);
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 2733d92f2d..0cb2bddd61 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -149,19 +149,29 @@ static int do_sgio_worker(void *opaque)
     io_hdr.dxferp = (char *)data->buf;
     io_hdr.dxfer_len = data->sz;
     ret = ioctl(data->fd, SG_IO, &io_hdr);
-    status = sg_io_sense_from_errno(ret < 0 ? errno : 0, &io_hdr,
-                                    &sense_code);
+
+    if (ret < 0) {
+        status = scsi_sense_from_errno(errno, &sense_code);
+        if (status == CHECK_CONDITION) {
+            scsi_build_sense(data->sense, sense_code);
+        }
+    } else if (io_hdr.host_status != SCSI_HOST_OK) {
+        status = scsi_sense_from_host_status(io_hdr.host_status, &sense_code);
+        if (status == CHECK_CONDITION) {
+            scsi_build_sense(data->sense, sense_code);
+        }
+    } else if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
+        status = BUSY;
+    } else {
+        status = io_hdr.status;
+    }
+
     if (status == GOOD) {
         data->sz -= io_hdr.resid;
     } else {
         data->sz = 0;
     }
 
-    if (status == CHECK_CONDITION &&
-        !(io_hdr.driver_status & SG_ERR_DRIVER_SENSE)) {
-        scsi_build_sense(data->sense, sense_code);
-    }
-
     return status;
 }
 
diff --git a/scsi/utils.c b/scsi/utils.c
index 28eb32746e..873e05aeaf 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -658,26 +658,3 @@ int scsi_sense_from_host_status(uint8_t host_status,
     }
     return GOOD;
 }
-
-#ifdef CONFIG_LINUX
-int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
-                           SCSISense *sense)
-{
-    if (errno_value != 0) {
-        return scsi_sense_from_errno(errno_value, sense);
-    } else {
-        int status = scsi_sense_from_host_status(io_hdr->host_status, sense);
-        if (status) {
-            return status;
-        } else if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
-            return BUSY;
-        } else if (io_hdr->status) {
-            return io_hdr->status;
-        } else if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
-            return CHECK_CONDITION;
-        } else {
-            return GOOD;
-        }
-    }
-}
-#endif
-- 
2.29.2




^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 10/10] scsi: move host_status handling into SCSI drivers
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
                   ` (8 preceding siblings ...)
  2021-02-24 18:24 ` [PATCH 09/10] scsi: inline sg_io_sense_from_errno() into the callers Paolo Bonzini
@ 2021-02-24 18:24 ` Paolo Bonzini
  2021-02-24 19:05 ` [PATCH 00/11] scsi-generic: error handling overhaul Philippe Mathieu-Daudé
  10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-02-24 18:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: hare

From: Hannes Reinecke <hare@suse.de>

Some SCSI drivers like virtio have an internal mapping for the
host_status. This patch moves the host_status translation into
the SCSI drivers to allow those drivers to set up the correct
values.

Signed-off-by: Hannes Reinecke <hare@suse.de>.
[Added default handling to avoid touching all drivers. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c       | 23 +++++++++++++++++++-
 hw/scsi/scsi-disk.c      |  7 ++----
 hw/scsi/scsi-generic.c   |  6 ++----
 hw/scsi/virtio-scsi.c    | 46 ++++++++++++++++++++++++++++++++++++++++
 hw/scsi/vmw_pvscsi.c     | 39 ++++++++++++++++++++++++++++++++++
 include/hw/scsi/scsi.h   |  4 +++-
 include/scsi/constants.h |  3 +++
 7 files changed, 117 insertions(+), 11 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index dc4141ec8d..6088601925 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -692,6 +692,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
     req->lun = lun;
     req->hba_private = hba_private;
     req->status = -1;
+    req->host_status = -1;
     req->ops = reqops;
     object_ref(OBJECT(d));
     object_ref(OBJECT(qbus->parent));
@@ -1455,6 +1456,17 @@ void scsi_req_print(SCSIRequest *req)
     }
 }
 
+static void scsi_req_complete_failed(SCSIRequest *req)
+{
+     SCSISense sense;
+
+     req->status = scsi_sense_from_host_status(req->host_status, &sense);
+     if (req->status == CHECK_CONDITION) {
+         scsi_req_build_sense(req, sense);
+     }
+     req->bus->info->complete(req, 0);
+}
+
 void scsi_req_complete(SCSIRequest *req, int status)
 {
     assert(req->status == -1);
@@ -1483,7 +1495,16 @@ void scsi_req_complete(SCSIRequest *req, int status)
 
     scsi_req_ref(req);
     scsi_req_dequeue(req);
-    req->bus->info->complete(req, req->resid);
+    if (status == CHECK_HOST_STATUS) {
+        if (req->bus->info->fail) {
+            req->bus->info->fail(req);
+        } else {
+            scsi_req_complete_failed(req);
+        }
+    } else {
+        req->host_status = SCSI_HOST_OK;
+        req->bus->info->complete(req, req->resid);
+    }
 
     /* Cancelled requests might end up being completed instead of cancelled */
     notifier_list_notify(&req->cancel_notifiers, req);
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ceaf78b423..b7e53b1b6f 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2704,14 +2704,11 @@ static void scsi_block_sgio_complete(void *opaque, int ret)
     SCSIDiskReq *r = &req->req;
     SCSIDevice *s = r->req.dev;
     sg_io_hdr_t *io_hdr = &req->io_header;
-    SCSISense sense;
 
     if (ret == 0) {
         if (io_hdr->host_status != SCSI_HOST_OK) {
-            ret = scsi_sense_from_host_status(io_hdr->host_status, &sense);
-            if (ret == CHECK_CONDITION) {
-                scsi_req_build_sense(&r->req, sense);
-            }
+            ret = CHECK_HOST_STATUS;
+            r->req.host_status = io_hdr->host_status;
         } else if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
             ret = BUSY;
         } else {
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 716caf0d22..8e26fccf74 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -89,10 +89,8 @@ static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
             scsi_req_build_sense(&r->req, sense);
         }
     } else if (io_hdr->host_status != SCSI_HOST_OK) {
-        status = scsi_sense_from_host_status(io_hdr->host_status, &sense);
-        if (status == CHECK_CONDITION) {
-            scsi_req_build_sense(&r->req, sense);
-        }
+        r->req.host_status = io_hdr->host_status;
+        status = CHECK_HOST_STATUS;
     } else if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
         status = BUSY;
     } else {
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 358c0e70b0..6d80730287 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -500,6 +500,51 @@ static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req)
     virtio_scsi_complete_req(req);
 }
 
+static void virtio_scsi_command_failed(SCSIRequest *r)
+{
+    VirtIOSCSIReq *req = r->hba_private;
+
+    if (r->io_canceled) {
+        return;
+    }
+
+    req->resp.cmd.status = GOOD;
+    switch (r->host_status) {
+    case SCSI_HOST_NO_LUN:
+        req->resp.cmd.response = VIRTIO_SCSI_S_INCORRECT_LUN;
+        break;
+    case SCSI_HOST_BUSY:
+        req->resp.cmd.response = VIRTIO_SCSI_S_BUSY;
+        break;
+    case SCSI_HOST_TIME_OUT:
+    case SCSI_HOST_ABORTED:
+        req->resp.cmd.response = VIRTIO_SCSI_S_ABORTED;
+        break;
+    case SCSI_HOST_BAD_RESPONSE:
+        req->resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET;
+        break;
+    case SCSI_HOST_RESET:
+        req->resp.cmd.response = VIRTIO_SCSI_S_RESET;
+        break;
+    case SCSI_HOST_TRANSPORT_DISRUPTED:
+        req->resp.cmd.response = VIRTIO_SCSI_S_TRANSPORT_FAILURE;
+        break;
+    case SCSI_HOST_TARGET_FAILURE:
+        req->resp.cmd.response = VIRTIO_SCSI_S_TARGET_FAILURE;
+        break;
+    case SCSI_HOST_RESERVATION_ERROR:
+        req->resp.cmd.response = VIRTIO_SCSI_S_NEXUS_FAILURE;
+        break;
+    case SCSI_HOST_ALLOCATION_FAILURE:
+    case SCSI_HOST_MEDIUM_ERROR:
+    case SCSI_HOST_ERROR:
+    default:
+        req->resp.cmd.response = VIRTIO_SCSI_S_FAILURE;
+        break;
+    }
+    virtio_scsi_complete_cmd_req(req);
+}
+
 static void virtio_scsi_command_complete(SCSIRequest *r, size_t resid)
 {
     VirtIOSCSIReq *req = r->hba_private;
@@ -908,6 +953,7 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
     .max_lun = VIRTIO_SCSI_MAX_LUN,
 
     .complete = virtio_scsi_command_complete,
+    .fail = virtio_scsi_command_failed,
     .cancel = virtio_scsi_request_cancelled,
     .change = virtio_scsi_change,
     .parse_cdb = virtio_scsi_parse_cdb,
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 0da378ed50..1f30cb020a 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -510,6 +510,44 @@ pvscsi_write_sense(PVSCSIRequest *r, uint8_t *sense, int len)
     cpu_physical_memory_write(r->req.senseAddr, sense, r->cmp.senseLen);
 }
 
+static void
+pvscsi_command_failed(SCSIRequest *req)
+{
+    PVSCSIRequest *pvscsi_req = req->hba_private;
+    PVSCSIState *s;
+
+    if (!pvscsi_req) {
+        trace_pvscsi_command_complete_not_found(req->tag);
+        return;
+    }
+    s = pvscsi_req->dev;
+
+    switch (req->host_status) {
+    case SCSI_HOST_NO_LUN:
+        pvscsi_req->cmp.hostStatus = BTSTAT_LUNMISMATCH;
+        break;
+    case SCSI_HOST_BUSY:
+        pvscsi_req->cmp.hostStatus = BTSTAT_ABORTQUEUE;
+        break;
+    case SCSI_HOST_TIME_OUT:
+    case SCSI_HOST_ABORTED:
+        pvscsi_req->cmp.hostStatus = BTSTAT_SENTRST;
+        break;
+    case SCSI_HOST_BAD_RESPONSE:
+        pvscsi_req->cmp.hostStatus = BTSTAT_SELTIMEO;
+        break;
+    case SCSI_HOST_RESET:
+        pvscsi_req->cmp.hostStatus = BTSTAT_BUSRESET;
+        break;
+    default:
+        pvscsi_req->cmp.hostStatus = BTSTAT_HASOFTWARE;
+        break;
+    }
+    pvscsi_req->cmp.scsiStatus = GOOD;
+    qemu_sglist_destroy(&pvscsi_req->sgl);
+    pvscsi_complete_request(s, pvscsi_req);
+}
+
 static void
 pvscsi_command_complete(SCSIRequest *req, size_t resid)
 {
@@ -1103,6 +1141,7 @@ static const struct SCSIBusInfo pvscsi_scsi_info = {
         .get_sg_list = pvscsi_get_sg_list,
         .complete = pvscsi_command_complete,
         .cancel = pvscsi_request_cancelled,
+        .fail = pvscsi_command_failed,
 };
 
 static void
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 5d992e6e1d..ee7876674c 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -27,7 +27,8 @@ struct SCSIRequest {
     uint32_t          refcount;
     uint32_t          tag;
     uint32_t          lun;
-    uint32_t          status;
+    int16_t           status;
+    int16_t           host_status;
     void              *hba_private;
     size_t            resid;
     SCSICommand       cmd;
@@ -123,6 +124,7 @@ struct SCSIBusInfo {
     int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
                      void *hba_private);
     void (*transfer_data)(SCSIRequest *req, uint32_t arg);
+    void (*fail)(SCSIRequest *req);
     void (*complete)(SCSIRequest *req, size_t resid);
     void (*cancel)(SCSIRequest *req);
     void (*change)(SCSIBus *bus, SCSIDevice *dev, SCSISense sense);
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 2a32c08b5e..0ee29bfd7d 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -181,6 +181,9 @@
 
 #define STATUS_MASK          0x3e
 
+/* QEMU only.  */
+#define CHECK_HOST_STATUS    0xff
+
 /*
  *  SENSE KEYS
  */
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH 00/11] scsi-generic: error handling overhaul
  2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
                   ` (9 preceding siblings ...)
  2021-02-24 18:24 ` [PATCH 10/10] scsi: move host_status handling into SCSI drivers Paolo Bonzini
@ 2021-02-24 19:05 ` Philippe Mathieu-Daudé
  10 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-24 19:05 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: hare

On 2/24/21 7:24 PM, Paolo Bonzini wrote:
> This series is mostly a refresh of Hannes's patches at
> https://patchew.org/QEMU/20201116184041.60465-1-hare@suse.de/, which
> haven't been applied yet because they broke the testsuite.
> 
> In addition to Hannes's introduction of host status in the SCSI
> subsystem, which survives mostly in patches 3 and 6-10, I am also changing
> scsi_handle_rw_error to always pass down guest-recoverable sense codes,
> even if rerror=stop/werror=stop.  Without this change, [rw]error=stop are
> mostly unusable because the guest will stop even for unit attention sense.
> 
> Paolo
> 
> Supersedes: <20201116184041.60465-1-hare@suse.de>
> 
> Hannes Reinecke (5):
>   scsi: drop 'result' argument from command_complete callback
>   scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error
>     codes
>   scsi: Add mapping for generic SCSI_HOST status to sense codes
>   scsi: inline sg_io_sense_from_errno() into the callers.
>   scsi: move host_status handling into SCSI drivers
> 
> Paolo Bonzini (5):
>   scsi-disk: move scsi_handle_rw_error earlier
>   scsi-disk: do not complete requests early for rerror/werror=ignore
>   scsi: introduce scsi_sense_from_errno()
>   scsi-disk: pass SCSI status to scsi_handle_rw_error
>   scsi-disk: pass guest recoverable errors through even for rerror=stop

$ b4 am 20210224182453.587731-1-pbonzini@redhat.com
Grabbing thread from lore.kernel.org/qemu-devel
Analyzing 11 messages in the thread
---
  ERROR: missing [11/11]!
---
Total patches: 10
---
WARNING: Thread incomplete!

I suppose this is OK ;)

patchew is stuck too:
Only 10 patches received!
https://patchew.org/QEMU/20210224182453.587731-1-pbonzini@redhat.com/



^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-02-24 19:13 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-24 18:24 [PATCH 00/11] scsi-generic: error handling overhaul Paolo Bonzini
2021-02-24 18:24 ` [PATCH 01/10] scsi-disk: move scsi_handle_rw_error earlier Paolo Bonzini
2021-02-24 18:24 ` [PATCH 02/10] scsi-disk: do not complete requests early for rerror/werror=ignore Paolo Bonzini
2021-02-24 18:24 ` [PATCH 03/10] scsi: introduce scsi_sense_from_errno() Paolo Bonzini
2021-02-24 18:24 ` [PATCH 04/10] scsi-disk: pass SCSI status to scsi_handle_rw_error Paolo Bonzini
2021-02-24 18:24 ` [PATCH 05/10] scsi-disk: pass guest recoverable errors through even for rerror=stop Paolo Bonzini
2021-02-24 18:24 ` [PATCH 06/10] scsi: drop 'result' argument from command_complete callback Paolo Bonzini
2021-02-24 18:24 ` [PATCH 07/10] scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error codes Paolo Bonzini
2021-02-24 18:24 ` [PATCH 08/10] scsi: Add mapping for generic SCSI_HOST status to sense codes Paolo Bonzini
2021-02-24 18:24 ` [PATCH 09/10] scsi: inline sg_io_sense_from_errno() into the callers Paolo Bonzini
2021-02-24 18:24 ` [PATCH 10/10] scsi: move host_status handling into SCSI drivers Paolo Bonzini
2021-02-24 19:05 ` [PATCH 00/11] scsi-generic: error handling overhaul Philippe Mathieu-Daudé

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.