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

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

[Christian Borntraeger]

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.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
CC: Christoph Hellwig <hch@lst.de>
---
 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;
-- 
1.7.0.4

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 <pbonzini@redhat.com>

Paolo

> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> CC: Christoph Hellwig <hch@lst.de>
> ---
>  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;

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

[Alexander Graf]

On 24.05.2012, at 13:48, Paolo Bonzini <pbonzini@redhat.com> wrote:

> 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.

I also don't understand why this code is in virtio-blk.c. What does block geometry adjustment have to do with virtio?

Alex

> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Paolo
> 
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> ---
>> 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;
> 

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

[Christian Borntraeger]

On 24/05/12 15:06, Alexander Graf wrote:
> 
> 
> On 24.05.2012, at 13:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> 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.
> 
> I also don't understand why this code is in virtio-blk.c. What does block geometry adjustment have to do with virtio?

Indeed,my first version of the patch was just

-    blkcfg.sectors = secs & ~s->sector_mask;
+    blkcfg.sectors = secs;

But then I read the commit message of this line, and the idea seems to be that
a given geometry should be able to cover the full capacity of a disk (no half filled
cylinders)
>From an s390 perspective all cases are fine and I would just remove the masking.
I was just trying to keep the behaviour.

Christian

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

[Stefan Hajnoczi]

On Thu, May 24, 2012 at 2:26 PM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> On 24/05/12 15:06, Alexander Graf wrote:
>>
>>
>> On 24.05.2012, at 13:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>> 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.
>>
>> I also don't understand why this code is in virtio-blk.c. What does block geometry adjustment have to do with virtio?
>
> Indeed,my first version of the patch was just
>
> -    blkcfg.sectors = secs & ~s->sector_mask;
> +    blkcfg.sectors = secs;
>
> But then I read the commit message of this line, and the idea seems to be that
> a given geometry should be able to cover the full capacity of a disk (no half filled
> cylinders)

Yes, this is how I interpret it as well.  We also perform checks on
individual virtio-blk requests to make sure they meet the block size -
this ensures we don't violate alignment requirements.

Stefan