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
<address@hidden> wrote:
> On 24/05/12 15:06, Alexander Graf wrote:
>>
>>
>> On 24.05.2012, at 13:48, Paolo Bonzini <address@hidden> 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