* [PATCH v3] block: make BlockConf.*_size properties 32-bit
@ 2020-04-29 9:18 Roman Kagan
2020-04-29 9:41 ` Philippe Mathieu-Daudé
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Roman Kagan @ 2020-04-29 9:18 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P. Berrangé,
Eduardo Habkost, qemu-block, Paolo Bonzini
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).
Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
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/core/qdev-properties.c | 34 ++++++++++++++++++++++++----------
3 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index d7246f3862..9dd6bba56a 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/core/qdev-properties.c b/hw/core/qdev-properties.c
index 2047114fca..e673f3c43f 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.25.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: make BlockConf.*_size properties 32-bit
2020-04-29 9:18 [PATCH v3] block: make BlockConf.*_size properties 32-bit Roman Kagan
@ 2020-04-29 9:41 ` Philippe Mathieu-Daudé
2020-04-29 12:19 ` Roman Kagan
2020-05-19 7:17 ` Roman Kagan
2020-05-19 14:08 ` Kevin Wolf
2 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-29 9:41 UTC (permalink / raw)
To: Roman Kagan, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
Eduardo Habkost, qemu-block, Michael S. Tsirkin",
Stefan Hajnoczi, Paolo Bonzini
Cc'ing virtio-blk and scsi maintainers.
On 4/29/20 11:18 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).
>
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> 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/core/qdev-properties.c | 34 ++++++++++++++++++++++++----------
> 3 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index d7246f3862..9dd6bba56a 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/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2047114fca..e673f3c43f 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,
> };
>
1/ Don't you need to update SCSIBlockLimits too?
2/ It seems hw/block/virtio-blk.c can get underflow now.
Maybe you miss this change:
-- >8 --
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -917,7 +917,7 @@ static void virtio_blk_update_config(VirtIODevice
*vdev, uint8_t *config)
s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128
- 2);
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_stl_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);
blkcfg.geometry.heads = conf->heads;
/*
---
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: make BlockConf.*_size properties 32-bit
2020-04-29 9:41 ` Philippe Mathieu-Daudé
@ 2020-04-29 12:19 ` Roman Kagan
2020-04-29 12:59 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: Roman Kagan @ 2020-04-29 12:19 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
Eduardo Habkost, qemu-block, Michael S. Tsirkin",
qemu-devel, Stefan Hajnoczi, Paolo Bonzini
On Wed, Apr 29, 2020 at 11:41:04AM +0200, Philippe Mathieu-Daudé wrote:
> Cc'ing virtio-blk and scsi maintainers.
>
> On 4/29/20 11:18 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).
> >
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> > 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/core/qdev-properties.c | 34 ++++++++++++++++++++++++----------
> > 3 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index d7246f3862..9dd6bba56a 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/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 2047114fca..e673f3c43f 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,
> > };
> >
>
> 1/ Don't you need to update SCSIBlockLimits too?
I guess you mean SCSIBlockLimits.min_io_size which is the only uint16_t
field there, do you?
> 2/ It seems hw/block/virtio-blk.c can get underflow now.
Both SCSIBlockLimits.min_io_size and virtio_blk_config.min_io_size are
expressed in logical blocks so there appears to be no problem here.
> Maybe you miss this change:
>
> -- >8 --
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -917,7 +917,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
> uint8_t *config)
> s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 -
> 2);
> 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_stl_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
The width of this field in the device's config space is defined in the
spec and can't be changed.
Nor is there any need due to this patch.
Thanks,
Roman.
> virtio_stw_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
> blkcfg.geometry.heads = conf->heads;
> /*
> ---
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: make BlockConf.*_size properties 32-bit
2020-04-29 12:19 ` Roman Kagan
@ 2020-04-29 12:59 ` Philippe Mathieu-Daudé
2020-04-30 9:25 ` Roman Kagan
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-29 12:59 UTC (permalink / raw)
To: Roman Kagan, qemu-devel, Kevin Wolf, Daniel P. Berrangé,
Eduardo Habkost, qemu-block, Paolo Bonzini,
Michael S. Tsirkin",
Stefan Hajnoczi, Fam Zheng
On 4/29/20 2:19 PM, Roman Kagan wrote:
> On Wed, Apr 29, 2020 at 11:41:04AM +0200, Philippe Mathieu-Daudé wrote:
>> Cc'ing virtio-blk and scsi maintainers.
>>
>> On 4/29/20 11:18 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).
>>>
>>> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>> 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/core/qdev-properties.c | 34 ++++++++++++++++++++++++----------
>>> 3 files changed, 29 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
>>> index d7246f3862..9dd6bba56a 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/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index 2047114fca..e673f3c43f 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,
>>> };
>>>
>>
>> 1/ Don't you need to update SCSIBlockLimits too?
>
> I guess you mean SCSIBlockLimits.min_io_size which is the only uint16_t
> field there, do you?
Yes.
>
>> 2/ It seems hw/block/virtio-blk.c can get underflow now.
>
> Both SCSIBlockLimits.min_io_size and virtio_blk_config.min_io_size are
> expressed in logical blocks so there appears to be no problem here.
>
>> Maybe you miss this change:
>>
>> -- >8 --
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -917,7 +917,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
>> uint8_t *config)
>> s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 -
>> 2);
>> 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_stl_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
>
> The width of this field in the device's config space is defined in the
> spec and can't be changed.
>
> Nor is there any need due to this patch.
OK, thanks :)
>
> Thanks,
> Roman.
>
>> virtio_stw_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
>> blkcfg.geometry.heads = conf->heads;
>> /*
>> ---
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: make BlockConf.*_size properties 32-bit
2020-04-29 12:59 ` Philippe Mathieu-Daudé
@ 2020-04-30 9:25 ` Roman Kagan
2020-04-30 9:56 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: Roman Kagan @ 2020-04-30 9:25 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
Eduardo Habkost, qemu-block, Michael S. Tsirkin",
qemu-devel, Stefan Hajnoczi, Paolo Bonzini
On Wed, Apr 29, 2020 at 02:59:31PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/29/20 2:19 PM, Roman Kagan wrote:
> > On Wed, Apr 29, 2020 at 11:41:04AM +0200, Philippe Mathieu-Daudé wrote:
> > > Cc'ing virtio-blk and scsi maintainers.
> > >
> > > On 4/29/20 11:18 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).
> > > >
> > > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > > > ---
> > > > 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/core/qdev-properties.c | 34 ++++++++++++++++++++++++----------
> > > > 3 files changed, 29 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > > > index d7246f3862..9dd6bba56a 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/core/qdev-properties.c b/hw/core/qdev-properties.c
> > > > index 2047114fca..e673f3c43f 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,
> > > > };
> > > >
> > >
> > > 1/ Don't you need to update SCSIBlockLimits too?
> >
> > I guess you mean SCSIBlockLimits.min_io_size which is the only uint16_t
> > field there, do you?
>
> Yes.
>
> >
> > > 2/ It seems hw/block/virtio-blk.c can get underflow now.
> >
> > Both SCSIBlockLimits.min_io_size and virtio_blk_config.min_io_size are
> > expressed in logical blocks so there appears to be no problem here.
> >
> > > Maybe you miss this change:
> > >
> > > -- >8 --
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -917,7 +917,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
> > > uint8_t *config)
> > > s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 -
> > > 2);
> > > 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_stl_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> >
> > The width of this field in the device's config space is defined in the
> > spec and can't be changed.
> >
> > Nor is there any need due to this patch.
>
> OK, thanks :)
May I count this as an r-b? :)
Thanks,
Roman.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: make BlockConf.*_size properties 32-bit
2020-04-30 9:25 ` Roman Kagan
@ 2020-04-30 9:56 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-30 9:56 UTC (permalink / raw)
To: Roman Kagan, qemu-devel, Kevin Wolf, Daniel P. Berrangé,
Eduardo Habkost, qemu-block, Paolo Bonzini,
Michael S. Tsirkin",
Stefan Hajnoczi, Fam Zheng
On 4/30/20 11:25 AM, Roman Kagan wrote:
> On Wed, Apr 29, 2020 at 02:59:31PM +0200, Philippe Mathieu-Daudé wrote:
>> On 4/29/20 2:19 PM, Roman Kagan wrote:
>>> On Wed, Apr 29, 2020 at 11:41:04AM +0200, Philippe Mathieu-Daudé wrote:
>>>> Cc'ing virtio-blk and scsi maintainers.
>>>>
>>>> On 4/29/20 11:18 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).
>>>>>
>>>>> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> ---
>>>>> 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/core/qdev-properties.c | 34 ++++++++++++++++++++++++----------
>>>>> 3 files changed, 29 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
>>>>> index d7246f3862..9dd6bba56a 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/core/qdev-properties.c b/hw/core/qdev-properties.c
>>>>> index 2047114fca..e673f3c43f 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,
>>>>> };
>>>>>
>>>>
>>>> 1/ Don't you need to update SCSIBlockLimits too?
>>>
>>> I guess you mean SCSIBlockLimits.min_io_size which is the only uint16_t
>>> field there, do you?
>>
>> Yes.
>>
>>>
>>>> 2/ It seems hw/block/virtio-blk.c can get underflow now.
>>>
>>> Both SCSIBlockLimits.min_io_size and virtio_blk_config.min_io_size are
>>> expressed in logical blocks so there appears to be no problem here.
>>>
>>>> Maybe you miss this change:
>>>>
>>>> -- >8 --
>>>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>>>> --- a/hw/block/virtio-blk.c
>>>> +++ b/hw/block/virtio-blk.c
>>>> @@ -917,7 +917,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
>>>> uint8_t *config)
>>>> s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 -
>>>> 2);
>>>> 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_stl_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
>>>
>>> The width of this field in the device's config space is defined in the
>>> spec and can't be changed.
>>>
>>> Nor is there any need due to this patch.
>>
>> OK, thanks :)
>
> May I count this as an r-b? :)
Unfortunately no, because my comments proved I don't know this code
enough :/
>
> Thanks,
> Roman.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: make BlockConf.*_size properties 32-bit
2020-04-29 9:18 [PATCH v3] block: make BlockConf.*_size properties 32-bit Roman Kagan
2020-04-29 9:41 ` Philippe Mathieu-Daudé
@ 2020-05-19 7:17 ` Roman Kagan
2020-05-19 14:08 ` Kevin Wolf
2 siblings, 0 replies; 9+ messages in thread
From: Roman Kagan @ 2020-05-19 7:17 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, qemu-block
On Wed, Apr 29, 2020 at 12:18:13PM +0300, 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).
>
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> 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/core/qdev-properties.c | 34 ++++++++++++++++++++++++----------
> 3 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index d7246f3862..9dd6bba56a 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/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2047114fca..e673f3c43f 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.25.4
Ping?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: make BlockConf.*_size properties 32-bit
2020-04-29 9:18 [PATCH v3] block: make BlockConf.*_size properties 32-bit Roman Kagan
2020-04-29 9:41 ` Philippe Mathieu-Daudé
2020-05-19 7:17 ` Roman Kagan
@ 2020-05-19 14:08 ` Kevin Wolf
2020-05-19 18:23 ` Roman Kagan
2 siblings, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2020-05-19 14:08 UTC (permalink / raw)
To: Roman Kagan
Cc: Daniel P. Berrangé,
Eduardo Habkost, qemu-block, qemu-devel, Paolo Bonzini
Am 29.04.2020 um 11:18 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).
>
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> 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/core/qdev-properties.c | 34 ++++++++++++++++++++++++----------
> 3 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index d7246f3862..9dd6bba56a 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), \
This one doesn't go through set_blocksize(), so does the 2 MB
limitation actually not apply to min_io_size?
Let's go back to the line that Philippe already quoted:
virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
This means that we'll get silent 16 bit truncation of the size specified
by the user. This is the new bug. In addition, like before, we also
silently round down to the next multiple of the logical block size.
Of course, opt_io_size already has the same problems. I wonder whether
both should be converted to DEFINE_PROP_BLOCKSIZE. In either case, I
think virtio_blk_device_realize() must check that both values are
plausible; even if we use a block size property, it still needs to
verify that they are a multiple of the block size.
The same thing is true of hw/scsi/scsi-disk.c.
All other devices with DEFINE_BLOCK_PROPERTIES accept a value, but
silently ignore it, so nothing changes for them (though it's also not
the perfect state).
> 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)
The 2 MB limit means that we won't get 16 bit truncation for anything
that is stored in sectors (512 * 64k = 32M).
As far as I can see, all devices can deal with this change then.
> #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/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2047114fca..e673f3c43f 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,
> };
This part looks good to me, too.
So I think we just need some additional checks for min_io_size (and
potentially opt_io_size while you're at it). Maybe that could be a
separate patch before this one.
Kevin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: make BlockConf.*_size properties 32-bit
2020-05-19 14:08 ` Kevin Wolf
@ 2020-05-19 18:23 ` Roman Kagan
0 siblings, 0 replies; 9+ messages in thread
From: Roman Kagan @ 2020-05-19 18:23 UTC (permalink / raw)
To: Kevin Wolf
Cc: Daniel P. Berrangé,
Eduardo Habkost, qemu-block, qemu-devel, Paolo Bonzini
On Tue, May 19, 2020 at 04:08:26PM +0200, Kevin Wolf wrote:
> Am 29.04.2020 um 11:18 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).
> >
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> > 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/core/qdev-properties.c | 34 ++++++++++++++++++++++++----------
> > 3 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index d7246f3862..9dd6bba56a 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), \
>
> This one doesn't go through set_blocksize(), so does the 2 MB
> limitation actually not apply to min_io_size?
>
> Let's go back to the line that Philippe already quoted:
>
> virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
>
> This means that we'll get silent 16 bit truncation of the size specified
> by the user. This is the new bug. In addition, like before, we also
> silently round down to the next multiple of the logical block size.
>
> Of course, opt_io_size already has the same problems. I wonder whether
> both should be converted to DEFINE_PROP_BLOCKSIZE.
DEFINE_PROP_BLOCKSIZE, in addition to capping the value at 2 MB, also
requires it to be a power of two. I'm not sure it's safe to assume
min_io_size and opt_io_size are ok with this.
> In either case, I
> think virtio_blk_device_realize() must check that both values are
> plausible; even if we use a block size property, it still needs to
> verify that they are a multiple of the block size.
>
> The same thing is true of hw/scsi/scsi-disk.c.
>
> All other devices with DEFINE_BLOCK_PROPERTIES accept a value, but
> silently ignore it, so nothing changes for them (though it's also not
> the perfect state).
>
> > 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)
>
> The 2 MB limit means that we won't get 16 bit truncation for anything
> that is stored in sectors (512 * 64k = 32M).
>
> As far as I can see, all devices can deal with this change then.
>
> > #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/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 2047114fca..e673f3c43f 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,
> > };
>
> This part looks good to me, too.
>
> So I think we just need some additional checks for min_io_size (and
> potentially opt_io_size while you're at it). Maybe that could be a
> separate patch before this one.
I must admit I didn't consider it as a practical issue; if it is, I'm
thinking of adding all those consistency checks to blkconf_blocksizes
(and nuking the open-coded ones from the respective realize functions).
I'll cook something up in the next iteration.
Thanks,
Roman.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-05-19 18:27 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-29 9:18 [PATCH v3] block: make BlockConf.*_size properties 32-bit Roman Kagan
2020-04-29 9:41 ` Philippe Mathieu-Daudé
2020-04-29 12:19 ` Roman Kagan
2020-04-29 12:59 ` Philippe Mathieu-Daudé
2020-04-30 9:25 ` Roman Kagan
2020-04-30 9:56 ` Philippe Mathieu-Daudé
2020-05-19 7:17 ` Roman Kagan
2020-05-19 14:08 ` Kevin Wolf
2020-05-19 18:23 ` Roman Kagan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.