| Message ID | 20200520080657.29080-4-rvkagan@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | block: make BlockConf.*_size properties 32-bit | expand |
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, > }; >
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
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, > > }; > > >
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 >
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
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, };
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(-)