[Qemu-devel] [PATCH 0/3] block patches for auto detection of geometry and block size

[Qemu-devel] [PATCH 0/3] block patches for auto detection of geometry and block size

[Christian Borntraeger]

Here are three patches that allow to detect the host configuration of
geometry and block sizes. This is necessary on s390 to make the guest partition
detection work (on dasds the geometry still influences the layout), as well as
to not break cache=none (e.g. dasd disks often have 4k block size)

These patches try to minize the impact by:
- using HGIO_GETGEO only in cases were the layout cannot be read from disk
- using host block size only if the user or the bus specifies the special
  value of 0 for logical_block_size

Einar Lueck (3):
  hd-geometry.c: Integrate HDIO_GETGEO in guessing
  hd-geometry.c/s390: Disable geometry translation
  block: support auto-sensing of block sizes

 hw/block-common.c    |   29 ++++++++++++
 hw/block-common.h    |   12 +++--
 hw/hd-geometry.c     |  129 +++++++++++++++++++++++++++++++++++---------------
 hw/ide/qdev.c        |    1 +
 hw/qdev-properties.c |    4 +-
 hw/s390-virtio-bus.c |    2 +-
 hw/scsi-disk.c       |    1 +
 hw/virtio-blk.c      |    1 +
 8 files changed, 137 insertions(+), 42 deletions(-)

-- 
1.7.10.1

[Qemu-devel] [PATCH 1/3] hd-geometry.c: Integrate HDIO_GETGEO in guessing

[Christian Borntraeger]

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

This patch extends the function guess_disk_lchs. If no geo could
be derived from reading disk content, HDIO_GETGEO ioctl is issued.
If this is not successful (e.g. image files) geometry is derived
from the size of the disk (as before). To achieve this, the MSDOS
partition table reading logic and the translation computation logic
have been moved into a separate function guess_disk_msdosgeo. The
HDIO_GETGEO logic is captured in a new function guess_disk_ioctlgeo.
guess_disk_lchs then encapsulates both (the overall guessing logic).
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 |  124 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 87 insertions(+), 37 deletions(-)

diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
index 1cdb9fb..4cf040d 100644
--- a/hw/hd-geometry.c
+++ b/hw/hd-geometry.c
@@ -33,6 +33,10 @@
 #include "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,13 +51,65 @@ 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)
+{
+    uint64_t nb_sectors;
+    int cylinders;
+
+    bdrv_get_geometry(bs, &nb_sectors);
+
+    cylinders = nb_sectors / (16 * 63);
+    if (cylinders > 16383) {
+        cylinders = 16383;
+    } else if (cylinders < 2) {
+        cylinders = 2;
+    }
+    *pcyls = cylinders;
+    *pheads = 16;
+    *psecs = 63;
+}
+
+/* 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_ioctlgeo(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;
+    trace_hd_geometry_lchs_guess(bs, *pcylinders, *pheads, *psectors);
+    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,
-                           int *pcylinders, int *pheads, int *psectors)
+static int guess_disk_msdosgeo(BlockDriverState *bs,
+                               uint32_t *pcylinders, uint32_t *pheads,
+                               uint32_t *psectors, int *ptranslation)
 {
     uint8_t buf[BDRV_SECTOR_SIZE];
-    int i, heads, sectors, cylinders;
+    int i, translation;
+    uint32_t heads, sectors, cylinders;
     struct partition *p;
     uint32_t nr_sects;
     uint64_t nb_sectors;
@@ -87,9 +143,26 @@ static int guess_disk_lchs(BlockDriverState *bs,
             if (cylinders < 1 || cylinders > 16383) {
                 continue;
             }
+
+            if (heads > 16) {
+                /* LCHS guess with heads > 16 means that a BIOS LBA
+                   translation was active, so a standard physical disk
+                   geometry is OK */
+                guess_chs_for_size(bs, &cylinders, &heads, &sectors);
+                translation = cylinders * heads <= 131072
+                    ? BIOS_ATA_TRANSLATION_LARGE
+                    : BIOS_ATA_TRANSLATION_LBA;
+            } else {
+                /* LCHS guess with heads <= 16: use as physical geometry */
+                /* disable any translation to be in sync with
+                   the logical geometry */
+                translation = BIOS_ATA_TRANSLATION_NONE;
+            }
             *pheads = heads;
             *psectors = sectors;
             *pcylinders = cylinders;
+            *ptranslation = translation;
+
             trace_hd_geometry_lchs_guess(bs, cylinders, heads, sectors);
             return 0;
         }
@@ -97,51 +170,28 @@ static int guess_disk_lchs(BlockDriverState *bs,
     return -1;
 }
 
-static void guess_chs_for_size(BlockDriverState *bs,
-                uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs)
-{
-    uint64_t nb_sectors;
-    int cylinders;
-
-    bdrv_get_geometry(bs, &nb_sectors);
-
-    cylinders = nb_sectors / (16 * 63);
-    if (cylinders > 16383) {
-        cylinders = 16383;
-    } else if (cylinders < 2) {
-        cylinders = 2;
+/* try to guess the disk logical geometry
+   Return 0 if OK, -1 if could not guess */
+static int guess_disk_lchs(BlockDriverState *bs,
+                           uint32_t *pcylinders, uint32_t *pheads, 
+                           uint32_t *psectors, int *ptranslation) {
+    if (!guess_disk_msdosgeo(bs, pcylinders, pheads, psectors, ptranslation)) {
+        return 0;
     }
-    *pcyls = cylinders;
-    *pheads = 16;
-    *psecs = 63;
+
+    return guess_disk_ioctlgeo(bs, pcylinders, pheads, psectors, ptranslation);
 }
 
 void hd_geometry_guess(BlockDriverState *bs,
                        uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
                        int *ptrans)
 {
-    int cylinders, heads, secs, translation;
+    int translation;
 
-    if (guess_disk_lchs(bs, &cylinders, &heads, &secs) < 0) {
+    if (guess_disk_lchs(bs, pcyls, pheads, psecs, &translation) < 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);
-    } else if (heads > 16) {
-        /* LCHS guess with heads > 16 means that a BIOS LBA
-           translation was active, so a standard physical disk
-           geometry is OK */
-        guess_chs_for_size(bs, pcyls, pheads, psecs);
-        translation = *pcyls * *pheads <= 131072
-            ? BIOS_ATA_TRANSLATION_LARGE
-            : BIOS_ATA_TRANSLATION_LBA;
-    } else {
-        /* LCHS guess with heads <= 16: use as physical geometry */
-        *pcyls = cylinders;
-        *pheads = heads;
-        *psecs = secs;
-        /* disable any translation to be in sync with
-           the logical geometry */
-        translation = BIOS_ATA_TRANSLATION_NONE;
     }
     if (ptrans) {
         *ptrans = translation;
-- 
1.7.10.1

[Qemu-devel] [PATCH 2/3] hd-geometry.c/s390: Disable geometry translation

[Christian Borntraeger]

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

This patch disables the translation of geometry information read from
disks on s390. On s390 such translations lead to wrong geometries being
advertized to the guest because there is no entity doing these kinds
of translation on this architecture.

Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/hd-geometry.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
index 4cf040d..db1dc81 100644
--- a/hw/hd-geometry.c
+++ b/hw/hd-geometry.c
@@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
                 continue;
             }
 
+#ifdef __s390__
+            /* on s390 there is no BIOS doing any kind of translation */
+            translation = BIOS_ATA_TRANSLATION_NONE;
+#else
             if (heads > 16) {
                 /* LCHS guess with heads > 16 means that a BIOS LBA
                    translation was active, so a standard physical disk
@@ -158,6 +162,7 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
                    the logical geometry */
                 translation = BIOS_ATA_TRANSLATION_NONE;
             }
+#endif
             *pheads = heads;
             *psectors = sectors;
             *pcylinders = cylinders;
-- 
1.7.10.1

[Qemu-devel] [PATCH 3/3] block: support auto-sensing of block sizes

[Christian Borntraeger]

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

Virtio-blk does not impose fixed block sizes for access to
backing devices. This patch introduces support for auto
lookup of the block sizes of the backing block device. This
automatic lookup needs to be enabled explicitly. Users may
do this by specifying (physical|logical)_block_size=0.
Machine types may do this for their defaults, too.
To achieve this, a new function blkconf_blocksizes is
implemented. If physical or logical block size are zero
a corresponding ioctl tries to find an appropriate value.
If this does not work, 512 is used.

C. Borntraeger: added calls to blkconf_blocksizes for scsi and ide
to ensure that a command line parameter with blocksize == 0 will
not break scsi or IDE.

For s390-virtio-blk, this patch configures auto lookup as
default.

Signed-off-by: Einar Lueck <elelueck@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/block-common.c    |   29 +++++++++++++++++++++++++++++
 hw/block-common.h    |   12 +++++++++---
 hw/ide/qdev.c        |    1 +
 hw/qdev-properties.c |    4 +++-
 hw/s390-virtio-bus.c |    2 +-
 hw/scsi-disk.c       |    1 +
 hw/virtio-blk.c      |    1 +
 7 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/hw/block-common.c b/hw/block-common.c
index f0196d7..a9c8a09 100644
--- a/hw/block-common.c
+++ b/hw/block-common.c
@@ -10,6 +10,9 @@
 #include "blockdev.h"
 #include "hw/block-common.h"
 #include "qemu-error.h"
+#ifdef __linux__
+#include <linux/fs.h>
+#endif
 
 void blkconf_serial(BlockConf *conf, char **serial)
 {
@@ -24,6 +27,32 @@ void blkconf_serial(BlockConf *conf, char **serial)
     }
 }
 
+/* Read the host block size in case the machine or the user asked for it */
+void blkconf_blocksizes(BlockConf *conf)
+{
+#ifdef __linux__
+    int block_size;
+
+    if (!conf->physical_block_size) {
+        if (bdrv_ioctl(conf->bs, BLKPBSZGET, &block_size) == 0) {
+           conf->physical_block_size = (uint16_t) block_size;
+        } else {
+            conf->physical_block_size = BLOCK_PROPERTY_STD_BLKSIZE;
+        }
+    }
+    if (!conf->logical_block_size) {
+        if (bdrv_ioctl(conf->bs, BLKSSZGET, &block_size) == 0) {
+            conf->logical_block_size = (uint16_t) block_size;
+        } else {
+            conf->logical_block_size = BLOCK_PROPERTY_STD_BLKSIZE;
+        }
+    }
+#else
+    conf->physical_block_size = BLOCK_PROPERTY_STD_BLKSIZE;
+    conf->logical_block_size  = BLOCK_PROPERTY_STD_BLKSIZE;
+#endif
+}
+
 int blkconf_geometry(BlockConf *conf, int *ptrans,
                      unsigned cyls_max, unsigned heads_max, unsigned secs_max)
 {
diff --git a/hw/block-common.h b/hw/block-common.h
index bb808f7..d593128 100644
--- a/hw/block-common.h
+++ b/hw/block-common.h
@@ -40,18 +40,23 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
     return exp;
 }
 
-#define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
+#define BLOCK_PROPERTY_STD_BLKSIZE 512
+#define DEFINE_BLOCK_PROPERTIES_EXTENDED(_state, _conf, _blksize)       \
     DEFINE_PROP_DRIVE("drive", _state, _conf.bs),                       \
     DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
-                          _conf.logical_block_size, 512),               \
+                          _conf.logical_block_size, _blksize),          \
     DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
-                          _conf.physical_block_size, 512),              \
+                          _conf.physical_block_size, _blksize),         \
     DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
     DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
     DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1),        \
     DEFINE_PROP_UINT32("discard_granularity", _state, \
                        _conf.discard_granularity, 0)
 
+#define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
+    DEFINE_BLOCK_PROPERTIES_EXTENDED(_state, _conf,                     \
+                                     BLOCK_PROPERTY_STD_BLKSIZE)
+
 #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
     DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
     DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
@@ -60,6 +65,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
 /* Configuration helpers */
 
 void blkconf_serial(BlockConf *conf, char **serial);
+void blkconf_blocksizes(BlockConf *conf);
 int blkconf_geometry(BlockConf *conf, int *trans,
                      unsigned cyls_max, unsigned heads_max, unsigned secs_max);
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index f2e4ea4..e110caf 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -149,6 +149,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
     }
 
     blkconf_serial(&dev->conf, &dev->serial);
+    blkconf_blocksizes(&dev->conf);
     if (kind != IDE_CD
         && blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255) < 0) {
         return -1;
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 8aca0d4..e99dee4 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -904,7 +904,9 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque,
         error_propagate(errp, local_err);
         return;
     }
-    if (value < min || value > max) {
+
+    /* value == 0 indicates that block size should be sensed later on */
+    if ((value < min || value > max) && value > 0) {
         error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
                   dev->id?:"", name, (int64_t)value, min, max);
         return;
diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index 5849a96..223ff2d 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -401,7 +401,7 @@ static TypeInfo s390_virtio_net = {
 };
 
 static Property s390_virtio_blk_properties[] = {
-    DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, blk.conf),
+    DEFINE_BLOCK_PROPERTIES_EXTENDED(VirtIOS390Device, blk.conf, 0),
     DEFINE_BLOCK_CHS_PROPERTIES(VirtIOS390Device, blk.conf),
     DEFINE_PROP_STRING("serial", VirtIOS390Device, blk.serial),
 #ifdef __linux__
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 1b0afa6..29df433 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -2046,6 +2046,7 @@ static int scsi_initfn(SCSIDevice *dev)
     }
 
     blkconf_serial(&s->qdev.conf, &s->serial);
+    blkconf_blocksizes(&s->qdev.conf);
     if (dev->type == TYPE_DISK
         && blkconf_geometry(&dev->conf, NULL, 65535, 255, 255) < 0) {
         return -1;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index e25cc96..f11ceb7 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -615,6 +615,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
     }
 
     blkconf_serial(&blk->conf, &blk->serial);
+    blkconf_blocksizes(&blk->conf);
     if (blkconf_geometry(&blk->conf, NULL, 65535, 255, 255) < 0) {
         return NULL;
     }
-- 
1.7.10.1

Re: [Qemu-devel] [PATCH 2/3] hd-geometry.c/s390: Disable geometry translation

[Blue Swirl]

On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>
> This patch disables the translation of geometry information read from
> disks on s390. On s390 such translations lead to wrong geometries being
> advertized to the guest because there is no entity doing these kinds
> of translation on this architecture.
>
> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/hd-geometry.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
> index 4cf040d..db1dc81 100644
> --- a/hw/hd-geometry.c
> +++ b/hw/hd-geometry.c
> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>                  continue;
>              }
>
> +#ifdef __s390__

No, this would make all system emulators (e.g. x86) on s390 host to
use this translation, not just s390. I think you want to use #ifdef
CONFIG_S390X instead.

> +            /* on s390 there is no BIOS doing any kind of translation */
> +            translation = BIOS_ATA_TRANSLATION_NONE;
> +#else
>              if (heads > 16) {
>                  /* LCHS guess with heads > 16 means that a BIOS LBA
>                     translation was active, so a standard physical disk
> @@ -158,6 +162,7 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>                     the logical geometry */
>                  translation = BIOS_ATA_TRANSLATION_NONE;
>              }
> +#endif
>              *pheads = heads;
>              *psectors = sectors;
>              *pcylinders = cylinders;
> --
> 1.7.10.1
>
>

Re: [Qemu-devel] [PATCH 2/3] hd-geometry.c/s390: Disable geometry translation

[Paolo Bonzini]

Il 17/11/2012 17:07, Blue Swirl ha scritto:
> On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>>
>> This patch disables the translation of geometry information read from
>> disks on s390. On s390 such translations lead to wrong geometries being
>> advertized to the guest because there is no entity doing these kinds
>> of translation on this architecture.
>>
>> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
>> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  hw/hd-geometry.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>> index 4cf040d..db1dc81 100644
>> --- a/hw/hd-geometry.c
>> +++ b/hw/hd-geometry.c
>> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>                  continue;
>>              }
>>
>> +#ifdef __s390__
> 
> No, this would make all system emulators (e.g. x86) on s390 host to
> use this translation, not just s390. I think you want to use #ifdef
> CONFIG_S390X instead.

This symbol is not available because this file is compiled just once.
Which non-x86 targets actually care about translation stuff?

Probably it would be best to split the MS-DOS stuff in a separate file,
and use the stub mechanism (no weak symbols, alas :)) to provide a
default implementation that just returns BIOS_ATA_TRANSLATION_NONE.

>> +            /* on s390 there is no BIOS doing any kind of translation */
>> +            translation = BIOS_ATA_TRANSLATION_NONE;
>> +#else
>>              if (heads > 16) {
>>                  /* LCHS guess with heads > 16 means that a BIOS LBA
>>                     translation was active, so a standard physical disk
>> @@ -158,6 +162,7 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>                     the logical geometry */
>>                  translation = BIOS_ATA_TRANSLATION_NONE;
>>              }
>> +#endif
>>              *pheads = heads;
>>              *psectors = sectors;
>>              *pcylinders = cylinders;
>> --
>> 1.7.10.1
>>
>>
> 
> 

Re: [Qemu-devel] [PATCH 2/3] hd-geometry.c/s390: Disable geometry translation

[Blue Swirl]

On Sun, Nov 18, 2012 at 4:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/11/2012 17:07, Blue Swirl ha scritto:
>> On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
>> <borntraeger@de.ibm.com> wrote:
>>> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>
>>> This patch disables the translation of geometry information read from
>>> disks on s390. On s390 such translations lead to wrong geometries being
>>> advertized to the guest because there is no entity doing these kinds
>>> of translation on this architecture.
>>>
>>> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
>>> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>  hw/hd-geometry.c |    5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>>> index 4cf040d..db1dc81 100644
>>> --- a/hw/hd-geometry.c
>>> +++ b/hw/hd-geometry.c
>>> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>>                  continue;
>>>              }
>>>
>>> +#ifdef __s390__
>>
>> No, this would make all system emulators (e.g. x86) on s390 host to
>> use this translation, not just s390. I think you want to use #ifdef
>> CONFIG_S390X instead.
>
> This symbol is not available because this file is compiled just once.
> Which non-x86 targets actually care about translation stuff?

I meant TARGET_S390X, sorry. I have no idea, but hosts and targets
should not be confused.

>
> Probably it would be best to split the MS-DOS stuff in a separate file,
> and use the stub mechanism (no weak symbols, alas :)) to provide a
> default implementation that just returns BIOS_ATA_TRANSLATION_NONE.

A global variable, initialized by target specific code would work too.

>
>>> +            /* on s390 there is no BIOS doing any kind of translation */
>>> +            translation = BIOS_ATA_TRANSLATION_NONE;
>>> +#else
>>>              if (heads > 16) {
>>>                  /* LCHS guess with heads > 16 means that a BIOS LBA
>>>                     translation was active, so a standard physical disk
>>> @@ -158,6 +162,7 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>>                     the logical geometry */
>>>                  translation = BIOS_ATA_TRANSLATION_NONE;
>>>              }
>>> +#endif
>>>              *pheads = heads;
>>>              *psectors = sectors;
>>>              *pcylinders = cylinders;
>>> --
>>> 1.7.10.1
>>>
>>>
>>
>>
>

Re: [Qemu-devel] [PATCH 2/3] hd-geometry.c/s390: Disable geometry translation

[Christian Borntraeger]

On 18/11/12 20:14, Blue Swirl wrote:
> On Sun, Nov 18, 2012 at 4:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 17/11/2012 17:07, Blue Swirl ha scritto:
>>> On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
>>> <borntraeger@de.ibm.com> wrote:
>>>> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>>
>>>> This patch disables the translation of geometry information read from
>>>> disks on s390. On s390 such translations lead to wrong geometries being
>>>> advertized to the guest because there is no entity doing these kinds
>>>> of translation on this architecture.
>>>>
>>>> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>  hw/hd-geometry.c |    5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>>>> index 4cf040d..db1dc81 100644
>>>> --- a/hw/hd-geometry.c
>>>> +++ b/hw/hd-geometry.c
>>>> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>>>                  continue;
>>>>              }
>>>>
>>>> +#ifdef __s390__
>>>
>>> No, this would make all system emulators (e.g. x86) on s390 host to
>>> use this translation, not just s390. I think you want to use #ifdef
>>> CONFIG_S390X instead.
>>
>> This symbol is not available because this file is compiled just once.
>> Which non-x86 targets actually care about translation stuff?
> 
> I meant TARGET_S390X, sorry. I have no idea, but hosts and targets
> should not be confused.

Would a modified patch with TARGET_S390X be acceptable?

Re: [Qemu-devel] [PATCH 2/3] hd-geometry.c/s390: Disable geometry translation

[Alexander Graf]

On 19.11.2012, at 11:59, Christian Borntraeger wrote:

> On 18/11/12 20:14, Blue Swirl wrote:
>> On Sun, Nov 18, 2012 at 4:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 17/11/2012 17:07, Blue Swirl ha scritto:
>>>> On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
>>>> <borntraeger@de.ibm.com> wrote:
>>>>> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>>> 
>>>>> This patch disables the translation of geometry information read from
>>>>> disks on s390. On s390 such translations lead to wrong geometries being
>>>>> advertized to the guest because there is no entity doing these kinds
>>>>> of translation on this architecture.
>>>>> 
>>>>> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>>> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> ---
>>>>> hw/hd-geometry.c |    5 +++++
>>>>> 1 file changed, 5 insertions(+)
>>>>> 
>>>>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>>>>> index 4cf040d..db1dc81 100644
>>>>> --- a/hw/hd-geometry.c
>>>>> +++ b/hw/hd-geometry.c
>>>>> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>>>>                 continue;
>>>>>             }
>>>>> 
>>>>> +#ifdef __s390__
>>>> 
>>>> No, this would make all system emulators (e.g. x86) on s390 host to
>>>> use this translation, not just s390. I think you want to use #ifdef
>>>> CONFIG_S390X instead.
>>> 
>>> This symbol is not available because this file is compiled just once.
>>> Which non-x86 targets actually care about translation stuff?
>> 
>> I meant TARGET_S390X, sorry. I have no idea, but hosts and targets
>> should not be confused.
> 
> Would a modified patch with TARGET_S390X be acceptable?

If TARGET_S390X is defined in that file (read: it's compiled individually for every target), yes. Otherwise please make this a runtime check and set the runtime checked variable in code that is target specific.


Alex

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

[Markus Armbruster]

Christian Borntraeger <borntraeger@de.ibm.com> writes:

> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>
> This patch extends the function guess_disk_lchs. If no geo could
> be derived from reading disk content, HDIO_GETGEO ioctl is issued.
> If this is not successful (e.g. image files) geometry is derived
> from the size of the disk (as before). To achieve this, the MSDOS
> partition table reading logic and the translation computation logic
> have been moved into a separate function guess_disk_msdosgeo. The
> HDIO_GETGEO logic is captured in a new function guess_disk_ioctlgeo.
> guess_disk_lchs then encapsulates both (the overall guessing logic).
> 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 |  124 ++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 87 insertions(+), 37 deletions(-)
>
> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
> index 1cdb9fb..4cf040d 100644
> --- a/hw/hd-geometry.c
> +++ b/hw/hd-geometry.c
> @@ -33,6 +33,10 @@
>  #include "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,13 +51,65 @@ 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)
> +{
> +    uint64_t nb_sectors;
> +    int cylinders;
> +
> +    bdrv_get_geometry(bs, &nb_sectors);
> +
> +    cylinders = nb_sectors / (16 * 63);
> +    if (cylinders > 16383) {
> +        cylinders = 16383;
> +    } else if (cylinders < 2) {
> +        cylinders = 2;
> +    }
> +    *pcyls = cylinders;
> +    *pheads = 16;
> +    *psecs = 63;
> +}

Code motion, because you're reordering functions.  Best avoided, unless
it really helps readability.

> +
> +/* 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_ioctlgeo(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;
> +    trace_hd_geometry_lchs_guess(bs, *pcylinders, *pheads, *psectors);

Abuse of existing trace point.  Please define a new one!

> +    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,
> -                           int *pcylinders, int *pheads, int *psectors)
> +static int guess_disk_msdosgeo(BlockDriverState *bs,
> +                               uint32_t *pcylinders, uint32_t *pheads,
> +                               uint32_t *psectors, int *ptranslation)

I don't like your new name.

There are two kinds of geometry in the DOS world:

* Physical: property of the disk, used for CHS addressing of disk
  sectors when communicating with the disk, say via ATA.  Obsolete in
  ATA, superseded by LBA.  This is the common meaning of geometry.

* Logical: something the PC BIOS makes up to make the best use of its
  ill-designed int 13h CHS addressing.  Used only by really old software
  like DOS.

The old function name makes it explicit that this function is about
*logical* CHS, unlike the other functions in this file.

I'd be fine with _dos_lchs, or _pc_lchs, or leaving it as _lchs.

>  {
>      uint8_t buf[BDRV_SECTOR_SIZE];
> -    int i, heads, sectors, cylinders;
> +    int i, translation;
> +    uint32_t heads, sectors, cylinders;
>      struct partition *p;
>      uint32_t nr_sects;
>      uint64_t nb_sectors;
> @@ -87,9 +143,26 @@ static int guess_disk_lchs(BlockDriverState *bs,
>              if (cylinders < 1 || cylinders > 16383) {
>                  continue;
>              }
> +
> +            if (heads > 16) {
> +                /* LCHS guess with heads > 16 means that a BIOS LBA
> +                   translation was active, so a standard physical disk
> +                   geometry is OK */
> +                guess_chs_for_size(bs, &cylinders, &heads, &sectors);
> +                translation = cylinders * heads <= 131072
> +                    ? BIOS_ATA_TRANSLATION_LARGE
> +                    : BIOS_ATA_TRANSLATION_LBA;
> +            } else {
> +                /* LCHS guess with heads <= 16: use as physical geometry */
> +                /* disable any translation to be in sync with
> +                   the logical geometry */
> +                translation = BIOS_ATA_TRANSLATION_NONE;
> +            }

You're undoing my work separation of guessing L-CHS from the DOS
partition table and choosing a translation.  Why?

>              *pheads = heads;
>              *psectors = sectors;
>              *pcylinders = cylinders;
> +            *ptranslation = translation;
> +
>              trace_hd_geometry_lchs_guess(bs, cylinders, heads, sectors);
>              return 0;
>          }
> @@ -97,51 +170,28 @@ static int guess_disk_lchs(BlockDriverState *bs,
>      return -1;
>  }
>  
> -static void guess_chs_for_size(BlockDriverState *bs,
> -                uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs)
> -{
> -    uint64_t nb_sectors;
> -    int cylinders;
> -
> -    bdrv_get_geometry(bs, &nb_sectors);
> -
> -    cylinders = nb_sectors / (16 * 63);
> -    if (cylinders > 16383) {
> -        cylinders = 16383;
> -    } else if (cylinders < 2) {
> -        cylinders = 2;
> +/* try to guess the disk logical geometry
> +   Return 0 if OK, -1 if could not guess */
> +static int guess_disk_lchs(BlockDriverState *bs,
> +                           uint32_t *pcylinders, uint32_t *pheads, 
> +                           uint32_t *psectors, int *ptranslation) {
> +    if (!guess_disk_msdosgeo(bs, pcylinders, pheads, psectors, ptranslation)) {
> +        return 0;
>      }
> -    *pcyls = cylinders;
> -    *pheads = 16;
> -    *psecs = 63;
> +
> +    return guess_disk_ioctlgeo(bs, pcylinders, pheads, psectors, ptranslation);

Are you sure HDIO_GETGEO returns *logical* geometry?

If not, the function name and comment lies.

>  }
>  
>  void hd_geometry_guess(BlockDriverState *bs,
>                         uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
>                         int *ptrans)
>  {
> -    int cylinders, heads, secs, translation;
> +    int translation;
>  
> -    if (guess_disk_lchs(bs, &cylinders, &heads, &secs) < 0) {
> +    if (guess_disk_lchs(bs, pcyls, pheads, psecs, &translation) < 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);
> -    } else if (heads > 16) {
> -        /* LCHS guess with heads > 16 means that a BIOS LBA
> -           translation was active, so a standard physical disk
> -           geometry is OK */
> -        guess_chs_for_size(bs, pcyls, pheads, psecs);
> -        translation = *pcyls * *pheads <= 131072
> -            ? BIOS_ATA_TRANSLATION_LARGE
> -            : BIOS_ATA_TRANSLATION_LBA;
> -    } else {
> -        /* LCHS guess with heads <= 16: use as physical geometry */
> -        *pcyls = cylinders;
> -        *pheads = heads;
> -        *psecs = secs;
> -        /* disable any translation to be in sync with
> -           the logical geometry */
> -        translation = BIOS_ATA_TRANSLATION_NONE;
>      }
>      if (ptrans) {
>          *ptrans = translation;

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

[Christian Borntraeger]

On 19/11/12 16:08, Markus Armbruster wrote:
> You're undoing my work separation of guessing L-CHS from the DOS
> partition table and choosing a translation.  Why?

Might be my fault during rebasing. Will check with Einar.