Re: [Qemu-devel] [PATCH v2 5/6] geometry: Call backend function to detect geometry and blocksize

[Markus Armbruster]

Ekaterina Tumanova <address@hidden> writes:

> 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 DASDs),
> the blkconf_geometry will pass-through the backing device geometry.
>
> Signed-off-by: Ekaterina Tumanova <address@hidden>
> ---
>  hw/block/block.c         | 11 +++++++++++
>  hw/block/hd-geometry.c   |  9 +++++++++
>  hw/block/virtio-blk.c    |  1 +
>  include/hw/block/block.h |  1 +
>  4 files changed, 22 insertions(+)
>
> diff --git a/hw/block/block.c b/hw/block/block.c
> index a625773..f1d29bc 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -25,6 +25,17 @@ void blkconf_serial(BlockConf *conf, char **serial)
>      }
>  }
>  
> +void blkconf_blocksizes(BlockConf *conf)
> +{
> +    BlockBackend *blk = conf->blk;
> +    struct ProbeBlockSize blocksize = blk_probe_blocksizes(blk);
> +
> +    if (blocksize.rc == 0) {
> +        conf->physical_block_size = blocksize.size.phys;
> +        conf->logical_block_size = blocksize.size.log;
> +    }
> +}
> +

Uh, doesn't this overwrite the user's geometry?

The existing blkconf_ functions do nothing when the user supplied the
configuration.  In other words, they only provide defaults.  Why should
this one be different?

>  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..4972114 100644
> --- a/hw/block/hd-geometry.c
> +++ b/hw/block/hd-geometry.c
> @@ -121,6 +121,14 @@ void hd_geometry_guess(BlockBackend *blk,
>                         int *ptrans)
>  {
>      int cylinders, heads, secs, translation;
> +    struct ProbeGeometry geometry = blk_probe_geometry(blk);
> +
> +    if (geometry.rc == 0) {
> +        *pcyls = geometry.geo.cylinders;
> +        *psecs = geometry.geo.sectors;
> +        *pheads = geometry.geo.heads;
> +        goto done;
> +    }
>  

hd_geometry_guess() is called by blkconf_geometry() to conjure up a
default geometry when the user didn't pick one.  Fairly elaborate
guesswork.  blkconf_geometry() runs for IDE, SCSI and virtio-blk only.

Your patch makes it pick the backend's geometry as reported by
blk_probe_geometry() before anything else.

This is an incompatible change for backends where blk_probe_geometry()
succeeds.  Currently, it succeeds only for DASD, and there you *want*
the incompatible change.

My point is: if we rely on blk_probe_geometry() failing except for DASD,
then we should call it something like blk_dasd_geometry() to make that
obvious.

The commit message needs to spell out the incompatible change.

>      if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
>          /* no LCHS guess: use a standard physical disk geometry  */
> @@ -143,6 +151,7 @@ void hd_geometry_guess(BlockBackend *blk,
>             the logical geometry */
>          translation = BIOS_ATA_TRANSLATION_NONE;
>      }
> +done:
>      if (ptrans) {
>          *ptrans = translation;
>      }
> 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));

Why only virtio-blk, and not the other device models sporting
configurable block sizes (nvme, IDE, SCSI, usb-storage)?

This is an incompatible change for backends where blk_probe_blocksizes()
succeeds and returns something other than (512, 512).  Currently, it
succeeds only for DASD.  Is that what we want?

The commit message needs to spell out the incompatible change.

Perhaps this patch should be split up into one for geometry and one for
block sizes.

> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 0d0ce9a..856bf75 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -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 */