[Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Ekaterina Tumanova]

Hi folks,

I'm sorry for the recent spam. I messed up during code submission last time.
So please ignore any previous notes you received from me and answer only to
this thread.

This is the rework of the geometry+blocksize patch, which was
recently discussed here:
http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01148.html

Markus suggested that we only detect blocksize and geometry for DASDs.

According to this agreement new version contains DASD special casing.
The driver methods are implemented only for "host_device" and inner hdev_xxx
functions check if the backing storage is a DASD by means of
BIODASDINFO2 ioctl.

Original patchset can be found here:
http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03791.html

Ekaterina Tumanova (6):
  geometry: add bdrv functions for geometry and blocksize
  geometry: Detect blocksize via ioctls in separate static functions
  geometry: Add driver methods to probe blocksizes and geometry
  geometry: Add block-backend wrappers for geometry probing
  geometry: Call backend function to detect geometry and blocksize
  geometry: Target specific hook for s390x in geometry guessing

 block.c                        |  26 +++++++++
 block/block-backend.c          |  10 ++++
 block/raw-posix.c              | 123 ++++++++++++++++++++++++++++++++++-------
 block/raw_bsd.c                |  12 ++++
 hw/block/Makefile.objs         |   6 +-
 hw/block/block.c               |  11 ++++
 hw/block/hd-geometry.c         |  43 ++++++++++++--
 hw/block/virtio-blk.c          |   1 +
 include/block/block.h          |  20 +++++++
 include/block/block_int.h      |   3 +
 include/hw/block/block.h       |   1 +
 include/sysemu/block-backend.h |   2 +
 12 files changed, 234 insertions(+), 24 deletions(-)

-- 
1.8.5.5

[Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize

[Ekaterina Tumanova]

Add driver functions for geometry and blocksize detection

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
---
 block.c                   | 26 ++++++++++++++++++++++++++
 include/block/block.h     | 20 ++++++++++++++++++++
 include/block/block_int.h |  3 +++
 3 files changed, 49 insertions(+)

diff --git a/block.c b/block.c
index a612594..5df35cf 100644
--- a/block.c
+++ b/block.c
@@ -548,6 +548,32 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 }
 
+struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    struct ProbeBlockSize err_geo = { .rc = -1 };
+
+    assert(drv != NULL);
+    if (drv->bdrv_probe_blocksizes) {
+        return drv->bdrv_probe_blocksizes(bs);
+    }
+
+    return err_geo;
+}
+
+struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    struct ProbeGeometry err_geo = { .rc = -1 };
+
+    assert(drv != NULL);
+    if (drv->bdrv_probe_geometry) {
+        return drv->bdrv_probe_geometry(bs);
+    }
+
+    return err_geo;
+}
+
 /*
  * Create a uniquely-named empty temporary file.
  * Return 0 upon success, otherwise a negative errno value.
diff --git a/include/block/block.h b/include/block/block.h
index 5450610..3287dbc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -60,6 +60,24 @@ typedef enum {
     BDRV_REQ_MAY_UNMAP    = 0x4,
 } BdrvRequestFlags;
 
+struct ProbeBlockSize {
+    int rc;
+    struct BlockSize {
+        uint16_t phys;
+        uint16_t log;
+    } size;
+};
+
+struct ProbeGeometry {
+    int rc;
+    struct HDGeometry {
+        uint32_t heads;
+        uint32_t sectors;
+        uint32_t cylinders;
+        uint32_t start;
+    } geo;
+};
+
 #define BDRV_O_RDWR        0x0002
 #define BDRV_O_SNAPSHOT    0x0008 /* open the file read only and save writes in a snapshot */
 #define BDRV_O_TEMPORARY   0x0010 /* delete the file after use */
@@ -538,6 +556,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
  * the old #AioContext is not executing.
  */
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
+struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs);
+struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs);
 
 void bdrv_io_plug(BlockDriverState *bs);
 void bdrv_io_unplug(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a1c17b9..830e564 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -271,6 +271,9 @@ struct BlockDriver {
     void (*bdrv_io_unplug)(BlockDriverState *bs);
     void (*bdrv_flush_io_queue)(BlockDriverState *bs);
 
+    struct ProbeBlockSize (*bdrv_probe_blocksizes)(BlockDriverState *bs);
+    struct ProbeGeometry (*bdrv_probe_geometry)(BlockDriverState *bs);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.8.5.5

[Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Ekaterina Tumanova]

Move the IOCTL calls that detect logical blocksize from raw_probe_alignment
into separate function (probe_logical_blocksize).
Introduce function which detect physical blocksize via IOCTL
(probe_physical_blocksize).
Both functions will be used in the next patch.

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
---
 block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index e100ae2..45f1d79 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char **filename)
 }
 #endif
 
-static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
+static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd)
 {
-    BDRVRawState *s = bs->opaque;
-    char *buf;
-    unsigned int sector_size;
-
-    /* For /dev/sg devices the alignment is not really used.
-       With buffered I/O, we don't have any restrictions. */
-    if (bs->sg || !s->needs_alignment) {
-        bs->request_alignment = 1;
-        s->buf_align = 1;
-        return;
-    }
+    unsigned int sector_size = 0;
 
     /* Try a few ioctls to get the right size */
-    bs->request_alignment = 0;
-    s->buf_align = 0;
-
 #ifdef BLKSSZGET
     if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
-        bs->request_alignment = sector_size;
+        return sector_size;
     }
 #endif
 #ifdef DKIOCGETBLOCKSIZE
     if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
-        bs->request_alignment = sector_size;
+        return sector_size;
     }
 #endif
 #ifdef DIOCGSECTORSIZE
     if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
-        bs->request_alignment = sector_size;
+        return sector_size;
     }
 #endif
 #ifdef CONFIG_XFS
     if (s->is_xfs) {
         struct dioattr da;
         if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
-            bs->request_alignment = da.d_miniosz;
+            sector_size = da.d_miniosz;
             /* The kernel returns wrong information for d_mem */
             /* s->buf_align = da.d_mem; */
+            return sector_size;
         }
     }
 #endif
 
+    return 0;
+}
+
+static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd)
+{
+    unsigned int blk_size = 0;
+#ifdef BLKPBSZGET
+    if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) {
+        return blk_size;
+    }
+#endif
+
+    return 0;
+}
+
+static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
+{
+    BDRVRawState *s = bs->opaque;
+    char *buf;
+
+    /* For /dev/sg devices the alignment is not really used.
+       With buffered I/O, we don't have any restrictions. */
+    if (bs->sg || !s->needs_alignment) {
+        bs->request_alignment = 1;
+        s->buf_align = 1;
+        return;
+    }
+
+    s->buf_align = 0;
+    /* Let's try to use the logical blocksize for the alignment. */
+    bs->request_alignment = probe_logical_blocksize(bs, fd);
+
     /* If we could not get the sizes so far, we can only guess them */
     if (!s->buf_align) {
         size_t align;
-- 
1.8.5.5

[Qemu-devel] [PATCH v2 3/6] geometry: Add driver methods to probe blocksizes and geometry

[Ekaterina Tumanova]

This patch introduces driver methods of defining disk blocksizes
(physical and logical) and hard drive geometry.
The method is only implemented for "host_device". For "raw" devices
driver calls child's method.
The detection will only work for DASD devices. In order to check that
a local CheckForDASD function was introduced, which calls BIODASDINFO2 ioctl
and returns 0 only if it succeeds.

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
---
 block/raw-posix.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/raw_bsd.c   | 12 ++++++++++
 2 files changed, 77 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 45f1d79..274ceda 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -56,6 +56,7 @@
 #include <linux/cdrom.h>
 #include <linux/fd.h>
 #include <linux/fs.h>
+#include <linux/hdreg.h>
 #ifndef FS_NOCOW_FL
 #define FS_NOCOW_FL                     0x00800000 /* Do not cow file */
 #endif
@@ -93,6 +94,10 @@
 #include <xfs/xfs.h>
 #endif
 
+#ifdef __s390__
+#include <asm/dasd.h>
+#endif
+
 //#define DEBUG_FLOPPY
 
 //#define DEBUG_BLOCK
@@ -678,6 +683,64 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
     bs->bl.opt_mem_alignment = s->buf_align;
 }
 
+static int CheckForDASD(int fd)
+{
+#ifdef BIODASDINFO2
+    struct dasd_information2_t info = {0};
+
+    return ioctl(fd, BIODASDINFO2, &info);
+#endif
+    return -1;
+}
+
+static struct ProbeBlockSize hdev_probe_blocksizes(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+    struct ProbeBlockSize block_sizes = {0};
+
+    block_sizes.rc = CheckForDASD(s->fd);
+    /* If DASD, get blocksizes */
+    if (block_sizes.rc == 0) {
+        block_sizes.size.log = probe_logical_blocksize(bs, s->fd);
+        block_sizes.size.phys = probe_physical_blocksize(bs, s->fd);
+    }
+
+    return block_sizes;
+}
+
+static struct ProbeGeometry hdev_probe_geometry(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+    struct ProbeGeometry geometry = {0};
+    struct hd_geometry ioctl_geo = {0};
+
+    geometry.rc = CheckForDASD(s->fd);
+    if (geometry.rc != 0) {
+        goto done;
+    }
+    /* If DASD, get it's geometry */
+    geometry.rc = ioctl(s->fd, HDIO_GETGEO, &ioctl_geo);
+    /* Do not return a geometry for partition */
+    if (ioctl_geo.start != 0) {
+        geometry.rc = -1;
+        goto done;
+    }
+    /* HDIO_GETGEO may return success even though geo contains zeros
+       (e.g. certain multipath setups) */
+    if (!ioctl_geo.heads || !ioctl_geo.sectors || !ioctl_geo.cylinders) {
+        geometry.rc = -1;
+        goto done;
+    }
+    if (geometry.rc == 0) {
+        geometry.geo.heads = (uint32_t)ioctl_geo.heads;
+        geometry.geo.sectors = (uint32_t)ioctl_geo.sectors;
+        geometry.geo.cylinders = (uint32_t)ioctl_geo.cylinders;
+        geometry.geo.start = (uint32_t)ioctl_geo.start;
+    }
+done:
+   return geometry;
+}
+
 static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
 {
     int ret;
@@ -2128,6 +2191,8 @@ static BlockDriver bdrv_host_device = {
     .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
+    .bdrv_probe_blocksizes = hdev_probe_blocksizes,
+    .bdrv_probe_geometry = hdev_probe_geometry,
 
     .bdrv_detach_aio_context = raw_detach_aio_context,
     .bdrv_attach_aio_context = raw_attach_aio_context,
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 401b967..d164eba 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -173,6 +173,16 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
     return 1;
 }
 
+static struct ProbeBlockSize raw_probe_blocksizes(BlockDriverState *bs)
+{
+    return bdrv_probe_blocksizes(bs->file);
+}
+
+static struct ProbeGeometry raw_probe_geometry(BlockDriverState *bs)
+{
+    return bdrv_probe_geometry(bs->file);
+}
+
 static BlockDriver bdrv_raw = {
     .format_name          = "raw",
     .bdrv_probe           = &raw_probe,
@@ -190,6 +200,8 @@ static BlockDriver bdrv_raw = {
     .has_variable_length  = true,
     .bdrv_get_info        = &raw_get_info,
     .bdrv_refresh_limits  = &raw_refresh_limits,
+    .bdrv_probe_blocksizes = &raw_probe_blocksizes,
+    .bdrv_probe_geometry  = &raw_probe_geometry,
     .bdrv_is_inserted     = &raw_is_inserted,
     .bdrv_media_changed   = &raw_media_changed,
     .bdrv_eject           = &raw_eject,
-- 
1.8.5.5

[Qemu-devel] [PATCH v2 4/6] geometry: Add block-backend wrappers for geometry probing

[Ekaterina Tumanova]

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
---
 block/block-backend.c          | 10 ++++++++++
 include/sysemu/block-backend.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index d0692b1..6717301 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -629,3 +629,13 @@ void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
 {
     return qemu_aio_get(aiocb_info, blk_bs(blk), cb, opaque);
 }
+
+struct ProbeBlockSize blk_probe_blocksizes(BlockBackend *blk)
+{
+    return bdrv_probe_blocksizes(blk->bs);
+}
+
+struct ProbeGeometry blk_probe_geometry(BlockBackend *blk)
+{
+    return bdrv_probe_geometry(blk->bs);
+}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 52d13c1..e8b497a 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -138,5 +138,7 @@ BlockAcctStats *blk_get_stats(BlockBackend *blk);
 
 void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
                   BlockCompletionFunc *cb, void *opaque);
+struct ProbeBlockSize blk_probe_blocksizes(BlockBackend *blk);
+struct ProbeGeometry blk_probe_geometry(BlockBackend *blk);
 
 #endif
-- 
1.8.5.5

[Qemu-devel] [PATCH v2 5/6] geometry: Call backend function to detect geometry and blocksize

[Ekaterina Tumanova]

hd_geometry_guess function autodetects the drive geometry. This patch
adds a block backend call, that probes the backing device geometry.
If the inner driver method is implemented and succeeds (currently only DASDs),
the blkconf_geometry will pass-through the backing device geometry.

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
---
 hw/block/block.c         | 11 +++++++++++
 hw/block/hd-geometry.c   |  9 +++++++++
 hw/block/virtio-blk.c    |  1 +
 include/hw/block/block.h |  1 +
 4 files changed, 22 insertions(+)

diff --git a/hw/block/block.c b/hw/block/block.c
index a625773..f1d29bc 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -25,6 +25,17 @@ void blkconf_serial(BlockConf *conf, char **serial)
     }
 }
 
+void blkconf_blocksizes(BlockConf *conf)
+{
+    BlockBackend *blk = conf->blk;
+    struct ProbeBlockSize blocksize = blk_probe_blocksizes(blk);
+
+    if (blocksize.rc == 0) {
+        conf->physical_block_size = blocksize.size.phys;
+        conf->logical_block_size = blocksize.size.log;
+    }
+}
+
 void blkconf_geometry(BlockConf *conf, int *ptrans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp)
diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index 6fcf74d..4972114 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -121,6 +121,14 @@ void hd_geometry_guess(BlockBackend *blk,
                        int *ptrans)
 {
     int cylinders, heads, secs, translation;
+    struct ProbeGeometry geometry = blk_probe_geometry(blk);
+
+    if (geometry.rc == 0) {
+        *pcyls = geometry.geo.cylinders;
+        *psecs = geometry.geo.sectors;
+        *pheads = geometry.geo.heads;
+        goto done;
+    }
 
     if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
         /* no LCHS guess: use a standard physical disk geometry  */
@@ -143,6 +151,7 @@ void hd_geometry_guess(BlockBackend *blk,
            the logical geometry */
         translation = BIOS_ATA_TRANSLATION_NONE;
     }
+done:
     if (ptrans) {
         *ptrans = translation;
     }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b19b102..6f01565 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -745,6 +745,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
+    blkconf_blocksizes(&conf->conf);
 
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
                 sizeof(struct virtio_blk_config));
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 0d0ce9a..856bf75 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -63,6 +63,7 @@ void blkconf_serial(BlockConf *conf, char **serial);
 void blkconf_geometry(BlockConf *conf, int *trans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp);
+void blkconf_blocksizes(BlockConf *conf);
 
 /* Hard disk geometry */
 
-- 
1.8.5.5

[Qemu-devel] [PATCH v2 6/6] geometry: Target specific hook for s390x in geometry guessing

[Ekaterina Tumanova]

For target-s390x, the behaviour is chosen as follows:
If no geo could be guessed from backing device, guess_disk_lchs (partition
table guessing) is called.
If this is not successful (e.g. image files) geometry is derived from the
size of the disk (as before).
We have no translation on s390, so we simply set in to NONE.

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
---
 hw/block/Makefile.objs |  6 +++++-
 hw/block/hd-geometry.c | 36 +++++++++++++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index d4c3ab7..1f7da7a 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-y += block.o cdrom.o hd-geometry.o
+common-obj-y += block.o cdrom.o
 common-obj-$(CONFIG_FDC) += fdc.o
 common-obj-$(CONFIG_SSI_M25P80) += m25p80.o
 common-obj-$(CONFIG_NAND) += nand.o
@@ -13,3 +13,7 @@ obj-$(CONFIG_SH4) += tc58128.o
 
 obj-$(CONFIG_VIRTIO) += virtio-blk.o
 obj-$(CONFIG_VIRTIO) += dataplane/
+
+# geometry is target/architecture dependent and therefore needs to be built
+# for every target platform
+obj-y += hd-geometry.o
diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index 4972114..b462225 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -49,11 +49,12 @@ struct partition {
 
 /* 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(BlockBackend *blk,
-                           int *pcylinders, int *pheads, int *psectors)
+static int guess_disk_lchs(BlockBackend *blk, uint32_t *pcylinders,
+                           uint32_t *pheads, uint32_t *psectors)
 {
     uint8_t buf[BDRV_SECTOR_SIZE];
-    int i, heads, sectors, cylinders;
+    int i;
+    uint32_t heads, sectors, cylinders;
     struct partition *p;
     uint32_t nr_sects;
     uint64_t nb_sectors;
@@ -116,11 +117,11 @@ static void guess_chs_for_size(BlockBackend *blk,
     *psecs = 63;
 }
 
+#ifdef TARGET_S390X
 void hd_geometry_guess(BlockBackend *blk,
                        uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
                        int *ptrans)
 {
-    int cylinders, heads, secs, translation;
     struct ProbeGeometry geometry = blk_probe_geometry(blk);
 
     if (geometry.rc == 0) {
@@ -129,7 +130,32 @@ void hd_geometry_guess(BlockBackend *blk,
         *pheads = geometry.geo.heads;
         goto done;
     }
+    if (guess_disk_lchs(blk, pcyls, pheads, psecs) == 0) {
+        goto done;
+    }
+    guess_chs_for_size(blk, pcyls, pheads, psecs);
+done:
+    if (ptrans) {
+        *ptrans = BIOS_ATA_TRANSLATION_NONE;
+    }
 
+    trace_hd_geometry_guess(blk, *pcyls, *pheads, *psecs,
+                            BIOS_ATA_TRANSLATION_NONE);
+}
+#else
+void hd_geometry_guess(BlockBackend *blk,
+                       uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
+                       int *ptrans)
+{
+    int cylinders, heads, secs, translation;
+    struct ProbeGeometry geometry = blk_probe_geometry(blk);
+
+    if (geometry.rc == 0) {
+        *pcyls = geometry.geo.cylinders;
+        *psecs = geometry.geo.sectors;
+        *pheads = geometry.geo.heads;
+        goto done;
+    }
     if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
         /* no LCHS guess: use a standard physical disk geometry  */
         guess_chs_for_size(blk, pcyls, pheads, psecs);
@@ -157,7 +183,7 @@ done:
     }
     trace_hd_geometry_guess(blk, *pcyls, *pheads, *psecs, translation);
 }
-
+#endif
 int hd_bios_chs_auto_trans(uint32_t cyls, uint32_t heads, uint32_t secs)
 {
     return cyls <= 1024 && heads <= 16 && secs <= 63
-- 
1.8.5.5

Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Christian Borntraeger]

Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova:
> Hi folks,
> 
> I'm sorry for the recent spam. I messed up during code submission last time.
> So please ignore any previous notes you received from me and answer only to
> this thread.
> 
> This is the rework of the geometry+blocksize patch, which was
> recently discussed here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01148.html
> 
> Markus suggested that we only detect blocksize and geometry for DASDs.
> 
> According to this agreement new version contains DASD special casing.
> The driver methods are implemented only for "host_device" and inner hdev_xxx
> functions check if the backing storage is a DASD by means of
> BIODASDINFO2 ioctl.
> 
> Original patchset can be found here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03791.html
> 
> Ekaterina Tumanova (6):
>   geometry: add bdrv functions for geometry and blocksize
>   geometry: Detect blocksize via ioctls in separate static functions
>   geometry: Add driver methods to probe blocksizes and geometry
>   geometry: Add block-backend wrappers for geometry probing
>   geometry: Call backend function to detect geometry and blocksize
>   geometry: Target specific hook for s390x in geometry guessing
> 
>  block.c                        |  26 +++++++++
>  block/block-backend.c          |  10 ++++
>  block/raw-posix.c              | 123 ++++++++++++++++++++++++++++++++++-------
>  block/raw_bsd.c                |  12 ++++
>  hw/block/Makefile.objs         |   6 +-
>  hw/block/block.c               |  11 ++++
>  hw/block/hd-geometry.c         |  43 ++++++++++++--
>  hw/block/virtio-blk.c          |   1 +
>  include/block/block.h          |  20 +++++++
>  include/block/block_int.h      |   3 +
>  include/hw/block/block.h       |   1 +
>  include/sysemu/block-backend.h |   2 +
>  12 files changed, 234 insertions(+), 24 deletions(-)
> 

I can confirm that it makes dasd devices on s390 work (partition detection is fine, so geometry/sector size must be as well)

This patch set needs to be fixed for i386, though:

/home/cborntra/REPOS/qemu/hw/block/hd-geometry.c: In function 'hd_geometry_guess':
/home/cborntra/REPOS/qemu/hw/block/hd-geometry.c:159:5: error: pointer targets in passing argument 2 of 'guess_disk_lchs' differ in signedness [-Werror=pointer-sign]
     if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
     ^
/home/cborntra/REPOS/qemu/hw/block/hd-geometry.c:52:12: note: expected 'uint32_t *' but argument is of type 'int *'
 static int guess_disk_lchs(BlockBackend *blk, uint32_t *pcylinders,

[Qemu-devel] [PATCH] geometry: fix i386 compilation

[Ekaterina Tumanova]

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
---
 hw/block/hd-geometry.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index b462225..905d2c6 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -147,7 +147,8 @@ void hd_geometry_guess(BlockBackend *blk,
                        uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
                        int *ptrans)
 {
-    int cylinders, heads, secs, translation;
+    uint32_t cylinders, heads, secs;
+    int translation = BIOS_ATA_TRANSLATION_NONE;
     struct ProbeGeometry geometry = blk_probe_geometry(blk);
 
     if (geometry.rc == 0) {
@@ -173,9 +174,6 @@ void hd_geometry_guess(BlockBackend *blk,
         *pcyls = cylinders;
         *pheads = heads;
         *psecs = secs;
-        /* disable any translation to be in sync with
-           the logical geometry */
-        translation = BIOS_ATA_TRANSLATION_NONE;
     }
 done:
     if (ptrans) {
-- 
1.8.5.5

Re: [Qemu-devel] [PATCH] geometry: fix i386 compilation

[Peter Maydell]

On 19 November 2014 14:01, Ekaterina Tumanova
<tumanova@linux.vnet.ibm.com> wrote:
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>

Could you give the compiler error/warning message, please
(and the compiler version)?

These changes look sensible but it's not clear to me why
the current code would cause a compilation failure or why
that would be specific to i386...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] geometry: fix i386 compilation

[Cornelia Huck]

On Wed, 19 Nov 2014 14:40:07 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 19 November 2014 14:01, Ekaterina Tumanova
> <tumanova@linux.vnet.ibm.com> wrote:
> > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> 
> Could you give the compiler error/warning message, please
> (and the compiler version)?
> 
> These changes look sensible but it's not clear to me why
> the current code would cause a compilation failure or why
> that would be specific to i386...

This patch is referring to Kate's patch set "[PATCH v2 0/6] Geometry
and blocksize support for backing devices", which did not compile on
i386 (see <546C8161.10100@de.ibm.com>).

Re: [Qemu-devel] [PATCH] geometry: fix i386 compilation

[Kevin Wolf]

Am 19.11.2014 um 16:04 hat Cornelia Huck geschrieben:
> On Wed, 19 Nov 2014 14:40:07 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On 19 November 2014 14:01, Ekaterina Tumanova
> > <tumanova@linux.vnet.ibm.com> wrote:
> > > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> > 
> > Could you give the compiler error/warning message, please
> > (and the compiler version)?
> > 
> > These changes look sensible but it's not clear to me why
> > the current code would cause a compilation failure or why
> > that would be specific to i386...
> 
> This patch is referring to Kate's patch set "[PATCH v2 0/6] Geometry
> and blocksize support for backing devices", which did not compile on
> i386 (see <546C8161.10100@de.ibm.com>).

Where does it need to be squashed in? We want to keep the tree
bisectable, so breaking the build and then fixing it at the end of the
series isn't an option.

Kevin

Re: [Qemu-devel] [PATCH] geometry: fix i386 compilation

[Christian Borntraeger]

Am 20.11.2014 um 17:18 schrieb Kevin Wolf:
> Am 19.11.2014 um 16:04 hat Cornelia Huck geschrieben:
>> On Wed, 19 Nov 2014 14:40:07 +0000
>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>>> On 19 November 2014 14:01, Ekaterina Tumanova
>>> <tumanova@linux.vnet.ibm.com> wrote:
>>>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>>
>>> Could you give the compiler error/warning message, please
>>> (and the compiler version)?
>>>
>>> These changes look sensible but it's not clear to me why
>>> the current code would cause a compilation failure or why
>>> that would be specific to i386...
>>
>> This patch is referring to Kate's patch set "[PATCH v2 0/6] Geometry
>> and blocksize support for backing devices", which did not compile on
>> i386 (see <546C8161.10100@de.ibm.com>).
> 
> Where does it need to be squashed in? We want to keep the tree
> bisectable, so breaking the build and then fixing it at the end of the
> series isn't an option.

I think Kate just wanted to avoid spamming the list with v3 if only one 
patch is fixed up when there was no review comment about the content of
the series. Using the -in-reply-to option plus a comment in the patch
would have avoided the confusion - I guess.

Christian

Re: [Qemu-devel] [PATCH] geometry: fix i386 compilation

[Ekaterina Tumanova]

On 11/20/2014 07:18 PM, Kevin Wolf wrote:
> Am 19.11.2014 um 16:04 hat Cornelia Huck geschrieben:
>> On Wed, 19 Nov 2014 14:40:07 +0000
>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>>> On 19 November 2014 14:01, Ekaterina Tumanova
>>> <tumanova@linux.vnet.ibm.com> wrote:
>>>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>>
>>> Could you give the compiler error/warning message, please
>>> (and the compiler version)?
>>>
>>> These changes look sensible but it's not clear to me why
>>> the current code would cause a compilation failure or why
>>> that would be specific to i386...
>>
>> This patch is referring to Kate's patch set "[PATCH v2 0/6] Geometry
>> and blocksize support for backing devices", which did not compile on
>> i386 (see <546C8161.10100@de.ibm.com>).
>
> Where does it need to be squashed in? We want to keep the tree
> bisectable, so breaking the build and then fixing it at the end of the
> series isn't an option.
>
> Kevin
>

This is just a small -in-reply-to patch to Christian's note, fixing the
patch set compilation on i386.
It should be squashed into the last patch in a series:
"[PATCH v2 6/6] geometry: Target specific hook for s390x in geometry
guessing".
I just don't want to send a new version before receiving any substantial
comments.

Kate.

Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize

[Christian Borntraeger]

Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova:
> Add driver functions for geometry and blocksize detection
> 
[...]
>  /*
>   * Create a uniquely-named empty temporary file.
>   * Return 0 upon success, otherwise a negative errno value.
> diff --git a/include/block/block.h b/include/block/block.h
> index 5450610..3287dbc 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -60,6 +60,24 @@ typedef enum {
>      BDRV_REQ_MAY_UNMAP    = 0x4,
>  } BdrvRequestFlags;
> 

question for Kevin/Stefan/Marcus,
is belows rc type of return code ok for you (see patch 5 for its usage)?


> +struct ProbeBlockSize {
> +    int rc;
> +    struct BlockSize {
> +        uint16_t phys;
> +        uint16_t log;
> +    } size;
> +};

Kate, can you make this a typedef so that you dont need to drag along "struct"?


> +
> +struct ProbeGeometry {
> +    int rc;
> +    struct HDGeometry {
> +        uint32_t heads;
> +        uint32_t sectors;
> +        uint32_t cylinders;
> +        uint32_t start;
> +    } geo;
> +};

dito


Otherwise looks sane.

Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Christian Borntraeger]

Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova:
> Move the IOCTL calls that detect logical blocksize from raw_probe_alignment
> into separate function (probe_logical_blocksize).
> Introduce function which detect physical blocksize via IOCTL
> (probe_physical_blocksize).
> Both functions will be used in the next patch.
> 
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>

>From what I can tell this should be a no-op for raw_probe_alignment.

probe_physical_blocksize looks also good. When this patch is applied stand-alone,
gcc will complain about a defined but unused function, though.

So we might want to move this function into patch 3 or just add an __attribute__((unused))
here (and remove that in patch 3). Or just leave it as is.

Otherwise
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e100ae2..45f1d79 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char **filename)
>  }
>  #endif
> 
> -static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> +static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd)
>  {
> -    BDRVRawState *s = bs->opaque;
> -    char *buf;
> -    unsigned int sector_size;
> -
> -    /* For /dev/sg devices the alignment is not really used.
> -       With buffered I/O, we don't have any restrictions. */
> -    if (bs->sg || !s->needs_alignment) {
> -        bs->request_alignment = 1;
> -        s->buf_align = 1;
> -        return;
> -    }
> +    unsigned int sector_size = 0;
> 
>      /* Try a few ioctls to get the right size */
> -    bs->request_alignment = 0;
> -    s->buf_align = 0;
> -
>  #ifdef BLKSSZGET
>      if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef DKIOCGETBLOCKSIZE
>      if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef DIOCGSECTORSIZE
>      if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef CONFIG_XFS
>      if (s->is_xfs) {
>          struct dioattr da;
>          if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
> -            bs->request_alignment = da.d_miniosz;
> +            sector_size = da.d_miniosz;
>              /* The kernel returns wrong information for d_mem */
>              /* s->buf_align = da.d_mem; */
> +            return sector_size;
>          }
>      }
>  #endif
> 
> +    return 0;
> +}
> +
> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd)
> +{
> +    unsigned int blk_size = 0;
> +#ifdef BLKPBSZGET
> +    if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) {
> +        return blk_size;
> +    }
> +#endif
> +
> +    return 0;
> +}
> +
> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    char *buf;
> +
> +    /* For /dev/sg devices the alignment is not really used.
> +       With buffered I/O, we don't have any restrictions. */
> +    if (bs->sg || !s->needs_alignment) {
> +        bs->request_alignment = 1;
> +        s->buf_align = 1;
> +        return;
> +    }
> +
> +    s->buf_align = 0;
> +    /* Let's try to use the logical blocksize for the alignment. */
> +    bs->request_alignment = probe_logical_blocksize(bs, fd);
> +
>      /* If we could not get the sizes so far, we can only guess them */
>      if (!s->buf_align) {
>          size_t align;
> 

Re: [Qemu-devel] [PATCH v2 3/6] geometry: Add driver methods to probe blocksizes and geometry

[Christian Borntraeger]

Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova:
[...]

> @@ -678,6 +683,64 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>      bs->bl.opt_mem_alignment = s->buf_align;
>  }
> 
> +static int CheckForDASD(int fd)

no camelcase for function names check_for_dasd 

[...]

Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Christian Borntraeger]

Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova:
> Hi folks,
> 
> I'm sorry for the recent spam. I messed up during code submission last time.
> So please ignore any previous notes you received from me and answer only to
> this thread.
> 
> This is the rework of the geometry+blocksize patch, which was
> recently discussed here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01148.html
> 
> Markus suggested that we only detect blocksize and geometry for DASDs.
> 
> According to this agreement new version contains DASD special casing.
> The driver methods are implemented only for "host_device" and inner hdev_xxx
> functions check if the backing storage is a DASD by means of
> BIODASDINFO2 ioctl.
> 
> Original patchset can be found here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03791.html

I have done some review on the patches itself, but the overall direction if this is the way to do for the block layer has to come from Kevin, Stefan or Markus.

Christian

Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

On Fri, Nov 21, 2014 at 11:17:02AM +0100, Christian Borntraeger wrote:
> Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova:
> > Move the IOCTL calls that detect logical blocksize from raw_probe_alignment
> > into separate function (probe_logical_blocksize).
> > Introduce function which detect physical blocksize via IOCTL
> > (probe_physical_blocksize).
> > Both functions will be used in the next patch.
> > 
> > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> 
> From what I can tell this should be a no-op for raw_probe_alignment.
> 
> probe_physical_blocksize looks also good. When this patch is applied stand-alone,
> gcc will complain about a defined but unused function, though.
> 
> So we might want to move this function into patch 3 or just add an __attribute__((unused))
> here (and remove that in patch 3). Or just leave it as is.

Please move probe_physical_blocksize() to Patch 3 because some QEMU
builds default to -Werror where this patch causes a build failure.

(In order for git-bisect(1) to work patches must not break the build.)

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote:
> Hi folks,
> 
> I'm sorry for the recent spam. I messed up during code submission last time.
> So please ignore any previous notes you received from me and answer only to
> this thread.
> 
> This is the rework of the geometry+blocksize patch, which was
> recently discussed here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01148.html
> 
> Markus suggested that we only detect blocksize and geometry for DASDs.
> 
> According to this agreement new version contains DASD special casing.
> The driver methods are implemented only for "host_device" and inner hdev_xxx
> functions check if the backing storage is a DASD by means of
> BIODASDINFO2 ioctl.
> 
> Original patchset can be found here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03791.html

This is description is mainly a changelog.  Links to previous email
threads are useful for additional info but please include a
self-contained description of the series and the rationale behind it.

Comments:

1. This series overrides the logical_block_size and
   physical_block_size options for raw images on DASD devices.  Users
   expect their command-line options to be honored, so the options
   should not be overriden if they have been given on the command-line.

2. Only virtio_blk is modified, this is inconsistent.  All emulated
   storage controllers using BlockConf have the same block size
   probing behavior.

3. Why does s390 need to customize hd_geometry_guess()?

4. Please use scripts/checkpatch.pl to check coding style.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Ekaterina Tumanova]

On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote:
> On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote:
>> Hi folks,
>>
>> I'm sorry for the recent spam. I messed up during code submission last time.
>> So please ignore any previous notes you received from me and answer only to
>> this thread.
>>
>> This is the rework of the geometry+blocksize patch, which was
>> recently discussed here:
>> http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01148.html
>>
>> Markus suggested that we only detect blocksize and geometry for DASDs.
>>
>> According to this agreement new version contains DASD special casing.
>> The driver methods are implemented only for "host_device" and inner hdev_xxx
>> functions check if the backing storage is a DASD by means of
>> BIODASDINFO2 ioctl.
>>
>> Original patchset can be found here:
>> http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03791.html
>
> This is description is mainly a changelog.  Links to previous email
> threads are useful for additional info but please include a
> self-contained description of the series and the rationale behind it.
>
will include into the next version

> Comments:
>
> 1. This series overrides the logical_block_size and
>     physical_block_size options for raw images on DASD devices.  Users
>     expect their command-line options to be honored, so the options
>     should not be overriden if they have been given on the command-line.
>
will fix that

> 2. Only virtio_blk is modified, this is inconsistent.  All emulated
>     storage controllers using BlockConf have the same block size
>     probing behavior.

I will add blkconf_blocksizes call to other BlockConf users.

>
> 3. Why does s390 need to customize hd_geometry_guess()?
>
Since hd_geometry_guess contains semantics of x86-specific LBA 
translation, we have to modify it not to get in the way of z
architecture

> 4. Please use scripts/checkpatch.pl to check coding style.
>
I did :)

Thanks a lot,
Kate.

Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize

[Markus Armbruster]

I'm sorry for the delay.  I got the flu and have difficulties thinking
straight for longer than a few minutes.

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> Add driver functions for geometry and blocksize detection
>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  block.c                   | 26 ++++++++++++++++++++++++++
>  include/block/block.h     | 20 ++++++++++++++++++++
>  include/block/block_int.h |  3 +++
>  3 files changed, 49 insertions(+)
>
> diff --git a/block.c b/block.c
> index a612594..5df35cf 100644
> --- a/block.c
> +++ b/block.c
> @@ -548,6 +548,32 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>      }
>  }
>  
> +struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs)
> +{
> +    BlockDriver *drv = bs->drv;
> +    struct ProbeBlockSize err_geo = { .rc = -1 };
> +
> +    assert(drv != NULL);
> +    if (drv->bdrv_probe_blocksizes) {
> +        return drv->bdrv_probe_blocksizes(bs);
> +    }
> +
> +    return err_geo;
> +}

I'm not sure about "probe".  Naming is hard.  "get"?

Squashing status and actual payload into a single struct to use as
return type isn't wrong, but unusual.  When the payload can't represent
failure conveniently, we usually return status, and write the payload to
a buffer provided by the caller, like this:

    int bdrv_get_blocksizes(BlockDriverState *bs,
                            uint16_t *physical_blk_sz,
                            uint16_t *logical_blk_sz)

Or, with a struct to hold both sizes:

    int bdrv_get_blocksizes(BlockDriverState *bs, BlockSizes *bsz)

Such a struct should ideally be used in other places where we store both
sizes.

A brief function contract comment wouldn't hurt.  Something like

    /*
     * Try to get @bs's logical and physical block size.
     * Block sizes are always a multiple of BDRV_SECTOR_SIZE.
     * On success, store them in ... and return 0.
     * On failure, return -errno.
     */

> +
> +struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs)
> +{
> +    BlockDriver *drv = bs->drv;
> +    struct ProbeGeometry err_geo = { .rc = -1 };
> +
> +    assert(drv != NULL);
> +    if (drv->bdrv_probe_geometry) {
> +        return drv->bdrv_probe_geometry(bs);
> +    }
> +
> +    return err_geo;
> +}
> +
>  /*
>   * Create a uniquely-named empty temporary file.
>   * Return 0 upon success, otherwise a negative errno value.
> diff --git a/include/block/block.h b/include/block/block.h
> index 5450610..3287dbc 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -60,6 +60,24 @@ typedef enum {
>      BDRV_REQ_MAY_UNMAP    = 0x4,
>  } BdrvRequestFlags;
>  
> +struct ProbeBlockSize {
> +    int rc;
> +    struct BlockSize {
> +        uint16_t phys;
> +        uint16_t log;
> +    } size;
> +};

Use of uint16_t for block sizes is silly, but not your fault, you just
follow existing usage.

> +
> +struct ProbeGeometry {
> +    int rc;
> +    struct HDGeometry {
> +        uint32_t heads;
> +        uint32_t sectors;
> +        uint32_t cylinders;
> +        uint32_t start;
> +    } geo;
> +};
> +
>  #define BDRV_O_RDWR        0x0002
>  #define BDRV_O_SNAPSHOT    0x0008 /* open the file read only and save writes in a snapshot */
>  #define BDRV_O_TEMPORARY   0x0010 /* delete the file after use */
> @@ -538,6 +556,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>   * the old #AioContext is not executing.
>   */
>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
> +struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs);
> +struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs);
>  
>  void bdrv_io_plug(BlockDriverState *bs);
>  void bdrv_io_unplug(BlockDriverState *bs);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index a1c17b9..830e564 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -271,6 +271,9 @@ struct BlockDriver {
>      void (*bdrv_io_unplug)(BlockDriverState *bs);
>      void (*bdrv_flush_io_queue)(BlockDriverState *bs);
>  
> +    struct ProbeBlockSize (*bdrv_probe_blocksizes)(BlockDriverState *bs);
> +    struct ProbeGeometry (*bdrv_probe_geometry)(BlockDriverState *bs);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };

Callbacks need contracts even more than functions do.  I know this file
is full of bad examples.  Let's not add more :)

Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize

[Ekaterina Tumanova]

On 11/27/2014 05:55 PM, Markus Armbruster wrote:
> I'm sorry for the delay.  I got the flu and have difficulties thinking
> straight for longer than a few minutes.
>
> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:
>
>> Add driver functions for geometry and blocksize detection
>>
>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> ---
>>   block.c                   | 26 ++++++++++++++++++++++++++
>>   include/block/block.h     | 20 ++++++++++++++++++++
>>   include/block/block_int.h |  3 +++
>>   3 files changed, 49 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index a612594..5df35cf 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -548,6 +548,32 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>>       }
>>   }
>>
>> +struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    struct ProbeBlockSize err_geo = { .rc = -1 };
>> +
>> +    assert(drv != NULL);
>> +    if (drv->bdrv_probe_blocksizes) {
>> +        return drv->bdrv_probe_blocksizes(bs);
>> +    }
>> +
>> +    return err_geo;
>> +}
>
> I'm not sure about "probe".  Naming is hard.  "get"?
>
There was already "bdrv_get_geometry", and I wanted the _geometry and 
_blocksize functions to use the same naming convention.
I thought probe might be more suitable, since we do not "get" the value
for sure. maybe "detect"?

> Squashing status and actual payload into a single struct to use as
> return type isn't wrong, but unusual.  When the payload can't represent
> failure conveniently, we usually return status, and write the payload to
> a buffer provided by the caller, like this:
>
>      int bdrv_get_blocksizes(BlockDriverState *bs,
>                              uint16_t *physical_blk_sz,
>                              uint16_t *logical_blk_sz)
>
> Or, with a struct to hold both sizes:
>
>      int bdrv_get_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>
Do you insist on changing that? Returning a struct via stack seemed
useful to me, since there was less probability of caller allocating
a buffer of incorrect size or smth like that.

> Such a struct should ideally be used in other places where we store both
> sizes.
>
> A brief function contract comment wouldn't hurt.  Something like
>
>      /*
>       * Try to get @bs's logical and physical block size.
>       * Block sizes are always a multiple of BDRV_SECTOR_SIZE.
>       * On success, store them in ... and return 0.
>       * On failure, return -errno.
>       */
>
will do

>> +
>> +struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    struct ProbeGeometry err_geo = { .rc = -1 };
>> +
>> +    assert(drv != NULL);
>> +    if (drv->bdrv_probe_geometry) {
>> +        return drv->bdrv_probe_geometry(bs);
>> +    }
>> +
>> +    return err_geo;
>> +}
>> +
>>   /*
>>    * Create a uniquely-named empty temporary file.
>>    * Return 0 upon success, otherwise a negative errno value.
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 5450610..3287dbc 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -60,6 +60,24 @@ typedef enum {
>>       BDRV_REQ_MAY_UNMAP    = 0x4,
>>   } BdrvRequestFlags;
>>
>> +struct ProbeBlockSize {
>> +    int rc;
>> +    struct BlockSize {
>> +        uint16_t phys;
>> +        uint16_t log;
>> +    } size;
>> +};
>
> Use of uint16_t for block sizes is silly, but not your fault, you just
> follow existing usage.
>
>> +
>> +struct ProbeGeometry {
>> +    int rc;
>> +    struct HDGeometry {
>> +        uint32_t heads;
>> +        uint32_t sectors;
>> +        uint32_t cylinders;
>> +        uint32_t start;
>> +    } geo;
>> +};
>> +
>>   #define BDRV_O_RDWR        0x0002
>>   #define BDRV_O_SNAPSHOT    0x0008 /* open the file read only and save writes in a snapshot */
>>   #define BDRV_O_TEMPORARY   0x0010 /* delete the file after use */
>> @@ -538,6 +556,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>>    * the old #AioContext is not executing.
>>    */
>>   void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
>> +struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs);
>> +struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs);
>>
>>   void bdrv_io_plug(BlockDriverState *bs);
>>   void bdrv_io_unplug(BlockDriverState *bs);
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index a1c17b9..830e564 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -271,6 +271,9 @@ struct BlockDriver {
>>       void (*bdrv_io_unplug)(BlockDriverState *bs);
>>       void (*bdrv_flush_io_queue)(BlockDriverState *bs);
>>
>> +    struct ProbeBlockSize (*bdrv_probe_blocksizes)(BlockDriverState *bs);
>> +    struct ProbeGeometry (*bdrv_probe_geometry)(BlockDriverState *bs);
>> +
>>       QLIST_ENTRY(BlockDriver) list;
>>   };
>
> Callbacks need contracts even more than functions do.  I know this file
> is full of bad examples.  Let's not add more :)
>

Thanks!
Kate.

Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> Move the IOCTL calls that detect logical blocksize from raw_probe_alignment
> into separate function (probe_logical_blocksize).
> Introduce function which detect physical blocksize via IOCTL
> (probe_physical_blocksize).
> Both functions will be used in the next patch.

The first one is used in this patch, actually.

>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>

Subject's subsystem is "geometry".  Geometry is your topic.  The
subsystem is what you're patching.  Here, it's "raw-posix" or
"block/raw-posix".  Likewise for the other patches in this series.

Stefan asked you move probe_physical_blocksize() to the patch that uses
it, and I concur.

With that done, I'd write a commit message like

    raw-posix: Factor block size detection out of raw_probe_alignment()

    Put it in new probe_logical_blocksize().
  
> ---
>  block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 19 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e100ae2..45f1d79 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char **filename)
>  }
>  #endif
>  
> -static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> +static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd)

Parameter bs is unused, let's drop it.

Suggest function comment

    /**
     * Return logical block size, or zero if we can't figure it out
     */

>  {
> -    BDRVRawState *s = bs->opaque;
> -    char *buf;
> -    unsigned int sector_size;
> -
> -    /* For /dev/sg devices the alignment is not really used.
> -       With buffered I/O, we don't have any restrictions. */
> -    if (bs->sg || !s->needs_alignment) {
> -        bs->request_alignment = 1;
> -        s->buf_align = 1;
> -        return;
> -    }
> +    unsigned int sector_size = 0;

Pointless initialization.

>  
>      /* Try a few ioctls to get the right size */
> -    bs->request_alignment = 0;
> -    s->buf_align = 0;
> -
>  #ifdef BLKSSZGET
>      if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef DKIOCGETBLOCKSIZE
>      if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef DIOCGSECTORSIZE
>      if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef CONFIG_XFS
>      if (s->is_xfs) {
>          struct dioattr da;
>          if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
> -            bs->request_alignment = da.d_miniosz;
> +            sector_size = da.d_miniosz;
>              /* The kernel returns wrong information for d_mem */
>              /* s->buf_align = da.d_mem; */

Since you keep the enabled assignments to s->buf_align out of this
function, you should keep out this disabled one, too.

> +            return sector_size;
>          }
>      }
>  #endif
>  
> +    return 0;
> +}
> +
> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd)

Parameter bs is unused, let's drop it.

> +{
> +    unsigned int blk_size = 0;

Pointless initialization.

> +#ifdef BLKPBSZGET
> +    if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) {
> +        return blk_size;
> +    }
> +#endif
> +
> +    return 0;
> +}
> +
> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    char *buf;
> +
> +    /* For /dev/sg devices the alignment is not really used.
> +       With buffered I/O, we don't have any restrictions. */
> +    if (bs->sg || !s->needs_alignment) {
> +        bs->request_alignment = 1;
> +        s->buf_align = 1;
> +        return;
> +    }
> +
> +    s->buf_align = 0;
> +    /* Let's try to use the logical blocksize for the alignment. */
> +    bs->request_alignment = probe_logical_blocksize(bs, fd);
> +
>      /* If we could not get the sizes so far, we can only guess them */
>      if (!s->buf_align) {
>          size_t align;

Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> On 11/27/2014 05:55 PM, Markus Armbruster wrote:
>> I'm sorry for the delay.  I got the flu and have difficulties thinking
>> straight for longer than a few minutes.
>>
>> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:
>>
>>> Add driver functions for geometry and blocksize detection
>>>
>>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>> ---
>>>   block.c                   | 26 ++++++++++++++++++++++++++
>>>   include/block/block.h     | 20 ++++++++++++++++++++
>>>   include/block/block_int.h |  3 +++
>>>   3 files changed, 49 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index a612594..5df35cf 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -548,6 +548,32 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>>>       }
>>>   }
>>>
>>> +struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +    struct ProbeBlockSize err_geo = { .rc = -1 };
>>> +
>>> +    assert(drv != NULL);
>>> +    if (drv->bdrv_probe_blocksizes) {
>>> +        return drv->bdrv_probe_blocksizes(bs);
>>> +    }
>>> +
>>> +    return err_geo;
>>> +}
>>
>> I'm not sure about "probe".  Naming is hard.  "get"?
>>
> There was already "bdrv_get_geometry", and I wanted the _geometry and
> _blocksize functions to use the same naming convention.

Fair enough.

bdrv_get_geometry() is a silly wrapper around bdrv_nb_sectors() that
maps failure to zero sectors.  I hope to kill it some time.  Doesn't
help you now.

> I thought probe might be more suitable, since we do not "get" the value
> for sure. maybe "detect"?

Feel free to stick to "probe".

>> Squashing status and actual payload into a single struct to use as
>> return type isn't wrong, but unusual.  When the payload can't represent
>> failure conveniently, we usually return status, and write the payload to
>> a buffer provided by the caller, like this:
>>
>>      int bdrv_get_blocksizes(BlockDriverState *bs,
>>                              uint16_t *physical_blk_sz,
>>                              uint16_t *logical_blk_sz)
>>
>> Or, with a struct to hold both sizes:
>>
>>      int bdrv_get_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>>
> Do you insist on changing that? Returning a struct via stack seemed
> useful to me, since there was less probability of caller allocating
> a buffer of incorrect size or smth like that.

You'd have to do crazy stuff to defeat the static type checker.

Please stick to the common technique.

[...]

Re: [Qemu-devel] [PATCH v2 3/6] geometry: Add driver methods to probe blocksizes and geometry

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> This patch introduces driver methods of defining disk blocksizes
> (physical and logical) and hard drive geometry.
> The method is only implemented for "host_device". For "raw" devices
> driver calls child's method.
> The detection will only work for DASD devices. In order to check that
> a local CheckForDASD function was introduced, which calls BIODASDINFO2 ioctl
> and returns 0 only if it succeeds.
>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  block/raw-posix.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/raw_bsd.c   | 12 ++++++++++
>  2 files changed, 77 insertions(+)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 45f1d79..274ceda 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -56,6 +56,7 @@
>  #include <linux/cdrom.h>
>  #include <linux/fd.h>
>  #include <linux/fs.h>
> +#include <linux/hdreg.h>
>  #ifndef FS_NOCOW_FL
>  #define FS_NOCOW_FL                     0x00800000 /* Do not cow file */
>  #endif
> @@ -93,6 +94,10 @@
>  #include <xfs/xfs.h>
>  #endif
>  
> +#ifdef __s390__
> +#include <asm/dasd.h>
> +#endif
> +
>  //#define DEBUG_FLOPPY
>  
>  //#define DEBUG_BLOCK
> @@ -678,6 +683,64 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>      bs->bl.opt_mem_alignment = s->buf_align;
>  }
>  
> +static int CheckForDASD(int fd)

As Christian said, no CamelCase for functions.

I guess your function returns either 0 or -1.  Can't say for sure
without looking up BIODASDINFO2.

I'd do a slightly higher level is_dasd() returning bool.  Your choice.

> +{
> +#ifdef BIODASDINFO2
> +    struct dasd_information2_t info = {0};
> +
> +    return ioctl(fd, BIODASDINFO2, &info);
> +#endif
> +    return -1;
> +}
> +
> +static struct ProbeBlockSize hdev_probe_blocksizes(BlockDriverState *bs)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    struct ProbeBlockSize block_sizes = {0};
> +
> +    block_sizes.rc = CheckForDASD(s->fd);
> +    /* If DASD, get blocksizes */
> +    if (block_sizes.rc == 0) {
> +        block_sizes.size.log = probe_logical_blocksize(bs, s->fd);
> +        block_sizes.size.phys = probe_physical_blocksize(bs, s->fd);
> +    }
> +
> +    return block_sizes;
> +}

Fails unless DASD.  Why?  The block size concept applies not just to
DASD, and the probe_*_blocksize() functions should just work, shouldn't
they?

> +
> +static struct ProbeGeometry hdev_probe_geometry(BlockDriverState *bs)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    struct ProbeGeometry geometry = {0};
> +    struct hd_geometry ioctl_geo = {0};

Is this initializer really necessary?

Because I like my local variable names short & sweet, I'd call this one
geo.  Your choice.

> +
> +    geometry.rc = CheckForDASD(s->fd);
> +    if (geometry.rc != 0) {

Works if your function really returns either 0 or -1.  If it can return
a positive value, it breaks the callback's contract.

> +        goto done;
> +    }
> +    /* If DASD, get it's geometry */

its

> +    geometry.rc = ioctl(s->fd, HDIO_GETGEO, &ioctl_geo);
> +    /* Do not return a geometry for partition */
> +    if (ioctl_geo.start != 0) {
> +        geometry.rc = -1;
> +        goto done;
> +    }
> +    /* HDIO_GETGEO may return success even though geo contains zeros
> +       (e.g. certain multipath setups) */
> +    if (!ioctl_geo.heads || !ioctl_geo.sectors || !ioctl_geo.cylinders) {
> +        geometry.rc = -1;
> +        goto done;
> +    }
> +    if (geometry.rc == 0) {
> +        geometry.geo.heads = (uint32_t)ioctl_geo.heads;
> +        geometry.geo.sectors = (uint32_t)ioctl_geo.sectors;
> +        geometry.geo.cylinders = (uint32_t)ioctl_geo.cylinders;

The LHS is uint32_t, the RHS is unsigned char or unsigned short, thus
the type casts are superfluous clutter.

8-bit head * 8-bit sectors * 16-bit cylinders can't represent today's
disk sizes, so ioctl_geo.cylinders is generally useless.  The common
advice is to ignore it, and do something like

    geometry.geo.cylinders = nb_sectors / (ioctl_geo.heads * ioctl_geo.sectors)

A possible alternative is to explicitly document that the returned
cylinders value can't be trusted.

Another one is not to return the number of cylinders :)

> +        geometry.geo.start = (uint32_t)ioctl_geo.start;

Always assigns zero (we checked above).  Why is member geo_start useful?

> +    }
> +done:
> +   return geometry;
> +}

Also fails unless DASD.  The geometry concept applies pretty much only
to disks older than thirty years or so, and to software designed for
them, like HDIO_GETGEO.

> +
>  static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
>  {
>      int ret;
> @@ -2128,6 +2191,8 @@ static BlockDriver bdrv_host_device = {
>      .bdrv_get_info = raw_get_info,
>      .bdrv_get_allocated_file_size
>                          = raw_get_allocated_file_size,
> +    .bdrv_probe_blocksizes = hdev_probe_blocksizes,
> +    .bdrv_probe_geometry = hdev_probe_geometry,
>  
>      .bdrv_detach_aio_context = raw_detach_aio_context,
>      .bdrv_attach_aio_context = raw_attach_aio_context,
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 401b967..d164eba 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -173,6 +173,16 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
>      return 1;
>  }
>  
> +static struct ProbeBlockSize raw_probe_blocksizes(BlockDriverState *bs)
> +{
> +    return bdrv_probe_blocksizes(bs->file);
> +}
> +
> +static struct ProbeGeometry raw_probe_geometry(BlockDriverState *bs)
> +{
> +    return bdrv_probe_geometry(bs->file);
> +}
> +
>  static BlockDriver bdrv_raw = {
>      .format_name          = "raw",
>      .bdrv_probe           = &raw_probe,
> @@ -190,6 +200,8 @@ static BlockDriver bdrv_raw = {
>      .has_variable_length  = true,
>      .bdrv_get_info        = &raw_get_info,
>      .bdrv_refresh_limits  = &raw_refresh_limits,
> +    .bdrv_probe_blocksizes = &raw_probe_blocksizes,
> +    .bdrv_probe_geometry  = &raw_probe_geometry,
>      .bdrv_is_inserted     = &raw_is_inserted,
>      .bdrv_media_changed   = &raw_media_changed,
>      .bdrv_eject           = &raw_eject,

Re: [Qemu-devel] [PATCH v2 5/6] geometry: Call backend function to detect geometry and blocksize

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> hd_geometry_guess function autodetects the drive geometry. This patch
> adds a block backend call, that probes the backing device geometry.
> If the inner driver method is implemented and succeeds (currently only DASDs),
> the blkconf_geometry will pass-through the backing device geometry.
>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  hw/block/block.c         | 11 +++++++++++
>  hw/block/hd-geometry.c   |  9 +++++++++
>  hw/block/virtio-blk.c    |  1 +
>  include/hw/block/block.h |  1 +
>  4 files changed, 22 insertions(+)
>
> diff --git a/hw/block/block.c b/hw/block/block.c
> index a625773..f1d29bc 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -25,6 +25,17 @@ void blkconf_serial(BlockConf *conf, char **serial)
>      }
>  }
>  
> +void blkconf_blocksizes(BlockConf *conf)
> +{
> +    BlockBackend *blk = conf->blk;
> +    struct ProbeBlockSize blocksize = blk_probe_blocksizes(blk);
> +
> +    if (blocksize.rc == 0) {
> +        conf->physical_block_size = blocksize.size.phys;
> +        conf->logical_block_size = blocksize.size.log;
> +    }
> +}
> +

Uh, doesn't this overwrite the user's geometry?

The existing blkconf_ functions do nothing when the user supplied the
configuration.  In other words, they only provide defaults.  Why should
this one be different?

>  void blkconf_geometry(BlockConf *conf, int *ptrans,
>                        unsigned cyls_max, unsigned heads_max, unsigned secs_max,
>                        Error **errp)
> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
> index 6fcf74d..4972114 100644
> --- a/hw/block/hd-geometry.c
> +++ b/hw/block/hd-geometry.c
> @@ -121,6 +121,14 @@ void hd_geometry_guess(BlockBackend *blk,
>                         int *ptrans)
>  {
>      int cylinders, heads, secs, translation;
> +    struct ProbeGeometry geometry = blk_probe_geometry(blk);
> +
> +    if (geometry.rc == 0) {
> +        *pcyls = geometry.geo.cylinders;
> +        *psecs = geometry.geo.sectors;
> +        *pheads = geometry.geo.heads;
> +        goto done;
> +    }
>  

hd_geometry_guess() is called by blkconf_geometry() to conjure up a
default geometry when the user didn't pick one.  Fairly elaborate
guesswork.  blkconf_geometry() runs for IDE, SCSI and virtio-blk only.

Your patch makes it pick the backend's geometry as reported by
blk_probe_geometry() before anything else.

This is an incompatible change for backends where blk_probe_geometry()
succeeds.  Currently, it succeeds only for DASD, and there you *want*
the incompatible change.

My point is: if we rely on blk_probe_geometry() failing except for DASD,
then we should call it something like blk_dasd_geometry() to make that
obvious.

The commit message needs to spell out the incompatible change.

>      if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
>          /* no LCHS guess: use a standard physical disk geometry  */
> @@ -143,6 +151,7 @@ void hd_geometry_guess(BlockBackend *blk,
>             the logical geometry */
>          translation = BIOS_ATA_TRANSLATION_NONE;
>      }
> +done:
>      if (ptrans) {
>          *ptrans = translation;
>      }
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index b19b102..6f01565 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -745,6 +745,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>          error_propagate(errp, err);
>          return;
>      }
> +    blkconf_blocksizes(&conf->conf);
>  
>      virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
>                  sizeof(struct virtio_blk_config));

Why only virtio-blk, and not the other device models sporting
configurable block sizes (nvme, IDE, SCSI, usb-storage)?

This is an incompatible change for backends where blk_probe_blocksizes()
succeeds and returns something other than (512, 512).  Currently, it
succeeds only for DASD.  Is that what we want?

The commit message needs to spell out the incompatible change.

Perhaps this patch should be split up into one for geometry and one for
block sizes.

> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 0d0ce9a..856bf75 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -63,6 +63,7 @@ void blkconf_serial(BlockConf *conf, char **serial);
>  void blkconf_geometry(BlockConf *conf, int *trans,
>                        unsigned cyls_max, unsigned heads_max, unsigned secs_max,
>                        Error **errp);
> +void blkconf_blocksizes(BlockConf *conf);
>  
>  /* Hard disk geometry */

Re: [Qemu-devel] [PATCH v2 5/6] geometry: Call backend function to detect geometry and blocksize

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> hd_geometry_guess function autodetects the drive geometry. This patch
> adds a block backend call, that probes the backing device geometry.
> If the inner driver method is implemented and succeeds (currently only DASDs),
> the blkconf_geometry will pass-through the backing device geometry.
[...]
> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
> index 6fcf74d..4972114 100644
> --- a/hw/block/hd-geometry.c
> +++ b/hw/block/hd-geometry.c
> @@ -121,6 +121,14 @@ void hd_geometry_guess(BlockBackend *blk,
>                         int *ptrans)
>  {
>      int cylinders, heads, secs, translation;
> +    struct ProbeGeometry geometry = blk_probe_geometry(blk);
> +
> +    if (geometry.rc == 0) {
> +        *pcyls = geometry.geo.cylinders;
> +        *psecs = geometry.geo.sectors;
> +        *pheads = geometry.geo.heads;

Missing:

        translation = ...

I'm not sure what value to assign.

> +        goto done;
> +    }
>  
>      if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
>          /* no LCHS guess: use a standard physical disk geometry  */
[...]

Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On Wed, Nov 26, 2014 at 01:16:48PM +0300, Ekaterina Tumanova wrote:
> On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote:
> >On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote:
> >3. Why does s390 need to customize hd_geometry_guess()?
> >
> Since hd_geometry_guess contains semantics of x86-specific LBA translation,
> we have to modify it not to get in the way of z
> architecture

If the behavior is x86-specific, it should be made x86-specific.  z,
ppc, sparc, arm, etc should share the non-x86-specific code path.  It's
weird to single out z here, this seems like the wrong way around.

Is the x86-specific behavior a problem in practice?  No one seemed to
mind for the other targets.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

On Wed, Nov 26, 2014 at 01:16:48PM +0300, Ekaterina Tumanova wrote:
> On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote:
> >On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote:
> >4. Please use scripts/checkpatch.pl to check coding style.
> >
> I did :)

You are right, checkpatch.pl doesn't complain.

That's weird, I thought there were a few instances that don't follow the
QEMU coding style.  Either I was wrong or checkpatch.pl is incomplete.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 6/6] geometry: Target specific hook for s390x in geometry guessing

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> For target-s390x, the behaviour is chosen as follows:
> If no geo could be guessed from backing device, guess_disk_lchs (partition
> table guessing) is called.
> If this is not successful (e.g. image files) geometry is derived from the
> size of the disk (as before).
> We have no translation on s390, so we simply set in to NONE.
>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  hw/block/Makefile.objs |  6 +++++-
>  hw/block/hd-geometry.c | 36 +++++++++++++++++++++++++++++++-----
>  2 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> index d4c3ab7..1f7da7a 100644
> --- a/hw/block/Makefile.objs
> +++ b/hw/block/Makefile.objs
> @@ -1,4 +1,4 @@
> -common-obj-y += block.o cdrom.o hd-geometry.o
> +common-obj-y += block.o cdrom.o
>  common-obj-$(CONFIG_FDC) += fdc.o
>  common-obj-$(CONFIG_SSI_M25P80) += m25p80.o
>  common-obj-$(CONFIG_NAND) += nand.o
> @@ -13,3 +13,7 @@ obj-$(CONFIG_SH4) += tc58128.o
>  
>  obj-$(CONFIG_VIRTIO) += virtio-blk.o
>  obj-$(CONFIG_VIRTIO) += dataplane/
> +
> +# geometry is target/architecture dependent and therefore needs to be built
> +# for every target platform
> +obj-y += hd-geometry.o
> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
> index 4972114..b462225 100644
> --- a/hw/block/hd-geometry.c
> +++ b/hw/block/hd-geometry.c
> @@ -49,11 +49,12 @@ struct partition {
>  
>  /* 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(BlockBackend *blk,
> -                           int *pcylinders, int *pheads, int *psectors)
> +static int guess_disk_lchs(BlockBackend *blk, uint32_t *pcylinders,
> +                           uint32_t *pheads, uint32_t *psectors)
>  {
>      uint8_t buf[BDRV_SECTOR_SIZE];
> -    int i, heads, sectors, cylinders;
> +    int i;
> +    uint32_t heads, sectors, cylinders;
>      struct partition *p;
>      uint32_t nr_sects;
>      uint64_t nb_sectors;
> @@ -116,11 +117,11 @@ static void guess_chs_for_size(BlockBackend *blk,
>      *psecs = 63;
>  }
>  
> +#ifdef TARGET_S390X
>  void hd_geometry_guess(BlockBackend *blk,
>                         uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
>                         int *ptrans)
>  {
> -    int cylinders, heads, secs, translation;
>      struct ProbeGeometry geometry = blk_probe_geometry(blk);
>  
>      if (geometry.rc == 0) {
> @@ -129,7 +130,32 @@ void hd_geometry_guess(BlockBackend *blk,
>          *pheads = geometry.geo.heads;
>          goto done;
>      }
> +    if (guess_disk_lchs(blk, pcyls, pheads, psecs) == 0) {
> +        goto done;
> +    }
> +    guess_chs_for_size(blk, pcyls, pheads, psecs);
> +done:
> +    if (ptrans) {
> +        *ptrans = BIOS_ATA_TRANSLATION_NONE;
> +    }

The goto looks awkward...  What about

    if (guess_disk_lchs(blk, pcyls, pheads, psecs) < 0) {
        guess_chs_for_size(blk, pcyls, pheads, psecs);
    }

>  
> +    trace_hd_geometry_guess(blk, *pcyls, *pheads, *psecs,
> +                            BIOS_ATA_TRANSLATION_NONE);
> +}
> +#else
> +void hd_geometry_guess(BlockBackend *blk,
> +                       uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
> +                       int *ptrans)
> +{
> +    int cylinders, heads, secs, translation;
> +    struct ProbeGeometry geometry = blk_probe_geometry(blk);
> +
> +    if (geometry.rc == 0) {
> +        *pcyls = geometry.geo.cylinders;
> +        *psecs = geometry.geo.sectors;
> +        *pheads = geometry.geo.heads;
> +        goto done;
> +    }
>      if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
>          /* no LCHS guess: use a standard physical disk geometry  */
>          guess_chs_for_size(blk, pcyls, pheads, psecs);
> @@ -157,7 +183,7 @@ done:
>      }
>      trace_hd_geometry_guess(blk, *pcyls, *pheads, *psecs, translation);
>  }
> -
> +#endif

Please put the blank line after the #endif.

>  int hd_bios_chs_auto_trans(uint32_t cyls, uint32_t heads, uint32_t secs)
>  {
>      return cyls <= 1024 && heads <= 16 && secs <= 63

hd_geometry_guess() behavior before this series:

    If we can't guess LCHS from MSDOS partition table
        guess geometry on size
        translation is NONE for very small disk, else LBA
    Else if LCHS guess has heads > 16
        BIOS LBA translation must be active
        guess geometry on size
        translation is LARGE for sufficiently small disk, else LBA
    Else
        use LCHS guess
        translation is NONE

Afterwards, targets other than s390x:

    If backend has a geometry (currently only DASD has)
        use backend geometry
        translation is NONE
    Else behave like before this series

Incompatible change.  Why do we want it?

Afterwards, target s390x:

    If backend has a geometry (currently only DASD has)
        use backend geometry
        translation is NONE
    Else if we can't guess LCHS from MSDOS partition table
        guess geometry on size
    Else
        use LCHS guess
    Regardless, translation is NONE

Non-trivial incompatible change.  At least some of it is wanted.

Why do you still guess from an MS-DOS partition table with an s390x
target?

The only caller that passes non-null ptrans is ide_dev_initfn(), and the
only user of the value stored there is pc_cmos_init_late().  Thus,
what you store here doesn't matter.

Re: [Qemu-devel] [PATCH] geometry: fix i386 compilation

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  hw/block/hd-geometry.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
> index b462225..905d2c6 100644
> --- a/hw/block/hd-geometry.c
> +++ b/hw/block/hd-geometry.c
> @@ -147,7 +147,8 @@ void hd_geometry_guess(BlockBackend *blk,
>                         uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
>                         int *ptrans)
>  {
> -    int cylinders, heads, secs, translation;
> +    uint32_t cylinders, heads, secs;
> +    int translation = BIOS_ATA_TRANSLATION_NONE;
>      struct ProbeGeometry geometry = blk_probe_geometry(blk);
>  
>      if (geometry.rc == 0) {
> @@ -173,9 +174,6 @@ void hd_geometry_guess(BlockBackend *blk,
>          *pcyls = cylinders;
>          *pheads = heads;
>          *psecs = secs;
> -        /* disable any translation to be in sync with
> -           the logical geometry */
> -        translation = BIOS_ATA_TRANSLATION_NONE;
>      }

Actually broken in PATCH 5, so it needs to be fixed there, and not in
PATCH 6 (which this one fixes up).

Moreover, your fixup makes the code less clear.  Please add the missing
translation = ... instead.

>  done:
>      if (ptrans) {

Re: [Qemu-devel] [PATCH v2 5/6] geometry: Call backend function to detect geometry and blocksize

[Ekaterina Tumanova]

On 11/28/2014 01:10 PM, Markus Armbruster wrote:
> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:
>
>> hd_geometry_guess function autodetects the drive geometry. This patch
>> adds a block backend call, that probes the backing device geometry.
>> If the inner driver method is implemented and succeeds (currently only DASDs),
>> the blkconf_geometry will pass-through the backing device geometry.
>>
>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> ---
>>   hw/block/block.c         | 11 +++++++++++
>>   hw/block/hd-geometry.c   |  9 +++++++++
>>   hw/block/virtio-blk.c    |  1 +
>>   include/hw/block/block.h |  1 +
>>   4 files changed, 22 insertions(+)
>>
>> diff --git a/hw/block/block.c b/hw/block/block.c
>> index a625773..f1d29bc 100644
>> --- a/hw/block/block.c
>> +++ b/hw/block/block.c
>> @@ -25,6 +25,17 @@ void blkconf_serial(BlockConf *conf, char **serial)
>>       }
>>   }
>>
>> +void blkconf_blocksizes(BlockConf *conf)
>> +{
>> +    BlockBackend *blk = conf->blk;
>> +    struct ProbeBlockSize blocksize = blk_probe_blocksizes(blk);
>> +
>> +    if (blocksize.rc == 0) {
>> +        conf->physical_block_size = blocksize.size.phys;
>> +        conf->logical_block_size = blocksize.size.log;
>> +    }
>> +}
>> +
>
> Uh, doesn't this overwrite the user's geometry?
>
> The existing blkconf_ functions do nothing when the user supplied the
> configuration.  In other words, they only provide defaults.  Why should
> this one be different?
>

this will be fixed in the next version

>>   void blkconf_geometry(BlockConf *conf, int *ptrans,
>>                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
>>                         Error **errp)
>> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
>> index 6fcf74d..4972114 100644
>> --- a/hw/block/hd-geometry.c
>> +++ b/hw/block/hd-geometry.c
>> @@ -121,6 +121,14 @@ void hd_geometry_guess(BlockBackend *blk,
>>                          int *ptrans)
>>   {
>>       int cylinders, heads, secs, translation;
>> +    struct ProbeGeometry geometry = blk_probe_geometry(blk);
>> +
>> +    if (geometry.rc == 0) {
>> +        *pcyls = geometry.geo.cylinders;
>> +        *psecs = geometry.geo.sectors;
>> +        *pheads = geometry.geo.heads;
>> +        goto done;
>> +    }
>>
>
> hd_geometry_guess() is called by blkconf_geometry() to conjure up a
> default geometry when the user didn't pick one.  Fairly elaborate
> guesswork.  blkconf_geometry() runs for IDE, SCSI and virtio-blk only.
>
> Your patch makes it pick the backend's geometry as reported by
> blk_probe_geometry() before anything else.
>
> This is an incompatible change for backends where blk_probe_geometry()
> succeeds.  Currently, it succeeds only for DASD, and there you *want*
> the incompatible change.
>
> My point is: if we rely on blk_probe_geometry() failing except for DASD,
> then we should call it something like blk_dasd_geometry() to make that
> obvious.
>

This function itself is not DASD specific. It calls the inner method for
"host_device" driver, which currently only succeeds for DASDs.
But in future someone can define methods for other drivers or
even modify the host_device driver.

> The commit message needs to spell out the incompatible change.
>
>>       if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
>>           /* no LCHS guess: use a standard physical disk geometry  */
>> @@ -143,6 +151,7 @@ void hd_geometry_guess(BlockBackend *blk,
>>              the logical geometry */
>>           translation = BIOS_ATA_TRANSLATION_NONE;
>>       }
>> +done:
>>       if (ptrans) {
>>           *ptrans = translation;
>>       }
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index b19b102..6f01565 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -745,6 +745,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>>           error_propagate(errp, err);
>>           return;
>>       }
>> +    blkconf_blocksizes(&conf->conf);
>>
>>       virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
>>                   sizeof(struct virtio_blk_config));
>
> Why only virtio-blk, and not the other device models sporting
> configurable block sizes (nvme, IDE, SCSI, usb-storage)?
>
> This is an incompatible change for backends where blk_probe_blocksizes()
> succeeds and returns something other than (512, 512).  Currently, it
> succeeds only for DASD.  Is that what we want?
>
> The commit message needs to spell out the incompatible change.
>
> Perhaps this patch should be split up into one for geometry and one for
> block sizes.
>
>> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
>> index 0d0ce9a..856bf75 100644
>> --- a/include/hw/block/block.h
>> +++ b/include/hw/block/block.h
>> @@ -63,6 +63,7 @@ void blkconf_serial(BlockConf *conf, char **serial);
>>   void blkconf_geometry(BlockConf *conf, int *trans,
>>                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
>>                         Error **errp);
>> +void blkconf_blocksizes(BlockConf *conf);
>>
>>   /* Hard disk geometry */
>

Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote:
[...]
>> 3. Why does s390 need to customize hd_geometry_guess()?
>>
> Since hd_geometry_guess contains semantics of x86-specific LBA
> translation, we have to modify it not to get in the way of z
> architecture

I already explained this in my review, but it's buried among other
stuff, so let me repeat it here:

The only caller that passes non-null ptrans to hd_geometry_guess() is
ide_dev_initfn(), and the only user of the value that gets stored there
is pc_cmos_init_late().

We store a highly PC-specific value there.  That's okay, it's used only
by PC machines.

The whole concept "BIOS translation" makes no sense for other machines.
Heck, it makes barely sense even on PCs.

Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Ekaterina Tumanova]

>
> Suggest function comment
>
>      /**
>       * Return logical block size, or zero if we can't figure it out
>       */
>
>>   {
>> -    BDRVRawState *s = bs->opaque;
>> -    char *buf;
>> -    unsigned int sector_size;
>> -
>> -    /* For /dev/sg devices the alignment is not really used.
>> -       With buffered I/O, we don't have any restrictions. */
>> -    if (bs->sg || !s->needs_alignment) {
>> -        bs->request_alignment = 1;
>> -        s->buf_align = 1;
>> -        return;
>> -    }
>> +    unsigned int sector_size = 0;
>
> Pointless initialization.

If I do not initialize the sector_size, and the ioctl fails,
I will return garbage as a blocksize to the caller.

>
>>
>>       /* Try a few ioctls to get the right size */
>> -    bs->request_alignment = 0;
>> -    s->buf_align = 0;
>> -
>>   #ifdef BLKSSZGET
>>       if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
>> -        bs->request_alignment = sector_size;
>> +        return sector_size;
>>       }
>>   #endif
>>   #ifdef DKIOCGETBLOCKSIZE
>>       if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
>> -        bs->request_alignment = sector_size;
>> +        return sector_size;
>>       }
>>   #endif
>>   #ifdef DIOCGSECTORSIZE
>>       if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
>> -        bs->request_alignment = sector_size;
>> +        return sector_size;
>>       }
>>   #endif
>>   #ifdef CONFIG_XFS
>>       if (s->is_xfs) {
>>           struct dioattr da;
>>           if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
>> -            bs->request_alignment = da.d_miniosz;
>> +            sector_size = da.d_miniosz;
>>               /* The kernel returns wrong information for d_mem */
>>               /* s->buf_align = da.d_mem; */
>
> Since you keep the enabled assignments to s->buf_align out of this
> function, you should keep out this disabled one, too.
>
>> +            return sector_size;
>>           }
>>       }
>>   #endif
>>
>> +    return 0;
>> +}
>> +
>> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd)
>
> Parameter bs is unused, let's drop it.
>
>> +{
>> +    unsigned int blk_size = 0;
>
> Pointless initialization.

Same here.

>
>> +#ifdef BLKPBSZGET
>> +    if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) {
>> +        return blk_size;
>> +    }
>> +#endif
>> +
>> +    return 0;
>> +}
>> +
>> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>> +{
>> +    BDRVRawState *s = bs->opaque;
>> +    char *buf;
>> +
>> +    /* For /dev/sg devices the alignment is not really used.
>> +       With buffered I/O, we don't have any restrictions. */
>> +    if (bs->sg || !s->needs_alignment) {
>> +        bs->request_alignment = 1;
>> +        s->buf_align = 1;
>> +        return;
>> +    }
>> +
>> +    s->buf_align = 0;
>> +    /* Let's try to use the logical blocksize for the alignment. */
>> +    bs->request_alignment = probe_logical_blocksize(bs, fd);
>> +
>>       /* If we could not get the sizes so far, we can only guess them */
>>       if (!s->buf_align) {
>>           size_t align;
>

Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Ekaterina Tumanova]

On 11/28/2014 01:35 PM, Stefan Hajnoczi wrote:
> On Wed, Nov 26, 2014 at 01:16:48PM +0300, Ekaterina Tumanova wrote:
>> On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote:
>>> On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote:
>>> 3. Why does s390 need to customize hd_geometry_guess()?
>>>
>> Since hd_geometry_guess contains semantics of x86-specific LBA translation,
>> we have to modify it not to get in the way of z
>> architecture
>
> If the behavior is x86-specific, it should be made x86-specific.  z,
> ppc, sparc, arm, etc should share the non-x86-specific code path.  It's
> weird to single out z here, this seems like the wrong way around.
>
> Is the x86-specific behavior a problem in practice?  No one seemed to
> mind for the other targets.
>

on s390 arch this adds 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.

If you guys don't like the way patch goes, I can exclude it from patch
series and we can discuss it later.

But I thought that since it's related to the hard drive geometry,
and since we change hd_geometry_guess in this patchset anyway, why not
get rid of this problem as well.

Kate.

Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

>>
>> Suggest function comment
>>
>>      /**
>>       * Return logical block size, or zero if we can't figure it out
>>       */
>>
>>>   {
>>> -    BDRVRawState *s = bs->opaque;
>>> -    char *buf;
>>> -    unsigned int sector_size;
>>> -
>>> -    /* For /dev/sg devices the alignment is not really used.
>>> -       With buffered I/O, we don't have any restrictions. */
>>> -    if (bs->sg || !s->needs_alignment) {
>>> -        bs->request_alignment = 1;
>>> -        s->buf_align = 1;
>>> -        return;
>>> -    }
>>> +    unsigned int sector_size = 0;
>>
>> Pointless initialization.
>
> If I do not initialize the sector_size, and the ioctl fails,
> I will return garbage as a blocksize to the caller.

Where?  As far as I can see, we return it only after ioctl() succeeded.

>>>
>>>       /* Try a few ioctls to get the right size */
>>> -    bs->request_alignment = 0;
>>> -    s->buf_align = 0;
>>> -
>>>   #ifdef BLKSSZGET
>>>       if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
>>> -        bs->request_alignment = sector_size;
>>> +        return sector_size;
>>>       }
>>>   #endif
>>>   #ifdef DKIOCGETBLOCKSIZE
>>>       if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
>>> -        bs->request_alignment = sector_size;
>>> +        return sector_size;
>>>       }
>>>   #endif
>>>   #ifdef DIOCGSECTORSIZE
>>>       if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
>>> -        bs->request_alignment = sector_size;
>>> +        return sector_size;
>>>       }
>>>   #endif
>>>   #ifdef CONFIG_XFS
>>>       if (s->is_xfs) {
>>>           struct dioattr da;
>>>           if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
>>> -            bs->request_alignment = da.d_miniosz;
>>> +            sector_size = da.d_miniosz;
>>>               /* The kernel returns wrong information for d_mem */
>>>               /* s->buf_align = da.d_mem; */
>>
>> Since you keep the enabled assignments to s->buf_align out of this
>> function, you should keep out this disabled one, too.
>>
>>> +            return sector_size;
>>>           }
>>>       }
>>>   #endif
>>>
>>> +    return 0;
>>> +}
>>> +
>>> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd)
>>
>> Parameter bs is unused, let's drop it.
>>
>>> +{
>>> +    unsigned int blk_size = 0;
>>
>> Pointless initialization.
>
> Same here.

Again, we return it only after ioctl() succeeded.

>>> +#ifdef BLKPBSZGET
>>> +    if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) {
>>> +        return blk_size;
>>> +    }
>>> +#endif
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>>> +{
>>> +    BDRVRawState *s = bs->opaque;
>>> +    char *buf;
>>> +
>>> +    /* For /dev/sg devices the alignment is not really used.
>>> +       With buffered I/O, we don't have any restrictions. */
>>> +    if (bs->sg || !s->needs_alignment) {
>>> +        bs->request_alignment = 1;
>>> +        s->buf_align = 1;
>>> +        return;
>>> +    }
>>> +
>>> +    s->buf_align = 0;
>>> +    /* Let's try to use the logical blocksize for the alignment. */
>>> +    bs->request_alignment = probe_logical_blocksize(bs, fd);
>>> +
>>>       /* If we could not get the sizes so far, we can only guess them */
>>>       if (!s->buf_align) {
>>>           size_t align;
>>

Re: [Qemu-devel] [PATCH v2 5/6] geometry: Call backend function to detect geometry and blocksize

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> On 11/28/2014 01:10 PM, Markus Armbruster wrote:
>> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:
>>
>>> hd_geometry_guess function autodetects the drive geometry. This patch
>>> adds a block backend call, that probes the backing device geometry.
>>> If the inner driver method is implemented and succeeds (currently only DASDs),
>>> the blkconf_geometry will pass-through the backing device geometry.
>>>
>>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>> ---
>>>   hw/block/block.c         | 11 +++++++++++
>>>   hw/block/hd-geometry.c   |  9 +++++++++
>>>   hw/block/virtio-blk.c    |  1 +
>>>   include/hw/block/block.h |  1 +
>>>   4 files changed, 22 insertions(+)
>>>
>>> diff --git a/hw/block/block.c b/hw/block/block.c
>>> index a625773..f1d29bc 100644
>>> --- a/hw/block/block.c
>>> +++ b/hw/block/block.c
>>> @@ -25,6 +25,17 @@ void blkconf_serial(BlockConf *conf, char **serial)
>>>       }
>>>   }
>>>
>>> +void blkconf_blocksizes(BlockConf *conf)
>>> +{
>>> +    BlockBackend *blk = conf->blk;
>>> +    struct ProbeBlockSize blocksize = blk_probe_blocksizes(blk);
>>> +
>>> +    if (blocksize.rc == 0) {
>>> +        conf->physical_block_size = blocksize.size.phys;
>>> +        conf->logical_block_size = blocksize.size.log;
>>> +    }
>>> +}
>>> +
>>
>> Uh, doesn't this overwrite the user's geometry?
>>
>> The existing blkconf_ functions do nothing when the user supplied the
>> configuration.  In other words, they only provide defaults.  Why should
>> this one be different?
>>
>
> this will be fixed in the next version
>
>>>   void blkconf_geometry(BlockConf *conf, int *ptrans,
>>>                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
>>>                         Error **errp)
>>> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
>>> index 6fcf74d..4972114 100644
>>> --- a/hw/block/hd-geometry.c
>>> +++ b/hw/block/hd-geometry.c
>>> @@ -121,6 +121,14 @@ void hd_geometry_guess(BlockBackend *blk,
>>>                          int *ptrans)
>>>   {
>>>       int cylinders, heads, secs, translation;
>>> +    struct ProbeGeometry geometry = blk_probe_geometry(blk);
>>> +
>>> +    if (geometry.rc == 0) {
>>> +        *pcyls = geometry.geo.cylinders;
>>> +        *psecs = geometry.geo.sectors;
>>> +        *pheads = geometry.geo.heads;
>>> +        goto done;
>>> +    }
>>>
>>
>> hd_geometry_guess() is called by blkconf_geometry() to conjure up a
>> default geometry when the user didn't pick one.  Fairly elaborate
>> guesswork.  blkconf_geometry() runs for IDE, SCSI and virtio-blk only.
>>
>> Your patch makes it pick the backend's geometry as reported by
>> blk_probe_geometry() before anything else.
>>
>> This is an incompatible change for backends where blk_probe_geometry()
>> succeeds.  Currently, it succeeds only for DASD, and there you *want*
>> the incompatible change.
>>
>> My point is: if we rely on blk_probe_geometry() failing except for DASD,
>> then we should call it something like blk_dasd_geometry() to make that
>> obvious.
>>
>
> This function itself is not DASD specific.

I agree, but...

>                                            It calls the inner method for
> "host_device" driver, which currently only succeeds for DASDs.
> But in future someone can define methods for other drivers or
> even modify the host_device driver.

... doing that will change the default geometry of the devices using the
backend.

At the very least, such action at a distance needs to be documented
prominently in the source.

[...]

Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Ekaterina Tumanova]

On 11/28/2014 03:52 PM, Markus Armbruster wrote:
> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:
>
>>>
>>> Suggest function comment
>>>
>>>       /**
>>>        * Return logical block size, or zero if we can't figure it out
>>>        */
>>>
>>>>    {
>>>> -    BDRVRawState *s = bs->opaque;
>>>> -    char *buf;
>>>> -    unsigned int sector_size;
>>>> -
>>>> -    /* For /dev/sg devices the alignment is not really used.
>>>> -       With buffered I/O, we don't have any restrictions. */
>>>> -    if (bs->sg || !s->needs_alignment) {
>>>> -        bs->request_alignment = 1;
>>>> -        s->buf_align = 1;
>>>> -        return;
>>>> -    }
>>>> +    unsigned int sector_size = 0;
>>>
>>> Pointless initialization.
>>
>> If I do not initialize the sector_size, and the ioctl fails,
>> I will return garbage as a blocksize to the caller.
>
> Where?  As far as I can see, we return it only after ioctl() succeeded.
>

Sorry,
you're absolutely right. I kept seeing and thinking that I always
returned sector_size variable. ::facepalm::

>>>>
>>>>        /* Try a few ioctls to get the right size */
>>>> -    bs->request_alignment = 0;
>>>> -    s->buf_align = 0;
>>>> -
>>>>    #ifdef BLKSSZGET
>>>>        if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
>>>> -        bs->request_alignment = sector_size;
>>>> +        return sector_size;
>>>>        }
>>>>    #endif
>>>>    #ifdef DKIOCGETBLOCKSIZE
>>>>        if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
>>>> -        bs->request_alignment = sector_size;
>>>> +        return sector_size;
>>>>        }
>>>>    #endif
>>>>    #ifdef DIOCGSECTORSIZE
>>>>        if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
>>>> -        bs->request_alignment = sector_size;
>>>> +        return sector_size;
>>>>        }
>>>>    #endif
>>>>    #ifdef CONFIG_XFS
>>>>        if (s->is_xfs) {
>>>>            struct dioattr da;
>>>>            if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
>>>> -            bs->request_alignment = da.d_miniosz;
>>>> +            sector_size = da.d_miniosz;
>>>>                /* The kernel returns wrong information for d_mem */
>>>>                /* s->buf_align = da.d_mem; */
>>>
>>> Since you keep the enabled assignments to s->buf_align out of this
>>> function, you should keep out this disabled one, too.
>>>
>>>> +            return sector_size;
>>>>            }
>>>>        }
>>>>    #endif
>>>>
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd)
>>>
>>> Parameter bs is unused, let's drop it.
>>>
>>>> +{
>>>> +    unsigned int blk_size = 0;
>>>
>>> Pointless initialization.
>>
>> Same here.
>
> Again, we return it only after ioctl() succeeded.
>
>>>> +#ifdef BLKPBSZGET
>>>> +    if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) {
>>>> +        return blk_size;
>>>> +    }
>>>> +#endif
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>>>> +{
>>>> +    BDRVRawState *s = bs->opaque;
>>>> +    char *buf;
>>>> +
>>>> +    /* For /dev/sg devices the alignment is not really used.
>>>> +       With buffered I/O, we don't have any restrictions. */
>>>> +    if (bs->sg || !s->needs_alignment) {
>>>> +        bs->request_alignment = 1;
>>>> +        s->buf_align = 1;
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    s->buf_align = 0;
>>>> +    /* Let's try to use the logical blocksize for the alignment. */
>>>> +    bs->request_alignment = probe_logical_blocksize(bs, fd);
>>>> +
>>>>        /* If we could not get the sizes so far, we can only guess them */
>>>>        if (!s->buf_align) {
>>>>            size_t align;
>>>
>

Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices

[Markus Armbruster]

Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:

> On 11/28/2014 01:35 PM, Stefan Hajnoczi wrote:
>> On Wed, Nov 26, 2014 at 01:16:48PM +0300, Ekaterina Tumanova wrote:
>>> On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote:
>>>> On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote:
>>>> 3. Why does s390 need to customize hd_geometry_guess()?
>>>>
>>> Since hd_geometry_guess contains semantics of x86-specific LBA translation,
>>> we have to modify it not to get in the way of z
>>> architecture
>>
>> If the behavior is x86-specific, it should be made x86-specific.  z,
>> ppc, sparc, arm, etc should share the non-x86-specific code path.  It's
>> weird to single out z here, this seems like the wrong way around.
>>
>> Is the x86-specific behavior a problem in practice?  No one seemed to
>> mind for the other targets.
>>
>
> on s390 arch this adds 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.

Isn't it like this for all targets?

    If we can't guess LCHS from MSDOS partition table
        guess geometry on size
        translation is NONE for very small disk, else LBA
    Else if LCHS guess has heads > 16
        BIOS LBA translation must be active
        guess geometry on size
        translation is LARGE for sufficiently small disk, else LBA
    Else
        use LCHS guess
        translation is NONE

Before writing a partition table, the default geometry is based on size.

Afterwards, we it may be based on the partition table instead.

Yes, writing an MS-DOS partition table can change the default geometry.
Horrible misfeature, independent of target.  Every time I try to kill a
horrible misfeature, Kevin yells at me ;)

Mitigating factor 1: no change when the partition table looks like LBA
is active.  Should be the common case nowadays.

Mitigating factor 2: most guest software is blissfully unaware of
geometry.

> If you guys don't like the way patch goes, I can exclude it from patch
> series and we can discuss it later.
>
> But I thought that since it's related to the hard drive geometry,
> and since we change hd_geometry_guess in this patchset anyway, why not
> get rid of this problem as well.

I think we should discuss what we actually want, then let you implement
it.

Perhaps the stupidest solution that could possibly work is the state
after your PATCH 5:

    If backend has a geometry (currently only DASD has; may change)
        // Incompatible change!
        use backend geometry
        translation is NONE
    Else if we can't guess LCHS from MSDOS partition table
        guess geometry on size
        translation is NONE for very small disk, else LBA
    Else if LCHS guess has heads > 16
        BIOS LBA translation must be active
        guess geometry on size
        translation is LARGE for sufficiently small disk, else LBA
    Else
        use LCHS guess
        translation is NONE

Note that translation is relevant only for a PC machine's IDE disks.
Everything else ignores it.