[PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation

[PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation

[Lin Ma]

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

[PATCH v2 1/3] block: Add bdrv_co_get_lba_status

[Lin Ma]

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

[PATCH v2 2/3] block: Add GET LBA STATUS support

[Lin Ma]

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

[PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

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

[PATCH v2 1/3] block: Add bdrv_co_get_lba_status

[Lin Ma]

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

Re: [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation

[no-reply]

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

Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status

[Stefan Hajnoczi]

[-- 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 --]

Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status

[Lin Ma]

[-- Attachment #1: Type: text/plain, Size: 4470 bytes --]

On 2020-06-25 20:59, Lin Ma wrote:
> From: Stefan Hajnoczi
> Sent: Monday, June 22, 2020 6:25 PM
> ...
>> 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.

I'll add the comments.

>> +{
>> +    BlockDriverState *bs = child->bs;
>
> Why does this function take a BdrvChild argument instead of
> BlockDriverState? Most I/O functions take BlockDriverState.

OK, I'll use BlockDriverState instead.

>> +    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.

As you mentioned in comment of patch 3/3, is_deallocated boolean is not enough
to represent 3 states. I'll keep the uint32_t *, but rename 'is_deallocated' to
'provisioning_status'

> Please initialize is_deallocated to false at the beginning of the
> function to avoid accidental uninitialized variable accesses in the
> caller.

It has already been initialized to 0 by 'data = g_new0(GetLbaStatusCBData, 1);'
in function scsi_disk_emulate_get_lba_status of patch 3/3, Do I still need to
initialize it at the beginning of this function?

>> +            } 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?

OK, I'll change it to '*num_blocks += pnum / bytes;'
The 'bytes' is the logical block size(default value is 512).
In fact, 'count' has the same meaning as 'pnum', It is the number of bytes
(including and immediately following the specified offset) that are known to
be in the same allocated/unallocated state. I'll rename the 'count' to 'pnum'.

Once this function returns, The number of contiguous logical blocks sharing
the same provisioning status as the specified logical block will be saved into
'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);
>
> 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.

I'll move it to include/block/block.h.

[-- Attachment #2: Type: text/html, Size: 9059 bytes --]

Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

On Thu, Jun 25, 2020 at 01:07:14PM +0000, Lin Ma wrote:
> On 2020-06-25 20:59, Lin Ma wrote:
> > Please initialize is_deallocated to false at the beginning of the
> > function to avoid accidental uninitialized variable accesses in the
> > caller.
> 
> It has already been initialized to 0 by 'data = g_new0(GetLbaStatusCBData, 1);'
> in function scsi_disk_emulate_get_lba_status of patch 3/3, Do I still need to
> initialize it at the beginning of this function?

It's safer to set output arguments in all code paths. That way callers
don't need to remember to initialize the variable to a specific value
(false in this case).

You can leave this assumptnion but please include it in the doc comment
so it's clear that callers are responsible for setting it to false.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]