Re: [Qemu-devel] [PATCH v5 5/5] BlockConf: Call backend functions to detect geometry and blocksizes

[Thomas Huth]

On Thu, 18 Dec 2014 12:18:04 +0100
Ekaterina Tumanova <address@hidden> wrote:

> geometry: hd_geometry_guess function autodetects the drive geometry.
> This patch adds a block backend call, that probes the backing device
> geometry. If the inner driver method is implemented and succeeds
> (currently only for DASDs), the blkconf_geometry will pass-through
> the backing device geometry. Otherwise will fallback to old logic.
> 
> blocksize: This patch initializes blocksize properties to 0.
> In order to set the properly a blkconf_blocksizes was introduced.
> If user didn't set physical or logical blocksize, it will
> retrieve it's value from a driver (which will return a default 512 for
> any backend but DASD).
> 
> The blkconf_blocksizes call was added to all users of BlkConf.
> 
> Signed-off-by: Ekaterina Tumanova <address@hidden>
> Reviewed-by: Markus Armbruster <address@hidden>
> ---
>  hw/block/block.c          | 15 +++++++++++++++
>  hw/block/hd-geometry.c    | 10 +++++++++-
>  hw/block/nvme.c           |  1 +
>  hw/block/virtio-blk.c     |  1 +
>  hw/core/qdev-properties.c |  3 ++-
>  hw/ide/qdev.c             |  1 +
>  hw/scsi/scsi-disk.c       |  1 +
>  hw/usb/dev-storage.c      |  1 +
>  include/hw/block/block.h  |  5 +++--
>  9 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/block.c b/hw/block/block.c
> index a625773..a4f4f06 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -25,6 +25,21 @@ void blkconf_serial(BlockConf *conf, char **serial)
>      }
>  }
> 
> +void blkconf_blocksizes(BlockConf *conf)
> +{
> +    BlockBackend *blk = conf->blk;
> +    BlockSizes blocksizes;
> +
> +    blk_probe_blocksizes(blk, &blocksizes);
> +    /* fill in detected values if they are not defined via qemu command line 
> */
> +    if (!conf->physical_block_size) {
> +        conf->physical_block_size = blocksizes.phys;
> +    }
> +    if (!conf->logical_block_size) {
> +        conf->logical_block_size = blocksizes.log;
> +    }
> +}
> +
>  void blkconf_geometry(BlockConf *conf, int *ptrans,
>                        unsigned cyls_max, unsigned heads_max, unsigned 
> secs_max,
>                        Error **errp)
> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
> index 6fcf74d..b187878 100644
> --- a/hw/block/hd-geometry.c
> +++ b/hw/block/hd-geometry.c
> @@ -121,8 +121,16 @@ void hd_geometry_guess(BlockBackend *blk,
>                         int *ptrans)
>  {
>      int cylinders, heads, secs, translation;
> +    HDGeometry geo;
> 
> -    if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
> +    /* Try to probe the backing device geometry, otherwise fallback
> +       to the old logic. (as of 12/2014 probing only succeeds on DASDs) */
> +    if (blk_probe_geometry(blk, &geo) == 0) {
> +        *pcyls = geo.cylinders;
> +        *psecs = geo.sectors;
> +        *pheads = geo.heads;
> +        translation = BIOS_ATA_TRANSLATION_NONE;
> +    } else if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
>          /* no LCHS guess: use a standard physical disk geometry  */
>          guess_chs_for_size(blk, pcyls, pheads, psecs);
>          translation = hd_bios_chs_auto_trans(*pcyls, *pheads, *psecs);
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index aa1ed98..2c630bc 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -763,6 +763,7 @@ static int nvme_init(PCIDevice *pci_dev)
>      if (!n->serial) {
>          return -1;
>      }
> +    blkconf_blocksizes(&n->conf);
> 
>      pci_conf = pci_dev->config;
>      pci_conf[PCI_INTERRUPT_PIN] = 1;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index b19b102..6f01565 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -745,6 +745,7 @@ static void virtio_blk_device_realize(DeviceState *dev, 
> Error **errp)
>          error_propagate(errp, err);
>          return;
>      }
> +    blkconf_blocksizes(&conf->conf);
> 
>      virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
>                  sizeof(struct virtio_blk_config));
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2e47f70..ba81709 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -580,7 +580,8 @@ static void set_blocksize(Object *obj, Visitor *v, void 
> *opaque,
>          error_propagate(errp, local_err);
>          return;
>      }
> -    if (value < min || value > max) {
> +    /* value of 0 means "unset" */
> +    if (value && (value < min || value > max)) {
>          error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
>                    dev->id?:"", name, (int64_t)value, min, max);
>          return;
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 1ebb58d..353854c 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -169,6 +169,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind 
> kind)
>      }
> 
>      blkconf_serial(&dev->conf, &dev->serial);
> +    blkconf_blocksizes(&dev->conf);
>      if (kind != IDE_CD) {
>          blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
>          if (err) {
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index f65618d..e7244e6 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2251,6 +2251,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
>      }
> 
>      blkconf_serial(&s->qdev.conf, &s->serial);
> +    blkconf_blocksizes(&s->qdev.conf);
>      if (dev->type == TYPE_DISK) {
>          blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
>          if (err) {
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 4539733..cc02dfd 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -611,6 +611,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error 
> **errp)
>      }
> 
>      blkconf_serial(&s->conf, &dev->serial);
> +    blkconf_blocksizes(&s->conf);
> 
>      /*
>       * Hack alert: this pretends to be a block device, but it's really
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 0d0ce9a..3e502a8 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -44,9 +44,9 @@ static inline unsigned int get_physical_block_exp(BlockConf 
> *conf)
>  #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
>      DEFINE_PROP_DRIVE("drive", _state, _conf.blk),                      \
>      DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
> -                          _conf.logical_block_size, 512),               \
> +                          _conf.logical_block_size, 0),               \
>      DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
> -                          _conf.physical_block_size, 512),              \
> +                          _conf.physical_block_size, 0),              \
>      DEFINE_PROP_UINT16("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, \
> @@ -63,6 +63,7 @@ void blkconf_serial(BlockConf *conf, char **serial);
>  void blkconf_geometry(BlockConf *conf, int *trans,
>                        unsigned cyls_max, unsigned heads_max, unsigned 
> secs_max,
>                        Error **errp);
> +void blkconf_blocksizes(BlockConf *conf);
> 
>  /* Hard disk geometry */
> 

Patch series looks fine to me now.

Reviewed-by: Thomas Huth <address@hidden>