Re: [Qemu-devel] [PATCH 1.1 1/1] Fix geometry sector calculation

[Paolo Bonzini]

Il 24/05/2012 13:22, Christian Borntraeger ha scritto:
> Currently the sector value for the geometry is masked, even if the
> user usesa command line parameter that explicitely gives a number.
> This breaks dasd devices on s390. A dasd device can have
> a physical block size of 4096 (== same for logical block size)
> and a typcial geometry of 15 heads and 12 sectors per cyl.
> The ibm partition detection relies on a correct geometry
> reported by the device. Unfortunately the current code changes
> 12 to 8. This would be necessary if the total size is
> not a multiple of logical sector size,  but for dasd this
> is not the case.
> 
> This patch checks the device size and only applies sector
> mask if necessary.

Rereading the code, I have no idea what the masking is for.  Perhaps we
can even remove it.  However, your patch makes sense, it is safe, and it
would be nice to apply it even for 1.1.

Reviewed-by: Paolo Bonzini <address@hidden>

Paolo

> Signed-off-by: Christian Borntraeger <address@hidden>
> CC: Christoph Hellwig <address@hidden>
> ---
>  hw/virtio-blk.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index f9e1896..41c5bae 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -489,7 +489,22 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> uint8_t *config)
>      stw_raw(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
>      stw_raw(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
>      blkcfg.heads = heads;
> -    blkcfg.sectors = secs & ~s->sector_mask;
> +    /*
> +     * We must ensure that the block device capacity is a multiple of
> +     * the logical block size. If that is not the case, lets use
> +     * sector_mask to adopt the geometry to have a correct picture.
> +     * For those devices where the capacity is ok for the given geometry
> +     * we dont touch the sector value of the geometry, since some devices
> +     * (like s390 dasd) need a specific value. Here the capacity is already
> +     * cyls*heads*secs*blz_size and the sector value is not block size
> +     * divided by 512 - instead it is the amount of blk_size blocks
> +     * per track (cylinder).
> +     */
> +    if (bdrv_getlength(s->bs) /  heads / secs % blk_size) {
> +        blkcfg.sectors = secs & ~s->sector_mask;
> +    } else {
> +        blkcfg.sectors = secs;
> +    }
>      blkcfg.size_max = 0;
>      blkcfg.physical_block_exp = get_physical_block_exp(s->conf);
>      blkcfg.alignment_offset = 0;