From fa6acb0c2ff3f256fb5f2fede4768b27374b03ca Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Tue, 24 Apr 2012 16:29:04 +1000 Subject: [PATCH 01/14] ISCSI: Add support for thin-provisioning via discard/UNMAP and bigger LUNs Update the configure test for libiscsi support to detect version 1.3 or later. Version 1.3 of libiscsi provides both READCAPACITY16 as well as UNMAP commands. Update the iscsi block layer to use READCAPACITY16 to detect the size of the LUN instead of READCAPACITY10. This allows support for LUNs larger than 2TB. Update to implement bdrv_aio_discard() using the UNMAP command. This allows us to use thin-provisioned LUNs from TGTD and other iSCSI targets that support thin-provisioning. Signed-off-by: Ronnie Sahlberg [squashed in subsequent patch from Ronnie to fix off-by-one in LBA count] Signed-off-by: Paolo Bonzini --- block/iscsi.c | 86 +++++++++++++++++++++++++++++++++++++++++++-------- configure | 5 ++- 2 files changed, 77 insertions(+), 14 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index 5222726d0f..d37c4ee171 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -383,6 +383,65 @@ iscsi_aio_flush(BlockDriverState *bs, return &acb->common; } +static void +iscsi_unmap_cb(struct iscsi_context *iscsi, int status, + void *command_data, void *opaque) +{ + IscsiAIOCB *acb = opaque; + + if (acb->canceled != 0) { + qemu_aio_release(acb); + scsi_free_scsi_task(acb->task); + acb->task = NULL; + return; + } + + acb->status = 0; + if (status < 0) { + error_report("Failed to unmap data on iSCSI lun. %s", + iscsi_get_error(iscsi)); + acb->status = -EIO; + } + + iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); + scsi_free_scsi_task(acb->task); + acb->task = NULL; +} + +static BlockDriverAIOCB * +iscsi_aio_discard(BlockDriverState *bs, + int64_t sector_num, int nb_sectors, + BlockDriverCompletionFunc *cb, void *opaque) +{ + IscsiLun *iscsilun = bs->opaque; + struct iscsi_context *iscsi = iscsilun->iscsi; + IscsiAIOCB *acb; + struct unmap_list list[1]; + + acb = qemu_aio_get(&iscsi_aio_pool, bs, cb, opaque); + + acb->iscsilun = iscsilun; + acb->canceled = 0; + + list[0].lba = sector_qemu2lun(sector_num, iscsilun); + list[0].num = nb_sectors * BDRV_SECTOR_SIZE / iscsilun->block_size; + + acb->task = iscsi_unmap_task(iscsi, iscsilun->lun, + 0, 0, &list[0], 1, + iscsi_unmap_cb, + acb); + if (acb->task == NULL) { + error_report("iSCSI: Failed to send unmap command. %s", + iscsi_get_error(iscsi)); + qemu_aio_release(acb); + return NULL; + } + + iscsi_set_events(iscsilun); + + return &acb->common; +} + static int64_t iscsi_getlength(BlockDriverState *bs) { @@ -396,11 +455,11 @@ iscsi_getlength(BlockDriverState *bs) } static void -iscsi_readcapacity10_cb(struct iscsi_context *iscsi, int status, +iscsi_readcapacity16_cb(struct iscsi_context *iscsi, int status, void *command_data, void *opaque) { struct IscsiTask *itask = opaque; - struct scsi_readcapacity10 *rc10; + struct scsi_readcapacity16 *rc16; struct scsi_task *task = command_data; if (status != 0) { @@ -412,26 +471,25 @@ iscsi_readcapacity10_cb(struct iscsi_context *iscsi, int status, return; } - rc10 = scsi_datain_unmarshall(task); - if (rc10 == NULL) { - error_report("iSCSI: Failed to unmarshall readcapacity10 data."); + rc16 = scsi_datain_unmarshall(task); + if (rc16 == NULL) { + error_report("iSCSI: Failed to unmarshall readcapacity16 data."); itask->status = 1; itask->complete = 1; scsi_free_scsi_task(task); return; } - itask->iscsilun->block_size = rc10->block_size; - itask->iscsilun->num_blocks = rc10->lba; - itask->bs->total_sectors = (uint64_t)rc10->lba * - rc10->block_size / BDRV_SECTOR_SIZE ; + itask->iscsilun->block_size = rc16->block_length; + itask->iscsilun->num_blocks = rc16->returned_lba + 1; + itask->bs->total_sectors = itask->iscsilun->num_blocks * + itask->iscsilun->block_size / BDRV_SECTOR_SIZE ; itask->status = 0; itask->complete = 1; scsi_free_scsi_task(task); } - static void iscsi_connect_cb(struct iscsi_context *iscsi, int status, void *command_data, void *opaque) @@ -445,10 +503,10 @@ iscsi_connect_cb(struct iscsi_context *iscsi, int status, void *command_data, return; } - task = iscsi_readcapacity10_task(iscsi, itask->iscsilun->lun, 0, 0, - iscsi_readcapacity10_cb, opaque); + task = iscsi_readcapacity16_task(iscsi, itask->iscsilun->lun, + iscsi_readcapacity16_cb, opaque); if (task == NULL) { - error_report("iSCSI: failed to send readcapacity command."); + error_report("iSCSI: failed to send readcapacity16 command."); itask->status = 1; itask->complete = 1; return; @@ -700,6 +758,8 @@ static BlockDriver bdrv_iscsi = { .bdrv_aio_readv = iscsi_aio_readv, .bdrv_aio_writev = iscsi_aio_writev, .bdrv_aio_flush = iscsi_aio_flush, + + .bdrv_aio_discard = iscsi_aio_discard, }; static void iscsi_block_init(void) diff --git a/configure b/configure index 0111774cb0..0e3615be07 100755 --- a/configure +++ b/configure @@ -2546,10 +2546,13 @@ fi ########################################## # Do we have libiscsi +# We check for iscsi_unmap_sync() to make sure we have a +# recent enough version of libiscsi. if test "$libiscsi" != "no" ; then cat > $TMPC << EOF +#include #include -int main(void) { iscsi_create_context(""); return 0; } +int main(void) { iscsi_unmap_sync(NULL,0,0,0,NULL,0); return 0; } EOF if compile_prog "-Werror" "-liscsi" ; then libiscsi="yes" From 12a08998fe4f749af3622385521829a5143e6ff1 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 8 Feb 2012 11:49:43 +0100 Subject: [PATCH 02/14] scsi: prevent data transfer overflow Avoid sending more than 2GB of data, as that can cause overflows in int32_t variables. Signed-off-by: Paolo Bonzini --- hw/scsi-bus.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index dbdb99ce35..c29a4aea4a 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -239,6 +239,18 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus) return res; } +static int32_t scsi_invalid_field(SCSIRequest *req, uint8_t *buf) +{ + scsi_req_build_sense(req, SENSE_CODE(INVALID_FIELD)); + scsi_req_complete(req, CHECK_CONDITION); + return 0; +} + +static const struct SCSIReqOps reqops_invalid_field = { + .size = sizeof(SCSIRequest), + .send_command = scsi_invalid_field +}; + /* SCSIReqOps implementation for invalid commands. */ static int32_t scsi_invalid_command(SCSIRequest *req, uint8_t *buf) @@ -517,18 +529,20 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun, cmd.lba); } - if ((d->unit_attention.key == UNIT_ATTENTION || - bus->unit_attention.key == UNIT_ATTENTION) && - (buf[0] != INQUIRY && - buf[0] != REPORT_LUNS && - buf[0] != GET_CONFIGURATION && - buf[0] != GET_EVENT_STATUS_NOTIFICATION && + if (cmd.xfer > INT32_MAX) { + req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, hba_private); + } else if ((d->unit_attention.key == UNIT_ATTENTION || + bus->unit_attention.key == UNIT_ATTENTION) && + (buf[0] != INQUIRY && + buf[0] != REPORT_LUNS && + buf[0] != GET_CONFIGURATION && + buf[0] != GET_EVENT_STATUS_NOTIFICATION && - /* - * If we already have a pending unit attention condition, - * report this one before triggering another one. - */ - !(buf[0] == REQUEST_SENSE && d->sense_is_ua))) { + /* + * If we already have a pending unit attention condition, + * report this one before triggering another one. + */ + !(buf[0] == REQUEST_SENSE && d->sense_is_ua))) { req = scsi_req_alloc(&reqops_unit_attention, d, tag, lun, hba_private); } else if (lun != d->lun || From 31e8fd86f24b4eec8a1708d712bf0532460bb0a5 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 24 Apr 2012 08:41:04 +0200 Subject: [PATCH 03/14] scsi: fix refcounting for reads Recently introduced FUA support also gave us a use-after-free of the BlockAcctCookie within a SCSIDiskReq, due to unbalanced reference counting. The patch fixes this by making scsi_do_read look like a combination of scsi_*_complete + scsi_*_data. It does both a ref (like scsi_read_data) and an unref (like scsi_flush_complete). Reported-by: David Gibson Signed-off-by: Paolo Bonzini --- hw/scsi-disk.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index a029ab6e84..eca00a6b1d 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -296,6 +296,13 @@ static void scsi_do_read(void *opaque, int ret) } } + if (r->req.io_canceled) { + return; + } + + /* The request is used as the AIO opaque value, so add a ref. */ + scsi_req_ref(&r->req); + if (r->req.sg) { dma_acct_start(s->qdev.conf.bs, &r->acct, r->req.sg, BDRV_ACCT_READ); r->req.resid -= r->req.sg->size; From a5ee9085627eaeb501db31e3758df4e18500be71 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 8 Feb 2012 10:40:37 +0100 Subject: [PATCH 04/14] scsi: fix WRITE SAME transfer length and direction Signed-off-by: Paolo Bonzini --- hw/scsi-bus.c | 14 ++++++++------ hw/scsi-disk.c | 5 ++++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index c29a4aea4a..5640aae68d 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -791,7 +791,8 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf) case MODE_SENSE: break; case WRITE_SAME_10: - cmd->xfer = 1; + case WRITE_SAME_16: + cmd->xfer = dev->blocksize; break; case READ_CAPACITY_10: cmd->xfer = 8; @@ -909,6 +910,10 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu static void scsi_cmd_xfer_mode(SCSICommand *cmd) { + if (!cmd->xfer) { + cmd->mode = SCSI_XFER_NONE; + return; + } switch (cmd->buf[0]) { case WRITE_6: case WRITE_10: @@ -934,6 +939,7 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd) case UPDATE_BLOCK: case WRITE_LONG_10: case WRITE_SAME_10: + case WRITE_SAME_16: case SEARCH_HIGH_12: case SEARCH_EQUAL_12: case SEARCH_LOW_12: @@ -946,11 +952,7 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd) cmd->mode = SCSI_XFER_TO_DEV; break; default: - if (cmd->xfer) - cmd->mode = SCSI_XFER_FROM_DEV; - else { - cmd->mode = SCSI_XFER_NONE; - } + cmd->mode = SCSI_XFER_FROM_DEV; break; } } diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index eca00a6b1d..fbb10415cb 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -1566,8 +1566,11 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf) } break; case WRITE_SAME_10: + len = lduw_be_p(&buf[7]); + goto write_same; case WRITE_SAME_16: - len = r->req.cmd.xfer / s->qdev.blocksize; + len = ldl_be_p(&buf[10]) & 0xffffffffULL; + write_same: DPRINTF("WRITE SAME() (sector %" PRId64 ", count %d)\n", r->req.cmd.lba, len); From 381b634c275ca1a2806e97392527bbfc01bcb333 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Sat, 28 Apr 2012 23:49:36 +1000 Subject: [PATCH 05/14] scsi: Specify the xfer direction for UNMAP and ATA_PASSTHROUGH commands scsi_cmd_xfer_mode() is used to specify the xfer direction for SCSI commands that come in from the guest. If the direction is set incorrectly this will eventually cause QEMU to kernel-panic the guest. Add UNMAP and ATAPASSTHROUGH as commands that send data to the device. Without this change, recent kernels will send both UNMAP as well as ATAPASSTHROUGH commands to any /dev/sg* device, which due to the incorrect xfer direction very quickly causes the guest kernel to crash. Example causing a crash without the patch applied: ./x86_64-softmmu/qemu-system-x86_64 -m 1024 -enable-kvm -cdrom linuxmint-12-gnome-dvd-64bit.iso -drive file=/dev/sg4,if=scsi,bus=0,unit=6 Signed-off-by: Ronnie Sahlberg Signed-off-by: Paolo Bonzini --- hw/scsi-bus.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 5640aae68d..08d5088ce2 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -940,6 +940,7 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd) case WRITE_LONG_10: case WRITE_SAME_10: case WRITE_SAME_16: + case UNMAP: case SEARCH_HIGH_12: case SEARCH_EQUAL_12: case SEARCH_LOW_12: @@ -949,6 +950,7 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd) case SEND_DVD_STRUCTURE: case PERSISTENT_RESERVE_OUT: case MAINTENANCE_OUT: + case ATA_PASSTHROUGH: cmd->mode = SCSI_XFER_TO_DEV; break; default: From bfe3d7ac6d838b1931678d96d01c45d84c7e3de8 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 1 May 2012 10:23:54 +0200 Subject: [PATCH 06/14] scsi: change "removable" field to host many features It is pointless to add a uint32_t field for every new feature. Since we will need a new feature soon, convert accesses to "removable" to look at bit 0 only. Signed-off-by: Paolo Bonzini --- hw/scsi-disk.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index fbb10415cb..b1f5ef046e 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -61,10 +61,12 @@ typedef struct SCSIDiskReq { BlockAcctCookie acct; } SCSIDiskReq; +#define SCSI_DISK_F_REMOVABLE 0 + struct SCSIDiskState { SCSIDevice qdev; - uint32_t removable; + uint32_t features; bool media_changed; bool media_event; bool eject_request; @@ -669,7 +671,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) memset(outbuf, 0, buflen); outbuf[0] = s->qdev.type & 0x1f; - outbuf[1] = s->removable ? 0x80 : 0; + outbuf[1] = (s->features & (1 << SCSI_DISK_F_REMOVABLE)) ? 0x80 : 0; if (s->qdev.type == TYPE_ROM) { memcpy(&outbuf[16], "QEMU CD-ROM ", 16); } else { @@ -1710,7 +1712,8 @@ static int scsi_initfn(SCSIDevice *dev) return -1; } - if (!s->removable && !bdrv_is_inserted(s->qdev.conf.bs)) { + if (!(s->features & (1 << SCSI_DISK_F_REMOVABLE)) && + !bdrv_is_inserted(s->qdev.conf.bs)) { error_report("Device needs media, but drive is empty"); return -1; } @@ -1732,7 +1735,7 @@ static int scsi_initfn(SCSIDevice *dev) return -1; } - if (s->removable) { + if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) { bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_cd_block_ops, s); } bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize); @@ -1755,7 +1758,7 @@ static int scsi_cd_initfn(SCSIDevice *dev) SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); s->qdev.blocksize = 2048; s->qdev.type = TYPE_ROM; - s->removable = true; + s->features |= 1 << SCSI_DISK_F_REMOVABLE; return scsi_initfn(&s->qdev); } @@ -1828,7 +1831,9 @@ static int get_device_type(SCSIDiskState *s) return -1; } s->qdev.type = buf[0]; - s->removable = (buf[1] & 0x80) != 0; + if (buf[1] & 0x80) { + s->features |= 1 << SCSI_DISK_F_REMOVABLE; + } return 0; } @@ -1928,7 +1933,8 @@ static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag, static Property scsi_hd_properties[] = { DEFINE_SCSI_DISK_PROPERTIES(), - DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false), + DEFINE_PROP_BIT("removable", SCSIDiskState, features, + SCSI_DISK_F_REMOVABLE, false), DEFINE_PROP_END_OF_LIST(), }; @@ -2030,7 +2036,8 @@ static TypeInfo scsi_block_info = { static Property scsi_disk_properties[] = { DEFINE_SCSI_DISK_PROPERTIES(), - DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false), + DEFINE_PROP_BIT("removable", SCSIDiskState, features, + SCSI_DISK_F_REMOVABLE, false), DEFINE_PROP_END_OF_LIST(), }; From da8365dbab51c445832137aa637bb5b990174b24 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 1 May 2012 10:25:16 +0200 Subject: [PATCH 07/14] scsi-disk: add dpofua property Linux expects REQ_FUA to be advertised only if WRITE+FUA is faster than WRITE+SYNCHRONIZE CACHE, so we should not set the DPOFUA bit. However, it is useful to have it for testing purposes, so add a qdev property to set it. Signed-off-by: Paolo Bonzini --- hw/scsi-disk.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index b1f5ef046e..43726ffb89 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -62,6 +62,7 @@ typedef struct SCSIDiskReq { } SCSIDiskReq; #define SCSI_DISK_F_REMOVABLE 0 +#define SCSI_DISK_F_DPOFUA 1 struct SCSIDiskState { @@ -1103,7 +1104,7 @@ static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf) p = outbuf; if (s->qdev.type == TYPE_DISK) { - dev_specific_param = 0x10; /* DPOFUA */ + dev_specific_param = s->features & (1 << SCSI_DISK_F_DPOFUA) ? 0x10 : 0; if (bdrv_is_read_only(s->qdev.conf.bs)) { dev_specific_param |= 0x80; /* Readonly. */ } @@ -1935,6 +1936,8 @@ static Property scsi_hd_properties[] = { DEFINE_SCSI_DISK_PROPERTIES(), DEFINE_PROP_BIT("removable", SCSIDiskState, features, SCSI_DISK_F_REMOVABLE, false), + DEFINE_PROP_BIT("dpofua", SCSIDiskState, features, + SCSI_DISK_F_DPOFUA, false), DEFINE_PROP_END_OF_LIST(), }; @@ -2038,6 +2041,8 @@ static Property scsi_disk_properties[] = { DEFINE_SCSI_DISK_PROPERTIES(), DEFINE_PROP_BIT("removable", SCSIDiskState, features, SCSI_DISK_F_REMOVABLE, false), + DEFINE_PROP_BIT("dpofua", SCSIDiskState, features, + SCSI_DISK_F_DPOFUA, false), DEFINE_PROP_END_OF_LIST(), }; From f62d0594604399e89ca8ece730a2a79110de5d77 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 3 May 2012 15:28:05 +0200 Subject: [PATCH 08/14] scsi: do not report bogus overruns for commands in the 0x00-0x1F range Interpreting cdb[4] == 0 as a request to transfer 256 blocks is only needed for READ_6 and WRITE_6. No other command in that range needs that special-casing, and the resulting overrun breaks scsi-testsuite's attempt to use command 2 as a known-invalid command. Signed-off-by: Paolo Bonzini --- hw/scsi-bus.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 08d5088ce2..5fbf8dbb14 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -735,10 +735,6 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf) case 0: cmd->xfer = buf[4]; cmd->len = 6; - /* length 0 means 256 blocks */ - if (cmd->xfer == 0) { - cmd->xfer = 256; - } break; case 1: case 2: @@ -808,18 +804,26 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf) cmd->xfer = buf[9] | (buf[8] << 8); } break; + case WRITE_6: + /* length 0 means 256 blocks */ + if (cmd->xfer == 0) { + cmd->xfer = 256; + } case WRITE_10: case WRITE_VERIFY_10: - case WRITE_6: case WRITE_12: case WRITE_VERIFY_12: case WRITE_16: case WRITE_VERIFY_16: cmd->xfer *= dev->blocksize; break; - case READ_10: case READ_6: case READ_REVERSE: + /* length 0 means 256 blocks */ + if (cmd->xfer == 0) { + cmd->xfer = 256; + } + case READ_10: case RECOVER_BUFFERED_DATA: case READ_12: case READ_16: From 065c25996b6275e306704816c6075d6c0ff66e84 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 4 May 2012 10:28:55 +0200 Subject: [PATCH 09/14] scsi: parse 16-byte tape CDBs The transfer length for these commands is different from the transfer length of the corresponding disk commands, so parse it specially. Signed-off-by: Paolo Bonzini --- hw/scsi-bus.c | 10 ++++++++++ hw/scsi-defs.h | 1 + 2 files changed, 11 insertions(+) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 5fbf8dbb14..46cd1f9324 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -891,6 +891,16 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu cmd->xfer *= dev->blocksize; } break; + case READ_16: + case READ_REVERSE_16: + case VERIFY_16: + case WRITE_16: + cmd->len = 16; + cmd->xfer = buf[14] | (buf[13] << 8) | (buf[12] << 16); + if (buf[1] & 0x01) { /* fixed */ + cmd->xfer *= dev->blocksize; + } + break; case REWIND: case START_STOP: cmd->len = 6; diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h index ca24192d53..219c84dfb1 100644 --- a/hw/scsi-defs.h +++ b/hw/scsi-defs.h @@ -92,6 +92,7 @@ #define PERSISTENT_RESERVE_OUT 0x5f #define VARLENGTH_CDB 0x7f #define WRITE_FILEMARKS_16 0x80 +#define READ_REVERSE_16 0x81 #define ALLOW_OVERWRITE 0x82 #define EXTENDED_COPY 0x83 #define ATA_PASSTHROUGH 0x85 From 3c3d8a95cafb6b38515497688db5a26db67fe6ce Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 3 May 2012 14:34:45 +0200 Subject: [PATCH 10/14] scsi: do not require a minimum allocation length for INQUIRY The requirements on the INQUIRY buffer size are not in my copy of SPC (SPC-4 r27) and not observed by LIO. Rip them out. Signed-off-by: Paolo Bonzini --- hw/scsi-bus.c | 8 -------- hw/scsi-disk.c | 11 ----------- 2 files changed, 19 deletions(-) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 46cd1f9324..4090b9f1ab 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -367,10 +367,6 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r) if (r->req.cmd.buf[1] & 0x1) { /* Vital product data */ uint8_t page_code = r->req.cmd.buf[2]; - if (r->req.cmd.xfer < 4) { - return false; - } - r->buf[r->len++] = page_code ; /* this page */ r->buf[r->len++] = 0x00; @@ -398,10 +394,6 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r) } /* PAGE CODE == 0 */ - if (r->req.cmd.xfer < 5) { - return false; - } - r->len = MIN(r->req.cmd.xfer, 36); memset(r->buf, 0, r->len); if (r->req.lun != 0) { diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 43726ffb89..41580321c4 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -524,11 +524,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) if (req->cmd.buf[1] & 0x1) { /* Vital product data */ uint8_t page_code = req->cmd.buf[2]; - if (req->cmd.xfer < 4) { - BADF("Error: Inquiry (EVPD[%02X]) buffer size %zd is " - "less than 4\n", page_code, req->cmd.xfer); - return -1; - } outbuf[buflen++] = s->qdev.type & 0x1f; outbuf[buflen++] = page_code ; // this page @@ -659,12 +654,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) } /* PAGE CODE == 0 */ - if (req->cmd.xfer < 5) { - BADF("Error: Inquiry (STANDARD) buffer size %zd " - "is less than 5\n", req->cmd.xfer); - return -1; - } - buflen = req->cmd.xfer; if (buflen > SCSI_MAX_INQUIRY_LEN) { buflen = SCSI_MAX_INQUIRY_LEN; From e5f38ff6f530de7f14825fb117de854ed52084fa Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 3 May 2012 15:57:08 +0200 Subject: [PATCH 11/14] scsi: do not require a minimum allocation length for REQUEST SENSE The requirements on the REQUEST SENSE buffer size are not in my copy of SPC (SPC-4 r27) and not observed by LIO. Rip them out. Signed-off-by: Paolo Bonzini --- hw/scsi-bus.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 4090b9f1ab..925c3aeecb 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -427,9 +427,6 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf) } break; case REQUEST_SENSE: - if (req->cmd.xfer < 4) { - goto illegal_request; - } r->len = scsi_device_get_sense(r->req.dev, r->buf, MIN(req->cmd.xfer, sizeof r->buf), (req->cmd.buf[1] & 1) == 0); @@ -538,8 +535,8 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun, req = scsi_req_alloc(&reqops_unit_attention, d, tag, lun, hba_private); } else if (lun != d->lun || - buf[0] == REPORT_LUNS || - (buf[0] == REQUEST_SENSE && (d->sense_len || cmd.xfer < 4))) { + buf[0] == REPORT_LUNS || + (buf[0] == REQUEST_SENSE && d->sense_len)) { req = scsi_req_alloc(&reqops_target_command, d, tag, lun, hba_private); } else { From 77e4743c94d2a926623e280913e05ad6c840791e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 3 May 2012 17:59:27 +0200 Subject: [PATCH 12/14] scsi: set VALID bit to 0 in fixed format sense data The INFORMATION field (bytes 3..6) is never set by QEMU, so the VALID bit must be 0. Signed-off-by: Paolo Bonzini --- hw/scsi-bus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 925c3aeecb..add1d4f28d 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -649,7 +649,7 @@ void scsi_req_build_sense(SCSIRequest *req, SCSISense sense) trace_scsi_req_build_sense(req->dev->id, req->lun, req->tag, sense.key, sense.asc, sense.ascq); memset(req->sense, 0, 18); - req->sense[0] = 0xf0; + req->sense[0] = 0x70; req->sense[2] = sense.key; req->sense[7] = 10; req->sense[12] = sense.asc; @@ -1148,7 +1148,7 @@ int scsi_build_sense(uint8_t *in_buf, int in_len, memset(buf, 0, len); if (fixed) { /* Return fixed format sense buffer */ - buf[0] = 0xf0; + buf[0] = 0x70; buf[2] = sense.key; buf[7] = 10; buf[12] = sense.asc; From 2a92fbff49a286ddad1686d60532d791f20f4ce1 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 3 May 2012 18:26:13 +0200 Subject: [PATCH 13/14] scsi: remove useless debug messages Optional inquiry information is declared obsolete in the latest versions of the standard; invalid CDBs or unsupported VPD pages are supported can be diagnosed with trace_scsi_inquiry. Signed-off-by: Paolo Bonzini --- hw/scsi-disk.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 41580321c4..045c764d9b 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -28,9 +28,6 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0) #define DPRINTF(fmt, ...) do {} while(0) #endif -#define BADF(fmt, ...) \ -do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0) - #include "qemu-common.h" #include "qemu-error.h" #include "scsi.h" @@ -515,12 +512,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev); int buflen = 0; - if (req->cmd.buf[1] & 0x2) { - /* Command support data - optional, not implemented */ - BADF("optional INQUIRY command support request not implemented\n"); - return -1; - } - if (req->cmd.buf[1] & 0x1) { /* Vital product data */ uint8_t page_code = req->cmd.buf[2]; @@ -638,8 +629,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) break; } default: - BADF("Error: unsupported Inquiry (EVPD[%02X]) " - "buffer size %zd\n", page_code, req->cmd.xfer); return -1; } /* done with EVPD */ @@ -648,8 +637,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) /* Standard INQUIRY data */ if (req->cmd.buf[2] != 0) { - BADF("Error: Inquiry (STANDARD) page or code " - "is non-zero [%02X]\n", req->cmd.buf[2]); return -1; } From 68bd348ade453821fd5378479e6718e69bf181f1 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Fri, 4 May 2012 08:51:16 +0200 Subject: [PATCH 14/14] scsi: Add assertion for use-after-free errors The QEMU emulation which is currently used with Raspberry PI images (qemu-system-arm -M versatilepb ...) accesses memory which was freed. Valgrind output (extract): ==17857== Invalid write of size 4 ==17857== at 0x24EB06: scsi_req_unref (scsi-bus.c:1273) ==17857== by 0x24FFAE: scsi_read_complete (scsi-disk.c:277) ==17857== by 0x152ACC: bdrv_co_em_bh (block.c:3363) ==17857== by 0x13D49C: qemu_bh_poll (async.c:71) ==17857== by 0x211A8C: main_loop_wait (main-loop.c:503) ==17857== by 0x207954: main_loop (vl.c:1555) ==17857== by 0x20E9C9: main (vl.c:3653) ==17857== Address 0x1c54383c is 12 bytes inside a block of size 260 free'd ==17857== at 0x4824B3A: free (vg_replace_malloc.c:366) ==17857== by 0x20ADFA: free_and_trace (vl.c:2250) ==17857== by 0x4899FC5: g_free (in /lib/libglib-2.0.so.0.2400.1) ==17857== by 0x24EB3B: scsi_req_unref (scsi-bus.c:1277) ==17857== by 0x24F003: scsi_req_complete (scsi-bus.c:1383) ==17857== by 0x25022A: scsi_read_data (scsi-disk.c:334) ==17857== by 0x24EB9F: scsi_req_continue (scsi-bus.c:1289) ==17857== by 0x1C7787: lsi_do_dma (lsi53c895a.c:575) ==17857== by 0x1C8CDA: lsi_execute_script (lsi53c895a.c:1147) ==17857== by 0x1C74EA: lsi_resume_script (lsi53c895a.c:510) ==17857== by 0x1C7ECD: lsi_transfer_data (lsi53c895a.c:746) ==17857== by 0x24EC90: scsi_req_data (scsi-bus.c:1307) (There are some more similar messages.) This patch adds an assertion which also detects those errors: Calling scsi_req_unref is not allowed when the previous call of that function has decremented refcount to 0, because in this case req was freed. Signed-off-by: Stefan Weil Signed-off-by: Paolo Bonzini --- hw/scsi-bus.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index add1d4f28d..8ab9bcda86 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -1291,6 +1291,7 @@ SCSIRequest *scsi_req_ref(SCSIRequest *req) void scsi_req_unref(SCSIRequest *req) { + assert(req->refcount > 0); if (--req->refcount == 0) { if (req->ops->free_req) { req->ops->free_req(req);