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 <rvka...@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
