Re: does drive_get_next(IF_NONE) make sense?
On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: > > Thomas Huth writes: > > > On 03/11/2021 09.41, Markus Armbruster wrote: > >> Peter Maydell writes: > >> > >>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ? > >> Short answer: hell, no! ;) > > > > Would it make sense to add an "assert(type != IF_NONE)" to drive_get() > > to avoid such mistakes in the future? > > Worth a try. You need to fix the sifive_u_otp device first :-) -- PMM
Re: does drive_get_next(IF_NONE) make sense?
Peter Maydell writes: > On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: >> >> Thomas Huth writes: >> >> > On 03/11/2021 09.41, Markus Armbruster wrote: >> >> Peter Maydell writes: >> >> >> >>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ? >> >> Short answer: hell, no! ;) >> > >> > Would it make sense to add an "assert(type != IF_NONE)" to drive_get() >> > to avoid such mistakes in the future? >> >> Worth a try. > > You need to fix the sifive_u_otp device first :-) And for that, we may want Hao Wu's "[PATCH v4 5/7] blockdev: Add a new IF type IF_OTHER" first.
Re: does drive_get_next(IF_NONE) make sense?
On Mon, Nov 15, 2021 at 3:32 PM Markus Armbruster wrote: > > Peter Maydell writes: > > > On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: > >> > >> Thomas Huth writes: > >> > >> > On 03/11/2021 09.41, Markus Armbruster wrote: > >> >> Peter Maydell writes: > >> >> > >> >>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ? > >> >> Short answer: hell, no! ;) > >> > > >> > Would it make sense to add an "assert(type != IF_NONE)" to drive_get() > >> > to avoid such mistakes in the future? > >> > >> Worth a try. > > > > You need to fix the sifive_u_otp device first :-) > > And for that, we may want Hao Wu's "[PATCH v4 5/7] blockdev: Add a new > IF type IF_OTHER" first. I can fixup sifive_u_otp, just let me know what the prefered solution is Alistair
[PATCH 0/2] support BLKSECDISCARD
From: Yadong Qi 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
From: Yadong Qi
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
---
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;
@
[PATCH 2/2] virtio-blk: support BLKSECDISCARD
From: Yadong Qi
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
---
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_MQ12 /* 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_SECDISCARD15 /* WRITE ZEROES is supported *
