From efaf4781a995aacd22b1dd521b14e4644bafae14 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 23 Jun 2016 16:37:11 -0600 Subject: scsi: Advertise limits by blocksize, not 512 s->blocksize may be larger than 512, in which case our tweaks to max_xfer_len and opt_xfer_len must be scaled appropriately. CC: qemu-stable@nongnu.org Reported-by: Fam Zheng Signed-off-by: Eric Blake Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- hw/scsi/scsi-generic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c index 6a2d89afba..75e227de95 100644 --- a/hw/scsi/scsi-generic.c +++ b/hw/scsi/scsi-generic.c @@ -225,7 +225,8 @@ static void scsi_read_complete(void * opaque, int ret) if (s->type == TYPE_DISK && r->req.cmd.buf[0] == INQUIRY && r->req.cmd.buf[2] == 0xb0) { - uint32_t max_xfer_len = blk_get_max_transfer_length(s->conf.blk); + uint32_t max_xfer_len = blk_get_max_transfer_length(s->conf.blk) / + (s->blocksize / BDRV_SECTOR_SIZE); if (max_xfer_len) { stl_be_p(&r->buf[8], max_xfer_len); /* Also take care of the opt xfer len. */ -- cgit v1.2.1 From 24ce9a20260713e86377cfa78fb8699335759f4f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 23 Jun 2016 16:37:12 -0600 Subject: block: Give nonzero result to blk_get_max_transfer_length() Making all callers special-case 0 as unlimited is awkward, and we DO have a hard maximum of BDRV_REQUEST_MAX_SECTORS given our current block layer API limits. In the case of scsi, this means that we now always advertise a limit to the guest, even in cases where the underlying layers previously use 0 for no inherent limit beyond the block layer. Signed-off-by: Eric Blake Reviewed-by: Kevin Wolf Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- hw/block/virtio-blk.c | 3 +-- hw/scsi/scsi-generic.c | 12 ++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) (limited to 'hw') diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index fb43bbaa46..dd94cd4dc7 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -384,7 +384,7 @@ static int multireq_compare(const void *a, const void *b) void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb) { int i = 0, start = 0, num_reqs = 0, niov = 0, nb_sectors = 0; - int max_xfer_len = 0; + int max_xfer_len; int64_t sector_num = 0; if (mrb->num_reqs == 1) { @@ -394,7 +394,6 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb) } max_xfer_len = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk); - max_xfer_len = MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECTORS); qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs), &multireq_compare); diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c index 75e227de95..0cb8568f1f 100644 --- a/hw/scsi/scsi-generic.c +++ b/hw/scsi/scsi-generic.c @@ -227,12 +227,12 @@ static void scsi_read_complete(void * opaque, int ret) r->req.cmd.buf[2] == 0xb0) { uint32_t max_xfer_len = blk_get_max_transfer_length(s->conf.blk) / (s->blocksize / BDRV_SECTOR_SIZE); - if (max_xfer_len) { - stl_be_p(&r->buf[8], max_xfer_len); - /* Also take care of the opt xfer len. */ - if (ldl_be_p(&r->buf[12]) > max_xfer_len) { - stl_be_p(&r->buf[12], max_xfer_len); - } + + assert(max_xfer_len); + stl_be_p(&r->buf[8], max_xfer_len); + /* Also take care of the opt xfer len. */ + if (ldl_be_p(&r->buf[12]) > max_xfer_len) { + stl_be_p(&r->buf[12], max_xfer_len); } } scsi_req_data(&r->req, len); -- cgit v1.2.1 From 5def6b80e1eca696c1fc6099e7f4d36729686402 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 23 Jun 2016 16:37:19 -0600 Subject: block: Switch transfer length bounds to byte-based Sector-based limits are awkward to think about; in our on-going quest to move to byte-based interfaces, convert max_transfer_length and opt_transfer_length. Rename them (dropping the _length suffix) so that the compiler will help us catch the change in semantics across any rebased code, and improve the documentation. Use unsigned values, so that we don't have to worry about negative values and so that bit-twiddling is easier; however, we are still constrained by 2^31 of signed int in most APIs. When a value comes from an external source (iscsi and raw-posix), sanitize the results to ensure that opt_transfer is a power of 2. Signed-off-by: Eric Blake Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- hw/block/virtio-blk.c | 9 +++++---- hw/scsi/scsi-generic.c | 12 ++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) (limited to 'hw') diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index dd94cd4dc7..ae86e944ea 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -384,7 +384,7 @@ static int multireq_compare(const void *a, const void *b) void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb) { int i = 0, start = 0, num_reqs = 0, niov = 0, nb_sectors = 0; - int max_xfer_len; + uint32_t max_transfer; int64_t sector_num = 0; if (mrb->num_reqs == 1) { @@ -393,7 +393,7 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb) return; } - max_xfer_len = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk); + max_transfer = blk_get_max_transfer(mrb->reqs[0]->dev->blk); qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs), &multireq_compare); @@ -409,8 +409,9 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb) */ if (sector_num + nb_sectors != req->sector_num || niov > blk_get_max_iov(blk) - req->qiov.niov || - req->qiov.size / BDRV_SECTOR_SIZE > max_xfer_len || - nb_sectors > max_xfer_len - req->qiov.size / BDRV_SECTOR_SIZE) { + req->qiov.size > max_transfer || + nb_sectors > (max_transfer - + req->qiov.size) / BDRV_SECTOR_SIZE) { submit_requests(blk, mrb, start, num_reqs, niov); num_reqs = 0; } diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c index 0cb8568f1f..7a588a7ad4 100644 --- a/hw/scsi/scsi-generic.c +++ b/hw/scsi/scsi-generic.c @@ -225,14 +225,14 @@ static void scsi_read_complete(void * opaque, int ret) if (s->type == TYPE_DISK && r->req.cmd.buf[0] == INQUIRY && r->req.cmd.buf[2] == 0xb0) { - uint32_t max_xfer_len = blk_get_max_transfer_length(s->conf.blk) / - (s->blocksize / BDRV_SECTOR_SIZE); + uint32_t max_transfer = + blk_get_max_transfer(s->conf.blk) / s->blocksize; - assert(max_xfer_len); - stl_be_p(&r->buf[8], max_xfer_len); + assert(max_transfer); + stl_be_p(&r->buf[8], max_transfer); /* Also take care of the opt xfer len. */ - if (ldl_be_p(&r->buf[12]) > max_xfer_len) { - stl_be_p(&r->buf[12], max_xfer_len); + if (ldl_be_p(&r->buf[12]) > max_transfer) { + stl_be_p(&r->buf[12], max_transfer); } } scsi_req_data(&r->req, len); -- cgit v1.2.1 From a9d52a75634ac9aa7d101bf7f63e10bf6655a865 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 09:30:01 +0200 Subject: block/qdev: Fix NULL access when using BB twice BlockBackend has only a single pointer to its guest device, so it makes sure that only a single guest device is attached to it. device-add returns an error if you try to attach a second device to a BB. In order to make the error message nicer, -device that manually connects to a if=none block device get a different message than -drive that implicitly creates a guest device. The if=... option is stored in DriveInfo. However, since blockdev-add exists, not every BlockBackend has a DriveInfo any more. Check that it exists before we dereference it. QMP reproducer resulting in a segfault: {"execute":"blockdev-add","arguments":{"options":{"id":"disk","driver":"file","filename":"/tmp/test.img"}}} {"execute":"device_add","arguments":{"driver":"virtio-blk-pci","drive":"disk"}} {"execute":"device_add","arguments":{"driver":"virtio-blk-pci","drive":"disk"}} Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi --- hw/core/qdev-properties-system.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 891219ae05..df38b8a03b 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -82,7 +82,7 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr, if (blk_attach_dev(blk, dev) < 0) { DriveInfo *dinfo = blk_legacy_dinfo(blk); - if (dinfo->type != IF_NONE) { + if (dinfo && dinfo->type != IF_NONE) { error_setg(errp, "Drive '%s' is already in use because " "it has been automatically connected to another " "device (did you need 'if=none' in the drive options?)", -- cgit v1.2.1