diff mbox series

[v7,6/8] block: make BlockConf size props 32bit and accept size suffixes

Message ID 20200528213946.1636444-7-rvkagan@yandex-team.ru (mailing list archive)
State New, archived
Headers show
Series block: enhance handling of size-related BlockConf properties | expand

Commit Message

Roman Kagan May 28, 2020, 9:39 p.m. UTC 
Convert all size-related properties in BlockConf to 32bit.  This will
allow to accomodate bigger block sizes (in a followup patch).
This also allows to make them all accept size suffixes, either via
DEFINE_PROP_BLOCKSIZE or via DEFINE_PROP_SIZE32.

Also, since min_io_size is exposed to the guest by scsi and virtio-blk
devices as an uint16_t in units of logical blocks, introduce an
additional check in blkconf_blocksizes to prevent its silent truncation.

Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
---
v6 -> v7:
- split out into separate patch [Eric]
- avoid overflow in min_io_size check [Eric]

 include/hw/block/block.h     | 12 ++++++------
 include/hw/qdev-properties.h |  2 +-
 hw/block/block.c             | 11 +++++++++++
 hw/core/qdev-properties.c    |  4 ++--
 4 files changed, 20 insertions(+), 9 deletions(-)

Comments

Eric Blake May 28, 2020, 9:53 p.m. UTC | #1 
On 5/28/20 4:39 PM, Roman Kagan wrote:
> Convert all size-related properties in BlockConf to 32bit.  This will
> allow to accomodate bigger block sizes (in a followup patch).

s/allow to accomodate/accommodate/

> This also allows to make them all accept size suffixes, either via
> DEFINE_PROP_BLOCKSIZE or via DEFINE_PROP_SIZE32.
> 
> Also, since min_io_size is exposed to the guest by scsi and virtio-blk
> devices as an uint16_t in units of logical blocks, introduce an
> additional check in blkconf_blocksizes to prevent its silent truncation.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> ---

> +    if (conf->min_io_size / conf->logical_block_size > UINT16_MAX) {
> +        error_setg(errp,
> +                   "min_io_size must not exceed " stringify(UINT16_MAX)
> +                   " logical blocks");

On my libc, this results in "must not exceed (65535) logical blocks".

Worse, I could envision a platform where it prints something funky like:

"exceed (2 * (32768) + 1) logical", based on however complex the 
definition of UINT16_MAX is.  You're better off printing this one with 
%d than with stringify().
diff mbox series

Patch

diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 784953a237..1e8b6253dd 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,9 +51,9 @@  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("opt_io_size", _state, _conf.opt_io_size, 0),    \
-    DEFINE_PROP_UINT32("discard_granularity", _state,                   \
+    DEFINE_PROP_SIZE32("min_io_size", _state, _conf.min_io_size, 0),    \
+    DEFINE_PROP_SIZE32("opt_io_size", _state, _conf.opt_io_size, 0),    \
+    DEFINE_PROP_SIZE32("discard_granularity", _state,                   \
                        _conf.discard_granularity, -1),                  \
     DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce,           \
                             ON_OFF_AUTO_AUTO),                          \
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index c03eadfad6..5252bb6b1a 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -200,7 +200,7 @@  extern const PropertyInfo qdev_prop_pcie_link_width;
 #define DEFINE_PROP_SIZE32(_n, _s, _f, _d)                       \
     DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_size32, uint32_t)
 #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 b22207c921..7410b24dee 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 not exceed " stringify(UINT16_MAX)
+                   " logical blocks");
+        return false;
+    }
+
     if (!QEMU_IS_ALIGNED(conf->opt_io_size, conf->logical_block_size)) {
         error_setg(errp,
                    "opt_io_size must be a multiple of logical_block_size");
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index a79062b428..3cbe3f56a8 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -782,7 +782,7 @@  static void set_blocksize(Object *obj, Visitor *v, const char *name,
 {
     DeviceState *dev = DEVICE(obj);
     Property *prop = opaque;
-    uint16_t *ptr = qdev_get_prop_ptr(dev, prop);
+    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
     uint64_t value;
     Error *local_err = NULL;
 
@@ -821,7 +821,7 @@  const PropertyInfo qdev_prop_blocksize = {
     .name  = "size",
     .description = "A power of two between " MIN_BLOCK_SIZE_STR
                    " and " MAX_BLOCK_SIZE_STR,
-    .get   = get_uint16,
+    .get   = get_uint32,
     .set   = set_blocksize,
     .set_default_value = set_default_value_uint,
 };