* [Qemu-devel] [PATCH v2] scsi-bus: fix to allow some special SCSI commands
@ 2014-07-12 10:21 TAMUKI Shoichi
2014-07-15 17:05 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: TAMUKI Shoichi @ 2014-07-12 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, TAMUKI Shoichi
Currently, some special SCSI commands sent from the initiator in a
guest do not reach the target device. To avoid this, extended (0x7e,)
variable length (0x7f,) and vendor specific (0xc0..0xff) opcodes are
now treated as valid CDBs.
Originally, the most significant 3 bits of a SCSI opcode specified the
length of the CDB. However, when variable-length CDBs were created,
this correspondence was changed, and the entire opcode must be
examined to determine the CDB length. The CDBs with the opcodes above
are done that way for now.
Signed-off-by: TAMUKI Shoichi <tamuki@linet.gr.jp>
---
v2: add a new argument to scsi_req_new(), and modify all invocations
in hw/{scsi,usb}, since this function is not called only for virtio-
scsi.
hw/scsi/esp.c | 20 ++++++++++----------
hw/scsi/lsi53c895a.c | 2 +-
hw/scsi/megasas.c | 12 +++++++-----
hw/scsi/scsi-bus.c | 26 ++++++++++++++++++++------
hw/scsi/spapr_vscsi.c | 3 ++-
hw/scsi/virtio-scsi.c | 2 +-
hw/scsi/vmw_pvscsi.c | 3 ++-
hw/usb/dev-storage.c | 3 ++-
hw/usb/dev-uas.c | 3 ++-
include/hw/scsi/scsi.h | 2 +-
10 files changed, 48 insertions(+), 28 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 5ab44d8..ad58d84 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -120,7 +120,7 @@ static uint32_t get_cmd(ESPState *s, uint8_t *buf)
return dmalen;
}
-static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
+static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid, uint32_t len)
{
int32_t datalen;
int lun;
@@ -129,7 +129,7 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
trace_esp_do_busid_cmd(busid);
lun = busid & 7;
current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
- s->current_req = scsi_req_new(current_lun, 0, lun, buf, s);
+ s->current_req = scsi_req_new(current_lun, 0, lun, buf, s, len);
datalen = scsi_req_enqueue(s->current_req);
s->ti_size = datalen;
if (datalen != 0) {
@@ -148,17 +148,17 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
esp_raise_irq(s);
}
-static void do_cmd(ESPState *s, uint8_t *buf)
+static void do_cmd(ESPState *s, uint8_t *buf, uint32_t len)
{
uint8_t busid = buf[0];
- do_busid_cmd(s, &buf[1], busid);
+ do_busid_cmd(s, &buf[1], busid, len);
}
static void handle_satn(ESPState *s)
{
uint8_t buf[32];
- int len;
+ uint32_t len;
if (s->dma && !s->dma_enabled) {
s->dma_cb = handle_satn;
@@ -166,13 +166,13 @@ static void handle_satn(ESPState *s)
}
len = get_cmd(s, buf);
if (len)
- do_cmd(s, buf);
+ do_cmd(s, buf, len);
}
static void handle_s_without_atn(ESPState *s)
{
uint8_t buf[32];
- int len;
+ uint32_t len;
if (s->dma && !s->dma_enabled) {
s->dma_cb = handle_s_without_atn;
@@ -180,7 +180,7 @@ static void handle_s_without_atn(ESPState *s)
}
len = get_cmd(s, buf);
if (len) {
- do_busid_cmd(s, buf, 0);
+ do_busid_cmd(s, buf, 0, len);
}
}
@@ -245,7 +245,7 @@ static void esp_do_dma(ESPState *s)
s->ti_size = 0;
s->cmdlen = 0;
s->do_cmd = 0;
- do_cmd(s, s->cmdbuf);
+ do_cmd(s, s->cmdbuf, len);
return;
}
if (s->async_len == 0) {
@@ -355,7 +355,7 @@ static void handle_ti(ESPState *s)
s->ti_size = 0;
s->cmdlen = 0;
s->do_cmd = 0;
- do_cmd(s, s->cmdbuf);
+ do_cmd(s, s->cmdbuf, minlen);
return;
}
}
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 786d848..1609721 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -783,7 +783,7 @@ static void lsi_do_command(LSIState *s)
s->current = g_malloc0(sizeof(lsi_request));
s->current->tag = s->select_tag;
s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun, buf,
- s->current);
+ s->current, s->dbc);
n = scsi_req_enqueue(s->current->req);
if (n) {
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index c68a873..60ce4d1 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -990,7 +990,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));
- req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd);
+ req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd, 6);
if (!req) {
trace_megasas_dcmd_req_alloc_failed(cmd->index,
"PD get info std inquiry");
@@ -1008,7 +1008,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));
- req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd);
+ req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd, 6);
if (!req) {
trace_megasas_dcmd_req_alloc_failed(cmd->index,
"PD get info vpd inquiry");
@@ -1153,7 +1153,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
memset(cmd->iov_buf, 0x0, dcmd_size);
info = cmd->iov_buf;
megasas_setup_inquiry(cdb, 0x83, sizeof(info->vpd_page83));
- req = scsi_req_new(sdev, cmd->index, lun, cdb, cmd);
+ req = scsi_req_new(sdev, cmd->index, lun, cdb, cmd, 6);
if (!req) {
trace_megasas_dcmd_req_alloc_failed(cmd->index,
"LD get info vpd inquiry");
@@ -1610,7 +1610,8 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
}
cmd->req = scsi_req_new(sdev, cmd->index,
- cmd->frame->header.lun_id, cdb, cmd);
+ cmd->frame->header.lun_id, cdb, cmd,
+ cmd->frame->header.cdb_len);
if (!cmd->req) {
trace_megasas_scsi_req_alloc_failed(
mfi_frame_desc[cmd->frame->header.frame_cmd],
@@ -1687,7 +1688,8 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd)
megasas_encode_lba(cdb, lba_start, lba_count, is_write);
cmd->req = scsi_req_new(sdev, cmd->index,
- cmd->frame->header.lun_id, cdb, cmd);
+ cmd->frame->header.lun_id, cdb, cmd,
+ cmd->frame->header.cdb_len);
if (!cmd->req) {
trace_megasas_scsi_req_alloc_failed(
mfi_frame_desc[cmd->frame->header.frame_cmd],
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index ea1ac09..d039b6d 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -9,7 +9,8 @@
static char *scsibus_get_dev_path(DeviceState *dev);
static char *scsibus_get_fw_dev_path(DeviceState *dev);
-static int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf);
+static int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf,
+ uint32_t len);
static void scsi_req_dequeue(SCSIRequest *req);
static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len);
static void scsi_target_free_buf(SCSIRequest *req);
@@ -557,13 +558,13 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
}
SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
- uint8_t *buf, void *hba_private)
+ uint8_t *buf, void *hba_private, uint32_t len)
{
SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
SCSIRequest *req;
SCSICommand cmd;
- if (scsi_req_parse(&cmd, d, buf) != 0) {
+ if (scsi_req_parse(&cmd, d, buf, len) != 0) {
trace_scsi_req_parse_bad(d->id, lun, tag, buf[0]);
req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, lun, hba_private);
} else {
@@ -1181,7 +1182,8 @@ static uint64_t scsi_cmd_lba(SCSICommand *cmd)
return lba;
}
-static int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+static int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf,
+ uint32_t len)
{
int rc;
@@ -1193,12 +1195,23 @@ static int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
case 2:
cmd->len = 10;
break;
+ case 3:
+ if (buf[0] == 0x7e || (buf[0] == 0x7f && len >= 16)) {
+ cmd->len = len;
+ } else {
+ return -1;
+ }
+ break;
case 4:
cmd->len = 16;
break;
case 5:
cmd->len = 12;
break;
+ case 6:
+ case 7:
+ cmd->len = len;
+ break;
default:
return -1;
}
@@ -1861,14 +1874,15 @@ 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];
+ int len;
uint32_t tag;
uint32_t lun;
SCSIRequest *req;
- qemu_get_buffer(f, buf, sizeof(buf));
+ len = qemu_get_buffer(f, buf, sizeof(buf));
qemu_get_be32s(f, &tag);
qemu_get_be32s(f, &lun);
- req = scsi_req_new(s, tag, lun, buf, NULL);
+ req = scsi_req_new(s, tag, lun, buf, NULL, len);
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 048cfc7..9151bf0 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -796,7 +796,8 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
} return 1;
}
- req->sreq = scsi_req_new(sdev, req->qtag, lun, srp->cmd.cdb, req);
+ req->sreq = scsi_req_new(sdev, req->qtag, lun, srp->cmd.cdb, req,
+ 16 + srp->cmd.add_cdb_len);
n = scsi_req_enqueue(req->sreq);
DPRINTF("VSCSI: Queued command tag 0x%x CMD 0x%x=%s LUN %d ret: %d\n",
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 0eb069a..2de9b28 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -466,7 +466,7 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
}
req->sreq = scsi_req_new(d, req->req.cmd.tag,
virtio_scsi_get_lun(req->req.cmd.lun),
- req->req.cdb, req);
+ req->req.cdb, req, vs->cdb_size);
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 f9ed926..4925073 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -649,7 +649,8 @@ pvscsi_process_request_descriptor(PVSCSIState *s,
r->sg.elemAddr = descr->dataAddr;
}
- r->sreq = scsi_req_new(d, descr->context, r->lun, descr->cdb, r);
+ r->sreq = scsi_req_new(d, descr->context, r->lun, descr->cdb, r,
+ descr->cdbLen);
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 ae4efcb..a930d6a 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -437,7 +437,8 @@ static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
tag, cbw.flags, 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, NULL);
+ s->req = scsi_req_new(scsi_dev, tag, cbw.lun, cbw.cmd, NULL,
+ cbw.cmd_len);
#ifdef DEBUG_MSD
scsi_req_print(s->req);
#endif
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 9832385..c6cb61b 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -714,7 +714,8 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
req->req = scsi_req_new(req->dev, req->tag,
usb_uas_get_lun(req->lun),
- iu->command.cdb, req);
+ iu->command.cdb, req,
+ 16 + iu->command.add_cdb_length);
if (uas->requestlog) {
scsi_req_print(req->req);
}
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 1adb549..76365c2 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -238,7 +238,7 @@ int scsi_build_sense(uint8_t *in_buf, int in_len,
SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
uint32_t tag, uint32_t lun, void *hba_private);
SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
- uint8_t *buf, void *hba_private);
+ uint8_t *buf, void *hba_private, uint32_t len);
int32_t scsi_req_enqueue(SCSIRequest *req);
void scsi_req_free(SCSIRequest *req);
SCSIRequest *scsi_req_ref(SCSIRequest *req);
--
1.9.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v2] scsi-bus: fix to allow some special SCSI commands
2014-07-12 10:21 [Qemu-devel] [PATCH v2] scsi-bus: fix to allow some special SCSI commands TAMUKI Shoichi
@ 2014-07-15 17:05 ` Paolo Bonzini
2014-07-15 21:20 ` TAMUKI Shoichi
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2014-07-15 17:05 UTC (permalink / raw)
To: TAMUKI Shoichi, qemu-devel
Il 12/07/2014 12:21, TAMUKI Shoichi ha scritto:
> Currently, some special SCSI commands sent from the initiator in a
> guest do not reach the target device. To avoid this, extended (0x7e,)
> variable length (0x7f,) and vendor specific (0xc0..0xff) opcodes are
> now treated as valid CDBs.
>
> Originally, the most significant 3 bits of a SCSI opcode specified the
> length of the CDB. However, when variable-length CDBs were created,
> this correspondence was changed, and the entire opcode must be
> examined to determine the CDB length. The CDBs with the opcodes above
> are done that way for now.
>
> Signed-off-by: TAMUKI Shoichi <tamuki@linet.gr.jp>
> ---
> v2: add a new argument to scsi_req_new(), and modify all invocations
> in hw/{scsi,usb}, since this function is not called only for virtio-
> scsi.
I think that for scsi-generic it is harmless to pass extra bytes at the
end of the CDB, and QEMU right now does not support more than 16 bytes
for the CDB (see SCSI_CMD_BUF_SIZE in include/hw/scsi/scsi.h).
Assuming 16-byte commands are enough, does this patch work for you?
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 4341754..51e4f37 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1194,6 +1194,9 @@
case 2:
cmd->len = 10;
break;
+ case 3:
+ cmd->len = SCSI_CMD_BUF_SIZE;
+ break;
case 4:
cmd->len = 16;
break;
You will probably also need to pass the transfer length and direction
down from the device model to scsi-generic.c. Effectively you will be
ignoring cmd->xfer and cmd->mode if the host device can provide them if
the first byte in the cdb identifies a vendor-specific command. You can
add a callback to SCSIBusInfo, and call it from scsi_req_parse; for
virtio-scsi the callback could look something like this:
int virtio_scsi_parse_req(SCSICommand *cmd, void *hba_private)
{
VirtIOSCSIReq *req = hba_private;
cmd->xfer = req->qsgl.size;
if (cmd->xfer == 0) {
cmd->mode = SCSI_XFER_NONE;
} else if (iov_size(req->elem._sg, req->elem.in_num)
> req->resp_size)) {
cmd->mode = SCSI_XFER_FROM_DEV;
} else {
cmd->mode = SCSI_XFER_TO_DEV;
}
}
I'll try to prepare a complete patch tomorrow, but I would like to
understand your actual requirements for the CDB length.
Paolo
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v2] scsi-bus: fix to allow some special SCSI commands
2014-07-15 17:05 ` Paolo Bonzini
@ 2014-07-15 21:20 ` TAMUKI Shoichi
0 siblings, 0 replies; 3+ messages in thread
From: TAMUKI Shoichi @ 2014-07-15 21:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, TAMUKI Shoichi
Hello,
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v2] scsi-bus: fix to allow some special SCSI commands
Date: Tue, 15 Jul 2014 19:05:25 +0200
> > Currently, some special SCSI commands sent from the initiator in a
> > guest do not reach the target device. To avoid this, extended (0x7e,)
> > variable length (0x7f,) and vendor specific (0xc0..0xff) opcodes are
> > now treated as valid CDBs.
> >
> > Originally, the most significant 3 bits of a SCSI opcode specified the
> > length of the CDB. However, when variable-length CDBs were created,
> > this correspondence was changed, and the entire opcode must be
> > examined to determine the CDB length. The CDBs with the opcodes above
> > are done that way for now.
> >
> > Signed-off-by: TAMUKI Shoichi <tamuki@linet.gr.jp>
> > ---
> > v2: add a new argument to scsi_req_new(), and modify all invocations
> > in hw/{scsi,usb}, since this function is not called only for virtio-
> > scsi.
>
> I think that for scsi-generic it is harmless to pass extra bytes at the
> end of the CDB, and QEMU right now does not support more than 16 bytes
> for the CDB (see SCSI_CMD_BUF_SIZE in include/hw/scsi/scsi.h).
>
> Assuming 16-byte commands are enough, does this patch work for you?
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 4341754..51e4f37 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1194,6 +1194,9 @@
> case 2:
> cmd->len = 10;
> break;
> + case 3:
> + cmd->len = SCSI_CMD_BUF_SIZE;
> + break;
> case 4:
> cmd->len = 16;
> break;
Yes, it is ok for me since I currently care neither 0x7e extended
opcode nor 0x7f variable length opcode.
> You will probably also need to pass the transfer length and direction
> down from the device model to scsi-generic.c. Effectively you will be
> ignoring cmd->xfer and cmd->mode if the host device can provide them if
> the first byte in the cdb identifies a vendor-specific command. You can
> add a callback to SCSIBusInfo, and call it from scsi_req_parse; for
> virtio-scsi the callback could look something like this:
>
> int virtio_scsi_parse_req(SCSICommand *cmd, void *hba_private)
> {
> VirtIOSCSIReq *req = hba_private;
>
> cmd->xfer = req->qsgl.size;
> if (cmd->xfer == 0) {
> cmd->mode = SCSI_XFER_NONE;
> } else if (iov_size(req->elem._sg, req->elem.in_num)
> > req->resp_size)) {
> cmd->mode = SCSI_XFER_FROM_DEV;
> } else {
> cmd->mode = SCSI_XFER_TO_DEV;
> }
> }
Thank you for pointing that out.
> I'll try to prepare a complete patch tomorrow, but I would like to
> understand your actual requirements for the CDB length.
I am very glad to hear that. My actual requirement is to pass vendor-
specific commands and their CDB length is less than 16 bytes.
Thanks for your cooperation.
Regards,
TAMUKI Shoichi
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-07-15 21:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-12 10:21 [Qemu-devel] [PATCH v2] scsi-bus: fix to allow some special SCSI commands TAMUKI Shoichi
2014-07-15 17:05 ` Paolo Bonzini
2014-07-15 21:20 ` TAMUKI Shoichi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).