[Qemu-devel] [PATCH v3 0/5] Geometry and blocksize detection for backing devices.

[Qemu-devel] [PATCH v3 0/5] Geometry and blocksize detection for backing devices.

[Ekaterina Tumanova]

This 3rd revision of the patch set.

Proper geometry and blocksize information is vital for support of
DASD/ECKD drives in Linux guests. Otherwise things will fail in
certain cases.

The existing geometry and blocksize qemu defaults have no sense
for DASD drives (hd_geometry_guess detection and 512 for sizes).
Setting this information manually in XML file is far from user-friendly,
as the admin has to manually look up the properties of the
host disks and then modify the guest definition accordingly.

Since Linux uses DASDs as normal block devices, we actually
want to use virtio-blk to pass those to KVM guests.

In order to avoid any change in behavior of other drives, the DASD
special casing was advised. We call ioctl BIODASDINFO2 on the block
device, which will only succeed if the device is really a DASD.

In order to retrieve the underlying device geometry and blocksizes
a new block-backend functions and underlying driver functions were
introduced (blk_probe_blocksizes anf blk_probe_geometry wrappers
and corresponding bdrv_xxxxxx functions).

As for now only "host_device" driver received new detection methods.
For "raw" we call childs method as usual. In future one may update
other drivers to add some other detection heuristics.

If the host_device appears to be a DASD, the driver functions
(hdev_probe_blocksizes and hdev_probe_geometry) will call certain
ioctls in order to detect geometry and blocksizes of the underlying device.
if probing failed bdrv_probe_blocksizes caller will set defaults,
and bdrv_probe_geometry will fail to allow fallback to old detection logic.

The front-end (BlockConf API) was updated:
1. a new blkconf_blocksizes function was added. It doesn't
change user-defined blocksize values. If properties are unset, it will
set values, returned by blk_probe_backend. In order to allow this logic,
blocksize properties were initialized with 0. (driver will return 512 if
backing device probing didn't succeed or if driver method is not defined).
2. hd_geometry guess was changed to firstly try to retrieve values via
blk_probe_geometry and if it fails, fallback to the old logic.

updates from v2:
- avoid passing structs as a function result in backend part
- fix patch desctiptions
- add the blkconf_blocksizes call to all the users of BlockConf
- do not change geometry and blocksize values set via command-line, to do that:
remove hardcoding 512 in DEFINE_BLOCK_PROPERTIES. set defaults in
blk_probe_blocksizes (now always succeeds) and call it only if property is 0.
- fix cylinders calculation.
- document blk_probe_geometry call as "currently only for DASD".
- anything I forgot :)

Ekaterina Tumanova (5):
  block: add bdrv functions for geometry and blocksize
  raw-posix: Factor block size detection out of raw_probe_alignment()
  block: Add driver methods to probe blocksizes and geometry
  block-backend: Add wrappers for blocksizes and geometry probing
  BlockConf: Call backend functions to detect geometry and blocksizes

 block.c                        |  35 +++++++++++
 block/block-backend.c          |  10 +++
 block/raw-posix.c              | 135 +++++++++++++++++++++++++++++++++++------
 block/raw_bsd.c                |  14 +++++
 hw/block/block.c               |  18 ++++++
 hw/block/hd-geometry.c         |  12 ++++
 hw/block/nvme.c                |   1 +
 hw/block/virtio-blk.c          |   1 +
 hw/core/qdev-properties.c      |   3 +-
 hw/ide/qdev.c                  |   1 +
 hw/scsi/scsi-disk.c            |   1 +
 hw/usb/dev-storage.c           |   1 +
 include/block/block.h          |  13 ++++
 include/block/block_int.h      |   5 ++
 include/hw/block/block.h       |   5 +-
 include/sysemu/block-backend.h |   2 +
 16 files changed, 237 insertions(+), 20 deletions(-)

-- 
1.8.5.5

[Qemu-devel] [PATCH v3 1/5] block: 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                   | 35 +++++++++++++++++++++++++++++++++++
 include/block/block.h     | 13 +++++++++++++
 include/block/block_int.h |  5 +++++
 3 files changed, 53 insertions(+)

diff --git a/block.c b/block.c
index a612594..7b0b804 100644
--- a/block.c
+++ b/block.c
@@ -548,6 +548,41 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 }
 
+/**
+ * Try to get @bs's logical and physical block size.
+ * On success, store them in bsz struct.
+ * On failure, set default blocksize.
+ */
+void bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
+{
+    BlockDriver *drv = bs->drv;
+
+    assert(drv != NULL);
+    if (drv->bdrv_probe_blocksizes &&
+        !drv->bdrv_probe_blocksizes(bs, bsz)) {
+            return;
+    }
+    bsz->log = BDRV_SECTOR_SIZE;
+    bsz->phys = BDRV_SECTOR_SIZE;
+}
+
+/**
+ * Try to get @bs's geometry (cyls, heads, sectos)
+ * On success, store them in geo struct and return 0.
+ * On failure return -errno.
+ */
+int bdrv_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
+{
+    BlockDriver *drv = bs->drv;
+
+    assert(drv != NULL);
+    if (drv->bdrv_probe_geometry) {
+        return drv->bdrv_probe_geometry(bs, geo);
+    }
+
+    return -1;
+}
+
 /*
  * 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..17184b6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -60,6 +60,17 @@ typedef enum {
     BDRV_REQ_MAY_UNMAP    = 0x4,
 } BdrvRequestFlags;
 
+typedef struct BlockSizes {
+    uint32_t phys;
+    uint32_t log;
+} BlockSizes;
+
+typedef struct hdGeometry {
+    uint32_t heads;
+    uint32_t sectors;
+    uint32_t cylinders;
+} hdGeometry;
+
 #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 +549,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
  * the old #AioContext is not executing.
  */
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
+void bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
+int bdrv_probe_geometry(BlockDriverState *bs, hdGeometry *geo);
 
 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..16e53e9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -271,6 +271,11 @@ struct BlockDriver {
     void (*bdrv_io_unplug)(BlockDriverState *bs);
     void (*bdrv_flush_io_queue)(BlockDriverState *bs);
 
+    /* try to get physical and logical blocksizes */
+    int (*bdrv_probe_blocksizes)(BlockDriverState *bs, BlockSizes *bsz);
+    /* tey to get hard drive geometry (cyls, head, sectors) */
+    int (*bdrv_probe_geometry)(BlockDriverState *bs, hdGeometry *geo);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.8.5.5

[Qemu-devel] [PATCH v3 2/5] raw-posix: Factor block size detection out of raw_probe_alignment()

[Ekaterina Tumanova]

Put it in new probe_logical_blocksize().

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index b1af77e..633d5bc 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -217,11 +217,35 @@ static int raw_normalize_devicepath(const char **filename)
 }
 #endif
 
+/*
+ * Set logical block size via ioctl. On success return 0. Otherwise -errno.
+ */
+static int probe_logical_blocksize(int fd, unsigned int *sector_size)
+{
+    /* Try a few ioctls to get the right size */
+#ifdef BLKSSZGET
+    if (ioctl(fd, BLKSSZGET, sector_size) < 0) {
+        return -errno;
+    }
+#endif
+#ifdef DKIOCGETBLOCKSIZE
+    if (ioctl(fd, DKIOCGETBLOCKSIZE, sector_size) < 0) {
+        return -errno;
+    }
+#endif
+#ifdef DIOCGSECTORSIZE
+    if (ioctl(fd, DIOCGSECTORSIZE, sector_size) < 0) {
+        return -errno;
+    }
+#endif
+
+    return 0;
+}
+
 static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
 {
     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. */
@@ -231,25 +255,11 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
         return;
     }
 
-    /* Try a few ioctls to get the right size */
     bs->request_alignment = 0;
     s->buf_align = 0;
+    /* Let's try to use the logical blocksize for the alignment. */
+    probe_logical_blocksize(fd, &bs->request_alignment);
 
-#ifdef BLKSSZGET
-    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
-        bs->request_alignment = sector_size;
-    }
-#endif
-#ifdef DKIOCGETBLOCKSIZE
-    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
-        bs->request_alignment = sector_size;
-    }
-#endif
-#ifdef DIOCGSECTORSIZE
-    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
-        bs->request_alignment = sector_size;
-    }
-#endif
 #ifdef CONFIG_XFS
     if (s->is_xfs) {
         struct dioattr da;
-- 
1.8.5.5

[Qemu-devel] [PATCH v3 3/5] block: 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.

For the time being geometry detection will only work for DASD devices.
In order to check that a local check_for_dasd function was introduced,
which calls BIODASDINFO2 ioctl and returns its rc.

Blocksizes detection fuction will probe sizes for DASD devices and
set default for other devices.

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 633d5bc..33f9983 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
@@ -90,6 +91,10 @@
 #include <xfs/xfs.h>
 #endif
 
+#ifdef __s390__
+#include <asm/dasd.h>
+#endif
+
 //#define DEBUG_FLOPPY
 
 //#define DEBUG_BLOCK
@@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size)
     return 0;
 }
 
+/*
+ * Set physical block size via ioctl. On success return 0. Otherwise -errno.
+ */
+static int probe_physical_blocksize(int fd, unsigned int *blk_size)
+{
+#ifdef BLKPBSZGET
+    if (ioctl(fd, BLKPBSZGET, blk_size) < 0) {
+        return -errno;
+    }
+#endif
+
+    return 0;
+}
+
 static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
@@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
     bs->bl.opt_mem_alignment = s->buf_align;
 }
 
+static int check_for_dasd(int fd)
+{
+#ifdef BIODASDINFO2
+    struct dasd_information2_t info = {0};
+
+    return ioctl(fd, BIODASDINFO2, &info);
+#endif
+    return -1;
+}
+
+/*
+ * Try to get the device blocksize. On success 0. On failure return -errno.
+ * Currently only implemented for DASD drives.
+ */
+static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
+{
+    BDRVRawState *s = bs->opaque;
+    int ret;
+
+    /* If DASD, get blocksizes */
+    if (check_for_dasd(s->fd) < 0) {
+        return -1;
+    }
+    ret = probe_logical_blocksize(s->fd, &bsz->log);
+    if (ret < 0) {
+        return ret;
+    }
+    return probe_physical_blocksize(s->fd, &bsz->phys);
+}
+
+/*
+ * Try to get the device geometry. On success 0. On failure return -errno.
+ * Currently only implemented for DASD drives.
+ */
+static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
+{
+    BDRVRawState *s = bs->opaque;
+    struct hd_geometry ioctl_geo = {0};
+    uint32_t blksize;
+
+    /* If DASD, get it's geometry */
+    if (check_for_dasd(s->fd) < 0) {
+        return -1;
+    }
+    if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) {
+        return -errno;
+    }
+    /* 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) {
+        return -1;
+    }
+    /* Do not return a geometry for partition */
+    if (ioctl_geo.start != 0) {
+        return -1;
+    }
+    geo->heads = ioctl_geo.heads;
+    geo->sectors = ioctl_geo.sectors;
+    if (bs->total_sectors) {
+        if (!probe_physical_blocksize(s->fd, &blksize)) {
+            geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE)
+                                               / (geo->heads * geo->sectors);
+            return 0;
+        }
+    }
+    geo->cylinders = ioctl_geo.cylinders;
+
+    return 0;
+}
+
 static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
 {
     int ret;
@@ -2127,6 +2216,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..cfd5249 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
     return 1;
 }
 
+static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
+{
+    bdrv_probe_blocksizes(bs->file, bsz);
+
+    return 0;
+}
+
+static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
+{
+    return bdrv_probe_geometry(bs->file, geo);
+}
+
 static BlockDriver bdrv_raw = {
     .format_name          = "raw",
     .bdrv_probe           = &raw_probe,
@@ -190,6 +202,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 v3 4/5] block-backend: Add wrappers for blocksizes and 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..9cd97c6 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);
 }
+
+void blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz)
+{
+    bdrv_probe_blocksizes(blk->bs, bsz);
+}
+
+int blk_probe_geometry(BlockBackend *blk, hdGeometry *geo)
+{
+    return bdrv_probe_geometry(blk->bs, geo);
+}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 52d13c1..3f934c5 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);
+void blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz);
+int blk_probe_geometry(BlockBackend *blk, hdGeometry *geo);
 
 #endif
-- 
1.8.5.5

[Qemu-devel] [PATCH v3 5/5] BlockConf: Call backend functions to detect geometry and blocksizes

[Ekaterina Tumanova]

geometry: 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 for DASDs), the blkconf_geometry will pass-through
the backing device geometry. Otherwise will fallback to old logic.

blocksize: This patch initializes blocksize properties to 0.
In order to set the properly a blkconf_blocksizes was introduced.
If user didn't set physical or logical blocksize, it will
retrieve it's value from a driver (which will return a default 512 for
any backend but DASD).

The blkconf_blocksizes call was added to all users of BlkConf.

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
---
 hw/block/block.c          | 18 ++++++++++++++++++
 hw/block/hd-geometry.c    | 12 ++++++++++++
 hw/block/nvme.c           |  1 +
 hw/block/virtio-blk.c     |  1 +
 hw/core/qdev-properties.c |  3 ++-
 hw/ide/qdev.c             |  1 +
 hw/scsi/scsi-disk.c       |  1 +
 hw/usb/dev-storage.c      |  1 +
 include/hw/block/block.h  |  5 +++--
 9 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index a625773..9c07516 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -25,6 +25,24 @@ void blkconf_serial(BlockConf *conf, char **serial)
     }
 }
 
+void blkconf_blocksizes(BlockConf *conf)
+{
+    BlockBackend *blk = conf->blk;
+    BlockSizes blocksizes;
+
+    if (conf->physical_block_size && conf->logical_block_size) {
+        return;
+    }
+    blk_probe_blocksizes(blk, &blocksizes);
+
+    if (!conf->physical_block_size) {
+        conf->physical_block_size = blocksizes.phys;
+    }
+    if (!conf->logical_block_size) {
+        conf->logical_block_size = blocksizes.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..fbd602d 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -121,7 +121,17 @@ void hd_geometry_guess(BlockBackend *blk,
                        int *ptrans)
 {
     int cylinders, heads, secs, translation;
+    hdGeometry geo;
 
+    /* Try to probe the backing device geometry, otherwise fallback
+       to the old logic. (as of 12/2014 probing only succeeds on DASDs) */
+    if (blk_probe_geometry(blk, &geo) == 0) {
+        *pcyls = geo.cylinders;
+        *psecs = geo.sectors;
+        *pheads = geo.heads;
+        translation = BIOS_ATA_TRANSLATION_NONE;
+        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);
@@ -142,7 +152,9 @@ void hd_geometry_guess(BlockBackend *blk,
         /* disable any translation to be in sync with
            the logical geometry */
         translation = BIOS_ATA_TRANSLATION_NONE;
+
     }
+done:
     if (ptrans) {
         *ptrans = translation;
     }
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1327658..244e382 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -763,6 +763,7 @@ static int nvme_init(PCIDevice *pci_dev)
     if (!n->serial) {
         return -1;
     }
+    blkconf_blocksizes(&n->conf);
 
     pci_conf = pci_dev->config;
     pci_conf[PCI_INTERRUPT_PIN] = 1;
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/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 2e47f70..ba81709 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -580,7 +580,8 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque,
         error_propagate(errp, local_err);
         return;
     }
-    if (value < min || value > max) {
+    /* value of 0 means "unset" */
+    if (value && (value < min || value > max)) {
         error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
                   dev->id?:"", name, (int64_t)value, min, max);
         return;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index b4f096e..e71ff7f 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -164,6 +164,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
     }
 
     blkconf_serial(&dev->conf, &dev->serial);
+    blkconf_blocksizes(&dev->conf);
     if (kind != IDE_CD) {
         blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
         if (err) {
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 2f75d7d..5288129 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2230,6 +2230,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
     }
 
     blkconf_serial(&s->qdev.conf, &s->serial);
+    blkconf_blocksizes(&s->qdev.conf);
     if (dev->type == TYPE_DISK) {
         blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
         if (err) {
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 4539733..cc02dfd 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -611,6 +611,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
     }
 
     blkconf_serial(&s->conf, &dev->serial);
+    blkconf_blocksizes(&s->conf);
 
     /*
      * Hack alert: this pretends to be a block device, but it's really
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 0d0ce9a..3e502a8 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -44,9 +44,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
 #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
     DEFINE_PROP_DRIVE("drive", _state, _conf.blk),                      \
     DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
-                          _conf.logical_block_size, 512),               \
+                          _conf.logical_block_size, 0),               \
     DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
-                          _conf.physical_block_size, 512),              \
+                          _conf.physical_block_size, 0),              \
     DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
     DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
     DEFINE_PROP_UINT32("discard_granularity", _state, \
@@ -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

Re: [Qemu-devel] [PATCH v3 1/5] block: add bdrv functions for geometry and blocksize

[Thomas Huth]

On Fri,  5 Dec 2014 18:56:17 +0100
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote:

> Add driver functions for geometry and blocksize detection
> 
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  block.c                   | 35 +++++++++++++++++++++++++++++++++++
>  include/block/block.h     | 13 +++++++++++++
>  include/block/block_int.h |  5 +++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/block.c b/block.c
> index a612594..7b0b804 100644
> --- a/block.c
> +++ b/block.c
> @@ -548,6 +548,41 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>      }
>  }
> 
> +/**
> + * Try to get @bs's logical and physical block size.
> + * On success, store them in bsz struct.
> + * On failure, set default blocksize.
> + */
> +void bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    assert(drv != NULL);
> +    if (drv->bdrv_probe_blocksizes &&
> +        !drv->bdrv_probe_blocksizes(bs, bsz)) {
> +            return;
> +    }
> +    bsz->log = BDRV_SECTOR_SIZE;
> +    bsz->phys = BDRV_SECTOR_SIZE;
> +}
> +
> +/**
> + * Try to get @bs's geometry (cyls, heads, sectos)
> + * On success, store them in geo struct and return 0.
> + * On failure return -errno.
> + */
> +int bdrv_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    assert(drv != NULL);
> +    if (drv->bdrv_probe_geometry) {
> +        return drv->bdrv_probe_geometry(bs, geo);
> +    }
> +
> +    return -1;
> +}
> +
>  /*
>   * 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..17184b6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -60,6 +60,17 @@ typedef enum {
>      BDRV_REQ_MAY_UNMAP    = 0x4,
>  } BdrvRequestFlags;
> 
> +typedef struct BlockSizes {
> +    uint32_t phys;
> +    uint32_t log;
> +} BlockSizes;
> +
> +typedef struct hdGeometry {
> +    uint32_t heads;
> +    uint32_t sectors;
> +    uint32_t cylinders;
> +} hdGeometry;

I think the "camel-case" should already use a capital letter for the
first letter, so may I suggest to change that into "HdGeometry" instead
of "hdGeometry"? (Or maybe something like "DiskGeometry" instead?)

>  #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 +549,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>   * the old #AioContext is not executing.
>   */
>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
> +void bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
> +int bdrv_probe_geometry(BlockDriverState *bs, hdGeometry *geo);
> 
>  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..16e53e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -271,6 +271,11 @@ struct BlockDriver {
>      void (*bdrv_io_unplug)(BlockDriverState *bs);
>      void (*bdrv_flush_io_queue)(BlockDriverState *bs);
> 
> +    /* try to get physical and logical blocksizes */
> +    int (*bdrv_probe_blocksizes)(BlockDriverState *bs, BlockSizes *bsz);
> +    /* tey to get hard drive geometry (cyls, head, sectors) */
> +    int (*bdrv_probe_geometry)(BlockDriverState *bs, hdGeometry *geo);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };

Looks fine to me (apart from the very minor camel-case issue above):

Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH v3 2/5] raw-posix: Factor block size detection out of raw_probe_alignment()

[Thomas Huth]

On Fri,  5 Dec 2014 18:56:18 +0100
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote:

> Put it in new probe_logical_blocksize().
> 
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  block/raw-posix.c | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index b1af77e..633d5bc 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -217,11 +217,35 @@ static int raw_normalize_devicepath(const char **filename)
>  }
>  #endif
> 
> +/*
> + * Set logical block size via ioctl. On success return 0. Otherwise -errno.

s/Set/Get/

> + */
> +static int probe_logical_blocksize(int fd, unsigned int *sector_size)
> +{
> +    /* Try a few ioctls to get the right size */
> +#ifdef BLKSSZGET
> +    if (ioctl(fd, BLKSSZGET, sector_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +#ifdef DKIOCGETBLOCKSIZE
> +    if (ioctl(fd, DKIOCGETBLOCKSIZE, sector_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +#ifdef DIOCGSECTORSIZE
> +    if (ioctl(fd, DIOCGSECTORSIZE, sector_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +
> +    return 0;

Is "return 0" the right thing here? Or should this function rather
return an error code if no ioctl was available?
(I know you're not using the return value yet, but it might get
important later)

> +}
> +
>  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>  {
>      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. */
> @@ -231,25 +255,11 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>          return;
>      }
> 
> -    /* Try a few ioctls to get the right size */
>      bs->request_alignment = 0;
>      s->buf_align = 0;
> +    /* Let's try to use the logical blocksize for the alignment. */
> +    probe_logical_blocksize(fd, &bs->request_alignment);

Can we be sure that the &bs->request_alignment is not changed in case
the ioctl failed? If not, it might be necessary to do something like
this instead:

    if (probe_logical_blocksize(fd, &bs->request_alignment) != 0) {
        bs->request_alignment = 0;
    }

 Thomas

Re: [Qemu-devel] [PATCH v3 3/5] block: Add driver methods to probe blocksizes and geometry

[Thomas Huth]

On Fri,  5 Dec 2014 18:56:19 +0100
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote:

> 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.
> 
> For the time being geometry detection will only work for DASD devices.
> In order to check that a local check_for_dasd function was introduced,
> which calls BIODASDINFO2 ioctl and returns its rc.
> 
> Blocksizes detection fuction will probe sizes for DASD devices and
> set default for other devices.
> 
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/raw_bsd.c   | 14 +++++++++
>  2 files changed, 105 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 633d5bc..33f9983 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
> @@ -90,6 +91,10 @@
>  #include <xfs/xfs.h>
>  #endif
> 
> +#ifdef __s390__
> +#include <asm/dasd.h>
> +#endif
> +
>  //#define DEBUG_FLOPPY
> 
>  //#define DEBUG_BLOCK
> @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size)
>      return 0;
>  }
> 
> +/*
> + * Set physical block size via ioctl. On success return 0. Otherwise -errno.
> + */
> +static int probe_physical_blocksize(int fd, unsigned int *blk_size)
> +{
> +#ifdef BLKPBSZGET
> +    if (ioctl(fd, BLKPBSZGET, blk_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +
> +    return 0;
> +}
> +
>  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
> @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>      bs->bl.opt_mem_alignment = s->buf_align;
>  }
> 
> +static int check_for_dasd(int fd)
> +{
> +#ifdef BIODASDINFO2
> +    struct dasd_information2_t info = {0};
> +
> +    return ioctl(fd, BIODASDINFO2, &info);
> +#endif
> +    return -1;

I'd put the "return -1" line into an #else branch of the #ifdef, so
that you do not end up with two consecutive return statements in case
BIODASDINFO2 is defined.

> +}
> +
> +/*
> + * Try to get the device blocksize. On success 0. On failure return -errno.
> + * Currently only implemented for DASD drives.
> + */
> +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    int ret;
> +
> +    /* If DASD, get blocksizes */
> +    if (check_for_dasd(s->fd) < 0) {
> +        return -1;
> +    }
> +    ret = probe_logical_blocksize(s->fd, &bsz->log);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return probe_physical_blocksize(s->fd, &bsz->phys);
> +}
> +
> +/*
> + * Try to get the device geometry. On success 0. On failure return -errno.

"On success return 0"

> + * Currently only implemented for DASD drives.
> + */
> +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    struct hd_geometry ioctl_geo = {0};
> +    uint32_t blksize;
> +
> +    /* If DASD, get it's geometry */
> +    if (check_for_dasd(s->fd) < 0) {
> +        return -1;
> +    }
> +    if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) {
> +        return -errno;
> +    }
> +    /* 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) {
> +        return -1;
> +    }
> +    /* Do not return a geometry for partition */
> +    if (ioctl_geo.start != 0) {
> +        return -1;
> +    }
> +    geo->heads = ioctl_geo.heads;
> +    geo->sectors = ioctl_geo.sectors;
> +    if (bs->total_sectors) {

Maybe add a comment here why you've got to calculate the cylinders here
instead of using ioctl_geo.cylinders ?

> +        if (!probe_physical_blocksize(s->fd, &blksize)) {
> +            geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE)
> +                                               / (geo->heads * geo->sectors);
> +            return 0;
> +        }
> +    }
> +    geo->cylinders = ioctl_geo.cylinders;
> +
> +    return 0;
> +}
> +
>  static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
>  {
>      int ret;
> @@ -2127,6 +2216,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..cfd5249 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
>      return 1;
>  }
> 
> +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
> +{
> +    bdrv_probe_blocksizes(bs->file, bsz);
> +
> +    return 0;
> +}
> +
> +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
> +{
> +    return bdrv_probe_geometry(bs->file, geo);
> +}
> +
>  static BlockDriver bdrv_raw = {
>      .format_name          = "raw",
>      .bdrv_probe           = &raw_probe,
> @@ -190,6 +202,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,

Hmmm, raw_probe_blocksizes() calls bdrv_probe_blocksizes(), but when I
look at your patch 1/5, bdrv_probe_blocksizes() wants to call
drv->bdrv_probe_blocksizes() (i.e. raw_probe_blocksizes()) again?
Don't you get an endless recursive loop here? Or did I miss something?
*confused*

 Thomas

Re: [Qemu-devel] [PATCH v3 5/5] BlockConf: Call backend functions to detect geometry and blocksizes

[Thomas Huth]

This patch looks basically good to me, I just got some cosmetic
requests:

On Fri,  5 Dec 2014 18:56:21 +0100
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote:
...
> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
> index 6fcf74d..fbd602d 100644
> --- a/hw/block/hd-geometry.c
> +++ b/hw/block/hd-geometry.c
> @@ -121,7 +121,17 @@ void hd_geometry_guess(BlockBackend *blk,
>                         int *ptrans)
>  {
>      int cylinders, heads, secs, translation;
> +    hdGeometry geo;
> 
> +    /* Try to probe the backing device geometry, otherwise fallback
> +       to the old logic. (as of 12/2014 probing only succeeds on DASDs) */
> +    if (blk_probe_geometry(blk, &geo) == 0) {
> +        *pcyls = geo.cylinders;
> +        *psecs = geo.sectors;
> +        *pheads = geo.heads;
> +        translation = BIOS_ATA_TRANSLATION_NONE;
> +        goto done;
> +    }
>      if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {

Can you get rid of the "goto" above by placing an "else" in front of
this if-statement?

>          /* no LCHS guess: use a standard physical disk geometry  */
>          guess_chs_for_size(blk, pcyls, pheads, psecs);
> @@ -142,7 +152,9 @@ void hd_geometry_guess(BlockBackend *blk,
>          /* disable any translation to be in sync with
>             the logical geometry */
>          translation = BIOS_ATA_TRANSLATION_NONE;
> +

Unnecessary empty line, please remove this hunk.

>      }
> +done:
>      if (ptrans) {
>          *ptrans = translation;
>      }

 Thomas

Re: [Qemu-devel] [PATCH v3 3/5] block: Add driver methods to probe blocksizes and geometry

[Ekaterina Tumanova]

On 12/10/2014 04:14 PM, Thomas Huth wrote:
> On Fri,  5 Dec 2014 18:56:19 +0100
> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote:
>
>> 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.
>>
>> For the time being geometry detection will only work for DASD devices.
>> In order to check that a local check_for_dasd function was introduced,
>> which calls BIODASDINFO2 ioctl and returns its rc.
>>
>> Blocksizes detection fuction will probe sizes for DASD devices and
>> set default for other devices.
>>
>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> ---
>>   block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/raw_bsd.c   | 14 +++++++++
>>   2 files changed, 105 insertions(+)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 633d5bc..33f9983 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
>> @@ -90,6 +91,10 @@
>>   #include <xfs/xfs.h>
>>   #endif
>>
>> +#ifdef __s390__
>> +#include <asm/dasd.h>
>> +#endif
>> +
>>   //#define DEBUG_FLOPPY
>>
>>   //#define DEBUG_BLOCK
>> @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size)
>>       return 0;
>>   }
>>
>> +/*
>> + * Set physical block size via ioctl. On success return 0. Otherwise -errno.
>> + */
>> +static int probe_physical_blocksize(int fd, unsigned int *blk_size)
>> +{
>> +#ifdef BLKPBSZGET
>> +    if (ioctl(fd, BLKPBSZGET, blk_size) < 0) {
>> +        return -errno;
>> +    }
>> +#endif
>> +
>> +    return 0;
>> +}
>> +
>>   static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>>   {
>>       BDRVRawState *s = bs->opaque;
>> @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>       bs->bl.opt_mem_alignment = s->buf_align;
>>   }
>>
>> +static int check_for_dasd(int fd)
>> +{
>> +#ifdef BIODASDINFO2
>> +    struct dasd_information2_t info = {0};
>> +
>> +    return ioctl(fd, BIODASDINFO2, &info);
>> +#endif
>> +    return -1;
>
> I'd put the "return -1" line into an #else branch of the #ifdef, so
> that you do not end up with two consecutive return statements in case
> BIODASDINFO2 is defined.
>
>> +}
>> +
>> +/*
>> + * Try to get the device blocksize. On success 0. On failure return -errno.
>> + * Currently only implemented for DASD drives.
>> + */
>> +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>> +{
>> +    BDRVRawState *s = bs->opaque;
>> +    int ret;
>> +
>> +    /* If DASD, get blocksizes */
>> +    if (check_for_dasd(s->fd) < 0) {
>> +        return -1;
>> +    }
>> +    ret = probe_logical_blocksize(s->fd, &bsz->log);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    return probe_physical_blocksize(s->fd, &bsz->phys);
>> +}
>> +
>> +/*
>> + * Try to get the device geometry. On success 0. On failure return -errno.
>
> "On success return 0"
>
>> + * Currently only implemented for DASD drives.
>> + */
>> +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
>> +{
>> +    BDRVRawState *s = bs->opaque;
>> +    struct hd_geometry ioctl_geo = {0};
>> +    uint32_t blksize;
>> +
>> +    /* If DASD, get it's geometry */
>> +    if (check_for_dasd(s->fd) < 0) {
>> +        return -1;
>> +    }
>> +    if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) {
>> +        return -errno;
>> +    }
>> +    /* 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) {
>> +        return -1;
>> +    }
>> +    /* Do not return a geometry for partition */
>> +    if (ioctl_geo.start != 0) {
>> +        return -1;
>> +    }
>> +    geo->heads = ioctl_geo.heads;
>> +    geo->sectors = ioctl_geo.sectors;
>> +    if (bs->total_sectors) {
>
> Maybe add a comment here why you've got to calculate the cylinders here
> instead of using ioctl_geo.cylinders ?
>
>> +        if (!probe_physical_blocksize(s->fd, &blksize)) {
>> +            geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE)
>> +                                               / (geo->heads * geo->sectors);
>> +            return 0;
>> +        }
>> +    }
>> +    geo->cylinders = ioctl_geo.cylinders;
>> +
>> +    return 0;
>> +}
>> +
>>   static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
>>   {
>>       int ret;
>> @@ -2127,6 +2216,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..cfd5249 100644
>> --- a/block/raw_bsd.c
>> +++ b/block/raw_bsd.c
>> @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
>>       return 1;
>>   }
>>
>> +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>> +{
>> +    bdrv_probe_blocksizes(bs->file, bsz);
>> +
>> +    return 0;
>> +}
>> +
>> +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
>> +{
>> +    return bdrv_probe_geometry(bs->file, geo);
>> +}
>> +
>>   static BlockDriver bdrv_raw = {
>>       .format_name          = "raw",
>>       .bdrv_probe           = &raw_probe,
>> @@ -190,6 +202,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,
>
> Hmmm, raw_probe_blocksizes() calls bdrv_probe_blocksizes(), but when I
> look at your patch 1/5, bdrv_probe_blocksizes() wants to call
> drv->bdrv_probe_blocksizes() (i.e. raw_probe_blocksizes()) again?
> Don't you get an endless recursive loop here? Or did I miss something?
> *confused*
>
>   Thomas
>
>

No I don't :) Because raw_probe_blocksizes indeed calls 
bdrv_probe_blocksizes() dispatcher, but it calls it against different
driver: "bs->file". This child points to other driver, which represents
the actual nature of the device.

So the 2nd drv->bdrv_probe_blocksizes() call will actually be
a call to either hdev_probe_blocksizes() for "host_device" driver or
will be null for other drivers.

Kate.

Re: [Qemu-devel] [PATCH v3 3/5] block: Add driver methods to probe blocksizes and geometry

[Thomas Huth]

On Thu, 11 Dec 2014 14:17:21 +0300
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote:

> On 12/10/2014 04:14 PM, Thomas Huth wrote:
> > On Fri,  5 Dec 2014 18:56:19 +0100
> > Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote:
...
> >> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> >> index 401b967..cfd5249 100644
> >> --- a/block/raw_bsd.c
> >> +++ b/block/raw_bsd.c
> >> @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
> >>       return 1;
> >>   }
> >>
> >> +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
> >> +{
> >> +    bdrv_probe_blocksizes(bs->file, bsz);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
> >> +{
> >> +    return bdrv_probe_geometry(bs->file, geo);
> >> +}
> >> +
> >>   static BlockDriver bdrv_raw = {
> >>       .format_name          = "raw",
> >>       .bdrv_probe           = &raw_probe,
> >> @@ -190,6 +202,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,
> >
> > Hmmm, raw_probe_blocksizes() calls bdrv_probe_blocksizes(), but when I
> > look at your patch 1/5, bdrv_probe_blocksizes() wants to call
> > drv->bdrv_probe_blocksizes() (i.e. raw_probe_blocksizes()) again?
> > Don't you get an endless recursive loop here? Or did I miss something?
> > *confused*
> >
> >   Thomas
> 
> No I don't :) Because raw_probe_blocksizes indeed calls 
> bdrv_probe_blocksizes() dispatcher, but it calls it against different
> driver: "bs->file". This child points to other driver, which represents
> the actual nature of the device.
> 
> So the 2nd drv->bdrv_probe_blocksizes() call will actually be
> a call to either hdev_probe_blocksizes() for "host_device" driver or
> will be null for other drivers.

Ah, ok, you're right of course! Thanks for the explanation and sorry
for the confusion!

 Thomas

Re: [Qemu-devel] [PATCH v3 0/5] Geometry and blocksize detection for backing devices.

[Ekaterina Tumanova]

On 12/05/2014 08:56 PM, Ekaterina Tumanova wrote:
> This 3rd revision of the patch set.
>
> Proper geometry and blocksize information is vital for support of
> DASD/ECKD drives in Linux guests. Otherwise things will fail in
> certain cases.
>
> The existing geometry and blocksize qemu defaults have no sense
> for DASD drives (hd_geometry_guess detection and 512 for sizes).
> Setting this information manually in XML file is far from user-friendly,
> as the admin has to manually look up the properties of the
> host disks and then modify the guest definition accordingly.
>
> Since Linux uses DASDs as normal block devices, we actually
> want to use virtio-blk to pass those to KVM guests.
>
> In order to avoid any change in behavior of other drives, the DASD
> special casing was advised. We call ioctl BIODASDINFO2 on the block
> device, which will only succeed if the device is really a DASD.
>
> In order to retrieve the underlying device geometry and blocksizes
> a new block-backend functions and underlying driver functions were
> introduced (blk_probe_blocksizes anf blk_probe_geometry wrappers
> and corresponding bdrv_xxxxxx functions).
>
> As for now only "host_device" driver received new detection methods.
> For "raw" we call childs method as usual. In future one may update
> other drivers to add some other detection heuristics.
>
> If the host_device appears to be a DASD, the driver functions
> (hdev_probe_blocksizes and hdev_probe_geometry) will call certain
> ioctls in order to detect geometry and blocksizes of the underlying device.
> if probing failed bdrv_probe_blocksizes caller will set defaults,
> and bdrv_probe_geometry will fail to allow fallback to old detection logic.
>
> The front-end (BlockConf API) was updated:
> 1. a new blkconf_blocksizes function was added. It doesn't
> change user-defined blocksize values. If properties are unset, it will
> set values, returned by blk_probe_backend. In order to allow this logic,
> blocksize properties were initialized with 0. (driver will return 512 if
> backing device probing didn't succeed or if driver method is not defined).
> 2. hd_geometry guess was changed to firstly try to retrieve values via
> blk_probe_geometry and if it fails, fallback to the old logic.
>
> updates from v2:
> - avoid passing structs as a function result in backend part
> - fix patch desctiptions
> - add the blkconf_blocksizes call to all the users of BlockConf
> - do not change geometry and blocksize values set via command-line, to do that:
> remove hardcoding 512 in DEFINE_BLOCK_PROPERTIES. set defaults in
> blk_probe_blocksizes (now always succeeds) and call it only if property is 0.
> - fix cylinders calculation.
> - document blk_probe_geometry call as "currently only for DASD".
> - anything I forgot :)
>
> Ekaterina Tumanova (5):
>    block: add bdrv functions for geometry and blocksize
>    raw-posix: Factor block size detection out of raw_probe_alignment()
>    block: Add driver methods to probe blocksizes and geometry
>    block-backend: Add wrappers for blocksizes and geometry probing
>    BlockConf: Call backend functions to detect geometry and blocksizes
>
>   block.c                        |  35 +++++++++++
>   block/block-backend.c          |  10 +++
>   block/raw-posix.c              | 135 +++++++++++++++++++++++++++++++++++------
>   block/raw_bsd.c                |  14 +++++
>   hw/block/block.c               |  18 ++++++
>   hw/block/hd-geometry.c         |  12 ++++
>   hw/block/nvme.c                |   1 +
>   hw/block/virtio-blk.c          |   1 +
>   hw/core/qdev-properties.c      |   3 +-
>   hw/ide/qdev.c                  |   1 +
>   hw/scsi/scsi-disk.c            |   1 +
>   hw/usb/dev-storage.c           |   1 +
>   include/block/block.h          |  13 ++++
>   include/block/block_int.h      |   5 ++
>   include/hw/block/block.h       |   5 +-
>   include/sysemu/block-backend.h |   2 +
>   16 files changed, 237 insertions(+), 20 deletions(-)
>

friendly *ping* to maintainers of block layer :) I would rather not send 
next version without getting your feedback first.

Re: [Qemu-devel] [PATCH v3 1/5] block: add bdrv functions for geometry and blocksize

[Markus Armbruster]

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                   | 35 +++++++++++++++++++++++++++++++++++
>  include/block/block.h     | 13 +++++++++++++
>  include/block/block_int.h |  5 +++++
>  3 files changed, 53 insertions(+)
>
> diff --git a/block.c b/block.c
> index a612594..7b0b804 100644
> --- a/block.c
> +++ b/block.c
> @@ -548,6 +548,41 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>      }
>  }
>  
> +/**
> + * Try to get @bs's logical and physical block size.
> + * On success, store them in bsz struct.

@bsz

> + * On failure, set default blocksize.
> + */

Suggest not to talk about failure here.  Like this:

/**
 * Get @bs's logical and physical block size, store them in @bsz.
 */

> +void bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    assert(drv != NULL);

Dies when @bs is empty (no medium).  Either return default block sizes
instead, or amend the function contract with "@bs must not be empty."

Should've caught that in v2, sorry.

> +    if (drv->bdrv_probe_blocksizes &&
> +        !drv->bdrv_probe_blocksizes(bs, bsz)) {
> +            return;
> +    }
> +    bsz->log = BDRV_SECTOR_SIZE;
> +    bsz->phys = BDRV_SECTOR_SIZE;
> +}
> +
> +/**
> + * Try to get @bs's geometry (cyls, heads, sectos)
> + * On success, store them in geo struct and return 0.

... store it in @geo and ...

> + * On failure return -errno.
> + */
> +int bdrv_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    assert(drv != NULL);

Dies when @bs is empty (no medium).  Either return -ENOMEDIUM instead,
or amend the function contract with "@bs must not be empty."  The former
seems simpler to me.

> +    if (drv->bdrv_probe_geometry) {
> +        return drv->bdrv_probe_geometry(bs, geo);
> +    }
> +
> +    return -1;

This is not a negative errno!  I'd return -ENOTSUP.

> +}
> +
>  /*
>   * 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..17184b6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -60,6 +60,17 @@ typedef enum {
>      BDRV_REQ_MAY_UNMAP    = 0x4,
>  } BdrvRequestFlags;
>  
> +typedef struct BlockSizes {
> +    uint32_t phys;
> +    uint32_t log;
> +} BlockSizes;
> +
> +typedef struct hdGeometry {
> +    uint32_t heads;
> +    uint32_t sectors;
> +    uint32_t cylinders;
> +} hdGeometry;
> +
>  #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 +549,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>   * the old #AioContext is not executing.
>   */
>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
> +void bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
> +int bdrv_probe_geometry(BlockDriverState *bs, hdGeometry *geo);
>  
>  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..16e53e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -271,6 +271,11 @@ struct BlockDriver {
>      void (*bdrv_io_unplug)(BlockDriverState *bs);
>      void (*bdrv_flush_io_queue)(BlockDriverState *bs);
>  
> +    /* try to get physical and logical blocksizes */
> +    int (*bdrv_probe_blocksizes)(BlockDriverState *bs, BlockSizes *bsz);

I recommend a function contract, not just a paraphrasing of the function
name.  The one you put in block.c needs just a small touchup to fit
here nicely:

/**
 * Try to get @bs's logical and physical block size.
 * On success, store them in @bsz and return zero.
 * On failure, return negative errno.
 */

> +    /* tey to get hard drive geometry (cyls, head, sectors) */

s/tey/try/

> +    int (*bdrv_probe_geometry)(BlockDriverState *bs, hdGeometry *geo);
> +

/**
 * Try to get @bs's geometry (cyls, heads, sectos)
 * On success, store them in @geo and return 0.
 * On failure return -errno.
 */

>      QLIST_ENTRY(BlockDriver) list;
>  };

Re: [Qemu-devel] [PATCH v3 2/5] raw-posix: Factor block size detection out of raw_probe_alignment()

[Markus Armbruster]

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

> Put it in new probe_logical_blocksize().
>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  block/raw-posix.c | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index b1af77e..633d5bc 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -217,11 +217,35 @@ static int raw_normalize_devicepath(const char **filename)
>  }
>  #endif
>  
> +/*
> + * Set logical block size via ioctl. On success return 0. Otherwise -errno.

Set?  I suspect you mean get.

> + */
> +static int probe_logical_blocksize(int fd, unsigned int *sector_size)
> +{
> +    /* Try a few ioctls to get the right size */
> +#ifdef BLKSSZGET
> +    if (ioctl(fd, BLKSSZGET, sector_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +#ifdef DKIOCGETBLOCKSIZE
> +    if (ioctl(fd, DKIOCGETBLOCKSIZE, sector_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +#ifdef DIOCGSECTORSIZE
> +    if (ioctl(fd, DIOCGSECTORSIZE, sector_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +
> +    return 0;
> +}
> +
>  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>  {
>      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. */
> @@ -231,25 +255,11 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>          return;
>      }
>  
> -    /* Try a few ioctls to get the right size */
>      bs->request_alignment = 0;
>      s->buf_align = 0;
> +    /* Let's try to use the logical blocksize for the alignment. */
> +    probe_logical_blocksize(fd, &bs->request_alignment);

I figure this works, but perhaps the following would be clearer:

       if (probe_logical_blocksize(fd, &bs->request_alignment) < 0) {
           bs->request_alignment = 0;
       }

Your call.

>  
> -#ifdef BLKSSZGET
> -    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> -    }
> -#endif
> -#ifdef DKIOCGETBLOCKSIZE
> -    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> -    }
> -#endif
> -#ifdef DIOCGSECTORSIZE
> -    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> -    }
> -#endif
>  #ifdef CONFIG_XFS
>      if (s->is_xfs) {
>          struct dioattr da;

Re: [Qemu-devel] [PATCH v3 3/5] block: 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.
>
> For the time being geometry detection will only work for DASD devices.
> In order to check that a local check_for_dasd function was introduced,
> which calls BIODASDINFO2 ioctl and returns its rc.
>
> Blocksizes detection fuction will probe sizes for DASD devices and
> set default for other devices.
>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/raw_bsd.c   | 14 +++++++++
>  2 files changed, 105 insertions(+)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 633d5bc..33f9983 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
> @@ -90,6 +91,10 @@
>  #include <xfs/xfs.h>
>  #endif
>  
> +#ifdef __s390__
> +#include <asm/dasd.h>
> +#endif
> +
>  //#define DEBUG_FLOPPY
>  
>  //#define DEBUG_BLOCK
> @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size)
>      return 0;
>  }
>  
> +/*
> + * Set physical block size via ioctl. On success return 0. Otherwise -errno.
> + */

Set?

Ah, now I get your logic, you mean "set *blk_size"!

Suggest to say it like this:

/**
 * Get physical block size of @fd.
 * On success, store it in @blk_size and return 0.
 * On failure, return -errno.
 */

> +static int probe_physical_blocksize(int fd, unsigned int *blk_size)
> +{
> +#ifdef BLKPBSZGET
> +    if (ioctl(fd, BLKPBSZGET, blk_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +
> +    return 0;

If !defined(BLKPBSZGET), you return 0 without setting *blk_size.  I
think you need to fail then.  -ENOTSUP should do.

> +}
> +
>  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
> @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>      bs->bl.opt_mem_alignment = s->buf_align;
>  }
>  
> +static int check_for_dasd(int fd)
> +{
> +#ifdef BIODASDINFO2
> +    struct dasd_information2_t info = {0};
> +
> +    return ioctl(fd, BIODASDINFO2, &info);
> +#endif
> +    return -1;
> +}
> +
> +/*
> + * Try to get the device blocksize. On success 0. On failure return -errno.
> + * Currently only implemented for DASD drives.
> + */

Compare to the function contract I wrote for the callback in my review
of PATCH 1.  If you like that one better, feel free to reuse it here.

> +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    int ret;
> +
> +    /* If DASD, get blocksizes */
> +    if (check_for_dasd(s->fd) < 0) {
> +        return -1;

This is not a negative error code!  -ENOTSUP.

> +    }
> +    ret = probe_logical_blocksize(s->fd, &bsz->log);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return probe_physical_blocksize(s->fd, &bsz->phys);
> +}
> +
> +/*
> + * Try to get the device geometry. On success 0. On failure return -errno.
> + * Currently only implemented for DASD drives.
> + */

Compare to the function contract I wrote for the callback in my review
of PATCH 1.  If you like that one better, feel free to reuse it here.

> +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    struct hd_geometry ioctl_geo = {0};
> +    uint32_t blksize;
> +
> +    /* If DASD, get it's geometry */
> +    if (check_for_dasd(s->fd) < 0) {
> +        return -1;

This is not a negative error code!  -ENOTSUP.

> +    }
> +    if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) {
> +        return -errno;
> +    }
> +    /* 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) {
> +        return -1;

Need an error code.

> +    }
> +    /* Do not return a geometry for partition */
> +    if (ioctl_geo.start != 0) {
> +        return -1;

Need an error code.

> +    }
> +    geo->heads = ioctl_geo.heads;
> +    geo->sectors = ioctl_geo.sectors;
> +    if (bs->total_sectors) {
> +        if (!probe_physical_blocksize(s->fd, &blksize)) {
> +            geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE)
> +                                               / (geo->heads * geo->sectors);
> +            return 0;
> +        }
> +    }

How could !bs->total_sectors happen?

> +    geo->cylinders = ioctl_geo.cylinders;
> +
> +    return 0;
> +}
> +
>  static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
>  {
>      int ret;
> @@ -2127,6 +2216,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..cfd5249 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
>      return 1;
>  }
>  
> +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
> +{
> +    bdrv_probe_blocksizes(bs->file, bsz);
> +
> +    return 0;
> +}

Correct, just a bit awkward due to the difference between
bdrv_probe_blocksizes() and ->bdrv_probe_blocksizes().  I guess I'd make
them the same, but it's your patch.

> +
> +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
> +{
> +    return bdrv_probe_geometry(bs->file, geo);
> +}
> +
>  static BlockDriver bdrv_raw = {
>      .format_name          = "raw",
>      .bdrv_probe           = &raw_probe,
> @@ -190,6 +202,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 v3 5/5] BlockConf: Call backend functions to detect geometry and blocksizes

[Markus Armbruster]

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

> geometry: 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 for DASDs), the blkconf_geometry will pass-through
> the backing device geometry. Otherwise will fallback to old logic.
>
> blocksize: This patch initializes blocksize properties to 0.
> In order to set the properly a blkconf_blocksizes was introduced.
> If user didn't set physical or logical blocksize, it will
> retrieve it's value from a driver (which will return a default 512 for
> any backend but DASD).
>
> The blkconf_blocksizes call was added to all users of BlkConf.
>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  hw/block/block.c          | 18 ++++++++++++++++++
>  hw/block/hd-geometry.c    | 12 ++++++++++++
>  hw/block/nvme.c           |  1 +
>  hw/block/virtio-blk.c     |  1 +
>  hw/core/qdev-properties.c |  3 ++-
>  hw/ide/qdev.c             |  1 +
>  hw/scsi/scsi-disk.c       |  1 +
>  hw/usb/dev-storage.c      |  1 +
>  include/hw/block/block.h  |  5 +++--
>  9 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/hw/block/block.c b/hw/block/block.c
> index a625773..9c07516 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -25,6 +25,24 @@ void blkconf_serial(BlockConf *conf, char **serial)
>      }
>  }
>  
> +void blkconf_blocksizes(BlockConf *conf)
> +{
> +    BlockBackend *blk = conf->blk;
> +    BlockSizes blocksizes;
> +
> +    if (conf->physical_block_size && conf->logical_block_size) {
> +        return;
> +    }

This conditional isn't necessary for correctness.  Feel free to drop it.

> +    blk_probe_blocksizes(blk, &blocksizes);
> +
> +    if (!conf->physical_block_size) {
> +        conf->physical_block_size = blocksizes.phys;
> +    }
> +    if (!conf->logical_block_size) {
> +        conf->logical_block_size = blocksizes.log;
> +    }

This works because you change the default block size to 0 (second to
last hunk).

> +}
> +
>  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..fbd602d 100644
> --- a/hw/block/hd-geometry.c
> +++ b/hw/block/hd-geometry.c
> @@ -121,7 +121,17 @@ void hd_geometry_guess(BlockBackend *blk,
>                         int *ptrans)
>  {
>      int cylinders, heads, secs, translation;
> +    hdGeometry geo;
>  
> +    /* Try to probe the backing device geometry, otherwise fallback
> +       to the old logic. (as of 12/2014 probing only succeeds on DASDs) */
> +    if (blk_probe_geometry(blk, &geo) == 0) {
> +        *pcyls = geo.cylinders;
> +        *psecs = geo.sectors;
> +        *pheads = geo.heads;
> +        translation = BIOS_ATA_TRANSLATION_NONE;
> +        goto done;
> +    }

"else if" instead of goto, please.

>      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);
> @@ -142,7 +152,9 @@ void hd_geometry_guess(BlockBackend *blk,
>          /* disable any translation to be in sync with
>             the logical geometry */
>          translation = BIOS_ATA_TRANSLATION_NONE;
> +
>      }

Humor me: put the empty line behind the brace instead of before.

> +done:
>      if (ptrans) {
>          *ptrans = translation;
>      }

Much easier to gauge than v2.  Geometry change is a compatibility
problem.  You change it only when blk_probe_geometry() succeeds.  It
succeeds only for DASD.  Mission accomplished.

Recommend to add a hint to the function contract of the
bdrv_probe_geometry() callback in block_int.h:

    /**
     * Try to get @bs's geometry (cyls, heads, sectos)
     * On success, store them in @geo and return 0.
     * On failure return -errno.
     * Only drivers that want to override guest geometry implement this
     * callback; see hd_geometry_guess().
     */
    int (*bdrv_probe_geometry)(BlockDriverState *bs, hdGeometry *geo);

> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 1327658..244e382 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -763,6 +763,7 @@ static int nvme_init(PCIDevice *pci_dev)
>      if (!n->serial) {
>          return -1;
>      }
> +    blkconf_blocksizes(&n->conf);
>  
>      pci_conf = pci_dev->config;
>      pci_conf[PCI_INTERRUPT_PIN] = 1;
> 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/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2e47f70..ba81709 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -580,7 +580,8 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque,
>          error_propagate(errp, local_err);
>          return;
>      }
> -    if (value < min || value > max) {
> +    /* value of 0 means "unset" */
> +    if (value && (value < min || value > max)) {
>          error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
>                    dev->id?:"", name, (int64_t)value, min, max);
>          return;
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index b4f096e..e71ff7f 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -164,6 +164,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>      }
>  
>      blkconf_serial(&dev->conf, &dev->serial);
> +    blkconf_blocksizes(&dev->conf);
>      if (kind != IDE_CD) {
>          blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
>          if (err) {
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 2f75d7d..5288129 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2230,6 +2230,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
>      }
>  
>      blkconf_serial(&s->qdev.conf, &s->serial);
> +    blkconf_blocksizes(&s->qdev.conf);
>      if (dev->type == TYPE_DISK) {
>          blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
>          if (err) {
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 4539733..cc02dfd 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -611,6 +611,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
>      }
>  
>      blkconf_serial(&s->conf, &dev->serial);
> +    blkconf_blocksizes(&s->conf);
>  
>      /*
>       * Hack alert: this pretends to be a block device, but it's really
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 0d0ce9a..3e502a8 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -44,9 +44,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>  #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
>      DEFINE_PROP_DRIVE("drive", _state, _conf.blk),                      \
>      DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
> -                          _conf.logical_block_size, 512),               \
> +                          _conf.logical_block_size, 0),               \
>      DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
> -                          _conf.physical_block_size, 512),              \
> +                          _conf.physical_block_size, 0),              \
>      DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
>      DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
>      DEFINE_PROP_UINT32("discard_granularity", _state, \
> @@ -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 v3 0/5] Geometry and blocksize detection for backing devices.

[Markus Armbruster]

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

> This 3rd revision of the patch set.

No longer applies cleanly, please rebase.

Sorry for taking so long to review.  Your general approach looks sound
to me now.  There are a few trivial bugs / inaccuracies here and there,
and of course I found some nits to pick.

Re: [Qemu-devel] [PATCH v3 3/5] block: Add driver methods to probe blocksizes and geometry

[Ekaterina Tumanova]

On 12/15/2014 04:29 PM, Markus Armbruster wrote:
> 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.
>>
>> For the time being geometry detection will only work for DASD devices.
>> In order to check that a local check_for_dasd function was introduced,
>> which calls BIODASDINFO2 ioctl and returns its rc.
>>
>> Blocksizes detection fuction will probe sizes for DASD devices and
>> set default for other devices.
>>
>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> ---
>>   block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/raw_bsd.c   | 14 +++++++++
>>   2 files changed, 105 insertions(+)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 633d5bc..33f9983 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
>> @@ -90,6 +91,10 @@
>>   #include <xfs/xfs.h>
>>   #endif
>>
>> +#ifdef __s390__
>> +#include <asm/dasd.h>
>> +#endif
>> +
>>   //#define DEBUG_FLOPPY
>>
>>   //#define DEBUG_BLOCK
>> @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size)
>>       return 0;
>>   }
>>
>> +/*
>> + * Set physical block size via ioctl. On success return 0. Otherwise -errno.
>> + */
>
> Set?
>
> Ah, now I get your logic, you mean "set *blk_size"!
>
> Suggest to say it like this:
>
> /**
>   * Get physical block size of @fd.
>   * On success, store it in @blk_size and return 0.
>   * On failure, return -errno.
>   */
>
>> +static int probe_physical_blocksize(int fd, unsigned int *blk_size)
>> +{
>> +#ifdef BLKPBSZGET
>> +    if (ioctl(fd, BLKPBSZGET, blk_size) < 0) {
>> +        return -errno;
>> +    }
>> +#endif
>> +
>> +    return 0;
>
> If !defined(BLKPBSZGET), you return 0 without setting *blk_size.  I
> think you need to fail then.  -ENOTSUP should do.
>
>> +}
>> +
>>   static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>>   {
>>       BDRVRawState *s = bs->opaque;
>> @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>       bs->bl.opt_mem_alignment = s->buf_align;
>>   }
>>
>> +static int check_for_dasd(int fd)
>> +{
>> +#ifdef BIODASDINFO2
>> +    struct dasd_information2_t info = {0};
>> +
>> +    return ioctl(fd, BIODASDINFO2, &info);
>> +#endif
>> +    return -1;
>> +}
>> +
>> +/*
>> + * Try to get the device blocksize. On success 0. On failure return -errno.
>> + * Currently only implemented for DASD drives.
>> + */
>
> Compare to the function contract I wrote for the callback in my review
> of PATCH 1.  If you like that one better, feel free to reuse it here.
>
>> +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>> +{
>> +    BDRVRawState *s = bs->opaque;
>> +    int ret;
>> +
>> +    /* If DASD, get blocksizes */
>> +    if (check_for_dasd(s->fd) < 0) {
>> +        return -1;
>
> This is not a negative error code!  -ENOTSUP.
>
>> +    }
>> +    ret = probe_logical_blocksize(s->fd, &bsz->log);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    return probe_physical_blocksize(s->fd, &bsz->phys);
>> +}
>> +
>> +/*
>> + * Try to get the device geometry. On success 0. On failure return -errno.
>> + * Currently only implemented for DASD drives.
>> + */
>
> Compare to the function contract I wrote for the callback in my review
> of PATCH 1.  If you like that one better, feel free to reuse it here.
>
>> +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
>> +{
>> +    BDRVRawState *s = bs->opaque;
>> +    struct hd_geometry ioctl_geo = {0};
>> +    uint32_t blksize;
>> +
>> +    /* If DASD, get it's geometry */
>> +    if (check_for_dasd(s->fd) < 0) {
>> +        return -1;
>
> This is not a negative error code!  -ENOTSUP.
>
>> +    }
>> +    if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) {
>> +        return -errno;
>> +    }
>> +    /* 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) {
>> +        return -1;
>
> Need an error code.
>
>> +    }
>> +    /* Do not return a geometry for partition */
>> +    if (ioctl_geo.start != 0) {
>> +        return -1;
>
> Need an error code.
>
>> +    }
>> +    geo->heads = ioctl_geo.heads;
>> +    geo->sectors = ioctl_geo.sectors;
>> +    if (bs->total_sectors) {
>> +        if (!probe_physical_blocksize(s->fd, &blksize)) {
>> +            geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE)
>> +                                               / (geo->heads * geo->sectors);
>> +            return 0;
>> +        }
>> +    }
>
> How could !bs->total_sectors happen?
>
>> +    geo->cylinders = ioctl_geo.cylinders;
>> +
>> +    return 0;
>> +}
>> +
>>   static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
>>   {
>>       int ret;
>> @@ -2127,6 +2216,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..cfd5249 100644
>> --- a/block/raw_bsd.c
>> +++ b/block/raw_bsd.c
>> @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
>>       return 1;
>>   }
>>
>> +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>> +{
>> +    bdrv_probe_blocksizes(bs->file, bsz);
>> +
>> +    return 0;
>> +}
>
> Correct, just a bit awkward due to the difference between
> bdrv_probe_blocksizes() and ->bdrv_probe_blocksizes().  I guess I'd make
> them the same, but it's your patch.
>

I think would stick with my approach. You mentioned during review of
v2, that blk_probe_blocksizes should always just work. And I think it's
right.
An error code from a method allows me to set a default in the wrapper.
So wrapper will never fail.

Thanks a lot for your review! I'll try to post a new version ASAP.

>> +
>> +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
>> +{
>> +    return bdrv_probe_geometry(bs->file, geo);
>> +}
>> +
>>   static BlockDriver bdrv_raw = {
>>       .format_name          = "raw",
>>       .bdrv_probe           = &raw_probe,
>> @@ -190,6 +202,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 v3 5/5] BlockConf: Call backend functions to detect geometry and blocksizes

[Ekaterina Tumanova]

On 12/15/2014 04:50 PM, Markus Armbruster wrote:
> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:
>
>> geometry: 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 for DASDs), the blkconf_geometry will pass-through
>> the backing device geometry. Otherwise will fallback to old logic.
>>
>> blocksize: This patch initializes blocksize properties to 0.
>> In order to set the properly a blkconf_blocksizes was introduced.
>> If user didn't set physical or logical blocksize, it will
>> retrieve it's value from a driver (which will return a default 512 for
>> any backend but DASD).
>>
>> The blkconf_blocksizes call was added to all users of BlkConf.
>>
>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> ---
>>   hw/block/block.c          | 18 ++++++++++++++++++
>>   hw/block/hd-geometry.c    | 12 ++++++++++++
>>   hw/block/nvme.c           |  1 +
>>   hw/block/virtio-blk.c     |  1 +
>>   hw/core/qdev-properties.c |  3 ++-
>>   hw/ide/qdev.c             |  1 +
>>   hw/scsi/scsi-disk.c       |  1 +
>>   hw/usb/dev-storage.c      |  1 +
>>   include/hw/block/block.h  |  5 +++--
>>   9 files changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/block/block.c b/hw/block/block.c
>> index a625773..9c07516 100644
>> --- a/hw/block/block.c
>> +++ b/hw/block/block.c
>> @@ -25,6 +25,24 @@ void blkconf_serial(BlockConf *conf, char **serial)
>>       }
>>   }
>>
>> +void blkconf_blocksizes(BlockConf *conf)
>> +{
>> +    BlockBackend *blk = conf->blk;
>> +    BlockSizes blocksizes;
>> +
>> +    if (conf->physical_block_size && conf->logical_block_size) {
>> +        return;
>> +    }
>
> This conditional isn't necessary for correctness.  Feel free to drop it.
>

But this allows to avoid the ioctl call when user has specified both
values. Remove anyway?

>> +    blk_probe_blocksizes(blk, &blocksizes);
>> +
>> +    if (!conf->physical_block_size) {
>> +        conf->physical_block_size = blocksizes.phys;
>> +    }
>> +    if (!conf->logical_block_size) {
>> +        conf->logical_block_size = blocksizes.log;
>> +    }
>

I'll add a comment to make it apparent.

> This works because you change the default block size to 0 (second to
> last hunk).
>
>> +}
>> +
>>   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..fbd602d 100644
>> --- a/hw/block/hd-geometry.c
>> +++ b/hw/block/hd-geometry.c
>> @@ -121,7 +121,17 @@ void hd_geometry_guess(BlockBackend *blk,
>>                          int *ptrans)
>>   {
>>       int cylinders, heads, secs, translation;
>> +    hdGeometry geo;
>>
>> +    /* Try to probe the backing device geometry, otherwise fallback
>> +       to the old logic. (as of 12/2014 probing only succeeds on DASDs) */
>> +    if (blk_probe_geometry(blk, &geo) == 0) {
>> +        *pcyls = geo.cylinders;
>> +        *psecs = geo.sectors;
>> +        *pheads = geo.heads;
>> +        translation = BIOS_ATA_TRANSLATION_NONE;
>> +        goto done;
>> +    }
>
> "else if" instead of goto, please.
>
>>       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);
>> @@ -142,7 +152,9 @@ void hd_geometry_guess(BlockBackend *blk,
>>           /* disable any translation to be in sync with
>>              the logical geometry */
>>           translation = BIOS_ATA_TRANSLATION_NONE;
>> +
>>       }
>
> Humor me: put the empty line behind the brace instead of before.
>
>> +done:
>>       if (ptrans) {
>>           *ptrans = translation;
>>       }
>
> Much easier to gauge than v2.  Geometry change is a compatibility
> problem.  You change it only when blk_probe_geometry() succeeds.  It
> succeeds only for DASD.  Mission accomplished.
>
> Recommend to add a hint to the function contract of the
> bdrv_probe_geometry() callback in block_int.h:
>
>      /**
>       * Try to get @bs's geometry (cyls, heads, sectos)
>       * On success, store them in @geo and return 0.
>       * On failure return -errno.
>       * Only drivers that want to override guest geometry implement this
>       * callback; see hd_geometry_guess().
>       */
>      int (*bdrv_probe_geometry)(BlockDriverState *bs, hdGeometry *geo);
>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 1327658..244e382 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -763,6 +763,7 @@ static int nvme_init(PCIDevice *pci_dev)
>>       if (!n->serial) {
>>           return -1;
>>       }
>> +    blkconf_blocksizes(&n->conf);
>>
>>       pci_conf = pci_dev->config;
>>       pci_conf[PCI_INTERRUPT_PIN] = 1;
>> 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/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index 2e47f70..ba81709 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -580,7 +580,8 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque,
>>           error_propagate(errp, local_err);
>>           return;
>>       }
>> -    if (value < min || value > max) {
>> +    /* value of 0 means "unset" */
>> +    if (value && (value < min || value > max)) {
>>           error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
>>                     dev->id?:"", name, (int64_t)value, min, max);
>>           return;
>> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>> index b4f096e..e71ff7f 100644
>> --- a/hw/ide/qdev.c
>> +++ b/hw/ide/qdev.c
>> @@ -164,6 +164,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>       }
>>
>>       blkconf_serial(&dev->conf, &dev->serial);
>> +    blkconf_blocksizes(&dev->conf);
>>       if (kind != IDE_CD) {
>>           blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
>>           if (err) {
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 2f75d7d..5288129 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -2230,6 +2230,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
>>       }
>>
>>       blkconf_serial(&s->qdev.conf, &s->serial);
>> +    blkconf_blocksizes(&s->qdev.conf);
>>       if (dev->type == TYPE_DISK) {
>>           blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
>>           if (err) {
>> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
>> index 4539733..cc02dfd 100644
>> --- a/hw/usb/dev-storage.c
>> +++ b/hw/usb/dev-storage.c
>> @@ -611,6 +611,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
>>       }
>>
>>       blkconf_serial(&s->conf, &dev->serial);
>> +    blkconf_blocksizes(&s->conf);
>>
>>       /*
>>        * Hack alert: this pretends to be a block device, but it's really
>> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
>> index 0d0ce9a..3e502a8 100644
>> --- a/include/hw/block/block.h
>> +++ b/include/hw/block/block.h
>> @@ -44,9 +44,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>>   #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
>>       DEFINE_PROP_DRIVE("drive", _state, _conf.blk),                      \
>>       DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
>> -                          _conf.logical_block_size, 512),               \
>> +                          _conf.logical_block_size, 0),               \
>>       DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
>> -                          _conf.physical_block_size, 512),              \
>> +                          _conf.physical_block_size, 0),              \
>>       DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
>>       DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
>>       DEFINE_PROP_UINT32("discard_granularity", _state, \
>> @@ -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 v3 5/5] BlockConf: Call backend functions to detect geometry and blocksizes

[Markus Armbruster]

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

> On 12/15/2014 04:50 PM, Markus Armbruster wrote:
>> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:
>>
>>> geometry: 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 for DASDs), the blkconf_geometry will pass-through
>>> the backing device geometry. Otherwise will fallback to old logic.
>>>
>>> blocksize: This patch initializes blocksize properties to 0.
>>> In order to set the properly a blkconf_blocksizes was introduced.
>>> If user didn't set physical or logical blocksize, it will
>>> retrieve it's value from a driver (which will return a default 512 for
>>> any backend but DASD).
>>>
>>> The blkconf_blocksizes call was added to all users of BlkConf.
>>>
>>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>> ---
>>>   hw/block/block.c          | 18 ++++++++++++++++++
>>>   hw/block/hd-geometry.c    | 12 ++++++++++++
>>>   hw/block/nvme.c           |  1 +
>>>   hw/block/virtio-blk.c     |  1 +
>>>   hw/core/qdev-properties.c |  3 ++-
>>>   hw/ide/qdev.c             |  1 +
>>>   hw/scsi/scsi-disk.c       |  1 +
>>>   hw/usb/dev-storage.c      |  1 +
>>>   include/hw/block/block.h  |  5 +++--
>>>   9 files changed, 40 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/block/block.c b/hw/block/block.c
>>> index a625773..9c07516 100644
>>> --- a/hw/block/block.c
>>> +++ b/hw/block/block.c
>>> @@ -25,6 +25,24 @@ void blkconf_serial(BlockConf *conf, char **serial)
>>>       }
>>>   }
>>>
>>> +void blkconf_blocksizes(BlockConf *conf)
>>> +{
>>> +    BlockBackend *blk = conf->blk;
>>> +    BlockSizes blocksizes;
>>> +
>>> +    if (conf->physical_block_size && conf->logical_block_size) {
>>> +        return;
>>> +    }
>>
>> This conditional isn't necessary for correctness.  Feel free to drop it.
>>
>
> But this allows to avoid the ioctl call when user has specified both
> values. Remove anyway?

It's device model setup, i.e. not a fast path.  I'd favor simplicity and
compactness over speed there.

>>> +    blk_probe_blocksizes(blk, &blocksizes);
>>> +
>>> +    if (!conf->physical_block_size) {
>>> +        conf->physical_block_size = blocksizes.phys;
>>> +    }
>>> +    if (!conf->logical_block_size) {
>>> +        conf->logical_block_size = blocksizes.log;
>>> +    }
>>
>
> I'll add a comment to make it apparent.
>
>> This works because you change the default block size to 0 (second to
>> last hunk).
>>
>>> +}
>>> +
>>>   void blkconf_geometry(BlockConf *conf, int *ptrans,
>>>                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
>>>                         Error **errp)
[...]