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

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

[Ekaterina Tumanova]

Updates:
v2 -> v3:
1. Fix comments
2. Fix error codes to -ENOTSUP.
3. Reduce LOC in probe_logical_blocksize.
4. Adjust #ifdef - #else logic in couple of places.
5. Rebased.

I hope that I addressed all the comments from the last round of review.
If you think it's ok now, can you please give me your explicit Review-by.
(I would really appreciate if you could give me some feedback this week,
because I'm leaving for vacation next week)

Looking forward to your reply,
Thanks a lot!
Kate.
---------------------------------------------------------------
   Patchset Description (didn't change):

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.


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                        |  34 ++++++++++
 block/block-backend.c          |  10 +++
 block/raw-posix.c              | 138 ++++++++++++++++++++++++++++++++++++-----
 block/raw_bsd.c                |  14 +++++
 hw/block/block.c               |  15 +++++
 hw/block/hd-geometry.c         |  10 ++-
 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      |  15 +++++
 include/hw/block/block.h       |   5 +-
 include/sysemu/block-backend.h |   2 +
 16 files changed, 243 insertions(+), 21 deletions(-)

-- 
1.8.5.5

[Qemu-devel] [PATCH v4 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>
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
---
 block.c                   | 34 ++++++++++++++++++++++++++++++++++
 include/block/block.h     | 13 +++++++++++++
 include/block/block_int.h | 15 +++++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/block.c b/block.c
index 4165d42..93409f5 100644
--- a/block.c
+++ b/block.c
@@ -548,6 +548,40 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 }
 
+/**
+ * Get @bs's logical and physical block size, store them in @bsz.
+ * @bs must not be empty.
+ */
+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. @bs must not be empty.
+ */
+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 -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 6e7275d..14ac3b1 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 */
@@ -539,6 +550,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 06a21dd..c2c5f0e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -273,6 +273,21 @@ struct BlockDriver {
     void (*bdrv_io_unplug)(BlockDriverState *bs);
     void (*bdrv_flush_io_queue)(BlockDriverState *bs);
 
+    /**
+     * Try to get @bs's logical and physical block size.
+     * On success, store them in @bsz and return zero.
+     * On failure, return negative errno.
+     */
+    int (*bdrv_probe_blocksizes)(BlockDriverState *bs, BlockSizes *bsz);
+    /**
+     * 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);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.8.5.5

[Qemu-devel] [PATCH v4 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 | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index e51293a..38172ca 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -217,11 +217,31 @@ static int raw_normalize_devicepath(const char **filename)
 }
 #endif
 
+/*
+ * Get logical block size via ioctl. On success return 0. Otherwise -errno.
+ */
+static int probe_logical_blocksize(int fd, unsigned int *sector_size)
+{
+#if defined(BLKSSZGET)
+#  define SECTOR_SIZE BLKSSZGET
+#elif defined(DKIOCGETBLOCKSIZE)
+#  define SECTOR_SIZE DKIOCGETBLOCKSIZE
+#elif defined(DIOCGSECTORSIZE)
+#  define SECTOR_SIZE DIOCGSECTORSIZE
+#else
+    return -ENOTSUP
+#endif
+    if (ioctl(fd, SECTOR_SIZE, sector_size) < 0) {
+        return -errno;
+    }
+    return 0;
+#undef SECTOR_SIZE
+}
+
 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 +251,12 @@ 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;
-
-#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;
+    /* Let's try to use the logical blocksize for the alignment. */
+    if (probe_logical_blocksize(fd, &bs->request_alignment) < 0) {
+        bs->request_alignment = 0;
     }
-#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 v4 3/5] block: Add driver methods to probe blocksizes and geometry

[Ekaterina Tumanova]

Introduce driver methods of defining disk blocksizes (physical and
logical) and hard drive geometry.
Methods are only implemented for "host_device". For "raw" devices
driver calls child's method.

For now geometry detection will only work for DASD devices. To check
that a local check_for_dasd function was introduced. It calls BIODASDINFO2
ioctl and returns its rc.

Blocksizes detection function 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 | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/raw_bsd.c   | 14 ++++++++
 2 files changed, 111 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 38172ca..e1e7b29 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
@@ -238,6 +243,23 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size)
 #undef SECTOR_SIZE
 }
 
+/**
+ * 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;
+    }
+    return 0;
+#else
+    return -ENOTSUP;
+#endif
+}
+
 static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
@@ -660,6 +682,79 @@ 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);
+#else
+    return -ENOTSUP;
+#endif
+}
+
+/**
+ * Try to get @bs's logical and physical block size.
+ * On success, store them in @bsz and return zero.
+ * On failure, return negative errno.
+ */
+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 -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 @bs's geometry (cyls, heads, sectos)
+ * On success, store them in @geo and return 0.
+ * On failure return -errno.
+ * (as for 12/2014 only DASDs are supported)
+ */
+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 -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 -ENOTSUP;
+    }
+    /* Do not return a geometry for partition */
+    if (ioctl_geo.start != 0) {
+        return -ENOTSUP;
+    }
+    geo->heads = ioctl_geo.heads;
+    geo->sectors = ioctl_geo.sectors;
+    if (!probe_physical_blocksize(s->fd, &blksize)) {
+        /* overwrite cyls: HDIO_GETGEO result is incorrect for big drives */
+        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;
@@ -2125,6 +2220,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 05b02c7..f3d532b 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -235,6 +235,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);
+}
+
 BlockDriver bdrv_raw = {
     .format_name          = "raw",
     .bdrv_probe           = &raw_probe,
@@ -252,6 +264,8 @@ 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 v4 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 ef16d73..4b9ed85 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -667,3 +667,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 8871a02..aecdc53 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -150,5 +150,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 v4 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          | 15 +++++++++++++++
 hw/block/hd-geometry.c    | 10 +++++++++-
 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, 34 insertions(+), 4 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index a625773..a4f4f06 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -25,6 +25,21 @@ void blkconf_serial(BlockConf *conf, char **serial)
     }
 }
 
+void blkconf_blocksizes(BlockConf *conf)
+{
+    BlockBackend *blk = conf->blk;
+    BlockSizes blocksizes;
+
+    blk_probe_blocksizes(blk, &blocksizes);
+    /* fill in detected values if they are not defined via qemu command line */
+    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..b187878 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -121,8 +121,16 @@ void hd_geometry_guess(BlockBackend *blk,
                        int *ptrans)
 {
     int cylinders, heads, secs, translation;
+    HDGeometry geo;
 
-    if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
+    /* 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;
+    } else 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);
         translation = hd_bios_chs_auto_trans(*pcyls, *pheads, *psecs);
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index aa1ed98..2c630bc 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 1ebb58d..353854c 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -169,6 +169,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 v4 0/5] Geometry and blocksize detection for backing devices.

[Christian Borntraeger]

Am 16.12.2014 um 12:10 schrieb Ekaterina Tumanova:
> Updates:
> v2 -> v3:
> 1. Fix comments
> 2. Fix error codes to -ENOTSUP.
> 3. Reduce LOC in probe_logical_blocksize.
> 4. Adjust #ifdef - #else logic in couple of places.
> 5. Rebased.
> 
> I hope that I addressed all the comments from the last round of review.
> If you think it's ok now, can you please give me your explicit Review-by.
> (I would really appreciate if you could give me some feedback this week,
> because I'm leaving for vacation next week)
> 
> Looking forward to your reply,
> Thanks a lot!
> Kate.
> ---------------------------------------------------------------
>    Patchset Description (didn't change):
> 
> 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.
> 
> 
> 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                        |  34 ++++++++++
>  block/block-backend.c          |  10 +++
>  block/raw-posix.c              | 138 ++++++++++++++++++++++++++++++++++++-----
>  block/raw_bsd.c                |  14 +++++
>  hw/block/block.c               |  15 +++++
>  hw/block/hd-geometry.c         |  10 ++-
>  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      |  15 +++++
>  include/hw/block/block.h       |   5 +-
>  include/sysemu/block-backend.h |   2 +
>  16 files changed, 243 insertions(+), 21 deletions(-)
> 

I can confirm that this patch still works for my DASDs.

Christian

Re: [Qemu-devel] [PATCH v4 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>
> Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> ---
>  block.c                   | 34 ++++++++++++++++++++++++++++++++++
>  include/block/block.h     | 13 +++++++++++++
>  include/block/block_int.h | 15 +++++++++++++++
>  3 files changed, 62 insertions(+)
>
> diff --git a/block.c b/block.c
> index 4165d42..93409f5 100644
> --- a/block.c
> +++ b/block.c
> @@ -548,6 +548,40 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>      }
>  }
>  
> +/**
> + * Get @bs's logical and physical block size, store them in @bsz.
> + * @bs must not be empty.
> + */
> +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)

s/sectos/sectors/

Since we're changing this comment anyway: I like my function contracts
to state restrictions prominently, and therefore prefer "@bs must not be
empty" on its own line, right here.

> + * On success, store them in @geo struct and return 0.
> + * On failure return -errno. @bs must not be empty.
> + */
> +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 -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 6e7275d..14ac3b1 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 */
> @@ -539,6 +550,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 06a21dd..c2c5f0e 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -273,6 +273,21 @@ struct BlockDriver {
>      void (*bdrv_io_unplug)(BlockDriverState *bs);
>      void (*bdrv_flush_io_queue)(BlockDriverState *bs);
>  
> +    /**
> +     * Try to get @bs's logical and physical block size.
> +     * On success, store them in @bsz and return zero.
> +     * On failure, return negative errno.
> +     */
> +    int (*bdrv_probe_blocksizes)(BlockDriverState *bs, BlockSizes *bsz);
> +    /**
> +     * Try to get @bs's geometry (cyls, heads, sectos)

s/sectos/sectors/

> +     * 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);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };

Re: [Qemu-devel] [PATCH v4 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 | 41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e51293a..38172ca 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -217,11 +217,31 @@ static int raw_normalize_devicepath(const char **filename)
>  }
>  #endif
>  
> +/*
> + * Get logical block size via ioctl. On success return 0. Otherwise -errno.
> + */
> +static int probe_logical_blocksize(int fd, unsigned int *sector_size)
> +{
> +#if defined(BLKSSZGET)
> +#  define SECTOR_SIZE BLKSSZGET
> +#elif defined(DKIOCGETBLOCKSIZE)
> +#  define SECTOR_SIZE DKIOCGETBLOCKSIZE
> +#elif defined(DIOCGSECTORSIZE)
> +#  define SECTOR_SIZE DIOCGSECTORSIZE
> +#else
> +    return -ENOTSUP
> +#endif
> +    if (ioctl(fd, SECTOR_SIZE, sector_size) < 0) {
> +        return -errno;
> +    }
> +    return 0;
> +#undef SECTOR_SIZE
> +}
> +
>  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 +251,12 @@ 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;
> -
> -#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;
> +    /* Let's try to use the logical blocksize for the alignment. */
> +    if (probe_logical_blocksize(fd, &bs->request_alignment) < 0) {
> +        bs->request_alignment = 0;
>      }
> -#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;

New in v4: this isn't just factoring out code, you also change it!

Before: try BLKSSZGET (if defined), DKIOCGETBLOCKSIZE (if defined),
DIOCGSECTORSIZE (if defined) in order until.  The last one to succeed
wins.  If none succeeds, assume 0.

After: use BLKSSZGET if defined, else try DKIOCGETBLOCKSIZE if defined,
else try DIOCGSECTORSIZE if defined, else assume 0.

Your change may well be an improvement.  But your commit message should
explain *why* it's an improvement.

I guess I'd do a separate patch for that, but since the code patch seems
obvious enough as it is, you may just improve its commit message and
call it a day.

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

[Markus Armbruster]

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

> Introduce driver methods of defining disk blocksizes (physical and
> logical) and hard drive geometry.
> Methods are only implemented for "host_device". For "raw" devices
> driver calls child's method.
>
> For now geometry detection will only work for DASD devices. To check
> that a local check_for_dasd function was introduced. It calls BIODASDINFO2
> ioctl and returns its rc.
>
> Blocksizes detection function 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 | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/raw_bsd.c   | 14 ++++++++
>  2 files changed, 111 insertions(+)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 38172ca..e1e7b29 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
> @@ -238,6 +243,23 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size)
>  #undef SECTOR_SIZE
>  }
>  
> +/**
> + * 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;
> +    }
> +    return 0;
> +#else
> +    return -ENOTSUP;
> +#endif
> +}
> +
>  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
> @@ -660,6 +682,79 @@ 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);
> +#else
> +    return -ENOTSUP;
> +#endif
> +}

This function is confused about its return value: 0/-1 vs. 0/-errno.
Please return -1 instead of -ENOTSUP, or replace it by an is_dasd()
returning bool.

[...]

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

[Markus Armbruster]

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

> Updates:
> v2 -> v3:
> 1. Fix comments
> 2. Fix error codes to -ENOTSUP.
> 3. Reduce LOC in probe_logical_blocksize.
> 4. Adjust #ifdef - #else logic in couple of places.
> 5. Rebased.
>
> I hope that I addressed all the comments from the last round of review.
> If you think it's ok now, can you please give me your explicit Review-by.
> (I would really appreciate if you could give me some feedback this week,
> because I'm leaving for vacation next week)

Down to a commit message needing improvement, a few comment typos, and
one harmless buglet.  If you fix those, you can add my R-by.

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

[David Hildenbrand]

> Add driver functions for geometry and blocksize detection
> 
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> ---
>  block.c                   | 34 ++++++++++++++++++++++++++++++++++
>  include/block/block.h     | 13 +++++++++++++
>  include/block/block_int.h | 15 +++++++++++++++
>  3 files changed, 62 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 4165d42..93409f5 100644
> --- a/block.c
> +++ b/block.c
> @@ -548,6 +548,40 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>      }
>  }
> 
> +/**
> + * Get @bs's logical and physical block size, store them in @bsz.
> + * @bs must not be empty.
> + */
> +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)) {

So we want to ignore/drop any error?

> +            return;
> +    }


Looks good to me.

David