[PATCH 0/2] support BLKSECDISCARD

[PATCH 0/2] support BLKSECDISCARD

[yadong.qi]

From: Yadong Qi <yadong.qi@intel.com>

Support BLKSECDISCARD passthrough for raw host_device backend.

For virtio-blk device:
    Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
    Add new virtio command: VIRTIO_BLK_T_SECDISCARD.

Usage:
    qemu-system-x86_64 \
    ... \
    -drive file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \
    -device virtio-blk-pci,drive=sd0,id=sd0_vblk \
    ...

Yadong Qi (2):
  block:hdev: support BLKSECDISCARD
  virtio-blk: support BLKSECDISCARD

 block.c                                     | 46 +++++++++++++++++++
 block/blkdebug.c                            |  5 ++-
 block/blklogwrites.c                        |  6 ++-
 block/blkreplay.c                           |  5 ++-
 block/block-backend.c                       | 15 ++++---
 block/copy-before-write.c                   |  5 ++-
 block/copy-on-read.c                        |  5 ++-
 block/coroutines.h                          |  6 ++-
 block/file-posix.c                          | 50 ++++++++++++++++++---
 block/filter-compress.c                     |  5 ++-
 block/io.c                                  |  5 ++-
 block/mirror.c                              |  5 ++-
 block/nbd.c                                 |  3 +-
 block/nvme.c                                |  3 +-
 block/preallocate.c                         |  5 ++-
 block/qcow2-refcount.c                      |  4 +-
 block/qcow2.c                               |  3 +-
 block/raw-format.c                          |  5 ++-
 block/throttle.c                            |  5 ++-
 hw/block/virtio-blk.c                       | 24 ++++++++--
 hw/ide/core.c                               |  1 +
 hw/nvme/ctrl.c                              |  3 +-
 hw/scsi/scsi-disk.c                         |  2 +-
 include/block/block.h                       | 13 +++++-
 include/block/block_int.h                   |  2 +-
 include/block/raw-aio.h                     |  4 +-
 include/standard-headers/linux/virtio_blk.h |  4 ++
 include/sysemu/block-backend.h              |  1 +
 tests/unit/test-block-iothread.c            |  9 ++--
 29 files changed, 195 insertions(+), 54 deletions(-)

-- 
2.25.1

[PATCH 1/2] block:hdev: support BLKSECDISCARD

[yadong.qi]

From: Yadong Qi <yadong.qi@intel.com>

Add a new option "secdiscard" for block drive. Parse it and
record it in bs->open_flags as bit(BDRV_O_SECDISCARD).

Add a new BDRV_REQ_SECDISCARD bit for secure discard request
from virtual device.

For host_device backend: implement by ioctl(BLKSECDISCARD) on
real host block device.
For other backend, no implementation.

E.g.:
    qemu-system-x86_64 \
    ... \
    -drive file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \
    -device virtio-blk-pci,drive=sd0,id=sd0_vblk \
    ...

Signed-off-by: Yadong Qi <yadong.qi@intel.com>
---
 block.c                          | 46 +++++++++++++++++++++++++++++
 block/blkdebug.c                 |  5 ++--
 block/blklogwrites.c             |  6 ++--
 block/blkreplay.c                |  5 ++--
 block/block-backend.c            | 15 ++++++----
 block/copy-before-write.c        |  5 ++--
 block/copy-on-read.c             |  5 ++--
 block/coroutines.h               |  6 ++--
 block/file-posix.c               | 50 ++++++++++++++++++++++++++++----
 block/filter-compress.c          |  5 ++--
 block/io.c                       |  5 ++--
 block/mirror.c                   |  5 ++--
 block/nbd.c                      |  3 +-
 block/nvme.c                     |  3 +-
 block/preallocate.c              |  5 ++--
 block/qcow2-refcount.c           |  4 +--
 block/qcow2.c                    |  3 +-
 block/raw-format.c               |  5 ++--
 block/throttle.c                 |  5 ++--
 hw/block/virtio-blk.c            |  2 +-
 hw/ide/core.c                    |  1 +
 hw/nvme/ctrl.c                   |  3 +-
 hw/scsi/scsi-disk.c              |  2 +-
 include/block/block.h            | 13 +++++++--
 include/block/block_int.h        |  2 +-
 include/block/raw-aio.h          |  4 ++-
 include/sysemu/block-backend.h   |  1 +
 tests/unit/test-block-iothread.c |  9 +++---
 28 files changed, 171 insertions(+), 52 deletions(-)

diff --git a/block.c b/block.c
index 580cb77a70..4f05e96d12 100644
--- a/block.c
+++ b/block.c
@@ -1128,6 +1128,32 @@ int bdrv_parse_discard_flags(const char *mode, int *flags)
     return 0;
 }
 
+/**
+ * Set open flags for a given secdiscard mode
+ *
+ * Return 0 on success, -1 if the secdiscard mode was invalid.
+ */
+int bdrv_parse_secdiscard_flags(const char *mode, int *flags, Error **errp)
+{
+    *flags &= ~BDRV_O_SECDISCARD;
+
+    if (!strcmp(mode, "off")) {
+        /* do nothing */
+    } else if (!strcmp(mode, "on")) {
+        if (!(*flags & BDRV_O_UNMAP)) {
+            error_setg(errp, "cannot enable secdiscard when discard is "
+                             "disabled!");
+            return -1;
+        }
+
+        *flags |= BDRV_O_SECDISCARD;
+    } else {
+        return -1;
+    }
+
+    return 0;
+}
+
 /**
  * Set open flags for a given cache mode
  *
@@ -1695,6 +1721,11 @@ QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "discard operation (ignore/off, unmap/on)",
         },
+        {
+            .name = BDRV_OPT_SECDISCARD,
+            .type = QEMU_OPT_STRING,
+            .help = "secure discard operation (off, on)",
+        },
         {
             .name = BDRV_OPT_FORCE_SHARE,
             .type = QEMU_OPT_BOOL,
@@ -1735,6 +1766,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
     const char *driver_name = NULL;
     const char *node_name = NULL;
     const char *discard;
+    const char *secdiscard;
     QemuOpts *opts;
     BlockDriver *drv;
     Error *local_err = NULL;
@@ -1829,6 +1861,16 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
         }
     }
 
+
+    secdiscard = qemu_opt_get(opts, BDRV_OPT_SECDISCARD);
+    if (secdiscard != NULL) {
+        if (bdrv_parse_secdiscard_flags(secdiscard, &bs->open_flags,
+                                        errp) != 0) {
+            ret = -EINVAL;
+            goto fail_opts;
+        }
+    }
+
     bs->detect_zeroes =
         bdrv_parse_detect_zeroes(opts, bs->open_flags, &local_err);
     if (local_err) {
@@ -3685,6 +3727,10 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
                                &flags, options, flags, options);
     }
 
+    if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_SECDISCARD), "on")) {
+            flags |= BDRV_O_SECDISCARD;
+    }
+
     bs->open_flags = flags;
     bs->options = options;
     options = qdict_clone_shallow(options);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index bbf2948703..b49bb6a3e9 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -717,7 +717,8 @@ static int coroutine_fn blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
 }
 
 static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
-                                             int64_t offset, int64_t bytes)
+                                             int64_t offset, int64_t bytes,
+                                             BdrvRequestFlags flags)
 {
     uint32_t align = bs->bl.pdiscard_alignment;
     int err;
@@ -747,7 +748,7 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
         return err;
     }
 
-    return bdrv_co_pdiscard(bs->file, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
 }
 
 static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index f7a251e91f..d8d81a40ae 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -456,7 +456,8 @@ static int coroutine_fn blk_log_writes_co_do_file_flush(BlkLogWritesFileReq *fr)
 static int coroutine_fn
 blk_log_writes_co_do_file_pdiscard(BlkLogWritesFileReq *fr)
 {
-    return bdrv_co_pdiscard(fr->bs->file, fr->offset, fr->bytes);
+    return bdrv_co_pdiscard(fr->bs->file, fr->offset, fr->bytes,
+                            fr->file_flags);
 }
 
 static int coroutine_fn
@@ -484,7 +485,8 @@ static int coroutine_fn blk_log_writes_co_flush_to_disk(BlockDriverState *bs)
 }
 
 static int coroutine_fn
-blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
+blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes,
+                                       BdrvRequestFlags flags)
 {
     return blk_log_writes_co_log(bs, offset, bytes, NULL, 0,
                                  blk_log_writes_co_do_file_pdiscard,
diff --git a/block/blkreplay.c b/block/blkreplay.c
index dcbe780ddb..65e66d0766 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -105,10 +105,11 @@ static int coroutine_fn blkreplay_co_pwrite_zeroes(BlockDriverState *bs,
 }
 
 static int coroutine_fn blkreplay_co_pdiscard(BlockDriverState *bs,
-                                              int64_t offset, int64_t bytes)
+                                              int64_t offset, int64_t bytes,
+                                              BdrvRequestFlags flags)
 {
     uint64_t reqid = blkreplay_next_id();
-    int ret = bdrv_co_pdiscard(bs->file, offset, bytes);
+    int ret = bdrv_co_pdiscard(bs->file, offset, bytes, flags);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
 
diff --git a/block/block-backend.c b/block/block-backend.c
index 12ef80ea17..f2c5776172 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1597,7 +1597,8 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
 
 /* To be called between exactly one pair of blk_inc/dec_in_flight() */
 int coroutine_fn
-blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
+blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
+                   BdrvRequestFlags flags)
 {
     int ret;
 
@@ -1608,7 +1609,7 @@ blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
         return ret;
     }
 
-    return bdrv_co_pdiscard(blk->root, offset, bytes);
+    return bdrv_co_pdiscard(blk->root, offset, bytes, flags);
 }
 
 static void blk_aio_pdiscard_entry(void *opaque)
@@ -1616,15 +1617,17 @@ static void blk_aio_pdiscard_entry(void *opaque)
     BlkAioEmAIOCB *acb = opaque;
     BlkRwCo *rwco = &acb->rwco;
 
-    rwco->ret = blk_co_do_pdiscard(rwco->blk, rwco->offset, acb->bytes);
+    rwco->ret = blk_co_do_pdiscard(rwco->blk, rwco->offset, acb->bytes,
+                                   rwco->flags);
     blk_aio_complete(acb);
 }
 
 BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
                              int64_t offset, int64_t bytes,
+                             BdrvRequestFlags flags,
                              BlockCompletionFunc *cb, void *opaque)
 {
-    return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_pdiscard_entry, 0,
+    return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_pdiscard_entry, flags,
                         cb, opaque);
 }
 
@@ -1634,7 +1637,7 @@ int coroutine_fn blk_co_pdiscard(BlockBackend *blk, int64_t offset,
     int ret;
 
     blk_inc_in_flight(blk);
-    ret = blk_co_do_pdiscard(blk, offset, bytes);
+    ret = blk_co_do_pdiscard(blk, offset, bytes, 0);
     blk_dec_in_flight(blk);
 
     return ret;
@@ -1645,7 +1648,7 @@ int blk_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
     int ret;
 
     blk_inc_in_flight(blk);
-    ret = blk_do_pdiscard(blk, offset, bytes);
+    ret = blk_do_pdiscard(blk, offset, bytes, 0);
     blk_dec_in_flight(blk);
 
     return ret;
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index c30a5ff8de..8d60a3028f 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -64,14 +64,15 @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
 }
 
 static int coroutine_fn cbw_co_pdiscard(BlockDriverState *bs,
-                                        int64_t offset, int64_t bytes)
+                                        int64_t offset, int64_t bytes,
+                                       BdrvRequestFlags flags)
 {
     int ret = cbw_do_copy_before_write(bs, offset, bytes, 0);
     if (ret < 0) {
         return ret;
     }
 
-    return bdrv_co_pdiscard(bs->file, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
 }
 
 static int coroutine_fn cbw_co_pwrite_zeroes(BlockDriverState *bs,
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 1fc7fb3333..52183cc9a2 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -201,9 +201,10 @@ static int coroutine_fn cor_co_pwrite_zeroes(BlockDriverState *bs,
 
 
 static int coroutine_fn cor_co_pdiscard(BlockDriverState *bs,
-                                        int64_t offset, int64_t bytes)
+                                        int64_t offset, int64_t bytes,
+                                       BdrvRequestFlags flags)
 {
-    return bdrv_co_pdiscard(bs->file, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
 }
 
 
diff --git a/block/coroutines.h b/block/coroutines.h
index c8c14a29c8..b0ba771bef 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -98,9 +98,11 @@ int coroutine_fn
 blk_co_do_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
 
 int generated_co_wrapper
-blk_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes);
+blk_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
+                BdrvRequestFlags flags);
 int coroutine_fn
-blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes);
+blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
+                   BdrvRequestFlags flags);
 
 int generated_co_wrapper blk_do_flush(BlockBackend *blk);
 int coroutine_fn blk_co_do_flush(BlockBackend *blk);
diff --git a/block/file-posix.c b/block/file-posix.c
index 7a27c83060..caa406e429 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -160,6 +160,7 @@ typedef struct BDRVRawState {
     bool is_xfs:1;
 #endif
     bool has_discard:1;
+    bool has_secdiscard:1;
     bool has_write_zeroes:1;
     bool discard_zeroes:1;
     bool use_linux_aio:1;
@@ -727,6 +728,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 #endif /* !defined(CONFIG_LINUX_IO_URING) */
 
     s->has_discard = true;
+    s->has_secdiscard = false;
     s->has_write_zeroes = true;
     if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
         s->needs_alignment = true;
@@ -765,6 +767,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
             s->discard_zeroes = true;
         }
 #endif
+
 #ifdef __linux__
         /* On Linux 3.10, BLKDISCARD leaves stale data in the page cache.  Do
          * not rely on the contents of discarded blocks unless using O_DIRECT.
@@ -1859,6 +1862,35 @@ static int handle_aiocb_discard(void *opaque)
     return ret;
 }
 
+static int handle_aiocb_secdiscard(void *opaque)
+{
+    RawPosixAIOData *aiocb = opaque;
+    int ret = -ENOTSUP;
+    BlockDriverState *bs = aiocb->bs;
+
+    if (!(bs->open_flags & BDRV_O_SECDISCARD)) {
+        return -ENOTSUP;
+    }
+
+    if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
+#ifdef BLKSECDISCARD
+        do {
+            uint64_t range[2] = { aiocb->aio_offset, aiocb->aio_nbytes };
+            if (ioctl(aiocb->aio_fildes, BLKSECDISCARD, range) == 0) {
+                return 0;
+            }
+        } while (errno == EINTR);
+
+        ret = translate_err(-errno);
+#endif
+    }
+
+    if (ret == -ENOTSUP) {
+        bs->open_flags &= ~BDRV_O_SECDISCARD;
+    }
+    return ret;
+}
+
 /*
  * Help alignment probing by allocating the first block.
  *
@@ -2953,7 +2985,7 @@ static void raw_account_discard(BDRVRawState *s, uint64_t nbytes, int ret)
 
 static coroutine_fn int
 raw_do_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes,
-                bool blkdev)
+                bool blkdev, BdrvRequestFlags flags)
 {
     BDRVRawState *s = bs->opaque;
     RawPosixAIOData acb;
@@ -2971,15 +3003,20 @@ raw_do_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes,
         acb.aio_type |= QEMU_AIO_BLKDEV;
     }
 
-    ret = raw_thread_pool_submit(bs, handle_aiocb_discard, &acb);
+    if (flags & BDRV_REQ_SECDISCARD) {
+        ret = raw_thread_pool_submit(bs, handle_aiocb_secdiscard, &acb);
+    } else {
+        ret = raw_thread_pool_submit(bs, handle_aiocb_discard, &acb);
+    }
     raw_account_discard(s, bytes, ret);
     return ret;
 }
 
 static coroutine_fn int
-raw_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
+raw_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes,
+                BdrvRequestFlags flags)
 {
-    return raw_do_pdiscard(bs, offset, bytes, false);
+    return raw_do_pdiscard(bs, offset, bytes, false, flags);
 }
 
 static int coroutine_fn
@@ -3602,7 +3639,8 @@ hdev_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 #endif /* linux */
 
 static coroutine_fn int
-hdev_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
+hdev_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes,
+                                       BdrvRequestFlags flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -3612,7 +3650,7 @@ hdev_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
         raw_account_discard(s, bytes, ret);
         return ret;
     }
-    return raw_do_pdiscard(bs, offset, bytes, true);
+    return raw_do_pdiscard(bs, offset, bytes, true, flags);
 }
 
 static coroutine_fn int hdev_co_pwrite_zeroes(BlockDriverState *bs,
diff --git a/block/filter-compress.c b/block/filter-compress.c
index d5be538619..aced5d6c8f 100644
--- a/block/filter-compress.c
+++ b/block/filter-compress.c
@@ -94,9 +94,10 @@ static int coroutine_fn compress_co_pwrite_zeroes(BlockDriverState *bs,
 
 
 static int coroutine_fn compress_co_pdiscard(BlockDriverState *bs,
-                                             int64_t offset, int64_t bytes)
+                                             int64_t offset, int64_t bytes,
+                                             BdrvRequestFlags flags)
 {
-    return bdrv_co_pdiscard(bs->file, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
 }
 
 
diff --git a/block/io.c b/block/io.c
index bb0a254def..b6ff244795 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3054,7 +3054,7 @@ early_exit:
 }
 
 int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
-                                  int64_t bytes)
+                                  int64_t bytes, BdrvRequestFlags flags)
 {
     BdrvTrackedRequest req;
     int ret;
@@ -3139,8 +3139,9 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
             ret = -ENOMEDIUM;
             goto out;
         }
+
         if (bs->drv->bdrv_co_pdiscard) {
-            ret = bs->drv->bdrv_co_pdiscard(bs, offset, num);
+            ret = bs->drv->bdrv_co_pdiscard(bs, offset, num, flags);
         } else {
             BlockAIOCB *acb;
             CoroutineIOCompletion co = {
diff --git a/block/mirror.c b/block/mirror.c
index efec2c7674..d91a6fe2ac 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1440,7 +1440,7 @@ static int coroutine_fn bdrv_mirror_top_do_write(BlockDriverState *bs,
         break;
 
     case MIRROR_METHOD_DISCARD:
-        ret = bdrv_co_pdiscard(bs->backing, offset, bytes);
+        ret = bdrv_co_pdiscard(bs->backing, offset, bytes, 0);
         break;
 
     default:
@@ -1516,7 +1516,8 @@ static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
 }
 
 static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
-    int64_t offset, int64_t bytes)
+    int64_t offset, int64_t bytes,
+    BdrvRequestFlags flags)
 {
     return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_DISCARD, offset, bytes,
                                     NULL, 0);
diff --git a/block/nbd.c b/block/nbd.c
index 5ef462db1b..54fc433c0f 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1296,7 +1296,8 @@ static int nbd_client_co_flush(BlockDriverState *bs)
 }
 
 static int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset,
-                                  int64_t bytes)
+                                  int64_t bytes,
+                                  BdrvRequestFlags flags)
 {
     BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
     NBDRequest request = {
diff --git a/block/nvme.c b/block/nvme.c
index e4f336d79c..217523438a 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1363,7 +1363,8 @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,
 
 static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
                                          int64_t offset,
-                                         int64_t bytes)
+                                         int64_t bytes,
+                                         BdrvRequestFlags flags)
 {
     BDRVNVMeState *s = bs->opaque;
     NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
diff --git a/block/preallocate.c b/block/preallocate.c
index 1d4233f730..4a098747d1 100644
--- a/block/preallocate.c
+++ b/block/preallocate.c
@@ -235,9 +235,10 @@ static coroutine_fn int preallocate_co_preadv_part(
 }
 
 static int coroutine_fn preallocate_co_pdiscard(BlockDriverState *bs,
-                                               int64_t offset, int64_t bytes)
+                                               int64_t offset, int64_t bytes,
+                                               BdrvRequestFlags flags)
 {
-    return bdrv_co_pdiscard(bs->file, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes, flags);
 }
 
 static bool can_write_resize(uint64_t perm)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 4614572252..6a81dfd13a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -738,7 +738,7 @@ void qcow2_process_discards(BlockDriverState *bs, int ret)
 
         /* Discard is optional, ignore the return value */
         if (ret >= 0) {
-            int r2 = bdrv_pdiscard(bs->file, d->offset, d->bytes);
+            int r2 = bdrv_pdiscard(bs->file, d->offset, d->bytes, 0);
             if (r2 < 0) {
                 trace_qcow2_process_discards_failed_region(d->offset, d->bytes,
                                                            r2);
@@ -1169,7 +1169,7 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
              ctype == QCOW2_CLUSTER_ZERO_ALLOC))
         {
             bdrv_pdiscard(s->data_file, l2_entry & L2E_OFFSET_MASK,
-                          s->cluster_size);
+                          s->cluster_size, 0);
         }
         return;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index d509016756..258f4f94e0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3996,7 +3996,8 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
 }
 
 static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
-                                          int64_t offset, int64_t bytes)
+                                          int64_t offset, int64_t bytes,
+                                          BdrvRequestFlags flags)
 {
     int ret;
     BDRVQcow2State *s = bs->opaque;
diff --git a/block/raw-format.c b/block/raw-format.c
index bda757fd19..d149982e8e 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -302,7 +302,8 @@ static int coroutine_fn raw_co_pwrite_zeroes(BlockDriverState *bs,
 }
 
 static int coroutine_fn raw_co_pdiscard(BlockDriverState *bs,
-                                        int64_t offset, int64_t bytes)
+                                        int64_t offset, int64_t bytes,
+                                        BdrvRequestFlags flags)
 {
     int ret;
 
@@ -310,7 +311,7 @@ static int coroutine_fn raw_co_pdiscard(BlockDriverState *bs,
     if (ret) {
         return ret;
     }
-    return bdrv_co_pdiscard(bs->file, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes, flags);
 }
 
 static int64_t raw_getlength(BlockDriverState *bs)
diff --git a/block/throttle.c b/block/throttle.c
index 6e8d52fa24..d7e6967296 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -145,12 +145,13 @@ static int coroutine_fn throttle_co_pwrite_zeroes(BlockDriverState *bs,
 }
 
 static int coroutine_fn throttle_co_pdiscard(BlockDriverState *bs,
-                                             int64_t offset, int64_t bytes)
+                                             int64_t offset, int64_t bytes,
+                                             BdrvRequestFlags flags)
 {
     ThrottleGroupMember *tgm = bs->opaque;
     throttle_group_co_io_limits_intercept(tgm, bytes, true);
 
-    return bdrv_co_pdiscard(bs->file, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes, flags);
 }
 
 static int coroutine_fn throttle_co_pwritev_compressed(BlockDriverState *bs,
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f139cd7cc9..dbc4c5a3cd 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -600,7 +600,7 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
             goto err;
         }
 
-        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
+        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, 0,
                          virtio_blk_discard_write_zeroes_complete, req);
     }
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index e28f8aad61..47c6cf116d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -483,6 +483,7 @@ static void ide_issue_trim_cb(void *opaque, int ret)
                 iocb->aiocb = blk_aio_pdiscard(s->blk,
                                                sector << BDRV_SECTOR_BITS,
                                                count << BDRV_SECTOR_BITS,
+                                               0,
                                                ide_issue_trim_cb, opaque);
                 return;
             }
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 6a571d18cf..45c45b83c7 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -2325,7 +2325,7 @@ next:
     }
 
     iocb->aiocb = blk_aio_pdiscard(ns->blkconf.blk, nvme_l2b(ns, slba),
-                                   nvme_l2b(ns, nlb),
+                                   nvme_l2b(ns, nlb), 0,
                                    nvme_dsm_md_cb, iocb);
     return;
 
@@ -5429,6 +5429,7 @@ static uint16_t nvme_format(NvmeCtrl *n, NvmeRequest *req)
     uint32_t nsid = le32_to_cpu(req->cmd.nsid);
     uint16_t status;
 
+
     iocb = qemu_aio_get(&nvme_format_aiocb_info, NULL, nvme_misc_cb, req);
 
     iocb->req = req;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e8a547dbb7..b83ab3e471 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1634,7 +1634,7 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
         r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
                                         r->sector * BDRV_SECTOR_SIZE,
                                         r->sector_count * BDRV_SECTOR_SIZE,
-                                        scsi_unmap_complete, data);
+                                        0, scsi_unmap_complete, data);
         data->count--;
         data->inbuf += 16;
         return;
diff --git a/include/block/block.h b/include/block/block.h
index e5dd22b034..bfa26de835 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -87,6 +87,11 @@ typedef enum {
      */
     BDRV_REQ_NO_WAIT = 0x400,
 
+    /*
+     * Request for secure discard
+     */
+    BDRV_REQ_SECDISCARD = 0x800,
+
     /* Mask of valid flags */
     BDRV_REQ_MASK               = 0x7ff,
 } BdrvRequestFlags;
@@ -122,6 +127,7 @@ typedef struct HDGeometry {
 #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
 #define BDRV_O_AUTO_RDONLY 0x20000 /* degrade to read-only if opening read-write fails */
 #define BDRV_O_IO_URING    0x40000 /* use io_uring instead of the thread pool */
+#define BDRV_O_SECDISCARD  0x80000 /* guest SECDISCARD operations */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
@@ -134,6 +140,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_READ_ONLY      "read-only"
 #define BDRV_OPT_AUTO_READ_ONLY "auto-read-only"
 #define BDRV_OPT_DISCARD        "discard"
+#define BDRV_OPT_SECDISCARD     "secdiscard"
 #define BDRV_OPT_FORCE_SHARE    "force-share"
 
 
@@ -370,6 +377,7 @@ int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
 int bdrv_parse_aio(const char *mode, int *flags);
 int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
+int bdrv_parse_secdiscard_flags(const char *mode, int *flags, Error **errp);
 BdrvChild *bdrv_open_child(const char *filename,
                            QDict *options, const char *bdref_key,
                            BlockDriverState* parent,
@@ -513,8 +521,9 @@ void bdrv_drain_all(void);
                    cond); })
 
 int generated_co_wrapper bdrv_pdiscard(BdrvChild *child, int64_t offset,
-                                       int64_t bytes);
-int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
+                                       int64_t bytes, BdrvRequestFlags flags);
+int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes,
+                     BdrvRequestFlags flags);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f4c75e8ba9..773e5131d4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -303,7 +303,7 @@ struct BlockDriver {
     int coroutine_fn (*bdrv_co_pwrite_zeroes)(BlockDriverState *bs,
         int64_t offset, int64_t bytes, BdrvRequestFlags flags);
     int coroutine_fn (*bdrv_co_pdiscard)(BlockDriverState *bs,
-        int64_t offset, int64_t bytes);
+        int64_t offset, int64_t bytes, BdrvRequestFlags flags);
 
     /* Map [offset, offset + nbytes) range onto a child of @bs to copy from,
      * and invoke bdrv_co_copy_range_from(child, ...), or invoke
diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index 21fc10c4c9..d9138277e3 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -29,6 +29,7 @@
 #define QEMU_AIO_WRITE_ZEROES 0x0020
 #define QEMU_AIO_COPY_RANGE   0x0040
 #define QEMU_AIO_TRUNCATE     0x0080
+#define QEMU_AIO_SECDISCARD   0x0100
 #define QEMU_AIO_TYPE_MASK \
         (QEMU_AIO_READ | \
          QEMU_AIO_WRITE | \
@@ -37,7 +38,8 @@
          QEMU_AIO_DISCARD | \
          QEMU_AIO_WRITE_ZEROES | \
          QEMU_AIO_COPY_RANGE | \
-         QEMU_AIO_TRUNCATE)
+         QEMU_AIO_TRUNCATE | \
+         QEMU_AIO_SECDISCARD)
 
 /* AIO flags */
 #define QEMU_AIO_MISALIGNED   0x1000
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index e5e1524f06..53630ff791 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -179,6 +179,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,
 BlockAIOCB *blk_aio_flush(BlockBackend *blk,
                           BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
+                             BdrvRequestFlags flags,
                              BlockCompletionFunc *cb, void *opaque);
 void blk_aio_cancel(BlockAIOCB *acb);
 void blk_aio_cancel_async(BlockAIOCB *acb);
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index aea660aeed..fa0a1b0fed 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -48,7 +48,8 @@ static int coroutine_fn bdrv_test_co_pwritev(BlockDriverState *bs,
 }
 
 static int coroutine_fn bdrv_test_co_pdiscard(BlockDriverState *bs,
-                                              int64_t offset, int64_t bytes)
+                                              int64_t offset, int64_t bytes,
+                                              BdrvRequestFlags flags)
 {
     return 0;
 }
@@ -164,16 +165,16 @@ static void test_sync_op_pdiscard(BdrvChild *c)
 
     /* Normal success path */
     c->bs->open_flags |= BDRV_O_UNMAP;
-    ret = bdrv_pdiscard(c, 0, 512);
+    ret = bdrv_pdiscard(c, 0, 512, 0);
     g_assert_cmpint(ret, ==, 0);
 
     /* Early success: UNMAP not supported */
     c->bs->open_flags &= ~BDRV_O_UNMAP;
-    ret = bdrv_pdiscard(c, 0, 512);
+    ret = bdrv_pdiscard(c, 0, 512, 0);
     g_assert_cmpint(ret, ==, 0);
 
     /* Early error: Negative offset */
-    ret = bdrv_pdiscard(c, -2, 512);
+    ret = bdrv_pdiscard(c, -2, 512, 0);
     g_assert_cmpint(ret, ==, -EIO);
 }
 
-- 
2.25.1

Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD

[Kevin Wolf]

Am 15.11.2021 um 05:51 hat yadong.qi@intel.com geschrieben:
> From: Yadong Qi <yadong.qi@intel.com>
> 
> Add a new option "secdiscard" for block drive. Parse it and
> record it in bs->open_flags as bit(BDRV_O_SECDISCARD).
> 
> Add a new BDRV_REQ_SECDISCARD bit for secure discard request
> from virtual device.
> 
> For host_device backend: implement by ioctl(BLKSECDISCARD) on
> real host block device.
> For other backend, no implementation.
> 
> E.g.:
>     qemu-system-x86_64 \
>     ... \
>     -drive file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \
>     -device virtio-blk-pci,drive=sd0,id=sd0_vblk \
>     ...
> 
> Signed-off-by: Yadong Qi <yadong.qi@intel.com>
> ---
>  block.c                          | 46 +++++++++++++++++++++++++++++
>  block/blkdebug.c                 |  5 ++--
>  block/blklogwrites.c             |  6 ++--
>  block/blkreplay.c                |  5 ++--
>  block/block-backend.c            | 15 ++++++----
>  block/copy-before-write.c        |  5 ++--
>  block/copy-on-read.c             |  5 ++--
>  block/coroutines.h               |  6 ++--
>  block/file-posix.c               | 50 ++++++++++++++++++++++++++++----
>  block/filter-compress.c          |  5 ++--
>  block/io.c                       |  5 ++--
>  block/mirror.c                   |  5 ++--
>  block/nbd.c                      |  3 +-
>  block/nvme.c                     |  3 +-
>  block/preallocate.c              |  5 ++--
>  block/qcow2-refcount.c           |  4 +--
>  block/qcow2.c                    |  3 +-
>  block/raw-format.c               |  5 ++--
>  block/throttle.c                 |  5 ++--
>  hw/block/virtio-blk.c            |  2 +-
>  hw/ide/core.c                    |  1 +
>  hw/nvme/ctrl.c                   |  3 +-
>  hw/scsi/scsi-disk.c              |  2 +-
>  include/block/block.h            | 13 +++++++--
>  include/block/block_int.h        |  2 +-
>  include/block/raw-aio.h          |  4 ++-
>  include/sysemu/block-backend.h   |  1 +
>  tests/unit/test-block-iothread.c |  9 +++---
>  28 files changed, 171 insertions(+), 52 deletions(-)

Notably absent: qapi/block-core.json. Without changing this, the option
can't be available in -blockdev, which is the primary option to configure
block device backends.

This patch seems to contain multiple logical changes that should be
split into separate patches:

* Adding a flags parameter to .bdrv_co_pdiscard

* Support for the new feature in the core block layer (should be done
  with -blockdev)

* Convenience magic for -drive (BDRV_O_SECDISCARD). It's not clear that
  this should be done at all because the option is really specific to
  one single block driver (file-posix). I think in your patch, all
  other block drivers silently ignore the option, which is not what we
  want.

> diff --git a/block.c b/block.c
> index 580cb77a70..4f05e96d12 100644
> --- a/block.c
> +++ b/block.c
> @@ -1128,6 +1128,32 @@ int bdrv_parse_discard_flags(const char *mode, int *flags)
>      return 0;
>  }
>  
> +/**
> + * Set open flags for a given secdiscard mode
> + *
> + * Return 0 on success, -1 if the secdiscard mode was invalid.
> + */
> +int bdrv_parse_secdiscard_flags(const char *mode, int *flags, Error **errp)
> +{
> +    *flags &= ~BDRV_O_SECDISCARD;
> +
> +    if (!strcmp(mode, "off")) {
> +        /* do nothing */
> +    } else if (!strcmp(mode, "on")) {
> +        if (!(*flags & BDRV_O_UNMAP)) {
> +            error_setg(errp, "cannot enable secdiscard when discard is "
> +                             "disabled!");
> +            return -1;
> +        }

This check has nothing to do with parsing the option, it's validating
its value.

You don't even need a new function to parse it, because there is already
qemu_opt_get_bool(). Duplicating it means only that you're inconsistent
with other boolean options, which alternatively accept "yes"/"no",
"true"/"false", "y/n".

> +
> +        *flags |= BDRV_O_SECDISCARD;
> +    } else {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  /**
>   * Set open flags for a given cache mode
>   *
> @@ -1695,6 +1721,11 @@ QemuOptsList bdrv_runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "discard operation (ignore/off, unmap/on)",
>          },
> +        {
> +            .name = BDRV_OPT_SECDISCARD,
> +            .type = QEMU_OPT_STRING,
> +            .help = "secure discard operation (off, on)",
> +        },
>          {
>              .name = BDRV_OPT_FORCE_SHARE,
>              .type = QEMU_OPT_BOOL,
> @@ -1735,6 +1766,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
>      const char *driver_name = NULL;
>      const char *node_name = NULL;
>      const char *discard;
> +    const char *secdiscard;
>      QemuOpts *opts;
>      BlockDriver *drv;
>      Error *local_err = NULL;
> @@ -1829,6 +1861,16 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
>          }
>      }
>  
> +
> +    secdiscard = qemu_opt_get(opts, BDRV_OPT_SECDISCARD);
> +    if (secdiscard != NULL) {
> +        if (bdrv_parse_secdiscard_flags(secdiscard, &bs->open_flags,
> +                                        errp) != 0) {
> +            ret = -EINVAL;
> +            goto fail_opts;
> +        }
> +    }
> +
>      bs->detect_zeroes =
>          bdrv_parse_detect_zeroes(opts, bs->open_flags, &local_err);
>      if (local_err) {
> @@ -3685,6 +3727,10 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>                                 &flags, options, flags, options);
>      }
>  
> +    if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_SECDISCARD), "on")) {
> +            flags |= BDRV_O_SECDISCARD;

Indentation is off.

> +    }
> +
>      bs->open_flags = flags;
>      bs->options = options;
>      options = qdict_clone_shallow(options);
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index bbf2948703..b49bb6a3e9 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -717,7 +717,8 @@ static int coroutine_fn blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
>  }
>  
>  static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
> -                                             int64_t offset, int64_t bytes)
> +                                             int64_t offset, int64_t bytes,
> +                                             BdrvRequestFlags flags)
>  {
>      uint32_t align = bs->bl.pdiscard_alignment;
>      int err;
> @@ -747,7 +748,7 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
>          return err;
>      }
>  
> -    return bdrv_co_pdiscard(bs->file, offset, bytes);
> +    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
>  }
>  
>  static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,
> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> index f7a251e91f..d8d81a40ae 100644
> --- a/block/blklogwrites.c
> +++ b/block/blklogwrites.c
> @@ -456,7 +456,8 @@ static int coroutine_fn blk_log_writes_co_do_file_flush(BlkLogWritesFileReq *fr)
>  static int coroutine_fn
>  blk_log_writes_co_do_file_pdiscard(BlkLogWritesFileReq *fr)
>  {
> -    return bdrv_co_pdiscard(fr->bs->file, fr->offset, fr->bytes);
> +    return bdrv_co_pdiscard(fr->bs->file, fr->offset, fr->bytes,
> +                            fr->file_flags);
>  }
>  
>  static int coroutine_fn
> @@ -484,7 +485,8 @@ static int coroutine_fn blk_log_writes_co_flush_to_disk(BlockDriverState *bs)
>  }
>  
>  static int coroutine_fn
> -blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
> +blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes,
> +                                       BdrvRequestFlags flags)
>  {
>      return blk_log_writes_co_log(bs, offset, bytes, NULL, 0,
>                                   blk_log_writes_co_do_file_pdiscard,
> diff --git a/block/blkreplay.c b/block/blkreplay.c
> index dcbe780ddb..65e66d0766 100644
> --- a/block/blkreplay.c
> +++ b/block/blkreplay.c
> @@ -105,10 +105,11 @@ static int coroutine_fn blkreplay_co_pwrite_zeroes(BlockDriverState *bs,
>  }
>  
>  static int coroutine_fn blkreplay_co_pdiscard(BlockDriverState *bs,
> -                                              int64_t offset, int64_t bytes)
> +                                              int64_t offset, int64_t bytes,
> +                                              BdrvRequestFlags flags)
>  {
>      uint64_t reqid = blkreplay_next_id();
> -    int ret = bdrv_co_pdiscard(bs->file, offset, bytes);
> +    int ret = bdrv_co_pdiscard(bs->file, offset, bytes, flags);
>      block_request_create(reqid, bs, qemu_coroutine_self());
>      qemu_coroutine_yield();
>  
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 12ef80ea17..f2c5776172 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1597,7 +1597,8 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
>  
>  /* To be called between exactly one pair of blk_inc/dec_in_flight() */
>  int coroutine_fn
> -blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
> +blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
> +                   BdrvRequestFlags flags)
>  {
>      int ret;
>  
> @@ -1608,7 +1609,7 @@ blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
>          return ret;
>      }
>  
> -    return bdrv_co_pdiscard(blk->root, offset, bytes);
> +    return bdrv_co_pdiscard(blk->root, offset, bytes, flags);
>  }
>  
>  static void blk_aio_pdiscard_entry(void *opaque)
> @@ -1616,15 +1617,17 @@ static void blk_aio_pdiscard_entry(void *opaque)
>      BlkAioEmAIOCB *acb = opaque;
>      BlkRwCo *rwco = &acb->rwco;
>  
> -    rwco->ret = blk_co_do_pdiscard(rwco->blk, rwco->offset, acb->bytes);
> +    rwco->ret = blk_co_do_pdiscard(rwco->blk, rwco->offset, acb->bytes,
> +                                   rwco->flags);
>      blk_aio_complete(acb);
>  }
>  
>  BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
>                               int64_t offset, int64_t bytes,
> +                             BdrvRequestFlags flags,
>                               BlockCompletionFunc *cb, void *opaque)
>  {
> -    return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_pdiscard_entry, 0,
> +    return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_pdiscard_entry, flags,
>                          cb, opaque);
>  }
>  
> @@ -1634,7 +1637,7 @@ int coroutine_fn blk_co_pdiscard(BlockBackend *blk, int64_t offset,
>      int ret;
>  
>      blk_inc_in_flight(blk);
> -    ret = blk_co_do_pdiscard(blk, offset, bytes);
> +    ret = blk_co_do_pdiscard(blk, offset, bytes, 0);
>      blk_dec_in_flight(blk);
>  
>      return ret;
> @@ -1645,7 +1648,7 @@ int blk_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
>      int ret;
>  
>      blk_inc_in_flight(blk);
> -    ret = blk_do_pdiscard(blk, offset, bytes);
> +    ret = blk_do_pdiscard(blk, offset, bytes, 0);
>      blk_dec_in_flight(blk);
>  
>      return ret;
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index c30a5ff8de..8d60a3028f 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -64,14 +64,15 @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
>  }
>  
>  static int coroutine_fn cbw_co_pdiscard(BlockDriverState *bs,
> -                                        int64_t offset, int64_t bytes)
> +                                        int64_t offset, int64_t bytes,
> +                                       BdrvRequestFlags flags)
>  {
>      int ret = cbw_do_copy_before_write(bs, offset, bytes, 0);
>      if (ret < 0) {
>          return ret;
>      }
>  
> -    return bdrv_co_pdiscard(bs->file, offset, bytes);
> +    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
>  }
>  
>  static int coroutine_fn cbw_co_pwrite_zeroes(BlockDriverState *bs,
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 1fc7fb3333..52183cc9a2 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -201,9 +201,10 @@ static int coroutine_fn cor_co_pwrite_zeroes(BlockDriverState *bs,
>  
>  
>  static int coroutine_fn cor_co_pdiscard(BlockDriverState *bs,
> -                                        int64_t offset, int64_t bytes)
> +                                        int64_t offset, int64_t bytes,
> +                                       BdrvRequestFlags flags)
>  {
> -    return bdrv_co_pdiscard(bs->file, offset, bytes);
> +    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
>  }
>  
>  
> diff --git a/block/coroutines.h b/block/coroutines.h
> index c8c14a29c8..b0ba771bef 100644
> --- a/block/coroutines.h
> +++ b/block/coroutines.h
> @@ -98,9 +98,11 @@ int coroutine_fn
>  blk_co_do_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
>  
>  int generated_co_wrapper
> -blk_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes);
> +blk_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
> +                BdrvRequestFlags flags);
>  int coroutine_fn
> -blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes);
> +blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
> +                   BdrvRequestFlags flags);
>  
>  int generated_co_wrapper blk_do_flush(BlockBackend *blk);
>  int coroutine_fn blk_co_do_flush(BlockBackend *blk);
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 7a27c83060..caa406e429 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -160,6 +160,7 @@ typedef struct BDRVRawState {
>      bool is_xfs:1;
>  #endif
>      bool has_discard:1;
> +    bool has_secdiscard:1;
>      bool has_write_zeroes:1;
>      bool discard_zeroes:1;
>      bool use_linux_aio:1;

has_secdiscard is only set to false in raw_open_common() and never
changed or used.

> @@ -727,6 +728,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>  #endif /* !defined(CONFIG_LINUX_IO_URING) */
>  
>      s->has_discard = true;
> +    s->has_secdiscard = false;
>      s->has_write_zeroes = true;
>      if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
>          s->needs_alignment = true;
> @@ -765,6 +767,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>              s->discard_zeroes = true;
>          }
>  #endif
> +
>  #ifdef __linux__
>          /* On Linux 3.10, BLKDISCARD leaves stale data in the page cache.  Do
>           * not rely on the contents of discarded blocks unless using O_DIRECT.

Unrelated hunk.

> @@ -1859,6 +1862,35 @@ static int handle_aiocb_discard(void *opaque)
>      return ret;
>  }
>  
> +static int handle_aiocb_secdiscard(void *opaque)
> +{
> +    RawPosixAIOData *aiocb = opaque;
> +    int ret = -ENOTSUP;
> +    BlockDriverState *bs = aiocb->bs;
> +
> +    if (!(bs->open_flags & BDRV_O_SECDISCARD)) {
> +        return -ENOTSUP;
> +    }
> +
> +    if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
> +#ifdef BLKSECDISCARD
> +        do {
> +            uint64_t range[2] = { aiocb->aio_offset, aiocb->aio_nbytes };
> +            if (ioctl(aiocb->aio_fildes, BLKSECDISCARD, range) == 0) {
> +                return 0;
> +            }
> +        } while (errno == EINTR);
> +
> +        ret = translate_err(-errno);
> +#endif
> +    }
> +
> +    if (ret == -ENOTSUP) {
> +        bs->open_flags &= ~BDRV_O_SECDISCARD;

I'd rather avoid changing bs->open_flags. This is user input and I would
preserve it in its original state.

We already know when opening the image whether it is a block device. Why
do we even open the image instead of erroring out there?

> +    }
> +    return ret;
> +}
> +
>  /*
>   * Help alignment probing by allocating the first block.
>   *

Kevin

RE: [PATCH 1/2] block:hdev: support BLKSECDISCARD

[Qi, Yadong]

> 
> Notably absent: qapi/block-core.json. Without changing this, the option can't be
> available in -blockdev, which is the primary option to configure block device
> backends.
> 
> This patch seems to contain multiple logical changes that should be split into
> separate patches:
> 
> * Adding a flags parameter to .bdrv_co_pdiscard
> 
> * Support for the new feature in the core block layer (should be done
>   with -blockdev)
> 
> * Convenience magic for -drive (BDRV_O_SECDISCARD). It's not clear that
>   this should be done at all because the option is really specific to
>   one single block driver (file-posix). I think in your patch, all
>   other block drivers silently ignore the option, which is not what we
>   want.
Sorry for the absent for -blockdev. Will try add that.
Also I will try to split the patches.
And for the BDRV_O_SECDISCARD, it is specific for file-posix.c(host_device). Maybe
I need to add the option only for file-posix.c.

> 
> > diff --git a/block.c b/block.c
> > index 580cb77a70..4f05e96d12 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1128,6 +1128,32 @@ int bdrv_parse_discard_flags(const char *mode,
> int *flags)
> >      return 0;
> >  }
> >
> > +/**
> > + * Set open flags for a given secdiscard mode
> > + *
> > + * Return 0 on success, -1 if the secdiscard mode was invalid.
> > + */
> > +int bdrv_parse_secdiscard_flags(const char *mode, int *flags, Error
> > +**errp) {
> > +    *flags &= ~BDRV_O_SECDISCARD;
> > +
> > +    if (!strcmp(mode, "off")) {
> > +        /* do nothing */
> > +    } else if (!strcmp(mode, "on")) {
> > +        if (!(*flags & BDRV_O_UNMAP)) {
> > +            error_setg(errp, "cannot enable secdiscard when discard is "
> > +                             "disabled!");
> > +            return -1;
> > +        }
> 
> This check has nothing to do with parsing the option, it's validating its value.
> 
> You don't even need a new function to parse it, because there is already
> qemu_opt_get_bool(). Duplicating it means only that you're inconsistent with
> other boolean options, which alternatively accept "yes"/"no", "true"/"false",
> "y/n".
Sure. Will use qemu_opt_get_bool() instead.

> 
> > +
> > +        *flags |= BDRV_O_SECDISCARD;
> > +    } else {
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  /**
> >   * Set open flags for a given cache mode
> >   *
> > @@ -1695,6 +1721,11 @@ QemuOptsList bdrv_runtime_opts = {
> >              .type = QEMU_OPT_STRING,
> >              .help = "discard operation (ignore/off, unmap/on)",
> >          },
> > +        {
> > +            .name = BDRV_OPT_SECDISCARD,
> > +            .type = QEMU_OPT_STRING,
> > +            .help = "secure discard operation (off, on)",
> > +        },
> >          {
> >              .name = BDRV_OPT_FORCE_SHARE,
> >              .type = QEMU_OPT_BOOL,
> > @@ -1735,6 +1766,7 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockBackend *file,
> >      const char *driver_name = NULL;
> >      const char *node_name = NULL;
> >      const char *discard;
> > +    const char *secdiscard;
> >      QemuOpts *opts;
> >      BlockDriver *drv;
> >      Error *local_err = NULL;
> > @@ -1829,6 +1861,16 @@ static int bdrv_open_common(BlockDriverState
> *bs, BlockBackend *file,
> >          }
> >      }
> >
> > +
> > +    secdiscard = qemu_opt_get(opts, BDRV_OPT_SECDISCARD);
> > +    if (secdiscard != NULL) {
> > +        if (bdrv_parse_secdiscard_flags(secdiscard, &bs->open_flags,
> > +                                        errp) != 0) {
> > +            ret = -EINVAL;
> > +            goto fail_opts;
> > +        }
> > +    }
> > +
> >      bs->detect_zeroes =
> >          bdrv_parse_detect_zeroes(opts, bs->open_flags, &local_err);
> >      if (local_err) {
> > @@ -3685,6 +3727,10 @@ static BlockDriverState *bdrv_open_inherit(const
> char *filename,
> >                                 &flags, options, flags, options);
> >      }
> >
> > +    if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_SECDISCARD), "on")) {
> > +            flags |= BDRV_O_SECDISCARD;
> 
> Indentation is off.
Will fix it.

> 
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -160,6 +160,7 @@ typedef struct BDRVRawState {
> >      bool is_xfs:1;
> >  #endif
> >      bool has_discard:1;
> > +    bool has_secdiscard:1;
> >      bool has_write_zeroes:1;
> >      bool discard_zeroes:1;
> >      bool use_linux_aio:1;
> 
> has_secdiscard is only set to false in raw_open_common() and never changed or
> used.
Will remove it.

> 
> > @@ -727,6 +728,7 @@ static int raw_open_common(BlockDriverState *bs,
> > QDict *options,  #endif /* !defined(CONFIG_LINUX_IO_URING) */
> >
> >      s->has_discard = true;
> > +    s->has_secdiscard = false;
> >      s->has_write_zeroes = true;
> >      if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd))
> {
> >          s->needs_alignment = true;
> > @@ -765,6 +767,7 @@ static int raw_open_common(BlockDriverState *bs,
> QDict *options,
> >              s->discard_zeroes = true;
> >          }
> >  #endif
> > +
> >  #ifdef __linux__
> >          /* On Linux 3.10, BLKDISCARD leaves stale data in the page cache.  Do
> >           * not rely on the contents of discarded blocks unless using O_DIRECT.
> 
> Unrelated hunk.
Will fix it.

> 
> > @@ -1859,6 +1862,35 @@ static int handle_aiocb_discard(void *opaque)
> >      return ret;
> >  }
> >
> > +static int handle_aiocb_secdiscard(void *opaque) {
> > +    RawPosixAIOData *aiocb = opaque;
> > +    int ret = -ENOTSUP;
> > +    BlockDriverState *bs = aiocb->bs;
> > +
> > +    if (!(bs->open_flags & BDRV_O_SECDISCARD)) {
> > +        return -ENOTSUP;
> > +    }
> > +
> > +    if (aiocb->aio_type & QEMU_AIO_BLKDEV) { #ifdef BLKSECDISCARD
> > +        do {
> > +            uint64_t range[2] = { aiocb->aio_offset, aiocb->aio_nbytes };
> > +            if (ioctl(aiocb->aio_fildes, BLKSECDISCARD, range) == 0) {
> > +                return 0;
> > +            }
> > +        } while (errno == EINTR);
> > +
> > +        ret = translate_err(-errno);
> > +#endif
> > +    }
> > +
> > +    if (ret == -ENOTSUP) {
> > +        bs->open_flags &= ~BDRV_O_SECDISCARD;
> 
> I'd rather avoid changing bs->open_flags. This is user input and I would preserve
> it in its original state.
> 
> We already know when opening the image whether it is a block device. Why do
> we even open the image instead of erroring out there?
OK. I will try to find another way to record whether backend driver would support
SECDISCARD.

Best Regard
Yadong

Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD

[Stefan Hajnoczi]

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

On Mon, Nov 15, 2021 at 12:51:59PM +0800, yadong.qi@intel.com wrote:
> From: Yadong Qi <yadong.qi@intel.com>
> 
> Add a new option "secdiscard" for block drive. Parse it and
> record it in bs->open_flags as bit(BDRV_O_SECDISCARD).
> 
> Add a new BDRV_REQ_SECDISCARD bit for secure discard request
> from virtual device.
> 
> For host_device backend: implement by ioctl(BLKSECDISCARD) on
> real host block device.
> For other backend, no implementation.
> 
> E.g.:
>     qemu-system-x86_64 \
>     ... \
>     -drive file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \
>     -device virtio-blk-pci,drive=sd0,id=sd0_vblk \
>     ...

I'm curious why there is explicit control over this feature (-drive
secdiscard=on|off). For example, is there a reason why users would want
to disable secdiscard on a drive that supports it? I guess there is no
way to emulate it correctly so secdiscard=on on a drive that doesn't
support it produces an error?

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

RE: [PATCH 1/2] block:hdev: support BLKSECDISCARD

[Qi, Yadong]

> > Add a new option "secdiscard" for block drive. Parse it and record it
> > in bs->open_flags as bit(BDRV_O_SECDISCARD).
> >
> > Add a new BDRV_REQ_SECDISCARD bit for secure discard request from
> > virtual device.
> >
> > For host_device backend: implement by ioctl(BLKSECDISCARD) on real
> > host block device.
> > For other backend, no implementation.
> >
> > E.g.:
> >     qemu-system-x86_64 \
> >     ... \
> >     -drive
> file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \
> >     -device virtio-blk-pci,drive=sd0,id=sd0_vblk \
> >     ...
> 
> I'm curious why there is explicit control over this feature (-drive
> secdiscard=on|off). For example, is there a reason why users would want to
> disable secdiscard on a drive that supports it? I guess there is no way to emulate
> it correctly so secdiscard=on on a drive that doesn't support it produces an error?

Yes. AFAIK, there is no way to emulate a "secure" discard. But it seems also hard to
detect whether a host drive support secdiscard unless it really perform a real
secdiscard. So I choose to add an option for user to enable it for virtual block.
There is an assumption that user knows whether host support secdiscard.


Best Regard
Yadong

Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD

[Stefan Hajnoczi]

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

On Tue, Nov 16, 2021 at 02:03:15AM +0000, Qi, Yadong wrote:
> > > Add a new option "secdiscard" for block drive. Parse it and record it
> > > in bs->open_flags as bit(BDRV_O_SECDISCARD).
> > >
> > > Add a new BDRV_REQ_SECDISCARD bit for secure discard request from
> > > virtual device.
> > >
> > > For host_device backend: implement by ioctl(BLKSECDISCARD) on real
> > > host block device.
> > > For other backend, no implementation.
> > >
> > > E.g.:
> > >     qemu-system-x86_64 \
> > >     ... \
> > >     -drive
> > file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \
> > >     -device virtio-blk-pci,drive=sd0,id=sd0_vblk \
> > >     ...
> > 
> > I'm curious why there is explicit control over this feature (-drive
> > secdiscard=on|off). For example, is there a reason why users would want to
> > disable secdiscard on a drive that supports it? I guess there is no way to emulate
> > it correctly so secdiscard=on on a drive that doesn't support it produces an error?
> 
> Yes. AFAIK, there is no way to emulate a "secure" discard. But it seems also hard to
> detect whether a host drive support secdiscard unless it really perform a real
> secdiscard. So I choose to add an option for user to enable it for virtual block.
> There is an assumption that user knows whether host support secdiscard.

Question for Jens and Christoph:

Is there a way for userspace to detect whether a Linux block device
supports SECDISCARD?

If not, then maybe a new sysfs attribute can be added:

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index cef1f713370b..c946ef49ac15 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -304,6 +304,7 @@ QUEUE_SYSFS_BIT_FNS(nonrot, NONROT, 1);
 QUEUE_SYSFS_BIT_FNS(random, ADD_RANDOM, 0);
 QUEUE_SYSFS_BIT_FNS(iostats, IO_STAT, 0);
 QUEUE_SYSFS_BIT_FNS(stable_writes, STABLE_WRITES, 0);
+QUEUE_SYSFS_BIT_FNS(secure_erase, SECERASE, 0);
 #undef QUEUE_SYSFS_BIT_FNS

 static ssize_t queue_zoned_show(struct request_queue *q, char *page)

Stefan

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

Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD

[Christoph Hellwig]

On Tue, Nov 16, 2021 at 10:58:30AM +0000, Stefan Hajnoczi wrote:
> Question for Jens and Christoph:
> 
> Is there a way for userspace to detect whether a Linux block device
> supports SECDISCARD?

I don't know of one.

> If not, then maybe a new sysfs attribute can be added:

This looks correct, but if we start looking into SECDISCARD seriously
I'd like to split the max_sectors settings for it from discard as that
is currently a bit of a mess.  If we then expose the secure erase max
sectors in sysfs that would also be a good indicator.

What is the use case for exposing secure erase in qemu?  The whole
concept for a LBA based secure erase is generally not a very smart
idea for flash based media..

Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD

[Stefan Hajnoczi]

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

On Tue, Nov 16, 2021 at 09:53:39PM -0800, Christoph Hellwig wrote:
> On Tue, Nov 16, 2021 at 10:58:30AM +0000, Stefan Hajnoczi wrote:
> > Question for Jens and Christoph:
> > 
> > Is there a way for userspace to detect whether a Linux block device
> > supports SECDISCARD?
> 
> I don't know of one.
> 
> > If not, then maybe a new sysfs attribute can be added:
> 
> This looks correct, but if we start looking into SECDISCARD seriously
> I'd like to split the max_sectors settings for it from discard as that
> is currently a bit of a mess.  If we then expose the secure erase max
> sectors in sysfs that would also be a good indicator.

That is probably better because userspace would the queue limits
information too. Thanks!

> What is the use case for exposing secure erase in qemu?  The whole
> concept for a LBA based secure erase is generally not a very smart
> idea for flash based media..

Yadong, please see Christoph's question above.

Stefan

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

RE: [PATCH 1/2] block:hdev: support BLKSECDISCARD

[Qi, Yadong]

> What is the use case for exposing secure erase in qemu?  The whole concept for
> a LBA based secure erase is generally not a very smart idea for flash based
> media..
Hi, Christoph
We got a user requirement: support BLKSECDISCARD in VM. Which is:
ioctl(BLKSECDISCARD) in guest -> qemu backend perform secure discard
on native block device accordingly.
This is a requirement to meet the security level of the project.
So I sent out these patches to see whether qemu could accept such changes.

[PATCH 2/2] virtio-blk: support BLKSECDISCARD

[yadong.qi]

From: Yadong Qi <yadong.qi@intel.com>

Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
Add new virtio command: VIRTIO_BLK_T_SECDISCARD.

This feature is disabled by default, it will check the backend
bs->open_flags & BDRV_O_SECDISCARD, enable it if BDRV_O_SECDISCARD
is supported.

Signed-off-by: Yadong Qi <yadong.qi@intel.com>
---
 hw/block/virtio-blk.c                       | 26 +++++++++++++++++----
 include/standard-headers/linux/virtio_blk.h |  4 ++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index dbc4c5a3cd..7bc3484521 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -536,7 +536,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
 }
 
 static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
-    struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
+    struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes,
+    bool is_secdiscard)
 {
     VirtIOBlock *s = req->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -577,8 +578,8 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
         goto err;
     }
 
+    int blk_aio_flags = 0;
     if (is_write_zeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
-        int blk_aio_flags = 0;
 
         if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
             blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
@@ -600,7 +601,12 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
             goto err;
         }
 
-        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, 0,
+        if (is_secdiscard) {
+            blk_aio_flags |= BDRV_REQ_SECDISCARD;
+        }
+
+        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
+                         blk_aio_flags,
                          virtio_blk_discard_write_zeroes_complete, req);
     }
 
@@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     unsigned out_num = req->elem.out_num;
     VirtIOBlock *s = req->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    bool is_secdiscard = false;
 
     if (req->elem.out_num < 1 || req->elem.in_num < 1) {
         virtio_error(vdev, "virtio-blk missing headers");
@@ -722,6 +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
      * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
      * so we must mask it for these requests, then we will check if it is set.
      */
+    case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
+        is_secdiscard = true;
+        __attribute__((fallthrough));
     case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
     case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
     {
@@ -752,7 +762,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
         }
 
         err_status = virtio_blk_handle_discard_write_zeroes(req, &dwz_hdr,
-                                                            is_write_zeroes);
+                                                            is_write_zeroes,
+                                                            is_secdiscard);
         if (err_status != VIRTIO_BLK_S_OK) {
             virtio_blk_req_complete(req, err_status);
             virtio_blk_free_request(req);
@@ -1201,6 +1212,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (blk_get_flags(conf->conf.blk) & BDRV_O_SECDISCARD)
+        virtio_add_feature(&s->host_features, VIRTIO_BLK_F_SECDISCARD);
+    else
+        virtio_clear_feature(&s->host_features, VIRTIO_BLK_F_SECDISCARD);
+
     if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_WRITE_ZEROES) &&
         (!conf->max_write_zeroes_sectors ||
          conf->max_write_zeroes_sectors > BDRV_REQUEST_MAX_SECTORS)) {
@@ -1307,6 +1323,8 @@ static Property virtio_blk_properties[] = {
                      conf.report_discard_granularity, true),
     DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
                       VIRTIO_BLK_F_WRITE_ZEROES, true),
+    DEFINE_PROP_BIT64("secdiscard", VirtIOBlock, host_features,
+                      VIRTIO_BLK_F_SECDISCARD, false),
     DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock,
                        conf.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS),
     DEFINE_PROP_UINT32("max-write-zeroes-sectors", VirtIOBlock,
diff --git a/include/standard-headers/linux/virtio_blk.h b/include/standard-headers/linux/virtio_blk.h
index 2dcc90826a..c55a07840c 100644
--- a/include/standard-headers/linux/virtio_blk.h
+++ b/include/standard-headers/linux/virtio_blk.h
@@ -40,6 +40,7 @@
 #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
 #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
 #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
+#define VIRTIO_BLK_F_SECDISCARD	15	/* WRITE ZEROES is supported */
 
 /* Legacy feature bits */
 #ifndef VIRTIO_BLK_NO_LEGACY
@@ -153,6 +154,9 @@ struct virtio_blk_config {
 /* Write zeroes command */
 #define VIRTIO_BLK_T_WRITE_ZEROES	13
 
+/* Secure discard command */
+#define VIRTIO_BLK_T_SECDISCARD	14
+
 #ifndef VIRTIO_BLK_NO_LEGACY
 /* Barrier before this op. */
 #define VIRTIO_BLK_T_BARRIER	0x80000000
-- 
2.25.1

Re: [PATCH 2/2] virtio-blk: support BLKSECDISCARD

[Stefano Garzarella]

On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong.qi@intel.com wrote:
>From: Yadong Qi <yadong.qi@intel.com>
>
>Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
>Add new virtio command: VIRTIO_BLK_T_SECDISCARD.

Has a proposal been sent out yet to virtio-comment mailing list for 
discussing these specification changes?

>
>This feature is disabled by default, it will check the backend
>bs->open_flags & BDRV_O_SECDISCARD, enable it if BDRV_O_SECDISCARD
>is supported.
>
>Signed-off-by: Yadong Qi <yadong.qi@intel.com>
>---
> hw/block/virtio-blk.c                       | 26 +++++++++++++++++----
> include/standard-headers/linux/virtio_blk.h |  4 ++++
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
>diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>index dbc4c5a3cd..7bc3484521 100644
>--- a/hw/block/virtio-blk.c
>+++ b/hw/block/virtio-blk.c
>@@ -536,7 +536,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
> }
>
> static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
>-    struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
>+    struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes,
>+    bool is_secdiscard)

Since the function now handles 3 commands, I'm thinking if it's better 
to pass the command directly and then make a switch instead of using 2 
booleans.

> {
>     VirtIOBlock *s = req->dev;
>     VirtIODevice *vdev = VIRTIO_DEVICE(s);
>@@ -577,8 +578,8 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
>         goto err;
>     }
>
>+    int blk_aio_flags = 0;

Maybe better to move it to the beginning of the function.

>     if (is_write_zeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
>-        int blk_aio_flags = 0;
>
>         if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
>             blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
>@@ -600,7 +601,12 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
>             goto err;
>         }
>
>-        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, 0,
>+        if (is_secdiscard) {
>+            blk_aio_flags |= BDRV_REQ_SECDISCARD;
>+        }
>+
>+        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
>+                         blk_aio_flags,
>                          virtio_blk_discard_write_zeroes_complete, req);
>     }
>
>@@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>     unsigned out_num = req->elem.out_num;
>     VirtIOBlock *s = req->dev;
>     VirtIODevice *vdev = VIRTIO_DEVICE(s);
>+    bool is_secdiscard = false;
>
>     if (req->elem.out_num < 1 || req->elem.in_num < 1) {
>         virtio_error(vdev, "virtio-blk missing headers");
>@@ -722,6 +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>      * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
>      * so we must mask it for these requests, then we will check if it is set.
>      */
>+    case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
>+        is_secdiscard = true;
>+        __attribute__((fallthrough));

We can use QEMU_FALLTHROUGH here.

>     case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
>     case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
>     {
>@@ -752,7 +762,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq 
>*req, MultiReqBuffer *mrb)
>         }
>
>         err_status = virtio_blk_handle_discard_write_zeroes(req, &dwz_hdr,
>-                                                            is_write_zeroes);
>+                                                            is_write_zeroes,
>+                                                            is_secdiscard);
>         if (err_status != VIRTIO_BLK_S_OK) {
>             virtio_blk_req_complete(req, err_status);
>             virtio_blk_free_request(req);
>@@ -1201,6 +1212,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>         return;
>     }
>
>+    if (blk_get_flags(conf->conf.blk) & BDRV_O_SECDISCARD)
>+        virtio_add_feature(&s->host_features, 
>VIRTIO_BLK_F_SECDISCARD);
>+    else
>+        virtio_clear_feature(&s->host_features, VIRTIO_BLK_F_SECDISCARD);
>+

IIUC here we set or not the feature if BDRV_O_SECDISCARD is set.

Should we keep it disabled if "secdiscard" is false? (e.g. to avoid 
migration problems)

Otherwise what is the purpose of the "secdiscard" property?

Thanks,
Stefano

RE: [PATCH 2/2] virtio-blk: support BLKSECDISCARD

[Qi, Yadong]

> >Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
> >Add new virtio command: VIRTIO_BLK_T_SECDISCARD.
> 
> Has a proposal been sent out yet to virtio-comment mailing list for discussing
> these specification changes?
> 
Not yet. I will draft a proposal to virtio-comment if no big concern of this patch
From maintainer.
> >
> >diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index
> >dbc4c5a3cd..7bc3484521 100644
> >--- a/hw/block/virtio-blk.c
> >+++ b/hw/block/virtio-blk.c
> >@@ -536,7 +536,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock
> >*dev,  }
> >
> > static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
> >-    struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
> >+    struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes,
> >+    bool is_secdiscard)
> 
> Since the function now handles 3 commands, I'm thinking if it's better to pass
> the command directly and then make a switch instead of using 2 booleans.
> 
Make sense.

> > {
> >     VirtIOBlock *s = req->dev;
> >     VirtIODevice *vdev = VIRTIO_DEVICE(s); @@ -577,8 +578,8 @@ static
> >uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
> >         goto err;
> >     }
> >
> >+    int blk_aio_flags = 0;
> 
> Maybe better to move it to the beginning of the function.
Sure.

> 
> >
> >-        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, 0,
> >+        if (is_secdiscard) {
> >+            blk_aio_flags |= BDRV_REQ_SECDISCARD;
> >+        }
> >+
> >+        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
> >+                         blk_aio_flags,
> >                          virtio_blk_discard_write_zeroes_complete, req);
> >     }
> >
> >@@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq
> *req, MultiReqBuffer *mrb)
> >     unsigned out_num = req->elem.out_num;
> >     VirtIOBlock *s = req->dev;
> >     VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >+    bool is_secdiscard = false;
> >
> >     if (req->elem.out_num < 1 || req->elem.in_num < 1) {
> >         virtio_error(vdev, "virtio-blk missing headers"); @@ -722,6
> >+729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> >      * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
> >      * so we must mask it for these requests, then we will check if it is set.
> >      */
> >+    case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
> >+        is_secdiscard = true;
> >+        __attribute__((fallthrough));
> 
> We can use QEMU_FALLTHROUGH here.
Sure.

> 
> >
> >         err_status = virtio_blk_handle_discard_write_zeroes(req, &dwz_hdr,
> >-                                                            is_write_zeroes);
> >+                                                            is_write_zeroes,
> >+
> >+ is_secdiscard);


> >
> >+    if (blk_get_flags(conf->conf.blk) & BDRV_O_SECDISCARD)
> >+        virtio_add_feature(&s->host_features,
> >VIRTIO_BLK_F_SECDISCARD);
> >+    else
> >+        virtio_clear_feature(&s->host_features,
> >+ VIRTIO_BLK_F_SECDISCARD);
> >+
> 
> IIUC here we set or not the feature if BDRV_O_SECDISCARD is set.
> 
> Should we keep it disabled if "secdiscard" is false? (e.g. to avoid migration
> problems)
Yes, BDRV_O_SECDISCARD=(secdiscard=="on") ? 1 : 0;

> 
> Otherwise what is the purpose of the "secdiscard" property?
I cannot find a good method to detect whether host device support BLKSECDISCARD.
So I add this "secdiscard" property to explicitly enable this feature.

Best Regard
Yadong
> 
> Thanks,
> Stefano

Re: [PATCH 2/2] virtio-blk: support BLKSECDISCARD

[Michael S. Tsirkin]

On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong.qi@intel.com wrote:
> From: Yadong Qi <yadong.qi@intel.com>
> 
> Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
> Add new virtio command: VIRTIO_BLK_T_SECDISCARD.
> 
> This feature is disabled by default, it will check the backend
> bs->open_flags & BDRV_O_SECDISCARD, enable it if BDRV_O_SECDISCARD
> is supported.
> 
> Signed-off-by: Yadong Qi <yadong.qi@intel.com>
> ---
>  hw/block/virtio-blk.c                       | 26 +++++++++++++++++----
>  include/standard-headers/linux/virtio_blk.h |  4 ++++


Any changes to standard headers need to go to linux and virtio TC.

>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index dbc4c5a3cd..7bc3484521 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -536,7 +536,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
>  }
>  
>  static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
> -    struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
> +    struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes,
> +    bool is_secdiscard)
>  {
>      VirtIOBlock *s = req->dev;
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> @@ -577,8 +578,8 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
>          goto err;
>      }
>  
> +    int blk_aio_flags = 0;
>      if (is_write_zeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
> -        int blk_aio_flags = 0;
>  
>          if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
>              blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
> @@ -600,7 +601,12 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
>              goto err;
>          }
>  
> -        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, 0,
> +        if (is_secdiscard) {
> +            blk_aio_flags |= BDRV_REQ_SECDISCARD;
> +        }
> +
> +        blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
> +                         blk_aio_flags,
>                           virtio_blk_discard_write_zeroes_complete, req);
>      }
>  
> @@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>      unsigned out_num = req->elem.out_num;
>      VirtIOBlock *s = req->dev;
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +    bool is_secdiscard = false;
>  
>      if (req->elem.out_num < 1 || req->elem.in_num < 1) {
>          virtio_error(vdev, "virtio-blk missing headers");
> @@ -722,6 +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>       * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
>       * so we must mask it for these requests, then we will check if it is set.
>       */
> +    case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
> +        is_secdiscard = true;
> +        __attribute__((fallthrough));
>      case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
>      case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
>      {
> @@ -752,7 +762,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>          }
>  
>          err_status = virtio_blk_handle_discard_write_zeroes(req, &dwz_hdr,
> -                                                            is_write_zeroes);
> +                                                            is_write_zeroes,
> +                                                            is_secdiscard);
>          if (err_status != VIRTIO_BLK_S_OK) {
>              virtio_blk_req_complete(req, err_status);
>              virtio_blk_free_request(req);
> @@ -1201,6 +1212,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (blk_get_flags(conf->conf.blk) & BDRV_O_SECDISCARD)
> +        virtio_add_feature(&s->host_features, VIRTIO_BLK_F_SECDISCARD);
> +    else
> +        virtio_clear_feature(&s->host_features, VIRTIO_BLK_F_SECDISCARD);
> +
>      if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_WRITE_ZEROES) &&
>          (!conf->max_write_zeroes_sectors ||
>           conf->max_write_zeroes_sectors > BDRV_REQUEST_MAX_SECTORS)) {
> @@ -1307,6 +1323,8 @@ static Property virtio_blk_properties[] = {
>                       conf.report_discard_granularity, true),
>      DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
>                        VIRTIO_BLK_F_WRITE_ZEROES, true),
> +    DEFINE_PROP_BIT64("secdiscard", VirtIOBlock, host_features,
> +                      VIRTIO_BLK_F_SECDISCARD, false),
>      DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock,
>                         conf.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS),
>      DEFINE_PROP_UINT32("max-write-zeroes-sectors", VirtIOBlock,
> diff --git a/include/standard-headers/linux/virtio_blk.h b/include/standard-headers/linux/virtio_blk.h
> index 2dcc90826a..c55a07840c 100644
> --- a/include/standard-headers/linux/virtio_blk.h
> +++ b/include/standard-headers/linux/virtio_blk.h
> @@ -40,6 +40,7 @@
>  #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
>  #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
>  #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
> +#define VIRTIO_BLK_F_SECDISCARD	15	/* WRITE ZEROES is supported */

Surely not.


>  
>  /* Legacy feature bits */
>  #ifndef VIRTIO_BLK_NO_LEGACY
> @@ -153,6 +154,9 @@ struct virtio_blk_config {
>  /* Write zeroes command */
>  #define VIRTIO_BLK_T_WRITE_ZEROES	13
>  
> +/* Secure discard command */
> +#define VIRTIO_BLK_T_SECDISCARD	14
> +
>  #ifndef VIRTIO_BLK_NO_LEGACY
>  /* Barrier before this op. */
>  #define VIRTIO_BLK_T_BARRIER	0x80000000
> -- 
> 2.25.1

RE: [PATCH 2/2] virtio-blk: support BLKSECDISCARD

[Qi, Yadong]

> On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong.qi@intel.com wrote:
> > From: Yadong Qi <yadong.qi@intel.com>
> >
> > Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
> > Add new virtio command: VIRTIO_BLK_T_SECDISCARD.
> >
> > This feature is disabled by default, it will check the backend
> > bs->open_flags & BDRV_O_SECDISCARD, enable it if BDRV_O_SECDISCARD
> > is supported.
> >
> > Signed-off-by: Yadong Qi <yadong.qi@intel.com>
> > ---
> >  hw/block/virtio-blk.c                       | 26 +++++++++++++++++----
> >  include/standard-headers/linux/virtio_blk.h |  4 ++++
> 
> 
> Any changes to standard headers need to go to linux and virtio TC.
Sure. I will draft proposal to virtio-comment for review.

Re: [PATCH 2/2] virtio-blk: support BLKSECDISCARD

[Stefan Hajnoczi]

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

On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong.qi@intel.com wrote:

The Linux block layer shares the DISCARD queue limits with SECDISCARD.
That's different from BLKZEROOUT (QEMU's WRITE_ZEROES). This is a Linux
implementation detail but I guess virtio-blk can share the DISCARD
limits with SECDISCARD too...

> @@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>      unsigned out_num = req->elem.out_num;
>      VirtIOBlock *s = req->dev;
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +    bool is_secdiscard = false;
>  
>      if (req->elem.out_num < 1 || req->elem.in_num < 1) {
>          virtio_error(vdev, "virtio-blk missing headers");
> @@ -722,6 +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>       * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
>       * so we must mask it for these requests, then we will check if it is set.
>       */
> +    case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
> +        is_secdiscard = true;
> +        __attribute__((fallthrough));

The DISCARD case doesn't use __attribute__((fallthrough)) so this is
inconsistent. QEMU doesn't use __attribute__((fallthrough)) so I suggest
dropping this.

> diff --git a/include/standard-headers/linux/virtio_blk.h b/include/standard-headers/linux/virtio_blk.h
> index 2dcc90826a..c55a07840c 100644
> --- a/include/standard-headers/linux/virtio_blk.h
> +++ b/include/standard-headers/linux/virtio_blk.h
> @@ -40,6 +40,7 @@
>  #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
>  #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
>  #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
> +#define VIRTIO_BLK_F_SECDISCARD	15	/* WRITE ZEROES is supported */

The comment is copy-pasted from WRITE_ZEROES.

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

RE: [PATCH 2/2] virtio-blk: support BLKSECDISCARD

[Qi, Yadong]

> The Linux block layer shares the DISCARD queue limits with SECDISCARD.
> That's different from BLKZEROOUT (QEMU's WRITE_ZEROES). This is a Linux
> implementation detail but I guess virtio-blk can share the DISCARD limits with
> SECDISCARD too...
> 
> > @@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq
> *req, MultiReqBuffer *mrb)
> >      unsigned out_num = req->elem.out_num;
> >      VirtIOBlock *s = req->dev;
> >      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > +    bool is_secdiscard = false;
> >
> >      if (req->elem.out_num < 1 || req->elem.in_num < 1) {
> >          virtio_error(vdev, "virtio-blk missing headers"); @@ -722,6
> > +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> >       * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
> >       * so we must mask it for these requests, then we will check if it is set.
> >       */
> > +    case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
> > +        is_secdiscard = true;
> > +        __attribute__((fallthrough));
> 
> The DISCARD case doesn't use __attribute__((fallthrough)) so this is inconsistent.
> QEMU doesn't use __attribute__((fallthrough)) so I suggest dropping this.
Sure, will try to drop the fallthrough case.

> 
> > diff --git a/include/standard-headers/linux/virtio_blk.h
> > b/include/standard-headers/linux/virtio_blk.h
> > index 2dcc90826a..c55a07840c 100644
> > --- a/include/standard-headers/linux/virtio_blk.h
> > +++ b/include/standard-headers/linux/virtio_blk.h
> > @@ -40,6 +40,7 @@
> >  #define VIRTIO_BLK_F_MQ		12	/* support more than one vq
> */
> >  #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
> >  #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is
> supported */
> > +#define VIRTIO_BLK_F_SECDISCARD	15	/* WRITE ZEROES is supported
> */
> 
> The comment is copy-pasted from WRITE_ZEROES.
Will fix it.