* [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation
@ 2020-06-17 10:20 Lin Ma
2020-06-17 10:20 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Lin Ma @ 2020-06-17 10:20 UTC (permalink / raw)
To: qemu-block; +Cc: fam, kwolf, qemu-devel, mreitz, stefanha, Lin Ma, pbonzini
v2->v1:
Follow Claudio's suggestions and the docker test result, Fix the dereferencing
'void *' pointer issues and the coding style issues.
In this current design, The GET LBA STATUS parameter data only contains
an eight-byte header + one LBA status descriptor.
How to test:
host:~ # qemu-system-x86_64 \
...
-drive file=/vm0/disk0.raw,format=raw,if=none,id=drive0,discard=unmap \
-device scsi-hd,id=scsi0,drive=drive0 \
...
guest:~ # dd if=/dev/zero of=/dev/sda bs=512 seek=1024 count=256
guest:~ # sg_unmap -l 1024 -n 32 /dev/sda
guest:~ # sg_get_lba_status /dev/sda -l 1024
No indication of the completion condition
RTP=0
descriptor LBA: 0x0000000000000400 blocks: 32 deallocated
Lin Ma (3):
block: Add bdrv_co_get_lba_status
block: Add GET LBA STATUS support
scsi-disk: Add support for the GET LBA STATUS 16 command
block/block-backend.c | 38 ++++++++++++++
block/io.c | 43 ++++++++++++++++
hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++++++++++++++++
include/block/accounting.h | 1 +
include/block/block_int.h | 5 ++
include/scsi/constants.h | 1 +
include/sysemu/block-backend.h | 2 +
7 files changed, 180 insertions(+)
--
2.26.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/3] block: Add bdrv_co_get_lba_status
2020-06-17 10:20 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
@ 2020-06-17 10:20 ` Lin Ma
2020-06-17 10:20 ` [PATCH v2 2/3] block: Add GET LBA STATUS support Lin Ma
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Lin Ma @ 2020-06-17 10:20 UTC (permalink / raw)
To: qemu-block; +Cc: fam, kwolf, qemu-devel, mreitz, stefanha, Lin Ma, pbonzini
The get lba status wrapper based on the bdrv_block_status. The following
patches will add GET LBA STATUS 16 support for scsi emulation layer.
Signed-off-by: Lin Ma <lma@suse.com>
---
block/io.c | 43 +++++++++++++++++++++++++++++++++++++++
include/block/block_int.h | 5 +++++
2 files changed, 48 insertions(+)
diff --git a/block/io.c b/block/io.c
index df8f2a98d4..2064016b19 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2208,6 +2208,49 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
BDRV_REQ_ZERO_WRITE | flags);
}
+int coroutine_fn
+bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
+ uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+ BlockDriverState *bs = child->bs;
+ int ret = 0;
+ int64_t target_size, count = 0;
+ bool first = true;
+ uint8_t wanted_bit1 = 0;
+
+ target_size = bdrv_getlength(bs);
+ if (target_size < 0) {
+ return -EIO;
+ }
+
+ if (offset < 0 || bytes < 0) {
+ return -EIO;
+ }
+
+ for ( ; offset <= target_size - bytes; offset += count) {
+ ret = bdrv_block_status(bs, offset, bytes, &count, NULL, NULL);
+ if (ret < 0) {
+ goto out;
+ }
+ if (first) {
+ if (ret & BDRV_BLOCK_ZERO) {
+ wanted_bit1 = BDRV_BLOCK_ZERO >> 1;
+ *is_deallocated = 1;
+ } else {
+ wanted_bit1 = 0;
+ }
+ first = false;
+ }
+ if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) {
+ (*num_blocks)++;
+ } else {
+ break;
+ }
+ }
+out:
+ return ret;
+}
+
/*
* Flush ALL BDSes regardless of if they are reachable via a BlkBackend or not.
*/
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 791de6a59c..43f90591b9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1296,6 +1296,11 @@ int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
int64_t *pnum,
int64_t *map,
BlockDriverState **file);
+int coroutine_fn bdrv_co_get_lba_status(BdrvChild *child,
+ int64_t offset,
+ int64_t bytes,
+ uint32_t *num_blocks,
+ uint32_t *is_deallocated);
const char *bdrv_get_parent_name(const BlockDriverState *bs);
void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
bool blk_dev_has_removable_media(BlockBackend *blk);
--
2.26.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] block: Add GET LBA STATUS support
2020-06-17 10:20 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:20 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
@ 2020-06-17 10:20 ` Lin Ma
2020-06-17 10:20 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
2020-06-17 10:54 ` [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation no-reply
3 siblings, 0 replies; 12+ messages in thread
From: Lin Ma @ 2020-06-17 10:20 UTC (permalink / raw)
To: qemu-block; +Cc: fam, kwolf, qemu-devel, mreitz, stefanha, Lin Ma, pbonzini
Signed-off-by: Lin Ma <lma@suse.com>
---
block/block-backend.c | 38 ++++++++++++++++++++++++++++++++++
include/sysemu/block-backend.h | 2 ++
2 files changed, 40 insertions(+)
diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25c83..6d08dd5e0d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1650,6 +1650,44 @@ int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
return blk_prw(blk, offset, NULL, bytes, blk_pdiscard_entry, 0);
}
+static int coroutine_fn
+blk_do_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
+ uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+ int ret;
+
+ blk_wait_while_drained(blk);
+
+ ret = blk_check_byte_request(blk, offset, bytes);
+ if (ret < 0) {
+ return ret;
+ }
+
+ return bdrv_co_get_lba_status(blk->root, offset, bytes, num_blocks,
+ is_deallocated);
+}
+
+static void blk_aio_get_lba_status_entry(void *opaque)
+{
+ BlkAioEmAIOCB *acb = opaque;
+ BlkRwCo *rwco = &acb->rwco;
+
+ void *data = acb->common.opaque;
+ uint32_t *num_blocks = (uint32_t *)data;
+ uint32_t *is_deallocated = (uint32_t *)(data + sizeof(uint32_t));
+
+ rwco->ret = blk_do_get_lba_status(rwco->blk, rwco->offset, acb->bytes,
+ num_blocks, is_deallocated);
+ blk_aio_complete(acb);
+}
+
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
+ BlockCompletionFunc *cb, void *opaque)
+{
+ return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_get_lba_status_entry,
+ 0, cb, opaque);
+}
+
/* To be called between exactly one pair of blk_inc/dec_in_flight() */
static int coroutine_fn blk_do_flush(BlockBackend *blk)
{
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8203d7f6f9..cd527ec0c9 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -171,6 +171,8 @@ BlockAIOCB *blk_aio_flush(BlockBackend *blk,
BlockCompletionFunc *cb, void *opaque);
BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t offset, int bytes,
BlockCompletionFunc *cb, void *opaque);
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
+ BlockCompletionFunc *cb, void *opaque);
void blk_aio_cancel(BlockAIOCB *acb);
void blk_aio_cancel_async(BlockAIOCB *acb);
int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
--
2.26.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-17 10:20 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:20 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
2020-06-17 10:20 ` [PATCH v2 2/3] block: Add GET LBA STATUS support Lin Ma
@ 2020-06-17 10:20 ` Lin Ma
2020-06-17 10:54 ` [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation no-reply
3 siblings, 0 replies; 12+ messages in thread
From: Lin Ma @ 2020-06-17 10:20 UTC (permalink / raw)
To: qemu-block; +Cc: fam, kwolf, qemu-devel, mreitz, stefanha, Lin Ma, pbonzini
Signed-off-by: Lin Ma <lma@suse.com>
---
hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++++++++++++++++++++
include/block/accounting.h | 1 +
include/scsi/constants.h | 1 +
3 files changed, 92 insertions(+)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..9e3002ddaf 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
}
}
+typedef struct GetLbaStatusCBData {
+ uint32_t num_blocks;
+ uint32_t is_deallocated;
+ SCSIDiskReq *r;
+} GetLbaStatusCBData;
+
+static void scsi_get_lba_status_complete(void *opaque, int ret);
+
+static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
+{
+ SCSIDiskReq *r = data->r;
+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+ assert(r->req.aiocb == NULL);
+
+ block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
+ s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
+
+ r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
+ r->req.cmd.lba * s->qdev.blocksize,
+ s->qdev.blocksize,
+ scsi_get_lba_status_complete, data);
+}
+
+static void scsi_get_lba_status_complete(void *opaque, int ret)
+{
+ GetLbaStatusCBData *data = opaque;
+ SCSIDiskReq *r = data->r;
+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+ assert(r->req.aiocb != NULL);
+ r->req.aiocb = NULL;
+
+ aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
+ if (scsi_disk_req_check_error(r, ret, true)) {
+ g_free(data);
+ goto done;
+ }
+
+ block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
+ scsi_req_unref(&r->req);
+ g_free(data);
+
+done:
+ aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
+}
+
+static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
+{
+ SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+ GetLbaStatusCBData *data;
+ uint32_t *num_blocks;
+ uint32_t *is_deallocated;
+
+ data = g_new0(GetLbaStatusCBData, 1);
+ data->r = r;
+ num_blocks = &(data->num_blocks);
+ is_deallocated = &(data->is_deallocated);
+
+ scsi_req_ref(&r->req);
+ scsi_get_lba_status_complete_noio(data, 0);
+
+ /*
+ * 8 + 16 is the length in bytes of response header and
+ * one LBA status descriptor
+ */
+ memset(outbuf, 0, 8 + 16);
+ outbuf[3] = 20;
+ outbuf[8] = (req->cmd.lba >> 56) & 0xff;
+ outbuf[9] = (req->cmd.lba >> 48) & 0xff;
+ outbuf[10] = (req->cmd.lba >> 40) & 0xff;
+ outbuf[11] = (req->cmd.lba >> 32) & 0xff;
+ outbuf[12] = (req->cmd.lba >> 24) & 0xff;
+ outbuf[13] = (req->cmd.lba >> 16) & 0xff;
+ outbuf[14] = (req->cmd.lba >> 8) & 0xff;
+ outbuf[15] = req->cmd.lba & 0xff;
+ outbuf[16] = (*num_blocks >> 24) & 0xff;
+ outbuf[17] = (*num_blocks >> 16) & 0xff;
+ outbuf[18] = (*num_blocks >> 8) & 0xff;
+ outbuf[19] = *num_blocks & 0xff;
+ outbuf[20] = *is_deallocated ? 1 : 0;
+}
+
static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
{
SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
/* Protection, exponent and lowest lba field left blank. */
break;
+ } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
+ if (req->cmd.lba > s->qdev.max_lba) {
+ goto illegal_lba;
+ }
+ scsi_disk_emulate_get_lba_status(req, outbuf);
+ r->iov.iov_len = req->cmd.xfer;
+ return r->iov.iov_len;
}
trace_scsi_disk_emulate_command_SAI_unsupported();
goto illegal_request;
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..645014fb0b 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -38,6 +38,7 @@ enum BlockAcctType {
BLOCK_ACCT_WRITE,
BLOCK_ACCT_FLUSH,
BLOCK_ACCT_UNMAP,
+ BLOCK_ACCT_GET_LBA_STATUS,
BLOCK_MAX_IOTYPE,
};
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..b18377b214 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -154,6 +154,7 @@
* SERVICE ACTION IN subcodes
*/
#define SAI_READ_CAPACITY_16 0x10
+#define SAI_GET_LBA_STATUS 0x12
/*
* READ POSITION service action codes
--
2.26.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation
2020-06-17 10:20 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
` (2 preceding siblings ...)
2020-06-17 10:20 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
@ 2020-06-17 10:54 ` no-reply
3 siblings, 0 replies; 12+ messages in thread
From: no-reply @ 2020-06-17 10:54 UTC (permalink / raw)
To: lma; +Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, lma, pbonzini
Patchew URL: https://patchew.org/QEMU/20200617102019.29652-1-lma@suse.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
CC contrib/vhost-user-input/main.o
LINK tests/qemu-iotests/socket_scm_helper
GEN docs/interop/qemu-qmp-ref.html
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
GEN docs/interop/qemu-qmp-ref.txt
GEN docs/interop/qemu-qmp-ref.7
CC qga/commands.o
---
GEN docs/interop/qemu-ga-ref.html
GEN docs/interop/qemu-ga-ref.7
GEN docs/interop/qemu-ga-ref.txt
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
AS pc-bios/optionrom/linuxboot.o
AS pc-bios/optionrom/multiboot.o
CC pc-bios/optionrom/linuxboot_dma.o
---
LINK qemu-ga
LINK qemu-keymap
LINK ivshmem-client
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK ivshmem-server
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-nbd
LINK qemu-storage-daemon
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-img
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-io
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-edid
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK fsdev/virtfs-proxy-helper
LINK scsi/qemu-pr-helper
LINK qemu-bridge-helper
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK virtiofsd
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK vhost-user-input
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
GEN x86_64-softmmu/hmp-commands-info.h
GEN x86_64-softmmu/hmp-commands.h
GEN x86_64-softmmu/config-target.h
---
CC x86_64-softmmu/accel/tcg/tcg-all.o
CC x86_64-softmmu/accel/tcg/cputlb.o
CC x86_64-softmmu/accel/tcg/tcg-runtime.o
/tmp/qemu-test/src/fpu/softfloat.c:3365:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3423:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ0 &= ~ ( ( (uint64_t) ( absZ1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3483:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ0 &= ~(((uint64_t)(absZ1<<1) == 0) & roundNearestEven);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3606:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3760:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig &= ~ ( ( ( roundBits ^ 0x200 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3987:21: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
~ ( ( (uint64_t) ( zSig1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:4003:22: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig0 &= ~ ( ( (uint64_t) ( zSig1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:4273:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig1 &= ~ ( ( zSig2 + zSig2 == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
8 errors generated.
CC x86_64-softmmu/accel/tcg/tcg-runtime-gvec.o
CC x86_64-softmmu/accel/tcg/cpu-exec.o
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: fpu/softfloat.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC x86_64-softmmu/accel/tcg/cpu-exec-common.o
/tmp/qemu-test/src/migration/ram.c:919:45: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
xbzrle_counters.encoding_rate = UINT64_MAX;
~ ^~~~~~~~~~
/usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'
---
18446744073709551615UL
^~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1
make: *** [Makefile:527: x86_64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 669, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=c58e006aef6f48e3a1993e62ae28e302', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-b1yphne3/src/docker-src.2020-06-17-06.49.07.27387:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=c58e006aef6f48e3a1993e62ae28e302
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-b1yphne3/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 5m51.519s
user 0m9.104s
The full log is available at
http://patchew.org/logs/20200617102019.29652-1-lma@suse.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-29 16:02 ` Eric Blake
@ 2020-06-30 15:40 ` Paolo Bonzini
0 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2020-06-30 15:40 UTC (permalink / raw)
To: Eric Blake, Stefan Hajnoczi, Lin Ma
Cc: fam, kwolf, qemu-devel, Stefan Hajnoczi, mreitz
On 29/06/20 18:02, Eric Blake wrote:
> - allocation implies that data comes from this layer of a backing chain, rather than deferring to a backing image
>
> - allocation implies that storage is reserved (that is, not sparse)
>
> It sounds like we are trying to represent the second question for scsi
> (namely, the same question that gets answered by lseek(SEEK_HOLE) for
> POSIX files), and not the first (namely, the question answered for qcow2
> images).
>
Yes, SCSI does not know about layers.
Paolo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-29 10:39 ` Stefan Hajnoczi
2020-06-29 13:01 ` Paolo Bonzini
@ 2020-06-29 16:02 ` Eric Blake
2020-06-30 15:40 ` Paolo Bonzini
1 sibling, 1 reply; 12+ messages in thread
From: Eric Blake @ 2020-06-29 16:02 UTC (permalink / raw)
To: Stefan Hajnoczi, Lin Ma
Cc: fam, kwolf, qemu-devel, mreitz, Stefan Hajnoczi, pbonzini
On 6/29/20 5:39 AM, Stefan Hajnoczi wrote:
>>> SCSI defines 3 values and QEMU can represent all of them:
>>>
>>> 0 - mapped or unknown
>>> 1 - deallocated
>>> 2 - anchored
>>>
>>> See the BDRV_BLOCK_* constants in include/block/block.h. The
>>> is_deallocated boolean is not enough to represent this state, but the
>>> bdrv_block_status() return value can be used instead.
>>
>> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
>> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
>>
>> I'd like to use these combinations to analyze the bdrv_block_status() return value:
>> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
>> 'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
>> Am I right?
>
> My understanding is that the SCSI status are mapped to QEMU block status
> as follows:
>
> allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
I haven't read the scsi code, but as written, I would assume that any
portion of the block status with this result has the following properties:
- reading sees guest-visible data; we cannot guarantee whether it is all
zero, and the data reserves actual space
> anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
- reading sees all zeros, but the disk has reserved actual space such
that future writes will not run out of space
> deallocated: !BDRV_BLOCK_DATA
- reading does not see data; future writes will have to reserve space.
I would expect this option might also have '| BDRV_BLOCK_ZERO' depending
on whether this particular scsi device guarantees that reads of an
unallocated portion see all zeros instead of unspecified garbage.
>
> I have CCed Eric Blake, who is familiar with block status.
Adding Vladimir in CC as well, as he has pending patches that try to
clear up the use of BDRV_BLOCK_* constants with regards to two different
queries:
- allocation implies that data comes from this layer of a backing chain,
rather than deferring to a backing image
- allocation implies that storage is reserved (that is, not sparse)
It sounds like we are trying to represent the second question for scsi
(namely, the same question that gets answered by lseek(SEEK_HOLE) for
POSIX files), and not the first (namely, the question answered for qcow2
images).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-29 10:39 ` Stefan Hajnoczi
@ 2020-06-29 13:01 ` Paolo Bonzini
2020-06-29 16:02 ` Eric Blake
1 sibling, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2020-06-29 13:01 UTC (permalink / raw)
To: Stefan Hajnoczi, Lin Ma; +Cc: fam, kwolf, qemu-devel, mreitz, Stefan Hajnoczi
On 29/06/20 12:39, Stefan Hajnoczi wrote:
>> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
>> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
>>
>> I'd like to use these combinations to analyze the bdrv_block_status() return value:
>> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
>> 'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
>> Am I right?
> My understanding is that the SCSI status are mapped to QEMU block status
> as follows:
>
> allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
> anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
> deallocated: !BDRV_BLOCK_DATA
I agree except that I wouldn't test BDRV_BLOCK_OFFSET_VALID. For
example a compressed cluster would still be reported as allocated even
if BDRV_BLOCK_OFFSET_VALID is cleared.
Paolo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-25 13:31 Lin Ma
@ 2020-06-29 10:39 ` Stefan Hajnoczi
2020-06-29 13:01 ` Paolo Bonzini
2020-06-29 16:02 ` Eric Blake
0 siblings, 2 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-06-29 10:39 UTC (permalink / raw)
To: Lin Ma; +Cc: fam, kwolf, qemu-devel, mreitz, Stefan Hajnoczi, pbonzini
[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]
On Thu, Jun 25, 2020 at 01:31:56PM +0000, Lin Ma wrote:
> On 2020-06-25 21:25, Lin Ma wrote:
> >> + /*
> >> + * 8 + 16 is the length in bytes of response header and
> >> + * one LBA status descriptor
> >> + */
> >> + memset(outbuf, 0, 8 + 16);
> >> + outbuf[3] = 20;
> >> + outbuf[8] = (req->cmd.lba >> 56) & 0xff;
> >> + outbuf[9] = (req->cmd.lba >> 48) & 0xff;
> >> + outbuf[10] = (req->cmd.lba >> 40) & 0xff;
> >> + outbuf[11] = (req->cmd.lba >> 32) & 0xff;
> >> + outbuf[12] = (req->cmd.lba >> 24) & 0xff;
> >> + outbuf[13] = (req->cmd.lba >> 16) & 0xff;
> >> + outbuf[14] = (req->cmd.lba >> 8) & 0xff;
> >> + outbuf[15] = req->cmd.lba & 0xff;
> >> + outbuf[16] = (*num_blocks >> 24) & 0xff;
> >> + outbuf[17] = (*num_blocks >> 16) & 0xff;
> >> + outbuf[18] = (*num_blocks >> 8) & 0xff;
> >> + outbuf[19] = *num_blocks & 0xff;
> >> + outbuf[20] = *is_deallocated ? 1 : 0;
> >
> > SCSI defines 3 values and QEMU can represent all of them:
> >
> > 0 - mapped or unknown
> > 1 - deallocated
> > 2 - anchored
> >
> > See the BDRV_BLOCK_* constants in include/block/block.h. The
> > is_deallocated boolean is not enough to represent this state, but the
> > bdrv_block_status() return value can be used instead.
>
> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
>
> I'd like to use these combinations to analyze the bdrv_block_status() return value:
> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
> 'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
> Am I right?
My understanding is that the SCSI status are mapped to QEMU block status
as follows:
allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
deallocated: !BDRV_BLOCK_DATA
I have CCed Eric Blake, who is familiar with block status.
> >> +}
> >> +
> >> static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> >> {
> >> SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> >> @@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> >>
> >> /* Protection, exponent and lowest lba field left blank. */
> >> break;
> >> + } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
> >> + if (req->cmd.lba > s->qdev.max_lba) {
> >> + goto illegal_lba;
> >> + }
> >> + scsi_disk_emulate_get_lba_status(req, outbuf);
> >> + r->iov.iov_len = req->cmd.xfer;
> >> + return r->iov.iov_len;
> >
> > Is there something tricky going on here with iov_len that prevents us
> > from using break here and sharing the functions normal return code path?
>
> Using 'break' here and following the normal return code path cause the assertion
> 'assert(!r->req.aiocb)'.
>
> In fact, There is a 'return' statement in the 'case SYNCHRONIZE_CACHE' in function
> scsi_disk_emulate_command, It causes the same assertion if no this 'return' statement.
> I borrowed this idea to avoid the assertion.
The assertion shows that this patch isn't asynchronous. I think the
assertion will pass once you convert GET_LBA_STATUS to async and then
a break statement will work.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
@ 2020-06-25 13:31 Lin Ma
2020-06-29 10:39 ` Stefan Hajnoczi
0 siblings, 1 reply; 12+ messages in thread
From: Lin Ma @ 2020-06-25 13:31 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: fam, kwolf, pbonzini, qemu-devel, mreitz
On 2020-06-25 21:25, Lin Ma wrote:
> From: Stefan Hajnoczi
> Sent: Monday, June 22, 2020 8:14 PM
> ...
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 387503e11b..9e3002ddaf 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
>> }
>> }
>>
>> +typedef struct GetLbaStatusCBData {
>> + uint32_t num_blocks;
>> + uint32_t is_deallocated;
>> + SCSIDiskReq *r;
>> +} GetLbaStatusCBData;
>> +
>> +static void scsi_get_lba_status_complete(void *opaque, int ret);
>> +
>> +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
>> +{
>> + SCSIDiskReq *r = data->r;
>> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> +
>> + assert(r->req.aiocb == NULL);
>> +
>> + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
>> + s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
>> +
>> + r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
>> + r->req.cmd.lba * s->qdev.blocksize,
>> + s->qdev.blocksize,
>> + scsi_get_lba_status_complete, data);
>> +}
>> +
>> +static void scsi_get_lba_status_complete(void *opaque, int ret)
>> +{
>> + GetLbaStatusCBData *data = opaque;
>> + SCSIDiskReq *r = data->r;
>> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> +
>> + assert(r->req.aiocb != NULL);
>> + r->req.aiocb = NULL;
>> +
>> + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
>> + if (scsi_disk_req_check_error(r, ret, true)) {
>> + g_free(data);
>> + goto done;
>> + }
>> +
>> + block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
>> + scsi_req_unref(&r->req);
>> + g_free(data);
>> +
>> +done:
>> + aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
>> +}
>> +
>> +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
>> +{
>> + SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>> + GetLbaStatusCBData *data;
>> + uint32_t *num_blocks;
>> + uint32_t *is_deallocated;
>> +
>> + data = g_new0(GetLbaStatusCBData, 1);
>> + data->r = r;
>> + num_blocks = &(data->num_blocks);
>> + is_deallocated = &(data->is_deallocated);
>> +
>> + scsi_req_ref(&r->req);
>> + scsi_get_lba_status_complete_noio(data, 0);
>
> scsi_get_lba_status_complete_noio() looks asynchronous. If the
> BlockDriver yields in .bdrv_co_block_status() then the operation has not
> completed yet when scsi_get_lba_status_complete_noio() returns. It is
> not safe to access the GetLbaStatusCBData data until the async operation
> is complete.
>
> Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
> use-after-free when *num_blocks and *is_deallocated are accessed.
>
> These issues can be solved by making this code asynchronous (similar to
> read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in
> the completion function before g_free(data) is called.
Emm, I need to dig into the code for better understanding your suggestion.
I used to think that I already completely make 'scsi_get_lba_status_complete_noio'
asynchronous.
>> +
>> + /*
>> + * 8 + 16 is the length in bytes of response header and
>> + * one LBA status descriptor
>> + */
>> + memset(outbuf, 0, 8 + 16);
>> + outbuf[3] = 20;
>> + outbuf[8] = (req->cmd.lba >> 56) & 0xff;
>> + outbuf[9] = (req->cmd.lba >> 48) & 0xff;
>> + outbuf[10] = (req->cmd.lba >> 40) & 0xff;
>> + outbuf[11] = (req->cmd.lba >> 32) & 0xff;
>> + outbuf[12] = (req->cmd.lba >> 24) & 0xff;
>> + outbuf[13] = (req->cmd.lba >> 16) & 0xff;
>> + outbuf[14] = (req->cmd.lba >> 8) & 0xff;
>> + outbuf[15] = req->cmd.lba & 0xff;
>> + outbuf[16] = (*num_blocks >> 24) & 0xff;
>> + outbuf[17] = (*num_blocks >> 16) & 0xff;
>> + outbuf[18] = (*num_blocks >> 8) & 0xff;
>> + outbuf[19] = *num_blocks & 0xff;
>> + outbuf[20] = *is_deallocated ? 1 : 0;
>
> SCSI defines 3 values and QEMU can represent all of them:
>
> 0 - mapped or unknown
> 1 - deallocated
> 2 - anchored
>
> See the BDRV_BLOCK_* constants in include/block/block.h. The
> is_deallocated boolean is not enough to represent this state, but the
> bdrv_block_status() return value can be used instead.
I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
I'd like to use these combinations to analyze the bdrv_block_status() return value:
'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
Am I right?
>> +}
>> +
>> static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>> {
>> SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>> @@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>>
>> /* Protection, exponent and lowest lba field left blank. */
>> break;
>> + } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
>> + if (req->cmd.lba > s->qdev.max_lba) {
>> + goto illegal_lba;
>> + }
>> + scsi_disk_emulate_get_lba_status(req, outbuf);
>> + r->iov.iov_len = req->cmd.xfer;
>> + return r->iov.iov_len;
>
> Is there something tricky going on here with iov_len that prevents us
> from using break here and sharing the functions normal return code path?
Using 'break' here and following the normal return code path cause the assertion
'assert(!r->req.aiocb)'.
In fact, There is a 'return' statement in the 'case SYNCHRONIZE_CACHE' in function
scsi_disk_emulate_command, It causes the same assertion if no this 'return' statement.
I borrowed this idea to avoid the assertion.
>> }
>> trace_scsi_disk_emulate_command_SAI_unsupported();
>> goto illegal_request;
>> diff --git a/include/block/accounting.h b/include/block/accounting.h
>> index 878b4c3581..645014fb0b 100644
>> --- a/include/block/accounting.h
>> +++ b/include/block/accounting.h
>> @@ -38,6 +38,7 @@ enum BlockAcctType {
>> BLOCK_ACCT_WRITE,
>> BLOCK_ACCT_FLUSH,
>> BLOCK_ACCT_UNMAP,
>> + BLOCK_ACCT_GET_LBA_STATUS,
>> BLOCK_MAX_IOTYPE,
>> };
>>
>> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
>> index 874176019e..b18377b214 100644
>> --- a/include/scsi/constants.h
>> +++ b/include/scsi/constants.h
>> @@ -154,6 +154,7 @@
>> * SERVICE ACTION IN subcodes
>> */
>> #define SAI_READ_CAPACITY_16 0x10
>> +#define SAI_GET_LBA_STATUS 0x12
>>
>> /*
>> * READ POSITION service action codes
>> --
>> 2.26.0
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-17 10:30 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
@ 2020-06-22 12:14 ` Stefan Hajnoczi
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-06-22 12:14 UTC (permalink / raw)
To: Lin Ma; +Cc: fam, kwolf, pbonzini, qemu-devel, mreitz
[-- Attachment #1: Type: text/plain, Size: 6163 bytes --]
On Wed, Jun 17, 2020 at 06:30:18PM +0800, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
> hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++++++++++++++++++++
> include/block/accounting.h | 1 +
> include/scsi/constants.h | 1 +
> 3 files changed, 92 insertions(+)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 387503e11b..9e3002ddaf 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
> }
> }
>
> +typedef struct GetLbaStatusCBData {
> + uint32_t num_blocks;
> + uint32_t is_deallocated;
> + SCSIDiskReq *r;
> +} GetLbaStatusCBData;
> +
> +static void scsi_get_lba_status_complete(void *opaque, int ret);
> +
> +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
> +{
> + SCSIDiskReq *r = data->r;
> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> + assert(r->req.aiocb == NULL);
> +
> + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
> + s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
> +
> + r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
> + r->req.cmd.lba * s->qdev.blocksize,
> + s->qdev.blocksize,
> + scsi_get_lba_status_complete, data);
> +}
> +
> +static void scsi_get_lba_status_complete(void *opaque, int ret)
> +{
> + GetLbaStatusCBData *data = opaque;
> + SCSIDiskReq *r = data->r;
> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> + assert(r->req.aiocb != NULL);
> + r->req.aiocb = NULL;
> +
> + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> + if (scsi_disk_req_check_error(r, ret, true)) {
> + g_free(data);
> + goto done;
> + }
> +
> + block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
> + scsi_req_unref(&r->req);
> + g_free(data);
> +
> +done:
> + aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
> +}
> +
> +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
> +{
> + SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> + GetLbaStatusCBData *data;
> + uint32_t *num_blocks;
> + uint32_t *is_deallocated;
> +
> + data = g_new0(GetLbaStatusCBData, 1);
> + data->r = r;
> + num_blocks = &(data->num_blocks);
> + is_deallocated = &(data->is_deallocated);
> +
> + scsi_req_ref(&r->req);
> + scsi_get_lba_status_complete_noio(data, 0);
scsi_get_lba_status_complete_noio() looks asynchronous. If the
BlockDriver yields in .bdrv_co_block_status() then the operation has not
completed yet when scsi_get_lba_status_complete_noio() returns. It is
not safe to access the GetLbaStatusCBData data until the async operation
is complete.
Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
use-after-free when *num_blocks and *is_deallocated are accessed.
These issues can be solved by making this code asynchronous (similar to
read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in
the completion function before g_free(data) is called.
> +
> + /*
> + * 8 + 16 is the length in bytes of response header and
> + * one LBA status descriptor
> + */
> + memset(outbuf, 0, 8 + 16);
> + outbuf[3] = 20;
> + outbuf[8] = (req->cmd.lba >> 56) & 0xff;
> + outbuf[9] = (req->cmd.lba >> 48) & 0xff;
> + outbuf[10] = (req->cmd.lba >> 40) & 0xff;
> + outbuf[11] = (req->cmd.lba >> 32) & 0xff;
> + outbuf[12] = (req->cmd.lba >> 24) & 0xff;
> + outbuf[13] = (req->cmd.lba >> 16) & 0xff;
> + outbuf[14] = (req->cmd.lba >> 8) & 0xff;
> + outbuf[15] = req->cmd.lba & 0xff;
> + outbuf[16] = (*num_blocks >> 24) & 0xff;
> + outbuf[17] = (*num_blocks >> 16) & 0xff;
> + outbuf[18] = (*num_blocks >> 8) & 0xff;
> + outbuf[19] = *num_blocks & 0xff;
> + outbuf[20] = *is_deallocated ? 1 : 0;
SCSI defines 3 values and QEMU can represent all of them:
0 - mapped or unknown
1 - deallocated
2 - anchored
See the BDRV_BLOCK_* constants in include/block/block.h. The
is_deallocated boolean is not enough to represent this state, but the
bdrv_block_status() return value can be used instead.
> +}
> +
> static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> {
> SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> @@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>
> /* Protection, exponent and lowest lba field left blank. */
> break;
> + } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
> + if (req->cmd.lba > s->qdev.max_lba) {
> + goto illegal_lba;
> + }
> + scsi_disk_emulate_get_lba_status(req, outbuf);
> + r->iov.iov_len = req->cmd.xfer;
> + return r->iov.iov_len;
Is there something tricky going on here with iov_len that prevents us
from using break here and sharing the functions normal return code path?
> }
> trace_scsi_disk_emulate_command_SAI_unsupported();
> goto illegal_request;
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index 878b4c3581..645014fb0b 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -38,6 +38,7 @@ enum BlockAcctType {
> BLOCK_ACCT_WRITE,
> BLOCK_ACCT_FLUSH,
> BLOCK_ACCT_UNMAP,
> + BLOCK_ACCT_GET_LBA_STATUS,
> BLOCK_MAX_IOTYPE,
> };
>
> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
> index 874176019e..b18377b214 100644
> --- a/include/scsi/constants.h
> +++ b/include/scsi/constants.h
> @@ -154,6 +154,7 @@
> * SERVICE ACTION IN subcodes
> */
> #define SAI_READ_CAPACITY_16 0x10
> +#define SAI_GET_LBA_STATUS 0x12
>
> /*
> * READ POSITION service action codes
> --
> 2.26.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-17 10:30 Lin Ma
@ 2020-06-17 10:30 ` Lin Ma
2020-06-22 12:14 ` Stefan Hajnoczi
0 siblings, 1 reply; 12+ messages in thread
From: Lin Ma @ 2020-06-17 10:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fam, kwolf, mreitz, stefanha, Lin Ma, pbonzini
Signed-off-by: Lin Ma <lma@suse.com>
---
hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++++++++++++++++++++
include/block/accounting.h | 1 +
include/scsi/constants.h | 1 +
3 files changed, 92 insertions(+)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..9e3002ddaf 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
}
}
+typedef struct GetLbaStatusCBData {
+ uint32_t num_blocks;
+ uint32_t is_deallocated;
+ SCSIDiskReq *r;
+} GetLbaStatusCBData;
+
+static void scsi_get_lba_status_complete(void *opaque, int ret);
+
+static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
+{
+ SCSIDiskReq *r = data->r;
+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+ assert(r->req.aiocb == NULL);
+
+ block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
+ s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
+
+ r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
+ r->req.cmd.lba * s->qdev.blocksize,
+ s->qdev.blocksize,
+ scsi_get_lba_status_complete, data);
+}
+
+static void scsi_get_lba_status_complete(void *opaque, int ret)
+{
+ GetLbaStatusCBData *data = opaque;
+ SCSIDiskReq *r = data->r;
+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+ assert(r->req.aiocb != NULL);
+ r->req.aiocb = NULL;
+
+ aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
+ if (scsi_disk_req_check_error(r, ret, true)) {
+ g_free(data);
+ goto done;
+ }
+
+ block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
+ scsi_req_unref(&r->req);
+ g_free(data);
+
+done:
+ aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
+}
+
+static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
+{
+ SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+ GetLbaStatusCBData *data;
+ uint32_t *num_blocks;
+ uint32_t *is_deallocated;
+
+ data = g_new0(GetLbaStatusCBData, 1);
+ data->r = r;
+ num_blocks = &(data->num_blocks);
+ is_deallocated = &(data->is_deallocated);
+
+ scsi_req_ref(&r->req);
+ scsi_get_lba_status_complete_noio(data, 0);
+
+ /*
+ * 8 + 16 is the length in bytes of response header and
+ * one LBA status descriptor
+ */
+ memset(outbuf, 0, 8 + 16);
+ outbuf[3] = 20;
+ outbuf[8] = (req->cmd.lba >> 56) & 0xff;
+ outbuf[9] = (req->cmd.lba >> 48) & 0xff;
+ outbuf[10] = (req->cmd.lba >> 40) & 0xff;
+ outbuf[11] = (req->cmd.lba >> 32) & 0xff;
+ outbuf[12] = (req->cmd.lba >> 24) & 0xff;
+ outbuf[13] = (req->cmd.lba >> 16) & 0xff;
+ outbuf[14] = (req->cmd.lba >> 8) & 0xff;
+ outbuf[15] = req->cmd.lba & 0xff;
+ outbuf[16] = (*num_blocks >> 24) & 0xff;
+ outbuf[17] = (*num_blocks >> 16) & 0xff;
+ outbuf[18] = (*num_blocks >> 8) & 0xff;
+ outbuf[19] = *num_blocks & 0xff;
+ outbuf[20] = *is_deallocated ? 1 : 0;
+}
+
static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
{
SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
/* Protection, exponent and lowest lba field left blank. */
break;
+ } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
+ if (req->cmd.lba > s->qdev.max_lba) {
+ goto illegal_lba;
+ }
+ scsi_disk_emulate_get_lba_status(req, outbuf);
+ r->iov.iov_len = req->cmd.xfer;
+ return r->iov.iov_len;
}
trace_scsi_disk_emulate_command_SAI_unsupported();
goto illegal_request;
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..645014fb0b 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -38,6 +38,7 @@ enum BlockAcctType {
BLOCK_ACCT_WRITE,
BLOCK_ACCT_FLUSH,
BLOCK_ACCT_UNMAP,
+ BLOCK_ACCT_GET_LBA_STATUS,
BLOCK_MAX_IOTYPE,
};
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..b18377b214 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -154,6 +154,7 @@
* SERVICE ACTION IN subcodes
*/
#define SAI_READ_CAPACITY_16 0x10
+#define SAI_GET_LBA_STATUS 0x12
/*
* READ POSITION service action codes
--
2.26.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-06-30 15:44 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 10:20 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:20 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
2020-06-17 10:20 ` [PATCH v2 2/3] block: Add GET LBA STATUS support Lin Ma
2020-06-17 10:20 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
2020-06-17 10:54 ` [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation no-reply
2020-06-17 10:30 Lin Ma
2020-06-17 10:30 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
2020-06-22 12:14 ` Stefan Hajnoczi
2020-06-25 13:31 Lin Ma
2020-06-29 10:39 ` Stefan Hajnoczi
2020-06-29 13:01 ` Paolo Bonzini
2020-06-29 16:02 ` Eric Blake
2020-06-30 15:40 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).