* [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation
@ 2020-06-17 10:30 Lin Ma
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
` (3 more replies)
0 siblings, 4 replies; 10+ 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
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] 10+ messages in thread
* [PATCH v2 1/3] block: Add bdrv_co_get_lba_status
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
@ 2020-06-17 10:30 ` Lin Ma
2020-06-22 10:25 ` Stefan Hajnoczi
2020-06-17 10:30 ` [PATCH v2 2/3] block: Add GET LBA STATUS support Lin Ma
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ 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
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] 10+ messages in thread
* [PATCH v2 2/3] block: Add GET LBA STATUS support
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
@ 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-17 11:14 ` [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation no-reply
3 siblings, 0 replies; 10+ 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>
---
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] 10+ messages in thread
* [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
2020-06-17 10:30 ` [PATCH v2 2/3] block: Add GET LBA STATUS support Lin Ma
@ 2020-06-17 10:30 ` Lin Ma
2020-06-22 12:14 ` Stefan Hajnoczi
2020-06-17 11:14 ` [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation no-reply
3 siblings, 1 reply; 10+ 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] 10+ messages in thread
* Re: [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
` (2 preceding siblings ...)
2020-06-17 10:30 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
@ 2020-06-17 11:14 ` no-reply
3 siblings, 0 replies; 10+ messages in thread
From: no-reply @ 2020-06-17 11:14 UTC (permalink / raw)
To: lma; +Cc: fam, kwolf, qemu-devel, mreitz, stefanha, lma, pbonzini
Patchew URL: https://patchew.org/QEMU/20200617103018.18026-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 ===
GEN docs/interop/qemu-qmp-ref.txt
GEN docs/interop/qemu-qmp-ref.7
CC qga/commands.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)
CC qga/guest-agent-command-state.o
CC qga/main.o
CC qga/commands-posix.o
---
GEN docs/interop/qemu-ga-ref.html
GEN docs/interop/qemu-ga-ref.txt
GEN docs/interop/qemu-ga-ref.7
/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/multiboot.o
AS pc-bios/optionrom/linuxboot.o
CC pc-bios/optionrom/linuxboot_dma.o
LINK qemu-keymap
/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/kvmvapic.o
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)
AS pc-bios/optionrom/pvh.o
CC pc-bios/optionrom/pvh_main.o
BUILD pc-bios/optionrom/multiboot.img
---
BUILD pc-bios/optionrom/kvmvapic.img
BUILD pc-bios/optionrom/multiboot.raw
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)
/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)
BUILD pc-bios/optionrom/linuxboot.raw
BUILD pc-bios/optionrom/linuxboot_dma.raw
BUILD pc-bios/optionrom/kvmvapic.raw
BUILD pc-bios/optionrom/pvh.img
SIGN pc-bios/optionrom/multiboot.bin
/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)
SIGN pc-bios/optionrom/linuxboot.bin
SIGN pc-bios/optionrom/linuxboot_dma.bin
SIGN pc-bios/optionrom/kvmvapic.bin
BUILD pc-bios/optionrom/pvh.raw
SIGN pc-bios/optionrom/pvh.bin
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
LINK fsdev/virtfs-proxy-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)
/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 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)
/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
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)
LINK qemu-ga
/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)
/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-devices.h
---
CC x86_64-softmmu/accel/tcg/tcg-runtime-gvec.o
CC x86_64-softmmu/accel/tcg/cpu-exec.o
CC x86_64-softmmu/accel/tcg/cpu-exec-common.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.
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: fpu/softfloat.o] Error 1
make[1]: *** Waiting for unfinished jobs....
/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=3b000e4674de454ca1785efd680d2175', '-u', '1003', '--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/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-2shrqpbq/src/docker-src.2020-06-17-07.09.45.12034:/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=3b000e4674de454ca1785efd680d2175
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-2shrqpbq/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 4m33.543s
user 0m7.633s
The full log is available at
http://patchew.org/logs/20200617103018.18026-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] 10+ messages in thread
* Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
@ 2020-06-22 10:25 ` Stefan Hajnoczi
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2020-06-22 10:25 UTC (permalink / raw)
To: Lin Ma; +Cc: fam, kwolf, qemu-devel, mreitz, pbonzini
[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]
On Wed, Jun 17, 2020 at 06:30:16PM +0800, Lin Ma wrote:
> 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)
Missing doc comments.
> +{
> + BlockDriverState *bs = child->bs;
Why does this function take a BdrvChild argument instead of
BlockDriverState? Most I/O functions take BlockDriverState.
> + 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;
This is a boolean. Please use bool instead of uint32_t.
Please initialize is_deallocated to false at the beginning of the
function to avoid accidental uninitialized variable accesses in the
caller.
> + } else {
> + wanted_bit1 = 0;
> + }
> + first = false;
> + }
> + if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) {
> + (*num_blocks)++;
If there is a long span of allocated/deallocated blocks then this
function only increments num_blocks once without counting the number of
blocks. I expected something like num_blocks += pnum / block_size. What
is the relationship between bytes, count, and blocks in this function?
> + } 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);
Should this function be in include/block/block.h (the public API) so
that any part of QEMU can call it? It's not an internal API.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ 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
2020-06-29 9:18 ` 回复: " Lin Ma
0 siblings, 1 reply; 10+ 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] 10+ messages in thread
* 回复: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-22 12:14 ` Stefan Hajnoczi
@ 2020-06-29 9:18 ` Lin Ma
2020-07-08 12:29 ` Stefan Hajnoczi
0 siblings, 1 reply; 10+ messages in thread
From: Lin Ma @ 2020-06-29 9:18 UTC (permalink / raw)
To: 'Stefan Hajnoczi'
Cc: 'fam@euphon.net', 'kwolf@redhat.com',
'pbonzini@redhat.com', 'qemu-devel@nongnu.org',
'mreitz@redhat.com'
> -----邮件原件-----
> 发件人: Stefan Hajnoczi <stefanha@redhat.com>
> 发送时间: 2020年6月22日 20:14
> 收件人: Lin Ma <LMa@suse.com>
> 抄送: qemu-devel@nongnu.org; fam@euphon.net; kwolf@redhat.com;
> mreitz@redhat.com; pbonzini@redhat.com
> 主题: Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16
> command
>
> 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.
Got it, I'll fill the outbuf[] in the completion function in V3.
> 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.
I looked into block/io.c, The 'bdrv_co_pdiscard()', the 'bdrv_co_block_status' and the
'bdrv_co_flush()', They look similiar, They called corresponding bs->drv->bdrv_co_*()
or the bs->drv->bdrv_aio_*() between pair of blk_inc/dec_in_flight():
The 'bdrv_co_pdiscard()' calls bs->drv->bdrv_co_pdiscard() or bs->drv->bdrv_aio_pdiscard()
The 'bdrv_co_flush()' calls bs->drv->bdrv_co_flush*() or bs->drv->bdrv_aio_flush().
The 'bdrv_co_block_status' calls bs->drv->bdrv_co_block_status(). qemu contains the
coroutine version of block_status, no aio version of block_status.
About "making this code asynchronous", Well, In fact I havn't realized yet where the issue is.
If what you mean is that make the 'bdrv_co_get_lba_status()' asynchronous, How about
directly calling coroutine-based 'bdrv_co_block_status()' instead of 'bdrv_block_status()' in it?
Or could you please suggest more detailed information?
BTW, IMO the existing BlockDriver->bdrv_co_block_status() is enough, It's not necessary to
implement a drv->bdrv_aio_get_block_status() in BlockDrivers(say qcow2 or raw), Am I right?
I'm not familiar with qemu block layer and coroutine, Sorry for any inconvenience.
> > +
> > + /*
> > + * 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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 回复: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-29 9:18 ` 回复: " Lin Ma
@ 2020-07-08 12:29 ` Stefan Hajnoczi
2020-07-08 12:38 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2020-07-08 12:29 UTC (permalink / raw)
To: Lin Ma
Cc: 'fam@euphon.net', 'kwolf@redhat.com',
'pbonzini@redhat.com', 'qemu-devel@nongnu.org',
'mreitz@redhat.com'
[-- Attachment #1: Type: text/plain, Size: 7345 bytes --]
On Mon, Jun 29, 2020 at 09:18:59AM +0000, Lin Ma wrote:
>
>
> > -----邮件原件-----
> > 发件人: Stefan Hajnoczi <stefanha@redhat.com>
> > 发送时间: 2020年6月22日 20:14
> > 收件人: Lin Ma <LMa@suse.com>
> > 抄送: qemu-devel@nongnu.org; fam@euphon.net; kwolf@redhat.com;
> > mreitz@redhat.com; pbonzini@redhat.com
> > 主题: Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16
> > command
> >
> > 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.
>
> Got it, I'll fill the outbuf[] in the completion function in V3.
>
> > 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.
>
> I looked into block/io.c, The 'bdrv_co_pdiscard()', the 'bdrv_co_block_status' and the
> 'bdrv_co_flush()', They look similiar, They called corresponding bs->drv->bdrv_co_*()
> or the bs->drv->bdrv_aio_*() between pair of blk_inc/dec_in_flight():
> The 'bdrv_co_pdiscard()' calls bs->drv->bdrv_co_pdiscard() or bs->drv->bdrv_aio_pdiscard()
> The 'bdrv_co_flush()' calls bs->drv->bdrv_co_flush*() or bs->drv->bdrv_aio_flush().
> The 'bdrv_co_block_status' calls bs->drv->bdrv_co_block_status(). qemu contains the
> coroutine version of block_status, no aio version of block_status.
>
> About "making this code asynchronous", Well, In fact I havn't realized yet where the issue is.
> If what you mean is that make the 'bdrv_co_get_lba_status()' asynchronous, How about
> directly calling coroutine-based 'bdrv_co_block_status()' instead of 'bdrv_block_status()' in it?
> Or could you please suggest more detailed information?
> BTW, IMO the existing BlockDriver->bdrv_co_block_status() is enough, It's not necessary to
> implement a drv->bdrv_aio_get_block_status() in BlockDrivers(say qcow2 or raw), Am I right?
scsi_disk_emulate_get_lba_status() is called outside coroutine context.
It is expected to return without blocking so that other activity can
continue (e.g. the vCPU can continue execution).
scsi_disk_emulate_get_lba_status() cannot fill in outbuf because we may
not have fetched the LBA status yet when it needs to return.
Luckily there are other SCSI commands in scsi_disk_emulate_command()
that are asynchronous. You can follow their model:
case SYNCHRONIZE_CACHE:
/* The request is used as the AIO opaque value, so add a ref. */
scsi_req_ref(&r->req);
block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0,
BLOCK_ACCT_FLUSH);
r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_aio_complete, r);
return 0;
The request is not completed by scsi_disk_emulate_command(). Instead
blk_aio_flush() launches a flush operation and the SCSI request is
passed along as the argument to the scsi_aio_complete() completion
function.
Something similar is needed for GET_LBA_STATUS. Since there is no
bdrv_aio_block_status() you can create a coroutine instead of an aiocb:
static void coroutine_fn scsi_co_block_status(void *opaque)
{
int ret;
aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
ret = bdrv_co_block_status(...);
...fill in outbuf...
scsi_req_complete(&r->req, GOOD);
aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
scsi_req_unref(&r->req);
}
...in scsi_disk_emulate_command()...
scsi_req_ref(&r->req);
co = qemu_coroutine_create(scsi_co_block_status, r);
aio_co_schedule(blk_get_aio_context(s->qdev.conf.blk), co);
return 0;
This is just a sketch, I haven't checked the details. The trickiest
issue is probably how to deal with r->req.aiocb, which is normally set
for async requests. It will be necessary to study the code to figure out
a solution because there is no BlockAIOCB in this case (we're using a
coroutine instead).
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 回复: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-07-08 12:29 ` Stefan Hajnoczi
@ 2020-07-08 12:38 ` Paolo Bonzini
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2020-07-08 12:38 UTC (permalink / raw)
To: Stefan Hajnoczi, Lin Ma
Cc: 'fam@euphon.net', 'kwolf@redhat.com',
'qemu-devel@nongnu.org', 'mreitz@redhat.com'
[-- Attachment #1.1: Type: text/plain, Size: 1304 bytes --]
On 08/07/20 14:29, Stefan Hajnoczi wrote:
> Something similar is needed for GET_LBA_STATUS. Since there is no
> bdrv_aio_block_status() you can create a coroutine instead of an aiocb:
>
> static void coroutine_fn scsi_co_block_status(void *opaque)
> {
> int ret;
>
> aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
>
> ret = bdrv_co_block_status(...);
>
> ...fill in outbuf...
>
> scsi_req_complete(&r->req, GOOD);
>
> aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
>
> scsi_req_unref(&r->req);
> }
>
> ...in scsi_disk_emulate_command()...
> scsi_req_ref(&r->req);
> co = qemu_coroutine_create(scsi_co_block_status, r);
> aio_co_schedule(blk_get_aio_context(s->qdev.conf.blk), co);
> return 0;
>
> This is just a sketch, I haven't checked the details. The trickiest
> issue is probably how to deal with r->req.aiocb, which is normally set
> for async requests. It will be necessary to study the code to figure out
> a solution because there is no BlockAIOCB in this case (we're using a
> coroutine instead).
It's probably simplest to put the code above in block/block-backend.c,
in the form of blk_aio_block_status which would follow what is done in
blk_aio_prwv.
Paolo
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-07-08 22:18 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
2020-06-22 10:25 ` Stefan Hajnoczi
2020-06-17 10:30 ` [PATCH v2 2/3] block: Add GET LBA STATUS support 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-29 9:18 ` 回复: " Lin Ma
2020-07-08 12:29 ` Stefan Hajnoczi
2020-07-08 12:38 ` Paolo Bonzini
2020-06-17 11:14 ` [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.