Re: [PATCH 1/2] scsi: Add buf_len parameter to scsi_req_new()

[John Millikin <john@john-millikin.com>]

Thank you for the suggestions for CDB sizes! Especially the tricky ones
in spapr_vscsi.c and dev-uas.c.

v2: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02997.html

On Fri, Aug 19, 2022 at 06:06:13PM +0200, Paolo Bonzini wrote:
> On 8/17/22 07:34, John Millikin wrote:
> > The sigil SCSI_CMD_BUF_LEN_TODO() is used to indicate that the buffer
> > length calculation is TODO it should be replaced by a better value,
> > such as the length of a successful DMA read.
> 
> Let's just do it right:
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index e8a4a705e7..05a43ec807 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -864,7 +864,7 @@ static void lsi_do_command(LSIState *s)
>      s->current = g_new0(lsi_request, 1);
>      s->current->tag = s->select_tag;
>      s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun, buf,
> -                                   SCSI_CMD_BUF_LEN_TODO(s->dbc), s->current);
> +                                   s->dbc, s->current);
>      n = scsi_req_enqueue(s->current->req);
>      if (n) {
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index e887ae8adb..842edc3f9d 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1053,7 +1053,6 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
>      uint64_t pd_size;
>      uint16_t pd_id = ((sdev->id & 0xFF) << 8) | (lun & 0xFF);
>      uint8_t cmdbuf[6];
> -    size_t cmdbuf_len = SCSI_CMD_BUF_LEN_TODO(sizeof(cmdbuf));
>      size_t len;
>      dma_addr_t residual;
> @@ -1063,7 +1062,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
>          info->inquiry_data[0] = 0x7f; /* Force PQual 0x3, PType 0x1f */
>          info->vpd_page83[0] = 0x7f;
>          megasas_setup_inquiry(cmdbuf, 0, sizeof(info->inquiry_data));
> -        cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmdbuf_len, cmd);
> +        cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, sizeof(cmdbuf), cmd);
>          if (!cmd->req) {
>              trace_megasas_dcmd_req_alloc_failed(cmd->index,
>                                                  "PD get info std inquiry");
> @@ -1081,7 +1080,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
>          return MFI_STAT_INVALID_STATUS;
>      } else if (info->inquiry_data[0] != 0x7f && info->vpd_page83[0] == 0x7f) {
>          megasas_setup_inquiry(cmdbuf, 0x83, sizeof(info->vpd_page83));
> -        cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmdbuf_len, cmd);
> +        cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, sizeof(cmdbuf), cmd);
>          if (!cmd->req) {
>              trace_megasas_dcmd_req_alloc_failed(cmd->index,
>                                                  "PD get info vpd inquiry");
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index 711613b5b1..a90c2546f1 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -324,8 +324,8 @@ static int mptsas_process_scsi_io_request(MPTSASState *s,
>      }
>      req->sreq = scsi_req_new(sdev, scsi_io->MsgContext,
> -                            scsi_io->LUN[1], scsi_io->CDB,
> -                            SCSI_CMD_BUF_LEN_TODO(SIZE_MAX), req);
> +                             scsi_io->LUN[1], scsi_io->CDB,
> +                             scsi_io->CDBLength, req);
>      if (req->sreq->cmd.xfer > scsi_io->DataLength) {
>          goto overrun;
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 288ea12969..cc71524024 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1707,7 +1707,6 @@ static int get_scsi_requests(QEMUFile *f, void *pv, size_t size,
>      while ((sbyte = qemu_get_sbyte(f)) > 0) {
>          uint8_t buf[SCSI_CMD_BUF_SIZE];
> -        size_t buf_len;
>          uint32_t tag;
>          uint32_t lun;
>          SCSIRequest *req;
> @@ -1715,8 +1714,11 @@ static int get_scsi_requests(QEMUFile *f, void *pv, size_t size,
>          qemu_get_buffer(f, buf, sizeof(buf));
>          qemu_get_be32s(f, &tag);
>          qemu_get_be32s(f, &lun);
> -        buf_len = SCSI_CMD_BUF_LEN_TODO(sizeof(buf));
> -        req = scsi_req_new(s, tag, lun, buf, buf_len, NULL);
> +        /*
> +         * A too-short CDB would have been rejected by scsi_req_new, so just use
> +         * SCSI_CMD_BUF_SIZE as the CDB length.
> +         */
> +        req = scsi_req_new(s, tag, lun, buf, sizeof(buf), NULL);
>          req->retry = (sbyte == 1);
>          if (bus->info->load_request) {
>              req->hba_private = bus->info->load_request(f, req);
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index c17bb47f7b..0a8cbf5a4b 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -783,7 +783,7 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
>      union srp_iu *srp = &req_iu(req)->srp;
>      SCSIDevice *sdev;
>      int n, lun;
> -    size_t cdb_len = SCSI_CMD_BUF_LEN_TODO(SIZE_MAX);
> +    size_t cdb_len = sizeof (srp->cmd.cdb) + (srp->cmd.add_cdb_len & ~3);
>      if ((srp->cmd.lun == 0 || be64_to_cpu(srp->cmd.lun) == SRP_REPORT_LUNS_WLUN)
>        && srp->cmd.cdb[0] == REPORT_LUNS) {
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 632e3aa58f..41f2a56301 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -673,7 +673,6 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
>      VirtIOSCSICommon *vs = &s->parent_obj;
>      SCSIDevice *d;
>      int rc;
> -    size_t cdb_len = SCSI_CMD_BUF_LEN_TODO(SIZE_MAX);
>      rc = virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
>                                 sizeof(VirtIOSCSICmdResp) + vs->sense_size);
> @@ -698,7 +697,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
>      virtio_scsi_ctx_check(s, d);
>      req->sreq = scsi_req_new(d, req->req.cmd.tag,
>                               virtio_scsi_get_lun(req->req.cmd.lun),
> -                             req->req.cmd.cdb, cdb_len, req);
> +                             req->req.cmd.cdb, vs->cdb_size, req);
>      if (req->sreq->cmd.mode != SCSI_XFER_NONE
>          && (req->sreq->cmd.mode != req->mode ||
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index f845c88378..91e2f858ab 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -716,7 +716,6 @@ pvscsi_process_request_descriptor(PVSCSIState *s,
>      SCSIDevice *d;
>      PVSCSIRequest *r = pvscsi_queue_pending_descriptor(s, &d, descr);
>      int64_t n;
> -    size_t cdb_len = SCSI_CMD_BUF_LEN_TODO(SIZE_MAX);
>      trace_pvscsi_process_req_descr(descr->cdb[0], descr->context);
> @@ -731,7 +730,7 @@ pvscsi_process_request_descriptor(PVSCSIState *s,
>          r->sg.elemAddr = descr->dataAddr;
>      }
> -    r->sreq = scsi_req_new(d, descr->context, r->lun, descr->cdb, cdb_len, r);
> +    r->sreq = scsi_req_new(d, descr->context, r->lun, descr->cdb, descr->cdbLen, r);
>      if (r->sreq->cmd.mode == SCSI_XFER_FROM_DEV &&
>          (descr->flags & PVSCSI_FLAG_CMD_DIR_TODEVICE)) {
>          r->cmp.hostStatus = BTSTAT_BADMSG;
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 353e129fac..98639696e6 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -379,7 +379,6 @@ static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
>      uint8_t devep = p->ep->nr;
>      SCSIDevice *scsi_dev;
>      uint32_t len;
> -    size_t cdb_len = SCSI_CMD_BUF_LEN_TODO(SIZE_MAX);
>      switch (p->pid) {
>      case USB_TOKEN_OUT:
> @@ -416,7 +415,7 @@ static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
>                                       cbw.cmd_len, s->data_len);
>              assert(le32_to_cpu(s->csw.residue) == 0);
>              s->scsi_len = 0;
> -            s->req = scsi_req_new(scsi_dev, tag, cbw.lun, cbw.cmd, cdb_len, NULL);
> +            s->req = scsi_req_new(scsi_dev, tag, cbw.lun, cbw.cmd, cbw.cmd_len, NULL);
>              if (s->commandlog) {
>                  scsi_req_print(s->req);
>              }
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> index 768df8958c..5192b062d6 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -71,7 +71,7 @@ typedef struct {
>      uint8_t    reserved_2;
>      uint64_t   lun;
>      uint8_t    cdb[16];
> -    uint8_t    add_cdb[1];      /* not supported by QEMU */
> +    uint8_t    add_cdb[1];
>  } QEMU_PACKED  uas_iu_command;
>  typedef struct {
> @@ -699,7 +699,7 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
>      UASRequest *req;
>      uint32_t len;
>      uint16_t tag = be16_to_cpu(iu->hdr.tag);
> -    size_t cdb_len = SCSI_CMD_BUF_LEN_TODO(SIZE_MAX);
> +    size_t cdb_len = sizeof(iu->command.cdb) + iu->command.add_cdb_length;
>      if (iu->command.add_cdb_length > 0) {
>          qemu_log_mask(LOG_UNIMP, "additional adb length not yet supported\n");
> diff --git a/include/scsi/utils.h b/include/scsi/utils.h
> index 1a36d25ee4..d5c8efa16e 100644
> --- a/include/scsi/utils.h
> +++ b/include/scsi/utils.h
> @@ -144,10 +144,4 @@ int scsi_cdb_length(uint8_t *buf);
>  int scsi_sense_from_errno(int errno_value, SCSISense *sense);
>  int scsi_sense_from_host_status(uint8_t host_status, SCSISense *sense);
> -/**
> - * Annotates places where a SCSI command buffer is passed to scsi_req_new()
> - * but haven't yet been updated to pass the buffer's true (populated) length.
> - */
> -#define SCSI_CMD_BUF_LEN_TODO(guess) guess
> -
>  #endif
> 
> (or something like that).
> 
> paolo
>