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

[Paolo Bonzini]

Il 26/04/2012 15:49, Christian Borntraeger ha scritto:
> Currently the sector value for the geometry is masked based on
> the sector size. This works fine for with 512 physical sector size,
> but it fails for 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 for a physical block size /
> device size that is not dividable by 4096) but for dasd this
> is not the case.
> 
> This tries to fix the block layer by handling both cases
> dasd and non-dasd by taking the physical block size into
> account.
> 
> Signed-off-by: Christian Borntraeger <address@hidden>
> CC: Christoph Hellwig <address@hidden>
> ---
>  hw/virtio-blk.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 49990f8..5258533 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -483,6 +483,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> uint8_t *config)
>      uint64_t capacity;
>      int cylinders, heads, secs;
>      int blk_size = s->conf->logical_block_size;
> +    int pblk_size = s->conf->physical_block_size;
>  
>      bdrv_get_geometry(s->bs, &capacity);
>      bdrv_get_geometry_hint(s->bs, &cylinders, &heads, &secs);
> @@ -494,7 +495,16 @@ 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 round down the block device size to a multiple of the logical
> +     * block size. The geometry value for sectors is based on the physical
> +     * sector size and NOT on the Linux default block size of 512. For
> +     * example a dasd device usually has 12 sectors per track and each
> +     * sector has 4096 bytes (the whole cylinder == 48k). The geometry
> +     * is specified as secs=12. Thus we cannot use sector_mask, instead 
> +     * we have to use some special case.
> +     */
> +    blkcfg.sectors = secs & ~(blk_size / pblk_size - 1);

I'm not sure here what you mean.  Usually blk_size >= pblk_size on
non-s390 systems, so this is completely different from the previous
code, which is effectively

   blkcfg.sectors = secs & ~(blk_size / 512 - 1);

I wonder if s390 gives a different meaning to logical vs. physical
sector sizes, compared to what virtio expects (which is what SCSI says,
basically).  Physical block sizes on non-s390 systems are really just
useful as an alignment hint, they do not affect correctness.

Paolo

>      blkcfg.size_max = 0;
>      blkcfg.physical_block_exp = get_physical_block_exp(s->conf);
>      blkcfg.alignment_offset = 0;