[Qemu-devel] [PATCH V2] hd-geometry.c: Integrate HDIO_GETGEO in guessing

[Qemu-devel] [PATCH V2] hd-geometry.c: Integrate HDIO_GETGEO in guessing

[Einar Lueck]

Hi,
here is a reworked version.

V1 -> V2:
* removed useless code movements
* corrected terminology into logical and physical CHS and structured/layered
  functions accordingly

Einar Lueck (1):
  hd-geometry.c: Integrate HDIO_GETGEO in guessing

 hw/hd-geometry.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH V2] hd-geometry.c: Integrate HDIO_GETGEO in guessing

[Einar Lueck]

This patch extends the function hd_geometry_guess. If no geo could
be guessed via guess_disk_lchs, a new function called guess_disk_pchs is
called. The latter utilizes HDIO_GET_GEO ioctl to ask the underlying disk
for geometry.
If this is not successful (e.g. image files) geometry is derived
from the size of the disk (as before).
The new HDIO_GETGEO logic is required for two use cases:
a) Support for geometries of Direct Attached Storage Disks (DASD)
on s390x configured as backing of virtio block devices.
b) Support for FCP attached SCSI disks that do not yet have a
partition table. Without this patch, fdisk -l on the host would
return different results then fdisk -l in the guest.

Signed-off-by: Einar Lueck <elelueck@linux.vnet.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Reviewed-by: Carsten Otte <cotte@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/hd-geometry.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
index c305143..d35e25f 100644
--- a/hw/hd-geometry.c
+++ b/hw/hd-geometry.c
@@ -33,6 +33,10 @@
 #include "block/block.h"
 #include "hw/block-common.h"
 #include "trace.h"
+#ifdef __linux__
+#include <linux/fs.h>
+#include <linux/hdreg.h>
+#endif
 
 struct partition {
         uint8_t boot_ind;           /* 0x80 - active */
@@ -47,6 +51,39 @@ struct partition {
         uint32_t nr_sects;          /* nr of sectors in partition */
 } QEMU_PACKED;
 
+static void guess_chs_for_size(BlockDriverState *bs,
+                uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs);
+
+/* try to get geometry from disk via HDIO_GETGEO ioctl
+   Return 0 if OK, -1 if ioctl does not work (e.g. image file) */
+static inline int guess_disk_pchs(BlockDriverState *bs,
+                                     uint32_t *pcylinders, uint32_t *pheads,
+                                     uint32_t *psectors, int *ptranslation)
+{
+#ifdef __linux__
+    struct hd_geometry geo;
+
+    if (bdrv_ioctl(bs, HDIO_GETGEO, &geo)) {
+        return -1;
+    }
+
+    /* HDIO_GETGEO may return success even though geo contains zeros
+       (e.g. certain multipath setups) */
+    if (!geo.heads || !geo.sectors || !geo.cylinders) {
+        return -1;
+    }
+
+    *pheads = geo.heads;
+    *psectors = geo.sectors;
+    *pcylinders = geo.cylinders;
+    *ptranslation = BIOS_ATA_TRANSLATION_NONE;
+    return 0;
+#else
+    return -1;
+#endif
+}
+
+
 /* try to guess the disk logical geometry from the MSDOS partition table.
    Return 0 if OK, -1 if could not guess */
 static int guess_disk_lchs(BlockDriverState *bs,
@@ -123,9 +160,11 @@ void hd_geometry_guess(BlockDriverState *bs,
     int cylinders, heads, secs, translation;
 
     if (guess_disk_lchs(bs, &cylinders, &heads, &secs) < 0) {
-        /* no LCHS guess: use a standard physical disk geometry  */
-        guess_chs_for_size(bs, pcyls, pheads, psecs);
-        translation = hd_bios_chs_auto_trans(*pcyls, *pheads, *psecs);
+        if (guess_disk_pchs(bs, pcyls, pheads, psecs, &translation) < 0) {
+            /* no LCHS and no PCHS guess: use a standard physical disk geometry  */
+            guess_chs_for_size(bs, pcyls, pheads, psecs);
+            translation = hd_bios_chs_auto_trans(*pcyls, *pheads, *psecs);
+        }
     } else if (heads > 16) {
         /* LCHS guess with heads > 16 means that a BIOS LBA
            translation was active, so a standard physical disk
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH V2] hd-geometry.c: Integrate HDIO_GETGEO in guessing

[Markus Armbruster]

Einar Lueck <elelueck@linux.vnet.ibm.com> writes:

> This patch extends the function hd_geometry_guess. If no geo could
> be guessed via guess_disk_lchs, a new function called guess_disk_pchs is
> called. The latter utilizes HDIO_GET_GEO ioctl to ask the underlying disk
> for geometry.
> If this is not successful (e.g. image files) geometry is derived
> from the size of the disk (as before).
> The new HDIO_GETGEO logic is required for two use cases:
> a) Support for geometries of Direct Attached Storage Disks (DASD)
> on s390x configured as backing of virtio block devices.
> b) Support for FCP attached SCSI disks that do not yet have a
> partition table. Without this patch, fdisk -l on the host would
> return different results then fdisk -l in the guest.

I'm afraid this could mess up existing, working disks.

Consider a disk where guess_disk_lchs() fails and HDIO_GETGEO succeeds.

The old code arbitrarily picks a "standard" physical geometry then.

Your code picks the one returned by HDIO_GETGEO.

Unless the two geometries happen to lead to a compatible address
translation, any guest that actually uses the translation now sees
different disk contents, with predictably bad results.

Can this happen?  If no, why not?

Re: [Qemu-devel] [PATCH V2] hd-geometry.c: Integrate HDIO_GETGEO in guessing

[Einar Lueck]

On 01/14/2013 02:23 PM, Markus Armbruster wrote:
> Einar Lueck <elelueck@linux.vnet.ibm.com> writes:
>
>> This patch extends the function hd_geometry_guess. If no geo could
>> be guessed via guess_disk_lchs, a new function called guess_disk_pchs is
>> called. The latter utilizes HDIO_GET_GEO ioctl to ask the underlying disk
>> for geometry.
>> If this is not successful (e.g. image files) geometry is derived
>> from the size of the disk (as before).
>> The new HDIO_GETGEO logic is required for two use cases:
>> a) Support for geometries of Direct Attached Storage Disks (DASD)
>> on s390x configured as backing of virtio block devices.
>> b) Support for FCP attached SCSI disks that do not yet have a
>> partition table. Without this patch, fdisk -l on the host would
>> return different results then fdisk -l in the guest.
>
> I'm afraid this could mess up existing, working disks.
>
> Consider a disk where guess_disk_lchs() fails and HDIO_GETGEO succeeds.
>
> The old code arbitrarily picks a "standard" physical geometry then.
>
> Your code picks the one returned by HDIO_GETGEO.
>
> Unless the two geometries happen to lead to a compatible address
> translation, any guest that actually uses the translation now sees
> different disk contents, with predictably bad results.
>
> Can this happen?  If no, why not?
>

You are right. There may be such cases. Thus, I see two options:
a) making this code portion platform specific (s390x)
b) introduction of a a command line option to force reconsideration of 
HDIO_GETGEO
My impression is, that the value of b) is limited to s390x and therefore 
a) is the way to go.

Regards,
Einar.

Re: [Qemu-devel] [PATCH V2] hd-geometry.c: Integrate HDIO_GETGEO in guessing

[Markus Armbruster]

Einar Lueck <elelueck@linux.vnet.ibm.com> writes:

> On 01/14/2013 02:23 PM, Markus Armbruster wrote:
>> Einar Lueck <elelueck@linux.vnet.ibm.com> writes:
>>
>>> This patch extends the function hd_geometry_guess. If no geo could
>>> be guessed via guess_disk_lchs, a new function called guess_disk_pchs is
>>> called. The latter utilizes HDIO_GET_GEO ioctl to ask the underlying disk
>>> for geometry.
>>> If this is not successful (e.g. image files) geometry is derived
>>> from the size of the disk (as before).
>>> The new HDIO_GETGEO logic is required for two use cases:
>>> a) Support for geometries of Direct Attached Storage Disks (DASD)
>>> on s390x configured as backing of virtio block devices.
>>> b) Support for FCP attached SCSI disks that do not yet have a
>>> partition table. Without this patch, fdisk -l on the host would
>>> return different results then fdisk -l in the guest.
>>
>> I'm afraid this could mess up existing, working disks.
>>
>> Consider a disk where guess_disk_lchs() fails and HDIO_GETGEO succeeds.
>>
>> The old code arbitrarily picks a "standard" physical geometry then.
>>
>> Your code picks the one returned by HDIO_GETGEO.
>>
>> Unless the two geometries happen to lead to a compatible address
>> translation, any guest that actually uses the translation now sees
>> different disk contents, with predictably bad results.
>>
>> Can this happen?  If no, why not?
>>
>
> You are right. There may be such cases. Thus, I see two options:
> a) making this code portion platform specific (s390x)
> b) introduction of a a command line option to force reconsideration of
> HDIO_GETGEO
> My impression is, that the value of b) is limited to s390x and
> therefore a) is the way to go.

No objection from me.