[2/3] scsi: replace 'tag' with 'hba_private' pointer

Commit Message

Hannes Reinecke July 1, 2011, 7:42 a.m. UTC

'tag' is just an abstraction to identify the command
from the driver. So we should make that explicit by
replacing 'tag' with a driver-defined pointer 'hba_private'.
This saves the lookup for driver handling several commands
in parallel.

Signed-off-by: Hannes Reinecke <[e-mail]>
---
 hw/esp.c          |    2 +-
 hw/lsi53c895a.c   |   17 ++++++++---------
 hw/scsi-bus.c     |   22 +++++++++++-----------
 hw/scsi-disk.c    |    5 ++---
 hw/scsi-generic.c |    4 ++--
 hw/scsi.h         |    8 ++++----
 hw/spapr_vscsi.c  |   41 ++++++++++++-----------------------------
 hw/usb-msd.c      |   10 +++++-----
 trace-events      |   14 +++++++-------
 9 files changed, 52 insertions(+), 71 deletions(-)

Comments

Paolo Bonzini July 1, 2011, 8:27 a.m. UTC | #1

On 07/01/2011 09:42 AM, Hannes Reinecke wrote:
> 'tag' is just an abstraction to identify the command
> from the driver. So we should make that explicit by
> replacing 'tag' with a driver-defined pointer 'hba_private'.
> This saves the lookup for driver handling several commands
> in parallel.

This makes tracing a bit harder to follow.  Perhaps you can keep the 
transport tag (a uint64_t) in the SCSIRequest for debugging purposes?

> Signed-off-by: Hannes Reinecke<[e-mail]>
> ---
>   hw/esp.c          |    2 +-
>   hw/lsi53c895a.c   |   17 ++++++++---------
>   hw/scsi-bus.c     |   22 +++++++++++-----------
>   hw/scsi-disk.c    |    5 ++---
>   hw/scsi-generic.c |    4 ++--
>   hw/scsi.h         |    8 ++++----
>   hw/spapr_vscsi.c  |   41 ++++++++++++-----------------------------
>   hw/usb-msd.c      |   10 +++++-----
>   trace-events      |   14 +++++++-------
>   9 files changed, 52 insertions(+), 71 deletions(-)
>
> diff --git a/hw/esp.c b/hw/esp.c
> index 6d3f5d2..912ff89 100644
> --- a/hw/esp.c
> +++ b/hw/esp.c
> @@ -244,7 +244,7 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
>
>       DPRINTF("do_busid_cmd: busid 0x%x\n", busid);
>       lun = busid&  7;
> -    s->current_req = scsi_req_new(s->current_dev, 0, lun);
> +    s->current_req = scsi_req_new(s->current_dev, lun, s);

Might as well pass NULL here.  The hba_private value is basically 
unnecessary when the adapter doesn't support tagged command queuing.

> diff --git a/hw/usb-msd.c b/hw/usb-msd.c
> index 86582cc..4e2ea03 100644
> --- a/hw/usb-msd.c
> +++ b/hw/usb-msd.c
> @@ -216,8 +216,8 @@ static void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
>       MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
>       USBPacket *p = s->packet;
>
> -    if (req->tag != s->tag) {
> -        fprintf(stderr, "usb-msd: Unexpected SCSI Tag 0x%x\n", req->tag);
> +    if (req->hba_private != s) {
> +        fprintf(stderr, "usb-msd: Unexpected SCSI command 0x%p\n", req);
>       }

Same here, just pass NULL and remove these ifs.

Otherwise looks like a very good idea.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hannes Reinecke July 1, 2011, 8:57 a.m. UTC | #2

On 07/01/2011 10:27 AM, Paolo Bonzini wrote:
> On 07/01/2011 09:42 AM, Hannes Reinecke wrote:
>> 'tag' is just an abstraction to identify the command
>> from the driver. So we should make that explicit by
>> replacing 'tag' with a driver-defined pointer 'hba_private'.
>> This saves the lookup for driver handling several commands
>> in parallel.
>
> This makes tracing a bit harder to follow. Perhaps you can keep the
> transport tag (a uint64_t) in the SCSIRequest for debugging purposes?
>
Sure. Anything to get the patches accepted :-)

>> Signed-off-by: Hannes Reinecke<[e-mail]>
>> ---
>> hw/esp.c | 2 +-
>> hw/lsi53c895a.c | 17 ++++++++---------
>> hw/scsi-bus.c | 22 +++++++++++-----------
>> hw/scsi-disk.c | 5 ++---
>> hw/scsi-generic.c | 4 ++--
>> hw/scsi.h | 8 ++++----
>> hw/spapr_vscsi.c | 41 ++++++++++++-----------------------------
>> hw/usb-msd.c | 10 +++++-----
>> trace-events | 14 +++++++-------
>> 9 files changed, 52 insertions(+), 71 deletions(-)
>>
>> diff --git a/hw/esp.c b/hw/esp.c
>> index 6d3f5d2..912ff89 100644
>> --- a/hw/esp.c
>> +++ b/hw/esp.c
>> @@ -244,7 +244,7 @@ static void do_busid_cmd(ESPState *s, uint8_t
>> *buf, uint8_t busid)
>>
>> DPRINTF("do_busid_cmd: busid 0x%x\n", busid);
>> lun = busid& 7;
>> - s->current_req = scsi_req_new(s->current_dev, 0, lun);
>> + s->current_req = scsi_req_new(s->current_dev, lun, s);
>
> Might as well pass NULL here. The hba_private value is basically
> unnecessary when the adapter doesn't support tagged command queuing.
>
>> diff --git a/hw/usb-msd.c b/hw/usb-msd.c
>> index 86582cc..4e2ea03 100644
>> --- a/hw/usb-msd.c
>> +++ b/hw/usb-msd.c
>> @@ -216,8 +216,8 @@ static void usb_msd_transfer_data(SCSIRequest
>> *req, uint32_t len)
>> MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
>> USBPacket *p = s->packet;
>>
>> - if (req->tag != s->tag) {
>> - fprintf(stderr, "usb-msd: Unexpected SCSI Tag 0x%x\n", req->tag);
>> + if (req->hba_private != s) {
>> + fprintf(stderr, "usb-msd: Unexpected SCSI command 0x%p\n", req);
>> }
>
> Same here, just pass NULL and remove these ifs.
>
> Otherwise looks like a very good idea.
>
Ok, I'll be resending both.

Cheers,

Hannes

Hannes Reinecke July 1, 2011, 1:11 p.m. UTC | #3

On 07/01/2011 10:27 AM, Paolo Bonzini wrote:
> On 07/01/2011 09:42 AM, Hannes Reinecke wrote:
>> 'tag' is just an abstraction to identify the command
>> from the driver. So we should make that explicit by
>> replacing 'tag' with a driver-defined pointer 'hba_private'.
>> This saves the lookup for driver handling several commands
>> in parallel.
>
> This makes tracing a bit harder to follow. Perhaps you can keep the
> transport tag (a uint64_t) in the SCSIRequest for debugging purposes?
>
Hmm. The transport tag wouldn't have any meaning outside scsi-bus.c.
And it's a 64-bit value. So why can't we use the hba_private pointer 
directly here?
After some I/O has been ongoing the linear 'tag' number becomes 
unreadable very quickly, so there's not much difference here ...

Cheers,

Hannes

Paolo Bonzini July 1, 2011, 2:33 p.m. UTC | #4

On 07/01/2011 03:11 PM, Hannes Reinecke wrote:
> On 07/01/2011 10:27 AM, Paolo Bonzini wrote:
>> On 07/01/2011 09:42 AM, Hannes Reinecke wrote:
>>> 'tag' is just an abstraction to identify the command
>>> from the driver. So we should make that explicit by
>>> replacing 'tag' with a driver-defined pointer 'hba_private'.
>>> This saves the lookup for driver handling several commands
>>> in parallel.
>>
>> This makes tracing a bit harder to follow. Perhaps you can keep the
>> transport tag (a uint64_t) in the SCSIRequest for debugging purposes?
>>
> Hmm. The transport tag wouldn't have any meaning outside scsi-bus.c.

It depends, in vmw_pvscsi I take it from a field in the request block 
that is 0..255.  So either you have a small tag that is recycled but 
stays nice, or a large tag that is unwieldy but should not be recycled 
ever.  A pointer is unwieldy _and_ is recycled, so it gives the worse of 
both worlds.

But I'm not very attached to this, I may even do it myself if/when I 
find the need.  Won't ack yet because of the nit with ESP/USB, but even 
if you do not bother I will ack the next respin.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/hw/esp.c b/hw/esp.c
index 6d3f5d2..912ff89 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -244,7 +244,7 @@  static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
 
     DPRINTF("do_busid_cmd: busid 0x%x\n", busid);
     lun = busid & 7;
-    s->current_req = scsi_req_new(s->current_dev, 0, lun);
+    s->current_req = scsi_req_new(s->current_dev, lun, s);
     datalen = scsi_req_enqueue(s->current_req, buf);
     s->ti_size = datalen;
     if (datalen != 0) {
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 940b43a..272e919 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -670,7 +670,7 @@  static void lsi_request_cancelled(SCSIRequest *req)
         return;
     }
 
-    p = lsi_find_by_tag(s, req->tag);
+    p = req->hba_private;
     if (p) {
         QTAILQ_REMOVE(&s->queue, p, next);
         scsi_req_unref(req);
@@ -680,18 +680,17 @@  static void lsi_request_cancelled(SCSIRequest *req)
 
 /* Record that data is available for a queued command.  Returns zero if
    the device was reselected, nonzero if the IO is deferred.  */
-static int lsi_queue_tag(LSIState *s, uint32_t tag, uint32_t len)
+static int lsi_queue_req(LSIState *s, SCSIRequest *req, uint32_t len)
 {
-    lsi_request *p;
+    lsi_request *p = req->hba_private;
 
-    p = lsi_find_by_tag(s, tag);
     if (!p) {
-        BADF("IO with unknown tag %d\n", tag);
+        BADF("IO with unknown reference %p\n", req->hba_private);
         return 1;
     }
 
     if (p->pending) {
-        BADF("Multiple IO pending for tag %d\n", tag);
+        BADF("Multiple IO pending for request %p\n", p);
     }
     p->pending = len;
     /* Reselect if waiting for it, or if reselection triggers an IRQ
@@ -743,9 +742,9 @@  static void lsi_transfer_data(SCSIRequest *req, uint32_t len)
     LSIState *s = DO_UPCAST(LSIState, dev.qdev, req->bus->qbus.parent);
     int out;
 
-    if (s->waiting == 1 || !s->current || req->tag != s->current->tag ||
+    if (s->waiting == 1 || !s->current || req->hba_private != s->current ||
         (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON))) {
-        if (lsi_queue_tag(s, req->tag, len)) {
+        if (lsi_queue_req(s, req, len)) {
             return;
         }
     }
@@ -789,7 +788,7 @@  static void lsi_do_command(LSIState *s)
     assert(s->current == NULL);
     s->current = qemu_mallocz(sizeof(lsi_request));
     s->current->tag = s->select_tag;
-    s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun);
+    s->current->req = scsi_req_new(dev, s->current_lun, s->current);
 
     n = scsi_req_enqueue(s->current->req, buf);
     if (n) {
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index ad6a730..d1fc481 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -131,7 +131,7 @@  int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
     return res;
 }
 
-SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun)
+SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t lun, void *hba_private)
 {
     SCSIRequest *req;
 
@@ -139,16 +139,16 @@  SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
     req->refcount = 1;
     req->bus = scsi_bus_from_device(d);
     req->dev = d;
-    req->tag = tag;
     req->lun = lun;
+    req->hba_private = hba_private;
     req->status = -1;
-    trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_alloc(req->dev->id, req->lun, req->hba_private);
     return req;
 }
 
-SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun)
+SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t lun, void *hba_private)
 {
-    return d->info->alloc_req(d, tag, lun);
+    return d->info->alloc_req(d, lun, hba_private);
 }
 
 uint8_t *scsi_req_get_buf(SCSIRequest *req)
@@ -182,7 +182,7 @@  int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
 
 static void scsi_req_dequeue(SCSIRequest *req)
 {
-    trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_dequeue(req->dev->id, req->lun, req->hba_private);
     if (req->enqueued) {
         QTAILQ_REMOVE(&req->dev->requests, req, next);
         req->enqueued = false;
@@ -214,7 +214,7 @@  static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
         req->cmd.len = 12;
         break;
     default:
-        trace_scsi_req_parse_bad(req->dev->id, req->lun, req->tag, cmd[0]);
+        trace_scsi_req_parse_bad(req->dev->id, req->lun, req->hba_private, cmd[0]);
         return -1;
     }
 
@@ -412,10 +412,10 @@  int scsi_req_parse(SCSIRequest *req, uint8_t *buf)
     memcpy(req->cmd.buf, buf, req->cmd.len);
     scsi_req_xfer_mode(req);
     req->cmd.lba = scsi_req_lba(req);
-    trace_scsi_req_parsed(req->dev->id, req->lun, req->tag, buf[0],
+    trace_scsi_req_parsed(req->dev->id, req->lun, req->hba_private, buf[0],
                           req->cmd.mode, req->cmd.xfer);
     if (req->cmd.lba != -1) {
-        trace_scsi_req_parsed_lba(req->dev->id, req->lun, req->tag, buf[0],
+        trace_scsi_req_parsed_lba(req->dev->id, req->lun, req->hba_private, buf[0],
                               req->cmd.lba);
     }
     return 0;
@@ -624,7 +624,7 @@  void scsi_req_unref(SCSIRequest *req)
    will start the next chunk or complete the command.  */
 void scsi_req_continue(SCSIRequest *req)
 {
-    trace_scsi_req_continue(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_continue(req->dev->id, req->lun, req->hba_private);
     if (req->cmd.mode == SCSI_XFER_TO_DEV) {
         req->dev->info->write_data(req);
     } else {
@@ -637,7 +637,7 @@  void scsi_req_continue(SCSIRequest *req)
    Once it completes, calling scsi_req_continue will restart I/O.  */
 void scsi_req_data(SCSIRequest *req, int len)
 {
-    trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
+    trace_scsi_req_data(req->dev->id, req->lun, req->hba_private, len);
     req->bus->ops->transfer_data(req, len);
 }
 
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index a8c7372..3098b62 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -80,14 +80,13 @@  struct SCSIDiskState
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
 static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf);
 
-static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
-        uint32_t lun)
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t lun, void *hba_private)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
     SCSIRequest *req;
     SCSIDiskReq *r;
 
-    req = scsi_req_alloc(sizeof(SCSIDiskReq), &s->qdev, tag, lun);
+    req = scsi_req_alloc(sizeof(SCSIDiskReq), &s->qdev, lun, hba_private);
     r = DO_UPCAST(SCSIDiskReq, req, req);
     r->iov.iov_base = qemu_blockalign(s->bs, SCSI_DMA_BUF_SIZE);
     return req;
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 8e59c7e..fa34ca3 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -96,11 +96,11 @@  static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len)
     return size;
 }
 
-static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun)
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t lun, void *hba_private)
 {
     SCSIRequest *req;
 
-    req = scsi_req_alloc(sizeof(SCSIGenericReq), d, tag, lun);
+    req = scsi_req_alloc(sizeof(SCSIGenericReq), d, lun, hba_private);
     return req;
 }
 
diff --git a/hw/scsi.h b/hw/scsi.h
index c1dca35..883309e 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -31,7 +31,6 @@  struct SCSIRequest {
     SCSIBus           *bus;
     SCSIDevice        *dev;
     uint32_t          refcount;
-    uint32_t          tag;
     uint32_t          lun;
     uint32_t          status;
     struct {
@@ -43,6 +42,7 @@  struct SCSIRequest {
     } cmd;
     BlockDriverAIOCB  *aiocb;
     bool enqueued;
+    void *hba_private;
     QTAILQ_ENTRY(SCSIRequest) next;
 };
 
@@ -67,7 +67,7 @@  struct SCSIDeviceInfo {
     DeviceInfo qdev;
     scsi_qdev_initfn init;
     void (*destroy)(SCSIDevice *s);
-    SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun);
+    SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t lun, void *hba_private);
     void (*free_req)(SCSIRequest *req);
     int32_t (*send_command)(SCSIRequest *req, uint8_t *buf);
     void (*read_data)(SCSIRequest *req);
@@ -138,8 +138,8 @@  extern const struct SCSISense sense_code_LUN_FAILURE;
 int scsi_build_sense(SCSISense sense, uint8_t *buf, int len, int fixed);
 int scsi_sense_valid(SCSISense sense);
 
-SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun);
-SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun);
+SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t lun, void *hba_private);
+SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t lun, void *hba_private);
 int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf);
 void scsi_req_free(SCSIRequest *req);
 SCSIRequest *scsi_req_ref(SCSIRequest *req);
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 1c901ef..f119194 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -75,7 +75,6 @@  typedef struct vscsi_req {
 
     /* SCSI request tracking */
     SCSIRequest             *sreq;
-    uint32_t                qtag; /* qemu tag != srp tag */
     int                     lun;
     int                     active;
     long                    data_len;
@@ -113,7 +112,6 @@  static struct vscsi_req *vscsi_get_req(VSCSIState *s)
         req = &s->reqs[i];
         if (!req->active) {
             memset(req, 0, sizeof(*req));
-            req->qtag = i;
             req->active = 1;
             return req;
         }
@@ -121,7 +119,7 @@  static struct vscsi_req *vscsi_get_req(VSCSIState *s)
     return NULL;
 }
 
-static void vscsi_put_req(VSCSIState *s, vscsi_req *req)
+static void vscsi_put_req(vscsi_req *req)
 {
     if (req->sreq != NULL) {
         scsi_req_unref(req->sreq);
@@ -130,15 +128,6 @@  static void vscsi_put_req(VSCSIState *s, vscsi_req *req)
     req->active = 0;
 }
 
-static vscsi_req *vscsi_find_req(VSCSIState *s, SCSIRequest *req)
-{
-    uint32_t tag = req->tag;
-    if (tag >= VSCSI_REQ_LIMIT || !s->reqs[tag].active) {
-        return NULL;
-    }
-    return &s->reqs[tag];
-}
-
 static void vscsi_decode_id_lun(uint64_t srp_lun, int *id, int *lun)
 {
     /* XXX Figure that one out properly ! This is crackpot */
@@ -454,7 +443,7 @@  static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
     if (n) {
         req->senselen = n;
         vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
-        vscsi_put_req(s, req);
+        vscsi_put_req(req);
         return;
     }
 
@@ -467,7 +456,7 @@  static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
     cdb[5] = 0;
     req->sensing = 1;
     n = scsi_req_enqueue(req->sreq, cdb);
-    dprintf("VSCSI: Queued request sense tag 0x%x\n", req->qtag);
+    dprintf("VSCSI: Queued request sense 0x%p\n", req);
     if (n < 0) {
         fprintf(stderr, "VSCSI: REQUEST_SENSE wants write data !?!?!?\n");
         vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
@@ -483,7 +472,7 @@  static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
 static void vscsi_transfer_data(SCSIRequest *sreq, uint32_t len)
 {
     VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, sreq->bus->qbus.parent);
-    vscsi_req *req = vscsi_find_req(s, sreq);
+    vscsi_req *req = sreq->hba_private;
     uint8_t *buf;
     int rc = 0;
 
@@ -530,8 +519,7 @@  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)
 {
-    VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, sreq->bus->qbus.parent);
-    vscsi_req *req = vscsi_find_req(s, sreq);
+    vscsi_req *req = sreq->hba_private;
     int32_t res_in = 0, res_out = 0;
 
     dprintf("VSCSI: SCSI cmd complete, r=0x%x tag=0x%x status=0x%x, req=%p\n",
@@ -563,15 +551,14 @@  static void vscsi_command_complete(SCSIRequest *sreq, uint32_t status)
         }
     }
     vscsi_send_rsp(s, req, 0, res_in, res_out);
-    vscsi_put_req(s, req);
+    vscsi_put_req(req);
 }
 
 static void vscsi_request_cancelled(SCSIRequest *sreq)
 {
-    VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, sreq->bus->qbus.parent);
-    vscsi_req *req = vscsi_find_req(s, sreq);
+    vscsi_req *req = sreq->hba_private;
 
-    vscsi_put_req(s, req);
+    vscsi_put_req(req);
 }
 
 static void vscsi_process_login(VSCSIState *s, vscsi_req *req)
@@ -659,11 +646,11 @@  static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
     }
 
     req->lun = lun;
-    req->sreq = scsi_req_new(sdev, req->qtag, lun);
+    req->sreq = scsi_req_new(sdev, lun, req);
     n = scsi_req_enqueue(req->sreq, srp->cmd.cdb);
 
-    dprintf("VSCSI: Queued command tag 0x%x CMD 0x%x ID %d LUN %d ret: %d\n",
-            req->qtag, srp->cmd.cdb[0], id, lun, n);
+    dprintf("VSCSI: Queued command 0x%p CMD 0x%x ID %d LUN %d ret: %d\n",
+            req, srp->cmd.cdb[0], id, lun, n);
 
     if (n) {
         /* Transfer direction must be set before preprocessing the
@@ -858,7 +845,7 @@  static void vscsi_got_payload(VSCSIState *s, vscsi_crq *crq)
     }
 
     if (done) {
-        vscsi_put_req(s, req);
+        vscsi_put_req(req);
     }
 }
 
@@ -935,11 +922,7 @@  static int spapr_vscsi_init(VIOsPAPRDevice *dev)
 
     dbg_vscsi_state = s;
 
-    /* Initialize qemu request tags */
     memset(s->reqs, 0, sizeof(s->reqs));
-    for (i = 0; i < VSCSI_REQ_LIMIT; i++) {
-        s->reqs[i].qtag = i;
-    }
 
     dev->crq.SendFunc = vscsi_do_crq;
 
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 86582cc..4e2ea03 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -216,8 +216,8 @@  static void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
     MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
     USBPacket *p = s->packet;
 
-    if (req->tag != s->tag) {
-        fprintf(stderr, "usb-msd: Unexpected SCSI Tag 0x%x\n", req->tag);
+    if (req->hba_private != s) {
+        fprintf(stderr, "usb-msd: Unexpected SCSI command 0x%p\n", req);
     }
 
     assert((s->mode == USB_MSDM_DATAOUT) == (req->cmd.mode == SCSI_XFER_TO_DEV));
@@ -241,8 +241,8 @@  static void usb_msd_command_complete(SCSIRequest *req, uint32_t status)
     MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
     USBPacket *p = s->packet;
 
-    if (req->tag != s->tag) {
-        fprintf(stderr, "usb-msd: Unexpected SCSI Tag 0x%x\n", req->tag);
+    if (req->hba_private != s) {
+        fprintf(stderr, "usb-msd: Unexpected SCSI command 0x%p\n", req);
     }
     DPRINTF("Command complete %d\n", status);
     s->residue = s->data_len;
@@ -387,7 +387,7 @@  static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
                     s->tag, cbw.flags, cbw.cmd_len, s->data_len);
             s->residue = 0;
             s->scsi_len = 0;
-            s->req = scsi_req_new(s->scsi_dev, s->tag, 0);
+            s->req = scsi_req_new(s->scsi_dev, 0, s);
             scsi_req_enqueue(s->req, cbw.cmd);
             /* ??? Should check that USB and SCSI data transfer
                directions match.  */
diff --git a/trace-events b/trace-events
index bebf612..8c52148 100644
--- a/trace-events
+++ b/trace-events
@@ -226,13 +226,13 @@  disable usb_clear_device_feature(int addr, int feature, int ret) "dev %d, featur
 disable usb_set_device_feature(int addr, int feature, int ret) "dev %d, feature %d, ret %d"
 
 # hw/scsi-bus.c
-disable scsi_req_alloc(int target, int lun, int tag) "target %d lun %d tag %d"
-disable scsi_req_data(int target, int lun, int tag, int len) "target %d lun %d tag %d len %d"
-disable scsi_req_dequeue(int target, int lun, int tag) "target %d lun %d tag %d"
-disable scsi_req_continue(int target, int lun, int tag) "target %d lun %d tag %d"
-disable scsi_req_parsed(int target, int lun, int tag, int cmd, int mode, int xfer) "target %d lun %d tag %d command %d dir %d length %d"
-disable scsi_req_parsed_lba(int target, int lun, int tag, int cmd, uint64_t lba) "target %d lun %d tag %d command %d lba %"PRIu64""
-disable scsi_req_parse_bad(int target, int lun, int tag, int cmd) "target %d lun %d tag %d command %d"
+disable scsi_req_alloc(int target, int lun, void *private) "target %d lun %d private %p"
+disable scsi_req_data(int target, int lun, void *private, int len) "target %d lun %d private %p len %d"
+disable scsi_req_dequeue(int target, int lun, void *private) "target %d lun %d p %p"
+disable scsi_req_continue(int target, int lun, void *private) "target %d lun %d private %p"
+disable scsi_req_parsed(int target, int lun, void *private, int cmd, int mode, int xfer) "target %d lun %d private %p command %d dir %d length %d"
+disable scsi_req_parsed_lba(int target, int lun, void *private, int cmd, uint64_t lba) "target %d lun %d private %p command %d lba %"PRIu64""
+disable scsi_req_parse_bad(int target, int lun, void *private, int cmd) "target %d lun %d private %p command %d"
 
 # vl.c
 disable vm_state_notify(int running, int reason) "running %d reason %d"