* [Qemu-devel] [PATCH v3 01/21] scsi: add tracing of scsi requests
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 15:49 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 02/21] scsi-generic: Remove bogus double complete Paolo Bonzini
` (20 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-bus.c | 6 ++++++
trace-events | 6 ++++++
2 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index ceeb4ec..0fd85fc 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -4,6 +4,7 @@
#include "scsi-defs.h"
#include "qdev.h"
#include "blockdev.h"
+#include "trace.h"
static char *scsibus_get_fw_dev_path(DeviceState *dev);
@@ -141,6 +142,7 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
req->lun = lun;
req->status = -1;
req->enqueued = true;
+ trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
QTAILQ_INSERT_TAIL(&d->requests, req, next);
return req;
}
@@ -159,6 +161,7 @@ SCSIRequest *scsi_req_find(SCSIDevice *d, uint32_t tag)
static void scsi_req_dequeue(SCSIRequest *req)
{
+ trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag);
if (req->enqueued) {
QTAILQ_REMOVE(&req->dev->requests, req, next);
req->enqueued = false;
@@ -195,6 +198,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]);
return -1;
}
@@ -392,6 +396,8 @@ 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],
+ req->cmd.mode, req->cmd.xfer, req->cmd.lba);
return 0;
}
diff --git a/trace-events b/trace-events
index a00b63c..55f89b4 100644
--- a/trace-events
+++ b/trace-events
@@ -205,6 +205,12 @@ disable usb_set_config(int addr, int config, int ret) "dev %d, config %d, ret %d
disable usb_clear_device_feature(int addr, int feature, int ret) "dev %d, feature %d, ret %d"
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_dequeue(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, uint64_t lba) "target %d lun %d tag %d command %d dir %d length %d lba %"PRIu64""
+disable scsi_req_parse_bad(int target, int lun, int tag, int cmd) "target %d lun %d tag %d command %d"
+
# vl.c
disable vm_state_notify(int running, int reason) "running %d reason %d"
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 02/21] scsi-generic: Remove bogus double complete
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 01/21] scsi: add tracing of scsi requests Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 15:50 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 03/21] scsi: introduce scsi_req_data Paolo Bonzini
` (19 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson
scsi-generic scsi_read_complete() should not -both- call the client
complete callback with SCSI_REASON_DATA -and- call
scsi_command_complete(). The former will cause the client to queue a
new read or write request, while the later will free the request data
structure, thus causing the new read or write request to use a
freed/stale structure when it completes.
This patch fixes the bug, fixing a crash with scsi-generic & RHEL5.5
installer.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-generic.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 9be1cca..102f1da 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -172,9 +172,11 @@ static void scsi_read_complete(void * opaque, int ret)
DPRINTF("Data ready tag=0x%x len=%d\n", r->req.tag, len);
r->len = -1;
- r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);
- if (len == 0)
+ if (len == 0) {
scsi_command_complete(r, 0);
+ } else {
+ r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);
+ }
}
/* Read more data from scsi device into buffer. */
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 02/21] scsi-generic: Remove bogus double complete
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 02/21] scsi-generic: Remove bogus double complete Paolo Bonzini
@ 2011-05-20 15:50 ` Christoph Hellwig
0 siblings, 0 replies; 52+ messages in thread
From: Christoph Hellwig @ 2011-05-20 15:50 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, David Gibson
On Tue, May 17, 2011 at 01:00:48PM +0200, Paolo Bonzini wrote:
> scsi-generic scsi_read_complete() should not -both- call the client
> complete callback with SCSI_REASON_DATA -and- call
> scsi_command_complete(). The former will cause the client to queue a
> new read or write request, while the later will free the request data
> structure, thus causing the new read or write request to use a
> freed/stale structure when it completes.
>
> This patch fixes the bug, fixing a crash with scsi-generic & RHEL5.5
> installer.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v3 03/21] scsi: introduce scsi_req_data
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 01/21] scsi: add tracing of scsi requests Paolo Bonzini
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 02/21] scsi-generic: Remove bogus double complete Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 15:52 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 04/21] scsi: introduce SCSIBusOps Paolo Bonzini
` (18 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel
This abstracts calling the command_complete callback, reducing churn
in the following patches.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-bus.c | 6 ++++++
hw/scsi-disk.c | 8 ++++----
hw/scsi-generic.c | 6 +++---
hw/scsi.h | 1 +
trace-events | 1 +
5 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 0fd85fc..0afe3fb 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -495,6 +495,12 @@ static const char *scsi_command_name(uint8_t cmd)
return names[cmd];
}
+void scsi_req_data(SCSIRequest *req, int len)
+{
+ trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
+ req->bus->complete(req->bus, SCSI_REASON_DATA, req->tag, len);
+}
+
void scsi_req_print(SCSIRequest *req)
{
FILE *fp = stderr;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index b05e654..2b5dc2a 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -167,7 +167,7 @@ static void scsi_read_complete(void * opaque, int ret)
n = r->iov.iov_len / 512;
r->sector += n;
r->sector_count -= n;
- r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, r->iov.iov_len);
+ scsi_req_data(&r->req, r->iov.iov_len);
}
@@ -179,7 +179,7 @@ static void scsi_read_request(SCSIDiskReq *r)
if (r->sector_count == (uint32_t)-1) {
DPRINTF("Read buf_len=%zd\n", r->iov.iov_len);
r->sector_count = 0;
- r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, r->iov.iov_len);
+ scsi_req_data(&r->req, r->iov.iov_len);
return;
}
DPRINTF("Read sector_count=%d\n", r->sector_count);
@@ -242,7 +242,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
vm_stop(VMSTOP_DISKFULL);
} else {
if (type == SCSI_REQ_STATUS_RETRY_READ) {
- r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
+ scsi_req_data(&r->req, 0);
}
scsi_command_complete(r, CHECK_CONDITION,
HARDWARE_ERROR);
@@ -278,7 +278,7 @@ static void scsi_write_complete(void * opaque, int ret)
}
r->iov.iov_len = len;
DPRINTF("Write complete tag=0x%x more=%d\n", r->req.tag, len);
- r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);
+ scsi_req_data(&r->req, len);
}
}
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 102f1da..e4f1f30 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -175,7 +175,7 @@ static void scsi_read_complete(void * opaque, int ret)
if (len == 0) {
scsi_command_complete(r, 0);
} else {
- r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, len);
+ scsi_req_data(&r->req, len);
}
}
@@ -212,7 +212,7 @@ static void scsi_read_data(SCSIDevice *d, uint32_t tag)
DPRINTF("Sense: %d %d %d %d %d %d %d %d\n",
r->buf[0], r->buf[1], r->buf[2], r->buf[3],
r->buf[4], r->buf[5], r->buf[6], r->buf[7]);
- r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, s->senselen);
+ scsi_req_data(&r->req, s->senselen);
return;
}
@@ -263,7 +263,7 @@ static int scsi_write_data(SCSIDevice *d, uint32_t tag)
if (r->len == 0) {
r->len = r->buflen;
- r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, r->len);
+ scsi_req_data(&r->req, r->len);
return 0;
}
diff --git a/hw/scsi.h b/hw/scsi.h
index d3b5d56..7c09f32 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -105,6 +105,7 @@ void scsi_req_free(SCSIRequest *req);
int scsi_req_parse(SCSIRequest *req, uint8_t *buf);
void scsi_req_print(SCSIRequest *req);
+void scsi_req_data(SCSIRequest *req, int len);
void scsi_req_complete(SCSIRequest *req);
#endif
diff --git a/trace-events b/trace-events
index 55f89b4..98e7b2d 100644
--- a/trace-events
+++ b/trace-events
@@ -207,6 +207,7 @@ disable usb_set_device_feature(int addr, int feature, int ret) "dev %d, feature
# 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_parsed(int target, int lun, int tag, int cmd, int mode, int xfer, uint64_t lba) "target %d lun %d tag %d command %d dir %d length %d lba %"PRIu64""
disable scsi_req_parse_bad(int target, int lun, int tag, int cmd) "target %d lun %d tag %d command %d"
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 04/21] scsi: introduce SCSIBusOps
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (2 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 03/21] scsi: introduce scsi_req_data Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 15:53 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 05/21] scsi: reference-count requests Paolo Bonzini
` (17 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel
There are more operations than a SCSI bus can handle, besides completing
commands. One example, which this series will introduce, is cleaning up
after a request is cancelled.
More long term, a "SCSI bus" can represent the LUNs attached to a
target; in this case, while all commands will ultimately reach a logical
unit, it is the target who is in charge of answering REPORT LUNs.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/esp.c | 6 +++++-
hw/lsi53c895a.c | 6 +++++-
hw/scsi-bus.c | 12 ++++++------
hw/scsi-generic.c | 2 +-
hw/scsi.h | 13 +++++++------
hw/spapr_vscsi.c | 6 +++++-
hw/usb-msd.c | 6 +++++-
7 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/hw/esp.c b/hw/esp.c
index fa9d2a2..d8bba7a 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -714,6 +714,10 @@ void esp_init(target_phys_addr_t espaddr, int it_shift,
*dma_enable = qdev_get_gpio_in(dev, 1);
}
+static struct SCSIBusOps esp_scsi_ops = {
+ .complete = esp_command_complete
+};
+
static int esp_init1(SysBusDevice *dev)
{
ESPState *s = FROM_SYSBUS(ESPState, dev);
@@ -728,7 +732,7 @@ static int esp_init1(SysBusDevice *dev)
qdev_init_gpio_in(&dev->qdev, esp_gpio_demux, 2);
- scsi_bus_new(&s->bus, &dev->qdev, 0, ESP_MAX_DEVS, esp_command_complete);
+ scsi_bus_new(&s->bus, &dev->qdev, 0, ESP_MAX_DEVS, &esp_scsi_ops);
return scsi_bus_legacy_handle_cmdline(&s->bus);
}
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 2ce38a9..ccea6ad 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -2205,6 +2205,10 @@ static int lsi_scsi_uninit(PCIDevice *d)
return 0;
}
+static struct SCSIBusOps lsi_scsi_ops = {
+ .complete = lsi_command_complete
+};
+
static int lsi_scsi_init(PCIDevice *dev)
{
LSIState *s = DO_UPCAST(LSIState, dev, dev);
@@ -2241,7 +2245,7 @@ static int lsi_scsi_init(PCIDevice *dev)
PCI_BASE_ADDRESS_SPACE_MEMORY, lsi_ram_mapfunc);
QTAILQ_INIT(&s->queue);
- scsi_bus_new(&s->bus, &dev->qdev, 1, LSI_MAX_DEVS, lsi_command_complete);
+ scsi_bus_new(&s->bus, &dev->qdev, 1, LSI_MAX_DEVS, &lsi_scsi_ops);
if (!dev->qdev.hotplugged) {
return scsi_bus_legacy_handle_cmdline(&s->bus);
}
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 0afe3fb..63d9a68 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -21,13 +21,13 @@ static int next_scsi_bus;
/* Create a scsi bus, and attach devices to it. */
void scsi_bus_new(SCSIBus *bus, DeviceState *host, int tcq, int ndev,
- scsi_completionfn complete)
+ SCSIBusOps *ops)
{
qbus_create_inplace(&bus->qbus, &scsi_bus_info, host, NULL);
bus->busnr = next_scsi_bus++;
bus->tcq = tcq;
bus->ndev = ndev;
- bus->complete = complete;
+ bus->ops = *ops;
bus->qbus.allow_hotplug = 1;
}
@@ -498,7 +498,7 @@ static const char *scsi_command_name(uint8_t cmd)
void scsi_req_data(SCSIRequest *req, int len)
{
trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
- req->bus->complete(req->bus, SCSI_REASON_DATA, req->tag, len);
+ req->bus->ops.complete(req->bus, SCSI_REASON_DATA, req->tag, len);
}
void scsi_req_print(SCSIRequest *req)
@@ -533,9 +533,9 @@ void scsi_req_complete(SCSIRequest *req)
{
assert(req->status != -1);
scsi_req_dequeue(req);
- req->bus->complete(req->bus, SCSI_REASON_DONE,
- req->tag,
- req->status);
+ req->bus->ops.complete(req->bus, SCSI_REASON_DONE,
+ req->tag,
+ req->status);
}
static char *scsibus_get_fw_dev_path(DeviceState *dev)
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index e4f1f30..3db734a 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -335,7 +335,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
s->senselen = 7;
s->driver_status = SG_ERR_DRIVER_SENSE;
bus = scsi_bus_from_device(d);
- bus->complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
+ bus->ops.complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
return 0;
}
diff --git a/hw/scsi.h b/hw/scsi.h
index 7c09f32..d1753f9 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -16,10 +16,9 @@ enum scsi_reason {
};
typedef struct SCSIBus SCSIBus;
+typedef struct SCSIBusOps SCSIBusOps;
typedef struct SCSIDevice SCSIDevice;
typedef struct SCSIDeviceInfo SCSIDeviceInfo;
-typedef void (*scsi_completionfn)(SCSIBus *bus, int reason, uint32_t tag,
- uint32_t arg);
enum SCSIXferMode {
SCSI_XFER_NONE, /* TEST_UNIT_READY, ... */
@@ -74,20 +73,22 @@ struct SCSIDeviceInfo {
uint8_t *(*get_buf)(SCSIDevice *s, uint32_t tag);
};
-typedef void (*SCSIAttachFn)(DeviceState *host, BlockDriverState *bdrv,
- int unit);
+struct SCSIBusOps {
+ void (*complete)(SCSIBus *bus, int reason, uint32_t tag, uint32_t arg);
+};
+
struct SCSIBus {
BusState qbus;
int busnr;
int tcq, ndev;
- scsi_completionfn complete;
+ SCSIBusOps ops;
SCSIDevice *devs[MAX_SCSI_DEVS];
};
void scsi_bus_new(SCSIBus *bus, DeviceState *host, int tcq, int ndev,
- scsi_completionfn complete);
+ SCSIBusOps *ops);
void scsi_qdev_register(SCSIDeviceInfo *info);
static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 9928334..9f5c154 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -907,6 +907,10 @@ static int vscsi_do_crq(struct VIOsPAPRDevice *dev, uint8_t *crq_data)
return 0;
}
+static struct SCSIBusOps vscsi_scsi_ops = {
+ .complete = vscsi_command_complete
+};
+
static int spapr_vscsi_init(VIOsPAPRDevice *dev)
{
VSCSIState *s = DO_UPCAST(VSCSIState, vdev, dev);
@@ -923,7 +927,7 @@ static int spapr_vscsi_init(VIOsPAPRDevice *dev)
dev->crq.SendFunc = vscsi_do_crq;
scsi_bus_new(&s->bus, &dev->qdev, 1, VSCSI_REQ_LIMIT,
- vscsi_command_complete);
+ &vscsi_scsi_ops);
if (!dev->qdev.hotplugged) {
scsi_bus_legacy_handle_cmdline(&s->bus);
}
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index bd1c3a4..7a07a12 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -487,6 +487,10 @@ static void usb_msd_password_cb(void *opaque, int err)
qdev_unplug(&s->dev.qdev);
}
+static struct SCSIBusOps usb_msd_scsi_ops = {
+ .complete = usb_msd_command_complete
+};
+
static int usb_msd_initfn(USBDevice *dev)
{
MSDState *s = DO_UPCAST(MSDState, dev, dev);
@@ -516,7 +520,7 @@ static int usb_msd_initfn(USBDevice *dev)
}
usb_desc_init(dev);
- scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, usb_msd_command_complete);
+ scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, &usb_msd_scsi_ops);
s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, !!s->removable);
if (!s->scsi_dev) {
return -1;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 04/21] scsi: introduce SCSIBusOps
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 04/21] scsi: introduce SCSIBusOps Paolo Bonzini
@ 2011-05-20 15:53 ` Christoph Hellwig
2011-05-20 17:46 ` Paolo Bonzini
0 siblings, 1 reply; 52+ messages in thread
From: Christoph Hellwig @ 2011-05-20 15:53 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
> qbus_create_inplace(&bus->qbus, &scsi_bus_info, host, NULL);
> bus->busnr = next_scsi_bus++;
> bus->tcq = tcq;
> bus->ndev = ndev;
> - bus->complete = complete;
> + bus->ops = *ops;
Normally bus->ops would be a pointer, so you can just assign it to
the address passed in instead of doing a copy. Any good reason to
do it differently here?
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/21] scsi: introduce SCSIBusOps
2011-05-20 15:53 ` Christoph Hellwig
@ 2011-05-20 17:46 ` Paolo Bonzini
0 siblings, 0 replies; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-20 17:46 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: qemu-devel
On 05/20/2011 05:53 PM, Christoph Hellwig wrote:
>> qbus_create_inplace(&bus->qbus,&scsi_bus_info, host, NULL);
>> > bus->busnr = next_scsi_bus++;
>> > bus->tcq = tcq;
>> > bus->ndev = ndev;
>> > - bus->complete = complete;
>> > + bus->ops = *ops;
>
> Normally bus->ops would be a pointer, so you can just assign it to
> the address passed in instead of doing a copy. Any good reason to
> do it differently here?
I was thinking that they could be modified in-place later, or built on
the stack depending on some qdev properties, but it's probably pointless
to do it differently. Will change.
Paolo
^ permalink raw reply [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v3 05/21] scsi: reference-count requests
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (3 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 04/21] scsi: introduce SCSIBusOps Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 15:58 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 06/21] lsi: extract lsi_find_by_tag Paolo Bonzini
` (16 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel
With the next patch, a device may hold SCSIRequest for an indefinite
time. Split a rather big patch, and protect against access errors,
by reference counting them. One such access error in fact exists (it
is visible by testing the lsi driver with MALLOC_PERTURB_), and this
patch provides the infrastructure to fix it later.
There is some ugliness in scsi_send_command implementation due to
the need to unref the request when it fails. This will go away
with the next patches, which move the unref'ing to the devices.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-bus.c | 22 ++++++++++++++++++++--
hw/scsi-disk.c | 20 +++++++++++++-------
hw/scsi-generic.c | 23 ++++++++++++++++-------
hw/scsi.h | 5 +++++
4 files changed, 54 insertions(+), 16 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 63d9a68..c0bc275 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -136,6 +136,7 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
SCSIRequest *req;
req = qemu_mallocz(size);
+ req->refcount = 2;
req->bus = scsi_bus_from_device(d);
req->dev = d;
req->tag = tag;
@@ -159,21 +160,23 @@ SCSIRequest *scsi_req_find(SCSIDevice *d, uint32_t tag)
return NULL;
}
-static void scsi_req_dequeue(SCSIRequest *req)
+void scsi_req_dequeue(SCSIRequest *req)
{
trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag);
if (req->enqueued) {
QTAILQ_REMOVE(&req->dev->requests, req, next);
req->enqueued = false;
+ scsi_req_unref(req);
}
}
void scsi_req_free(SCSIRequest *req)
{
- scsi_req_dequeue(req);
+ assert(req->refcount == 0);
qemu_free(req);
}
+
static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
{
switch (cmd[0] >> 5) {
@@ -495,6 +498,19 @@ static const char *scsi_command_name(uint8_t cmd)
return names[cmd];
}
+SCSIRequest *scsi_req_ref(SCSIRequest *req)
+{
+ req->refcount++;
+ return req;
+}
+
+void scsi_req_unref(SCSIRequest *req)
+{
+ if (--req->refcount == 0) {
+ req->dev->info->free_req(req);
+ }
+}
+
void scsi_req_data(SCSIRequest *req, int len)
{
trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
@@ -532,10 +548,12 @@ void scsi_req_print(SCSIRequest *req)
void scsi_req_complete(SCSIRequest *req)
{
assert(req->status != -1);
+ scsi_req_ref(req);
scsi_req_dequeue(req);
req->bus->ops.complete(req->bus, SCSI_REASON_DONE,
req->tag,
req->status);
+ scsi_req_unref(req);
}
static char *scsibus_get_fw_dev_path(DeviceState *dev)
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 2b5dc2a..ba7ffa1 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -95,8 +95,10 @@ static SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
return r;
}
-static void scsi_remove_request(SCSIDiskReq *r)
+static void scsi_free_request(SCSIRequest *req)
{
+ SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+
qemu_vfree(r->iov.iov_base);
scsi_req_free(&r->req);
}
@@ -131,7 +133,6 @@ static void scsi_command_complete(SCSIDiskReq *r, int status, int sense)
r->req.tag, status, sense);
scsi_req_set_status(r, status, sense);
scsi_req_complete(&r->req);
- scsi_remove_request(r);
}
/* Cancel a pending data transfer. */
@@ -145,7 +146,7 @@ static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
if (r->req.aiocb)
bdrv_aio_cancel(r->req.aiocb);
r->req.aiocb = NULL;
- scsi_remove_request(r);
+ scsi_req_dequeue(&r->req);
}
}
@@ -1030,7 +1031,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
uint8_t *buf, int lun)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
- uint32_t len;
+ int32_t len;
int is_write;
uint8_t command;
uint8_t *outbuf;
@@ -1092,6 +1093,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
case REZERO_UNIT:
rc = scsi_disk_emulate_command(r, outbuf);
if (rc < 0) {
+ scsi_req_unref(&r->req);
return 0;
}
@@ -1178,9 +1180,11 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
fail:
scsi_command_complete(r, CHECK_CONDITION, ILLEGAL_REQUEST);
+ scsi_req_unref(&r->req);
return 0;
illegal_lba:
scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
+ scsi_req_unref(&r->req);
return 0;
}
if (r->sector_count == 0 && r->iov.iov_len == 0) {
@@ -1188,12 +1192,13 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
}
len = r->sector_count * 512 + r->iov.iov_len;
if (is_write) {
- return -len;
+ len = -len;
} else {
if (!r->sector_count)
r->sector_count = -1;
- return len;
}
+ scsi_req_unref(&r->req);
+ return len;
}
static void scsi_disk_purge_requests(SCSIDiskState *s)
@@ -1205,7 +1210,7 @@ static void scsi_disk_purge_requests(SCSIDiskState *s)
if (r->req.aiocb) {
bdrv_aio_cancel(r->req.aiocb);
}
- scsi_remove_request(r);
+ scsi_req_dequeue(&r->req);
}
}
@@ -1288,6 +1293,7 @@ static SCSIDeviceInfo scsi_disk_info = {
.qdev.reset = scsi_disk_reset,
.init = scsi_disk_initfn,
.destroy = scsi_destroy,
+ .free_req = scsi_free_request,
.send_command = scsi_send_command,
.read_data = scsi_read_data,
.write_data = scsi_write_data,
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 3db734a..5739067 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -74,8 +74,10 @@ static SCSIGenericReq *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lu
return DO_UPCAST(SCSIGenericReq, req, req);
}
-static void scsi_remove_request(SCSIGenericReq *r)
+static void scsi_free_request(SCSIRequest *req)
{
+ SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
+
qemu_free(r->buf);
scsi_req_free(&r->req);
}
@@ -112,7 +114,6 @@ static void scsi_command_complete(void *opaque, int ret)
r, r->req.tag, r->req.status);
scsi_req_complete(&r->req);
- scsi_remove_request(r);
}
/* Cancel a pending data transfer. */
@@ -127,7 +128,7 @@ static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
if (r->req.aiocb)
bdrv_aio_cancel(r->req.aiocb);
r->req.aiocb = NULL;
- scsi_remove_request(r);
+ scsi_req_dequeue(&r->req);
}
}
@@ -320,6 +321,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
SCSIGenericReq *r;
SCSIBus *bus;
int ret;
+ int32_t len;
if (cmd[0] != REQUEST_SENSE &&
(lun != s->lun || (cmd[1] >> 5) != s->lun)) {
@@ -348,7 +350,8 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
if (-1 == scsi_req_parse(&r->req, cmd)) {
BADF("Unsupported command length, command %x\n", cmd[0]);
- scsi_remove_request(r);
+ scsi_req_dequeue(&r->req);
+ scsi_req_unref(&r->req);
return 0;
}
scsi_req_fixup(&r->req);
@@ -374,8 +377,10 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
ret = execute_command(s->bs, r, SG_DXFER_NONE, scsi_command_complete);
if (ret == -1) {
scsi_command_complete(r, -EINVAL);
+ scsi_req_unref(&r->req);
return 0;
}
+ scsi_req_unref(&r->req);
return 0;
}
@@ -390,10 +395,13 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
r->len = r->req.cmd.xfer;
if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
r->len = 0;
- return -r->req.cmd.xfer;
+ len = -r->req.cmd.xfer;
+ } else {
+ len = r->req.cmd.xfer;
}
- return r->req.cmd.xfer;
+ scsi_req_unref(&r->req);
+ return len;
}
static int get_blocksize(BlockDriverState *bdrv)
@@ -466,7 +474,7 @@ static void scsi_generic_purge_requests(SCSIGenericState *s)
if (r->req.aiocb) {
bdrv_aio_cancel(r->req.aiocb);
}
- scsi_remove_request(r);
+ scsi_req_dequeue(&r->req);
}
}
@@ -558,6 +566,7 @@ static SCSIDeviceInfo scsi_generic_info = {
.qdev.reset = scsi_generic_reset,
.init = scsi_generic_initfn,
.destroy = scsi_destroy,
+ .free_req = scsi_free_request,
.send_command = scsi_send_command,
.read_data = scsi_read_data,
.write_data = scsi_write_data,
diff --git a/hw/scsi.h b/hw/scsi.h
index d1753f9..ecf1aeb 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -29,6 +29,7 @@ enum SCSIXferMode {
typedef struct SCSIRequest {
SCSIBus *bus;
SCSIDevice *dev;
+ uint32_t refcount;
uint32_t tag;
uint32_t lun;
uint32_t status;
@@ -65,6 +66,7 @@ struct SCSIDeviceInfo {
DeviceInfo qdev;
scsi_qdev_initfn init;
void (*destroy)(SCSIDevice *s);
+ void (*free_req)(SCSIRequest *req);
int32_t (*send_command)(SCSIDevice *s, uint32_t tag, uint8_t *buf,
int lun);
void (*read_data)(SCSIDevice *s, uint32_t tag);
@@ -103,6 +105,9 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun);
SCSIRequest *scsi_req_find(SCSIDevice *d, uint32_t tag);
void scsi_req_free(SCSIRequest *req);
+void scsi_req_dequeue(SCSIRequest *req);
+SCSIRequest *scsi_req_ref(SCSIRequest *req);
+void scsi_req_unref(SCSIRequest *req);
int scsi_req_parse(SCSIRequest *req, uint8_t *buf);
void scsi_req_print(SCSIRequest *req);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 05/21] scsi: reference-count requests
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 05/21] scsi: reference-count requests Paolo Bonzini
@ 2011-05-20 15:58 ` Christoph Hellwig
2011-05-20 17:48 ` Paolo Bonzini
0 siblings, 1 reply; 52+ messages in thread
From: Christoph Hellwig @ 2011-05-20 15:58 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -136,6 +136,7 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
> SCSIRequest *req;
>
> req = qemu_mallocz(size);
> + req->refcount = 2;
> req->bus = scsi_bus_from_device(d);
> req->dev = d;
> req->tag = tag;
A little comment explaining why we start out with a reference count of 2
would be useful here. Might be worth making that a top of the function
block comment explaning the function a bit more while you're at it.
> void scsi_req_free(SCSIRequest *req)
> {
> - scsi_req_dequeue(req);
> + assert(req->refcount == 0);
> qemu_free(req);
> }
Is there any reason to keep a free function? The pattern should be
that people just call the function to decrement the reference count,
and that frees the structure when it hits zero. In the current model
that would mean moving the freeing out of ->free_req into scsi_req_unref,
but that seems pretty sensible anyway.
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 05/21] scsi: reference-count requests
2011-05-20 15:58 ` Christoph Hellwig
@ 2011-05-20 17:48 ` Paolo Bonzini
2011-05-20 18:03 ` Paolo Bonzini
0 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-20 17:48 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: qemu-devel
On 05/20/2011 05:58 PM, Christoph Hellwig wrote:
>> > void scsi_req_free(SCSIRequest *req)
>> > {
>> > - scsi_req_dequeue(req);
>> > + assert(req->refcount == 0);
>> > qemu_free(req);
>> > }
> Is there any reason to keep a free function?
It's internal for SCSIDevice implementation, kind of a "base
implementation" for free_req
> The pattern should be
> that people just call the function to decrement the reference count,
> and that frees the structure when it hits zero. In the current model
> that would mean moving the freeing out of ->free_req into scsi_req_unref,
> but that seems pretty sensible anyway.
free_req is still needed, because it takes care of freeing the bounce
buffers or any other allocated data.
Paolo
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 05/21] scsi: reference-count requests
2011-05-20 17:48 ` Paolo Bonzini
@ 2011-05-20 18:03 ` Paolo Bonzini
0 siblings, 0 replies; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-20 18:03 UTC (permalink / raw)
Cc: Christoph Hellwig, qemu-devel
On 05/20/2011 07:48 PM, Paolo Bonzini wrote:
>> Is there any reason to keep a free function?
>
> It's internal for SCSIDevice implementation, kind of a "base
> implementation" for free_req
>
>> The pattern should be
>> that people just call the function to decrement the reference count,
>> and that frees the structure when it hits zero. In the current model
>> that would mean moving the freeing out of ->free_req into scsi_req_unref,
>> but that seems pretty sensible anyway.
>
> free_req is still needed, because it takes care of freeing the bounce
> buffers or any other allocated data.
Nevermind, I see what you mean. Will do.
Paolo
^ permalink raw reply [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v3 06/21] lsi: extract lsi_find_by_tag
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (4 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 05/21] scsi: reference-count requests Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 15:58 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 07/21] scsi: Use 'SCSIRequest' directly Paolo Bonzini
` (15 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/lsi53c895a.c | 63 +++++++++++++++++++++++++++++++++---------------------
1 files changed, 38 insertions(+), 25 deletions(-)
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index ccea6ad..3b67155 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -652,38 +652,51 @@ static void lsi_reselect(LSIState *s, lsi_request *p)
}
}
-/* 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 arg)
+static lsi_request *lsi_find_by_tag(LSIState *s, uint32_t tag)
{
lsi_request *p;
QTAILQ_FOREACH(p, &s->queue, next) {
if (p->tag == tag) {
- if (p->pending) {
- BADF("Multiple IO pending for tag %d\n", tag);
- }
- p->pending = arg;
- /* Reselect if waiting for it, or if reselection triggers an IRQ
- and the bus is free.
- Since no interrupt stacking is implemented in the emulation, it
- is also required that there are no pending interrupts waiting
- for service from the device driver. */
- if (s->waiting == 1 ||
- (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON) &&
- !(s->istat0 & (LSI_ISTAT0_SIP | LSI_ISTAT0_DIP)))) {
- /* Reselect device. */
- lsi_reselect(s, p);
- return 0;
- } else {
- DPRINTF("Queueing IO tag=0x%x\n", tag);
- p->pending = arg;
- return 1;
- }
+ return p;
}
}
- BADF("IO with unknown tag %d\n", tag);
- return 1;
+
+ return NULL;
+}
+
+/* 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 arg)
+{
+ lsi_request *p;
+
+ p = lsi_find_by_tag(s, tag);
+ if (!p) {
+ BADF("IO with unknown tag %d\n", tag);
+ return 1;
+ }
+
+ if (p->pending) {
+ BADF("Multiple IO pending for tag %d\n", tag);
+ }
+ p->pending = arg;
+ /* Reselect if waiting for it, or if reselection triggers an IRQ
+ and the bus is free.
+ Since no interrupt stacking is implemented in the emulation, it
+ is also required that there are no pending interrupts waiting
+ for service from the device driver. */
+ if (s->waiting == 1 ||
+ (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON) &&
+ !(s->istat0 & (LSI_ISTAT0_SIP | LSI_ISTAT0_DIP)))) {
+ /* Reselect device. */
+ lsi_reselect(s, p);
+ return 0;
+ } else {
+ DPRINTF("Queueing IO tag=0x%x\n", tag);
+ p->pending = arg;
+ return 1;
+ }
}
/* Callback to indicate that the SCSI layer has completed a transfer. */
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 07/21] scsi: Use 'SCSIRequest' directly
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (5 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 06/21] lsi: extract lsi_find_by_tag Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 15:59 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 08/21] scsi: commonize purging requests Paolo Bonzini
` (14 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Hannes Reinecke, Christoph Hellwig
From: Hannes Reinecke <hare@suse.de>
Currently the SCSIRequest structure is abstracted away and cannot accessed
directly from the driver. This requires the handler to do a lookup on
an abstract 'tag' which identifies the SCSIRequest structure.
With this patch the SCSIRequest structure is exposed to the driver. This
allows use to use it directly as an argument to the SCSIDeviceInfo
callback functions and remove the lookup.
A new callback function 'alloc_req' is introduced matching 'free
req'; unref'ing to free up resources after use is moved into the
scsi_command_complete callbacks.
This temporarily introduces a leak of requests that are cancelled,
when they are removed from the queue and not from the driver. This
is fixed later by introducing scsi_req_cancel. That patch in turn
depends on this one, because the argument to scsi_req_cancel is a
SCSIRequest.
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
---
hw/esp.c | 29 ++++++++-----
hw/lsi53c895a.c | 56 +++++++++++++++-----------
hw/scsi-bus.c | 24 ++++-------
hw/scsi-disk.c | 116 ++++++++++++++--------------------------------------
hw/scsi-generic.c | 107 +++++++++++++++----------------------------------
hw/scsi.h | 21 +++++-----
hw/spapr_vscsi.c | 44 +++++++++++---------
hw/usb-msd.c | 27 +++++++-----
8 files changed, 172 insertions(+), 252 deletions(-)
diff --git a/hw/esp.c b/hw/esp.c
index d8bba7a..096f4dc 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -65,6 +65,7 @@ struct ESPState {
uint32_t dma;
SCSIBus bus;
SCSIDevice *current_dev;
+ SCSIRequest *current_req;
uint8_t cmdbuf[TI_BUFSZ];
uint32_t cmdlen;
uint32_t do_cmd;
@@ -209,7 +210,7 @@ static uint32_t get_cmd(ESPState *s, uint8_t *buf)
if (s->current_dev) {
/* Started a new command before the old one finished. Cancel it. */
- s->current_dev->info->cancel_io(s->current_dev, 0);
+ s->current_dev->info->cancel_io(s->current_req);
s->async_len = 0;
}
@@ -230,9 +231,10 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
int32_t datalen;
int lun;
- DPRINTF("do_busid_cmd: busid 0x%x\n", busid);
+ DPRINTF("do_busid_cmd: busid 0x%x\n", busid);
lun = busid & 7;
- datalen = s->current_dev->info->send_command(s->current_dev, 0, buf, lun);
+ s->current_req = s->current_dev->info->alloc_req(s->current_dev, 0, lun);
+ datalen = s->current_dev->info->send_command(s->current_req, buf);
s->ti_size = datalen;
if (datalen != 0) {
s->rregs[ESP_RSTAT] = STAT_TC;
@@ -240,10 +242,10 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
s->dma_counter = 0;
if (datalen > 0) {
s->rregs[ESP_RSTAT] |= STAT_DI;
- s->current_dev->info->read_data(s->current_dev, 0);
+ s->current_dev->info->read_data(s->current_req);
} else {
s->rregs[ESP_RSTAT] |= STAT_DO;
- s->current_dev->info->write_data(s->current_dev, 0);
+ s->current_dev->info->write_data(s->current_req);
}
}
s->rregs[ESP_RINTR] = INTR_BS | INTR_FC;
@@ -372,9 +374,9 @@ static void esp_do_dma(ESPState *s)
if (s->async_len == 0) {
if (to_device) {
// ti_size is negative
- s->current_dev->info->write_data(s->current_dev, 0);
+ s->current_dev->info->write_data(s->current_req);
} else {
- s->current_dev->info->read_data(s->current_dev, 0);
+ s->current_dev->info->read_data(s->current_req);
/* If there is still data to be read from the device then
complete the DMA operation immediately. Otherwise defer
until the scsi layer has completed. */
@@ -388,10 +390,9 @@ static void esp_do_dma(ESPState *s)
}
}
-static void esp_command_complete(SCSIBus *bus, int reason, uint32_t tag,
- uint32_t arg)
+static void esp_command_complete(SCSIRequest *req, int reason, uint32_t arg)
{
- ESPState *s = DO_UPCAST(ESPState, busdev.qdev, bus->qbus.parent);
+ ESPState *s = DO_UPCAST(ESPState, busdev.qdev, req->bus->qbus.parent);
if (reason == SCSI_REASON_DONE) {
DPRINTF("SCSI Command complete\n");
@@ -405,11 +406,15 @@ static void esp_command_complete(SCSIBus *bus, int reason, uint32_t tag,
s->sense = arg;
s->rregs[ESP_RSTAT] = STAT_ST;
esp_dma_done(s);
- s->current_dev = NULL;
+ if (s->current_req) {
+ scsi_req_unref(s->current_req);
+ s->current_req = NULL;
+ s->current_dev = NULL;
+ }
} else {
DPRINTF("transfer %d/%d\n", s->dma_left, s->ti_size);
s->async_len = arg;
- s->async_buf = s->current_dev->info->get_buf(s->current_dev, 0);
+ s->async_buf = s->current_dev->info->get_buf(req);
if (s->dma_left) {
esp_do_dma(s);
} else if (s->dma_counter != 0 && s->ti_size <= 0) {
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 3b67155..b9febae 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -174,6 +174,7 @@ do { fprintf(stderr, "lsi_scsi: error: " fmt , ## __VA_ARGS__);} while (0)
#define LSI_TAG_VALID (1 << 16)
typedef struct lsi_request {
+ SCSIRequest *req;
uint32_t tag;
uint32_t dma_len;
uint8_t *dma_buf;
@@ -567,11 +568,9 @@ static void lsi_do_dma(LSIState *s, int out)
s->csbc += count;
s->dnad += count;
s->dbc -= count;
-
- if (s->current->dma_buf == NULL) {
- s->current->dma_buf = dev->info->get_buf(dev, s->current->tag);
+ if (s->current->dma_buf == NULL) {
+ s->current->dma_buf = dev->info->get_buf(s->current->req);
}
-
/* ??? Set SFBR to first data byte. */
if (out) {
cpu_physical_memory_read(addr, s->current->dma_buf, count);
@@ -583,10 +582,10 @@ static void lsi_do_dma(LSIState *s, int out)
s->current->dma_buf = NULL;
if (out) {
/* Write the data. */
- dev->info->write_data(dev, s->current->tag);
+ dev->info->write_data(s->current->req);
} else {
/* Request any remaining data. */
- dev->info->read_data(dev, s->current->tag);
+ dev->info->read_data(s->current->req);
}
} else {
s->current->dma_buf += count;
@@ -698,12 +697,10 @@ static int lsi_queue_tag(LSIState *s, uint32_t tag, uint32_t arg)
return 1;
}
}
-
-/* Callback to indicate that the SCSI layer has completed a transfer. */
-static void lsi_command_complete(SCSIBus *bus, int reason, uint32_t tag,
- uint32_t arg)
+ /* Callback to indicate that the SCSI layer has completed a transfer. */
+static void lsi_command_complete(SCSIRequest *req, int reason, uint32_t arg)
{
- LSIState *s = DO_UPCAST(LSIState, dev.qdev, bus->qbus.parent);
+ LSIState *s = DO_UPCAST(LSIState, dev.qdev, req->bus->qbus.parent);
int out;
out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
@@ -718,21 +715,24 @@ static void lsi_command_complete(SCSIBus *bus, int reason, uint32_t tag,
lsi_set_phase(s, PHASE_ST);
}
- qemu_free(s->current);
- s->current = NULL;
-
+ if (req == s->current->req) {
+ scsi_req_unref(s->current->req);
+ qemu_free(s->current);
+ s->current = NULL;
+ }
lsi_resume_script(s);
return;
}
- if (s->waiting == 1 || !s->current || tag != s->current->tag ||
+ if (s->waiting == 1 || !s->current || req->tag != s->current->tag ||
(lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON))) {
- if (lsi_queue_tag(s, tag, arg))
+ if (lsi_queue_tag(s, req->tag, arg)) {
return;
+ }
}
/* host adapter (re)connected */
- DPRINTF("Data ready tag=0x%x len=%d\n", tag, arg);
+ DPRINTF("Data ready tag=0x%x len=%d\n", req->tag, arg);
s->current->dma_len = arg;
s->command_complete = 1;
if (!s->waiting)
@@ -768,14 +768,16 @@ 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 = dev->info->alloc_req(dev, s->current->tag,
+ s->current_lun);
- n = dev->info->send_command(dev, s->current->tag, buf, s->current_lun);
+ n = dev->info->send_command(s->current->req, buf);
if (n > 0) {
lsi_set_phase(s, PHASE_DI);
- dev->info->read_data(dev, s->current->tag);
+ dev->info->read_data(s->current->req);
} else if (n < 0) {
lsi_set_phase(s, PHASE_DO);
- dev->info->write_data(dev, s->current->tag);
+ dev->info->write_data(s->current->req);
}
if (!s->command_complete) {
@@ -868,13 +870,15 @@ static void lsi_do_msgout(LSIState *s)
int len;
uint32_t current_tag;
SCSIDevice *current_dev;
- lsi_request *p, *p_next;
+ lsi_request *current_req, *p, *p_next;
int id;
if (s->current) {
current_tag = s->current->tag;
+ current_req = s->current;
} else {
current_tag = s->select_tag;
+ current_req = lsi_find_by_tag(s, current_tag);
}
id = (current_tag >> 8) & 0xf;
current_dev = s->bus.devs[id];
@@ -926,7 +930,9 @@ static void lsi_do_msgout(LSIState *s)
case 0x0d:
/* The ABORT TAG message clears the current I/O process only. */
DPRINTF("MSG: ABORT TAG tag=0x%x\n", current_tag);
- current_dev->info->cancel_io(current_dev, current_tag);
+ if (current_req) {
+ current_dev->info->cancel_io(current_req->req);
+ }
lsi_disconnect(s);
break;
case 0x06:
@@ -949,7 +955,9 @@ static void lsi_do_msgout(LSIState *s)
}
/* clear the current I/O process */
- current_dev->info->cancel_io(current_dev, current_tag);
+ if (s->current) {
+ current_dev->info->cancel_io(s->current->req);
+ }
/* As the current implemented devices scsi_disk and scsi_generic
only support one LUN, we don't need to keep track of LUNs.
@@ -961,7 +969,7 @@ static void lsi_do_msgout(LSIState *s)
id = current_tag & 0x0000ff00;
QTAILQ_FOREACH_SAFE(p, &s->queue, next, p_next) {
if ((p->tag & 0x0000ff00) == id) {
- current_dev->info->cancel_io(current_dev, p->tag);
+ current_dev->info->cancel_io(p->req);
QTAILQ_REMOVE(&s->queue, p, next);
}
}
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index c0bc275..a175590 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -136,28 +136,22 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
SCSIRequest *req;
req = qemu_mallocz(size);
- req->refcount = 2;
+ req->refcount = 1;
req->bus = scsi_bus_from_device(d);
req->dev = d;
req->tag = tag;
req->lun = lun;
req->status = -1;
- req->enqueued = true;
trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
- QTAILQ_INSERT_TAIL(&d->requests, req, next);
return req;
}
-SCSIRequest *scsi_req_find(SCSIDevice *d, uint32_t tag)
+void scsi_req_enqueue(SCSIRequest *req)
{
- SCSIRequest *req;
-
- QTAILQ_FOREACH(req, &d->requests, next) {
- if (req->tag == tag) {
- return req;
- }
- }
- return NULL;
+ assert(!req->enqueued);
+ scsi_req_ref(req);
+ req->enqueued = true;
+ QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
}
void scsi_req_dequeue(SCSIRequest *req)
@@ -514,7 +508,7 @@ void scsi_req_unref(SCSIRequest *req)
void scsi_req_data(SCSIRequest *req, int len)
{
trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
- req->bus->ops.complete(req->bus, SCSI_REASON_DATA, req->tag, len);
+ req->bus->ops.complete(req, SCSI_REASON_DATA, len);
}
void scsi_req_print(SCSIRequest *req)
@@ -550,9 +544,7 @@ void scsi_req_complete(SCSIRequest *req)
assert(req->status != -1);
scsi_req_ref(req);
scsi_req_dequeue(req);
- req->bus->ops.complete(req->bus, SCSI_REASON_DONE,
- req->tag,
- req->status);
+ req->bus->ops.complete(req, SCSI_REASON_DONE, req->status);
scsi_req_unref(req);
}
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index ba7ffa1..7446115 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -83,16 +83,17 @@ 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 SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
uint32_t lun)
{
+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
SCSIRequest *req;
SCSIDiskReq *r;
req = scsi_req_alloc(sizeof(SCSIDiskReq), &s->qdev, tag, lun);
r = DO_UPCAST(SCSIDiskReq, req, req);
r->iov.iov_base = qemu_blockalign(s->bs, SCSI_DMA_BUF_SIZE);
- return r;
+ return req;
}
static void scsi_free_request(SCSIRequest *req)
@@ -103,11 +104,6 @@ static void scsi_free_request(SCSIRequest *req)
scsi_req_free(&r->req);
}
-static SCSIDiskReq *scsi_find_request(SCSIDiskState *s, uint32_t tag)
-{
- return DO_UPCAST(SCSIDiskReq, req, scsi_req_find(&s->qdev, tag));
-}
-
static void scsi_disk_clear_sense(SCSIDiskState *s)
{
memset(&s->sense, 0, sizeof(s->sense));
@@ -136,18 +132,16 @@ static void scsi_command_complete(SCSIDiskReq *r, int status, int sense)
}
/* Cancel a pending data transfer. */
-static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
+static void scsi_cancel_io(SCSIRequest *req)
{
- SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
- SCSIDiskReq *r;
- DPRINTF("Cancel tag=0x%x\n", tag);
- r = scsi_find_request(s, tag);
- if (r) {
- if (r->req.aiocb)
- bdrv_aio_cancel(r->req.aiocb);
- r->req.aiocb = NULL;
- scsi_req_dequeue(&r->req);
+ SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+
+ DPRINTF("Cancel tag=0x%x\n", req->tag);
+ if (r->req.aiocb) {
+ bdrv_aio_cancel(r->req.aiocb);
}
+ r->req.aiocb = NULL;
+ scsi_req_dequeue(&r->req);
}
static void scsi_read_complete(void * opaque, int ret)
@@ -172,8 +166,10 @@ static void scsi_read_complete(void * opaque, int ret)
}
-static void scsi_read_request(SCSIDiskReq *r)
+/* Read more data from scsi device into buffer. */
+static void scsi_read_data(SCSIRequest *req)
{
+ SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
uint32_t n;
@@ -205,23 +201,6 @@ static void scsi_read_request(SCSIDiskReq *r)
}
}
-/* Read more data from scsi device into buffer. */
-static void scsi_read_data(SCSIDevice *d, uint32_t tag)
-{
- SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
- SCSIDiskReq *r;
-
- r = scsi_find_request(s, tag);
- if (!r) {
- BADF("Bad read tag 0x%x\n", tag);
- /* ??? This is the wrong error. */
- scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
- return;
- }
-
- scsi_read_request(r);
-}
-
static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
{
int is_read = (type == SCSI_REQ_STATUS_RETRY_READ);
@@ -283,8 +262,9 @@ static void scsi_write_complete(void * opaque, int ret)
}
}
-static void scsi_write_request(SCSIDiskReq *r)
+static int scsi_write_data(SCSIRequest *req)
{
+ SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
uint32_t n;
@@ -303,24 +283,6 @@ static void scsi_write_request(SCSIDiskReq *r)
/* Invoke completion routine to fetch data from host. */
scsi_write_complete(r, 0);
}
-}
-
-/* Write data to a scsi device. Returns nonzero on failure.
- The transfer may complete asynchronously. */
-static int scsi_write_data(SCSIDevice *d, uint32_t tag)
-{
- SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
- SCSIDiskReq *r;
-
- DPRINTF("Write data tag=0x%x\n", tag);
- r = scsi_find_request(s, tag);
- if (!r) {
- BADF("Bad write tag 0x%x\n", tag);
- scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
- return 1;
- }
-
- scsi_write_request(r);
return 0;
}
@@ -345,10 +307,10 @@ static void scsi_dma_restart_bh(void *opaque)
switch (status & SCSI_REQ_STATUS_RETRY_TYPE_MASK) {
case SCSI_REQ_STATUS_RETRY_READ:
- scsi_read_request(r);
+ scsi_read_data(&r->req);
break;
case SCSI_REQ_STATUS_RETRY_WRITE:
- scsi_write_request(r);
+ scsi_write_data(&r->req);
break;
case SCSI_REQ_STATUS_RETRY_FLUSH:
ret = scsi_disk_emulate_command(r, r->iov.iov_base);
@@ -374,16 +336,10 @@ static void scsi_dma_restart_cb(void *opaque, int running, int reason)
}
/* Return a pointer to the data buffer. */
-static uint8_t *scsi_get_buf(SCSIDevice *d, uint32_t tag)
+static uint8_t *scsi_get_buf(SCSIRequest *req)
{
- SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
- SCSIDiskReq *r;
+ SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
- r = scsi_find_request(s, tag);
- if (!r) {
- BADF("Bad buffer tag 0x%x\n", tag);
- return NULL;
- }
return (uint8_t *)r->iov.iov_base;
}
@@ -1027,26 +983,18 @@ illegal_request:
(eg. disk reads), negative for transfers to the device (eg. disk writes),
and zero if the command does not transfer any data. */
-static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
- uint8_t *buf, int lun)
+static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
{
- SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
+ SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
int32_t len;
int is_write;
uint8_t command;
uint8_t *outbuf;
- SCSIDiskReq *r;
int rc;
+ scsi_req_enqueue(req);
command = buf[0];
- r = scsi_find_request(s, tag);
- if (r) {
- BADF("Tag 0x%x already in use\n", tag);
- scsi_cancel_io(d, tag);
- }
- /* ??? Tags are not unique for different luns. We only implement a
- single lun, so this should not matter. */
- r = scsi_new_request(s, tag, lun);
outbuf = (uint8_t *)r->iov.iov_base;
is_write = 0;
DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
@@ -1065,9 +1013,9 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
}
#endif
- if (lun || buf[1] >> 5) {
+ if (req->lun || buf[1] >> 5) {
/* Only LUN 0 supported. */
- DPRINTF("Unimplemented LUN %d\n", lun ? lun : buf[1] >> 5);
+ DPRINTF("Unimplemented LUN %d\n", req->lun ? req->lun : buf[1] >> 5);
if (command != REQUEST_SENSE && command != INQUIRY)
goto fail;
}
@@ -1093,7 +1041,6 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
case REZERO_UNIT:
rc = scsi_disk_emulate_command(r, outbuf);
if (rc < 0) {
- scsi_req_unref(&r->req);
return 0;
}
@@ -1103,7 +1050,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
case READ_10:
case READ_12:
case READ_16:
- len = r->req.cmd.xfer / d->blocksize;
+ len = r->req.cmd.xfer / s->qdev.blocksize;
DPRINTF("Read (sector %" PRId64 ", count %d)\n", r->req.cmd.lba, len);
if (r->req.cmd.lba > s->max_lba)
goto illegal_lba;
@@ -1117,7 +1064,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
case WRITE_VERIFY:
case WRITE_VERIFY_12:
case WRITE_VERIFY_16:
- len = r->req.cmd.xfer / d->blocksize;
+ len = r->req.cmd.xfer / s->qdev.blocksize;
DPRINTF("Write %s(sector %" PRId64 ", count %d)\n",
(command & 0xe) == 0xe ? "And Verify " : "",
r->req.cmd.lba, len);
@@ -1152,7 +1099,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
}
break;
case WRITE_SAME_16:
- len = r->req.cmd.xfer / d->blocksize;
+ len = r->req.cmd.xfer / s->qdev.blocksize;
DPRINTF("WRITE SAME(16) (sector %" PRId64 ", count %d)\n",
r->req.cmd.lba, len);
@@ -1180,11 +1127,9 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
fail:
scsi_command_complete(r, CHECK_CONDITION, ILLEGAL_REQUEST);
- scsi_req_unref(&r->req);
return 0;
illegal_lba:
scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
- scsi_req_unref(&r->req);
return 0;
}
if (r->sector_count == 0 && r->iov.iov_len == 0) {
@@ -1197,7 +1142,6 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
if (!r->sector_count)
r->sector_count = -1;
}
- scsi_req_unref(&r->req);
return len;
}
@@ -1211,6 +1155,7 @@ static void scsi_disk_purge_requests(SCSIDiskState *s)
bdrv_aio_cancel(r->req.aiocb);
}
scsi_req_dequeue(&r->req);
+ scsi_req_unref(&r->req);
}
}
@@ -1293,6 +1238,7 @@ static SCSIDeviceInfo scsi_disk_info = {
.qdev.reset = scsi_disk_reset,
.init = scsi_disk_initfn,
.destroy = scsi_destroy,
+ .alloc_req = scsi_new_request,
.free_req = scsi_free_request,
.send_command = scsi_send_command,
.read_data = scsi_read_data,
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 5739067..e3e4187 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -66,12 +66,12 @@ struct SCSIGenericState
uint8_t senselen;
};
-static SCSIGenericReq *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun)
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun)
{
SCSIRequest *req;
req = scsi_req_alloc(sizeof(SCSIGenericReq), d, tag, lun);
- return DO_UPCAST(SCSIGenericReq, req, req);
+ return req;
}
static void scsi_free_request(SCSIRequest *req)
@@ -82,11 +82,6 @@ static void scsi_free_request(SCSIRequest *req)
scsi_req_free(&r->req);
}
-static SCSIGenericReq *scsi_find_request(SCSIGenericState *s, uint32_t tag)
-{
- return DO_UPCAST(SCSIGenericReq, req, scsi_req_find(&s->qdev, tag));
-}
-
/* Helper function for command completion. */
static void scsi_command_complete(void *opaque, int ret)
{
@@ -117,19 +112,16 @@ static void scsi_command_complete(void *opaque, int ret)
}
/* Cancel a pending data transfer. */
-static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
+static void scsi_cancel_io(SCSIRequest *req)
{
- DPRINTF("scsi_cancel_io 0x%x\n", tag);
- SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, d);
- SCSIGenericReq *r;
- DPRINTF("Cancel tag=0x%x\n", tag);
- r = scsi_find_request(s, tag);
- if (r) {
- if (r->req.aiocb)
- bdrv_aio_cancel(r->req.aiocb);
- r->req.aiocb = NULL;
- scsi_req_dequeue(&r->req);
+ SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
+
+ DPRINTF("Cancel tag=0x%x\n", req->tag);
+ if (r->req.aiocb) {
+ bdrv_aio_cancel(r->req.aiocb);
}
+ r->req.aiocb = NULL;
+ scsi_req_dequeue(&r->req);
}
static int execute_command(BlockDriverState *bdrv,
@@ -181,21 +173,13 @@ static void scsi_read_complete(void * opaque, int ret)
}
/* Read more data from scsi device into buffer. */
-static void scsi_read_data(SCSIDevice *d, uint32_t tag)
+static void scsi_read_data(SCSIRequest *req)
{
- SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, d);
- SCSIGenericReq *r;
+ SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
+ SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, r->req.dev);
int ret;
- DPRINTF("scsi_read_data 0x%x\n", tag);
- r = scsi_find_request(s, tag);
- if (!r) {
- BADF("Bad read tag 0x%x\n", tag);
- /* ??? This is the wrong error. */
- scsi_command_complete(r, -EINVAL);
- return;
- }
-
+ DPRINTF("scsi_read_data 0x%x\n", req->tag);
if (r->len == -1) {
scsi_command_complete(r, 0);
return;
@@ -247,21 +231,13 @@ static void scsi_write_complete(void * opaque, int ret)
/* Write data to a scsi device. Returns nonzero on failure.
The transfer may complete asynchronously. */
-static int scsi_write_data(SCSIDevice *d, uint32_t tag)
+static int scsi_write_data(SCSIRequest *req)
{
- SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, d);
- SCSIGenericReq *r;
+ SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, req->dev);
+ SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
int ret;
- DPRINTF("scsi_write_data 0x%x\n", tag);
- r = scsi_find_request(s, tag);
- if (!r) {
- BADF("Bad write tag 0x%x\n", tag);
- /* ??? This is the wrong error. */
- scsi_command_complete(r, -EINVAL);
- return 0;
- }
-
+ DPRINTF("scsi_write_data 0x%x\n", req->tag);
if (r->len == 0) {
r->len = r->buflen;
scsi_req_data(&r->req, r->len);
@@ -278,15 +254,10 @@ static int scsi_write_data(SCSIDevice *d, uint32_t tag)
}
/* Return a pointer to the data buffer. */
-static uint8_t *scsi_get_buf(SCSIDevice *d, uint32_t tag)
+static uint8_t *scsi_get_buf(SCSIRequest *req)
{
- SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, d);
- SCSIGenericReq *r;
- r = scsi_find_request(s, tag);
- if (!r) {
- BADF("Bad buffer tag 0x%x\n", tag);
- return NULL;
- }
+ SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
+
return r->buf;
}
@@ -314,18 +285,17 @@ static void scsi_req_fixup(SCSIRequest *req)
(eg. disk reads), negative for transfers to the device (eg. disk writes),
and zero if the command does not transfer any data. */
-static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
- uint8_t *cmd, int lun)
+static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
{
- SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, d);
- SCSIGenericReq *r;
+ SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, req->dev);
+ SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
SCSIBus *bus;
int ret;
- int32_t len;
+ scsi_req_enqueue(req);
if (cmd[0] != REQUEST_SENSE &&
- (lun != s->lun || (cmd[1] >> 5) != s->lun)) {
- DPRINTF("Unimplemented LUN %d\n", lun ? lun : cmd[1] >> 5);
+ (req->lun != s->lun || (cmd[1] >> 5) != s->lun)) {
+ DPRINTF("Unimplemented LUN %d\n", req->lun ? req->lun : cmd[1] >> 5);
s->sensebuf[0] = 0x70;
s->sensebuf[1] = 0x00;
@@ -336,18 +306,11 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
s->sensebuf[6] = 0x00;
s->senselen = 7;
s->driver_status = SG_ERR_DRIVER_SENSE;
- bus = scsi_bus_from_device(d);
- bus->ops.complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
+ bus = scsi_bus_from_device(&s->qdev);
+ bus->ops.complete(req, SCSI_REASON_DONE, CHECK_CONDITION);
return 0;
}
- r = scsi_find_request(s, tag);
- if (r) {
- BADF("Tag 0x%x already in use %p\n", tag, r);
- scsi_cancel_io(d, tag);
- }
- r = scsi_new_request(d, tag, lun);
-
if (-1 == scsi_req_parse(&r->req, cmd)) {
BADF("Unsupported command length, command %x\n", cmd[0]);
scsi_req_dequeue(&r->req);
@@ -377,10 +340,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
ret = execute_command(s->bs, r, SG_DXFER_NONE, scsi_command_complete);
if (ret == -1) {
scsi_command_complete(r, -EINVAL);
- scsi_req_unref(&r->req);
- return 0;
}
- scsi_req_unref(&r->req);
return 0;
}
@@ -395,13 +355,10 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
r->len = r->req.cmd.xfer;
if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
r->len = 0;
- len = -r->req.cmd.xfer;
+ return -r->req.cmd.xfer;
} else {
- len = r->req.cmd.xfer;
+ return r->req.cmd.xfer;
}
-
- scsi_req_unref(&r->req);
- return len;
}
static int get_blocksize(BlockDriverState *bdrv)
@@ -475,6 +432,7 @@ static void scsi_generic_purge_requests(SCSIGenericState *s)
bdrv_aio_cancel(r->req.aiocb);
}
scsi_req_dequeue(&r->req);
+ scsi_req_unref(&r->req);
}
}
@@ -566,6 +524,7 @@ static SCSIDeviceInfo scsi_generic_info = {
.qdev.reset = scsi_generic_reset,
.init = scsi_generic_initfn,
.destroy = scsi_destroy,
+ .alloc_req = scsi_new_request,
.free_req = scsi_free_request,
.send_command = scsi_send_command,
.read_data = scsi_read_data,
diff --git a/hw/scsi.h b/hw/scsi.h
index ecf1aeb..e709989 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -19,6 +19,7 @@ typedef struct SCSIBus SCSIBus;
typedef struct SCSIBusOps SCSIBusOps;
typedef struct SCSIDevice SCSIDevice;
typedef struct SCSIDeviceInfo SCSIDeviceInfo;
+typedef struct SCSIRequest SCSIRequest;
enum SCSIXferMode {
SCSI_XFER_NONE, /* TEST_UNIT_READY, ... */
@@ -26,7 +27,7 @@ enum SCSIXferMode {
SCSI_XFER_TO_DEV, /* WRITE, MODE_SELECT, ... */
};
-typedef struct SCSIRequest {
+struct SCSIRequest {
SCSIBus *bus;
SCSIDevice *dev;
uint32_t refcount;
@@ -43,7 +44,7 @@ typedef struct SCSIRequest {
BlockDriverAIOCB *aiocb;
bool enqueued;
QTAILQ_ENTRY(SCSIRequest) next;
-} SCSIRequest;
+};
struct SCSIDevice
{
@@ -66,17 +67,17 @@ struct SCSIDeviceInfo {
DeviceInfo qdev;
scsi_qdev_initfn init;
void (*destroy)(SCSIDevice *s);
+ SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun);
void (*free_req)(SCSIRequest *req);
- int32_t (*send_command)(SCSIDevice *s, uint32_t tag, uint8_t *buf,
- int lun);
- void (*read_data)(SCSIDevice *s, uint32_t tag);
- int (*write_data)(SCSIDevice *s, uint32_t tag);
- void (*cancel_io)(SCSIDevice *s, uint32_t tag);
- uint8_t *(*get_buf)(SCSIDevice *s, uint32_t tag);
+ int32_t (*send_command)(SCSIRequest *req, uint8_t *buf);
+ void (*read_data)(SCSIRequest *req);
+ int (*write_data)(SCSIRequest *req);
+ void (*cancel_io)(SCSIRequest *req);
+ uint8_t *(*get_buf)(SCSIRequest *req);
};
struct SCSIBusOps {
- void (*complete)(SCSIBus *bus, int reason, uint32_t tag, uint32_t arg);
+ void (*complete)(SCSIRequest *req, int reason, uint32_t arg);
};
struct SCSIBus {
@@ -103,7 +104,7 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
int scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun);
-SCSIRequest *scsi_req_find(SCSIDevice *d, uint32_t tag);
+void scsi_req_enqueue(SCSIRequest *req);
void scsi_req_free(SCSIRequest *req);
void scsi_req_dequeue(SCSIRequest *req);
SCSIRequest *scsi_req_ref(SCSIRequest *req);
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 9f5c154..cf2ed73 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -75,6 +75,7 @@ typedef struct vscsi_req {
/* SCSI request tracking */
SCSIDevice *sdev;
+ SCSIRequest *sreq;
uint32_t qtag; /* qemu tag != srp tag */
int lun;
int active;
@@ -123,11 +124,16 @@ static struct vscsi_req *vscsi_get_req(VSCSIState *s)
static void vscsi_put_req(VSCSIState *s, vscsi_req *req)
{
+ if (req->sreq != NULL) {
+ scsi_req_unref(req->sreq);
+ }
+ req->sreq = NULL;
req->active = 0;
}
-static vscsi_req *vscsi_find_req(VSCSIState *s, uint32_t tag)
+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;
}
@@ -453,11 +459,11 @@ static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
cdb[4] = 96;
cdb[5] = 0;
req->sensing = 1;
- n = sdev->info->send_command(sdev, req->qtag, cdb, req->lun);
+ n = sdev->info->send_command(req->sreq, cdb);
dprintf("VSCSI: Queued request sense tag 0x%x\n", req->qtag);
if (n < 0) {
fprintf(stderr, "VSCSI: REQUEST_SENSE wants write data !?!?!?\n");
- sdev->info->cancel_io(sdev, req->qtag);
+ sdev->info->cancel_io(req->sreq);
vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
vscsi_put_req(s, req);
@@ -465,24 +471,23 @@ static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
} else if (n == 0) {
return;
}
- sdev->info->read_data(sdev, req->qtag);
+ sdev->info->read_data(req->sreq);
}
/* Callback to indicate that the SCSI layer has completed a transfer. */
-static void vscsi_command_complete(SCSIBus *bus, int reason, uint32_t tag,
- uint32_t arg)
+static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
{
- VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, bus->qbus.parent);
- vscsi_req *req = vscsi_find_req(s, tag);
+ VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, sreq->bus->qbus.parent);
+ vscsi_req *req = vscsi_find_req(s, sreq);
SCSIDevice *sdev;
uint8_t *buf;
int32_t res_in = 0, res_out = 0;
int len, rc = 0;
dprintf("VSCSI: SCSI cmd complete, r=0x%x tag=0x%x arg=0x%x, req=%p\n",
- reason, tag, arg, req);
+ reason, sreq->tag, arg, req);
if (req == NULL) {
- fprintf(stderr, "VSCSI: Can't find request for tag 0x%x\n", tag);
+ fprintf(stderr, "VSCSI: Can't find request for tag 0x%x\n", sreq->tag);
return;
}
sdev = req->sdev;
@@ -493,7 +498,7 @@ static void vscsi_command_complete(SCSIBus *bus, int reason, uint32_t tag,
vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
vscsi_put_req(s, req);
} else {
- uint8_t *buf = sdev->info->get_buf(sdev, tag);
+ uint8_t *buf = sdev->info->get_buf(sreq);
len = MIN(arg, SCSI_SENSE_BUF_SIZE);
dprintf("VSCSI: Sense data, %d bytes:\n", len);
@@ -505,7 +510,7 @@ static void vscsi_command_complete(SCSIBus *bus, int reason, uint32_t tag,
buf[12], buf[13], buf[14], buf[15]);
memcpy(req->sense, buf, len);
req->senselen = len;
- sdev->info->read_data(sdev, req->qtag);
+ sdev->info->read_data(sreq);
}
return;
}
@@ -537,12 +542,12 @@ static void vscsi_command_complete(SCSIBus *bus, int reason, uint32_t tag,
* to write for writes (ie, how much is to be DMA'd)
*/
if (arg) {
- buf = sdev->info->get_buf(sdev, tag);
+ buf = sdev->info->get_buf(sreq);
rc = vscsi_srp_transfer_data(s, req, req->writing, buf, arg);
}
if (rc < 0) {
fprintf(stderr, "VSCSI: RDMA error rc=%d!\n", rc);
- sdev->info->cancel_io(sdev, req->qtag);
+ sdev->info->cancel_io(sreq);
vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
vscsi_put_req(s, req);
@@ -552,9 +557,9 @@ static void vscsi_command_complete(SCSIBus *bus, int reason, uint32_t tag,
/* Start next chunk */
req->data_len -= rc;
if (req->writing) {
- sdev->info->write_data(sdev, req->qtag);
+ sdev->info->write_data(sreq);
} else {
- sdev->info->read_data(sdev, req->qtag);
+ sdev->info->read_data(sreq);
}
}
@@ -644,7 +649,8 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
req->sdev = sdev;
req->lun = lun;
- n = sdev->info->send_command(sdev, req->qtag, srp->cmd.cdb, lun);
+ req->sreq = sdev->info->alloc_req(sdev, req->qtag, lun);
+ n = sdev->info->send_command(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);
@@ -662,10 +668,10 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
/* Get transfer direction and initiate transfer */
if (n > 0) {
req->data_len = n;
- sdev->info->read_data(sdev, req->qtag);
+ sdev->info->read_data(req->sreq);
} else if (n < 0) {
req->data_len = -n;
- sdev->info->write_data(sdev, req->qtag);
+ sdev->info->write_data(req->sreq);
}
/* Don't touch req here, it may have been recycled already */
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 7a07a12..38f97d0 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -48,6 +48,7 @@ typedef struct {
uint32_t data_len;
uint32_t residue;
uint32_t tag;
+ SCSIRequest *req;
SCSIBus bus;
BlockConf conf;
SCSIDevice *scsi_dev;
@@ -190,9 +191,9 @@ static void usb_msd_copy_data(MSDState *s)
s->data_len -= len;
if (s->scsi_len == 0 || s->data_len == 0) {
if (s->mode == USB_MSDM_DATAIN) {
- s->scsi_dev->info->read_data(s->scsi_dev, s->tag);
+ s->scsi_dev->info->read_data(s->req);
} else if (s->mode == USB_MSDM_DATAOUT) {
- s->scsi_dev->info->write_data(s->scsi_dev, s->tag);
+ s->scsi_dev->info->write_data(s->req);
}
}
}
@@ -211,14 +212,13 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
memcpy(p->data, &csw, len);
}
-static void usb_msd_command_complete(SCSIBus *bus, int reason, uint32_t tag,
- uint32_t arg)
+static void usb_msd_command_complete(SCSIRequest *req, int reason, uint32_t arg)
{
- MSDState *s = DO_UPCAST(MSDState, dev.qdev, bus->qbus.parent);
+ MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
USBPacket *p = s->packet;
- if (tag != s->tag) {
- fprintf(stderr, "usb-msd: Unexpected SCSI Tag 0x%x\n", tag);
+ if (req->tag != s->tag) {
+ fprintf(stderr, "usb-msd: Unexpected SCSI Tag 0x%x\n", req->tag);
}
if (reason == SCSI_REASON_DONE) {
DPRINTF("Command complete %d\n", arg);
@@ -245,10 +245,12 @@ static void usb_msd_command_complete(SCSIBus *bus, int reason, uint32_t tag,
} else if (s->data_len == 0) {
s->mode = USB_MSDM_CSW;
}
+ scsi_req_unref(req);
+ s->req = NULL;
return;
}
s->scsi_len = arg;
- s->scsi_buf = s->scsi_dev->info->get_buf(s->scsi_dev, tag);
+ s->scsi_buf = s->scsi_dev->info->get_buf(req);
if (p) {
usb_msd_copy_data(s);
if (s->usb_len == 0) {
@@ -316,7 +318,7 @@ static int usb_msd_handle_control(USBDevice *dev, int request, int value,
static void usb_msd_cancel_io(USBPacket *p, void *opaque)
{
MSDState *s = opaque;
- s->scsi_dev->info->cancel_io(s->scsi_dev, s->tag);
+ s->scsi_dev->info->cancel_io(s->req);
s->packet = NULL;
s->scsi_len = 0;
}
@@ -365,14 +367,15 @@ 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->scsi_dev->info->send_command(s->scsi_dev, s->tag, cbw.cmd, 0);
+ s->req = s->scsi_dev->info->alloc_req(s->scsi_dev, s->tag, 0);
+ s->scsi_dev->info->send_command(s->req, cbw.cmd);
/* ??? Should check that USB and SCSI data transfer
directions match. */
if (s->residue == 0) {
if (s->mode == USB_MSDM_DATAIN) {
- s->scsi_dev->info->read_data(s->scsi_dev, s->tag);
+ s->scsi_dev->info->read_data(s->req);
} else if (s->mode == USB_MSDM_DATAOUT) {
- s->scsi_dev->info->write_data(s->scsi_dev, s->tag);
+ s->scsi_dev->info->write_data(s->req);
}
}
ret = len;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 08/21] scsi: commonize purging requests
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (6 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 07/21] scsi: Use 'SCSIRequest' directly Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 16:00 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 09/21] scsi: introduce scsi_req_abort Paolo Bonzini
` (13 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel
The code for canceling requests upon reset is already the same. Clean
it up and move it to scsi-bus.c.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-bus.c | 12 ++++++++++++
hw/scsi-disk.c | 18 ++----------------
hw/scsi-generic.c | 18 ++----------------
hw/scsi.h | 1 +
4 files changed, 17 insertions(+), 32 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index a175590..e1bb494 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -548,6 +548,18 @@ void scsi_req_complete(SCSIRequest *req)
scsi_req_unref(req);
}
+void scsi_device_purge_requests(SCSIDevice *sdev)
+{
+ SCSIRequest *req;
+
+ while (!QTAILQ_EMPTY(&sdev->requests)) {
+ req = QTAILQ_FIRST(&sdev->requests);
+ sdev->info->cancel_io(req);
+ scsi_req_dequeue(req);
+ scsi_req_unref(req);
+ }
+}
+
static char *scsibus_get_fw_dev_path(DeviceState *dev)
{
SCSIDevice *d = (SCSIDevice*)dev;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 7446115..8962c33 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1145,26 +1145,12 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
return len;
}
-static void scsi_disk_purge_requests(SCSIDiskState *s)
-{
- SCSIDiskReq *r;
-
- while (!QTAILQ_EMPTY(&s->qdev.requests)) {
- r = DO_UPCAST(SCSIDiskReq, req, QTAILQ_FIRST(&s->qdev.requests));
- if (r->req.aiocb) {
- bdrv_aio_cancel(r->req.aiocb);
- }
- scsi_req_dequeue(&r->req);
- scsi_req_unref(&r->req);
- }
-}
-
static void scsi_disk_reset(DeviceState *dev)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev.qdev, dev);
uint64_t nb_sectors;
- scsi_disk_purge_requests(s);
+ scsi_device_purge_requests(&s->qdev);
bdrv_get_geometry(s->bs, &nb_sectors);
nb_sectors /= s->cluster_size;
@@ -1178,7 +1164,7 @@ static void scsi_destroy(SCSIDevice *dev)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
- scsi_disk_purge_requests(s);
+ scsi_device_purge_requests(&s->qdev);
blockdev_mark_auto_del(s->qdev.conf.bs);
}
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index e3e4187..896797c 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -422,32 +422,18 @@ static int get_stream_blocksize(BlockDriverState *bdrv)
return (buf[9] << 16) | (buf[10] << 8) | buf[11];
}
-static void scsi_generic_purge_requests(SCSIGenericState *s)
-{
- SCSIGenericReq *r;
-
- while (!QTAILQ_EMPTY(&s->qdev.requests)) {
- r = DO_UPCAST(SCSIGenericReq, req, QTAILQ_FIRST(&s->qdev.requests));
- if (r->req.aiocb) {
- bdrv_aio_cancel(r->req.aiocb);
- }
- scsi_req_dequeue(&r->req);
- scsi_req_unref(&r->req);
- }
-}
-
static void scsi_generic_reset(DeviceState *dev)
{
SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev.qdev, dev);
- scsi_generic_purge_requests(s);
+ scsi_device_purge_requests(&s->qdev);
}
static void scsi_destroy(SCSIDevice *d)
{
SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, d);
- scsi_generic_purge_requests(s);
+ scsi_device_purge_requests(&s->qdev);
blockdev_mark_auto_del(s->qdev.conf.bs);
}
diff --git a/hw/scsi.h b/hw/scsi.h
index e709989..dee8567 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -114,5 +114,6 @@ int scsi_req_parse(SCSIRequest *req, uint8_t *buf);
void scsi_req_print(SCSIRequest *req);
void scsi_req_data(SCSIRequest *req, int len);
void scsi_req_complete(SCSIRequest *req);
+void scsi_device_purge_requests(SCSIDevice *sdev);
#endif
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 09/21] scsi: introduce scsi_req_abort
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (7 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 08/21] scsi: commonize purging requests Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 16:00 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 10/21] scsi: introduce scsi_req_cancel Paolo Bonzini
` (12 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel
This covers the case of canceling a request's I/O and still
completing it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-bus.c | 9 +++++++++
hw/scsi.h | 1 +
hw/spapr_vscsi.c | 8 ++------
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index e1bb494..1d7da9e 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -548,6 +548,15 @@ void scsi_req_complete(SCSIRequest *req)
scsi_req_unref(req);
}
+void scsi_req_abort(SCSIRequest *req, int status)
+{
+ req->status = status;
+ if (req->dev && req->dev->info->cancel_io) {
+ req->dev->info->cancel_io(req);
+ }
+ scsi_req_complete(req);
+}
+
void scsi_device_purge_requests(SCSIDevice *sdev)
{
SCSIRequest *req;
diff --git a/hw/scsi.h b/hw/scsi.h
index dee8567..6221fff 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -114,6 +114,7 @@ int scsi_req_parse(SCSIRequest *req, uint8_t *buf);
void scsi_req_print(SCSIRequest *req);
void scsi_req_data(SCSIRequest *req, int len);
void scsi_req_complete(SCSIRequest *req);
+void scsi_req_abort(SCSIRequest *req, int status);
void scsi_device_purge_requests(SCSIDevice *sdev);
#endif
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index cf2ed73..678cd00 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -463,10 +463,8 @@ static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
dprintf("VSCSI: Queued request sense tag 0x%x\n", req->qtag);
if (n < 0) {
fprintf(stderr, "VSCSI: REQUEST_SENSE wants write data !?!?!?\n");
- sdev->info->cancel_io(req->sreq);
vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
- vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
- vscsi_put_req(s, req);
+ scsi_req_abort(req->sreq, CHECK_CONDITION);
return;
} else if (n == 0) {
return;
@@ -547,10 +545,8 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
}
if (rc < 0) {
fprintf(stderr, "VSCSI: RDMA error rc=%d!\n", rc);
- sdev->info->cancel_io(sreq);
vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
- vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
- vscsi_put_req(s, req);
+ scsi_req_abort(req->sreq, CHECK_CONDITION);
return;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 10/21] scsi: introduce scsi_req_cancel
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (8 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 09/21] scsi: introduce scsi_req_abort Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 16:01 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 11/21] scsi: use scsi_req_complete Paolo Bonzini
` (11 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Jonathan Nieder
This is for when the request must be dropped in the void,
but still memory should be freed. To this end, the devices
register a second callback in SCSIBusOps.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
hw/esp.c | 16 ++++++++++++++--
hw/lsi53c895a.c | 33 +++++++++++++++++++++++++--------
hw/scsi-bus.c | 17 ++++++++++++++---
hw/scsi-disk.c | 1 -
hw/scsi-generic.c | 1 -
hw/scsi.h | 2 ++
hw/spapr_vscsi.c | 11 ++++++++++-
hw/usb-msd.c | 19 +++++++++++++++----
8 files changed, 80 insertions(+), 20 deletions(-)
diff --git a/hw/esp.c b/hw/esp.c
index 096f4dc..46157a8 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -188,6 +188,17 @@ static void esp_dma_enable(void *opaque, int irq, int level)
}
}
+static void esp_request_cancelled(SCSIRequest *req)
+{
+ ESPState *s = DO_UPCAST(ESPState, busdev.qdev, req->bus->qbus.parent);
+
+ if (req == s->current_req) {
+ scsi_req_unref(s->current_req);
+ s->current_req = NULL;
+ s->current_dev = NULL;
+ }
+}
+
static uint32_t get_cmd(ESPState *s, uint8_t *buf)
{
uint32_t dmalen;
@@ -210,7 +221,7 @@ static uint32_t get_cmd(ESPState *s, uint8_t *buf)
if (s->current_dev) {
/* Started a new command before the old one finished. Cancel it. */
- s->current_dev->info->cancel_io(s->current_req);
+ scsi_req_cancel(s->current_req);
s->async_len = 0;
}
@@ -720,7 +731,8 @@ void esp_init(target_phys_addr_t espaddr, int it_shift,
}
static struct SCSIBusOps esp_scsi_ops = {
- .complete = esp_command_complete
+ .complete = esp_command_complete,
+ .cancel = esp_request_cancelled
};
static int esp_init1(SysBusDevice *dev)
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index b9febae..f78b85f 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -664,6 +664,26 @@ static lsi_request *lsi_find_by_tag(LSIState *s, uint32_t tag)
return NULL;
}
+static void lsi_request_cancelled(SCSIRequest *req)
+{
+ LSIState *s = DO_UPCAST(LSIState, dev.qdev, req->bus->qbus.parent);
+ lsi_request *p;
+
+ if (req == s->current->req) {
+ scsi_req_unref(req);
+ qemu_free(s->current);
+ s->current = NULL;
+ return;
+ }
+
+ p = lsi_find_by_tag(s, req->tag);
+ if (p) {
+ QTAILQ_REMOVE(&s->queue, p, next);
+ scsi_req_unref(req);
+ qemu_free(p);
+ }
+}
+
/* 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 arg)
@@ -869,7 +889,6 @@ static void lsi_do_msgout(LSIState *s)
uint8_t msg;
int len;
uint32_t current_tag;
- SCSIDevice *current_dev;
lsi_request *current_req, *p, *p_next;
int id;
@@ -880,8 +899,6 @@ static void lsi_do_msgout(LSIState *s)
current_tag = s->select_tag;
current_req = lsi_find_by_tag(s, current_tag);
}
- id = (current_tag >> 8) & 0xf;
- current_dev = s->bus.devs[id];
DPRINTF("MSG out len=%d\n", s->dbc);
while (s->dbc) {
@@ -931,7 +948,7 @@ static void lsi_do_msgout(LSIState *s)
/* The ABORT TAG message clears the current I/O process only. */
DPRINTF("MSG: ABORT TAG tag=0x%x\n", current_tag);
if (current_req) {
- current_dev->info->cancel_io(current_req->req);
+ scsi_req_cancel(current_req->req);
}
lsi_disconnect(s);
break;
@@ -956,7 +973,7 @@ static void lsi_do_msgout(LSIState *s)
/* clear the current I/O process */
if (s->current) {
- current_dev->info->cancel_io(s->current->req);
+ scsi_req_cancel(s->current->req);
}
/* As the current implemented devices scsi_disk and scsi_generic
@@ -969,8 +986,7 @@ static void lsi_do_msgout(LSIState *s)
id = current_tag & 0x0000ff00;
QTAILQ_FOREACH_SAFE(p, &s->queue, next, p_next) {
if ((p->tag & 0x0000ff00) == id) {
- current_dev->info->cancel_io(p->req);
- QTAILQ_REMOVE(&s->queue, p, next);
+ scsi_req_cancel(p->req);
}
}
@@ -2227,7 +2243,8 @@ static int lsi_scsi_uninit(PCIDevice *d)
}
static struct SCSIBusOps lsi_scsi_ops = {
- .complete = lsi_command_complete
+ .complete = lsi_command_complete,
+ .cancel = lsi_request_cancelled
};
static int lsi_scsi_init(PCIDevice *dev)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 1d7da9e..6a8ea0b 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -548,6 +548,19 @@ void scsi_req_complete(SCSIRequest *req)
scsi_req_unref(req);
}
+void scsi_req_cancel(SCSIRequest *req)
+{
+ if (req->dev && req->dev->info->cancel_io) {
+ req->dev->info->cancel_io(req);
+ }
+ scsi_req_ref(req);
+ scsi_req_dequeue(req);
+ if (req->bus->ops.cancel) {
+ req->bus->ops.cancel(req);
+ }
+ scsi_req_unref(req);
+}
+
void scsi_req_abort(SCSIRequest *req, int status)
{
req->status = status;
@@ -563,9 +576,7 @@ void scsi_device_purge_requests(SCSIDevice *sdev)
while (!QTAILQ_EMPTY(&sdev->requests)) {
req = QTAILQ_FIRST(&sdev->requests);
- sdev->info->cancel_io(req);
- scsi_req_dequeue(req);
- scsi_req_unref(req);
+ scsi_req_cancel(req);
}
}
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 8962c33..0921c62 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -141,7 +141,6 @@ static void scsi_cancel_io(SCSIRequest *req)
bdrv_aio_cancel(r->req.aiocb);
}
r->req.aiocb = NULL;
- scsi_req_dequeue(&r->req);
}
static void scsi_read_complete(void * opaque, int ret)
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 896797c..5bfbb8a 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -121,7 +121,6 @@ static void scsi_cancel_io(SCSIRequest *req)
bdrv_aio_cancel(r->req.aiocb);
}
r->req.aiocb = NULL;
- scsi_req_dequeue(&r->req);
}
static int execute_command(BlockDriverState *bdrv,
diff --git a/hw/scsi.h b/hw/scsi.h
index 6221fff..ee16638 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -78,6 +78,7 @@ struct SCSIDeviceInfo {
struct SCSIBusOps {
void (*complete)(SCSIRequest *req, int reason, uint32_t arg);
+ void (*cancel)(SCSIRequest *req);
};
struct SCSIBus {
@@ -115,6 +116,7 @@ void scsi_req_print(SCSIRequest *req);
void scsi_req_data(SCSIRequest *req, int len);
void scsi_req_complete(SCSIRequest *req);
void scsi_req_abort(SCSIRequest *req, int status);
+void scsi_req_cancel(SCSIRequest *req);
void scsi_device_purge_requests(SCSIDevice *sdev);
#endif
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 678cd00..5b97a83 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -559,6 +559,14 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
}
}
+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_put_req(s, req);
+}
+
static void vscsi_process_login(VSCSIState *s, vscsi_req *req)
{
union viosrp_iu *iu = &req->iu;
@@ -910,7 +918,8 @@ static int vscsi_do_crq(struct VIOsPAPRDevice *dev, uint8_t *crq_data)
}
static struct SCSIBusOps vscsi_scsi_ops = {
- .complete = vscsi_command_complete
+ .complete = vscsi_command_complete,
+ .cancel = vscsi_request_cancelled
};
static int spapr_vscsi_init(VIOsPAPRDevice *dev)
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 38f97d0..9b238e8 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -264,6 +264,18 @@ static void usb_msd_command_complete(SCSIRequest *req, int reason, uint32_t arg)
}
}
+static void usb_msd_request_cancelled(SCSIRequest *req)
+{
+ MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
+
+ if (req == s->req) {
+ scsi_req_unref(s->req);
+ s->req = NULL;
+ s->packet = NULL;
+ s->scsi_len = 0;
+ }
+}
+
static void usb_msd_handle_reset(USBDevice *dev)
{
MSDState *s = (MSDState *)dev;
@@ -318,9 +330,7 @@ static int usb_msd_handle_control(USBDevice *dev, int request, int value,
static void usb_msd_cancel_io(USBPacket *p, void *opaque)
{
MSDState *s = opaque;
- s->scsi_dev->info->cancel_io(s->req);
- s->packet = NULL;
- s->scsi_len = 0;
+ scsi_req_cancel(s->req);
}
static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
@@ -491,7 +501,8 @@ static void usb_msd_password_cb(void *opaque, int err)
}
static struct SCSIBusOps usb_msd_scsi_ops = {
- .complete = usb_msd_command_complete
+ .complete = usb_msd_command_complete,
+ .cancel = usb_msd_request_cancelled
};
static int usb_msd_initfn(USBDevice *dev)
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 11/21] scsi: use scsi_req_complete
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (9 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 10/21] scsi: introduce scsi_req_cancel Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 16:01 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 12/21] scsi: Update sense code handling Paolo Bonzini
` (10 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-generic.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 5bfbb8a..e1f8a8a 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -288,7 +288,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
{
SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, req->dev);
SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
- SCSIBus *bus;
int ret;
scsi_req_enqueue(req);
@@ -305,8 +304,8 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
s->sensebuf[6] = 0x00;
s->senselen = 7;
s->driver_status = SG_ERR_DRIVER_SENSE;
- bus = scsi_bus_from_device(&s->qdev);
- bus->ops.complete(req, SCSI_REASON_DONE, CHECK_CONDITION);
+ r->req.status = CHECK_CONDITION;
+ scsi_req_complete(&r->req);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 12/21] scsi: Update sense code handling
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (10 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 11/21] scsi: use scsi_req_complete Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 16:02 ` Christoph Hellwig
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly Paolo Bonzini
` (9 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Hannes Reinecke, Christoph Hellwig
From: Hannes Reinecke <hare@suse.de>
The SCSI spec has a quite detailed list of sense codes available.
It even mandates the use of specific ones for some failure cases.
The current implementation just has one type of generic error
which is actually a violation of the spec in certain cases.
This patch introduces various predefined sense codes to have the
sense code reporting more in line with the spec.
On top of Hannes's patch I fixed the reply to REQUEST SENSE commands
with DESC=0 and a small (<18) length.
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
---
hw/scsi-bus.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
hw/scsi-disk.c | 82 ++++++++++++++++++++++-------------------------
hw/scsi-generic.c | 63 ++++++++++++++++++++++++-------------
hw/scsi.h | 39 ++++++++++++++++++++++-
4 files changed, 208 insertions(+), 67 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 6a8ea0b..b83dd88 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -154,7 +154,7 @@ void scsi_req_enqueue(SCSIRequest *req)
QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
}
-void scsi_req_dequeue(SCSIRequest *req)
+static void scsi_req_dequeue(SCSIRequest *req)
{
trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag);
if (req->enqueued) {
@@ -398,6 +398,95 @@ int scsi_req_parse(SCSIRequest *req, uint8_t *buf)
return 0;
}
+/*
+ * Predefined sense codes
+ */
+
+/* No sense data available */
+const struct SCSISense sense_code_NO_SENSE = {
+ .key = NO_SENSE , .asc = 0x00 , .ascq = 0x00
+};
+
+/* LUN not ready, Manual intervention required */
+const struct SCSISense sense_code_LUN_NOT_READY = {
+ .key = NOT_READY, .asc = 0x04, .ascq = 0x03
+};
+
+/* LUN not ready, Medium not present */
+const struct SCSISense sense_code_NO_MEDIUM = {
+ .key = NOT_READY, .asc = 0x3a, .ascq = 0x00
+};
+
+/* Hardware error, internal target failure */
+const struct SCSISense sense_code_TARGET_FAILURE = {
+ .key = HARDWARE_ERROR, .asc = 0x44, .ascq = 0x00
+};
+
+/* Illegal request, invalid command operation code */
+const struct SCSISense sense_code_INVALID_OPCODE = {
+ .key = ILLEGAL_REQUEST, .asc = 0x20, .ascq = 0x00
+};
+
+/* Illegal request, LBA out of range */
+const struct SCSISense sense_code_LBA_OUT_OF_RANGE = {
+ .key = ILLEGAL_REQUEST, .asc = 0x21, .ascq = 0x00
+};
+
+/* Illegal request, Invalid field in CDB */
+const struct SCSISense sense_code_INVALID_FIELD = {
+ .key = ILLEGAL_REQUEST, .asc = 0x24, .ascq = 0x00
+};
+
+/* Illegal request, LUN not supported */
+const struct SCSISense sense_code_LUN_NOT_SUPPORTED = {
+ .key = ILLEGAL_REQUEST, .asc = 0x25, .ascq = 0x00
+};
+
+/* Command aborted, I/O process terminated */
+const struct SCSISense sense_code_IO_ERROR = {
+ .key = ABORTED_COMMAND, .asc = 0x00, .ascq = 0x06
+};
+
+/* Command aborted, I_T Nexus loss occurred */
+const struct SCSISense sense_code_I_T_NEXUS_LOSS = {
+ .key = ABORTED_COMMAND, .asc = 0x29, .ascq = 0x07
+};
+
+/* Command aborted, Logical Unit failure */
+const struct SCSISense sense_code_LUN_FAILURE = {
+ .key = ABORTED_COMMAND, .asc = 0x3e, .ascq = 0x01
+};
+
+/*
+ * scsi_build_sense
+ *
+ * Build a sense buffer
+ */
+int scsi_build_sense(SCSISense sense, uint8_t *buf, int len, int fixed)
+{
+ if (!fixed && len < 8) {
+ return 0;
+ }
+
+ memset(buf, 0, len);
+ if (fixed) {
+ /* Return fixed format sense buffer */
+ buf[0] = 0xf0;
+ buf[2] = sense.key;
+ buf[7] = 7;
+ buf[12] = sense.asc;
+ buf[13] = sense.ascq;
+ return MIN(len, 18);
+ } else {
+ /* Return descriptor format sense buffer */
+ buf[0] = 0x72;
+ buf[1] = sense.key;
+ buf[2] = sense.asc;
+ buf[3] = sense.ascq;
+ return 8;
+ }
+}
+
static const char *scsi_command_name(uint8_t cmd)
{
static const char *names[] = {
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 0921c62..a82753f 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -49,10 +49,6 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
typedef struct SCSIDiskState SCSIDiskState;
-typedef struct SCSISense {
- uint8_t key;
-} SCSISense;
-
typedef struct SCSIDiskReq {
SCSIRequest req;
/* ??? We should probably keep track of whether the data transfer is
@@ -109,24 +105,19 @@ static void scsi_disk_clear_sense(SCSIDiskState *s)
memset(&s->sense, 0, sizeof(s->sense));
}
-static void scsi_disk_set_sense(SCSIDiskState *s, uint8_t key)
-{
- s->sense.key = key;
-}
-
-static void scsi_req_set_status(SCSIDiskReq *r, int status, int sense_code)
+static void scsi_req_set_status(SCSIDiskReq *r, int status, SCSISense sense)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
r->req.status = status;
- scsi_disk_set_sense(s, sense_code);
+ s->sense = sense;
}
/* Helper function for command completion. */
-static void scsi_command_complete(SCSIDiskReq *r, int status, int sense)
+static void scsi_command_complete(SCSIDiskReq *r, int status, SCSISense sense)
{
- DPRINTF("Command complete tag=0x%x status=%d sense=%d\n",
- r->req.tag, status, sense);
+ DPRINTF("Command complete tag=0x%x status=%d sense=%d/%d/%d\n",
+ r->req.tag, status, sense.key, sense.asc, sense.ascq);
scsi_req_set_status(r, status, sense);
scsi_req_complete(&r->req);
}
@@ -180,7 +171,7 @@ static void scsi_read_data(SCSIRequest *req)
}
DPRINTF("Read sector_count=%d\n", r->sector_count);
if (r->sector_count == 0) {
- scsi_command_complete(r, GOOD, NO_SENSE);
+ scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE));
return;
}
@@ -223,8 +214,13 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
if (type == SCSI_REQ_STATUS_RETRY_READ) {
scsi_req_data(&r->req, 0);
}
- scsi_command_complete(r, CHECK_CONDITION,
- HARDWARE_ERROR);
+ if (error == ENOMEM) {
+ scsi_command_complete(r, CHECK_CONDITION,
+ SENSE_CODE(TARGET_FAILURE));
+ } else {
+ scsi_command_complete(r, CHECK_CONDITION,
+ SENSE_CODE(IO_ERROR));
+ }
bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
}
@@ -249,7 +245,7 @@ static void scsi_write_complete(void * opaque, int ret)
r->sector += n;
r->sector_count -= n;
if (r->sector_count == 0) {
- scsi_command_complete(r, GOOD, NO_SENSE);
+ scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE));
} else {
len = r->sector_count * 512;
if (len > SCSI_DMA_BUF_SIZE) {
@@ -276,7 +272,7 @@ static int scsi_write_data(SCSIRequest *req)
r->req.aiocb = bdrv_aio_writev(s->bs, r->sector, &r->qiov, n,
scsi_write_complete, r);
if (r->req.aiocb == NULL) {
- scsi_write_complete(r, -EIO);
+ scsi_write_complete(r, -ENOMEM);
}
} else {
/* Invoke completion routine to fetch data from host. */
@@ -314,7 +310,7 @@ static void scsi_dma_restart_bh(void *opaque)
case SCSI_REQ_STATUS_RETRY_FLUSH:
ret = scsi_disk_emulate_command(r, r->iov.iov_base);
if (ret == 0) {
- scsi_command_complete(r, GOOD, NO_SENSE);
+ scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE));
}
}
}
@@ -813,19 +809,8 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
case REQUEST_SENSE:
if (req->cmd.xfer < 4)
goto illegal_request;
- memset(outbuf, 0, 4);
- buflen = 4;
- if (s->sense.key == NOT_READY && req->cmd.xfer >= 18) {
- memset(outbuf, 0, 18);
- buflen = 18;
- outbuf[7] = 10;
- /* asc 0x3a, ascq 0: Medium not present */
- outbuf[12] = 0x3a;
- outbuf[13] = 0;
- }
- outbuf[0] = 0xf0;
- outbuf[1] = 0;
- outbuf[2] = s->sense.key;
+ buflen = scsi_build_sense(s->sense, outbuf, req->cmd.xfer,
+ req->cmd.xfer > 13);
scsi_disk_clear_sense(s);
break;
case INQUIRY:
@@ -963,17 +948,22 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
}
break;
default:
- goto illegal_request;
+ scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_OPCODE));
+ return -1;
}
- scsi_req_set_status(r, GOOD, NO_SENSE);
+ scsi_req_set_status(r, GOOD, SENSE_CODE(NO_SENSE));
return buflen;
not_ready:
- scsi_command_complete(r, CHECK_CONDITION, NOT_READY);
+ if (!bdrv_is_inserted(s->bs)) {
+ scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(NO_MEDIUM));
+ } else {
+ scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(LUN_NOT_READY));
+ }
return -1;
illegal_request:
- scsi_command_complete(r, CHECK_CONDITION, ILLEGAL_REQUEST);
+ scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_FIELD));
return -1;
}
@@ -1000,7 +990,8 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
if (scsi_req_parse(&r->req, buf) != 0) {
BADF("Unsupported command length, command %x\n", command);
- goto fail;
+ scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_OPCODE));
+ return 0;
}
#ifdef DEBUG_SCSI
{
@@ -1015,8 +1006,11 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
if (req->lun || buf[1] >> 5) {
/* Only LUN 0 supported. */
DPRINTF("Unimplemented LUN %d\n", req->lun ? req->lun : buf[1] >> 5);
- if (command != REQUEST_SENSE && command != INQUIRY)
- goto fail;
+ if (command != REQUEST_SENSE && command != INQUIRY) {
+ scsi_command_complete(r, CHECK_CONDITION,
+ SENSE_CODE(LUN_NOT_SUPPORTED));
+ return 0;
+ }
}
switch (command) {
case TEST_UNIT_READY:
@@ -1124,15 +1118,17 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
break;
default:
DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
+ scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_OPCODE));
+ return 0;
fail:
- scsi_command_complete(r, CHECK_CONDITION, ILLEGAL_REQUEST);
+ scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_FIELD));
return 0;
illegal_lba:
- scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
+ scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(LBA_OUT_OF_RANGE));
return 0;
}
if (r->sector_count == 0 && r->iov.iov_len == 0) {
- scsi_command_complete(r, GOOD, NO_SENSE);
+ scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE));
}
len = r->sector_count * 512 + r->iov.iov_len;
if (is_write) {
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index e1f8a8a..b934ba4 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -66,6 +66,19 @@ struct SCSIGenericState
uint8_t senselen;
};
+static void scsi_set_sense(SCSIGenericState *s, SCSISense sense)
+{
+ s->senselen = scsi_build_sense(sense, s->sensebuf, SCSI_SENSE_BUF_SIZE, 0);
+ s->driver_status = SG_ERR_DRIVER_SENSE;
+}
+
+static void scsi_clear_sense(SCSIGenericState *s)
+{
+ memset(s->sensebuf, 0, SCSI_SENSE_BUF_SIZE);
+ s->senselen = 0;
+ s->driver_status = 0;
+}
+
static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun)
{
SCSIRequest *req;
@@ -92,9 +105,22 @@ static void scsi_command_complete(void *opaque, int ret)
if (s->driver_status & SG_ERR_DRIVER_SENSE)
s->senselen = r->io_header.sb_len_wr;
- if (ret != 0)
- r->req.status = BUSY;
- else {
+ if (ret != 0) {
+ switch (ret) {
+ case -EINVAL:
+ r->req.status = CHECK_CONDITION;
+ scsi_set_sense(s, SENSE_CODE(INVALID_FIELD));
+ break;
+ case -ENOMEM:
+ r->req.status = CHECK_CONDITION;
+ scsi_set_sense(s, SENSE_CODE(TARGET_FAILURE));
+ break;
+ default:
+ r->req.status = CHECK_CONDITION;
+ scsi_set_sense(s, SENSE_CODE(IO_ERROR));
+ break;
+ }
+ } else {
if (s->driver_status & SG_ERR_DRIVER_TIMEOUT) {
r->req.status = BUSY;
BADF("Driver Timeout\n");
@@ -144,7 +170,7 @@ static int execute_command(BlockDriverState *bdrv,
r->req.aiocb = bdrv_aio_ioctl(bdrv, SG_IO, &r->io_header, complete, r);
if (r->req.aiocb == NULL) {
BADF("execute_command: read failed !\n");
- return -1;
+ return -ENOMEM;
}
return 0;
@@ -197,12 +223,14 @@ static void scsi_read_data(SCSIRequest *req)
r->buf[0], r->buf[1], r->buf[2], r->buf[3],
r->buf[4], r->buf[5], r->buf[6], r->buf[7]);
scsi_req_data(&r->req, s->senselen);
+ /* Clear sensebuf after REQUEST_SENSE */
+ scsi_clear_sense(s);
return;
}
ret = execute_command(s->bs, r, SG_DXFER_FROM_DEV, scsi_read_complete);
- if (ret == -1) {
- scsi_command_complete(r, -EINVAL);
+ if (ret < 0) {
+ scsi_command_complete(r, ret);
return;
}
}
@@ -244,8 +272,8 @@ static int scsi_write_data(SCSIRequest *req)
}
ret = execute_command(s->bs, r, SG_DXFER_TO_DEV, scsi_write_complete);
- if (ret == -1) {
- scsi_command_complete(r, -EINVAL);
+ if (ret < 0) {
+ scsi_command_complete(r, ret);
return 1;
}
@@ -294,16 +322,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
if (cmd[0] != REQUEST_SENSE &&
(req->lun != s->lun || (cmd[1] >> 5) != s->lun)) {
DPRINTF("Unimplemented LUN %d\n", req->lun ? req->lun : cmd[1] >> 5);
-
- s->sensebuf[0] = 0x70;
- s->sensebuf[1] = 0x00;
- s->sensebuf[2] = ILLEGAL_REQUEST;
- s->sensebuf[3] = 0x00;
- s->sensebuf[4] = 0x00;
- s->sensebuf[5] = 0x00;
- s->sensebuf[6] = 0x00;
- s->senselen = 7;
- s->driver_status = SG_ERR_DRIVER_SENSE;
+ scsi_set_sense(s, SENSE_CODE(LUN_NOT_SUPPORTED));
r->req.status = CHECK_CONDITION;
scsi_req_complete(&r->req);
return 0;
@@ -311,8 +330,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
if (-1 == scsi_req_parse(&r->req, cmd)) {
BADF("Unsupported command length, command %x\n", cmd[0]);
- scsi_req_dequeue(&r->req);
- scsi_req_unref(&r->req);
+ scsi_command_complete(r, -EINVAL);
return 0;
}
scsi_req_fixup(&r->req);
@@ -336,8 +354,9 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
r->buflen = 0;
r->buf = NULL;
ret = execute_command(s->bs, r, SG_DXFER_NONE, scsi_command_complete);
- if (ret == -1) {
- scsi_command_complete(r, -EINVAL);
+ if (ret < 0) {
+ scsi_command_complete(r, ret);
+ return 0;
}
return 0;
}
diff --git a/hw/scsi.h b/hw/scsi.h
index ee16638..61ab7c9 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -27,6 +27,12 @@ enum SCSIXferMode {
SCSI_XFER_TO_DEV, /* WRITE, MODE_SELECT, ... */
};
+typedef struct SCSISense {
+ uint8_t key;
+ uint8_t asc;
+ uint8_t ascq;
+} SCSISense;
+
struct SCSIRequest {
SCSIBus *bus;
SCSIDevice *dev;
@@ -104,10 +110,41 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
int unit, bool removable);
int scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
+/*
+ * Predefined sense codes
+ */
+
+/* No sense data available */
+extern const struct SCSISense sense_code_NO_SENSE;
+/* LUN not ready, Manual intervention required */
+extern const struct SCSISense sense_code_LUN_NOT_READY;
+/* LUN not ready, Medium not present */
+extern const struct SCSISense sense_code_NO_MEDIUM;
+/* Hardware error, internal target failure */
+extern const struct SCSISense sense_code_TARGET_FAILURE;
+/* Illegal request, invalid command operation code */
+extern const struct SCSISense sense_code_INVALID_OPCODE;
+/* Illegal request, LBA out of range */
+extern const struct SCSISense sense_code_LBA_OUT_OF_RANGE;
+/* Illegal request, Invalid field in CDB */
+extern const struct SCSISense sense_code_INVALID_FIELD;
+/* Illegal request, LUN not supported */
+extern const struct SCSISense sense_code_LUN_NOT_SUPPORTED;
+/* Command aborted, I/O process terminated */
+extern const struct SCSISense sense_code_IO_ERROR;
+/* Command aborted, I_T Nexus loss occurred */
+extern const struct SCSISense sense_code_I_T_NEXUS_LOSS;
+/* Command aborted, Logical Unit failure */
+extern const struct SCSISense sense_code_LUN_FAILURE;
+
+#define SENSE_CODE(x) sense_code_ ## x
+
+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);
void scsi_req_enqueue(SCSIRequest *req);
void scsi_req_free(SCSIRequest *req);
-void scsi_req_dequeue(SCSIRequest *req);
SCSIRequest *scsi_req_ref(SCSIRequest *req);
void scsi_req_unref(SCSIRequest *req);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (11 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 12/21] scsi: Update sense code handling Paolo Bonzini
@ 2011-05-17 11:00 ` Paolo Bonzini
2011-05-20 16:04 ` Christoph Hellwig
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 14/21] scsi: introduce scsi_req_new Paolo Bonzini
` (8 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:00 UTC (permalink / raw)
To: qemu-devel
Move the common part of scsi-disk.c and scsi-generic.c to the SCSI layer.
At the same time, protect against the request being freed under the
feet of the send_command callback.
This fixes a use-after-free that happened when scsi-disk's
scsi_disk_emulate_command completed an illegal request, and still its
caller scsi_send_command accessed r->sector_count and r->iov.iov_len.
The return value from scsi_send_command was then bogus; the HBA device
model mistook the completed request for an I/O request and typically
SIGSEGVed on a NULL pointer access to the current request.
Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Tested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/esp.c | 2 +-
hw/lsi53c895a.c | 2 +-
hw/scsi-bus.c | 9 ++++++++-
hw/scsi-disk.c | 1 -
hw/scsi-generic.c | 1 -
hw/scsi.h | 2 +-
hw/spapr_vscsi.c | 4 ++--
hw/usb-msd.c | 2 +-
8 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/hw/esp.c b/hw/esp.c
index 46157a8..3a65aed 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -245,7 +245,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 = s->current_dev->info->alloc_req(s->current_dev, 0, lun);
- datalen = s->current_dev->info->send_command(s->current_req, buf);
+ datalen = scsi_req_enqueue(s->current_req, buf);
s->ti_size = datalen;
if (datalen != 0) {
s->rregs[ESP_RSTAT] = STAT_TC;
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index f78b85f..b2dbcaa 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -791,7 +791,7 @@ static void lsi_do_command(LSIState *s)
s->current->req = dev->info->alloc_req(dev, s->current->tag,
s->current_lun);
- n = dev->info->send_command(s->current->req, buf);
+ n = scsi_req_enqueue(s->current->req, buf);
if (n > 0) {
lsi_set_phase(s, PHASE_DI);
dev->info->read_data(s->current->req);
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index b83dd88..0b54a4c 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -146,12 +146,19 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
return req;
}
-void scsi_req_enqueue(SCSIRequest *req)
+int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
{
+ int32_t rc;
assert(!req->enqueued);
scsi_req_ref(req);
req->enqueued = true;
QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
+
+ /* Make sure the request doesn't disappear under send_command's feet. */
+ scsi_req_ref(req);
+ rc = req->dev->info->send_command(req, buf);
+ scsi_req_unref(req);
+ return rc;
}
static void scsi_req_dequeue(SCSIRequest *req)
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index a82753f..efb953b 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -982,7 +982,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
uint8_t *outbuf;
int rc;
- scsi_req_enqueue(req);
command = buf[0];
outbuf = (uint8_t *)r->iov.iov_base;
is_write = 0;
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index b934ba4..036ab9f 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -318,7 +318,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
int ret;
- scsi_req_enqueue(req);
if (cmd[0] != REQUEST_SENSE &&
(req->lun != s->lun || (cmd[1] >> 5) != s->lun)) {
DPRINTF("Unimplemented LUN %d\n", req->lun ? req->lun : cmd[1] >> 5);
diff --git a/hw/scsi.h b/hw/scsi.h
index 61ab7c9..c36c5cc 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -143,7 +143,7 @@ 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);
-void scsi_req_enqueue(SCSIRequest *req);
+int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf);
void scsi_req_free(SCSIRequest *req);
SCSIRequest *scsi_req_ref(SCSIRequest *req);
void scsi_req_unref(SCSIRequest *req);
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 5b97a83..307a17f 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -459,7 +459,7 @@ static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
cdb[4] = 96;
cdb[5] = 0;
req->sensing = 1;
- n = sdev->info->send_command(req->sreq, cdb);
+ n = scsi_req_enqueue(req->sreq, cdb);
dprintf("VSCSI: Queued request sense tag 0x%x\n", req->qtag);
if (n < 0) {
fprintf(stderr, "VSCSI: REQUEST_SENSE wants write data !?!?!?\n");
@@ -654,7 +654,7 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
req->sdev = sdev;
req->lun = lun;
req->sreq = sdev->info->alloc_req(sdev, req->qtag, lun);
- n = sdev->info->send_command(req->sreq, srp->cmd.cdb);
+ 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);
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 9b238e8..1375e82 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -378,7 +378,7 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
s->residue = 0;
s->scsi_len = 0;
s->req = s->scsi_dev->info->alloc_req(s->scsi_dev, s->tag, 0);
- s->scsi_dev->info->send_command(s->req, cbw.cmd);
+ scsi_req_enqueue(s->req, cbw.cmd);
/* ??? Should check that USB and SCSI data transfer
directions match. */
if (s->residue == 0) {
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly Paolo Bonzini
@ 2011-05-20 16:04 ` Christoph Hellwig
2011-05-20 17:43 ` Paolo Bonzini
0 siblings, 1 reply; 52+ messages in thread
From: Christoph Hellwig @ 2011-05-20 16:04 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
> -void scsi_req_enqueue(SCSIRequest *req)
> +int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
> {
> + int32_t rc;
> assert(!req->enqueued);
> scsi_req_ref(req);
> req->enqueued = true;
> QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
> +
> + /* Make sure the request doesn't disappear under send_command's feet. */
> + scsi_req_ref(req);
> + rc = req->dev->info->send_command(req, buf);
> + scsi_req_unref(req);
> + return rc;
How would it disappear given that we grabbed another reference just before?
That probably needs a bit more documentation here. Also why not move
the two scsi_req_ref calls together?
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly
2011-05-20 16:04 ` Christoph Hellwig
@ 2011-05-20 17:43 ` Paolo Bonzini
2011-05-24 13:05 ` Kevin Wolf
0 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-20 17:43 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: qemu-devel
On 05/20/2011 06:04 PM, Christoph Hellwig wrote:
>> -void scsi_req_enqueue(SCSIRequest *req)
>> +int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
>> {
>> + int32_t rc;
>> assert(!req->enqueued);
>> scsi_req_ref(req);
>> req->enqueued = true;
>> QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
>> +
>> + /* Make sure the request doesn't disappear under send_command's feet. */
>> + scsi_req_ref(req);
>> + rc = req->dev->info->send_command(req, buf);
>> + scsi_req_unref(req);
>> + return rc;
>
> How would it disappear given that we grabbed another reference just before?
Welcome to the wonderful world of nested callbacks. :(
Suppose send_command completes a request. scsi_req_complete then
dequeues it (which undoes the reference above), and calls the device who
owned it. The device who owned the request then presumably NULLs a
pointer and drops the last reference. The request is then freed.
This was exactly the kind of scenario that I was worried about when I
thought about reference counting. I couldn't actually put my fingers on
it, but I knew it was broken somewhere, and indeed a use-after-free was
reported less than a month later.
It is not strictly necessary to wrap send_command with a ref/unref pair,
but it is quite convenient when writing send_command. It also mirrors
what I do around scsi_req_complete and scsi_req_cancel.
> That probably needs a bit more documentation here. Also why not move
> the two scsi_req_ref calls together?
Because one is logically related to putting the request in the queue,
while the other is related to the send_command op.
Paolo
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly
2011-05-20 17:43 ` Paolo Bonzini
@ 2011-05-24 13:05 ` Kevin Wolf
2011-05-24 13:13 ` Paolo Bonzini
0 siblings, 1 reply; 52+ messages in thread
From: Kevin Wolf @ 2011-05-24 13:05 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Christoph Hellwig, qemu-devel
Am 20.05.2011 19:43, schrieb Paolo Bonzini:
> On 05/20/2011 06:04 PM, Christoph Hellwig wrote:
>>> -void scsi_req_enqueue(SCSIRequest *req)
>>> +int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
>>> {
>>> + int32_t rc;
>>> assert(!req->enqueued);
>>> scsi_req_ref(req);
>>> req->enqueued = true;
>>> QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
>>> +
>>> + /* Make sure the request doesn't disappear under send_command's feet. */
>>> + scsi_req_ref(req);
>>> + rc = req->dev->info->send_command(req, buf);
>>> + scsi_req_unref(req);
>>> + return rc;
>>
>> How would it disappear given that we grabbed another reference just before?
>
> Welcome to the wonderful world of nested callbacks. :(
>
> Suppose send_command completes a request. scsi_req_complete then
> dequeues it (which undoes the reference above), and calls the device who
> owned it. The device who owned the request then presumably NULLs a
> pointer and drops the last reference. The request is then freed.
Maybe the callback should be done from a BH then? It sounds like this
could cause more bugs than what you're fixing here.
Kevin
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly
2011-05-24 13:05 ` Kevin Wolf
@ 2011-05-24 13:13 ` Paolo Bonzini
0 siblings, 0 replies; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-24 13:13 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Christoph Hellwig, qemu-devel
On 05/24/2011 03:05 PM, Kevin Wolf wrote:
> Maybe the callback should be done from a BH then? It sounds like this
> could cause more bugs than what you're fixing here.
Not sure, after all it makes sense to answer some queries synchronously
(e.g. TEST_UNIT_READY). It's just the convoluted control flow that
tricked you when you moved accesses to after the request has been
completed. With reference counting, the data in the SCSIRequest remains
completely valid after it has been completed and until the last ref goes
away, so I see no reason to complicate the logic further by introducing
a BH.
Paolo
^ permalink raw reply [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v3 14/21] scsi: introduce scsi_req_new
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (12 preceding siblings ...)
2011-05-17 11:00 ` [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly Paolo Bonzini
@ 2011-05-17 11:01 ` Paolo Bonzini
2011-05-20 16:04 ` Christoph Hellwig
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 15/21] scsi: introduce scsi_req_kick Paolo Bonzini
` (7 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:01 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/esp.c | 2 +-
hw/lsi53c895a.c | 3 +--
hw/scsi-bus.c | 5 +++++
hw/scsi.h | 1 +
hw/spapr_vscsi.c | 2 +-
hw/usb-msd.c | 2 +-
6 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/hw/esp.c b/hw/esp.c
index 3a65aed..ad364b5 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 = s->current_dev->info->alloc_req(s->current_dev, 0, lun);
+ s->current_req = scsi_req_new(s->current_dev, 0, lun);
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 b2dbcaa..e2af25f 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -788,8 +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 = dev->info->alloc_req(dev, s->current->tag,
- s->current_lun);
+ s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun);
n = scsi_req_enqueue(s->current->req, buf);
if (n > 0) {
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 0b54a4c..c091159 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -146,6 +146,11 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
return req;
}
+SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun)
+{
+ return d->info->alloc_req(d, tag, lun);
+}
+
int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
{
int32_t rc;
diff --git a/hw/scsi.h b/hw/scsi.h
index c36c5cc..e44c194 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -143,6 +143,7 @@ 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);
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 307a17f..1bb4bf4 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -653,7 +653,7 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
req->sdev = sdev;
req->lun = lun;
- req->sreq = sdev->info->alloc_req(sdev, req->qtag, lun);
+ req->sreq = scsi_req_new(sdev, req->qtag, lun);
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",
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 1375e82..c52e394 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -377,7 +377,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 = s->scsi_dev->info->alloc_req(s->scsi_dev, s->tag, 0);
+ s->req = scsi_req_new(s->scsi_dev, s->tag, 0);
scsi_req_enqueue(s->req, cbw.cmd);
/* ??? Should check that USB and SCSI data transfer
directions match. */
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 15/21] scsi: introduce scsi_req_kick
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (13 preceding siblings ...)
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 14/21] scsi: introduce scsi_req_new Paolo Bonzini
@ 2011-05-17 11:01 ` Paolo Bonzini
2011-05-20 16:05 ` Christoph Hellwig
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 16/21] scsi: introduce scsi_req_get_buf Paolo Bonzini
` (6 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:01 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/esp.c | 26 ++++++++++----------------
hw/lsi53c895a.c | 22 ++++++++--------------
hw/scsi-bus.c | 10 ++++++++++
hw/scsi.h | 1 +
hw/spapr_vscsi.c | 26 ++++++++++----------------
hw/usb-msd.c | 15 ++++-----------
trace-events | 1 +
7 files changed, 44 insertions(+), 57 deletions(-)
diff --git a/hw/esp.c b/hw/esp.c
index ad364b5..1f342f8 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -253,11 +253,10 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
s->dma_counter = 0;
if (datalen > 0) {
s->rregs[ESP_RSTAT] |= STAT_DI;
- s->current_dev->info->read_data(s->current_req);
} else {
s->rregs[ESP_RSTAT] |= STAT_DO;
- s->current_dev->info->write_data(s->current_req);
}
+ scsi_req_kick(s->current_req);
}
s->rregs[ESP_RINTR] = INTR_BS | INTR_FC;
s->rregs[ESP_RSEQ] = SEQ_CD;
@@ -383,22 +382,17 @@ static void esp_do_dma(ESPState *s)
else
s->ti_size -= len;
if (s->async_len == 0) {
- if (to_device) {
- // ti_size is negative
- s->current_dev->info->write_data(s->current_req);
- } else {
- s->current_dev->info->read_data(s->current_req);
- /* If there is still data to be read from the device then
- complete the DMA operation immediately. Otherwise defer
- until the scsi layer has completed. */
- if (s->dma_left == 0 && s->ti_size > 0) {
- esp_dma_done(s);
- }
+ scsi_req_kick(s->current_req);
+ /* If there is still data to be read from the device then
+ complete the DMA operation immediately. Otherwise defer
+ until the scsi layer has completed. */
+ if (to_device || s->dma_left != 0 || s->ti_size == 0) {
+ return;
}
- } else {
- /* Partially filled a scsi buffer. Complete immediately. */
- esp_dma_done(s);
}
+
+ /* Partially filled a scsi buffer. Complete immediately. */
+ esp_dma_done(s);
}
static void esp_command_complete(SCSIRequest *req, int reason, uint32_t arg)
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index e2af25f..5458a82 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -580,13 +580,7 @@ static void lsi_do_dma(LSIState *s, int out)
s->current->dma_len -= count;
if (s->current->dma_len == 0) {
s->current->dma_buf = NULL;
- if (out) {
- /* Write the data. */
- dev->info->write_data(s->current->req);
- } else {
- /* Request any remaining data. */
- dev->info->read_data(s->current->req);
- }
+ scsi_req_kick(s->current->req);
} else {
s->current->dma_buf += count;
lsi_resume_script(s);
@@ -791,14 +785,14 @@ static void lsi_do_command(LSIState *s)
s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun);
n = scsi_req_enqueue(s->current->req, buf);
- if (n > 0) {
- lsi_set_phase(s, PHASE_DI);
- dev->info->read_data(s->current->req);
- } else if (n < 0) {
- lsi_set_phase(s, PHASE_DO);
- dev->info->write_data(s->current->req);
+ if (n) {
+ if (n > 0) {
+ lsi_set_phase(s, PHASE_DI);
+ } else if (n < 0) {
+ lsi_set_phase(s, PHASE_DO);
+ }
+ scsi_req_kick(s->current->req);
}
-
if (!s->command_complete) {
if (n) {
/* Command did not complete immediately so disconnect. */
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index c091159..acb1ffa 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -606,6 +606,16 @@ void scsi_req_unref(SCSIRequest *req)
}
}
+void scsi_req_kick(SCSIRequest *req)
+{
+ trace_scsi_req_kick(req->dev->id, req->lun, req->tag);
+ if (req->cmd.mode == SCSI_XFER_TO_DEV) {
+ req->dev->info->write_data(req);
+ } else {
+ req->dev->info->read_data(req);
+ }
+}
+
void scsi_req_data(SCSIRequest *req, int len)
{
trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
diff --git a/hw/scsi.h b/hw/scsi.h
index e44c194..f659503 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -151,6 +151,7 @@ void scsi_req_unref(SCSIRequest *req);
int scsi_req_parse(SCSIRequest *req, uint8_t *buf);
void scsi_req_print(SCSIRequest *req);
+void scsi_req_kick(SCSIRequest *req);
void scsi_req_data(SCSIRequest *req, int len);
void scsi_req_complete(SCSIRequest *req);
void scsi_req_abort(SCSIRequest *req, int status);
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 1bb4bf4..27c8e17 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -448,7 +448,6 @@ static int vscsi_preprocess_desc(vscsi_req *req)
static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
{
- SCSIDevice *sdev = req->sdev;
uint8_t *cdb = req->iu.srp.cmd.cdb;
int n;
@@ -469,7 +468,7 @@ static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
} else if (n == 0) {
return;
}
- sdev->info->read_data(req->sreq);
+ scsi_req_kick(req->sreq);
}
/* Callback to indicate that the SCSI layer has completed a transfer. */
@@ -508,7 +507,7 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
buf[12], buf[13], buf[14], buf[15]);
memcpy(req->sense, buf, len);
req->senselen = len;
- sdev->info->read_data(sreq);
+ scsi_req_kick(req->sreq);
}
return;
}
@@ -552,11 +551,7 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
/* Start next chunk */
req->data_len -= rc;
- if (req->writing) {
- sdev->info->write_data(sreq);
- } else {
- sdev->info->read_data(sreq);
- }
+ scsi_req_kick(sreq);
}
static void vscsi_request_cancelled(SCSIRequest *sreq)
@@ -667,15 +662,14 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
/* Preprocess RDMA descriptors */
vscsi_preprocess_desc(req);
- }
- /* Get transfer direction and initiate transfer */
- if (n > 0) {
- req->data_len = n;
- sdev->info->read_data(req->sreq);
- } else if (n < 0) {
- req->data_len = -n;
- sdev->info->write_data(req->sreq);
+ /* Get transfer direction and initiate transfer */
+ if (n > 0) {
+ req->data_len = n;
+ } else if (n < 0) {
+ req->data_len = -n;
+ }
+ scsi_req_kick(req->sreq);
}
/* Don't touch req here, it may have been recycled already */
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index c52e394..e307c80 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -190,11 +190,7 @@ static void usb_msd_copy_data(MSDState *s)
s->scsi_buf += len;
s->data_len -= len;
if (s->scsi_len == 0 || s->data_len == 0) {
- if (s->mode == USB_MSDM_DATAIN) {
- s->scsi_dev->info->read_data(s->req);
- } else if (s->mode == USB_MSDM_DATAOUT) {
- s->scsi_dev->info->write_data(s->req);
- }
+ scsi_req_kick(s->req);
}
}
@@ -249,6 +245,7 @@ static void usb_msd_command_complete(SCSIRequest *req, int reason, uint32_t arg)
s->req = NULL;
return;
}
+ assert((s->mode == USB_MSDM_DATAOUT) == (req->cmd.mode == SCSI_XFER_TO_DEV));
s->scsi_len = arg;
s->scsi_buf = s->scsi_dev->info->get_buf(req);
if (p) {
@@ -381,12 +378,8 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
scsi_req_enqueue(s->req, cbw.cmd);
/* ??? Should check that USB and SCSI data transfer
directions match. */
- if (s->residue == 0) {
- if (s->mode == USB_MSDM_DATAIN) {
- s->scsi_dev->info->read_data(s->req);
- } else if (s->mode == USB_MSDM_DATAOUT) {
- s->scsi_dev->info->write_data(s->req);
- }
+ if (s->mode != USB_MSDM_CSW && s->residue == 0) {
+ scsi_req_kick(s->req);
}
ret = len;
break;
diff --git a/trace-events b/trace-events
index 98e7b2d..4e5c5cf 100644
--- a/trace-events
+++ b/trace-events
@@ -209,6 +209,7 @@ disable usb_set_device_feature(int addr, int feature, int ret) "dev %d, feature
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_kick(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, uint64_t lba) "target %d lun %d tag %d command %d dir %d length %d lba %"PRIu64""
disable scsi_req_parse_bad(int target, int lun, int tag, int cmd) "target %d lun %d tag %d command %d"
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 16/21] scsi: introduce scsi_req_get_buf
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (14 preceding siblings ...)
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 15/21] scsi: introduce scsi_req_kick Paolo Bonzini
@ 2011-05-17 11:01 ` Paolo Bonzini
2011-05-20 16:06 ` Christoph Hellwig
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 17/21] scsi: Implement 'get_sense' callback Paolo Bonzini
` (5 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:01 UTC (permalink / raw)
To: qemu-devel
... and remove some SCSIDevice variables or fields that now become unused.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/esp.c | 2 +-
hw/lsi53c895a.c | 2 +-
hw/scsi-bus.c | 5 +++++
hw/scsi.h | 1 +
hw/spapr_vscsi.c | 8 ++------
hw/usb-msd.c | 2 +-
6 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/hw/esp.c b/hw/esp.c
index 1f342f8..051b0fa 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -419,7 +419,7 @@ static void esp_command_complete(SCSIRequest *req, int reason, uint32_t arg)
} else {
DPRINTF("transfer %d/%d\n", s->dma_left, s->ti_size);
s->async_len = arg;
- s->async_buf = s->current_dev->info->get_buf(req);
+ s->async_buf = scsi_req_get_buf(req);
if (s->dma_left) {
esp_do_dma(s);
} else if (s->dma_counter != 0 && s->ti_size <= 0) {
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 5458a82..3f618fa 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -569,7 +569,7 @@ static void lsi_do_dma(LSIState *s, int out)
s->dnad += count;
s->dbc -= count;
if (s->current->dma_buf == NULL) {
- s->current->dma_buf = dev->info->get_buf(s->current->req);
+ s->current->dma_buf = scsi_req_get_buf(s->current->req);
}
/* ??? Set SFBR to first data byte. */
if (out) {
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index acb1ffa..94dccaf 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -151,6 +151,11 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun)
return d->info->alloc_req(d, tag, lun);
}
+uint8_t *scsi_req_get_buf(SCSIRequest *req)
+{
+ return req->dev->info->get_buf(req);
+}
+
int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
{
int32_t rc;
diff --git a/hw/scsi.h b/hw/scsi.h
index f659503..3af6295 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -154,6 +154,7 @@ void scsi_req_print(SCSIRequest *req);
void scsi_req_kick(SCSIRequest *req);
void scsi_req_data(SCSIRequest *req, int len);
void scsi_req_complete(SCSIRequest *req);
+uint8_t *scsi_req_get_buf(SCSIRequest *req);
void scsi_req_abort(SCSIRequest *req, int status);
void scsi_req_cancel(SCSIRequest *req);
void scsi_device_purge_requests(SCSIDevice *sdev);
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 27c8e17..46cd1d7 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -74,7 +74,6 @@ typedef struct vscsi_req {
union viosrp_iu iu;
/* SCSI request tracking */
- SCSIDevice *sdev;
SCSIRequest *sreq;
uint32_t qtag; /* qemu tag != srp tag */
int lun;
@@ -476,7 +475,6 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
{
VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, sreq->bus->qbus.parent);
vscsi_req *req = vscsi_find_req(s, sreq);
- SCSIDevice *sdev;
uint8_t *buf;
int32_t res_in = 0, res_out = 0;
int len, rc = 0;
@@ -487,7 +485,6 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
fprintf(stderr, "VSCSI: Can't find request for tag 0x%x\n", sreq->tag);
return;
}
- sdev = req->sdev;
if (req->sensing) {
if (reason == SCSI_REASON_DONE) {
@@ -495,7 +492,7 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
vscsi_put_req(s, req);
} else {
- uint8_t *buf = sdev->info->get_buf(sreq);
+ uint8_t *buf = scsi_req_get_buf(sreq);
len = MIN(arg, SCSI_SENSE_BUF_SIZE);
dprintf("VSCSI: Sense data, %d bytes:\n", len);
@@ -539,7 +536,7 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
* to write for writes (ie, how much is to be DMA'd)
*/
if (arg) {
- buf = sdev->info->get_buf(sreq);
+ buf = scsi_req_get_buf(sreq);
rc = vscsi_srp_transfer_data(s, req, req->writing, buf, arg);
}
if (rc < 0) {
@@ -646,7 +643,6 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
} return 1;
}
- req->sdev = sdev;
req->lun = lun;
req->sreq = scsi_req_new(sdev, req->qtag, lun);
n = scsi_req_enqueue(req->sreq, srp->cmd.cdb);
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index e307c80..14e42e5 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -247,7 +247,7 @@ static void usb_msd_command_complete(SCSIRequest *req, int reason, uint32_t arg)
}
assert((s->mode == USB_MSDM_DATAOUT) == (req->cmd.mode == SCSI_XFER_TO_DEV));
s->scsi_len = arg;
- s->scsi_buf = s->scsi_dev->info->get_buf(req);
+ s->scsi_buf = scsi_req_get_buf(req);
if (p) {
usb_msd_copy_data(s);
if (s->usb_len == 0) {
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 17/21] scsi: Implement 'get_sense' callback
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (15 preceding siblings ...)
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 16/21] scsi: introduce scsi_req_get_buf Paolo Bonzini
@ 2011-05-17 11:01 ` Paolo Bonzini
2011-05-20 16:06 ` Christoph Hellwig
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 18/21] scsi-disk: add data direction checking Paolo Bonzini
` (4 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:01 UTC (permalink / raw)
To: qemu-devel; +Cc: Hannes Reinecke
From: Hannes Reinecke <hare@suse.de>
The get_sense callback copies existing sense information into
the provided buffer. This is required if sense information
should be transferred together with the command response.
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-bus.c | 9 +++++++++
hw/scsi-disk.c | 9 +++++++++
hw/scsi-generic.c | 18 ++++++++++++++++++
hw/scsi.h | 2 ++
hw/spapr_vscsi.c | 10 +++++++++-
5 files changed, 47 insertions(+), 1 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 94dccaf..7daa112 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -156,6 +156,15 @@ uint8_t *scsi_req_get_buf(SCSIRequest *req)
return req->dev->info->get_buf(req);
}
+int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len)
+{
+ if (req->dev->info->get_sense) {
+ return req->dev->info->get_sense(req, buf, len);
+ } else {
+ return 0;
+ }
+}
+
int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
{
int32_t rc;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index efb953b..4241fad 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -338,6 +338,14 @@ static uint8_t *scsi_get_buf(SCSIRequest *req)
return (uint8_t *)r->iov.iov_base;
}
+/* Copy sense information into the provided buffer */
+static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len)
+{
+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
+
+ return scsi_build_sense(s->sense, outbuf, len, len > 14);
+}
+
static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
@@ -1225,6 +1233,7 @@ static SCSIDeviceInfo scsi_disk_info = {
.write_data = scsi_write_data,
.cancel_io = scsi_cancel_io,
.get_buf = scsi_get_buf,
+ .get_sense = scsi_get_sense,
.qdev.props = (Property[]) {
DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),
DEFINE_PROP_STRING("ver", SCSIDiskState, version),
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 036ab9f..a4de39d 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -79,6 +79,23 @@ static void scsi_clear_sense(SCSIGenericState *s)
s->driver_status = 0;
}
+static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len)
+{
+ SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, req->dev);
+ int size = SCSI_SENSE_BUF_SIZE;
+
+ if (!(s->driver_status & SG_ERR_DRIVER_SENSE)) {
+ size = scsi_build_sense(SENSE_CODE(NO_SENSE), s->sensebuf,
+ SCSI_SENSE_BUF_SIZE, 0);
+ }
+ if (size > len) {
+ size = len;
+ }
+ memcpy(outbuf, s->sensebuf, size);
+
+ return size;
+}
+
static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun)
{
SCSIRequest *req;
@@ -533,6 +550,7 @@ static SCSIDeviceInfo scsi_generic_info = {
.write_data = scsi_write_data,
.cancel_io = scsi_cancel_io,
.get_buf = scsi_get_buf,
+ .get_sense = scsi_get_sense,
.qdev.props = (Property[]) {
DEFINE_BLOCK_PROPERTIES(SCSIGenericState, qdev.conf),
DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/scsi.h b/hw/scsi.h
index 3af6295..dbb69ef 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -80,6 +80,7 @@ struct SCSIDeviceInfo {
int (*write_data)(SCSIRequest *req);
void (*cancel_io)(SCSIRequest *req);
uint8_t *(*get_buf)(SCSIRequest *req);
+ int (*get_sense)(SCSIRequest *req, uint8_t *buf, int len);
};
struct SCSIBusOps {
@@ -155,6 +156,7 @@ void scsi_req_kick(SCSIRequest *req);
void scsi_req_data(SCSIRequest *req, int len);
void scsi_req_complete(SCSIRequest *req);
uint8_t *scsi_req_get_buf(SCSIRequest *req);
+int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
void scsi_req_abort(SCSIRequest *req, int status);
void scsi_req_cancel(SCSIRequest *req);
void scsi_device_purge_requests(SCSIDevice *sdev);
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 46cd1d7..8a47de0 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -450,6 +450,15 @@ static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
uint8_t *cdb = req->iu.srp.cmd.cdb;
int n;
+ n = scsi_req_get_sense(req->sreq, req->sense, sizeof(req->sense));
+ if (n) {
+ req->senselen = n;
+ vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
+ vscsi_put_req(s, req);
+ return;
+ }
+
+ dprintf("VSCSI: Got CHECK_CONDITION, requesting sense...\n");
cdb[0] = 3;
cdb[1] = 0;
cdb[2] = 0;
@@ -522,7 +531,6 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
}
vscsi_send_rsp(s, req, 0, res_in, res_out);
} else if (arg == CHECK_CONDITION) {
- dprintf("VSCSI: Got CHECK_CONDITION, requesting sense...\n");
vscsi_send_request_sense(s, req);
return;
} else {
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 18/21] scsi-disk: add data direction checking
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (16 preceding siblings ...)
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 17/21] scsi: Implement 'get_sense' callback Paolo Bonzini
@ 2011-05-17 11:01 ` Paolo Bonzini
2011-05-20 16:07 ` Christoph Hellwig
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 19/21] scsi: make write_data return void Paolo Bonzini
` (3 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:01 UTC (permalink / raw)
To: qemu-devel; +Cc: Hannes Reinecke
From: Hannes Reinecke <hare@suse.de>
scsi_req_parse() already provides for a data direction setting,
so we should be using it to check for correct direction.
And we should return the sense code 'INVALID FIELD IN CDB'
in these cases.
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-disk.c | 37 +++++++++++++++++++++++++------------
1 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 4241fad..65744c7 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -49,10 +49,8 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
typedef struct SCSIDiskState SCSIDiskState;
-typedef struct SCSIDiskReq {
+ typedef struct SCSIDiskReq {
SCSIRequest req;
- /* ??? We should probably keep track of whether the data transfer is
- a read or a write. Currently we rely on the host getting it right. */
/* Both sector and sector_count are in terms of qemu 512 byte blocks. */
uint64_t sector;
uint32_t sector_count;
@@ -178,6 +176,12 @@ static void scsi_read_data(SCSIRequest *req)
/* No data transfer may already be in progress */
assert(r->req.aiocb == NULL);
+ if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
+ DPRINTF("Data transfer direction invalid\n");
+ scsi_read_complete(r, -EINVAL);
+ return;
+ }
+
n = r->sector_count;
if (n > SCSI_DMA_BUF_SIZE / 512)
n = SCSI_DMA_BUF_SIZE / 512;
@@ -214,16 +218,22 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
if (type == SCSI_REQ_STATUS_RETRY_READ) {
scsi_req_data(&r->req, 0);
}
- if (error == ENOMEM) {
+ switch (error) {
+ case ENOMEM:
scsi_command_complete(r, CHECK_CONDITION,
SENSE_CODE(TARGET_FAILURE));
- } else {
+ break;
+ case EINVAL:
+ scsi_command_complete(r, CHECK_CONDITION,
+ SENSE_CODE(INVALID_FIELD));
+ break;
+ default:
scsi_command_complete(r, CHECK_CONDITION,
SENSE_CODE(IO_ERROR));
+ break;
}
bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
}
-
return 1;
}
@@ -266,6 +276,12 @@ static int scsi_write_data(SCSIRequest *req)
/* No data transfer may already be in progress */
assert(r->req.aiocb == NULL);
+ if (r->req.cmd.mode != SCSI_XFER_TO_DEV) {
+ DPRINTF("Data transfer direction invalid\n");
+ scsi_write_complete(r, -EINVAL);
+ return 0;
+ }
+
n = r->iov.iov_len / 512;
if (n) {
qemu_iovec_init_external(&r->qiov, &r->iov, 1);
@@ -985,14 +1001,12 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
int32_t len;
- int is_write;
uint8_t command;
uint8_t *outbuf;
int rc;
command = buf[0];
outbuf = (uint8_t *)r->iov.iov_base;
- is_write = 0;
DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
if (scsi_req_parse(&r->req, buf) != 0) {
@@ -1072,7 +1086,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
goto illegal_lba;
r->sector = r->req.cmd.lba * s->cluster_size;
r->sector_count = len * s->cluster_size;
- is_write = 1;
break;
case MODE_SELECT:
DPRINTF("Mode Select(6) (len %lu)\n", (long)r->req.cmd.xfer);
@@ -1138,13 +1151,13 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE));
}
len = r->sector_count * 512 + r->iov.iov_len;
- if (is_write) {
- len = -len;
+ if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
+ return -len;
} else {
if (!r->sector_count)
r->sector_count = -1;
+ return len;
}
- return len;
}
static void scsi_disk_reset(DeviceState *dev)
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 19/21] scsi: make write_data return void
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (17 preceding siblings ...)
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 18/21] scsi-disk: add data direction checking Paolo Bonzini
@ 2011-05-17 11:01 ` Paolo Bonzini
2011-05-20 16:08 ` Christoph Hellwig
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 20/21] scsi-generic: Handle queue full Paolo Bonzini
` (2 subsequent siblings)
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:01 UTC (permalink / raw)
To: qemu-devel
The return value is unused anyway.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-disk.c | 6 ++----
hw/scsi-generic.c | 7 ++-----
hw/scsi.h | 2 +-
3 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 65744c7..4c7a53e 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -267,7 +267,7 @@ static void scsi_write_complete(void * opaque, int ret)
}
}
-static int scsi_write_data(SCSIRequest *req)
+static void scsi_write_data(SCSIRequest *req)
{
SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
@@ -279,7 +279,7 @@ static int scsi_write_data(SCSIRequest *req)
if (r->req.cmd.mode != SCSI_XFER_TO_DEV) {
DPRINTF("Data transfer direction invalid\n");
scsi_write_complete(r, -EINVAL);
- return 0;
+ return;
}
n = r->iov.iov_len / 512;
@@ -294,8 +294,6 @@ static int scsi_write_data(SCSIRequest *req)
/* Invoke completion routine to fetch data from host. */
scsi_write_complete(r, 0);
}
-
- return 0;
}
static void scsi_dma_restart_bh(void *opaque)
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index a4de39d..1ea0930 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -275,7 +275,7 @@ static void scsi_write_complete(void * opaque, int ret)
/* Write data to a scsi device. Returns nonzero on failure.
The transfer may complete asynchronously. */
-static int scsi_write_data(SCSIRequest *req)
+static void scsi_write_data(SCSIRequest *req)
{
SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, req->dev);
SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
@@ -285,16 +285,13 @@ static int scsi_write_data(SCSIRequest *req)
if (r->len == 0) {
r->len = r->buflen;
scsi_req_data(&r->req, r->len);
- return 0;
+ return;
}
ret = execute_command(s->bs, r, SG_DXFER_TO_DEV, scsi_write_complete);
if (ret < 0) {
scsi_command_complete(r, ret);
- return 1;
}
-
- return 0;
}
/* Return a pointer to the data buffer. */
diff --git a/hw/scsi.h b/hw/scsi.h
index dbb69ef..7eed475 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -77,7 +77,7 @@ struct SCSIDeviceInfo {
void (*free_req)(SCSIRequest *req);
int32_t (*send_command)(SCSIRequest *req, uint8_t *buf);
void (*read_data)(SCSIRequest *req);
- int (*write_data)(SCSIRequest *req);
+ void (*write_data)(SCSIRequest *req);
void (*cancel_io)(SCSIRequest *req);
uint8_t *(*get_buf)(SCSIRequest *req);
int (*get_sense)(SCSIRequest *req, uint8_t *buf, int len);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 20/21] scsi-generic: Handle queue full
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (18 preceding siblings ...)
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 19/21] scsi: make write_data return void Paolo Bonzini
@ 2011-05-17 11:01 ` Paolo Bonzini
2011-05-20 16:09 ` Christoph Hellwig
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 21/21] scsi: split command_complete callback in two Paolo Bonzini
2011-05-19 20:30 ` [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:01 UTC (permalink / raw)
To: qemu-devel; +Cc: Hannes Reinecke
The sg driver currently has a hardcoded limit of commands it
can handle simultaneously. When this limit is reached the
driver will return -EDOM. So we need to capture this to
enable proper return values here.
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-generic.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 1ea0930..0c04606 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -124,6 +124,9 @@ static void scsi_command_complete(void *opaque, int ret)
if (ret != 0) {
switch (ret) {
+ case -EDOM:
+ r->req.status = TASK_SET_FULL;
+ break;
case -EINVAL:
r->req.status = CHECK_CONDITION;
scsi_set_sense(s, SENSE_CODE(INVALID_FIELD));
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* [Qemu-devel] [PATCH v3 21/21] scsi: split command_complete callback in two
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (19 preceding siblings ...)
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 20/21] scsi-generic: Handle queue full Paolo Bonzini
@ 2011-05-17 11:01 ` Paolo Bonzini
2011-05-20 16:11 ` Christoph Hellwig
2011-05-19 20:30 ` [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
21 siblings, 1 reply; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-17 11:01 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/esp.c | 60 +++++++++++++++++---------------
hw/lsi53c895a.c | 48 +++++++++++++++-----------
hw/scsi-bus.c | 4 +-
hw/scsi.h | 9 +----
hw/spapr_vscsi.c | 101 ++++++++++++++++++++++++++++++------------------------
hw/usb-msd.c | 69 +++++++++++++++++++++----------------
6 files changed, 159 insertions(+), 132 deletions(-)
diff --git a/hw/esp.c b/hw/esp.c
index 051b0fa..5a33c67 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -395,38 +395,41 @@ static void esp_do_dma(ESPState *s)
esp_dma_done(s);
}
-static void esp_command_complete(SCSIRequest *req, int reason, uint32_t arg)
+static void esp_command_complete(SCSIRequest *req, uint32_t arg)
{
ESPState *s = DO_UPCAST(ESPState, busdev.qdev, req->bus->qbus.parent);
- if (reason == SCSI_REASON_DONE) {
- DPRINTF("SCSI Command complete\n");
- if (s->ti_size != 0)
- DPRINTF("SCSI command completed unexpectedly\n");
- s->ti_size = 0;
- s->dma_left = 0;
- s->async_len = 0;
- if (arg)
- DPRINTF("Command failed\n");
- s->sense = arg;
- s->rregs[ESP_RSTAT] = STAT_ST;
+ DPRINTF("SCSI Command complete\n");
+ if (s->ti_size != 0)
+ DPRINTF("SCSI command completed unexpectedly\n");
+ s->ti_size = 0;
+ s->dma_left = 0;
+ s->async_len = 0;
+ if (arg)
+ DPRINTF("Command failed\n");
+ s->sense = arg;
+ s->rregs[ESP_RSTAT] = STAT_ST;
+ esp_dma_done(s);
+ if (s->current_req) {
+ scsi_req_unref(s->current_req);
+ s->current_req = NULL;
+ s->current_dev = NULL;
+ }
+}
+
+static void esp_transfer_data(SCSIRequest *req, uint32_t arg)
+{
+ ESPState *s = DO_UPCAST(ESPState, busdev.qdev, req->bus->qbus.parent);
+
+ DPRINTF("transfer %d/%d\n", s->dma_left, s->ti_size);
+ s->async_len = arg;
+ s->async_buf = scsi_req_get_buf(req);
+ if (s->dma_left) {
+ esp_do_dma(s);
+ } else if (s->dma_counter != 0 && s->ti_size <= 0) {
+ /* If this was the last part of a DMA transfer then the
+ completion interrupt is deferred to here. */
esp_dma_done(s);
- if (s->current_req) {
- scsi_req_unref(s->current_req);
- s->current_req = NULL;
- s->current_dev = NULL;
- }
- } else {
- DPRINTF("transfer %d/%d\n", s->dma_left, s->ti_size);
- s->async_len = arg;
- s->async_buf = scsi_req_get_buf(req);
- if (s->dma_left) {
- esp_do_dma(s);
- } else if (s->dma_counter != 0 && s->ti_size <= 0) {
- /* If this was the last part of a DMA transfer then the
- completion interrupt is deferred to here. */
- esp_dma_done(s);
- }
}
}
@@ -725,6 +728,7 @@ void esp_init(target_phys_addr_t espaddr, int it_shift,
}
static struct SCSIBusOps esp_scsi_ops = {
+ .transfer_data = esp_transfer_data,
.complete = esp_command_complete,
.cancel = esp_request_cancelled
};
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 3f618fa..43de6f8 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -711,40 +711,47 @@ static int lsi_queue_tag(LSIState *s, uint32_t tag, uint32_t arg)
return 1;
}
}
- /* Callback to indicate that the SCSI layer has completed a transfer. */
-static void lsi_command_complete(SCSIRequest *req, int reason, uint32_t arg)
+
+ /* Callback to indicate that the SCSI layer has completed a command. */
+static void lsi_command_complete(SCSIRequest *req, uint32_t arg)
{
LSIState *s = DO_UPCAST(LSIState, dev.qdev, req->bus->qbus.parent);
int out;
out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
- if (reason == SCSI_REASON_DONE) {
- DPRINTF("Command complete status=%d\n", (int)arg);
- s->status = arg;
- s->command_complete = 2;
- if (s->waiting && s->dbc != 0) {
- /* Raise phase mismatch for short transfers. */
- lsi_bad_phase(s, out, PHASE_ST);
- } else {
- lsi_set_phase(s, PHASE_ST);
- }
+ DPRINTF("Command complete status=%d\n", (int)arg);
+ s->status = arg;
+ s->command_complete = 2;
+ if (s->waiting && s->dbc != 0) {
+ /* Raise phase mismatch for short transfers. */
+ lsi_bad_phase(s, out, PHASE_ST);
+ } else {
+ lsi_set_phase(s, PHASE_ST);
+ }
- if (req == s->current->req) {
- scsi_req_unref(s->current->req);
- qemu_free(s->current);
- s->current = NULL;
- }
- lsi_resume_script(s);
- return;
+ if (req == s->current->req) {
+ scsi_req_unref(s->current->req);
+ qemu_free(s->current);
+ s->current = NULL;
}
+ lsi_resume_script(s);
+}
+
+ /* Callback to indicate that the SCSI layer has completed a transfer. */
+static void lsi_transfer_data(SCSIRequest *req, uint32_t arg)
+{
+ LSIState *s = DO_UPCAST(LSIState, dev.qdev, req->bus->qbus.parent);
+ int out;
if (s->waiting == 1 || !s->current || req->tag != s->current->tag ||
(lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON))) {
if (lsi_queue_tag(s, req->tag, arg)) {
return;
- }
+ }
}
+ out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
+
/* host adapter (re)connected */
DPRINTF("Data ready tag=0x%x len=%d\n", req->tag, arg);
s->current->dma_len = arg;
@@ -2236,6 +2243,7 @@ static int lsi_scsi_uninit(PCIDevice *d)
}
static struct SCSIBusOps lsi_scsi_ops = {
+ .transfer_data = lsi_transfer_data,
.complete = lsi_command_complete,
.cancel = lsi_request_cancelled
};
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 7daa112..57cfc87 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -633,7 +633,7 @@ void scsi_req_kick(SCSIRequest *req)
void scsi_req_data(SCSIRequest *req, int len)
{
trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
- req->bus->ops.complete(req, SCSI_REASON_DATA, len);
+ req->bus->ops.transfer_data(req, len);
}
void scsi_req_print(SCSIRequest *req)
@@ -669,7 +669,7 @@ void scsi_req_complete(SCSIRequest *req)
assert(req->status != -1);
scsi_req_ref(req);
scsi_req_dequeue(req);
- req->bus->ops.complete(req, SCSI_REASON_DONE, req->status);
+ req->bus->ops.complete(req, req->status);
scsi_req_unref(req);
}
diff --git a/hw/scsi.h b/hw/scsi.h
index 7eed475..93e9d35 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -9,12 +9,6 @@
#define SCSI_CMD_BUF_SIZE 16
-/* scsi-disk.c */
-enum scsi_reason {
- SCSI_REASON_DONE, /* Command complete. */
- SCSI_REASON_DATA /* Transfer complete, more data required. */
-};
-
typedef struct SCSIBus SCSIBus;
typedef struct SCSIBusOps SCSIBusOps;
typedef struct SCSIDevice SCSIDevice;
@@ -84,7 +78,8 @@ struct SCSIDeviceInfo {
};
struct SCSIBusOps {
- void (*complete)(SCSIRequest *req, int reason, uint32_t arg);
+ void (*transfer_data)(SCSIRequest *req, uint32_t arg);
+ void (*complete)(SCSIRequest *req, uint32_t arg);
void (*cancel)(SCSIRequest *req);
};
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 8a47de0..b195e06 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -480,63 +480,34 @@ static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
}
/* Callback to indicate that the SCSI layer has completed a transfer. */
-static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
+static void vscsi_transfer_data(SCSIRequest *sreq, uint32_t arg)
{
VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, sreq->bus->qbus.parent);
vscsi_req *req = vscsi_find_req(s, sreq);
uint8_t *buf;
- int32_t res_in = 0, res_out = 0;
int len, rc = 0;
- dprintf("VSCSI: SCSI cmd complete, r=0x%x tag=0x%x arg=0x%x, req=%p\n",
- reason, sreq->tag, arg, req);
+ dprintf("VSCSI: SCSI xfer complete tag=0x%x arg=0x%x, req=%p\n",
+ sreq->tag, arg, req);
if (req == NULL) {
fprintf(stderr, "VSCSI: Can't find request for tag 0x%x\n", sreq->tag);
return;
}
if (req->sensing) {
- if (reason == SCSI_REASON_DONE) {
- dprintf("VSCSI: Sense done !\n");
- vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
- vscsi_put_req(s, req);
- } else {
- uint8_t *buf = scsi_req_get_buf(sreq);
-
- len = MIN(arg, SCSI_SENSE_BUF_SIZE);
- dprintf("VSCSI: Sense data, %d bytes:\n", len);
- dprintf(" %02x %02x %02x %02x %02x %02x %02x %02x\n",
- buf[0], buf[1], buf[2], buf[3],
- buf[4], buf[5], buf[6], buf[7]);
- dprintf(" %02x %02x %02x %02x %02x %02x %02x %02x\n",
- buf[8], buf[9], buf[10], buf[11],
- buf[12], buf[13], buf[14], buf[15]);
- memcpy(req->sense, buf, len);
- req->senselen = len;
- scsi_req_kick(req->sreq);
- }
- return;
- }
-
- if (reason == SCSI_REASON_DONE) {
- dprintf("VSCSI: Command complete err=%d\n", arg);
- if (arg == 0) {
- /* We handle overflows, not underflows for normal commands,
- * but hopefully nobody cares
- */
- if (req->writing) {
- res_out = req->data_len;
- } else {
- res_in = req->data_len;
- }
- vscsi_send_rsp(s, req, 0, res_in, res_out);
- } else if (arg == CHECK_CONDITION) {
- vscsi_send_request_sense(s, req);
- return;
- } else {
- vscsi_send_rsp(s, req, arg, 0, 0);
- }
- vscsi_put_req(s, req);
+ uint8_t *buf = scsi_req_get_buf(sreq);
+
+ len = MIN(arg, SCSI_SENSE_BUF_SIZE);
+ dprintf("VSCSI: Sense data, %d bytes:\n", len);
+ dprintf(" %02x %02x %02x %02x %02x %02x %02x %02x\n",
+ buf[0], buf[1], buf[2], buf[3],
+ buf[4], buf[5], buf[6], buf[7]);
+ dprintf(" %02x %02x %02x %02x %02x %02x %02x %02x\n",
+ buf[8], buf[9], buf[10], buf[11],
+ buf[12], buf[13], buf[14], buf[15]);
+ memcpy(req->sense, buf, len);
+ req->senselen = len;
+ scsi_req_kick(req->sreq);
return;
}
@@ -559,6 +530,45 @@ static void vscsi_command_complete(SCSIRequest *sreq, int reason, uint32_t arg)
scsi_req_kick(sreq);
}
+/* Callback to indicate that the SCSI layer has completed a transfer. */
+static void vscsi_command_complete(SCSIRequest *sreq, uint32_t arg)
+{
+ VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, sreq->bus->qbus.parent);
+ vscsi_req *req = vscsi_find_req(s, sreq);
+ int32_t res_in = 0, res_out = 0;
+
+ dprintf("VSCSI: SCSI cmd complete, r=0x%x tag=0x%x arg=0x%x, req=%p\n",
+ reason, sreq->tag, arg, req);
+ if (req == NULL) {
+ fprintf(stderr, "VSCSI: Can't find request for tag 0x%x\n", sreq->tag);
+ return;
+ }
+
+ if (!req->sensing && arg == CHECK_CONDITION) {
+ vscsi_send_request_sense(s, req);
+ return;
+ }
+
+ if (req->sensing) {
+ dprintf("VSCSI: Sense done !\n");
+ arg = CHECK_CONDITION;
+ } else {
+ dprintf("VSCSI: Command complete err=%d\n", arg);
+ if (arg == 0) {
+ /* We handle overflows, not underflows for normal commands,
+ * but hopefully nobody cares
+ */
+ if (req->writing) {
+ res_out = req->data_len;
+ } else {
+ res_in = req->data_len;
+ }
+ }
+ }
+ vscsi_send_rsp(s, req, 0, res_in, res_out);
+ vscsi_put_req(s, req);
+}
+
static void vscsi_request_cancelled(SCSIRequest *sreq)
{
VSCSIState *s = DO_UPCAST(VSCSIState, vdev.qdev, sreq->bus->qbus.parent);
@@ -916,6 +926,7 @@ static int vscsi_do_crq(struct VIOsPAPRDevice *dev, uint8_t *crq_data)
}
static struct SCSIBusOps vscsi_scsi_ops = {
+ .transfer_data = vscsi_transfer_data,
.complete = vscsi_command_complete,
.cancel = vscsi_request_cancelled
};
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 14e42e5..dc80014 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -208,7 +208,7 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
memcpy(p->data, &csw, len);
}
-static void usb_msd_command_complete(SCSIRequest *req, int reason, uint32_t arg)
+static void usb_msd_transfer_data(SCSIRequest *req, uint32_t arg)
{
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
USBPacket *p = s->packet;
@@ -216,35 +216,7 @@ static void usb_msd_command_complete(SCSIRequest *req, int reason, uint32_t arg)
if (req->tag != s->tag) {
fprintf(stderr, "usb-msd: Unexpected SCSI Tag 0x%x\n", req->tag);
}
- if (reason == SCSI_REASON_DONE) {
- DPRINTF("Command complete %d\n", arg);
- s->residue = s->data_len;
- s->result = arg != 0;
- if (s->packet) {
- if (s->data_len == 0 && s->mode == USB_MSDM_DATAOUT) {
- /* A deferred packet with no write data remaining must be
- the status read packet. */
- usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
- } else {
- if (s->data_len) {
- s->data_len -= s->usb_len;
- if (s->mode == USB_MSDM_DATAIN)
- memset(s->usb_buf, 0, s->usb_len);
- s->usb_len = 0;
- }
- if (s->data_len == 0)
- s->mode = USB_MSDM_CSW;
- }
- s->packet = NULL;
- usb_packet_complete(&s->dev, p);
- } else if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
- }
- scsi_req_unref(req);
- s->req = NULL;
- return;
- }
+
assert((s->mode == USB_MSDM_DATAOUT) == (req->cmd.mode == SCSI_XFER_TO_DEV));
s->scsi_len = arg;
s->scsi_buf = scsi_req_get_buf(req);
@@ -261,6 +233,42 @@ static void usb_msd_command_complete(SCSIRequest *req, int reason, uint32_t arg)
}
}
+static void usb_msd_command_complete(SCSIRequest *req, uint32_t arg)
+{
+ 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);
+ }
+ DPRINTF("Command complete %d\n", arg);
+ s->residue = s->data_len;
+ s->result = arg != 0;
+ if (s->packet) {
+ if (s->data_len == 0 && s->mode == USB_MSDM_DATAOUT) {
+ /* A deferred packet with no write data remaining must be
+ the status read packet. */
+ usb_msd_send_status(s, p);
+ s->mode = USB_MSDM_CBW;
+ } else {
+ if (s->data_len) {
+ s->data_len -= s->usb_len;
+ if (s->mode == USB_MSDM_DATAIN)
+ memset(s->usb_buf, 0, s->usb_len);
+ s->usb_len = 0;
+ }
+ if (s->data_len == 0)
+ s->mode = USB_MSDM_CSW;
+ }
+ s->packet = NULL;
+ usb_packet_complete(&s->dev, p);
+ } else if (s->data_len == 0) {
+ s->mode = USB_MSDM_CSW;
+ }
+ scsi_req_unref(req);
+ s->req = NULL;
+}
+
static void usb_msd_request_cancelled(SCSIRequest *req)
{
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
@@ -494,6 +502,7 @@ static void usb_msd_password_cb(void *opaque, int err)
}
static struct SCSIBusOps usb_msd_scsi_ops = {
+ .transfer_data = usb_msd_transfer_data,
.complete = usb_msd_command_complete,
.cancel = usb_msd_request_cancelled
};
--
1.7.4.4
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 21/21] scsi: split command_complete callback in two
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 21/21] scsi: split command_complete callback in two Paolo Bonzini
@ 2011-05-20 16:11 ` Christoph Hellwig
2011-05-20 17:44 ` Paolo Bonzini
0 siblings, 1 reply; 52+ messages in thread
From: Christoph Hellwig @ 2011-05-20 16:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
> +static void esp_command_complete(SCSIRequest *req, uint32_t arg)
Shouldn't the "arg" argument to the new ->command_complete be renamed
to something like "sense" or "status"?
> +static void esp_transfer_data(SCSIRequest *req, uint32_t arg)
> +{
> + ESPState *s = DO_UPCAST(ESPState, busdev.qdev, req->bus->qbus.parent);
> +
> + DPRINTF("transfer %d/%d\n", s->dma_left, s->ti_size);
> + s->async_len = arg;
> + s->async_buf = scsi_req_get_buf(req);
> + if (s->dma_left) {
> + esp_do_dma(s);
> + } else if (s->dma_counter != 0 && s->ti_size <= 0) {
> + /* If this was the last part of a DMA transfer then the
> + completion interrupt is deferred to here. */
And for transfer_data "arg" should become "len".
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [Qemu-devel] [PATCH v3 21/21] scsi: split command_complete callback in two
2011-05-20 16:11 ` Christoph Hellwig
@ 2011-05-20 17:44 ` Paolo Bonzini
0 siblings, 0 replies; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-20 17:44 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: qemu-devel
On 05/20/2011 06:11 PM, Christoph Hellwig wrote:
>> +static void esp_command_complete(SCSIRequest *req, uint32_t arg)
>
> Shouldn't the "arg" argument to the new ->command_complete be renamed
> to something like "sense" or "status"?
>
>> +static void esp_transfer_data(SCSIRequest *req, uint32_t arg)
>> +{
>> + ESPState *s = DO_UPCAST(ESPState, busdev.qdev, req->bus->qbus.parent);
>> +
>> + DPRINTF("transfer %d/%d\n", s->dma_left, s->ti_size);
>> + s->async_len = arg;
>> + s->async_buf = scsi_req_get_buf(req);
>> + if (s->dma_left) {
>> + esp_do_dma(s);
>> + } else if (s->dma_counter != 0&& s->ti_size<= 0) {
>> + /* If this was the last part of a DMA transfer then the
>> + completion interrupt is deferred to here. */
>
> And for transfer_data "arg" should become "len".
True, I wanted to keep the patch as mechanical as possible. I'll add a
22nd patch doing it.
Paolo
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements
2011-05-17 11:00 [Qemu-devel] [PATCH v3 00/21] SCSI subsystem improvements Paolo Bonzini
` (20 preceding siblings ...)
2011-05-17 11:01 ` [Qemu-devel] [PATCH v3 21/21] scsi: split command_complete callback in two Paolo Bonzini
@ 2011-05-19 20:30 ` Paolo Bonzini
21 siblings, 0 replies; 52+ messages in thread
From: Paolo Bonzini @ 2011-05-19 20:30 UTC (permalink / raw)
To: qemu-devel
On 05/17/2011 01:00 PM, Paolo Bonzini wrote:
> This series includes the following improvements to the SCSI subsystem:
>
> 1) introduction of SCSIBusOps that generalize the existing
> command_complete callback;
>
> 2) widespread use of the SCSIRequest abstraction, with simpler memory
> management (refcounting) and with various common idioms converted into
> simple C functions instead of duplicating them all over the place;
>
> 3) support for autosense.
>
> Some patches are from Hannes Reinecke's megasas patchset posted last
> November, forward ported and applied to the new vSCSI controller as
> well.
Ping? Any acks or brave committers?
Paolo
^ permalink raw reply [flat|nested] 52+ messages in thread