qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [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).