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

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

[Christian Borntraeger]

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

I was trying to prevent the masking of the sector number. 
the first version of the patch simply did
blkcfg.sectors = secs;
but this broke setups that really need that masking.

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

Maybe that really points to the problem that we are trying to solve here.
For a dasd device, there is usually a 4096 byte block size and on the host
these 4096 arereported via getss and getpbsz. 
The geometry reported by the device driver is usually 15 head and 12 sectors
per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k).

What I want to achieve is that the guest view is identical to the host view
for
cyls, heads, secs, and all block sizes.

Christian

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

[Paolo Bonzini]

Il 02/05/2012 12:18, Christian Borntraeger ha scritto:
> Maybe that really points to the problem that we are trying to solve here.
> For a dasd device, there is usually a 4096 byte block size and on the host
> these 4096 arereported via getss and getpbsz. 
> The geometry reported by the device driver is usually 15 head and 12 sectors
> per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k).
> 
> What I want to achieve is that the guest view is identical to the host view
> for cyls, heads, secs, and all block sizes.

I think what you want is _not_ to have the same view as the host.  What
you want is simply to have a default that is consistent with what is
common on actual s390 disks.

Perhaps what we want here is to move the guessing of cyls/head/sector
count from -drive to -device, so that virtio-blk-s390 can apply a
different default (cyls=auto, head=15, sector=12, and also lbsz=pbsz=4096).

Markus, have you ever thought about that?  I'm a bit torn because these
are media parameters rather than device parameters, on the other hand
the same applies to lbsz/pbsz at least.

Paolo

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Christian Borntraeger]

On 27/04/12 18:12, Paolo Bonzini wrote:
> Il 26/04/2012 15:49, Christian Borntraeger ha scritto:
>> +#ifdef __linux__
>> +    } else if (bdrv_ioctl(bs, HDIO_GETGEO, &geo) == 0) {
>> +        *pcyls = geo.cylinders;
>> +        *pheads = geo.heads;
>> +        *psecs = geo.sectors;
>> +        bdrv_set_geometry_hint(bs, *pcyls, *pheads, *psecs);
>> +#endif
> 
> Perhaps you could instead move guess_disk_lchs to target-specific code,
> adding add this code to the s390-specific implementation and under
> #ifdef __s390__.  For x86 it doesn't make much sense, because a disk's
> geometry most likely will be a wrong guess for the geometry that a guest
> (for guests that care at all about geometries).

Fine with me. We care about the geometry only for dasd devices, Even for FCP-based
SCSI devices on s390 the geometry is not relevant. So moving that part to 
s390 specific code might make sense if nobody else needs that.
Is that the case?
Alex, would that be ok for you?

Christian

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

[Christian Borntraeger]

On 02/05/12 12:25, Paolo Bonzini wrote:
> Il 02/05/2012 12:18, Christian Borntraeger ha scritto:
>> Maybe that really points to the problem that we are trying to solve here.
>> For a dasd device, there is usually a 4096 byte block size and on the host
>> these 4096 arereported via getss and getpbsz. 
>> The geometry reported by the device driver is usually 15 head and 12 sectors
>> per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k).
>>
>> What I want to achieve is that the guest view is identical to the host view
>> for cyls, heads, secs, and all block sizes.
> 
> I think what you want is _not_ to have the same view as the host.  What
> you want is simply to have a default that is consistent with what is
> common on actual s390 disks.

Let me put it in another way:

I want to have these values to match the _device_ that we are passing to the guest
because several tools and the partition detection code for a compatible disk format
(those that can be accessed by z/OS) needs those values to work properly.

That of course means that the guest view is identical to the host view because both
views describe a real property of the hardware.

IOW the geometry for dasd devices is not an artifical number, it has some real meaning
that has a influence on the data structures on the disk.

Thing is, the easiest way of getting the hardware property is to query the host.

Does that make the situation a bit clearer?

Christian

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

[Paolo Bonzini]

Il 02/05/2012 12:50, Christian Borntraeger ha scritto:
> On 02/05/12 12:25, Paolo Bonzini wrote:
>> Il 02/05/2012 12:18, Christian Borntraeger ha scritto:
>>> Maybe that really points to the problem that we are trying to solve here.
>>> For a dasd device, there is usually a 4096 byte block size and on the host
>>> these 4096 arereported via getss and getpbsz. 
>>> The geometry reported by the device driver is usually 15 head and 12 sectors
>>> per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k).
>>>
>>> What I want to achieve is that the guest view is identical to the host view
>>> for cyls, heads, secs, and all block sizes.
>>
>> I think what you want is _not_ to have the same view as the host.  What
>> you want is simply to have a default that is consistent with what is
>> common on actual s390 disks.
> 
> Let me put it in another way:
> 
> I want to have these values to match the _device_ that we are passing to the guest
> because several tools and the partition detection code for a compatible disk format
> (those that can be accessed by z/OS) needs those values to work properly.

Ah, you never pass part of a disk to a guest and part of the same disk
to another?

> IOW the geometry for dasd devices is not an artifical number, it has some real meaning
> that has a influence on the data structures on the disk.

Yes, I understood this.

Paolo

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

[Alexander Graf]

On 05/02/2012 12:50 PM, Christian Borntraeger wrote:
> On 02/05/12 12:25, Paolo Bonzini wrote:
>> Il 02/05/2012 12:18, Christian Borntraeger ha scritto:
>>> Maybe that really points to the problem that we are trying to solve here.
>>> For a dasd device, there is usually a 4096 byte block size and on the host
>>> these 4096 arereported via getss and getpbsz.
>>> The geometry reported by the device driver is usually 15 head and 12 sectors
>>> per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k).
>>>
>>> What I want to achieve is that the guest view is identical to the host view
>>> for cyls, heads, secs, and all block sizes.
>> I think what you want is _not_ to have the same view as the host.  What
>> you want is simply to have a default that is consistent with what is
>> common on actual s390 disks.
> Let me put it in another way:
>
> I want to have these values to match the _device_ that we are passing to the guest
> because several tools and the partition detection code for a compatible disk format
> (those that can be accessed by z/OS) needs those values to work properly.
>
> That of course means that the guest view is identical to the host view because both
> views describe a real property of the hardware.
>
> IOW the geometry for dasd devices is not an artifical number, it has some real meaning
> that has a influence on the data structures on the disk.
>
> Thing is, the easiest way of getting the hardware property is to query the host.
>
> Does that make the situation a bit clearer?

Another thing to consider is that there are 2 generic types of disks:

   * SCSI disks
   * DASD disks

Both can be accessed using virtio-blk-s390. The former have the same 
semantics on geometry as what we're used to. They use normal MBRs and 
essentially the geometry doesn't mean too much these days anymore ;). 
However, if possible, I would like to diverge these as little as 
possible from x86 virtio disks.

For DASD disks, the geometry is important, as its disk label is usually 
not MBR, but something s390 specific. That one is different depending on 
the geometry. So here the geometry is very important. The geometry on 
the same disk can also be different, depending on how it's formatted. So 
it's not quite as easy to reconstruct it from the imformation we already 
have. IIUC if we have the logical block size and the information that 
it's a DASD disk, we should be able to guess the geometry though.


Alex

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

[Paolo Bonzini]

Il 02/05/2012 13:07, Alexander Graf ha scritto:
> Both can be accessed using virtio-blk-s390. The former have the same
> semantics on geometry as what we're used to. They use normal MBRs and
> essentially the geometry doesn't mean too much these days anymore ;).
> However, if possible, I would like to diverge these as little as
> possible from x86 virtio disks.

If anything, the geometry should be guessed like we do on x86.

> For DASD disks, the geometry is important, as its disk label is usually
> not MBR, but something s390 specific.

Can we use this to guess the geometry like we do on x86?

Paolo

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Alexander Graf]

On 05/02/2012 12:27 PM, Christian Borntraeger wrote:
> On 27/04/12 18:12, Paolo Bonzini wrote:
>> Il 26/04/2012 15:49, Christian Borntraeger ha scritto:
>>> +#ifdef __linux__
>>> +    } else if (bdrv_ioctl(bs, HDIO_GETGEO,&geo) == 0) {
>>> +        *pcyls = geo.cylinders;
>>> +        *pheads = geo.heads;
>>> +        *psecs = geo.sectors;
>>> +        bdrv_set_geometry_hint(bs, *pcyls, *pheads, *psecs);
>>> +#endif
>> Perhaps you could instead move guess_disk_lchs to target-specific code,
>> adding add this code to the s390-specific implementation and under
>> #ifdef __s390__.  For x86 it doesn't make much sense, because a disk's
>> geometry most likely will be a wrong guess for the geometry that a guest
>> (for guests that care at all about geometries).
> Fine with me. We care about the geometry only for dasd devices, Even for FCP-based
> SCSI devices on s390 the geometry is not relevant. So moving that part to
> s390 specific code might make sense if nobody else needs that.
> Is that the case?
> Alex, would that be ok for you?

As hinted in my other mail, I think the way to go would be to give a 
hint to the geometry code that we're running on a DASD disk. Then we can

   * Ask the host device if it can give us its geometry, if so, use it
   * Guess depending on the logical block size

and everyone should be happy :). I would really like to have as little 
#ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is even worse, as 
it means we won't be able to execise that code path on other architectures.


Alex

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

[Alexander Graf]

On 05/02/2012 01:09 PM, Paolo Bonzini wrote:
> Il 02/05/2012 13:07, Alexander Graf ha scritto:
>> Both can be accessed using virtio-blk-s390. The former have the same
>> semantics on geometry as what we're used to. They use normal MBRs and
>> essentially the geometry doesn't mean too much these days anymore ;).
>> However, if possible, I would like to diverge these as little as
>> possible from x86 virtio disks.
> If anything, the geometry should be guessed like we do on x86.
>
>> For DASD disks, the geometry is important, as its disk label is usually
>> not MBR, but something s390 specific.
> Can we use this to guess the geometry like we do on x86?

Yes, but what do you do with a blank disk? :)


Alex

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

[Paolo Bonzini]

Il 02/05/2012 13:10, Alexander Graf ha scritto:
>>> For DASD disks, the geometry is important, as its disk label is usually
>>> not MBR, but something s390 specific.
>> Can we use this to guess the geometry like we do on x86?
> 
> Yes, but what do you do with a blank disk? :)

Right. :)

Paolo

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Paolo Bonzini]

> and everyone should be happy :). I would really like to have as
> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is even worse,
> as it means we won't be able to execise that code path on other
> architectures.

True, but how do you exercise that code path with DASD geometry
on !__s390__?

Paolo

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Alexander Graf]

On 05/02/2012 01:26 PM, Paolo Bonzini wrote:
>> and everyone should be happy :). I would really like to have as
>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is even worse,
>> as it means we won't be able to execise that code path on other
>> architectures.
> True, but how do you exercise that code path with DASD geometry
> on !__s390__?

If we make things a flag for the guessing code, it should work just as 
well with image files, right?


Alex

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Paolo Bonzini]

> On 05/02/2012 01:26 PM, Paolo Bonzini wrote:
> >> and everyone should be happy :). I would really like to have as
> >> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is
> >> even worse,
> >> as it means we won't be able to execise that code path on other
> >> architectures.
> > True, but how do you exercise that code path with DASD geometry
> > on !__s390__?
> 
> If we make things a flag for the guessing code, it should work just
> as well with image files, right?

Only when they're not blank. :)  I was only thinking of #ifdef __s390__
for the call to HDIO_GETGEO.

Paolo

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Christian Borntraeger]

> As hinted in my other mail, I think the way to go would be to give a hint to the geometry code that we're running on a DASD disk..

Just as an idea if we are going that path, 
we might use the BIODASDINFO2 or DASDAPIVER ioctls in qemu to detect if that is a dasd.

Christian

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Alexander Graf]

On 05/02/2012 01:38 PM, Paolo Bonzini wrote:
>> On 05/02/2012 01:26 PM, Paolo Bonzini wrote:
>>>> and everyone should be happy :). I would really like to have as
>>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is
>>>> even worse,
>>>> as it means we won't be able to execise that code path on other
>>>> architectures.
>>> True, but how do you exercise that code path with DASD geometry
>>> on !__s390__?
>> If we make things a flag for the guessing code, it should work just
>> as well with image files, right?
> Only when they're not blank. :)  I was only thinking of #ifdef __s390__
> for the call to HDIO_GETGEO.

Well, if guessing is a function

   guess_size(disk_size, block_size)

then we would be able to do the same on an image file. Christian, would 
that work?


Alex

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Christian Borntraeger]

On 02/05/12 14:54, Alexander Graf wrote:
> On 05/02/2012 01:38 PM, Paolo Bonzini wrote:
>>> On 05/02/2012 01:26 PM, Paolo Bonzini wrote:
>>>>> and everyone should be happy :). I would really like to have as
>>>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is
>>>>> even worse,
>>>>> as it means we won't be able to execise that code path on other
>>>>> architectures.
>>>> True, but how do you exercise that code path with DASD geometry
>>>> on !__s390__?
>>> If we make things a flag for the guessing code, it should work just
>>> as well with image files, right?
>> Only when they're not blank. :)  I was only thinking of #ifdef __s390__
>> for the call to HDIO_GETGEO.
> 
> Well, if guessing is a function
> 
>   guess_size(disk_size, block_size)
> 
> then we would be able to do the same on an image file. Christian, would that work?

I think that the geometry values can not always be guessed correctly based on
block_size and disk_size.

Stefan, can you clarify that?

If we cannot reliably guess the geometry based on blocksize and size, I still think
that we should use the host values, e.g. after checking that BIODASDINFO2 returns 
successfully.

Christian

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Alexander Graf]

On 02.05.2012, at 16:27, Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02/05/12 14:54, Alexander Graf wrote:
>> On 05/02/2012 01:38 PM, Paolo Bonzini wrote:
>>>> On 05/02/2012 01:26 PM, Paolo Bonzini wrote:
>>>>>> and everyone should be happy :). I would really like to have as
>>>>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is
>>>>>> even worse,
>>>>>> as it means we won't be able to execise that code path on other
>>>>>> architectures.
>>>>> True, but how do you exercise that code path with DASD geometry
>>>>> on !__s390__?
>>>> If we make things a flag for the guessing code, it should work just
>>>> as well with image files, right?
>>> Only when they're not blank. :)  I was only thinking of #ifdef __s390__
>>> for the call to HDIO_GETGEO.
>> 
>> Well, if guessing is a function
>> 
>>  guess_size(disk_size, block_size)
>> 
>> then we would be able to do the same on an image file. Christian, would that work?
> 
> I think that the geometry values can not always be guessed correctly based on
> block_size and disk_size.
> 
> Stefan, can you clarify that?
> 
> If we cannot reliably guess the geometry based on blocksize and size, I still think
> that we should use the host values, e.g. after checking that BIODASDINFO2 returns 
> successfully.

Yeah, but only if it's always possible to force a specific geometry through the command line - otherwise reproducability suffers.


Alex

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Stefan Weinhuber]

On 2012-05-02 16:27, Christian Borntraeger wrote:
> On 02/05/12 14:54, Alexander Graf wrote:
>> On 05/02/2012 01:38 PM, Paolo Bonzini wrote:
>>>> On 05/02/2012 01:26 PM, Paolo Bonzini wrote:
>>>>>> and everyone should be happy :). I would really like to have as
>>>>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is
>>>>>> even worse,
>>>>>> as it means we won't be able to execise that code path on other
>>>>>> architectures.
>>>>> True, but how do you exercise that code path with DASD geometry
>>>>> on !__s390__?
>>>> If we make things a flag for the guessing code, it should work just
>>>> as well with image files, right?
>>> Only when they're not blank. :)  I was only thinking of #ifdef __s390__
>>> for the call to HDIO_GETGEO.
>>
>> Well, if guessing is a function
>>
>>    guess_size(disk_size, block_size)
>>
>> then we would be able to do the same on an image file. Christian, would that work?
>
> I think that the geometry values can not always be guessed correctly based on
> block_size and disk_size.
>
> Stefan, can you clarify that?
>
> If we cannot reliably guess the geometry based on blocksize and size, I still think
> that we should use the host values, e.g. after checking that BIODASDINFO2 returns
> successfully.

If we know the device type (e.g. 3390) and the block_size, then we can 
compute the number of blocks per track. The number of tracks per 
cylinder is a given (15) and the number of cylinders can be computed 
from these numbers and the disk_size.

How do we get the device type? I think we could get away with 
restricting ECKD DASDs to type 3390, but even then, how would we 
distinguish an ECKD DASD from anything else, e.g. a SCSI device?

We could simply attempt the above cylinder calculation for every device 
and if we get a result without a remainder we just assume that we have a 
DASD. This could lead to false positives, but maybe that is acceptable?

Stefan Weinhuber

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Alexander Graf]

On 02.05.2012, at 17:57, Stefan Weinhuber <wein@linux.vnet.ibm.com> wrote:

> On 2012-05-02 16:27, Christian Borntraeger wrote:
>> On 02/05/12 14:54, Alexander Graf wrote:
>>> On 05/02/2012 01:38 PM, Paolo Bonzini wrote:
>>>>> On 05/02/2012 01:26 PM, Paolo Bonzini wrote:
>>>>>>> and everyone should be happy :). I would really like to have as
>>>>>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is
>>>>>>> even worse,
>>>>>>> as it means we won't be able to execise that code path on other
>>>>>>> architectures.
>>>>>> True, but how do you exercise that code path with DASD geometry
>>>>>> on !__s390__?
>>>>> If we make things a flag for the guessing code, it should work just
>>>>> as well with image files, right?
>>>> Only when they're not blank. :)  I was only thinking of #ifdef __s390__
>>>> for the call to HDIO_GETGEO.
>>> 
>>> Well, if guessing is a function
>>> 
>>>   guess_size(disk_size, block_size)
>>> 
>>> then we would be able to do the same on an image file. Christian, would that work?
>> 
>> I think that the geometry values can not always be guessed correctly based on
>> block_size and disk_size.
>> 
>> Stefan, can you clarify that?
>> 
>> If we cannot reliably guess the geometry based on blocksize and size, I still think
>> that we should use the host values, e.g. after checking that BIODASDINFO2 returns
>> successfully.
> 
> If we know the device type (e.g. 3390) and the block_size, then we can compute the number of blocks per track. The number of tracks per cylinder is a given (15) and the number of cylinders can be computed from these numbers and the disk_size.
> 
> How do we get the device type? I think we could get away with restricting ECKD DASDs to type 3390, but even then, how would we distinguish an ECKD DASD from anything else, e.g. a SCSI device?
> 
> We could simply attempt the above cylinder calculation for every device and if we get a result without a remainder we just assume that we have a DASD. This could lead to false positives, but maybe that is acceptable?

We could add a parameter to the disk configuration like type=dasd (default would be type=auto) which tells the geometry detection code to assume a 3390. If type == auto, we try a dasd ioctl and if that works use type=dasd.

That way you could easily create a dump of the disk and get it working with a simple type=dasd. Later we could even add dasd disk label detection code which defaults type=auto to dasd if it finds one.

That way disk images should be as easy to use as real dasd devices :).


Alex

Re: [Qemu-devel] [PATCH 2/3] geometry detection: use HDIO_GETGEO

[Christian Borntraeger]

>>> Well, if guessing is a function
>>>
>>>  guess_size(disk_size, block_size)
>>>
>>> then we would be able to do the same on an image file. Christian, would that work?
>>
>> I think that the geometry values can not always be guessed correctly based on
>> block_size and disk_size.
>>
>> Stefan, can you clarify that?
>>
>> If we cannot reliably guess the geometry based on blocksize and size, I still think
>> that we should use the host values, e.g. after checking that BIODASDINFO2 returns 
>> successfully.
> 
> Yeah, but only if it's always possible to force a specific geometry through the command line - otherwise reproducability suffers.

That is already possible, with the exception of the sector part of the geometry (see the other
patch :-) )

Christian