Re: [Qemu-devel] [PATCH 05/21] scsi: Add 'hba_private' to SCSIRequest

[Anthony Liguori <anthony@codemonkey.ws>]

On 07/19/2011 05:15 AM, Kevin Wolf wrote:
> From: Hannes Reinecke<hare@suse.de>
>
> '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.
> 'tag' is still being kept for tracing purposes.
>
> Signed-off-by: Hannes Reinecke<hare@suse.de>
> Acked-by: Paolo Bonzini<pbonzini@redhat.com>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
>   hw/esp.c          |    2 +-
>   hw/lsi53c895a.c   |   22 ++++++++--------------
>   hw/scsi-bus.c     |    9 ++++++---
>   hw/scsi-disk.c    |    4 ++--
>   hw/scsi-generic.c |    5 +++--
>   hw/scsi.h         |   10 +++++++---
>   hw/spapr_vscsi.c  |   29 +++++++++--------------------
>   hw/usb-msd.c      |    9 +--------
>   8 files changed, 37 insertions(+), 53 deletions(-)
>
> diff --git a/hw/esp.c b/hw/esp.c
> index aa50800..9ddd637 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, 0, lun, NULL);
>       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..69eec1d 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -661,7 +661,7 @@ static lsi_request *lsi_find_by_tag(LSIState *s, uint32_t tag)
>   static void lsi_request_cancelled(SCSIRequest *req)
>   {
>       LSIState *s = DO_UPCAST(LSIState, dev.qdev, req->bus->qbus.parent);
> -    lsi_request *p;
> +    lsi_request *p = req->hba_private;
>
>       if (s->current&&  req == s->current->req) {
>           scsi_req_unref(req);
> @@ -670,7 +670,6 @@ static void lsi_request_cancelled(SCSIRequest *req)
>           return;
>       }
>
> -    p = lsi_find_by_tag(s, req->tag);
>       if (p) {
>           QTAILQ_REMOVE(&s->queue, p, next);
>           scsi_req_unref(req);
> @@ -680,18 +679,12 @@ 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;
> -
> -    p = lsi_find_by_tag(s, tag);
> -    if (!p) {
> -        BADF("IO with unknown tag %d\n", tag);
> -        return 1;
> -    }
> +    lsi_request *p = req->hba_private;
>
>       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 +736,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 +782,8 @@ 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->tag, 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..8b1a412 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -131,7 +131,8 @@ 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 tag,
> +                            uint32_t lun, void *hba_private)
>   {
>       SCSIRequest *req;
>
> @@ -141,14 +142,16 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
>       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);
>       return req;
>   }
>
> -SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun)
> +SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
> +                          void *hba_private)
>   {
> -    return d->info->alloc_req(d, tag, lun);
> +    return d->info->alloc_req(d, tag, lun, hba_private);
>   }
>
>   uint8_t *scsi_req_get_buf(SCSIRequest *req)
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index a8c7372..c2a99fe 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -81,13 +81,13 @@ 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)
> +                                     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, tag, 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..90345a7 100644
> --- a/hw/scsi-generic.c
> +++ b/hw/scsi-generic.c
> @@ -96,11 +96,12 @@ 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 tag, uint32_t lun,
> +                                     void *hba_private)
>   {
>       SCSIRequest *req;
>
> -    req = scsi_req_alloc(sizeof(SCSIGenericReq), d, tag, lun);
> +    req = scsi_req_alloc(sizeof(SCSIGenericReq), d, tag, lun, hba_private);
>       return req;
>   }
>
> diff --git a/hw/scsi.h b/hw/scsi.h
> index c1dca35..6b15bbc 100644
> --- a/hw/scsi.h
> +++ b/hw/scsi.h
> @@ -43,6 +43,7 @@ struct SCSIRequest {
>       } cmd;
>       BlockDriverAIOCB  *aiocb;
>       bool enqueued;
> +    void *hba_private;
>       QTAILQ_ENTRY(SCSIRequest) next;
>   };
>
> @@ -67,7 +68,8 @@ 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 tag, 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 +140,10 @@ 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 tag,
> +                            uint32_t lun, void *hba_private);
> +SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, 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..9e1cb2e 100644
> --- a/hw/spapr_vscsi.c
> +++ b/hw/spapr_vscsi.c
> @@ -121,7 +121,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);

This breaks the build:

make[1]: Nothing to be done for `all'.
   CC    ppc64-softmmu/spapr_vscsi.o
/home/anthony/git/qemu/hw/spapr_vscsi.c: In function 
‘vscsi_command_complete’:
/home/anthony/git/qemu/hw/spapr_vscsi.c:535:34: error: ‘s’ undeclared 
(first use in this function)
/home/anthony/git/qemu/hw/spapr_vscsi.c:535:34: note: each undeclared 
identifier is reported only once for each function it appears in

This file is only built when libfdt is installed which is probably why 
you didn't catch it.

Ben/David, is there a way we can still build most of this stuff without 
libfdt?  libfdt is still not commonly packaged by some distros.

Regards,

Anthony Liguori