Re: [Qemu-devel] [PATCH v2 2/3] qdev: add blocksize property type

[Paolo Bonzini]

Il 14/03/2012 16:57, Stefan Hajnoczi ha scritto:
> Storage interfaces like virtio-blk can be configured with block size
> information so that the guest can take advantage of efficient I/O
> request sizes.
> 
> According to the SCSI Block Commands (SBC) standard a device's block
> size is "almost always greater than one byte and may be a multiple of
> 512 bytes".  QEMU currently has a 512 byte minimum block size because
> the block layer functions work at that granularity.  Furthermore, the
> block size should be a power of 2 because QEMU calculates bitmasks from
> the value.
> 
> Introduce a "blocksize" property type so devices can enforce these
> constraints on block size values.  If the constraints are relaxed in the
> future then this property can be updated.
> 
> Introduce the new PropertyValueNotPowerOf2 QError so QMP clients know
> exactly why a block size value was rejected.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  hw/qdev-properties.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h            |    3 +++
>  qerror.c             |    5 +++++
>  qerror.h             |    4 ++++
>  4 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index bff9152..98dd06a 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -877,6 +877,52 @@ PropertyInfo qdev_prop_pci_devfn = {
>      .max   = 0xFFFFFFFFULL,
>  };
>  
> +/* --- blocksize --- */
> +
> +static void set_blocksize(Object *obj, Visitor *v, void *opaque,
> +                          const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    int16_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int(v, &value, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (value < prop->info->min || value > prop->info->max) {
> +        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> +                  dev->id?:"", name, value, prop->info->min,
> +                  prop->info->max);
> +        return;
> +    }
> +
> +    /* We rely on power-of-2 blocksizes for bitmasks */
> +    if ((value & (value - 1)) != 0) {
> +        error_set(errp, QERR_PROPERTY_VALUE_NOT_POWER_OF_2,
> +                  dev->id?:"", name, value);
> +        return;
> +    }
> +
> +    *ptr = value;
> +}
> +
> +PropertyInfo qdev_prop_blocksize = {
> +    .name  = "blocksize",
> +    .get   = get_int16,
> +    .set   = set_blocksize,
> +    .min   = 512,
> +    .max   = 65024,
> +};
> +
>  /* --- public helpers --- */
>  
>  static Property *qdev_prop_walk(Property *props, const char *name)
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 9cc3f98..f8a5ddf 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -223,6 +223,7 @@ extern PropertyInfo qdev_prop_drive;
>  extern PropertyInfo qdev_prop_netdev;
>  extern PropertyInfo qdev_prop_vlan;
>  extern PropertyInfo qdev_prop_pci_devfn;
> +extern PropertyInfo qdev_prop_blocksize;
>  
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
>          .name      = (_name),                                    \
> @@ -284,6 +285,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
>  #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
>      DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
>                          LostTickPolicy)
> +#define DEFINE_PROP_BLOCKSIZE(_n, _s, _f, _d) \
> +    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blocksize, uint16_t)
>  
>  #define DEFINE_PROP_END_OF_LIST()               \
>      {}
> diff --git a/qerror.c b/qerror.c
> index ecc5259..75c9a5c 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -233,6 +233,11 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Property '%(device).%(property)' can't find value 
> '%(value)'",
>      },
>      {
> +        .error_fmt = QERR_PROPERTY_VALUE_NOT_POWER_OF_2,
> +        .desc      = "Property '%(device).%(property)' doesn't take "
> +                     "value '%(value)', it's not a power of 2",
> +    },
> +    {
>          .error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
>          .desc      = "Property '%(device).%(property)' doesn't take "
>                       "value %(value) (minimum: %(min), maximum: %(max))",
> diff --git a/qerror.h b/qerror.h
> index e26c635..a3b9a59 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -196,6 +196,10 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_PROPERTY_VALUE_NOT_FOUND \
>      "{ 'class': 'PropertyValueNotFound', 'data': { 'device': %s, 'property': 
> %s, 'value': %s } }"
>  
> +#define QERR_PROPERTY_VALUE_NOT_POWER_OF_2 \
> +    "{ 'class': 'PropertyValueNotPowerOf2', 'data': { " \
> +    "'device': %s, 'property': %s, 'value': %"PRId64" } }"
> +
>  #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
>      "{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 
> 'property': %s, 'value': %"PRId64", 'min': %"PRId64", 'max': %"PRId64" } }"
>  

Reviewed-by: Paolo Bonzini <address@hidden>

Paolo