[Qemu-devel] [PATCH v5 0/4] virtio/block: handle zoned backing devices

[Qemu-devel] [PATCH v5 0/4] virtio/block: handle zoned backing devices

[Dmitry Fomichev]

Ping... Any objections to merging this patchset? Ask me if you are not
sure how to validate these patches without having the hardware :)


Currently, attaching zoned block devices (i.e., storage devices
compliant to ZAC/ZBC standards) using several virtio methods doesn't
work properly as zoned devices appear as regular block devices at the
guest. This may cause unexpected i/o errors and, potentially, some
data corruption.

To be more precise, attaching a zoned device via virtio-pci-blk,
virtio-scsi-pci/scsi-disk or virtio-scsi-pci/scsi-hd demonstrates the
above behavior. The virtio-scsi-pci/scsi-block method works with a
recent patch. The virtio-scsi-pci/scsi-generic method also appears to
handle zoned devices without problems.

This patch set adds code to check if the backing device that is being
opened is a zoned Host Managed device. If this is the case, the patch
prohibits attaching such device for all use cases lacking proper
zoned support.

Host Aware zoned block devices are designed to work as regular block
devices at a guest system that does not support ZBD. Therefore, this
patch set doesn't prohibit attachment of Host Aware devices.

Considering that there is still a couple of different working ways
to attach a ZBD, this patch set provides a reasonable short-term
solution for this problem.

ZBD support for virtio-scsi-pci/scsi-disk and virtio-scsi-pci/scsi-hd
does not seem as necessary. Users will be expected to attach zoned
block devices via virtio-scsi-pci/scsi-block instead.

This patch set contains some Linux-specific code. This code is
necessary to obtain Zoned Block Device model value from Linux sysfs.

History:

v1 -> v2:
- rework code to be permission-based
- always allow Host Aware devices to be attached
- add fix for Host Aware attachments aka RCAP output snoop

v2 -> v3:
- drop the patch for RCAP output snoop - merged separately

v3 -> v4:
- rebase to the current code

v4 -> v5:
- avoid checkpatch warning

Dmitry Fomichev (4):
  block: Add zoned device model property
  raw: Recognize zoned backing devices
  block/ide/scsi: Set BLK_PERM_SUPPORT_ZONED
  raw: Don't open ZBDs if backend can't handle them

 block.c                   | 19 +++++++++
 block/file-posix.c        | 88 +++++++++++++++++++++++++++++++++------
 block/raw-format.c        |  8 ++++
 hw/block/block.c          |  8 +++-
 hw/block/fdc.c            |  5 ++-
 hw/block/nvme.c           |  2 +-
 hw/block/virtio-blk.c     |  2 +-
 hw/block/xen-block.c      |  2 +-
 hw/ide/qdev.c             |  2 +-
 hw/scsi/scsi-disk.c       | 13 +++---
 hw/scsi/scsi-generic.c    |  2 +-
 hw/usb/dev-storage.c      |  2 +-
 include/block/block.h     | 21 +++++++++-
 include/block/block_int.h |  4 ++
 include/hw/block/block.h  |  3 +-
 15 files changed, 151 insertions(+), 30 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v5 1/4] block: Add zoned device model property

[Dmitry Fomichev]

This commit adds Zoned Device Model (as defined in T10 ZBC and
T13 ZAC standards) as a block driver property, along with some
useful access functions.

A new backend driver permission, BLK_PERM_SUPPORT_ZONED, is also
introduced. Only the drivers having this permission will be allowed
to open zoned block devices.

No code is added yet to initialize or check the value of this new
property, therefore this commit doesn't change any functionality.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 block.c                   | 19 +++++++++++++++++++
 include/block/block.h     | 21 ++++++++++++++++++++-
 include/block/block_int.h |  4 ++++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 874a29a983..6dd4cecded 100644
--- a/block.c
+++ b/block.c
@@ -4679,6 +4679,25 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
     *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
 }
 
+uint8_t bdrv_get_zoned_model(BlockDriverState *bs)
+{
+    if (bs->drv->bdrv_get_zoned_info) {
+        bs->drv->bdrv_get_zoned_info(bs);
+    }
+
+    return bs->bl.zoned_model;
+}
+
+uint8_t bdrv_is_zoned(BlockDriverState *bs)
+{
+    /*
+     * Host Aware zone devices are supposed to be able to work
+     * just like regular block devices. Thus, we only consider
+     * Host Managed devices to be zoned here.
+     */
+    return bdrv_get_zoned_model(bs) == BLK_ZONED_MODEL_HM;
+}
+
 bool bdrv_is_sg(BlockDriverState *bs)
 {
     return bs->sg;
diff --git a/include/block/block.h b/include/block/block.h
index 124ad40809..238c0f5ed7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -271,18 +271,35 @@ enum {
      */
     BLK_PERM_GRAPH_MOD          = 0x10,
 
+    /** This permission is required to open zoned block devices. */
+    BLK_PERM_SUPPORT_ZONED      = 0x20,
+
     BLK_PERM_ALL                = 0x1f,
 
     DEFAULT_PERM_PASSTHROUGH    = BLK_PERM_CONSISTENT_READ
                                  | BLK_PERM_WRITE
                                  | BLK_PERM_WRITE_UNCHANGED
-                                 | BLK_PERM_RESIZE,
+                                 | BLK_PERM_RESIZE
+                                 | BLK_PERM_SUPPORT_ZONED,
 
     DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
 };
 
 char *bdrv_perm_names(uint64_t perm);
 
+/*
+ * Known zoned device models.
+ *
+ * TODO For a Linux host, it could be preferrable to include
+ * /usr/include/linux/blkzoned.h instead of defining ZBD-specific
+ * values here.
+ */
+enum blk_zoned_model {
+    BLK_ZONED_MODEL_NONE, /* Regular block device */
+    BLK_ZONED_MODEL_HA,   /* Host-aware zoned block device */
+    BLK_ZONED_MODEL_HM,   /* Host-managed zoned block device */
+};
+
 /* disk I/O throttling */
 void bdrv_init(void);
 void bdrv_init_with_whitelist(void);
@@ -359,6 +376,8 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
 BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
                                BlockDriverState *in_bs, Error **errp);
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
+uint8_t bdrv_get_zoned_model(BlockDriverState *bs);
+uint8_t bdrv_is_zoned(BlockDriverState *bs);
 void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
 int bdrv_commit(BlockDriverState *bs);
 int bdrv_change_backing_file(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ceec8c2f56..91496e8149 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -415,6 +415,7 @@ struct BlockDriver {
     bool (*bdrv_debug_is_suspended)(BlockDriverState *bs, const char *tag);
 
     void (*bdrv_refresh_limits)(BlockDriverState *bs, Error **errp);
+    void (*bdrv_get_zoned_info)(BlockDriverState *bs);
 
     /*
      * Returns 1 if newly created images are guaranteed to contain only
@@ -620,6 +621,9 @@ typedef struct BlockLimits {
 
     /* maximum number of iovec elements */
     int max_iov;
+
+    /* Zoned device model. Zero value indicates a regular block device */
+    uint8_t zoned_model;
 } BlockLimits;
 
 typedef struct BdrvOpBlocker BdrvOpBlocker;
-- 
2.21.0

[Qemu-devel] [PATCH v5 2/4] raw: Recognize zoned backing devices

[Dmitry Fomichev]

The purpose of this patch is to recognize a zoned block device (ZBD)
when it is opened as a raw file. The new code initializes the zoned
model propery introduced by the previous commit.

This commit is Linux-specific as it gets the Zoned Block Device Model
value (none/host-managed/host-aware) from sysfs on the host.

In order to avoid code duplication in file-posix.c, a common helper
function is added to read values of sysfs entries under
/sys/block/<dev>/queue. This way, the existing function that reads
the value of "max_segments" entry and the the new function that reads
"zoned" value both share the same helper code.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 block/file-posix.c | 74 ++++++++++++++++++++++++++++++++++++++--------
 block/raw-format.c |  8 +++++
 2 files changed, 70 insertions(+), 12 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index fbeb0068db..d9f2fc5e46 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1067,15 +1067,13 @@ static int sg_get_max_transfer_length(int fd)
 #endif
 }
 
-static int sg_get_max_segments(int fd)
+static int hdev_read_blk_queue_entry(int fd, const char *key,
+    char *buf, int buf_len)
 {
 #ifdef CONFIG_LINUX
-    char buf[32];
-    const char *end;
     char *sysfspath = NULL;
     int ret;
     int sysfd = -1;
-    long max_segments;
     struct stat st;
 
     if (fstat(fd, &st)) {
@@ -1083,23 +1081,45 @@ static int sg_get_max_segments(int fd)
         goto out;
     }
 
-    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
-                                major(st.st_rdev), minor(st.st_rdev));
+    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
+                                major(st.st_rdev), minor(st.st_rdev), key);
     sysfd = open(sysfspath, O_RDONLY);
     if (sysfd == -1) {
         ret = -errno;
         goto out;
     }
     do {
-        ret = read(sysfd, buf, sizeof(buf) - 1);
+        ret = read(sysfd, buf, buf_len - 1);
     } while (ret == -1 && errno == EINTR);
     if (ret < 0) {
         ret = -errno;
-        goto out;
     } else if (ret == 0) {
         ret = -EIO;
+    }
+out:
+    if (sysfd != -1) {
+        close(sysfd);
+    }
+    g_free(sysfspath);
+    return ret;
+#else
+    return -ENOTSUP;
+#endif
+}
+
+static int sg_get_max_segments(int fd)
+{
+#ifdef CONFIG_LINUX
+    char buf[32];
+    const char *end;
+    int ret;
+    long max_segments;
+
+    ret = hdev_read_blk_queue_entry(fd, "max_segments", buf, sizeof(buf));
+    if (ret < 0) {
         goto out;
     }
+
     buf[ret] = 0;
     /* The file is ended with '\n', pass 'end' to accept that. */
     ret = qemu_strtol(buf, &end, 10, &max_segments);
@@ -1108,10 +1128,33 @@ static int sg_get_max_segments(int fd)
     }
 
 out:
-    if (sysfd != -1) {
-        close(sysfd);
+    return ret;
+#else
+    return -ENOTSUP;
+#endif
+}
+
+static int hdev_get_zoned_model(int fd)
+{
+#ifdef CONFIG_LINUX
+    char buf[32];
+    int ret;
+
+    ret = hdev_read_blk_queue_entry(fd, "zoned", buf, sizeof(buf));
+    if (ret < 0) {
+        ret = BLK_ZONED_MODEL_NONE;
+        goto out;
     }
-    g_free(sysfspath);
+
+    buf[ret - 1] = 0;
+    ret = BLK_ZONED_MODEL_NONE;
+    if (strcmp(buf, "host-managed") == 0) {
+        ret = BLK_ZONED_MODEL_HM;
+    } else if (strcmp(buf, "host-aware") == 0) {
+        ret = BLK_ZONED_MODEL_HA;
+    }
+
+out:
     return ret;
 #else
     return -ENOTSUP;
@@ -1121,9 +1164,10 @@ out:
 static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
+    int ret;
 
     if (bs->sg) {
-        int ret = sg_get_max_transfer_length(s->fd);
+        ret = sg_get_max_transfer_length(s->fd);
 
         if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
             bs->bl.max_transfer = pow2floor(ret);
@@ -1133,6 +1177,12 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
         if (ret > 0) {
             bs->bl.max_transfer = MIN(bs->bl.max_transfer, ret * getpagesize());
         }
+
+    }
+
+    ret = hdev_get_zoned_model(s->fd);
+    if (ret >= 0) {
+        bs->bl.zoned_model = ret;
     }
 
     raw_probe_alignment(bs, s->fd, errp);
diff --git a/block/raw-format.c b/block/raw-format.c
index 42c28cc29a..a606e4a7fe 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -369,6 +369,13 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 }
 
+static void raw_get_zoned_info(BlockDriverState *bs)
+{
+    if (!bs->probed) {
+        bs->bl.zoned_model = bs->file->bs->bl.zoned_model;
+    }
+}
+
 static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
                                         PreallocMode prealloc, Error **errp)
 {
@@ -578,6 +585,7 @@ BlockDriver bdrv_raw = {
     .create_opts          = &raw_create_opts,
     .bdrv_has_zero_init   = &raw_has_zero_init,
     .bdrv_has_zero_init_truncate = &raw_has_zero_init_truncate,
+    .bdrv_get_zoned_info  = &raw_get_zoned_info,
     .strong_runtime_opts  = raw_strong_runtime_opts,
     .mutable_opts         = mutable_opts,
 };
-- 
2.21.0

[Qemu-devel] [PATCH v5 3/4] block/ide/scsi: Set BLK_PERM_SUPPORT_ZONED

[Dmitry Fomichev]

Added a new boolean argument to blkconf_apply_backend_options()
to let the common block code know whether the chosen block
backend can handle zoned block devices or not.

blkconf_apply_backend_options() then sets BLK_PERM_SUPPORT_ZONED
permission accordingly. The raw code can then use this permission
to allow or deny opening a zone device by a particular block driver.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
Acked-by: Paul Durrant <paul.durrant@citrix.com>
---
 hw/block/block.c         |  8 ++++++--
 hw/block/fdc.c           |  5 +++--
 hw/block/nvme.c          |  2 +-
 hw/block/virtio-blk.c    |  2 +-
 hw/block/xen-block.c     |  2 +-
 hw/ide/qdev.c            |  2 +-
 hw/scsi/scsi-disk.c      | 13 +++++++------
 hw/scsi/scsi-generic.c   |  2 +-
 hw/usb/dev-storage.c     |  2 +-
 include/hw/block/block.h |  3 ++-
 10 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index bf56c7612b..23fbe4d567 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -86,7 +86,8 @@ void blkconf_blocksizes(BlockConf *conf)
 }
 
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
-                                   bool resizable, Error **errp)
+                                   bool resizable, bool zoned_support,
+                                   Error **errp)
 {
     BlockBackend *blk = conf->blk;
     BlockdevOnError rerror, werror;
@@ -98,9 +99,12 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
     if (!readonly) {
         perm |= BLK_PERM_WRITE;
     }
+    if (zoned_support) {
+        perm |= BLK_PERM_SUPPORT_ZONED;
+    }
 
     shared_perm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
-                  BLK_PERM_GRAPH_MOD;
+                  BLK_PERM_GRAPH_MOD | BLK_PERM_SUPPORT_ZONED;
     if (resizable) {
         shared_perm |= BLK_PERM_RESIZE;
     }
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index ac5d31e8c1..c5f41b3eb6 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -477,7 +477,7 @@ static void fd_change_cb(void *opaque, bool load, Error **errp)
     } else {
         if (!blkconf_apply_backend_options(drive->conf,
                                            blk_is_read_only(drive->blk), false,
-                                           errp)) {
+                                           false, errp)) {
             return;
         }
     }
@@ -569,7 +569,8 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
     dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO;
     dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
 
-    if (!blkconf_apply_backend_options(&dev->conf, read_only, false, errp)) {
+    if (!blkconf_apply_backend_options(&dev->conf, read_only, false, false,
+    				       errp)) {
         return;
     }
 
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 12d8254250..07f08d0768 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1334,7 +1334,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
     }
     blkconf_blocksizes(&n->conf);
     if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
-                                       false, errp)) {
+                                       false, false, errp)) {
         return;
     }
 
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 18851601cb..8be62903e2 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1127,7 +1127,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 
     if (!blkconf_apply_backend_options(&conf->conf,
                                        blk_is_read_only(conf->conf.blk), true,
-                                       errp)) {
+                                       false, errp)) {
         return;
     }
     s->original_wce = blk_enable_write_cache(conf->conf.blk);
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index f77343db60..57fe970908 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -229,7 +229,7 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
     }
 
     if (!blkconf_apply_backend_options(conf, blockdev->info & VDISK_READONLY,
-                                       true, errp)) {
+                                       true, false, errp)) {
         return;
     }
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6fba6b62b8..a57a8f1a8f 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -200,7 +200,7 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
         }
     }
     if (!blkconf_apply_backend_options(&dev->conf, kind == IDE_CD,
-                                       kind != IDE_CD, errp)) {
+                                       kind != IDE_CD, false, errp)) {
         return;
     }
 
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 915641a0f1..8a57caafd7 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2318,7 +2318,7 @@ static void scsi_disk_unit_attention_reported(SCSIDevice *dev)
     }
 }
 
-static void scsi_realize(SCSIDevice *dev, Error **errp)
+static void scsi_realize(SCSIDevice *dev, bool zoned_support, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     bool read_only;
@@ -2362,7 +2362,8 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
     }
 
     if (!blkconf_apply_backend_options(&dev->conf, read_only,
-                                       dev->type == TYPE_DISK, errp)) {
+                                       dev->type == TYPE_DISK, zoned_support,
+                                       errp)) {
         return;
     }
 
@@ -2421,7 +2422,7 @@ static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
     if (!s->product) {
         s->product = g_strdup("QEMU HARDDISK");
     }
-    scsi_realize(&s->qdev, errp);
+    scsi_realize(&s->qdev, false, errp);
     if (ctx) {
         aio_context_release(ctx);
     }
@@ -2449,7 +2450,7 @@ static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
     if (!s->product) {
         s->product = g_strdup("QEMU CD-ROM");
     }
-    scsi_realize(&s->qdev, errp);
+    scsi_realize(&s->qdev, false, errp);
     aio_context_release(ctx);
 }
 
@@ -2459,7 +2460,7 @@ static void scsi_disk_realize(SCSIDevice *dev, Error **errp)
     Error *local_err = NULL;
 
     if (!dev->conf.blk) {
-        scsi_realize(dev, &local_err);
+        scsi_realize(dev, false, &local_err);
         assert(local_err);
         error_propagate(errp, local_err);
         return;
@@ -2652,7 +2653,7 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
      */
     s->features |= (1 << SCSI_DISK_F_NO_REMOVABLE_DEVOPS);
 
-    scsi_realize(&s->qdev, errp);
+    scsi_realize(&s->qdev, true, errp);
     scsi_generic_read_device_inquiry(&s->qdev);
 
 out:
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index e7798ebcd0..ccce710497 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -692,7 +692,7 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
     }
     if (!blkconf_apply_backend_options(&s->conf,
                                        blk_is_read_only(s->conf.blk),
-                                       true, errp)) {
+                                       true, true, errp)) {
         return;
     }
 
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 8545193488..c75c0dd6a5 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -603,7 +603,7 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
 
     blkconf_blocksizes(&s->conf);
     if (!blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true,
-                                       errp)) {
+                                       false, errp)) {
         return;
     }
 
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 607539057a..f988edc87e 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -85,7 +85,8 @@ bool blkconf_geometry(BlockConf *conf, int *trans,
                       Error **errp);
 void blkconf_blocksizes(BlockConf *conf);
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
-                                   bool resizable, Error **errp);
+                                   bool resizable, bool zoned_support,
+                                   Error **errp);
 
 /* Hard disk geometry */
 
-- 
2.21.0

[Qemu-devel] [PATCH v5 4/4] raw: Don't open ZBDs if backend can't handle them

[Dmitry Fomichev]

Abort opening a zoned device as a raw file in case the chosen
block backend driver lacks proper support for this type of
storage.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 block/file-posix.c | 14 ++++++++++++++
 hw/block/fdc.c     |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index d9f2fc5e46..090e7c4d2f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2884,6 +2884,20 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
             goto fail;
         }
     }
+
+    /*
+     * If we are opening a zoned block device, check if the backend
+     * driver can properly handle such devices, abort if not.
+     */
+    if (bdrv_is_zoned(bs) &&
+        (shared & BLK_PERM_SUPPORT_ZONED) &&
+        !(perm & BLK_PERM_SUPPORT_ZONED)) {
+        error_setg(errp,
+                   "block backend driver doesn't support HM zoned devices");
+        ret = -ENOTSUP;
+        goto fail;
+    }
+
     return 0;
 
 fail:
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index c5f41b3eb6..673a8b39bc 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -570,7 +570,7 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
     dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
 
     if (!blkconf_apply_backend_options(&dev->conf, read_only, false, false,
-    				       errp)) {
+                                       errp)) {
         return;
     }
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH v5 0/4] virtio/block: handle zoned backing devices

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190823194927.23278-1-dmitry.fomichev@wdc.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v5 0/4] virtio/block: handle zoned backing devices
Message-id: 20190823194927.23278-1-dmitry.fomichev@wdc.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20190823194927.23278-1-dmitry.fomichev@wdc.com -> patchew/20190823194927.23278-1-dmitry.fomichev@wdc.com
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp'
Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out '7bfe584e321946771692711ff83ad2b5850daca7'
Cloning into 'roms/edk2'...
Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54'
Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl'
Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687'
Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl'
Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography'
Cloning into 'boringssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
Cloning into 'krb5'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168'
Cloning into 'pyca-cryptography'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/opensbi'...
Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'slirp'...
Submodule path 'slirp': checked out '126c04acbabd7ad32c2b018fe10dfac2a3bc1210'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
2da8bec raw: Don't open ZBDs if backend can't handle them
f6ad9c9 block/ide/scsi: Set BLK_PERM_SUPPORT_ZONED
ec5b5e3 raw: Recognize zoned backing devices
375d837 block: Add zoned device model property

=== OUTPUT BEGIN ===
1/4 Checking commit 375d83775dbe (block: Add zoned device model property)
2/4 Checking commit ec5b5e3a99c6 (raw: Recognize zoned backing devices)
3/4 Checking commit f6ad9c97b8a0 (block/ide/scsi: Set BLK_PERM_SUPPORT_ZONED)
ERROR: code indent should never use tabs
#66: FILE: hw/block/fdc.c:573:
+    ^I^I^I^I       errp)) {$

total: 1 errors, 0 warnings, 145 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/4 Checking commit 2da8becb737f (raw: Don't open ZBDs if backend can't handle them)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190823194927.23278-1-dmitry.fomichev@wdc.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [Qemu-block] [PATCH v5 1/4] block: Add zoned device model property

[Stefan Hajnoczi]

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

On Fri, Aug 23, 2019 at 03:49:24PM -0400, Dmitry Fomichev wrote:
> +uint8_t bdrv_is_zoned(BlockDriverState *bs)
> +{
> +    /*
> +     * Host Aware zone devices are supposed to be able to work
> +     * just like regular block devices. Thus, we only consider
> +     * Host Managed devices to be zoned here.
> +     */
> +    return bdrv_get_zoned_model(bs) == BLK_ZONED_MODEL_HM;

This function doesn't say whether bs is a zoned device in general, it
says whether it's Host Managed.  Please rename to
bdrv_is_host_managed_zoned(), bdrv_is_zoned_hm(), or similar.

> +}
> +
>  bool bdrv_is_sg(BlockDriverState *bs)
>  {
>      return bs->sg;
> diff --git a/include/block/block.h b/include/block/block.h
> index 124ad40809..238c0f5ed7 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -271,18 +271,35 @@ enum {
>       */
>      BLK_PERM_GRAPH_MOD          = 0x10,
>  
> +    /** This permission is required to open zoned block devices. */
> +    BLK_PERM_SUPPORT_ZONED      = 0x20,

Maybe BLK_PERM_SUPPORT_ZONED_HM is clearer?  Let's keep the distinction
between general zoned devices and host-managed zoned devices clear,
otherwise the code will get confusing if we ever want to treat
host-aware specially.

> +
>      BLK_PERM_ALL                = 0x1f,
>  
>      DEFAULT_PERM_PASSTHROUGH    = BLK_PERM_CONSISTENT_READ
>                                   | BLK_PERM_WRITE
>                                   | BLK_PERM_WRITE_UNCHANGED
> -                                 | BLK_PERM_RESIZE,
> +                                 | BLK_PERM_RESIZE
> +                                 | BLK_PERM_SUPPORT_ZONED,
>  
>      DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
>  };
>  
>  char *bdrv_perm_names(uint64_t perm);
>  
> +/*
> + * Known zoned device models.
> + *
> + * TODO For a Linux host, it could be preferrable to include
> + * /usr/include/linux/blkzoned.h instead of defining ZBD-specific
> + * values here.

It depends.  If it's necessary to convert back to the Linux enum often
then I agree.  Otherwise the code is cleaner a QEMU enum is defined here
and we can avoid #ifdef __linux__.

> + */
> +enum blk_zoned_model {
> +    BLK_ZONED_MODEL_NONE, /* Regular block device */
> +    BLK_ZONED_MODEL_HA,   /* Host-aware zoned block device */
> +    BLK_ZONED_MODEL_HM,   /* Host-managed zoned block device */
> +};

Please use the same typedef enum approach as the rest of block.h:

  typedef enum {
      BDRV_ZONED_MODEL_NONE, /* Regular block device */
      BDRV_ZONED_MODEL_HA,   /* Host-aware zoned block device */
      BDRV_ZONED_MODEL_HM,   /* Host-managed zoned block device */
  } BdrvZonedModel;

> +
>  /* disk I/O throttling */
>  void bdrv_init(void);
>  void bdrv_init_with_whitelist(void);
> @@ -359,6 +376,8 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
>  BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
>                                 BlockDriverState *in_bs, Error **errp);
>  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
> +uint8_t bdrv_get_zoned_model(BlockDriverState *bs);

Please use the enum:

  BdrvZonedModel bdrv_get_zoned_model(BlockDriverState *bs)

> +uint8_t bdrv_is_zoned(BlockDriverState *bs);

Please use bool instead of uint8_t.

>  void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
>  int bdrv_commit(BlockDriverState *bs);
>  int bdrv_change_backing_file(BlockDriverState *bs,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index ceec8c2f56..91496e8149 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -415,6 +415,7 @@ struct BlockDriver {
>      bool (*bdrv_debug_is_suspended)(BlockDriverState *bs, const char *tag);
>  
>      void (*bdrv_refresh_limits)(BlockDriverState *bs, Error **errp);
> +    void (*bdrv_get_zoned_info)(BlockDriverState *bs);

"get" is strange since this function returns void.  What is it supposed
to do, refresh just the BlockLimits::zoned_model field?  Should this be
called bdrv_refresh_zoned_model() instead?

>  
>      /*
>       * Returns 1 if newly created images are guaranteed to contain only
> @@ -620,6 +621,9 @@ typedef struct BlockLimits {
>  
>      /* maximum number of iovec elements */
>      int max_iov;
> +
> +    /* Zoned device model. Zero value indicates a regular block device */
> +    uint8_t zoned_model;

Please use the enum.

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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 2/4] raw: Recognize zoned backing devices

[Stefan Hajnoczi]

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

On Fri, Aug 23, 2019 at 03:49:25PM -0400, Dmitry Fomichev wrote:
> +static int hdev_get_zoned_model(int fd)

Please use the enum:

  static BdrvZonedModel hdev_get_zoned_model(int fd)

> +{
> +#ifdef CONFIG_LINUX
> +    char buf[32];
> +    int ret;
> +
> +    ret = hdev_read_blk_queue_entry(fd, "zoned", buf, sizeof(buf));
> +    if (ret < 0) {
> +        ret = BLK_ZONED_MODEL_NONE;
> +        goto out;
>      }
> -    g_free(sysfspath);
> +
> +    buf[ret - 1] = 0;

The ret - 1 looks suspicious since sg_get_max_segments() does buf[ret] =
0 instead.  A comment would make it clear why ret - 1 is used here:

  buf[ret - 1] = 0; /* strip newline and NUL-terminate */

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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 3/4] block/ide/scsi: Set BLK_PERM_SUPPORT_ZONED

[Stefan Hajnoczi]

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

On Fri, Aug 23, 2019 at 03:49:26PM -0400, Dmitry Fomichev wrote:
> Added a new boolean argument to blkconf_apply_backend_options()
> to let the common block code know whether the chosen block
> backend can handle zoned block devices or not.
> 
> blkconf_apply_backend_options() then sets BLK_PERM_SUPPORT_ZONED
> permission accordingly. The raw code can then use this permission
> to allow or deny opening a zone device by a particular block driver.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> Acked-by: Paul Durrant <paul.durrant@citrix.com>
> ---
>  hw/block/block.c         |  8 ++++++--
>  hw/block/fdc.c           |  5 +++--
>  hw/block/nvme.c          |  2 +-
>  hw/block/virtio-blk.c    |  2 +-
>  hw/block/xen-block.c     |  2 +-
>  hw/ide/qdev.c            |  2 +-
>  hw/scsi/scsi-disk.c      | 13 +++++++------
>  hw/scsi/scsi-generic.c   |  2 +-
>  hw/usb/dev-storage.c     |  2 +-
>  include/hw/block/block.h |  3 ++-
>  10 files changed, 24 insertions(+), 17 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v5 4/4] raw: Don't open ZBDs if backend can't handle them

[Stefan Hajnoczi]

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

On Fri, Aug 23, 2019 at 03:49:27PM -0400, Dmitry Fomichev wrote:
> diff --git a/block/file-posix.c b/block/file-posix.c
> index d9f2fc5e46..090e7c4d2f 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2884,6 +2884,20 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
>              goto fail;
>          }
>      }
> +
> +    /*
> +     * If we are opening a zoned block device, check if the backend
> +     * driver can properly handle such devices, abort if not.
> +     */
> +    if (bdrv_is_zoned(bs) &&
> +        (shared & BLK_PERM_SUPPORT_ZONED) &&
> +        !(perm & BLK_PERM_SUPPORT_ZONED)) {
> +        error_setg(errp,
> +                   "block backend driver doesn't support HM zoned devices");

Spelling out "host-managed" would be helpful in the error message.  Web
search results for "hm zoned" and "hm zoned devices" aren't great,
whereas "host-managed zoned" brings up the T10 page.

> +        ret = -ENOTSUP;
> +        goto fail;
> +    }
> +
>      return 0;
>  
>  fail:
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index c5f41b3eb6..673a8b39bc 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -570,7 +570,7 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
>      dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
>  
>      if (!blkconf_apply_backend_options(&dev->conf, read_only, false, false,
> -    				       errp)) {
> +                                       errp)) {

Please squash this whitespace change into the previous patch where the
false argument was first introduced.

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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 0/4] virtio/block: handle zoned backing devices

[Stefan Hajnoczi]

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

On Fri, Aug 23, 2019 at 03:49:23PM -0400, Dmitry Fomichev wrote:
> Dmitry Fomichev (4):
>   block: Add zoned device model property
>   raw: Recognize zoned backing devices
>   block/ide/scsi: Set BLK_PERM_SUPPORT_ZONED
>   raw: Don't open ZBDs if backend can't handle them

The overall approach looks good.

I wonder if bdrv_get_zoned_info() is really needed since zone_model is
part of BlockLimits and is already fetched via bdrv_refresh_limits().
Was it introduced because the block limits haven't been or cannot be
fetched when zone_model is queried the first time?  It would be nice to
get rid of bdrv_get_zoned_info() if possible.

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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 0/4] virtio/block: handle zoned backing devices

[Dmitry Fomichev]

On Wed, 2019-08-28 at 10:41 +0100, Stefan Hajnoczi wrote:
> On Fri, Aug 23, 2019 at 03:49:23PM -0400, Dmitry Fomichev wrote:
> > Dmitry Fomichev (4):
> >   block: Add zoned device model property
> >   raw: Recognize zoned backing devices
> >   block/ide/scsi: Set BLK_PERM_SUPPORT_ZONED
> >   raw: Don't open ZBDs if backend can't handle them
> 
> The overall approach looks good.
> 
> I wonder if bdrv_get_zoned_info() is really needed since zone_model is
> part of BlockLimits and is already fetched via bdrv_refresh_limits().
> Was it introduced because the block limits haven't been or cannot be
> fetched when zone_model is queried the first time?  It would be nice to
> get rid of bdrv_get_zoned_info() if possible.

Stefan,

Thank you for your review. I've just sent out the new version of the
patchset, v6, that addresses you comments. I was able to get rid of
bdrv_get_zoned_info() template function as a part of the latest changes.

Best regards,
Dmitry