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

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

[Ekaterina Tumanova]

Updates v5 -> v6:

Minor Updates according the last review from Stefan Hajnoczi:
1. Do not change the flow of code, factored out of raw_probe_alignment.
2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer.
3. adjusted the comment hdev_probe_geometry according suggestment.
4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
5. do not discard error blk_probe_blocksizes(). now has rc.
6. put the 512-byte default blocksize value in blkconf_blocksizes.
7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.

Thanks,
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                        |  36 ++++++++++
 block/block-backend.c          |  10 +++
 block/raw-posix.c              | 154 ++++++++++++++++++++++++++++++++++++-----
 block/raw_bsd.c                |  12 ++++
 hw/block/block.c               |  20 ++++++
 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/hw/qdev-properties.h   |   4 +-
 include/sysemu/block-backend.h |   2 +
 17 files changed, 267 insertions(+), 22 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH v6 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 block.c                   | 36 ++++++++++++++++++++++++++++++++++++
 include/block/block.h     | 13 +++++++++++++
 include/block/block_int.h | 15 +++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/block.c b/block.c
index cbe4a32..7e60892 100644
--- a/block.c
+++ b/block.c
@@ -569,6 +569,42 @@ 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 and return 0.
+ * On failure return -errno.
+ * @bs must not be empty.
+ */
+int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
+{
+    BlockDriver *drv = bs->drv;
+
+    assert(drv != NULL);
+    if (drv->bdrv_probe_blocksizes) {
+        return drv->bdrv_probe_blocksizes(bs, bsz);
+    }
+
+    return -ENOTSUP;
+}
+
+/**
+ * Try to get @bs's geometry (cyls, heads, sectors).
+ * 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 3082d2b..f445a10 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 */
@@ -546,6 +557,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
  * the old #AioContext is not executing.
  */
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
+int 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..b216156 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, 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;
 };
 
-- 
2.1.4

[Qemu-devel] [PATCH v6 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 block/raw-posix.c | 51 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index e51293a..48525f8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -217,39 +217,58 @@ static int raw_normalize_devicepath(const char **filename)
 }
 #endif
 
-static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
+/*
+ * Get logical block size via ioctl. On success store it in @sector_size_p.
+ */
+static int probe_logical_blocksize(int fd, unsigned int *sector_size_p)
 {
-    BDRVRawState *s = bs->opaque;
-    char *buf;
     unsigned int sector_size;
+    bool success = false;
 
-    /* For /dev/sg devices the alignment is not really used.
-       With buffered I/O, we don't have any restrictions. */
-    if (bs->sg || !s->needs_alignment) {
-        bs->request_alignment = 1;
-        s->buf_align = 1;
-        return;
-    }
+    errno = ENOTSUP;
 
     /* 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;
+        *sector_size_p = sector_size;
+        success = true;
     }
 #endif
 #ifdef DKIOCGETBLOCKSIZE
     if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
-        bs->request_alignment = sector_size;
+        *sector_size_p = sector_size;
+        success = true;
     }
 #endif
 #ifdef DIOCGSECTORSIZE
     if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
-        bs->request_alignment = sector_size;
+        *sector_size_p = sector_size;
+        success = true;
     }
 #endif
+
+    return success ? 0 : -errno;
+}
+
+static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
+{
+    BDRVRawState *s = bs->opaque;
+    char *buf;
+
+    /* For /dev/sg devices the alignment is not really used.
+       With buffered I/O, we don't have any restrictions. */
+    if (bs->sg || !s->needs_alignment) {
+        bs->request_alignment = 1;
+        s->buf_align = 1;
+        return;
+    }
+
+    bs->request_alignment = 0;
+    s->buf_align = 0;
+    /* Let's try to use the logical blocksize for the alignment. */
+    if (probe_logical_blocksize(fd, &bs->request_alignment) < 0) {
+        bs->request_alignment = 0;
+    }
 #ifdef CONFIG_XFS
     if (s->is_xfs) {
         struct dioattr da;
-- 
2.1.4

[Qemu-devel] [PATCH v6 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 block/raw-posix.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/raw_bsd.c   |  12 +++++++
 2 files changed, 115 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 48525f8..86ba479 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -56,6 +56,10 @@
 #include <linux/cdrom.h>
 #include <linux/fd.h>
 #include <linux/fs.h>
+#include <linux/hdreg.h>
+#ifdef __s390__
+#include <asm/dasd.h>
+#endif
 #ifndef FS_NOCOW_FL
 #define FS_NOCOW_FL                     0x00800000 /* Do not cow file */
 #endif
@@ -250,6 +254,23 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size_p)
     return success ? 0 : -errno;
 }
 
+/**
+ * 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;
@@ -672,6 +693,86 @@ 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 -1;
+#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, sectors.
+ * On success, store them in @geo and return 0.
+ * On failure return -errno.
+ * (Allows block driver to assign default geometry values that guest sees)
+ */
+#ifdef __linux__
+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 its 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 = bdrv_nb_sectors(bs) / (blksize / BDRV_SECTOR_SIZE)
+                                             / (geo->heads * geo->sectors);
+        return 0;
+    }
+    geo->cylinders = ioctl_geo.cylinders;
+
+    return 0;
+}
+#else /* __linux__ */
+static int hdev_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
+{
+    return -ENOTSUP;
+}
+#endif
+
 static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
 {
     int ret;
@@ -2137,6 +2238,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..e3d2d04 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -235,6 +235,16 @@ 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)
+{
+    return bdrv_probe_blocksizes(bs->file, bsz);
+}
+
+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 +262,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,
-- 
2.1.4

[Qemu-devel] [PATCH v6 4/5] block-backend: Add wrappers for blocksizes and geometry probing

[Ekaterina Tumanova]

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 d00c129..0f7baea 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -663,3 +663,13 @@ void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
 {
     return qemu_aio_get(aiocb_info, blk_bs(blk), cb, opaque);
 }
+
+int blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz)
+{
+    return 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..2c97726 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);
+int blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz);
+int blk_probe_geometry(BlockBackend *blk, HDGeometry *geo);
 
 #endif
-- 
2.1.4

[Qemu-devel] [PATCH v6 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 its 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/block/block.c             | 20 ++++++++++++++++++++
 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 +++--
 include/hw/qdev-properties.h |  4 ++--
 10 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index a625773..09dd5f1 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -25,6 +25,26 @@ void blkconf_serial(BlockConf *conf, char **serial)
     }
 }
 
+void blkconf_blocksizes(BlockConf *conf)
+{
+    BlockBackend *blk = conf->blk;
+    BlockSizes blocksizes;
+    int backend_ret;
+
+    backend_ret = blk_probe_blocksizes(blk, &blocksizes);
+    /* fill in detected values if they are not defined via qemu command line */
+    if (!conf->physical_block_size && !backend_ret) {
+        conf->physical_block_size = blocksizes.phys;
+    } else {
+        conf->physical_block_size = BDRV_SECTOR_SIZE;
+    }
+    if (!conf->logical_block_size && !backend_ret) {
+        conf->logical_block_size = blocksizes.log;
+    } else {
+        conf->logical_block_size = BDRV_SECTOR_SIZE;
+    }
+}
+
 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 ce079ae..0f3dfb9 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -765,6 +765,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 f65618d..e7244e6 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2251,6 +2251,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..8d7c4b4 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),                    \
     DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
-                          _conf.physical_block_size, 512),              \
+                          _conf.physical_block_size),                   \
     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 */
 
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 070006c..2fdcbce 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -149,8 +149,8 @@ extern PropertyInfo qdev_prop_arraylen;
                         LostTickPolicy)
 #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
-#define DEFINE_PROP_BLOCKSIZE(_n, _s, _f, _d) \
-    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blocksize, uint16_t)
+#define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
 #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
     DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
 
-- 
2.1.4

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

[Christian Borntraeger]

Am 19.01.2015 um 15:35 schrieb 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 its 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>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/block/block.c             | 20 ++++++++++++++++++++
>  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 +++--
>  include/hw/qdev-properties.h |  4 ++--
>  10 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/block.c b/hw/block/block.c
> index a625773..09dd5f1 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -25,6 +25,26 @@ void blkconf_serial(BlockConf *conf, char **serial)
>      }
>  }
> 
> +void blkconf_blocksizes(BlockConf *conf)
> +{
> +    BlockBackend *blk = conf->blk;
> +    BlockSizes blocksizes;
> +    int backend_ret;
> +
> +    backend_ret = blk_probe_blocksizes(blk, &blocksizes);
> +    /* fill in detected values if they are not defined via qemu command line */
> +    if (!conf->physical_block_size && !backend_ret) {
> +        conf->physical_block_size = blocksizes.phys;
> +    } else {
> +        conf->physical_block_size = BDRV_SECTOR_SIZE;
> +    }
> +    if (!conf->logical_block_size && !backend_ret) {
> +        conf->logical_block_size = blocksizes.log;
> +    } else {
> +        conf->logical_block_size = BDRV_SECTOR_SIZE;
> +    }

When we are going to fix this, I found another bug:

This code will fail when logical_block_size and physical_block_size are given at the command line AND detection (backend_ret != 0) did not work. It will use BDRV_SECTOR_SIZE instead of the command line value.
With something like 

--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -33,15 +33,19 @@ void blkconf_blocksizes(BlockConf *conf)
 
     backend_ret = blk_probe_blocksizes(blk, &blocksizes);
     /* fill in detected values if they are not defined via qemu command line */
-    if (!conf->physical_block_size && !backend_ret) {
-        conf->physical_block_size = blocksizes.phys;
-    } else {
-        conf->physical_block_size = BDRV_SECTOR_SIZE;
+    if (!conf->physical_block_size) {
+        if (!backend_ret) {
+            conf->physical_block_size = blocksizes.phys;
+        } else {
+            conf->physical_block_size = BDRV_SECTOR_SIZE;
+        }
     }
-    if (!conf->logical_block_size && !backend_ret) {
-        conf->logical_block_size = blocksizes.log;
-    } else {
-        conf->logical_block_size = BDRV_SECTOR_SIZE;
+    if (!conf->logical_block_size) {
+	if (!backend_ret) {
+	        conf->logical_block_size = blocksizes.log;
+	} else {
+		conf->logical_block_size = BDRV_SECTOR_SIZE;
+	}
     }
 }
 

No?

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

[Ekaterina Tumanova]

On 02/13/2015 03:23 PM, Christian Borntraeger wrote:
> Am 19.01.2015 um 15:35 schrieb 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 its 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>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   hw/block/block.c             | 20 ++++++++++++++++++++
>>   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 +++--
>>   include/hw/qdev-properties.h |  4 ++--
>>   10 files changed, 41 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/block/block.c b/hw/block/block.c
>> index a625773..09dd5f1 100644
>> --- a/hw/block/block.c
>> +++ b/hw/block/block.c
>> @@ -25,6 +25,26 @@ void blkconf_serial(BlockConf *conf, char **serial)
>>       }
>>   }
>>
>> +void blkconf_blocksizes(BlockConf *conf)
>> +{
>> +    BlockBackend *blk = conf->blk;
>> +    BlockSizes blocksizes;
>> +    int backend_ret;
>> +
>> +    backend_ret = blk_probe_blocksizes(blk, &blocksizes);
>> +    /* fill in detected values if they are not defined via qemu command line */
>> +    if (!conf->physical_block_size && !backend_ret) {
>> +        conf->physical_block_size = blocksizes.phys;
>> +    } else {
>> +        conf->physical_block_size = BDRV_SECTOR_SIZE;
>> +    }
>> +    if (!conf->logical_block_size && !backend_ret) {
>> +        conf->logical_block_size = blocksizes.log;
>> +    } else {
>> +        conf->logical_block_size = BDRV_SECTOR_SIZE;
>> +    }
>
> When we are going to fix this, I found another bug:
>
> This code will fail when logical_block_size and physical_block_size are given at the command line AND detection (backend_ret != 0) did not work. It will use BDRV_SECTOR_SIZE instead of the command line value.
> With something like
>
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -33,15 +33,19 @@ void blkconf_blocksizes(BlockConf *conf)
>
>       backend_ret = blk_probe_blocksizes(blk, &blocksizes);
>       /* fill in detected values if they are not defined via qemu command line */
> -    if (!conf->physical_block_size && !backend_ret) {
> -        conf->physical_block_size = blocksizes.phys;
> -    } else {
> -        conf->physical_block_size = BDRV_SECTOR_SIZE;
> +    if (!conf->physical_block_size) {
> +        if (!backend_ret) {
> +            conf->physical_block_size = blocksizes.phys;
> +        } else {
> +            conf->physical_block_size = BDRV_SECTOR_SIZE;
> +        }
>       }
> -    if (!conf->logical_block_size && !backend_ret) {
> -        conf->logical_block_size = blocksizes.log;
> -    } else {
> -        conf->logical_block_size = BDRV_SECTOR_SIZE;
> +    if (!conf->logical_block_size) {
> +	if (!backend_ret) {
> +	        conf->logical_block_size = blocksizes.log;
> +	} else {
> +		conf->logical_block_size = BDRV_SECTOR_SIZE;
> +	}
>       }
>   }
>
>
> No?
>

yes.
will be fix in v7.

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

[Christian Borntraeger]

Am 19.01.2015 um 15:34 schrieb Ekaterina Tumanova:
> Updates v5 -> v6:
> 
> Minor Updates according the last review from Stefan Hajnoczi:
> 1. Do not change the flow of code, factored out of raw_probe_alignment.
> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer.
> 3. adjusted the comment hdev_probe_geometry according suggestment.
> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
> 5. do not discard error blk_probe_blocksizes(). now has rc.
> 6. put the 512-byte default blocksize value in blkconf_blocksizes.
> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.
> 
> Thanks,
> 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                        |  36 ++++++++++
>  block/block-backend.c          |  10 +++
>  block/raw-posix.c              | 154 ++++++++++++++++++++++++++++++++++++-----
>  block/raw_bsd.c                |  12 ++++
>  hw/block/block.c               |  20 ++++++
>  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/hw/qdev-properties.h   |   4 +-
>  include/sysemu/block-backend.h |   2 +
>  17 files changed, 267 insertions(+), 22 deletions(-)
> 

Just gave version 6 a try. My dasd is recognizes as it should be.
So
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

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

[Ekaterina Tumanova]

On 01/19/2015 05:34 PM, Ekaterina Tumanova wrote:
> Updates v5 -> v6:
>
> Minor Updates according the last review from Stefan Hajnoczi:
> 1. Do not change the flow of code, factored out of raw_probe_alignment.
> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer.
> 3. adjusted the comment hdev_probe_geometry according suggestment.
> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
> 5. do not discard error blk_probe_blocksizes(). now has rc.
> 6. put the 512-byte default blocksize value in blkconf_blocksizes.
> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.
>
> Thanks,
> 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                        |  36 ++++++++++
>   block/block-backend.c          |  10 +++
>   block/raw-posix.c              | 154 ++++++++++++++++++++++++++++++++++++-----
>   block/raw_bsd.c                |  12 ++++
>   hw/block/block.c               |  20 ++++++
>   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/hw/qdev-properties.h   |   4 +-
>   include/sysemu/block-backend.h |   2 +
>   17 files changed, 267 insertions(+), 22 deletions(-)
>

ping

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

[Stefan Hajnoczi]

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

On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote:
> Updates v5 -> v6:
> 
> Minor Updates according the last review from Stefan Hajnoczi:
> 1. Do not change the flow of code, factored out of raw_probe_alignment.
> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer.
> 3. adjusted the comment hdev_probe_geometry according suggestment.
> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
> 5. do not discard error blk_probe_blocksizes(). now has rc.
> 6. put the 512-byte default blocksize value in blkconf_blocksizes.
> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.
> 
> Thanks,
> 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                        |  36 ++++++++++
>  block/block-backend.c          |  10 +++
>  block/raw-posix.c              | 154 ++++++++++++++++++++++++++++++++++++-----
>  block/raw_bsd.c                |  12 ++++
>  hw/block/block.c               |  20 ++++++
>  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/hw/qdev-properties.h   |   4 +-
>  include/sysemu/block-backend.h |   2 +
>  17 files changed, 267 insertions(+), 22 deletions(-)
> 
> -- 
> 2.1.4
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[Stefan Hajnoczi]

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

On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote:
> Updates v5 -> v6:
> 
> Minor Updates according the last review from Stefan Hajnoczi:
> 1. Do not change the flow of code, factored out of raw_probe_alignment.
> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer.
> 3. adjusted the comment hdev_probe_geometry according suggestment.
> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
> 5. do not discard error blk_probe_blocksizes(). now has rc.
> 6. put the 512-byte default blocksize value in blkconf_blocksizes.
> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.

Unfortunately this series breaks "make check" so it cannot be merged:

GTESTER check-qtest-x86_64
qemu-system-x86_64: logical_block_size must be 512 for IDE
qemu-system-x86_64: Device initialization failed.
qemu-system-x86_64: Initialization of device ide-cd failed
Broken pipe
GTester: last random seed: R02S942fac7e56eff09e8ab7a7f7fecf847e

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

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

[Christian Borntraeger]

Am 12.02.2015 um 16:46 schrieb Stefan Hajnoczi:
> On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote:
>> Updates v5 -> v6:
>>
>> Minor Updates according the last review from Stefan Hajnoczi:
>> 1. Do not change the flow of code, factored out of raw_probe_alignment.
>> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer.
>> 3. adjusted the comment hdev_probe_geometry according suggestment.
>> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
>> 5. do not discard error blk_probe_blocksizes(). now has rc.
>> 6. put the 512-byte default blocksize value in blkconf_blocksizes.
>> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.
> 
> Unfortunately this series breaks "make check" so it cannot be merged:
> 
> GTESTER check-qtest-x86_64
> qemu-system-x86_64: logical_block_size must be 512 for IDE
> qemu-system-x86_64: Device initialization failed.
> qemu-system-x86_64: Initialization of device ide-cd failed
> Broken pipe
> GTester: last random seed: R02S942fac7e56eff09e8ab7a7f7fecf847e
> 

This particular message came in with 

commit d20051856cd2fa8f10fed2d2a0b2751de5f7b20d
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Wed Dec 3 13:21:32 2014 +0100

    ide: Check validity of logical block size

so something like
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 353854c..2680275 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -163,7 +163,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         return -1;
     }
 
-    if (dev->conf.logical_block_size != 512) {
+    if (dev->conf.logical_block_size != 512 &&
+        dev->conf.logical_block_size != 0) {
         error_report("logical_block_size must be 512 for IDE");
         return -1;
     }


will fix this.

Now we have 
block.c:582: bdrv_probe_blocksizes: Assertion `drv != ((void *)0)' failed

so
we need something like this on top - I guess.

diff --git a/block.c b/block.c
index dbc2519..8bbcc6d 100644
--- a/block.c
+++ b/block.c
@@ -579,8 +579,7 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 {
     BlockDriver *drv = bs->drv;
 
-    assert(drv != NULL);
-    if (drv->bdrv_probe_blocksizes) {
+    if (drv && drv->bdrv_probe_blocksizes) {
         return drv->bdrv_probe_blocksizes(bs, bsz);
     }
 
@@ -597,8 +596,7 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
 {
     BlockDriver *drv = bs->drv;
 
-    assert(drv != NULL);
-    if (drv->bdrv_probe_geometry) {
+    if (drv && drv->bdrv_probe_geometry) {
         return drv->bdrv_probe_geometry(bs, geo);
     }
 

Kate, I think its time for a v7 :-(

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

[Christian Borntraeger]

Am 12.02.2015 um 17:42 schrieb Christian Borntraeger:
> Am 12.02.2015 um 16:46 schrieb Stefan Hajnoczi:
>> On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote:
>>> Updates v5 -> v6:
>>>
>>> Minor Updates according the last review from Stefan Hajnoczi:
>>> 1. Do not change the flow of code, factored out of raw_probe_alignment.
>>> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer.
>>> 3. adjusted the comment hdev_probe_geometry according suggestment.
>>> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
>>> 5. do not discard error blk_probe_blocksizes(). now has rc.
>>> 6. put the 512-byte default blocksize value in blkconf_blocksizes.
>>> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.
>>
>> Unfortunately this series breaks "make check" so it cannot be merged:
>>
>> GTESTER check-qtest-x86_64
>> qemu-system-x86_64: logical_block_size must be 512 for IDE
>> qemu-system-x86_64: Device initialization failed.
>> qemu-system-x86_64: Initialization of device ide-cd failed
>> Broken pipe
>> GTester: last random seed: R02S942fac7e56eff09e8ab7a7f7fecf847e
>>
> 
> This particular message came in with 
> 
> commit d20051856cd2fa8f10fed2d2a0b2751de5f7b20d
> Author: Kevin Wolf <kwolf@redhat.com>
> Date:   Wed Dec 3 13:21:32 2014 +0100
> 
>     ide: Check validity of logical block size
> 
> so something like
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 353854c..2680275 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -163,7 +163,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>          return -1;
>      }
>  
> -    if (dev->conf.logical_block_size != 512) {
> +    if (dev->conf.logical_block_size != 512 &&
> +        dev->conf.logical_block_size != 0) {
>          error_report("logical_block_size must be 512 for IDE");
>          return -1;
>      }
> 
> 
> will fix this.
> 
> Now we have 
> block.c:582: bdrv_probe_blocksizes: Assertion `drv != ((void *)0)' failed
> 
> so
> we need something like this on top - I guess.
> 
> diff --git a/block.c b/block.c
> index dbc2519..8bbcc6d 100644
> --- a/block.c
> +++ b/block.c
> @@ -579,8 +579,7 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>  {
>      BlockDriver *drv = bs->drv;
>  
> -    assert(drv != NULL);
> -    if (drv->bdrv_probe_blocksizes) {
> +    if (drv && drv->bdrv_probe_blocksizes) {
>          return drv->bdrv_probe_blocksizes(bs, bsz);
>      }
>  
> @@ -597,8 +596,7 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
>  {
>      BlockDriver *drv = bs->drv;
>  
> -    assert(drv != NULL);
> -    if (drv->bdrv_probe_geometry) {
> +    if (drv && drv->bdrv_probe_geometry) {
>          return drv->bdrv_probe_geometry(bs, geo);
>      }
>  
> 
> Kate, I think its time for a v7 :-(

Of course, please double check my quick hacks. I still see some broken pipe messages, so I must have missed something.

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

[Kevin Wolf]

Am 12.02.2015 um 17:42 hat Christian Borntraeger geschrieben:
> Am 12.02.2015 um 16:46 schrieb Stefan Hajnoczi:
> > On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote:
> >> Updates v5 -> v6:
> >>
> >> Minor Updates according the last review from Stefan Hajnoczi:
> >> 1. Do not change the flow of code, factored out of raw_probe_alignment.
> >> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer.
> >> 3. adjusted the comment hdev_probe_geometry according suggestment.
> >> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
> >> 5. do not discard error blk_probe_blocksizes(). now has rc.
> >> 6. put the 512-byte default blocksize value in blkconf_blocksizes.
> >> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.
> > 
> > Unfortunately this series breaks "make check" so it cannot be merged:
> > 
> > GTESTER check-qtest-x86_64
> > qemu-system-x86_64: logical_block_size must be 512 for IDE
> > qemu-system-x86_64: Device initialization failed.
> > qemu-system-x86_64: Initialization of device ide-cd failed
> > Broken pipe
> > GTester: last random seed: R02S942fac7e56eff09e8ab7a7f7fecf847e
> > 
> 
> This particular message came in with 
> 
> commit d20051856cd2fa8f10fed2d2a0b2751de5f7b20d
> Author: Kevin Wolf <kwolf@redhat.com>
> Date:   Wed Dec 3 13:21:32 2014 +0100
> 
>     ide: Check validity of logical block size
> 
> so something like
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 353854c..2680275 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -163,7 +163,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>          return -1;
>      }
>  
> -    if (dev->conf.logical_block_size != 512) {
> +    if (dev->conf.logical_block_size != 512 &&
> +        dev->conf.logical_block_size != 0) {
>          error_report("logical_block_size must be 512 for IDE");
>          return -1;
>      }
> 
> 
> will fix this.

It would probably be better to set the default first and then make sure
that the final value, no matter whether explicitly specified or default,
is 512.

Kevin

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

[Christian Borntraeger]

Am 13.02.2015 um 08:50 schrieb Kevin Wolf:
> Am 12.02.2015 um 17:42 hat Christian Borntraeger geschrieben:
>> Am 12.02.2015 um 16:46 schrieb Stefan Hajnoczi:
>>> On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote:
>>>> Updates v5 -> v6:
>>>>
>>>> Minor Updates according the last review from Stefan Hajnoczi:
>>>> 1. Do not change the flow of code, factored out of raw_probe_alignment.
>>>> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer.
>>>> 3. adjusted the comment hdev_probe_geometry according suggestment.
>>>> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
>>>> 5. do not discard error blk_probe_blocksizes(). now has rc.
>>>> 6. put the 512-byte default blocksize value in blkconf_blocksizes.
>>>> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.
>>>
>>> Unfortunately this series breaks "make check" so it cannot be merged:
>>>
>>> GTESTER check-qtest-x86_64
>>> qemu-system-x86_64: logical_block_size must be 512 for IDE
>>> qemu-system-x86_64: Device initialization failed.
>>> qemu-system-x86_64: Initialization of device ide-cd failed
>>> Broken pipe
>>> GTester: last random seed: R02S942fac7e56eff09e8ab7a7f7fecf847e
>>>
>>
>> This particular message came in with 
>>
>> commit d20051856cd2fa8f10fed2d2a0b2751de5f7b20d
>> Author: Kevin Wolf <kwolf@redhat.com>
>> Date:   Wed Dec 3 13:21:32 2014 +0100
>>
>>     ide: Check validity of logical block size
>>
>> so something like
>> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>> index 353854c..2680275 100644
>> --- a/hw/ide/qdev.c
>> +++ b/hw/ide/qdev.c
>> @@ -163,7 +163,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>          return -1;
>>      }
>>  
>> -    if (dev->conf.logical_block_size != 512) {
>> +    if (dev->conf.logical_block_size != 512 &&
>> +        dev->conf.logical_block_size != 0) {
>>          error_report("logical_block_size must be 512 for IDE");
>>          return -1;
>>      }
>>
>>
>> will fix this.
> 
> It would probably be better to set the default first and then make sure
> that the final value, no matter whether explicitly specified or default,
> is 512.
> 
> Kevin

+1 
Yes, this should minimize the impact.