Re: [Qemu-devel] [PATCH V3 2/2] virtio-block: support auto-sensing of block sizes

[Stefan Hajnoczi]

On Fri, Feb 08, 2013 at 03:52:35PM +0100, Einar Lueck wrote:
> @@ -22,6 +25,26 @@ void blkconf_serial(BlockConf *conf, char **serial)
>      }
>  }
>  
> +void blkconf_blocksizes(BlockConf *conf)
> +{
> +    int block_size;
> +
> +    if (!conf->physical_block_size) {
> +        if (bdrv_ioctl(conf->bs, BLKPBSZGET, &block_size) == 0) {

#ifdef __linux__

> +           conf->physical_block_size = (uint16_t) block_size;

4 spaces for indentation.

> +        } else {
> +            conf->physical_block_size = BLOCK_PROPERTY_STD_BLKSIZE;
> +        }
> +    }
> +    if (!conf->logical_block_size) {
> +        if (bdrv_ioctl(conf->bs, BLKSSZGET, &block_size) == 0) {
> +            conf->logical_block_size = (uint16_t) block_size;
> +        } else {
> +            conf->logical_block_size = BLOCK_PROPERTY_STD_BLKSIZE;
> +        }
> +    }
> +}
> +
>  int blkconf_geometry(BlockConf *conf, int *ptrans,
>                       unsigned cyls_max, unsigned heads_max, unsigned 
> secs_max)
>  {
> diff --git a/hw/block-common.h b/hw/block-common.h
> index bb808f7..d593128 100644
> --- a/hw/block-common.h
> +++ b/hw/block-common.h
> @@ -40,18 +40,23 @@ static inline unsigned int 
> get_physical_block_exp(BlockConf *conf)
>      return exp;
>  }
>  
> -#define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
> +#define BLOCK_PROPERTY_STD_BLKSIZE 512
> +#define DEFINE_BLOCK_PROPERTIES_EXTENDED(_state, _conf, _blksize)       \

Detecting the underlying block size is a generally useful configuration
option.  This should not be s390-specific, so no need to rename
DEFINE_BLOCK_PROPERTIES().

If you set the default to 0 then QEMU will do extra ioctls.  Perhaps
BlockDriverState needs a function to probe block sizes.  This way
block/raw-posix.c could do the probing only on hdev (host block
devices).

> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index a8a31f5..73b6da0 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -650,7 +650,9 @@ static void set_blocksize(Object *obj, Visitor *v, void 
> *opaque,
>          error_propagate(errp, local_err);
>          return;
>      }
> -    if (value < min || value > max) {
> +
> +    /* value == 0 indicates that block size should be sensed later on */
> +    if ((value < min || value > max) && value > 0) {
>          error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
>                    dev->id?:"", name, (int64_t)value, min, max);
>          return;

In the current patch this allows through the 0 value.  Since the patch
only adds a blkconf_blocksizes() call to hw/virtio-blk.c, who knows what
will happen to ide/scsi/etc -drives that are configured with 0.

We need to make sure that 0 is really handled for all emulated storage
controllers.

> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 34913ee..cd25712 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -654,6 +654,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, 
> VirtIOBlkConf *blk)
>      }
>  
>      blkconf_serial(&blk->conf, &blk->serial);
> +    blkconf_blocksizes(&blk->conf);

Shouldn't be virtio-specific, all -drives should be able to autodetect
block size.