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

[Nicholas A. Bellinger]

On Tue, 2010-06-15 at 17:16 +0200, Hannes Reinecke wrote:
> 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(-)
> 

Makes sense to me, commited.

--nab

> 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);