[Qemu-devel] [PATCH 6/7] scsi: Update sense handling

[Hannes Reinecke]

Each driver uses it's own means of storing the sense data, so the
sense code should be stored within the driver-specific structure.
And we should not allow a direct access to it but rather implement
an accessor 'scsi_req_sense' to copy the sense data into a
device-supplied buffer.

Signed-off-by: Hannes Reinecke <address@hidden>
---
 hw/megasas.c      |   52 +++++++++++++++++++++++++++++++++++++---------------
 hw/scsi-bus.c     |    9 +++++++--
 hw/scsi-disk.c    |   24 ++++++++++++++++++++----
 hw/scsi-generic.c |   18 +++++++++++++++++-
 hw/scsi.h         |    4 +++-
 5 files changed, 84 insertions(+), 23 deletions(-)

diff --git a/hw/megasas.c b/hw/megasas.c
index bc35566..a75872b 100644
--- a/hw/megasas.c
+++ b/hw/megasas.c
@@ -180,23 +180,46 @@ static void megasas_unmap_sgl(struct megasas_cmd_t *cmd)
 /*
  * passthrough sense and io sense are at the same offset
  */
-static void megasas_build_sense(struct megasas_cmd_t *cmd, SCSISense sense)
+static int megasas_build_sense(struct megasas_cmd_t *cmd, uint8_t *sense_ptr,
+    uint8_t sense_len)
 {
-    uint8_t *sense_ptr;
-    uint8_t sense_len;
     target_phys_addr_t pa, pa_hi = 0, pa_lo;
     uint16_t flags = le16_to_cpu(cmd->frame->header.flags);
     int is_sense64 = (flags & MFI_FRAME_SENSE64) ? 1 : 0;
 
-    sense_ptr = qemu_mallocz(cmd->frame->header.sense_len);
-    sense_len = scsi_build_sense(SENSE_CODE(INVALID_OPCODE), sense_ptr,
-                                 cmd->frame->header.sense_len, 0);
+    if (sense_len > cmd->frame->header.sense_len)
+        sense_len = cmd->frame->header.sense_len;
+
     pa_lo = le32_to_cpu(cmd->frame->pass.sense_addr_lo);
     if (is_sense64)
         pa_hi = le32_to_cpu(cmd->frame->pass.sense_addr_hi);
     pa = ((uint64_t) pa_hi << 32) | pa_lo;
     cpu_physical_memory_write(pa, sense_ptr, sense_len);
-    megasas_frame_set_sense_len(cmd->pa, sense_len);
+    cmd->frame->header.sense_len = sense_len;
+    return sense_len;
+}
+
+static void megasas_write_sense(struct megasas_cmd_t *cmd, SCSISense sense)
+{
+    uint8_t *sense_ptr;
+    uint8_t sense_len;
+
+    sense_ptr = qemu_mallocz(cmd->frame->header.sense_len);
+    sense_len = scsi_build_sense(sense, sense_ptr,
+                                 cmd->frame->header.sense_len, 0);
+    megasas_build_sense(cmd, sense_ptr, sense_len);
+    qemu_free(sense_ptr);
+}
+
+static void megasas_copy_sense(struct megasas_cmd_t *cmd)
+{
+    uint8_t *sense_ptr;
+    uint8_t sense_len;
+
+    sense_ptr = qemu_mallocz(cmd->frame->header.sense_len);
+    sense_len = scsi_req_sense(cmd->req, sense_ptr,
+                               cmd->frame->header.sense_len);
+    megasas_build_sense(cmd, sense_ptr, sense_len);
     qemu_free(sense_ptr);
 }
 
@@ -1124,8 +1147,8 @@ static int megasas_handle_scsi(MPTState *s, struct 
megasas_cmd_t *cmd, int is_lo
                 mfi_frame_desc[cmd->frame->header.frame_cmd],
                 cmd->frame->header.target_id, cmd->frame->header.lun_id,
                 cmd->frame->header.cdb_len);
-        megasas_build_sense(cmd, SENSE_CODE(INVALID_OPCODE));
-        megasas_frame_set_scsi_status(cmd->pa, CHECK_CONDITION);
+        megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
+        cmd->frame->header.scsi_status = CHECK_CONDITION;
         s->event_count++;
         return MFI_STAT_SCSI_DONE_WITH_ERROR;
     }
@@ -1176,8 +1199,8 @@ static int megasas_handle_io(MPTState *s, struct 
megasas_cmd_t *cmd)
                 mfi_frame_desc[cmd->frame->header.frame_cmd],
                 cmd->frame->header.target_id, cmd->frame->header.lun_id,
                 cmd->frame->header.cdb_len);
-        megasas_build_sense(cmd, SENSE_CODE(INVALID_OPCODE));
-        megasas_frame_set_scsi_status(cmd->pa, CHECK_CONDITION);
+        megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
+        cmd->frame->header.scsi_status = CHECK_CONDITION;
         s->event_count++;
         return MFI_STAT_SCSI_DONE_WITH_ERROR;
     }
@@ -1234,14 +1257,13 @@ static void megasas_command_complete(SCSIRequest *req)
         }
     } else {
         DPRINTF_IO("%s req %p cmd %p lun %p finished with status %x len %u\n",
-                   mfi_frame_desc[cmd->frame->header.frame_cmd], req, cmd, 
cmd->sdev,
-                   req->status, (unsigned)req->xferlen);
+                   mfi_frame_desc[cmd->frame->header.frame_cmd], req, cmd,
+                   cmd->sdev, req->status, (unsigned)req->xferlen);
         if (req->status != GOOD) {
             cmd_status = MFI_STAT_SCSI_DONE_WITH_ERROR;
         }
         if (req->status == CHECK_CONDITION) {
-            megasas_build_sense(cmd, cmd->sdev->sense);
-            scsi_dev_clear_sense(cmd->sdev);
+            megasas_copy_sense(cmd);
         }
 
         megasas_unmap_sgl(cmd);
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index c0e6dd3..b7c5178 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -200,9 +200,9 @@ int scsi_build_sense(SCSISense sense, uint8_t *buf, int 
len, int fixed)
     return len;
 }
 
-void scsi_dev_clear_sense(SCSIDevice *dev)
+int scsi_sense_valid(SCSISense sense)
 {
-    memset(&dev->sense, 0, sizeof(dev->sense));
+    return (sense.key != NO_SENSE);
 }
 
 SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t 
lun)
@@ -685,3 +685,8 @@ void scsi_req_put(SCSIRequest *req)
 {
     return req->dev->info->request_put(req);
 }
+
+int scsi_req_sense(SCSIRequest *req, uint8_t *buf, int len)
+{
+    return req->dev->info->request_sense(req, buf, len);
+}
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index fb3c189..a186874 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -67,6 +67,7 @@ struct SCSIDiskState
     uint32_t max_inflight;
     uint32_t max_xfer;
     uint32_t counts[32];
+    struct SCSISense sense;
 };
 
 /* fwd decls */
@@ -91,8 +92,10 @@ static void scsi_remove_request(SCSIDiskReq *r)
 
 static void scsi_req_set_status(SCSIRequest *req, int status, SCSISense sense)
 {
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
+
     req->status = status;
-    req->dev->sense = sense;
+    s->sense = sense;
 }
 
 static void scsi_dma_restart_bh(void *opaque)
@@ -534,9 +537,9 @@ static int scsi_disk_emulate_command(SCSIRequest *req, 
uint8_t *outbuf)
     case REQUEST_SENSE:
         if (req->cmd.xfer < 4)
             goto illegal_request;
-       buflen = scsi_build_sense(req->dev->sense, outbuf, req->cmd.xfer,
-                                 req->cmd.buf[1] & 0x1);
-        scsi_dev_clear_sense(req->dev);
+        buflen = scsi_build_sense(s->sense, outbuf, req->cmd.xfer,
+                                  req->cmd.buf[1] & 0x1);
+        memset(&s->sense, 0, sizeof(s->sense));
         break;
     case INQUIRY:
         buflen = scsi_disk_emulate_inquiry(req, outbuf);
@@ -940,6 +943,18 @@ static void scsi_disk_req_put(SCSIRequest *req)
     scsi_remove_request(r);
 }
 
+static int scsi_disk_req_sense(SCSIRequest *req, uint8_t *buf, int len)
+{
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
+    int n = 0;
+
+    if (scsi_sense_valid(s->sense)) {
+        n = scsi_build_sense(s->sense, buf, len, 0);
+        memset(&s->sense, 0, sizeof(s->sense));
+    }
+    return n;
+}
+
 static SCSIDeviceInfo scsi_disk_info = {
     .qdev.name    = "scsi-disk",
     .qdev.desc    = "virtual scsi disk or cdrom",
@@ -952,6 +967,7 @@ static SCSIDeviceInfo scsi_disk_info = {
     .request_buf  = scsi_disk_req_buf,
     .request_sgl  = scsi_disk_req_sgl,
     .request_put  = scsi_disk_req_put,
+    .request_sense = scsi_disk_req_sense,
 
     .qdev.props   = (Property[]) {
         DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index fe58975..1776048 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -98,7 +98,7 @@ static void scsi_command_complete(void *opaque, int ret)
     if (ret != 0) {
         if (ret == -EDOM) {
             /* sg driver uses EDOM to signal queue busy */
-            fprintf(stderr, "%s: sg queue busy\n", __FUNCTION__);
+            DPRINTF("%s: sg queue busy\n", __FUNCTION__);
             r->req.status = TASK_SET_FULL;
         } else {
             scsi_req_print(&r->req);
@@ -523,6 +523,21 @@ static void scsi_generic_req_put(SCSIRequest *req)
     scsi_remove_request(r);
 }
 
+static int scsi_generic_req_sense(SCSIRequest *req, uint8_t *buf, int len)
+{
+    SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
+    SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, r->req.dev);
+
+    if (buf == NULL || !len || !s->senselen) {
+        return 0;
+    }
+    if (len > s->senselen) {
+        len = s->senselen;
+    }
+    memcpy(buf, s->sensebuf, len);
+    return len;
+}
+
 static SCSIDeviceInfo scsi_generic_info = {
     .qdev.name    = "scsi-generic",
     .qdev.desc    = "pass through generic scsi device (/dev/sg*)",
@@ -535,6 +550,7 @@ static SCSIDeviceInfo scsi_generic_info = {
     .request_buf  = scsi_generic_req_buf,
     .request_sgl  = scsi_generic_req_sgl,
     .request_put  = scsi_generic_req_put,
+    .request_sense = scsi_generic_req_sense,
 
     .qdev.props   = (Property[]) {
         DEFINE_BLOCK_PROPERTIES(SCSIGenericState, qdev.conf),
diff --git a/hw/scsi.h b/hw/scsi.h
index 645fa45..1d5ce93 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -60,7 +60,6 @@ struct SCSIDevice
     QTAILQ_HEAD(, SCSIRequest) requests;
     int blocksize;
     int type;
-    struct SCSISense sense;
 };
 
 /* cdrom.c */
@@ -81,6 +80,7 @@ struct SCSIDeviceInfo {
     int (*request_buf)(SCSIRequest *req, uint8_t *buffer);
     int (*request_sgl)(SCSIRequest *req, struct QEMUSGList *sg);
     void (*request_put)(SCSIRequest *req);
+    int (*request_sense)(SCSIRequest *req, uint8_t *buf, int len);
 };
 
 typedef void (*SCSIAttachFn)(DeviceState *host, BlockDriverState *bdrv,
@@ -142,6 +142,8 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, 
uint32_t tag, uint32_t l
 SCSIRequest *scsi_req_find(SCSIDevice *d, uint32_t tag);
 void scsi_req_free(SCSIRequest *req);
 void scsi_req_complete(SCSIRequest *req);
+int scsi_req_sense(SCSIRequest *req, uint8_t *buf, int len);
+int scsi_sense_valid(SCSISense sense);
 
 /* for debugging */
 const char *scsi_status_name(uint8_t status);
-- 
1.6.6.1