[PATCH v4 0/3] block: make BlockConf.*_size properties 32-bit

[PATCH v4 0/3] block: make BlockConf.*_size properties 32-bit

[Roman Kagan]

Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
32-bit for logical_block_size, physical_block_size, and min_io_size.
However, the properties in BlockConf are defined as uint16_t limiting
the values to 32768.

This appears unnecessary tight, and we've seen bigger block sizes handy
at times.

Make them 32 bit instead and lift the limitation up to 2 MiB which
appears to be good enough for everybody, and matches the qcow2 cluster
size limit.

As the values can now be fairly big and awkward to type, make the
property setter accept common size suffixes (k, m).

While at this, introduce a few consistency checks on the blocksize-related
values in BlockConf, to prevent their silent truncation or rounding.

Also fix the accessor for opt_io_size in virtio-blk to make it consistent with
the size of the field, and thus synchronize virtio-blk and scsi in the way
opt_io_size is used.

History:
v3 -> v4:
- add patch to opt_io_size width
- add patch to perform consistency checks [Kevin]
- check min_io_size against truncation [Kevin]

v2 -> v3:
- mention qcow2 cluster size limit in the log and comment [Eric]

v1 -> v2:
- cap the property at 2 MiB [Eric]
- accept size suffixes

Roman Kagan (3):
  virtio-blk: store opt_io_size with correct size
  block: consolidate blocksize properties consistency checks
  block: make BlockConf.*_size properties 32-bit

 include/hw/block/block.h     | 10 ++++-----
 include/hw/qdev-properties.h |  2 +-
 hw/block/block.c             | 40 +++++++++++++++++++++++++++++++++++-
 hw/block/fdc.c               |  5 ++++-
 hw/block/nvme.c              |  5 ++++-
 hw/block/virtio-blk.c        |  9 ++------
 hw/core/qdev-properties.c    | 34 +++++++++++++++++++++---------
 hw/ide/qdev.c                |  5 ++++-
 hw/scsi/scsi-disk.c          | 10 +++------
 hw/usb/dev-storage.c         |  5 ++++-
 tests/qemu-iotests/172.out   |  2 +-
 11 files changed, 91 insertions(+), 36 deletions(-)

-- 
2.26.2

[PATCH v4 1/3] virtio-blk: store opt_io_size with correct size

[Roman Kagan]

The width of opt_io_size in virtio_blk_topology is 32bit.

Use the appropriate accessor to store it.

Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
---
v4: new patch

 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f5f6fc925e..413083e62f 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -918,7 +918,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
     virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
     virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
-    virtio_stw_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
+    virtio_stl_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
     blkcfg.geometry.heads = conf->heads;
     /*
      * We must ensure that the block device capacity is a multiple of
-- 
2.26.2

[PATCH v4 2/3] block: consolidate blocksize properties consistency checks

[Roman Kagan]

Several block device properties related to blocksize configuration must
be in certain relationship WRT each other: physical block must be no
smaller than logical block; min_io_size, opt_io_size, and
discard_granularity must be a multiple of a logical block.

To ensure these requirements are met, add corresponding consistency
checks to blkconf_blocksizes, adjusting its signature to communicate
possible error to the caller.  Also remove the now redundant consistency
checks from the specific devices.

Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
---
v4: new patch

 include/hw/block/block.h   |  2 +-
 hw/block/block.c           | 29 ++++++++++++++++++++++++++++-
 hw/block/fdc.c             |  5 ++++-
 hw/block/nvme.c            |  5 ++++-
 hw/block/virtio-blk.c      |  7 +------
 hw/ide/qdev.c              |  5 ++++-
 hw/scsi/scsi-disk.c        | 10 +++-------
 hw/usb/dev-storage.c       |  5 ++++-
 tests/qemu-iotests/172.out |  2 +-
 9 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index d7246f3862..784953a237 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
 bool blkconf_geometry(BlockConf *conf, int *trans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp);
-void blkconf_blocksizes(BlockConf *conf);
+bool blkconf_blocksizes(BlockConf *conf, Error **errp);
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
                                    bool resizable, Error **errp);
 
diff --git a/hw/block/block.c b/hw/block/block.c
index bf56c7612b..5f8ebff59c 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
     return true;
 }
 
-void blkconf_blocksizes(BlockConf *conf)
+bool blkconf_blocksizes(BlockConf *conf, Error **errp)
 {
     BlockBackend *blk = conf->blk;
     BlockSizes blocksizes;
@@ -83,6 +83,33 @@ void blkconf_blocksizes(BlockConf *conf)
             conf->logical_block_size = BDRV_SECTOR_SIZE;
         }
     }
+
+    if (conf->logical_block_size > conf->physical_block_size) {
+        error_setg(errp,
+                   "logical_block_size > physical_block_size not supported");
+        return false;
+    }
+
+    if (conf->min_io_size % conf->logical_block_size) {
+        error_setg(errp,
+                   "min_io_size must be a multple of logical_block_size");
+        return false;
+    }
+
+    if (conf->opt_io_size % conf->logical_block_size) {
+        error_setg(errp,
+                   "opt_io_size must be a multple of logical_block_size");
+        return false;
+    }
+
+    if (conf->discard_granularity != -1 &&
+        conf->discard_granularity % conf->logical_block_size) {
+        error_setg(errp, "discard_granularity must be "
+                   "a multple of logical_block_size");
+        return false;
+    }
+
+    return true;
 }
 
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index c5fb9d6ece..8eda572ef4 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -554,7 +554,10 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
         read_only = !blk_bs(dev->conf.blk) || blk_is_read_only(dev->conf.blk);
     }
 
-    blkconf_blocksizes(&dev->conf);
+    if (!blkconf_blocksizes(&dev->conf, errp)) {
+        return;
+    }
+
     if (dev->conf.logical_block_size != 512 ||
         dev->conf.physical_block_size != 512)
     {
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2f3100e56c..672650e162 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1390,7 +1390,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         host_memory_backend_set_mapped(n->pmrdev, true);
     }
 
-    blkconf_blocksizes(&n->conf);
+    if (!blkconf_blocksizes(&n->conf, errp)) {
+        return;
+    }
+
     if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
                                        false, errp)) {
         return;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 413083e62f..4ffdb130be 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1162,12 +1162,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    blkconf_blocksizes(&conf->conf);
-
-    if (conf->conf.logical_block_size >
-        conf->conf.physical_block_size) {
-        error_setg(errp,
-                   "logical_block_size > physical_block_size not supported");
+    if (!blkconf_blocksizes(&conf->conf, errp)) {
         return;
     }
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 06b11583f5..b4821b2403 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -187,7 +187,10 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
         return;
     }
 
-    blkconf_blocksizes(&dev->conf);
+    if (!blkconf_blocksizes(&dev->conf, errp)) {
+        return;
+    }
+
     if (dev->conf.logical_block_size != 512) {
         error_setg(errp, "logical_block_size must be 512 for IDE");
         return;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..06c8f1ba92 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2346,12 +2346,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
         return;
     }
 
-    blkconf_blocksizes(&s->qdev.conf);
-
-    if (s->qdev.conf.logical_block_size >
-        s->qdev.conf.physical_block_size) {
-        error_setg(errp,
-                   "logical_block_size > physical_block_size not supported");
+    if (!blkconf_blocksizes(&s->qdev.conf, errp)) {
         return;
     }
 
@@ -2436,7 +2431,8 @@ static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
     if (s->qdev.conf.blk) {
         ctx = blk_get_aio_context(s->qdev.conf.blk);
         aio_context_acquire(ctx);
-        blkconf_blocksizes(&s->qdev.conf);
+        /* ignore errors, blocksizes will be revalidated in scsi_realize */
+        blkconf_blocksizes(&s->qdev.conf, NULL);
     }
     s->qdev.blocksize = s->qdev.conf.logical_block_size;
     s->qdev.type = TYPE_DISK;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 4eba47538d..de461f37bd 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -599,7 +599,10 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
         return;
     }
 
-    blkconf_blocksizes(&s->conf);
+    if (!blkconf_blocksizes(&s->conf, errp)) {
+        return;
+    }
+
     if (!blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true,
                                        errp)) {
         return;
diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
index 7abbe82427..59cc70aebb 100644
--- a/tests/qemu-iotests/172.out
+++ b/tests/qemu-iotests/172.out
@@ -1204,7 +1204,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica
                 drive-type = "144"
 
 Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical_block_size=4096
-QEMU_PROG: -device floppy,drive=none0,logical_block_size=4096: Physical and logical block size must be 512 for floppy
+QEMU_PROG: -device floppy,drive=none0,logical_block_size=4096: logical_block_size > physical_block_size not supported
 
 Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physical_block_size=1024
 QEMU_PROG: -device floppy,drive=none0,physical_block_size=1024: Physical and logical block size must be 512 for floppy
-- 
2.26.2

[PATCH v4 3/3] block: make BlockConf.*_size properties 32-bit

[Roman Kagan]

Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
32-bit for logical_block_size, physical_block_size, and min_io_size.
However, the properties in BlockConf are defined as uint16_t limiting
the values to 32768.

This appears unnecessary tight, and we've seen bigger block sizes handy
at times.

Make them 32 bit instead and lift the limitation up to 2 MiB which
appears to be good enough for everybody, and matches the qcow2 cluster
size limit.

As the values can now be fairly big and awkward to type, make the
property setter accept common size suffixes (k, m).

Also as the devices which use min_io_size (virtio-blk and scsi) pass its
value to the guest in units of logical blocks in a 16bit field, to
prevent its silent truncation add a corresponding check to
blkconf_blocksizes.

Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
---
v3 -> v4:
- check min_io_size against truncation [Kevin]

v2 -> v3:
- mention qcow2 cluster size limit in the log and comment [Eric]

v1 -> v2:
- cap the property at 2 MiB [Eric]
- accept size suffixes

 include/hw/block/block.h     |  8 ++++----
 include/hw/qdev-properties.h |  2 +-
 hw/block/block.c             | 11 +++++++++++
 hw/core/qdev-properties.c    | 34 ++++++++++++++++++++++++----------
 4 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 784953a237..2fa09aa0b1 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -18,9 +18,9 @@
 
 typedef struct BlockConf {
     BlockBackend *blk;
-    uint16_t physical_block_size;
-    uint16_t logical_block_size;
-    uint16_t min_io_size;
+    uint32_t physical_block_size;
+    uint32_t logical_block_size;
+    uint32_t min_io_size;
     uint32_t opt_io_size;
     int32_t bootindex;
     uint32_t discard_granularity;
@@ -51,7 +51,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
                           _conf.logical_block_size),                    \
     DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
                           _conf.physical_block_size),                   \
-    DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),    \
+    DEFINE_PROP_UINT32("min_io_size", _state, _conf.min_io_size, 0),    \
     DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
     DEFINE_PROP_UINT32("discard_granularity", _state,                   \
                        _conf.discard_granularity, -1),                  \
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index f161604fb6..f9e0f8c041 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -197,7 +197,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
 #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
 #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
-    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
+    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
 #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
     DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
 #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
diff --git a/hw/block/block.c b/hw/block/block.c
index 5f8ebff59c..cd95e7e38f 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -96,6 +96,17 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
         return false;
     }
 
+    /*
+     * all devices which support min_io_size (scsi and virtio-blk) expose it to
+     * the guest as a uint16_t in units of logical blocks
+     */
+    if ((conf->min_io_size / conf->logical_block_size) > UINT16_MAX) {
+        error_setg(errp,
+                   "min_io_size must be no more than " stringify(UINT16_MAX)
+                   " of logical_block_size");
+        return false;
+    }
+
     if (conf->opt_io_size % conf->logical_block_size) {
         error_setg(errp,
                    "opt_io_size must be a multple of logical_block_size");
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index cc924815da..fd03cc7597 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -14,6 +14,7 @@
 #include "qapi/visitor.h"
 #include "chardev/char.h"
 #include "qemu/uuid.h"
+#include "qemu/units.h"
 
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
                                   Error **errp)
@@ -729,30 +730,42 @@ const PropertyInfo qdev_prop_pci_devfn = {
 
 /* --- blocksize --- */
 
+/* lower limit is sector size */
+#define MIN_BLOCK_SIZE          512
+#define MIN_BLOCK_SIZE_STR      "512 B"
+/*
+ * upper limit is arbitrary, 2 MiB looks sufficient for all sensible uses, and
+ * matches qcow2 cluster size limit
+ */
+#define MAX_BLOCK_SIZE          (2 * MiB)
+#define MAX_BLOCK_SIZE_STR      "2 MiB"
+
 static void set_blocksize(Object *obj, Visitor *v, const char *name,
                           void *opaque, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
     Property *prop = opaque;
-    uint16_t value, *ptr = qdev_get_prop_ptr(dev, prop);
+    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
+    uint64_t value;
     Error *local_err = NULL;
-    const int64_t min = 512;
-    const int64_t max = 32768;
 
     if (dev->realized) {
         qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
-    visit_type_uint16(v, name, &value, &local_err);
+    visit_type_size(v, name, &value, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
     /* value of 0 means "unset" */
-    if (value && (value < min || value > max)) {
-        error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
-                   dev->id ? : "", name, (int64_t)value, min, max);
+    if (value && (value < MIN_BLOCK_SIZE || value > MAX_BLOCK_SIZE)) {
+        error_setg(errp,
+                   "Property %s.%s doesn't take value %" PRIu64
+                   " (minimum: " MIN_BLOCK_SIZE_STR
+                   ", maximum: " MAX_BLOCK_SIZE_STR ")",
+                   dev->id ? : "", name, value);
         return;
     }
 
@@ -768,9 +781,10 @@ static void set_blocksize(Object *obj, Visitor *v, const char *name,
 }
 
 const PropertyInfo qdev_prop_blocksize = {
-    .name  = "uint16",
-    .description = "A power of two between 512 and 32768",
-    .get   = get_uint16,
+    .name  = "size",
+    .description = "A power of two between " MIN_BLOCK_SIZE_STR
+                   " and " MAX_BLOCK_SIZE_STR,
+    .get   = get_uint32,
     .set   = set_blocksize,
     .set_default_value = set_default_value_uint,
 };
-- 
2.26.2

Re: [PATCH v4 1/3] virtio-blk: store opt_io_size with correct size

[Philippe Mathieu-Daudé]

On 5/20/20 10:06 AM, Roman Kagan wrote:
> The width of opt_io_size in virtio_blk_topology is 32bit.
> 
> Use the appropriate accessor to store it.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> ---
> v4: new patch
> 
>   hw/block/virtio-blk.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f5f6fc925e..413083e62f 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -918,7 +918,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>       virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
>       virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
>       virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> -    virtio_stw_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
> +    virtio_stl_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
>       blkcfg.geometry.heads = conf->heads;
>       /*
>        * We must ensure that the block device capacity is a multiple of
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v4 2/3] block: consolidate blocksize properties consistency checks

[Philippe Mathieu-Daudé]

Hi Roman,

On 5/20/20 10:06 AM, Roman Kagan wrote:
> Several block device properties related to blocksize configuration must
> be in certain relationship WRT each other: physical block must be no
> smaller than logical block; min_io_size, opt_io_size, and
> discard_granularity must be a multiple of a logical block.
> 
> To ensure these requirements are met, add corresponding consistency
> checks to blkconf_blocksizes, adjusting its signature to communicate
> possible error to the caller.  Also remove the now redundant consistency
> checks from the specific devices.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> ---
> v4: new patch
> 
>   include/hw/block/block.h   |  2 +-
>   hw/block/block.c           | 29 ++++++++++++++++++++++++++++-
>   hw/block/fdc.c             |  5 ++++-
>   hw/block/nvme.c            |  5 ++++-
>   hw/block/virtio-blk.c      |  7 +------
>   hw/ide/qdev.c              |  5 ++++-
>   hw/scsi/scsi-disk.c        | 10 +++-------
>   hw/usb/dev-storage.c       |  5 ++++-
>   tests/qemu-iotests/172.out |  2 +-
>   9 files changed, 50 insertions(+), 20 deletions(-)
> 
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index d7246f3862..784953a237 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>   bool blkconf_geometry(BlockConf *conf, int *trans,
>                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
>                         Error **errp);
> -void blkconf_blocksizes(BlockConf *conf);
> +bool blkconf_blocksizes(BlockConf *conf, Error **errp);
>   bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
>                                      bool resizable, Error **errp);
>   
> diff --git a/hw/block/block.c b/hw/block/block.c
> index bf56c7612b..5f8ebff59c 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>       return true;
>   }
>   
> -void blkconf_blocksizes(BlockConf *conf)
> +bool blkconf_blocksizes(BlockConf *conf, Error **errp)
>   {
>       BlockBackend *blk = conf->blk;
>       BlockSizes blocksizes;
> @@ -83,6 +83,33 @@ void blkconf_blocksizes(BlockConf *conf)
>               conf->logical_block_size = BDRV_SECTOR_SIZE;
>           }
>       }
> +
> +    if (conf->logical_block_size > conf->physical_block_size) {
> +        error_setg(errp,
> +                   "logical_block_size > physical_block_size not supported");

"not supported" or "invalid"?

> +        return false;
> +    }
> +
> +    if (conf->min_io_size % conf->logical_block_size) {

It seems the block code usually do:

        if (!QEMU_IS_ALIGNED(conf->min_io_size, conf->logical_block_size)) {

> +        error_setg(errp,
> +                   "min_io_size must be a multple of logical_block_size");

Typo "multple" -> "multiple".

> +        return false;
> +    }
> +
> +    if (conf->opt_io_size % conf->logical_block_size) {
> +        error_setg(errp,
> +                   "opt_io_size must be a multple of logical_block_size");

Ditto.

> +        return false;
> +    }
> +
> +    if (conf->discard_granularity != -1 &&
> +        conf->discard_granularity % conf->logical_block_size) {
> +        error_setg(errp, "discard_granularity must be "
> +                   "a multple of logical_block_size");

Again.

> +        return false;
> +    }
> +
> +    return true;

Usually we return true for error, isn't it?

>   }
>   
>   bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index c5fb9d6ece..8eda572ef4 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -554,7 +554,10 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
>           read_only = !blk_bs(dev->conf.blk) || blk_is_read_only(dev->conf.blk);
>       }
>   
> -    blkconf_blocksizes(&dev->conf);
> +    if (!blkconf_blocksizes(&dev->conf, errp)) {
> +        return;
> +    }
> +
>       if (dev->conf.logical_block_size != 512 ||
>           dev->conf.physical_block_size != 512)
>       {
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 2f3100e56c..672650e162 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1390,7 +1390,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>           host_memory_backend_set_mapped(n->pmrdev, true);
>       }
>   
> -    blkconf_blocksizes(&n->conf);
> +    if (!blkconf_blocksizes(&n->conf, errp)) {
> +        return;
> +    }
> +
>       if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
>                                          false, errp)) {
>           return;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 413083e62f..4ffdb130be 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1162,12 +1162,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    blkconf_blocksizes(&conf->conf);
> -
> -    if (conf->conf.logical_block_size >
> -        conf->conf.physical_block_size) {
> -        error_setg(errp,
> -                   "logical_block_size > physical_block_size not supported");

Ah, "not supported" comes from here, OK.

> +    if (!blkconf_blocksizes(&conf->conf, errp)) {
>           return;
>       }
>   
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 06b11583f5..b4821b2403 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -187,7 +187,10 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
>           return;
>       }
>   
> -    blkconf_blocksizes(&dev->conf);
> +    if (!blkconf_blocksizes(&dev->conf, errp)) {
> +        return;
> +    }
> +
>       if (dev->conf.logical_block_size != 512) {
>           error_setg(errp, "logical_block_size must be 512 for IDE");
>           return;
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 387503e11b..06c8f1ba92 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2346,12 +2346,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
>           return;
>       }
>   
> -    blkconf_blocksizes(&s->qdev.conf);
> -
> -    if (s->qdev.conf.logical_block_size >
> -        s->qdev.conf.physical_block_size) {
> -        error_setg(errp,
> -                   "logical_block_size > physical_block_size not supported");
> +    if (!blkconf_blocksizes(&s->qdev.conf, errp)) {
>           return;
>       }
>   
> @@ -2436,7 +2431,8 @@ static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
>       if (s->qdev.conf.blk) {
>           ctx = blk_get_aio_context(s->qdev.conf.blk);
>           aio_context_acquire(ctx);
> -        blkconf_blocksizes(&s->qdev.conf);
> +        /* ignore errors, blocksizes will be revalidated in scsi_realize */
> +        blkconf_blocksizes(&s->qdev.conf, NULL);

Hmm, why not bail out ASAP on error?

Good cleanup otherwise.

>       }
>       s->qdev.blocksize = s->qdev.conf.logical_block_size;
>       s->qdev.type = TYPE_DISK;
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 4eba47538d..de461f37bd 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -599,7 +599,10 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
>           return;
>       }
>   
> -    blkconf_blocksizes(&s->conf);
> +    if (!blkconf_blocksizes(&s->conf, errp)) {
> +        return;
> +    }
> +
>       if (!blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true,
>                                          errp)) {
>           return;
> diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
> index 7abbe82427..59cc70aebb 100644
> --- a/tests/qemu-iotests/172.out
> +++ b/tests/qemu-iotests/172.out
> @@ -1204,7 +1204,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica
>                   drive-type = "144"
>   
>   Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical_block_size=4096
> -QEMU_PROG: -device floppy,drive=none0,logical_block_size=4096: Physical and logical block size must be 512 for floppy
> +QEMU_PROG: -device floppy,drive=none0,logical_block_size=4096: logical_block_size > physical_block_size not supported
>   
>   Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physical_block_size=1024
>   QEMU_PROG: -device floppy,drive=none0,physical_block_size=1024: Physical and logical block size must be 512 for floppy
> 

Re: [PATCH v4 2/3] block: consolidate blocksize properties consistency checks

[Philippe Mathieu-Daudé]

On Wed, May 20, 2020 at 10:57 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Roman,
>
> On 5/20/20 10:06 AM, Roman Kagan wrote:
> > Several block device properties related to blocksize configuration must
> > be in certain relationship WRT each other: physical block must be no
> > smaller than logical block; min_io_size, opt_io_size, and
> > discard_granularity must be a multiple of a logical block.
> >
> > To ensure these requirements are met, add corresponding consistency
> > checks to blkconf_blocksizes, adjusting its signature to communicate
> > possible error to the caller.  Also remove the now redundant consistency
> > checks from the specific devices.
> >
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > ---
> > v4: new patch
> >
> >   include/hw/block/block.h   |  2 +-
> >   hw/block/block.c           | 29 ++++++++++++++++++++++++++++-
> >   hw/block/fdc.c             |  5 ++++-
> >   hw/block/nvme.c            |  5 ++++-
> >   hw/block/virtio-blk.c      |  7 +------
> >   hw/ide/qdev.c              |  5 ++++-
> >   hw/scsi/scsi-disk.c        | 10 +++-------
> >   hw/usb/dev-storage.c       |  5 ++++-
> >   tests/qemu-iotests/172.out |  2 +-
> >   9 files changed, 50 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index d7246f3862..784953a237 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
> >   bool blkconf_geometry(BlockConf *conf, int *trans,
> >                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
> >                         Error **errp);
> > -void blkconf_blocksizes(BlockConf *conf);
> > +bool blkconf_blocksizes(BlockConf *conf, Error **errp);

Maybe rename blkconf_validate_blocksizes()?

> >   bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> >                                      bool resizable, Error **errp);
> >
[...]

Re: [PATCH v4 3/3] block: make BlockConf.*_size properties 32-bit

[Philippe Mathieu-Daudé]

On 5/20/20 10:06 AM, Roman Kagan wrote:
> Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
> 32-bit for logical_block_size, physical_block_size, and min_io_size.
> However, the properties in BlockConf are defined as uint16_t limiting
> the values to 32768.
> 
> This appears unnecessary tight, and we've seen bigger block sizes handy
> at times.
> 
> Make them 32 bit instead and lift the limitation up to 2 MiB which
> appears to be good enough for everybody, and matches the qcow2 cluster
> size limit.
> 
> As the values can now be fairly big and awkward to type, make the
> property setter accept common size suffixes (k, m).
> 
> Also as the devices which use min_io_size (virtio-blk and scsi) pass its
> value to the guest in units of logical blocks in a 16bit field, to
> prevent its silent truncation add a corresponding check to
> blkconf_blocksizes.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> ---
> v3 -> v4:
> - check min_io_size against truncation [Kevin]
> 
> v2 -> v3:
> - mention qcow2 cluster size limit in the log and comment [Eric]
> 
> v1 -> v2:
> - cap the property at 2 MiB [Eric]
> - accept size suffixes
> 
>   include/hw/block/block.h     |  8 ++++----
>   include/hw/qdev-properties.h |  2 +-
>   hw/block/block.c             | 11 +++++++++++
>   hw/core/qdev-properties.c    | 34 ++++++++++++++++++++++++----------
>   4 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 784953a237..2fa09aa0b1 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -18,9 +18,9 @@
>   
>   typedef struct BlockConf {
>       BlockBackend *blk;
> -    uint16_t physical_block_size;
> -    uint16_t logical_block_size;
> -    uint16_t min_io_size;
> +    uint32_t physical_block_size;
> +    uint32_t logical_block_size;
> +    uint32_t min_io_size;
>       uint32_t opt_io_size;
>       int32_t bootindex;
>       uint32_t discard_granularity;
> @@ -51,7 +51,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>                             _conf.logical_block_size),                    \
>       DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
>                             _conf.physical_block_size),                   \
> -    DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),    \
> +    DEFINE_PROP_UINT32("min_io_size", _state, _conf.min_io_size, 0),    \
>       DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
>       DEFINE_PROP_UINT32("discard_granularity", _state,                   \
>                          _conf.discard_granularity, -1),                  \
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index f161604fb6..f9e0f8c041 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -197,7 +197,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>   #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
>       DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
>   #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
>   #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
>       DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
>   #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
> diff --git a/hw/block/block.c b/hw/block/block.c
> index 5f8ebff59c..cd95e7e38f 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -96,6 +96,17 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
>           return false;
>       }
>   
> +    /*
> +     * all devices which support min_io_size (scsi and virtio-blk) expose it to
> +     * the guest as a uint16_t in units of logical blocks
> +     */
> +    if ((conf->min_io_size / conf->logical_block_size) > UINT16_MAX) {
> +        error_setg(errp,
> +                   "min_io_size must be no more than " stringify(UINT16_MAX)
> +                   " of logical_block_size");
> +        return false;
> +    }
> +
>       if (conf->opt_io_size % conf->logical_block_size) {
>           error_setg(errp,
>                      "opt_io_size must be a multple of logical_block_size");
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index cc924815da..fd03cc7597 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -14,6 +14,7 @@
>   #include "qapi/visitor.h"
>   #include "chardev/char.h"
>   #include "qemu/uuid.h"
> +#include "qemu/units.h"
>   
>   void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
>                                     Error **errp)
> @@ -729,30 +730,42 @@ const PropertyInfo qdev_prop_pci_devfn = {
>   
>   /* --- blocksize --- */
>   
> +/* lower limit is sector size */
> +#define MIN_BLOCK_SIZE          512
> +#define MIN_BLOCK_SIZE_STR      "512 B"
> +/*
> + * upper limit is arbitrary, 2 MiB looks sufficient for all sensible uses, and
> + * matches qcow2 cluster size limit
> + */
> +#define MAX_BLOCK_SIZE          (2 * MiB)

Can you split this patch?

- add/use definitions (here MAX_BLOCK_SIZE = 32 * KiB)
- use 32-bit
- raise limit to 2MB

Feel free to refuse, but having atomic changes is better for 
bisectability and cherry-picking/reverting.

Thanks,

Phil.

> +#define MAX_BLOCK_SIZE_STR      "2 MiB"
> +
>   static void set_blocksize(Object *obj, Visitor *v, const char *name,
>                             void *opaque, Error **errp)
>   {
>       DeviceState *dev = DEVICE(obj);
>       Property *prop = opaque;
> -    uint16_t value, *ptr = qdev_get_prop_ptr(dev, prop);
> +    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    uint64_t value;
>       Error *local_err = NULL;
> -    const int64_t min = 512;
> -    const int64_t max = 32768;
>   
>       if (dev->realized) {
>           qdev_prop_set_after_realize(dev, name, errp);
>           return;
>       }
>   
> -    visit_type_uint16(v, name, &value, &local_err);
> +    visit_type_size(v, name, &value, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
>           return;
>       }
>       /* value of 0 means "unset" */
> -    if (value && (value < min || value > max)) {
> -        error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> -                   dev->id ? : "", name, (int64_t)value, min, max);
> +    if (value && (value < MIN_BLOCK_SIZE || value > MAX_BLOCK_SIZE)) {
> +        error_setg(errp,
> +                   "Property %s.%s doesn't take value %" PRIu64
> +                   " (minimum: " MIN_BLOCK_SIZE_STR
> +                   ", maximum: " MAX_BLOCK_SIZE_STR ")",
> +                   dev->id ? : "", name, value);
>           return;
>       }
>   
> @@ -768,9 +781,10 @@ static void set_blocksize(Object *obj, Visitor *v, const char *name,
>   }
>   
>   const PropertyInfo qdev_prop_blocksize = {
> -    .name  = "uint16",
> -    .description = "A power of two between 512 and 32768",
> -    .get   = get_uint16,
> +    .name  = "size",
> +    .description = "A power of two between " MIN_BLOCK_SIZE_STR
> +                   " and " MAX_BLOCK_SIZE_STR,
> +    .get   = get_uint32,
>       .set   = set_blocksize,
>       .set_default_value = set_default_value_uint,
>   };
> 

Re: [PATCH v4 1/3] virtio-blk: store opt_io_size with correct size

[Michael S. Tsirkin]

On Wed, May 20, 2020 at 11:06:55AM +0300, Roman Kagan wrote:
> The width of opt_io_size in virtio_blk_topology is 32bit.
> 
> Use the appropriate accessor to store it.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>


Thanks for the patch!
Could you add a bit of analysis - when does this cause
bugs? I'm guessing on BE systems with legacy virtio, right?

Also, should we convert virtio_stw_p and friends to get the
pointer to the correct value type, as opposed to void *?

This will catch bugs like this ...



> ---
> v4: new patch
> 
>  hw/block/virtio-blk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f5f6fc925e..413083e62f 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -918,7 +918,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
>      virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
>      virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> -    virtio_stw_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
> +    virtio_stl_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
>      blkcfg.geometry.heads = conf->heads;
>      /*
>       * We must ensure that the block device capacity is a multiple of
> -- 
> 2.26.2

Re: [PATCH v4 1/3] virtio-blk: store opt_io_size with correct size

[Kevin Wolf]

Am 20.05.2020 um 10:06 hat Roman Kagan geschrieben:
> The width of opt_io_size in virtio_blk_topology is 32bit.

I think you mean virtio_blk_config?

> Use the appropriate accessor to store it.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH v4 2/3] block: consolidate blocksize properties consistency checks

[Kevin Wolf]

Am 20.05.2020 um 10:57 hat Philippe Mathieu-Daudé geschrieben:
> Hi Roman,
> 
> On 5/20/20 10:06 AM, Roman Kagan wrote:
> > Several block device properties related to blocksize configuration must
> > be in certain relationship WRT each other: physical block must be no
> > smaller than logical block; min_io_size, opt_io_size, and
> > discard_granularity must be a multiple of a logical block.
> > 
> > To ensure these requirements are met, add corresponding consistency
> > checks to blkconf_blocksizes, adjusting its signature to communicate
> > possible error to the caller.  Also remove the now redundant consistency
> > checks from the specific devices.
> > 
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > ---
> > v4: new patch
> > 
> >   include/hw/block/block.h   |  2 +-
> >   hw/block/block.c           | 29 ++++++++++++++++++++++++++++-
> >   hw/block/fdc.c             |  5 ++++-
> >   hw/block/nvme.c            |  5 ++++-
> >   hw/block/virtio-blk.c      |  7 +------
> >   hw/ide/qdev.c              |  5 ++++-
> >   hw/scsi/scsi-disk.c        | 10 +++-------
> >   hw/usb/dev-storage.c       |  5 ++++-
> >   tests/qemu-iotests/172.out |  2 +-
> >   9 files changed, 50 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index d7246f3862..784953a237 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
> >   bool blkconf_geometry(BlockConf *conf, int *trans,
> >                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
> >                         Error **errp);
> > -void blkconf_blocksizes(BlockConf *conf);
> > +bool blkconf_blocksizes(BlockConf *conf, Error **errp);
> >   bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> >                                      bool resizable, Error **errp);
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index bf56c7612b..5f8ebff59c 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
> >       return true;
> >   }
> > -void blkconf_blocksizes(BlockConf *conf)
> > +bool blkconf_blocksizes(BlockConf *conf, Error **errp)
> >   {
> >       BlockBackend *blk = conf->blk;
> >       BlockSizes blocksizes;
> > @@ -83,6 +83,33 @@ void blkconf_blocksizes(BlockConf *conf)
> >               conf->logical_block_size = BDRV_SECTOR_SIZE;
> >           }
> >       }
> > +
> > +    if (conf->logical_block_size > conf->physical_block_size) {
> > +        error_setg(errp,
> > +                   "logical_block_size > physical_block_size not supported");
> 
> "not supported" or "invalid"?

I'm not sure about strictly invalid, though it's certainly a weird case.
But there is enough weird stuff in real hardware...

"not supported" is correct either case, so I think the message is fine.

> > +        return false;
> > +    }
> > +
> > +    if (conf->min_io_size % conf->logical_block_size) {
> 
> It seems the block code usually do:
> 
>        if (!QEMU_IS_ALIGNED(conf->min_io_size, conf->logical_block_size)) {
> 
> > +        error_setg(errp,
> > +                   "min_io_size must be a multple of logical_block_size");
> 
> Typo "multple" -> "multiple".
> 
> > +        return false;
> > +    }
> > +
> > +    if (conf->opt_io_size % conf->logical_block_size) {
> > +        error_setg(errp,
> > +                   "opt_io_size must be a multple of logical_block_size");
> 
> Ditto.
> 
> > +        return false;
> > +    }
> > +
> > +    if (conf->discard_granularity != -1 &&
> > +        conf->discard_granularity % conf->logical_block_size) {
> > +        error_setg(errp, "discard_granularity must be "
> > +                   "a multple of logical_block_size");
> 
> Again.
> 
> > +        return false;
> > +    }
> > +
> > +    return true;
> 
> Usually we return true for error, isn't it?

I expect int functions to return 0 for success and -errno for failure,
but bool functions to return true for success and false for failure.
I'm not sure if this varies across the code base, but it is the general
pattern in the block subsystem at least.

I agree with your comments about QEMU_IS_ALIGNED() (both for min_io_size
and opt_io_size) and the typos, though.

Kevin

Re: [PATCH v4 2/3] block: consolidate blocksize properties consistency checks

[Kevin Wolf]

Am 20.05.2020 um 10:59 hat Philippe Mathieu-Daudé geschrieben:
> On Wed, May 20, 2020 at 10:57 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > Hi Roman,
> >
> > On 5/20/20 10:06 AM, Roman Kagan wrote:
> > > Several block device properties related to blocksize configuration must
> > > be in certain relationship WRT each other: physical block must be no
> > > smaller than logical block; min_io_size, opt_io_size, and
> > > discard_granularity must be a multiple of a logical block.
> > >
> > > To ensure these requirements are met, add corresponding consistency
> > > checks to blkconf_blocksizes, adjusting its signature to communicate
> > > possible error to the caller.  Also remove the now redundant consistency
> > > checks from the specific devices.
> > >
> > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > > ---
> > > v4: new patch
> > >
> > >   include/hw/block/block.h   |  2 +-
> > >   hw/block/block.c           | 29 ++++++++++++++++++++++++++++-
> > >   hw/block/fdc.c             |  5 ++++-
> > >   hw/block/nvme.c            |  5 ++++-
> > >   hw/block/virtio-blk.c      |  7 +------
> > >   hw/ide/qdev.c              |  5 ++++-
> > >   hw/scsi/scsi-disk.c        | 10 +++-------
> > >   hw/usb/dev-storage.c       |  5 ++++-
> > >   tests/qemu-iotests/172.out |  2 +-
> > >   9 files changed, 50 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > > index d7246f3862..784953a237 100644
> > > --- a/include/hw/block/block.h
> > > +++ b/include/hw/block/block.h
> > > @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
> > >   bool blkconf_geometry(BlockConf *conf, int *trans,
> > >                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
> > >                         Error **errp);
> > > -void blkconf_blocksizes(BlockConf *conf);
> > > +bool blkconf_blocksizes(BlockConf *conf, Error **errp);
> 
> Maybe rename blkconf_validate_blocksizes()?

It does more than validating. It also assigns defaults or guesses
settings from the host file.

Kevin

Re: [PATCH v4 3/3] block: make BlockConf.*_size properties 32-bit

[Kevin Wolf]

Am 20.05.2020 um 10:06 hat Roman Kagan geschrieben:
> Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
> 32-bit for logical_block_size, physical_block_size, and min_io_size.
> However, the properties in BlockConf are defined as uint16_t limiting
> the values to 32768.
> 
> This appears unnecessary tight, and we've seen bigger block sizes handy
> at times.
> 
> Make them 32 bit instead and lift the limitation up to 2 MiB which
> appears to be good enough for everybody, and matches the qcow2 cluster
> size limit.
> 
> As the values can now be fairly big and awkward to type, make the
> property setter accept common size suffixes (k, m).
> 
> Also as the devices which use min_io_size (virtio-blk and scsi) pass its
> value to the guest in units of logical blocks in a 16bit field, to
> prevent its silent truncation add a corresponding check to
> blkconf_blocksizes.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> ---
> v3 -> v4:
> - check min_io_size against truncation [Kevin]
> 
> v2 -> v3:
> - mention qcow2 cluster size limit in the log and comment [Eric]
> 
> v1 -> v2:
> - cap the property at 2 MiB [Eric]
> - accept size suffixes
> 
>  include/hw/block/block.h     |  8 ++++----
>  include/hw/qdev-properties.h |  2 +-
>  hw/block/block.c             | 11 +++++++++++
>  hw/core/qdev-properties.c    | 34 ++++++++++++++++++++++++----------
>  4 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 784953a237..2fa09aa0b1 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -18,9 +18,9 @@
>  
>  typedef struct BlockConf {
>      BlockBackend *blk;
> -    uint16_t physical_block_size;
> -    uint16_t logical_block_size;
> -    uint16_t min_io_size;
> +    uint32_t physical_block_size;
> +    uint32_t logical_block_size;
> +    uint32_t min_io_size;
>      uint32_t opt_io_size;
>      int32_t bootindex;
>      uint32_t discard_granularity;
> @@ -51,7 +51,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>                            _conf.logical_block_size),                    \
>      DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
>                            _conf.physical_block_size),                   \
> -    DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),    \
> +    DEFINE_PROP_UINT32("min_io_size", _state, _conf.min_io_size, 0),    \
>      DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
>      DEFINE_PROP_UINT32("discard_granularity", _state,                   \
>                         _conf.discard_granularity, -1),                  \
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index f161604fb6..f9e0f8c041 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -197,7 +197,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>  #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
>  #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
>  #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
>  #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
> diff --git a/hw/block/block.c b/hw/block/block.c
> index 5f8ebff59c..cd95e7e38f 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -96,6 +96,17 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
>          return false;
>      }
>  
> +    /*
> +     * all devices which support min_io_size (scsi and virtio-blk) expose it to
> +     * the guest as a uint16_t in units of logical blocks
> +     */
> +    if ((conf->min_io_size / conf->logical_block_size) > UINT16_MAX) {
> +        error_setg(errp,
> +                   "min_io_size must be no more than " stringify(UINT16_MAX)
> +                   " of logical_block_size");

I'm not a native speaker, but "no more than 65536 of logical_block_size"
sounds odd to me. Maybe "65536 times the logical_block_size"?

> +        return false;
> +    }
> +
>      if (conf->opt_io_size % conf->logical_block_size) {
>          error_setg(errp,
>                     "opt_io_size must be a multple of logical_block_size");

Kevin

Re: [PATCH v4 1/3] virtio-blk: store opt_io_size with correct size

[Roman Kagan]

On Wed, May 20, 2020 at 05:36:58PM +0200, Kevin Wolf wrote:
> Am 20.05.2020 um 10:06 hat Roman Kagan geschrieben:
> > The width of opt_io_size in virtio_blk_topology is 32bit.
> 
> I think you mean virtio_blk_config?

I consulted virtio-v1.1 spec where the topology-related fields are
grouped in struct virtio_blk_topology; I didn't realize in Linux (and
therefore in QEMU) these fields lay directly on virtio_blk_config.  So
yes, the log should read "virtio_blk_config", thanks for spotting!

> > Use the appropriate accessor to store it.
> > 
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 

Thanks,
Roman.

Re: [PATCH v4 1/3] virtio-blk: store opt_io_size with correct size

[Roman Kagan]

On Wed, May 20, 2020 at 06:44:44AM -0400, Michael S. Tsirkin wrote:
> On Wed, May 20, 2020 at 11:06:55AM +0300, Roman Kagan wrote:
> > The width of opt_io_size in virtio_blk_topology is 32bit.
> > 
> > Use the appropriate accessor to store it.
> > 
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> 
> 
> Thanks for the patch!
> Could you add a bit of analysis - when does this cause
> bugs? I'm guessing on BE systems with legacy virtio, right?

I guess so too.  It was found just by eye inspection, trying to figure
out the potential truncation of opt_io_size in virtio-blk and why it's
different from scsi.  I don't have any analysis to add :(

> Also, should we convert virtio_stw_p and friends to get the
> pointer to the correct value type, as opposed to void *?

I dunno.  I guess they were designed to be used with untyped buffers and
modeled after virtio_{st,ld}*_phys.  The same question applies to the
underlying {st,ld}_{b,l}e_p.

> This will catch bugs like this ...

I'll try and see if this change doesn't cause too much churn / pain.
But I suggest to decouple it from the simple patch at hand.

Thanks,
Roman.

> > ---
> > v4: new patch
> > 
> >  hw/block/virtio-blk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index f5f6fc925e..413083e62f 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -918,7 +918,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
> >      virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
> >      virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
> >      virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> > -    virtio_stw_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
> > +    virtio_stl_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
> >      blkcfg.geometry.heads = conf->heads;
> >      /*
> >       * We must ensure that the block device capacity is a multiple of
> > -- 
> > 2.26.2
> 

Re: [PATCH v4 2/3] block: consolidate blocksize properties consistency checks

[Roman Kagan]

On Wed, May 20, 2020 at 10:57:00AM +0200, Philippe Mathieu-Daudé wrote:
> On 5/20/20 10:06 AM, Roman Kagan wrote:
> > Several block device properties related to blocksize configuration must
> > be in certain relationship WRT each other: physical block must be no
> > smaller than logical block; min_io_size, opt_io_size, and
> > discard_granularity must be a multiple of a logical block.
> > 
> > To ensure these requirements are met, add corresponding consistency
> > checks to blkconf_blocksizes, adjusting its signature to communicate
> > possible error to the caller.  Also remove the now redundant consistency
> > checks from the specific devices.
> > 
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > ---
> > v4: new patch
> > 
> >   include/hw/block/block.h   |  2 +-
> >   hw/block/block.c           | 29 ++++++++++++++++++++++++++++-
> >   hw/block/fdc.c             |  5 ++++-
> >   hw/block/nvme.c            |  5 ++++-
> >   hw/block/virtio-blk.c      |  7 +------
> >   hw/ide/qdev.c              |  5 ++++-
> >   hw/scsi/scsi-disk.c        | 10 +++-------
> >   hw/usb/dev-storage.c       |  5 ++++-
> >   tests/qemu-iotests/172.out |  2 +-
> >   9 files changed, 50 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index d7246f3862..784953a237 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
> >   bool blkconf_geometry(BlockConf *conf, int *trans,
> >                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
> >                         Error **errp);
> > -void blkconf_blocksizes(BlockConf *conf);
> > +bool blkconf_blocksizes(BlockConf *conf, Error **errp);
> >   bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> >                                      bool resizable, Error **errp);
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index bf56c7612b..5f8ebff59c 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
> >       return true;
> >   }
> > -void blkconf_blocksizes(BlockConf *conf)
> > +bool blkconf_blocksizes(BlockConf *conf, Error **errp)
> >   {
> >       BlockBackend *blk = conf->blk;
> >       BlockSizes blocksizes;
> > @@ -83,6 +83,33 @@ void blkconf_blocksizes(BlockConf *conf)
> >               conf->logical_block_size = BDRV_SECTOR_SIZE;
> >           }
> >       }
> > +
> > +    if (conf->logical_block_size > conf->physical_block_size) {
> > +        error_setg(errp,
> > +                   "logical_block_size > physical_block_size not supported");
> 
> "not supported" or "invalid"?
> 
> > +        return false;
> > +    }
> > +
> > +    if (conf->min_io_size % conf->logical_block_size) {
> 
> It seems the block code usually do:
> 
>        if (!QEMU_IS_ALIGNED(conf->min_io_size, conf->logical_block_size)) {
> 
> > +        error_setg(errp,
> > +                   "min_io_size must be a multple of logical_block_size");
> 
> Typo "multple" -> "multiple".
> 
> > +        return false;
> > +    }
> > +
> > +    if (conf->opt_io_size % conf->logical_block_size) {
> > +        error_setg(errp,
> > +                   "opt_io_size must be a multple of logical_block_size");
> 
> Ditto.
> 
> > +        return false;
> > +    }
> > +
> > +    if (conf->discard_granularity != -1 &&
> > +        conf->discard_granularity % conf->logical_block_size) {
> > +        error_setg(errp, "discard_granularity must be "
> > +                   "a multple of logical_block_size");
> 
> Again.
> 
> > +        return false;
> > +    }
> > +
> > +    return true;
> 
> Usually we return true for error, isn't it?

I just followed the convention of all other functions with error
handling in this file.

> >   }
> >   bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index c5fb9d6ece..8eda572ef4 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -554,7 +554,10 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
> >           read_only = !blk_bs(dev->conf.blk) || blk_is_read_only(dev->conf.blk);
> >       }
> > -    blkconf_blocksizes(&dev->conf);
> > +    if (!blkconf_blocksizes(&dev->conf, errp)) {
> > +        return;
> > +    }
> > +
> >       if (dev->conf.logical_block_size != 512 ||
> >           dev->conf.physical_block_size != 512)
> >       {
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 2f3100e56c..672650e162 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1390,7 +1390,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >           host_memory_backend_set_mapped(n->pmrdev, true);
> >       }
> > -    blkconf_blocksizes(&n->conf);
> > +    if (!blkconf_blocksizes(&n->conf, errp)) {
> > +        return;
> > +    }
> > +
> >       if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
> >                                          false, errp)) {
> >           return;
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 413083e62f..4ffdb130be 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -1162,12 +1162,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
> >           return;
> >       }
> > -    blkconf_blocksizes(&conf->conf);
> > -
> > -    if (conf->conf.logical_block_size >
> > -        conf->conf.physical_block_size) {
> > -        error_setg(errp,
> > -                   "logical_block_size > physical_block_size not supported");
> 
> Ah, "not supported" comes from here, OK.

Indeed :)

> 
> > +    if (!blkconf_blocksizes(&conf->conf, errp)) {
> >           return;
> >       }
> > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> > index 06b11583f5..b4821b2403 100644
> > --- a/hw/ide/qdev.c
> > +++ b/hw/ide/qdev.c
> > @@ -187,7 +187,10 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
> >           return;
> >       }
> > -    blkconf_blocksizes(&dev->conf);
> > +    if (!blkconf_blocksizes(&dev->conf, errp)) {
> > +        return;
> > +    }
> > +
> >       if (dev->conf.logical_block_size != 512) {
> >           error_setg(errp, "logical_block_size must be 512 for IDE");
> >           return;
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> > index 387503e11b..06c8f1ba92 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -2346,12 +2346,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
> >           return;
> >       }
> > -    blkconf_blocksizes(&s->qdev.conf);
> > -
> > -    if (s->qdev.conf.logical_block_size >
> > -        s->qdev.conf.physical_block_size) {
> > -        error_setg(errp,
> > -                   "logical_block_size > physical_block_size not supported");
> > +    if (!blkconf_blocksizes(&s->qdev.conf, errp)) {
> >           return;
> >       }
> > @@ -2436,7 +2431,8 @@ static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
> >       if (s->qdev.conf.blk) {
> >           ctx = blk_get_aio_context(s->qdev.conf.blk);
> >           aio_context_acquire(ctx);
> > -        blkconf_blocksizes(&s->qdev.conf);
> > +        /* ignore errors, blocksizes will be revalidated in scsi_realize */
> > +        blkconf_blocksizes(&s->qdev.conf, NULL);
> 
> Hmm, why not bail out ASAP on error?

You're probably right, it'll be cleaner this way.

Thanks,
Roman.

> 
> Good cleanup otherwise.
> 
> >       }
> >       s->qdev.blocksize = s->qdev.conf.logical_block_size;
> >       s->qdev.type = TYPE_DISK;
> > diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> > index 4eba47538d..de461f37bd 100644
> > --- a/hw/usb/dev-storage.c
> > +++ b/hw/usb/dev-storage.c
> > @@ -599,7 +599,10 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
> >           return;
> >       }
> > -    blkconf_blocksizes(&s->conf);
> > +    if (!blkconf_blocksizes(&s->conf, errp)) {
> > +        return;
> > +    }
> > +
> >       if (!blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true,
> >                                          errp)) {
> >           return;
> > diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
> > index 7abbe82427..59cc70aebb 100644
> > --- a/tests/qemu-iotests/172.out
> > +++ b/tests/qemu-iotests/172.out
> > @@ -1204,7 +1204,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica
> >                   drive-type = "144"
> >   Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical_block_size=4096
> > -QEMU_PROG: -device floppy,drive=none0,logical_block_size=4096: Physical and logical block size must be 512 for floppy
> > +QEMU_PROG: -device floppy,drive=none0,logical_block_size=4096: logical_block_size > physical_block_size not supported
> >   Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physical_block_size=1024
> >   QEMU_PROG: -device floppy,drive=none0,physical_block_size=1024: Physical and logical block size must be 512 for floppy
> > 
> 

Re: [PATCH v4 3/3] block: make BlockConf.*_size properties 32-bit

[Roman Kagan]

On Wed, May 20, 2020 at 11:04:44AM +0200, Philippe Mathieu-Daudé wrote:
> On 5/20/20 10:06 AM, Roman Kagan wrote:
> > Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
> > 32-bit for logical_block_size, physical_block_size, and min_io_size.
> > However, the properties in BlockConf are defined as uint16_t limiting
> > the values to 32768.
> > 
> > This appears unnecessary tight, and we've seen bigger block sizes handy
> > at times.
> > 
> > Make them 32 bit instead and lift the limitation up to 2 MiB which
> > appears to be good enough for everybody, and matches the qcow2 cluster
> > size limit.
> > 
> > As the values can now be fairly big and awkward to type, make the
> > property setter accept common size suffixes (k, m).
> > 
> > Also as the devices which use min_io_size (virtio-blk and scsi) pass its
> > value to the guest in units of logical blocks in a 16bit field, to
> > prevent its silent truncation add a corresponding check to
> > blkconf_blocksizes.
> > 
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > ---
> > v3 -> v4:
> > - check min_io_size against truncation [Kevin]
> > 
> > v2 -> v3:
> > - mention qcow2 cluster size limit in the log and comment [Eric]
> > 
> > v1 -> v2:
> > - cap the property at 2 MiB [Eric]
> > - accept size suffixes
> > 
> >   include/hw/block/block.h     |  8 ++++----
> >   include/hw/qdev-properties.h |  2 +-
> >   hw/block/block.c             | 11 +++++++++++
> >   hw/core/qdev-properties.c    | 34 ++++++++++++++++++++++++----------
> >   4 files changed, 40 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index 784953a237..2fa09aa0b1 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -18,9 +18,9 @@
> >   typedef struct BlockConf {
> >       BlockBackend *blk;
> > -    uint16_t physical_block_size;
> > -    uint16_t logical_block_size;
> > -    uint16_t min_io_size;
> > +    uint32_t physical_block_size;
> > +    uint32_t logical_block_size;
> > +    uint32_t min_io_size;
> >       uint32_t opt_io_size;
> >       int32_t bootindex;
> >       uint32_t discard_granularity;
> > @@ -51,7 +51,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
> >                             _conf.logical_block_size),                    \
> >       DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
> >                             _conf.physical_block_size),                   \
> > -    DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),    \
> > +    DEFINE_PROP_UINT32("min_io_size", _state, _conf.min_io_size, 0),    \
> >       DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
> >       DEFINE_PROP_UINT32("discard_granularity", _state,                   \
> >                          _conf.discard_granularity, -1),                  \
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index f161604fb6..f9e0f8c041 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -197,7 +197,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
> >   #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
> >       DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
> >   #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> > -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> > +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
> >   #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
> >       DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
> >   #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index 5f8ebff59c..cd95e7e38f 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -96,6 +96,17 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
> >           return false;
> >       }
> > +    /*
> > +     * all devices which support min_io_size (scsi and virtio-blk) expose it to
> > +     * the guest as a uint16_t in units of logical blocks
> > +     */
> > +    if ((conf->min_io_size / conf->logical_block_size) > UINT16_MAX) {
> > +        error_setg(errp,
> > +                   "min_io_size must be no more than " stringify(UINT16_MAX)
> > +                   " of logical_block_size");
> > +        return false;
> > +    }
> > +
> >       if (conf->opt_io_size % conf->logical_block_size) {
> >           error_setg(errp,
> >                      "opt_io_size must be a multple of logical_block_size");
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index cc924815da..fd03cc7597 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -14,6 +14,7 @@
> >   #include "qapi/visitor.h"
> >   #include "chardev/char.h"
> >   #include "qemu/uuid.h"
> > +#include "qemu/units.h"
> >   void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
> >                                     Error **errp)
> > @@ -729,30 +730,42 @@ const PropertyInfo qdev_prop_pci_devfn = {
> >   /* --- blocksize --- */
> > +/* lower limit is sector size */
> > +#define MIN_BLOCK_SIZE          512
> > +#define MIN_BLOCK_SIZE_STR      "512 B"
> > +/*
> > + * upper limit is arbitrary, 2 MiB looks sufficient for all sensible uses, and
> > + * matches qcow2 cluster size limit
> > + */
> > +#define MAX_BLOCK_SIZE          (2 * MiB)
> 
> Can you split this patch?
> 
> - add/use definitions (here MAX_BLOCK_SIZE = 32 * KiB)
> - use 32-bit
> - raise limit to 2MB

I can see the value of splitting out the rework of the blocksize
property, but I'm struggling to justify expanding the fields without
increasing the limits.  Do you mind if I do it in two patches rather
than three?

Thanks,
Roman.

> 
> Feel free to refuse, but having atomic changes is better for bisectability
> and cherry-picking/reverting.
> 
> Thanks,
> 
> Phil.
> 
> > +#define MAX_BLOCK_SIZE_STR      "2 MiB"
> > +
> >   static void set_blocksize(Object *obj, Visitor *v, const char *name,
> >                             void *opaque, Error **errp)
> >   {
> >       DeviceState *dev = DEVICE(obj);
> >       Property *prop = opaque;
> > -    uint16_t value, *ptr = qdev_get_prop_ptr(dev, prop);
> > +    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> > +    uint64_t value;
> >       Error *local_err = NULL;
> > -    const int64_t min = 512;
> > -    const int64_t max = 32768;
> >       if (dev->realized) {
> >           qdev_prop_set_after_realize(dev, name, errp);
> >           return;
> >       }
> > -    visit_type_uint16(v, name, &value, &local_err);
> > +    visit_type_size(v, name, &value, &local_err);
> >       if (local_err) {
> >           error_propagate(errp, local_err);
> >           return;
> >       }
> >       /* value of 0 means "unset" */
> > -    if (value && (value < min || value > max)) {
> > -        error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> > -                   dev->id ? : "", name, (int64_t)value, min, max);
> > +    if (value && (value < MIN_BLOCK_SIZE || value > MAX_BLOCK_SIZE)) {
> > +        error_setg(errp,
> > +                   "Property %s.%s doesn't take value %" PRIu64
> > +                   " (minimum: " MIN_BLOCK_SIZE_STR
> > +                   ", maximum: " MAX_BLOCK_SIZE_STR ")",
> > +                   dev->id ? : "", name, value);
> >           return;
> >       }
> > @@ -768,9 +781,10 @@ static void set_blocksize(Object *obj, Visitor *v, const char *name,
> >   }
> >   const PropertyInfo qdev_prop_blocksize = {
> > -    .name  = "uint16",
> > -    .description = "A power of two between 512 and 32768",
> > -    .get   = get_uint16,
> > +    .name  = "size",
> > +    .description = "A power of two between " MIN_BLOCK_SIZE_STR
> > +                   " and " MAX_BLOCK_SIZE_STR,
> > +    .get   = get_uint32,
> >       .set   = set_blocksize,
> >       .set_default_value = set_default_value_uint,
> >   };
> > 
> 

Re: [PATCH v4 3/3] block: make BlockConf.*_size properties 32-bit

[Roman Kagan]

On Wed, May 20, 2020 at 05:54:44PM +0200, Kevin Wolf wrote:
> Am 20.05.2020 um 10:06 hat Roman Kagan geschrieben:
> > Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
> > 32-bit for logical_block_size, physical_block_size, and min_io_size.
> > However, the properties in BlockConf are defined as uint16_t limiting
> > the values to 32768.
> > 
> > This appears unnecessary tight, and we've seen bigger block sizes handy
> > at times.
> > 
> > Make them 32 bit instead and lift the limitation up to 2 MiB which
> > appears to be good enough for everybody, and matches the qcow2 cluster
> > size limit.
> > 
> > As the values can now be fairly big and awkward to type, make the
> > property setter accept common size suffixes (k, m).
> > 
> > Also as the devices which use min_io_size (virtio-blk and scsi) pass its
> > value to the guest in units of logical blocks in a 16bit field, to
> > prevent its silent truncation add a corresponding check to
> > blkconf_blocksizes.
> > 
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > ---
> > v3 -> v4:
> > - check min_io_size against truncation [Kevin]
> > 
> > v2 -> v3:
> > - mention qcow2 cluster size limit in the log and comment [Eric]
> > 
> > v1 -> v2:
> > - cap the property at 2 MiB [Eric]
> > - accept size suffixes
> > 
> >  include/hw/block/block.h     |  8 ++++----
> >  include/hw/qdev-properties.h |  2 +-
> >  hw/block/block.c             | 11 +++++++++++
> >  hw/core/qdev-properties.c    | 34 ++++++++++++++++++++++++----------
> >  4 files changed, 40 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index 784953a237..2fa09aa0b1 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -18,9 +18,9 @@
> >  
> >  typedef struct BlockConf {
> >      BlockBackend *blk;
> > -    uint16_t physical_block_size;
> > -    uint16_t logical_block_size;
> > -    uint16_t min_io_size;
> > +    uint32_t physical_block_size;
> > +    uint32_t logical_block_size;
> > +    uint32_t min_io_size;
> >      uint32_t opt_io_size;
> >      int32_t bootindex;
> >      uint32_t discard_granularity;
> > @@ -51,7 +51,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
> >                            _conf.logical_block_size),                    \
> >      DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
> >                            _conf.physical_block_size),                   \
> > -    DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),    \
> > +    DEFINE_PROP_UINT32("min_io_size", _state, _conf.min_io_size, 0),    \
> >      DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
> >      DEFINE_PROP_UINT32("discard_granularity", _state,                   \
> >                         _conf.discard_granularity, -1),                  \
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index f161604fb6..f9e0f8c041 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -197,7 +197,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
> >  #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
> >      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
> >  #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> > -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> > +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
> >  #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
> >      DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
> >  #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index 5f8ebff59c..cd95e7e38f 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -96,6 +96,17 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
> >          return false;
> >      }
> >  
> > +    /*
> > +     * all devices which support min_io_size (scsi and virtio-blk) expose it to
> > +     * the guest as a uint16_t in units of logical blocks
> > +     */
> > +    if ((conf->min_io_size / conf->logical_block_size) > UINT16_MAX) {
> > +        error_setg(errp,
> > +                   "min_io_size must be no more than " stringify(UINT16_MAX)
> > +                   " of logical_block_size");
> 
> I'm not a native speaker, but "no more than 65536 of
> logical_block_size" sounds odd to me.

Neither am I but I agree with the feeling.

> Maybe "65536 times the logical_block_size"?

Sounds better indeed, will do in the respin.
Or perhaps "no more than 65536 logical blocks"?

Thanks,
Roman.

> 
> > +        return false;
> > +    }
> > +
> >      if (conf->opt_io_size % conf->logical_block_size) {
> >          error_setg(errp,
> >                     "opt_io_size must be a multple of logical_block_size");
> 
> Kevin
> 

Re: [PATCH v4 3/3] block: make BlockConf.*_size properties 32-bit

[Kevin Wolf]

Am 20.05.2020 um 23:50 hat Roman Kagan geschrieben:
> On Wed, May 20, 2020 at 05:54:44PM +0200, Kevin Wolf wrote:
> > Am 20.05.2020 um 10:06 hat Roman Kagan geschrieben:
> > > Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
> > > 32-bit for logical_block_size, physical_block_size, and min_io_size.
> > > However, the properties in BlockConf are defined as uint16_t limiting
> > > the values to 32768.
> > > 
> > > This appears unnecessary tight, and we've seen bigger block sizes handy
> > > at times.
> > > 
> > > Make them 32 bit instead and lift the limitation up to 2 MiB which
> > > appears to be good enough for everybody, and matches the qcow2 cluster
> > > size limit.
> > > 
> > > As the values can now be fairly big and awkward to type, make the
> > > property setter accept common size suffixes (k, m).
> > > 
> > > Also as the devices which use min_io_size (virtio-blk and scsi) pass its
> > > value to the guest in units of logical blocks in a 16bit field, to
> > > prevent its silent truncation add a corresponding check to
> > > blkconf_blocksizes.
> > > 
> > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > > ---
> > > v3 -> v4:
> > > - check min_io_size against truncation [Kevin]
> > > 
> > > v2 -> v3:
> > > - mention qcow2 cluster size limit in the log and comment [Eric]
> > > 
> > > v1 -> v2:
> > > - cap the property at 2 MiB [Eric]
> > > - accept size suffixes
> > > 
> > >  include/hw/block/block.h     |  8 ++++----
> > >  include/hw/qdev-properties.h |  2 +-
> > >  hw/block/block.c             | 11 +++++++++++
> > >  hw/core/qdev-properties.c    | 34 ++++++++++++++++++++++++----------
> > >  4 files changed, 40 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > > index 784953a237..2fa09aa0b1 100644
> > > --- a/include/hw/block/block.h
> > > +++ b/include/hw/block/block.h
> > > @@ -18,9 +18,9 @@
> > >  
> > >  typedef struct BlockConf {
> > >      BlockBackend *blk;
> > > -    uint16_t physical_block_size;
> > > -    uint16_t logical_block_size;
> > > -    uint16_t min_io_size;
> > > +    uint32_t physical_block_size;
> > > +    uint32_t logical_block_size;
> > > +    uint32_t min_io_size;
> > >      uint32_t opt_io_size;
> > >      int32_t bootindex;
> > >      uint32_t discard_granularity;
> > > @@ -51,7 +51,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
> > >                            _conf.logical_block_size),                    \
> > >      DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
> > >                            _conf.physical_block_size),                   \
> > > -    DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),    \
> > > +    DEFINE_PROP_UINT32("min_io_size", _state, _conf.min_io_size, 0),    \
> > >      DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
> > >      DEFINE_PROP_UINT32("discard_granularity", _state,                   \
> > >                         _conf.discard_granularity, -1),                  \
> > > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > > index f161604fb6..f9e0f8c041 100644
> > > --- a/include/hw/qdev-properties.h
> > > +++ b/include/hw/qdev-properties.h
> > > @@ -197,7 +197,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
> > >  #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
> > >      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
> > >  #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> > > -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> > > +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
> > >  #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
> > >      DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
> > >  #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
> > > diff --git a/hw/block/block.c b/hw/block/block.c
> > > index 5f8ebff59c..cd95e7e38f 100644
> > > --- a/hw/block/block.c
> > > +++ b/hw/block/block.c
> > > @@ -96,6 +96,17 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
> > >          return false;
> > >      }
> > >  
> > > +    /*
> > > +     * all devices which support min_io_size (scsi and virtio-blk) expose it to
> > > +     * the guest as a uint16_t in units of logical blocks
> > > +     */
> > > +    if ((conf->min_io_size / conf->logical_block_size) > UINT16_MAX) {
> > > +        error_setg(errp,
> > > +                   "min_io_size must be no more than " stringify(UINT16_MAX)
> > > +                   " of logical_block_size");
> > 
> > I'm not a native speaker, but "no more than 65536 of
> > logical_block_size" sounds odd to me.
> 
> Neither am I but I agree with the feeling.
> 
> > Maybe "65536 times the logical_block_size"?
> 
> Sounds better indeed, will do in the respin.
> Or perhaps "no more than 65536 logical blocks"?

I guess that would work, too, and result in a shorter error message,
though logical_block_size is the option that the user gave, so using
that name wouldn't hurt either.

Matter of taste, I'd say.

Kevin