[Qemu-devel] [PATCH v4 00/14] block: kill sector-based blk_write/read

[Qemu-devel] [PATCH v4 00/14] block: kill sector-based blk_write/read

[Eric Blake]

2.7 material, depends on Kevin's block-next:
git://repo.or.cz/qemu/kevin.git block-next

Previously posted as part of a larger NBD series [1] (at v3, explaining
why this is v4), but these are independent enough to make for easier
review on their own, and is mostly orthogonal to Kevin's recent work
to also kill sector interfaces from the driver.

[1] https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03526.html

Also available as a tag at this location:
git fetch git://repo.or.cz/qemu/ericb.git nbd-block-v4

Changes since then:
add R-b/Acks received so far
rebase to Kevin's block-next branch
patch 8: use new defines for legibility [jsnow]

001/14:[----] [--] 'block: Allow BDRV_REQ_FUA through blk_pwrite()'
002/14:[----] [--] 'fdc: Switch to byte-based block access'
003/14:[----] [--] 'nand: Switch to byte-based block access'
004/14:[----] [--] 'onenand: Switch to byte-based block access'
005/14:[----] [--] 'pflash: Switch to byte-based block access'
006/14:[----] [--] 'sd: Switch to byte-based block access'
007/14:[----] [--] 'm25p80: Switch to byte-based block access'
008/14:[0019] [FC] 'atapi: Switch to byte-based block access'
009/14:[----] [--] 'nbd: Switch to byte-based block access'
010/14:[----] [--] 'qemu-img: Switch to byte-based block access'
011/14:[----] [--] 'qemu-io: Switch to byte-based block access'
012/14:[----] [-C] 'block: Switch blk_read_unthrottled() to byte interface'
013/14:[----] [--] 'block: Switch blk_write_zeroes() to byte interface'
014/14:[----] [--] 'block: Kill blk_write(), blk_read()'

Eric Blake (14):
  block: Allow BDRV_REQ_FUA through blk_pwrite()
  fdc: Switch to byte-based block access
  nand: Switch to byte-based block access
  onenand: Switch to byte-based block access
  pflash: Switch to byte-based block access
  sd: Switch to byte-based block access
  m25p80: Switch to byte-based block access
  atapi: Switch to byte-based block access
  nbd: Switch to byte-based block access
  qemu-img: Switch to byte-based block access
  qemu-io: Switch to byte-based block access
  block: Switch blk_read_unthrottled() to byte interface
  block: Switch blk_write_zeroes() to byte interface
  block: Kill blk_write(), blk_read()

 include/sysemu/block-backend.h | 15 ++++----
 block/block-backend.c          | 47 +++++++-------------------
 block/crypto.c                 |  2 +-
 block/parallels.c              |  5 +--
 block/qcow.c                   |  8 ++---
 block/qcow2.c                  |  4 +--
 block/qed.c                    |  6 ++--
 block/sheepdog.c               |  2 +-
 block/vdi.c                    |  4 +--
 block/vhdx.c                   |  5 +--
 block/vmdk.c                   | 10 +++---
 block/vpc.c                    | 10 +++---
 hw/block/fdc.c                 | 25 +++++++++-----
 hw/block/hd-geometry.c         |  2 +-
 hw/block/m25p80.c              |  3 +-
 hw/block/nand.c                | 36 +++++++++++++-------
 hw/block/onenand.c             | 36 ++++++++++++--------
 hw/block/pflash_cfi01.c        | 12 +++----
 hw/block/pflash_cfi02.c        | 12 +++----
 hw/ide/atapi.c                 | 19 ++++++-----
 hw/nvram/spapr_nvram.c         |  4 +--
 hw/sd/sd.c                     | 46 ++-----------------------
 nbd/server.c                   |  2 +-
 qemu-img.c                     | 31 +++++++++++------
 qemu-io-cmds.c                 | 77 ++++++++++--------------------------------
 qemu-nbd.c                     | 11 +++---
 26 files changed, 185 insertions(+), 249 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v4 01/14] block: Allow BDRV_REQ_FUA through blk_pwrite()

[Eric Blake]

We have several block drivers that understand BDRV_REQ_FUA,
and emulate it in the block layer for the rest by a full flush.
But without a way to actually request BDRV_REQ_FUA during a
pass-through blk_pwrite(), FUA-aware block drivers like NBD are
forced to repeat the emulation logic of a full flush regardless
of whether the backend they are writing to could do it more
efficiently.

This patch just wires up a flags argument; followup patches
will actually make use of it in the NBD driver and in qemu-io.

Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Denis V. Lunev <den@openvz.org>
---
 include/sysemu/block-backend.h |  3 ++-
 block/block-backend.c          |  6 ++++--
 block/crypto.c                 |  2 +-
 block/parallels.c              |  2 +-
 block/qcow.c                   |  8 ++++----
 block/qcow2.c                  |  4 ++--
 block/qed.c                    |  6 +++---
 block/sheepdog.c               |  2 +-
 block/vdi.c                    |  4 ++--
 block/vhdx.c                   |  5 +++--
 block/vmdk.c                   | 10 +++++-----
 block/vpc.c                    | 10 +++++-----
 hw/nvram/spapr_nvram.c         |  4 ++--
 nbd/server.c                   |  2 +-
 qemu-io-cmds.c                 |  2 +-
 15 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c62b6fe..6991b26 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -102,7 +102,8 @@ BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
                                  int nb_sectors, BdrvRequestFlags flags,
                                  BlockCompletionFunc *cb, void *opaque);
 int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int count);
-int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count);
+int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count,
+               BdrvRequestFlags flags);
 int64_t blk_getlength(BlockBackend *blk);
 void blk_get_geometry(BlockBackend *blk, uint64_t *nb_sectors_ptr);
 int64_t blk_nb_sectors(BlockBackend *blk);
diff --git a/block/block-backend.c b/block/block-backend.c
index a7623e8..96c1d7c 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -953,9 +953,11 @@ int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int count)
     return count;
 }

-int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count)
+int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count,
+               BdrvRequestFlags flags)
 {
-    int ret = blk_prw(blk, offset, (void*) buf, count, blk_write_entry, 0);
+    int ret = blk_prw(blk, offset, (void *) buf, count, blk_write_entry,
+                      flags);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/crypto.c b/block/crypto.c
index 1903e84..32ba17c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -91,7 +91,7 @@ static ssize_t block_crypto_write_func(QCryptoBlock *block,
     struct BlockCryptoCreateData *data = opaque;
     ssize_t ret;

-    ret = blk_pwrite(data->blk, offset, buf, buflen);
+    ret = blk_pwrite(data->blk, offset, buf, buflen, 0);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not write encryption header");
         return ret;
diff --git a/block/parallels.c b/block/parallels.c
index 324ed43..2d8bc87 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -512,7 +512,7 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
     memset(tmp, 0, sizeof(tmp));
     memcpy(tmp, &header, sizeof(header));

-    ret = blk_pwrite(file, 0, tmp, BDRV_SECTOR_SIZE);
+    ret = blk_pwrite(file, 0, tmp, BDRV_SECTOR_SIZE, 0);
     if (ret < 0) {
         goto exit;
     }
diff --git a/block/qcow.c b/block/qcow.c
index 60ddb12..d6dc1b0 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -853,14 +853,14 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
     }

     /* write all the data */
-    ret = blk_pwrite(qcow_blk, 0, &header, sizeof(header));
+    ret = blk_pwrite(qcow_blk, 0, &header, sizeof(header), 0);
     if (ret != sizeof(header)) {
         goto exit;
     }

     if (backing_file) {
         ret = blk_pwrite(qcow_blk, sizeof(header),
-            backing_file, backing_filename_len);
+                         backing_file, backing_filename_len, 0);
         if (ret != backing_filename_len) {
             goto exit;
         }
@@ -869,8 +869,8 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
     tmp = g_malloc0(BDRV_SECTOR_SIZE);
     for (i = 0; i < ((sizeof(uint64_t)*l1_size + BDRV_SECTOR_SIZE - 1)/
         BDRV_SECTOR_SIZE); i++) {
-        ret = blk_pwrite(qcow_blk, header_size +
-            BDRV_SECTOR_SIZE*i, tmp, BDRV_SECTOR_SIZE);
+        ret = blk_pwrite(qcow_blk, header_size + BDRV_SECTOR_SIZE * i,
+                         tmp, BDRV_SECTOR_SIZE, 0);
         if (ret != BDRV_SECTOR_SIZE) {
             g_free(tmp);
             goto exit;
diff --git a/block/qcow2.c b/block/qcow2.c
index 470734b..3090538 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2207,7 +2207,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
             cpu_to_be64(QCOW2_COMPAT_LAZY_REFCOUNTS);
     }

-    ret = blk_pwrite(blk, 0, header, cluster_size);
+    ret = blk_pwrite(blk, 0, header, cluster_size, 0);
     g_free(header);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not write qcow2 header");
@@ -2217,7 +2217,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     /* Write a refcount table with one refcount block */
     refcount_table = g_malloc0(2 * cluster_size);
     refcount_table[0] = cpu_to_be64(2 * cluster_size);
-    ret = blk_pwrite(blk, cluster_size, refcount_table, 2 * cluster_size);
+    ret = blk_pwrite(blk, cluster_size, refcount_table, 2 * cluster_size, 0);
     g_free(refcount_table);

     if (ret < 0) {
diff --git a/block/qed.c b/block/qed.c
index 0af5274..6cfd4c1 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -601,18 +601,18 @@ static int qed_create(const char *filename, uint32_t cluster_size,
     }

     qed_header_cpu_to_le(&header, &le_header);
-    ret = blk_pwrite(blk, 0, &le_header, sizeof(le_header));
+    ret = blk_pwrite(blk, 0, &le_header, sizeof(le_header), 0);
     if (ret < 0) {
         goto out;
     }
     ret = blk_pwrite(blk, sizeof(le_header), backing_file,
-                     header.backing_filename_size);
+                     header.backing_filename_size, 0);
     if (ret < 0) {
         goto out;
     }

     l1_table = g_malloc0(l1_size);
-    ret = blk_pwrite(blk, header.l1_table_offset, l1_table, l1_size);
+    ret = blk_pwrite(blk, header.l1_table_offset, l1_table, l1_size, 0);
     if (ret < 0) {
         goto out;
     }
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 33e0a33..625f876 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1678,7 +1678,7 @@ static int sd_prealloc(const char *filename, Error **errp)
         if (ret < 0) {
             goto out;
         }
-        ret = blk_pwrite(blk, idx * buf_size, buf, buf_size);
+        ret = blk_pwrite(blk, idx * buf_size, buf, buf_size, 0);
         if (ret < 0) {
             goto out;
         }
diff --git a/block/vdi.c b/block/vdi.c
index e5fe4e8..54e1144 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -827,7 +827,7 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
     vdi_header_print(&header);
 #endif
     vdi_header_to_le(&header);
-    ret = blk_pwrite(blk, offset, &header, sizeof(header));
+    ret = blk_pwrite(blk, offset, &header, sizeof(header), 0);
     if (ret < 0) {
         error_setg(errp, "Error writing header to %s", filename);
         goto exit;
@@ -848,7 +848,7 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
                 bmap[i] = VDI_UNALLOCATED;
             }
         }
-        ret = blk_pwrite(blk, offset, bmap, bmap_size);
+        ret = blk_pwrite(blk, offset, bmap, bmap_size, 0);
         if (ret < 0) {
             error_setg(errp, "Error writing bmap to %s", filename);
             goto exit;
diff --git a/block/vhdx.c b/block/vhdx.c
index 2b7b332..ec778fe 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1856,13 +1856,14 @@ static int vhdx_create(const char *filename, QemuOpts *opts, Error **errp)
     creator = g_utf8_to_utf16("QEMU v" QEMU_VERSION, -1, NULL,
                               &creator_items, NULL);
     signature = cpu_to_le64(VHDX_FILE_SIGNATURE);
-    ret = blk_pwrite(blk, VHDX_FILE_ID_OFFSET, &signature, sizeof(signature));
+    ret = blk_pwrite(blk, VHDX_FILE_ID_OFFSET, &signature, sizeof(signature),
+                     0);
     if (ret < 0) {
         goto delete_and_exit;
     }
     if (creator) {
         ret = blk_pwrite(blk, VHDX_FILE_ID_OFFSET + sizeof(signature),
-                         creator, creator_items * sizeof(gunichar2));
+                         creator, creator_items * sizeof(gunichar2), 0);
         if (ret < 0) {
             goto delete_and_exit;
         }
diff --git a/block/vmdk.c b/block/vmdk.c
index f243527..908d619 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1808,12 +1808,12 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
     header.check_bytes[3] = 0xa;

     /* write all the data */
-    ret = blk_pwrite(blk, 0, &magic, sizeof(magic));
+    ret = blk_pwrite(blk, 0, &magic, sizeof(magic), 0);
     if (ret < 0) {
         error_setg(errp, QERR_IO_ERROR);
         goto exit;
     }
-    ret = blk_pwrite(blk, sizeof(magic), &header, sizeof(header));
+    ret = blk_pwrite(blk, sizeof(magic), &header, sizeof(header), 0);
     if (ret < 0) {
         error_setg(errp, QERR_IO_ERROR);
         goto exit;
@@ -1833,7 +1833,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
         gd_buf[i] = cpu_to_le32(tmp);
     }
     ret = blk_pwrite(blk, le64_to_cpu(header.rgd_offset) * BDRV_SECTOR_SIZE,
-                     gd_buf, gd_buf_size);
+                     gd_buf, gd_buf_size, 0);
     if (ret < 0) {
         error_setg(errp, QERR_IO_ERROR);
         goto exit;
@@ -1845,7 +1845,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
         gd_buf[i] = cpu_to_le32(tmp);
     }
     ret = blk_pwrite(blk, le64_to_cpu(header.gd_offset) * BDRV_SECTOR_SIZE,
-                     gd_buf, gd_buf_size);
+                     gd_buf, gd_buf_size, 0);
     if (ret < 0) {
         error_setg(errp, QERR_IO_ERROR);
         goto exit;
@@ -2108,7 +2108,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)

     blk_set_allow_write_beyond_eof(new_blk, true);

-    ret = blk_pwrite(new_blk, desc_offset, desc, desc_len);
+    ret = blk_pwrite(new_blk, desc_offset, desc, desc_len, 0);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not write description");
         goto exit;
diff --git a/block/vpc.c b/block/vpc.c
index 2da4126..0379813 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -788,13 +788,13 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     block_size = 0x200000;
     num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);

-    ret = blk_pwrite(blk, offset, buf, HEADER_SIZE);
+    ret = blk_pwrite(blk, offset, buf, HEADER_SIZE, 0);
     if (ret < 0) {
         goto fail;
     }

     offset = 1536 + ((num_bat_entries * 4 + 511) & ~511);
-    ret = blk_pwrite(blk, offset, buf, HEADER_SIZE);
+    ret = blk_pwrite(blk, offset, buf, HEADER_SIZE, 0);
     if (ret < 0) {
         goto fail;
     }
@@ -804,7 +804,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,

     memset(buf, 0xFF, 512);
     for (i = 0; i < (num_bat_entries * 4 + 511) / 512; i++) {
-        ret = blk_pwrite(blk, offset, buf, 512);
+        ret = blk_pwrite(blk, offset, buf, 512, 0);
         if (ret < 0) {
             goto fail;
         }
@@ -831,7 +831,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     /* Write the header */
     offset = 512;

-    ret = blk_pwrite(blk, offset, buf, 1024);
+    ret = blk_pwrite(blk, offset, buf, 1024, 0);
     if (ret < 0) {
         goto fail;
     }
@@ -853,7 +853,7 @@ static int create_fixed_disk(BlockBackend *blk, uint8_t *buf,
         return ret;
     }

-    ret = blk_pwrite(blk, total_size - HEADER_SIZE, buf, HEADER_SIZE);
+    ret = blk_pwrite(blk, total_size - HEADER_SIZE, buf, HEADER_SIZE, 0);
     if (ret < 0) {
         return ret;
     }
diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index 802636e..019f25d 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -124,7 +124,7 @@ static void rtas_nvram_store(PowerPCCPU *cpu, sPAPRMachineState *spapr,

     alen = len;
     if (nvram->blk) {
-        alen = blk_pwrite(nvram->blk, offset, membuf, len);
+        alen = blk_pwrite(nvram->blk, offset, membuf, len, 0);
     }

     assert(nvram->buf);
@@ -190,7 +190,7 @@ static int spapr_nvram_post_load(void *opaque, int version_id)
     sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);

     if (nvram->blk) {
-        int alen = blk_pwrite(nvram->blk, 0, nvram->buf, nvram->size);
+        int alen = blk_pwrite(nvram->blk, 0, nvram->buf, nvram->size, 0);

         if (alen < 0) {
             return alen;
diff --git a/nbd/server.c b/nbd/server.c
index 2184c64..fa862cd 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1115,7 +1115,7 @@ static void nbd_trip(void *opaque)
         TRACE("Writing to device");

         ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
-                        req->data, request.len);
+                         req->data, request.len, 0);
         if (ret < 0) {
             LOG("writing to file failed");
             reply.error = -ret;
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index e34f777..e26e543 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -474,7 +474,7 @@ static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset,
         return -ERANGE;
     }

-    *total = blk_pwrite(blk, offset, (uint8_t *)buf, count);
+    *total = blk_pwrite(blk, offset, (uint8_t *)buf, count, 0);
     if (*total < 0) {
         return *total;
     }
-- 
2.5.5

[Qemu-devel] [PATCH v4 02/14] fdc: Switch to byte-based block access

[Eric Blake]

Sector-based blk_write() should die; switch to byte-based
blk_pwrite() instead.  Likewise for blk_read().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 hw/block/fdc.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 3722275..f73af7d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -223,6 +223,13 @@ static int fd_sector(FDrive *drv)
                           NUM_SIDES(drv));
 }

+/* Returns current position, in bytes, for given drive */
+static int fd_offset(FDrive *drv)
+{
+    g_assert(fd_sector(drv) < INT_MAX >> BDRV_SECTOR_BITS);
+    return fd_sector(drv) << BDRV_SECTOR_BITS;
+}
+
 /* Seek to a new position:
  * returns 0 if already on right track
  * returns 1 if track changed
@@ -1629,8 +1636,8 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
         if (fdctrl->data_dir != FD_DIR_WRITE ||
             len < FD_SECTOR_LEN || rel_pos != 0) {
             /* READ & SCAN commands and realign to a sector for WRITE */
-            if (blk_read(cur_drv->blk, fd_sector(cur_drv),
-                         fdctrl->fifo, 1) < 0) {
+            if (blk_pread(cur_drv->blk, fd_offset(cur_drv),
+                          fdctrl->fifo, BDRV_SECTOR_SIZE) < 0) {
                 FLOPPY_DPRINTF("Floppy: error getting sector %d\n",
                                fd_sector(cur_drv));
                 /* Sure, image size is too small... */
@@ -1657,8 +1664,8 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,

             k->read_memory(fdctrl->dma, nchan, fdctrl->fifo + rel_pos,
                            fdctrl->data_pos, len);
-            if (blk_write(cur_drv->blk, fd_sector(cur_drv),
-                          fdctrl->fifo, 1) < 0) {
+            if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv),
+                           fdctrl->fifo, BDRV_SECTOR_SIZE, 0) < 0) {
                 FLOPPY_DPRINTF("error writing sector %d\n",
                                fd_sector(cur_drv));
                 fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
@@ -1741,7 +1748,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
                                    fd_sector(cur_drv));
                     return 0;
                 }
-            if (blk_read(cur_drv->blk, fd_sector(cur_drv), fdctrl->fifo, 1)
+            if (blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
+                          BDRV_SECTOR_SIZE)
                 < 0) {
                 FLOPPY_DPRINTF("error getting sector %d\n",
                                fd_sector(cur_drv));
@@ -1820,7 +1828,8 @@ static void fdctrl_format_sector(FDCtrl *fdctrl)
     }
     memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
     if (cur_drv->blk == NULL ||
-        blk_write(cur_drv->blk, fd_sector(cur_drv), fdctrl->fifo, 1) < 0) {
+        blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
+                   BDRV_SECTOR_SIZE, 0) < 0) {
         FLOPPY_DPRINTF("error formatting sector %d\n", fd_sector(cur_drv));
         fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
     } else {
@@ -2243,8 +2252,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
         if (pos == FD_SECTOR_LEN - 1 ||
             fdctrl->data_pos == fdctrl->data_len) {
             cur_drv = get_cur_drv(fdctrl);
-            if (blk_write(cur_drv->blk, fd_sector(cur_drv), fdctrl->fifo, 1)
-                < 0) {
+            if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
+                           BDRV_SECTOR_SIZE, 0) < 0) {
                 FLOPPY_DPRINTF("error writing sector %d\n",
                                fd_sector(cur_drv));
                 break;
-- 
2.5.5

[Qemu-devel] [PATCH v4 03/14] nand: Switch to byte-based block access

[Eric Blake]

Sector-based blk_write() should die; switch to byte-based
blk_pwrite() instead.  Likewise for blk_read().

This file is doing some complex computations to map various
flash page sizes (256, 512, and 2048) atop generic uses of
512-byte sector operations.  Perhaps someone will want to tidy
up the file for fewer gymnastics in managing addresses and
offsets, and less wasteful visits of 256-byte pages, but it
was out of scope for this series, where I just went with the
mechanical conversion.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 hw/block/nand.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 29c6596..2703ff4 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -663,7 +663,8 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s)
         sector = SECTOR(s->addr);
         off = (s->addr & PAGE_MASK) + s->offset;
         soff = SECTOR_OFFSET(s->addr);
-        if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS) < 0) {
+        if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
+                      PAGE_SECTORS << BDRV_SECTOR_BITS) < 0) {
             printf("%s: read error in sector %" PRIu64 "\n", __func__, sector);
             return;
         }
@@ -675,21 +676,24 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s)
                             MIN(OOB_SIZE, off + s->iolen - PAGE_SIZE));
         }

-        if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS) < 0) {
+        if (blk_pwrite(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
+                       PAGE_SECTORS << BDRV_SECTOR_BITS, 0) < 0) {
             printf("%s: write error in sector %" PRIu64 "\n", __func__, sector);
         }
     } else {
         off = PAGE_START(s->addr) + (s->addr & PAGE_MASK) + s->offset;
         sector = off >> 9;
         soff = off & 0x1ff;
-        if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
+        if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
+                     (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
             printf("%s: read error in sector %" PRIu64 "\n", __func__, sector);
             return;
         }

         mem_and(iobuf + soff, s->io, s->iolen);

-        if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
+        if (blk_write(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
+                      (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS, 0) < 0) {
             printf("%s: write error in sector %" PRIu64 "\n", __func__, sector);
         }
     }
@@ -716,17 +720,20 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
         i = SECTOR(addr);
         page = SECTOR(addr + (1 << (ADDR_SHIFT + s->erase_shift)));
         for (; i < page; i ++)
-            if (blk_write(s->blk, i, iobuf, 1) < 0) {
+            if (blk_pwrite(s->blk, i << BDRV_SECTOR_BITS, iobuf,
+                           BDRV_SECTOR_SIZE, 0) < 0) {
                 printf("%s: write error in sector %" PRIu64 "\n", __func__, i);
             }
     } else {
         addr = PAGE_START(addr);
         page = addr >> 9;
-        if (blk_read(s->blk, page, iobuf, 1) < 0) {
+        if (blk_pread(s->blk, page << BDRV_SECTOR_BITS, iobuf,
+                      BDRV_SECTOR_SIZE) < 0) {
             printf("%s: read error in sector %" PRIu64 "\n", __func__, page);
         }
         memset(iobuf + (addr & 0x1ff), 0xff, (~addr & 0x1ff) + 1);
-        if (blk_write(s->blk, page, iobuf, 1) < 0) {
+        if (blk_pwrite(s->blk, page << BDRV_SECTOR_BITS, iobuf,
+                       BDRV_SECTOR_SIZE, 0) < 0) {
             printf("%s: write error in sector %" PRIu64 "\n", __func__, page);
         }

@@ -734,18 +741,20 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
         i = (addr & ~0x1ff) + 0x200;
         for (addr += ((PAGE_SIZE + OOB_SIZE) << s->erase_shift) - 0x200;
                         i < addr; i += 0x200) {
-            if (blk_write(s->blk, i >> 9, iobuf, 1) < 0) {
+            if (blk_pwrite(s->blk, i, iobuf, BDRV_SECTOR_SIZE, 0) < 0) {
                 printf("%s: write error in sector %" PRIu64 "\n",
                        __func__, i >> 9);
             }
         }

         page = i >> 9;
-        if (blk_read(s->blk, page, iobuf, 1) < 0) {
+        if (blk_pread(s->blk, page << BDRV_SECTOR_BITS, iobuf,
+                      BDRV_SECTOR_SIZE) < 0) {
             printf("%s: read error in sector %" PRIu64 "\n", __func__, page);
         }
         memset(iobuf, 0xff, ((addr - 1) & 0x1ff) + 1);
-        if (blk_write(s->blk, page, iobuf, 1) < 0) {
+        if (blk_pwrite(s->blk, page << BDRV_SECTOR_BITS, iobuf,
+                       BDRV_SECTOR_SIZE, 0) < 0) {
             printf("%s: write error in sector %" PRIu64 "\n", __func__, page);
         }
     }
@@ -760,7 +769,8 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,

     if (s->blk) {
         if (s->mem_oob) {
-            if (blk_read(s->blk, SECTOR(addr), s->io, PAGE_SECTORS) < 0) {
+            if (blk_pread(s->blk, SECTOR(addr) << BDRV_SECTOR_BITS, s->io,
+                          PAGE_SECTORS << BDRV_SECTOR_BITS) < 0) {
                 printf("%s: read error in sector %" PRIu64 "\n",
                                 __func__, SECTOR(addr));
             }
@@ -769,8 +779,8 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,
                             OOB_SIZE);
             s->ioaddr = s->io + SECTOR_OFFSET(s->addr) + offset;
         } else {
-            if (blk_read(s->blk, PAGE_START(addr) >> 9,
-                         s->io, (PAGE_SECTORS + 2)) < 0) {
+            if (blk_pread(s->blk, PAGE_START(addr), s->io,
+                          (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
                 printf("%s: read error in sector %" PRIu64 "\n",
                                 __func__, PAGE_START(addr) >> 9);
             }
-- 
2.5.5

[Qemu-devel] [PATCH v4 04/14] onenand: Switch to byte-based block access

[Eric Blake]

Sector-based blk_write() should die; switch to byte-based
blk_pwrite() instead.  Likewise for blk_read().

Signed-off-by: Eric Blake <eblake@redhat.com>

---
Not compile tested - I'm not sure what else I'd need in my environment
to actually test this one.  I have:
Fedora 23, dnf builddep qemu
./configure --enable-kvm --enable-system --disable-user --target-list=x86_64-softmmu,ppc64-softmmu --enable-debug
---
 hw/block/onenand.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 883f4b1..3d19b0c 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -224,7 +224,8 @@ static void onenand_reset(OneNANDState *s, int cold)
         /* Lock the whole flash */
         memset(s->blockwp, ONEN_LOCK_LOCKED, s->blocks);

-        if (s->blk_cur && blk_read(s->blk_cur, 0, s->boot[0], 8) < 0) {
+        if (s->blk_cur && blk_pread(s->blk_cur, 0, s->boot[0],
+                                    8 << BDRV_SECTOR_BITS) < 0) {
             hw_error("%s: Loading the BootRAM failed.\n", __func__);
         }
     }
@@ -241,7 +242,8 @@ static inline int onenand_load_main(OneNANDState *s, int sec, int secn,
                 void *dest)
 {
     if (s->blk_cur) {
-        return blk_read(s->blk_cur, sec, dest, secn) < 0;
+        return blk_pread(s->blk_cur, sec << BDRV_SECTOR_BITS, dest,
+                         secn << BDRV_SECTOR_BITS) < 0;
     } else if (sec + secn > s->secs_cur) {
         return 1;
     }
@@ -257,19 +259,20 @@ static inline int onenand_prog_main(OneNANDState *s, int sec, int secn,
     int result = 0;

     if (secn > 0) {
-        uint32_t size = (uint32_t)secn * 512;
+        uint32_t size = (uint32_t)secn << BDRV_SECTOR_BITS;
+        int64_t offset = sec << BDRV_SECTOR_BITS;
         const uint8_t *sp = (const uint8_t *)src;
         uint8_t *dp = 0;
         if (s->blk_cur) {
             dp = g_malloc(size);
-            if (!dp || blk_read(s->blk_cur, sec, dp, secn) < 0) {
+            if (!dp || blk_pread(s->blk_cur, offset, dp, size) < 0) {
                 result = 1;
             }
         } else {
             if (sec + secn > s->secs_cur) {
                 result = 1;
             } else {
-                dp = (uint8_t *)s->current + (sec << 9);
+                dp = (uint8_t *)s->current + offset;
             }
         }
         if (!result) {
@@ -278,7 +281,7 @@ static inline int onenand_prog_main(OneNANDState *s, int sec, int secn,
                 dp[i] &= sp[i];
             }
             if (s->blk_cur) {
-                result = blk_write(s->blk_cur, sec, dp, secn) < 0;
+                result = blk_pwrite(s->blk_cur, offset, dp, size, 0) < 0;
             }
         }
         if (dp && s->blk_cur) {
@@ -295,7 +298,8 @@ static inline int onenand_load_spare(OneNANDState *s, int sec, int secn,
     uint8_t buf[512];

     if (s->blk_cur) {
-        if (blk_read(s->blk_cur, s->secs_cur + (sec >> 5), buf, 1) < 0) {
+        int32_t offset = (s->secs_cur + (sec >> 5)) << BDRV_SECTOR_BITS;
+        if (blk_pread(s->blk_cur, offset, buf, BDRV_SECTOR_SIZE) < 0) {
             return 1;
         }
         memcpy(dest, buf + ((sec & 31) << 4), secn << 4);
@@ -304,7 +308,7 @@ static inline int onenand_load_spare(OneNANDState *s, int sec, int secn,
     } else {
         memcpy(dest, s->current + (s->secs_cur << 9) + (sec << 4), secn << 4);
     }
- 
+
     return 0;
 }

@@ -315,10 +319,11 @@ static inline int onenand_prog_spare(OneNANDState *s, int sec, int secn,
     if (secn > 0) {
         const uint8_t *sp = (const uint8_t *)src;
         uint8_t *dp = 0, *dpp = 0;
+        uint64_t offset = (s->secs_cur + (sec >> 5)) << BDRV_SECTOR_BITS;
         if (s->blk_cur) {
             dp = g_malloc(512);
             if (!dp
-                || blk_read(s->blk_cur, s->secs_cur + (sec >> 5), dp, 1) < 0) {
+                || blk_pread(s->blk_cur, offset, dp, BDRV_SECTOR_SIZE) < 0) {
                 result = 1;
             } else {
                 dpp = dp + ((sec & 31) << 4);
@@ -336,8 +341,8 @@ static inline int onenand_prog_spare(OneNANDState *s, int sec, int secn,
                 dpp[i] &= sp[i];
             }
             if (s->blk_cur) {
-                result = blk_write(s->blk_cur, s->secs_cur + (sec >> 5),
-                                   dp, 1) < 0;
+                result = blk_pwrite(s->blk_cur, offset, dp,
+                                    BDRV_SECTOR_SIZE, 0) < 0;
             }
         }
         g_free(dp);
@@ -355,14 +360,17 @@ static inline int onenand_erase(OneNANDState *s, int sec, int num)
     for (; num > 0; num--, sec++) {
         if (s->blk_cur) {
             int erasesec = s->secs_cur + (sec >> 5);
-            if (blk_write(s->blk_cur, sec, blankbuf, 1) < 0) {
+            if (blk_pwrite(s->blk_cur, sec << BDRV_SECTOR_BITS, blankbuf,
+                           BDRV_SECTOR_SIZE, 0) < 0) {
                 goto fail;
             }
-            if (blk_read(s->blk_cur, erasesec, tmpbuf, 1) < 0) {
+            if (blk_pread(s->blk_cur, erasesec << BDRV_SECTOR_BITS, tmpbuf,
+                          BDRV_SECTOR_SIZE) < 0) {
                 goto fail;
             }
             memcpy(tmpbuf + ((sec & 31) << 4), blankbuf, 1 << 4);
-            if (blk_write(s->blk_cur, erasesec, tmpbuf, 1) < 0) {
+            if (blk_pwrite(s->blk_cur, erasesec << BDRV_SECTOR_BITS, tmpbuf,
+                           BDRV_SECTOR_SIZE, 0) < 0) {
                 goto fail;
             }
         } else {
-- 
2.5.5

[Qemu-devel] [PATCH v4 05/14] pflash: Switch to byte-based block access

[Eric Blake]

Sector-based blk_write() should die; switch to byte-based
blk_pwrite() instead.  Likewise for blk_read().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 hw/block/pflash_cfi01.c | 12 ++++++------
 hw/block/pflash_cfi02.c | 12 ++++++------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 106a775..3a1f85d 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -413,11 +413,11 @@ static void pflash_update(pflash_t *pfl, int offset,
     int offset_end;
     if (pfl->blk) {
         offset_end = offset + size;
-        /* round to sectors */
-        offset = offset >> 9;
-        offset_end = (offset_end + 511) >> 9;
-        blk_write(pfl->blk, offset, pfl->storage + (offset << 9),
-                  offset_end - offset);
+        /* widen to sector boundaries */
+        offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
+        offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
+        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
+                   offset_end - offset, 0);
     }
 }

@@ -739,7 +739,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)

     if (pfl->blk) {
         /* read the initial flash content */
-        ret = blk_read(pfl->blk, 0, pfl->storage, total_len >> 9);
+        ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);

         if (ret < 0) {
             vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index b13172c..5f10610 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -253,11 +253,11 @@ static void pflash_update(pflash_t *pfl, int offset,
     int offset_end;
     if (pfl->blk) {
         offset_end = offset + size;
-        /* round to sectors */
-        offset = offset >> 9;
-        offset_end = (offset_end + 511) >> 9;
-        blk_write(pfl->blk, offset, pfl->storage + (offset << 9),
-                  offset_end - offset);
+        /* widen to sector boundaries */
+        offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
+        offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
+        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
+                   offset_end - offset, 0);
     }
 }

@@ -622,7 +622,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     pfl->chip_len = chip_len;
     if (pfl->blk) {
         /* read the initial flash content */
-        ret = blk_read(pfl->blk, 0, pfl->storage, chip_len >> 9);
+        ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
         if (ret < 0) {
             vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
             error_setg(errp, "failed to read the initial flash content");
-- 
2.5.5

[Qemu-devel] [PATCH v4 06/14] sd: Switch to byte-based block access

[Eric Blake]

Sector-based blk_write() should die; switch to byte-based
blk_pwrite() instead.  Likewise for blk_read().

Greatly simplifies the code, now that we let the block layer
take care of alignment and read-modify-write on our behalf :)

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 hw/sd/sd.c | 46 +++-------------------------------------------
 1 file changed, 3 insertions(+), 43 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b66e5d2..3c2f2f1 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1577,57 +1577,17 @@ send_response:

 static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
 {
-    uint64_t end = addr + len;
-
     DPRINTF("sd_blk_read: addr = 0x%08llx, len = %d\n",
             (unsigned long long) addr, len);
-    if (!sd->blk || blk_read(sd->blk, addr >> 9, sd->buf, 1) < 0) {
+    if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
         fprintf(stderr, "sd_blk_read: read error on host side\n");
-        return;
     }
-
-    if (end > (addr & ~511) + 512) {
-        memcpy(sd->data, sd->buf + (addr & 511), 512 - (addr & 511));
-
-        if (blk_read(sd->blk, end >> 9, sd->buf, 1) < 0) {
-            fprintf(stderr, "sd_blk_read: read error on host side\n");
-            return;
-        }
-        memcpy(sd->data + 512 - (addr & 511), sd->buf, end & 511);
-    } else
-        memcpy(sd->data, sd->buf + (addr & 511), len);
 }

 static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
 {
-    uint64_t end = addr + len;
-
-    if ((addr & 511) || len < 512)
-        if (!sd->blk || blk_read(sd->blk, addr >> 9, sd->buf, 1) < 0) {
-            fprintf(stderr, "sd_blk_write: read error on host side\n");
-            return;
-        }
-
-    if (end > (addr & ~511) + 512) {
-        memcpy(sd->buf + (addr & 511), sd->data, 512 - (addr & 511));
-        if (blk_write(sd->blk, addr >> 9, sd->buf, 1) < 0) {
-            fprintf(stderr, "sd_blk_write: write error on host side\n");
-            return;
-        }
-
-        if (blk_read(sd->blk, end >> 9, sd->buf, 1) < 0) {
-            fprintf(stderr, "sd_blk_write: read error on host side\n");
-            return;
-        }
-        memcpy(sd->buf, sd->data + 512 - (addr & 511), end & 511);
-        if (blk_write(sd->blk, end >> 9, sd->buf, 1) < 0) {
-            fprintf(stderr, "sd_blk_write: write error on host side\n");
-        }
-    } else {
-        memcpy(sd->buf + (addr & 511), sd->data, len);
-        if (!sd->blk || blk_write(sd->blk, addr >> 9, sd->buf, 1) < 0) {
-            fprintf(stderr, "sd_blk_write: write error on host side\n");
-        }
+    if (!sd->blk || blk_pwrite(sd->blk, addr, sd->buf, len, 0) < 0) {
+        fprintf(stderr, "sd_blk_write: write error on host side\n");
     }
 }

-- 
2.5.5

[Qemu-devel] [PATCH v4 07/14] m25p80: Switch to byte-based block access

[Eric Blake]

Sector-based blk_read() should die; switch to byte-based
blk_pread() instead.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
Not compile tested - I'm not sure what else I'd need in my environment
to actually test this one.  I have:
Fedora 23, dnf builddep qemu
./configure --enable-kvm --enable-system --disable-user --target-list=x86_64-softmmu,ppc64-softmmu --enable-debug
---
 hw/block/m25p80.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 906b712..01c51a2 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -907,8 +907,7 @@ static int m25p80_init(SSISlave *ss)
         s->storage = blk_blockalign(s->blk, s->size);

         /* FIXME: Move to late init */
-        if (blk_read(s->blk, 0, s->storage,
-                     DIV_ROUND_UP(s->size, BDRV_SECTOR_SIZE))) {
+        if (blk_pread(s->blk, 0, s->storage, s->size)) {
             fprintf(stderr, "Failed to initialize SPI flash!\n");
             return 1;
         }
-- 
2.5.5

[Qemu-devel] [PATCH v4 08/14] atapi: Switch to byte-based block access

[Eric Blake]

Sector-based blk_read() should die; switch to byte-based
blk_pread() instead.

Add new defines ATAPI_SECTOR_BITS and ATAPI_SECTOR_SIZE to
use anywhere we were previously scaling BDRV_SECTOR_* by 4,
for better legibility.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v4: add new defines for use in more places [jsnow]
---
 hw/ide/atapi.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 2bb606c..95056d9 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -28,6 +28,9 @@
 #include "hw/scsi/scsi.h"
 #include "sysemu/block-backend.h"

+#define ATAPI_SECTOR_BITS (2 + BDRV_SECTOR_BITS)
+#define ATAPI_SECTOR_SIZE (1 << ATAPI_SECTOR_BITS)
+
 static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret);

 static void padstr8(uint8_t *buf, int buf_size, const char *src)
@@ -111,7 +114,7 @@ cd_read_sector_sync(IDEState *s)
 {
     int ret;
     block_acct_start(blk_get_stats(s->blk), &s->acct,
-                     4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
+                     ATAPI_SECTOR_SIZE, BLOCK_ACCT_READ);

 #ifdef DEBUG_IDE_ATAPI
     printf("cd_read_sector_sync: lba=%d\n", s->lba);
@@ -119,12 +122,12 @@ cd_read_sector_sync(IDEState *s)

     switch (s->cd_sector_size) {
     case 2048:
-        ret = blk_read(s->blk, (int64_t)s->lba << 2,
-                       s->io_buffer, 4);
+        ret = blk_pread(s->blk, (int64_t)s->lba << ATAPI_SECTOR_BITS,
+                        s->io_buffer, ATAPI_SECTOR_SIZE);
         break;
     case 2352:
-        ret = blk_read(s->blk, (int64_t)s->lba << 2,
-                       s->io_buffer + 16, 4);
+        ret = blk_pread(s->blk, (int64_t)s->lba << ATAPI_SECTOR_BITS,
+                        s->io_buffer + 16, ATAPI_SECTOR_SIZE);
         if (ret >= 0) {
             cd_data_to_raw(s->io_buffer, s->lba);
         }
@@ -182,7 +185,7 @@ static int cd_read_sector(IDEState *s)
     s->iov.iov_base = (s->cd_sector_size == 2352) ?
                       s->io_buffer + 16 : s->io_buffer;

-    s->iov.iov_len = 4 * BDRV_SECTOR_SIZE;
+    s->iov.iov_len = ATAPI_SECTOR_SIZE;
     qemu_iovec_init_external(&s->qiov, &s->iov, 1);

 #ifdef DEBUG_IDE_ATAPI
@@ -190,7 +193,7 @@ static int cd_read_sector(IDEState *s)
 #endif

     block_acct_start(blk_get_stats(s->blk), &s->acct,
-                     4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
+                     ATAPI_SECTOR_SIZE, BLOCK_ACCT_READ);

     ide_buffered_readv(s, (int64_t)s->lba << 2, &s->qiov, 4,
                        cd_read_sector_cb, s);
@@ -435,7 +438,7 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
 #endif

     s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
-    s->bus->dma->iov.iov_len = n * 4 * 512;
+    s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
     qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);

     s->bus->dma->aiocb = ide_buffered_readv(s, (int64_t)s->lba << 2,
-- 
2.5.5

[Qemu-devel] [PATCH v4 09/14] nbd: Switch to byte-based block access

[Eric Blake]

Sector-based blk_read() should die; switch to byte-based
blk_pread() instead.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-nbd.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index c55b40f..c07ceef 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -159,12 +159,13 @@ static int find_partition(BlockBackend *blk, int partition,
                           off_t *offset, off_t *size)
 {
     struct partition_record mbr[4];
-    uint8_t data[512];
+    uint8_t data[BDRV_SECTOR_SIZE];
     int i;
     int ext_partnum = 4;
     int ret;

-    if ((ret = blk_read(blk, 0, data, 1)) < 0) {
+    ret = blk_pread(blk, 0, data, sizeof(data));
+    if (ret < 0) {
         error_report("error while reading: %s", strerror(-ret));
         exit(EXIT_FAILURE);
     }
@@ -182,10 +183,12 @@ static int find_partition(BlockBackend *blk, int partition,

         if (mbr[i].system == 0xF || mbr[i].system == 0x5) {
             struct partition_record ext[4];
-            uint8_t data1[512];
+            uint8_t data1[BDRV_SECTOR_SIZE];
             int j;

-            if ((ret = blk_read(blk, mbr[i].start_sector_abs, data1, 1)) < 0) {
+            ret = blk_pread(blk, mbr[i].start_sector_abs << BDRV_SECTOR_BITS,
+                            data1, sizeof(data1));
+            if (ret < 0) {
                 error_report("error while reading: %s", strerror(-ret));
                 exit(EXIT_FAILURE);
             }
-- 
2.5.5

[Qemu-devel] [PATCH v4 10/14] qemu-img: Switch to byte-based block access

[Eric Blake]

Sector-based blk_write() should die; switch to byte-based
blk_pwrite() instead.  Likewise for blk_read().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 46f2a6d..c19f9d4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1088,7 +1088,8 @@ static int check_empty_sectors(BlockBackend *blk, int64_t sect_num,
                                uint8_t *buffer, bool quiet)
 {
     int pnum, ret = 0;
-    ret = blk_read(blk, sect_num, buffer, sect_count);
+    ret = blk_pread(blk, sect_num << BDRV_SECTOR_BITS, buffer,
+                    sect_count << BDRV_SECTOR_BITS);
     if (ret < 0) {
         error_report("Error while reading offset %" PRId64 " of %s: %s",
                      sectors_to_bytes(sect_num), filename, strerror(-ret));
@@ -1301,7 +1302,8 @@ static int img_compare(int argc, char **argv)
             nb_sectors = MIN(pnum1, pnum2);
         } else if (allocated1 == allocated2) {
             if (allocated1) {
-                ret = blk_read(blk1, sector_num, buf1, nb_sectors);
+                ret = blk_pread(blk1, sector_num << BDRV_SECTOR_BITS, buf1,
+                                nb_sectors << BDRV_SECTOR_BITS);
                 if (ret < 0) {
                     error_report("Error while reading offset %" PRId64 " of %s:"
                                  " %s", sectors_to_bytes(sector_num), filename1,
@@ -1309,7 +1311,8 @@ static int img_compare(int argc, char **argv)
                     ret = 4;
                     goto out;
                 }
-                ret = blk_read(blk2, sector_num, buf2, nb_sectors);
+                ret = blk_pread(blk2, sector_num << BDRV_SECTOR_BITS, buf2,
+                                nb_sectors << BDRV_SECTOR_BITS);
                 if (ret < 0) {
                     error_report("Error while reading offset %" PRId64
                                  " of %s: %s", sectors_to_bytes(sector_num),
@@ -1522,7 +1525,9 @@ static int convert_read(ImgConvertState *s, int64_t sector_num, int nb_sectors,
         bs_sectors = s->src_sectors[s->src_cur];

         n = MIN(nb_sectors, bs_sectors - (sector_num - s->src_cur_offset));
-        ret = blk_read(blk, sector_num - s->src_cur_offset, buf, n);
+        ret = blk_pread(blk,
+                        (sector_num - s->src_cur_offset) << BDRV_SECTOR_BITS,
+                        buf, n << BDRV_SECTOR_BITS);
         if (ret < 0) {
             return ret;
         }
@@ -1577,7 +1582,8 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
             if (!s->min_sparse ||
                 is_allocated_sectors_min(buf, n, &n, s->min_sparse))
             {
-                ret = blk_write(s->target, sector_num, buf, n);
+                ret = blk_pwrite(s->target, sector_num << BDRV_SECTOR_BITS,
+                                 buf, n << BDRV_SECTOR_BITS, 0);
                 if (ret < 0) {
                     return ret;
                 }
@@ -3023,7 +3029,8 @@ static int img_rebase(int argc, char **argv)
                     n = old_backing_num_sectors - sector;
                 }

-                ret = blk_read(blk_old_backing, sector, buf_old, n);
+                ret = blk_pread(blk_old_backing, sector << BDRV_SECTOR_BITS,
+                                buf_old, n << BDRV_SECTOR_BITS);
                 if (ret < 0) {
                     error_report("error while reading from old backing file");
                     goto out;
@@ -3037,7 +3044,8 @@ static int img_rebase(int argc, char **argv)
                     n = new_backing_num_sectors - sector;
                 }

-                ret = blk_read(blk_new_backing, sector, buf_new, n);
+                ret = blk_pread(blk_new_backing, sector << BDRV_SECTOR_BITS,
+                                buf_new, n << BDRV_SECTOR_BITS);
                 if (ret < 0) {
                     error_report("error while reading from new backing file");
                     goto out;
@@ -3053,8 +3061,10 @@ static int img_rebase(int argc, char **argv)
                 if (compare_sectors(buf_old + written * 512,
                     buf_new + written * 512, n - written, &pnum))
                 {
-                    ret = blk_write(blk, sector + written,
-                                    buf_old + written * 512, pnum);
+                    ret = blk_pwrite(blk,
+                                     (sector + written) << BDRV_SECTOR_BITS,
+                                     buf_old + written * 512,
+                                     pnum << BDRV_SECTOR_BITS, 0);
                     if (ret < 0) {
                         error_report("Error while writing to COW image: %s",
                             strerror(-ret));
-- 
2.5.5

[Qemu-devel] [PATCH v4 11/14] qemu-io: Switch to byte-based block access

[Eric Blake]

blk_write() and blk_read() are now very simple wrappers around
blk_pwrite() and blk_pread().  There's no reason to require
the user to pass in aligned numbers.  Keep 'read -p' and
'write -p' so that I don't have to hunt down and update all
users of qemu-io, but make the default 'read' and 'write' now
do the same behavior that used to require -p.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-io-cmds.c | 75 +++++++++++++---------------------------------------------
 1 file changed, 16 insertions(+), 59 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index e26e543..4184fb8 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -419,40 +419,6 @@ fail:
     return buf;
 }

-static int do_read(BlockBackend *blk, char *buf, int64_t offset, int64_t count,
-                   int64_t *total)
-{
-    int ret;
-
-    if (count >> 9 > INT_MAX) {
-        return -ERANGE;
-    }
-
-    ret = blk_read(blk, offset >> 9, (uint8_t *)buf, count >> 9);
-    if (ret < 0) {
-        return ret;
-    }
-    *total = count;
-    return 1;
-}
-
-static int do_write(BlockBackend *blk, char *buf, int64_t offset, int64_t count,
-                    int64_t *total)
-{
-    int ret;
-
-    if (count >> 9 > INT_MAX) {
-        return -ERANGE;
-    }
-
-    ret = blk_write(blk, offset >> 9, (uint8_t *)buf, count >> 9);
-    if (ret < 0) {
-        return ret;
-    }
-    *total = count;
-    return 1;
-}
-
 static int do_pread(BlockBackend *blk, char *buf, int64_t offset,
                     int64_t count, int64_t *total)
 {
@@ -671,7 +637,7 @@ static void read_help(void)
 " -b, -- read from the VM state rather than the virtual disk\n"
 " -C, -- report statistics in a machine parsable format\n"
 " -l, -- length for pattern verification (only with -P)\n"
-" -p, -- use blk_pread to read the file\n"
+" -p, -- ignored for back-compat\n"
 " -P, -- use a pattern to verify read data\n"
 " -q, -- quiet mode, do not show I/O statistics\n"
 " -s, -- start offset for pattern verification (only with -P)\n"
@@ -687,7 +653,7 @@ static const cmdinfo_t read_cmd = {
     .cfunc      = read_f,
     .argmin     = 2,
     .argmax     = -1,
-    .args       = "[-abCpqv] [-P pattern [-s off] [-l len]] off len",
+    .args       = "[-abCqv] [-P pattern [-s off] [-l len]] off len",
     .oneline    = "reads a number of bytes at a specified offset",
     .help       = read_help,
 };
@@ -695,7 +661,7 @@ static const cmdinfo_t read_cmd = {
 static int read_f(BlockBackend *blk, int argc, char **argv)
 {
     struct timeval t1, t2;
-    int Cflag = 0, pflag = 0, qflag = 0, vflag = 0;
+    int Cflag = 0, qflag = 0, vflag = 0;
     int Pflag = 0, sflag = 0, lflag = 0, bflag = 0;
     int c, cnt;
     char *buf;
@@ -723,7 +689,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             }
             break;
         case 'p':
-            pflag = 1;
+            /* Ignored for back-compat */
             break;
         case 'P':
             Pflag = 1;
@@ -755,11 +721,6 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
         return qemuio_command_usage(&read_cmd);
     }

-    if (bflag && pflag) {
-        printf("-b and -p cannot be specified at the same time\n");
-        return 0;
-    }
-
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
         print_cvtnum_err(offset, argv[optind]);
@@ -790,7 +751,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
         return 0;
     }

-    if (!pflag) {
+    if (bflag) {
         if (offset & 0x1ff) {
             printf("offset %" PRId64 " is not sector aligned\n",
                    offset);
@@ -806,12 +767,10 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     buf = qemu_io_alloc(blk, count, 0xab);

     gettimeofday(&t1, NULL);
-    if (pflag) {
-        cnt = do_pread(blk, buf, offset, count, &total);
-    } else if (bflag) {
+    if (bflag) {
         cnt = do_load_vmstate(blk, buf, offset, count, &total);
     } else {
-        cnt = do_read(blk, buf, offset, count, &total);
+        cnt = do_pread(blk, buf, offset, count, &total);
     }
     gettimeofday(&t2, NULL);

@@ -991,7 +950,7 @@ static void write_help(void)
 " filled with a set pattern (0xcdcdcdcd).\n"
 " -b, -- write to the VM state rather than the virtual disk\n"
 " -c, -- write compressed data with blk_write_compressed\n"
-" -p, -- use blk_pwrite to write the file\n"
+" -p, -- ignored for back-compat\n"
 " -P, -- use different pattern to fill file\n"
 " -C, -- report statistics in a machine parsable format\n"
 " -q, -- quiet mode, do not show I/O statistics\n"
@@ -1007,7 +966,7 @@ static const cmdinfo_t write_cmd = {
     .cfunc      = write_f,
     .argmin     = 2,
     .argmax     = -1,
-    .args       = "[-bcCpqz] [-P pattern ] off len",
+    .args       = "[-bcCqz] [-P pattern ] off len",
     .oneline    = "writes a number of bytes at a specified offset",
     .help       = write_help,
 };
@@ -1015,7 +974,7 @@ static const cmdinfo_t write_cmd = {
 static int write_f(BlockBackend *blk, int argc, char **argv)
 {
     struct timeval t1, t2;
-    int Cflag = 0, pflag = 0, qflag = 0, bflag = 0, Pflag = 0, zflag = 0;
+    int Cflag = 0, qflag = 0, bflag = 0, Pflag = 0, zflag = 0;
     int cflag = 0;
     int c, cnt;
     char *buf = NULL;
@@ -1037,7 +996,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
             Cflag = 1;
             break;
         case 'p':
-            pflag = 1;
+            /* Ignored for back-compat */
             break;
         case 'P':
             Pflag = 1;
@@ -1061,8 +1020,8 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
         return qemuio_command_usage(&write_cmd);
     }

-    if (bflag + pflag + zflag > 1) {
-        printf("-b, -p, or -z cannot be specified at the same time\n");
+    if (bflag + zflag > 1) {
+        printf("-b and -z cannot be specified at the same time\n");
         return 0;
     }

@@ -1088,7 +1047,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
         return 0;
     }

-    if (!pflag) {
+    if (bflag || cflag || zflag) {
         if (offset & 0x1ff) {
             printf("offset %" PRId64 " is not sector aligned\n",
                    offset);
@@ -1107,16 +1066,14 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
     }

     gettimeofday(&t1, NULL);
-    if (pflag) {
-        cnt = do_pwrite(blk, buf, offset, count, &total);
-    } else if (bflag) {
+    if (bflag) {
         cnt = do_save_vmstate(blk, buf, offset, count, &total);
     } else if (zflag) {
         cnt = do_co_write_zeroes(blk, offset, count, &total);
     } else if (cflag) {
         cnt = do_write_compressed(blk, buf, offset, count, &total);
     } else {
-        cnt = do_write(blk, buf, offset, count, &total);
+        cnt = do_pwrite(blk, buf, offset, count, &total);
     }
     gettimeofday(&t2, NULL);

-- 
2.5.5

[Qemu-devel] [PATCH v4 12/14] block: Switch blk_read_unthrottled() to byte interface

[Eric Blake]

Sector-based blk_read() should die; convert the one-off
variant blk_read_unthrottled().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/sysemu/block-backend.h | 4 ++--
 block/block-backend.c          | 8 ++++----
 hw/block/hd-geometry.c         | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 6991b26..662a106 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -92,8 +92,8 @@ void *blk_get_attached_dev(BlockBackend *blk);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
 int blk_read(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
              int nb_sectors);
-int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
-                         int nb_sectors);
+int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
+                          int count);
 int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
               int nb_sectors);
 int blk_write_zeroes(BlockBackend *blk, int64_t sector_num,
diff --git a/block/block-backend.c b/block/block-backend.c
index 96c1d7c..e5a8a07 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -790,19 +790,19 @@ int blk_read(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
     return blk_rw(blk, sector_num, buf, nb_sectors, blk_read_entry, 0);
 }

-int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
-                         int nb_sectors)
+int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
+                          int count)
 {
     BlockDriverState *bs = blk_bs(blk);
     int ret;

-    ret = blk_check_request(blk, sector_num, nb_sectors);
+    ret = blk_check_byte_request(blk, offset, count);
     if (ret < 0) {
         return ret;
     }

     bdrv_no_throttling_begin(bs);
-    ret = blk_read(blk, sector_num, buf, nb_sectors);
+    ret = blk_pread(blk, offset, buf, count);
     bdrv_no_throttling_end(bs);
     return ret;
 }
diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index 6d02192..d388f13 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -66,7 +66,7 @@ static int guess_disk_lchs(BlockBackend *blk,
      * but also in async I/O mode. So the I/O throttling function has to
      * be disabled temporarily here, not permanently.
      */
-    if (blk_read_unthrottled(blk, 0, buf, 1) < 0) {
+    if (blk_pread_unthrottled(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
         return -1;
     }
     /* test msdos magic */
-- 
2.5.5

[Qemu-devel] [PATCH v4 13/14] block: Switch blk_write_zeroes() to byte interface

[Eric Blake]

Sector-based blk_write() should die; convert the one-off
variant blk_write_zeroes().

Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Denis V. Lunev <den@openvz.org>
---
 include/sysemu/block-backend.h | 4 ++--
 block/block-backend.c          | 8 ++++----
 block/parallels.c              | 3 ++-
 qemu-img.c                     | 3 ++-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 662a106..1246699 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -96,8 +96,8 @@ int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
                           int count);
 int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
               int nb_sectors);
-int blk_write_zeroes(BlockBackend *blk, int64_t sector_num,
-                     int nb_sectors, BdrvRequestFlags flags);
+int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+                     int count, BdrvRequestFlags flags);
 BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
                                  int nb_sectors, BdrvRequestFlags flags,
                                  BlockCompletionFunc *cb, void *opaque);
diff --git a/block/block-backend.c b/block/block-backend.c
index e5a8a07..71133b2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -814,11 +814,11 @@ int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
                   blk_write_entry, 0);
 }

-int blk_write_zeroes(BlockBackend *blk, int64_t sector_num,
-                     int nb_sectors, BdrvRequestFlags flags)
+int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+                      int count, BdrvRequestFlags flags)
 {
-    return blk_rw(blk, sector_num, NULL, nb_sectors, blk_write_entry,
-                  flags | BDRV_REQ_ZERO_WRITE);
+    return blk_prw(blk, offset, NULL, count, blk_write_entry,
+                   flags | BDRV_REQ_ZERO_WRITE);
 }

 static void error_callback_bh(void *opaque)
diff --git a/block/parallels.c b/block/parallels.c
index 2d8bc87..95bfc32 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -516,7 +516,8 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
     if (ret < 0) {
         goto exit;
     }
-    ret = blk_write_zeroes(file, 1, bat_sectors - 1, 0);
+    ret = blk_pwrite_zeroes(file, BDRV_SECTOR_SIZE,
+                            (bat_sectors - 1) << BDRV_SECTOR_BITS, 0);
     if (ret < 0) {
         goto exit;
     }
diff --git a/qemu-img.c b/qemu-img.c
index c19f9d4..41df87d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1595,7 +1595,8 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
             if (s->has_zero_init) {
                 break;
             }
-            ret = blk_write_zeroes(s->target, sector_num, n, 0);
+            ret = blk_pwrite_zeroes(s->target, sector_num << BDRV_SECTOR_BITS,
+                                    n << BDRV_SECTOR_BITS, 0);
             if (ret < 0) {
                 return ret;
             }
-- 
2.5.5

[Qemu-devel] [PATCH v4 14/14] block: Kill blk_write(), blk_read()

[Eric Blake]

Now that there are no remaining clients, we can drop these
functions, to ensure that all future users get the byte-based
interfaces.  Sadly, there are still remaining sector-based
interfaces, such as blk_aio_writev; those will have to wait
for another day.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/sysemu/block-backend.h |  4 ----
 block/block-backend.c          | 25 -------------------------
 2 files changed, 29 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 1246699..bf04086 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -90,12 +90,8 @@ void blk_attach_dev_nofail(BlockBackend *blk, void *dev);
 void blk_detach_dev(BlockBackend *blk, void *dev);
 void *blk_get_attached_dev(BlockBackend *blk);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
-int blk_read(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
-             int nb_sectors);
 int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
                           int count);
-int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
-              int nb_sectors);
 int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                      int count, BdrvRequestFlags flags);
 BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
diff --git a/block/block-backend.c b/block/block-backend.c
index 71133b2..e6ded54 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -772,24 +772,6 @@ static int blk_prw(BlockBackend *blk, int64_t offset, uint8_t *buf,
     return rwco.ret;
 }

-static int blk_rw(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
-                  int nb_sectors, CoroutineEntry co_entry,
-                  BdrvRequestFlags flags)
-{
-    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
-        return -EINVAL;
-    }
-
-    return blk_prw(blk, sector_num << BDRV_SECTOR_BITS, buf,
-                   nb_sectors << BDRV_SECTOR_BITS, co_entry, flags);
-}
-
-int blk_read(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
-             int nb_sectors)
-{
-    return blk_rw(blk, sector_num, buf, nb_sectors, blk_read_entry, 0);
-}
-
 int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
                           int count)
 {
@@ -807,13 +789,6 @@ int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
     return ret;
 }

-int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
-              int nb_sectors)
-{
-    return blk_rw(blk, sector_num, (uint8_t*) buf, nb_sectors,
-                  blk_write_entry, 0);
-}
-
 int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                       int count, BdrvRequestFlags flags)
 {
-- 
2.5.5

Re: [Qemu-devel] [PATCH v4 13/14] block: Switch blk_write_zeroes() to byte interface

[Eric Blake]

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

On 04/29/2016 02:08 PM, Eric Blake wrote:
> Sector-based blk_write() should die; convert the one-off
> variant blk_write_zeroes().
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Acked-by: Denis V. Lunev <den@openvz.org>
> ---

> +++ b/include/sysemu/block-backend.h
> @@ -96,8 +96,8 @@ int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
>                            int count);
>  int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
>                int nb_sectors);
> -int blk_write_zeroes(BlockBackend *blk, int64_t sector_num,
> -                     int nb_sectors, BdrvRequestFlags flags);
> +int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
> +                     int count, BdrvRequestFlags flags);

Maintainer may want to fix indentation if I don't need to respin.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 00/14] block: kill sector-based blk_write/read

[Kevin Wolf]

Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
> 2.7 material, depends on Kevin's block-next:
> git://repo.or.cz/qemu/kevin.git block-next
> 
> Previously posted as part of a larger NBD series [1] (at v3, explaining
> why this is v4), but these are independent enough to make for easier
> review on their own, and is mostly orthogonal to Kevin's recent work
> to also kill sector interfaces from the driver.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03526.html
> 
> Also available as a tag at this location:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-block-v4

If you send a v5, can you please make sure to explicitly CC qemu-block?
This series ended up only partially and without cover letter in my mail
folder that I generally use for processing patches, which is useless if
you want me to merge this series through my tree.

I also think it's a good rule of thumb that at least one person (the one
who is supposed to merge the series) should be directly copied on the
whole series instead of just parts of it.

Kevin

> Changes since then:
> add R-b/Acks received so far
> rebase to Kevin's block-next branch
> patch 8: use new defines for legibility [jsnow]
> 
> 001/14:[----] [--] 'block: Allow BDRV_REQ_FUA through blk_pwrite()'
> 002/14:[----] [--] 'fdc: Switch to byte-based block access'
> 003/14:[----] [--] 'nand: Switch to byte-based block access'
> 004/14:[----] [--] 'onenand: Switch to byte-based block access'
> 005/14:[----] [--] 'pflash: Switch to byte-based block access'
> 006/14:[----] [--] 'sd: Switch to byte-based block access'
> 007/14:[----] [--] 'm25p80: Switch to byte-based block access'
> 008/14:[0019] [FC] 'atapi: Switch to byte-based block access'
> 009/14:[----] [--] 'nbd: Switch to byte-based block access'
> 010/14:[----] [--] 'qemu-img: Switch to byte-based block access'
> 011/14:[----] [--] 'qemu-io: Switch to byte-based block access'
> 012/14:[----] [-C] 'block: Switch blk_read_unthrottled() to byte interface'
> 013/14:[----] [--] 'block: Switch blk_write_zeroes() to byte interface'
> 014/14:[----] [--] 'block: Kill blk_write(), blk_read()'
> 
> Eric Blake (14):
>   block: Allow BDRV_REQ_FUA through blk_pwrite()
>   fdc: Switch to byte-based block access
>   nand: Switch to byte-based block access
>   onenand: Switch to byte-based block access
>   pflash: Switch to byte-based block access
>   sd: Switch to byte-based block access
>   m25p80: Switch to byte-based block access
>   atapi: Switch to byte-based block access
>   nbd: Switch to byte-based block access
>   qemu-img: Switch to byte-based block access
>   qemu-io: Switch to byte-based block access
>   block: Switch blk_read_unthrottled() to byte interface
>   block: Switch blk_write_zeroes() to byte interface
>   block: Kill blk_write(), blk_read()
> 
>  include/sysemu/block-backend.h | 15 ++++----
>  block/block-backend.c          | 47 +++++++-------------------
>  block/crypto.c                 |  2 +-
>  block/parallels.c              |  5 +--
>  block/qcow.c                   |  8 ++---
>  block/qcow2.c                  |  4 +--
>  block/qed.c                    |  6 ++--
>  block/sheepdog.c               |  2 +-
>  block/vdi.c                    |  4 +--
>  block/vhdx.c                   |  5 +--
>  block/vmdk.c                   | 10 +++---
>  block/vpc.c                    | 10 +++---
>  hw/block/fdc.c                 | 25 +++++++++-----
>  hw/block/hd-geometry.c         |  2 +-
>  hw/block/m25p80.c              |  3 +-
>  hw/block/nand.c                | 36 +++++++++++++-------
>  hw/block/onenand.c             | 36 ++++++++++++--------
>  hw/block/pflash_cfi01.c        | 12 +++----
>  hw/block/pflash_cfi02.c        | 12 +++----
>  hw/ide/atapi.c                 | 19 ++++++-----
>  hw/nvram/spapr_nvram.c         |  4 +--
>  hw/sd/sd.c                     | 46 ++-----------------------
>  nbd/server.c                   |  2 +-
>  qemu-img.c                     | 31 +++++++++++------
>  qemu-io-cmds.c                 | 77 ++++++++++--------------------------------
>  qemu-nbd.c                     | 11 +++---
>  26 files changed, 185 insertions(+), 249 deletions(-)
> 
> -- 
> 2.5.5
> 
> 

Re: [Qemu-devel] [PATCH v4 00/14] block: kill sector-based blk_write/read

[Eric Blake]

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

On 05/02/2016 01:27 AM, Kevin Wolf wrote:
> Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
>> 2.7 material, depends on Kevin's block-next:
>> git://repo.or.cz/qemu/kevin.git block-next
>>
>> Previously posted as part of a larger NBD series [1] (at v3, explaining
>> why this is v4), but these are independent enough to make for easier
>> review on their own, and is mostly orthogonal to Kevin's recent work
>> to also kill sector interfaces from the driver.
>>
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03526.html
>>
>> Also available as a tag at this location:
>> git fetch git://repo.or.cz/qemu/ericb.git nbd-block-v4
> 
> If you send a v5, can you please make sure to explicitly CC qemu-block?
> This series ended up only partially and without cover letter in my mail
> folder that I generally use for processing patches, which is useless if
> you want me to merge this series through my tree.

Sure, will do (don't know why qemu-block as a list is not auto-assigned
to all nbd/ files; maybe we need a MAINTAINERS update there.  But this
patch touched more than just those files)

> 
> I also think it's a good rule of thumb that at least one person (the one
> who is supposed to merge the series) should be directly copied on the
> whole series instead of just parts of it.

I'm guessing Max and you are the two best candidates, according to
get_maintainers.pl for include/block/block_int.h?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 04/14] onenand: Switch to byte-based block access

[Kevin Wolf]

Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
> Sector-based blk_write() should die; switch to byte-based
> blk_pwrite() instead.  Likewise for blk_read().
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> Not compile tested - I'm not sure what else I'd need in my environment
> to actually test this one.  I have:
> Fedora 23, dnf builddep qemu
> ./configure --enable-kvm --enable-system --disable-user --target-list=x86_64-softmmu,ppc64-softmmu --enable-debug
> ---
>  hw/block/onenand.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/block/onenand.c b/hw/block/onenand.c
> index 883f4b1..3d19b0c 100644
> --- a/hw/block/onenand.c
> +++ b/hw/block/onenand.c
> @@ -224,7 +224,8 @@ static void onenand_reset(OneNANDState *s, int cold)
>          /* Lock the whole flash */
>          memset(s->blockwp, ONEN_LOCK_LOCKED, s->blocks);
> 
> -        if (s->blk_cur && blk_read(s->blk_cur, 0, s->boot[0], 8) < 0) {
> +        if (s->blk_cur && blk_pread(s->blk_cur, 0, s->boot[0],
> +                                    8 << BDRV_SECTOR_BITS) < 0) {
>              hw_error("%s: Loading the BootRAM failed.\n", __func__);
>          }
>      }
> @@ -241,7 +242,8 @@ static inline int onenand_load_main(OneNANDState *s, int sec, int secn,
>                  void *dest)
>  {
>      if (s->blk_cur) {
> -        return blk_read(s->blk_cur, sec, dest, secn) < 0;
> +        return blk_pread(s->blk_cur, sec << BDRV_SECTOR_BITS, dest,
> +                         secn << BDRV_SECTOR_BITS) < 0;
>      } else if (sec + secn > s->secs_cur) {
>          return 1;
>      }
> @@ -257,19 +259,20 @@ static inline int onenand_prog_main(OneNANDState *s, int sec, int secn,
>      int result = 0;
> 
>      if (secn > 0) {
> -        uint32_t size = (uint32_t)secn * 512;
> +        uint32_t size = (uint32_t)secn << BDRV_SECTOR_BITS;
> +        int64_t offset = sec << BDRV_SECTOR_BITS;

I'm not completely happy with the types here.

First of all, why signed? More importantly, though, sec is an int. I'm
not sure if we should cast it to uint64_t before shifting (I'm unsure
because this device seems to supports only sizes that fit in a uint32_t
anyway), but if we don't, wouldn't it make things more obvious if offset
were a uint32_t, too?

And if we decide for casting, there are more places in this patch where
an int is shifted.

>          const uint8_t *sp = (const uint8_t *)src;
>          uint8_t *dp = 0;
>          if (s->blk_cur) {
>              dp = g_malloc(size);
> -            if (!dp || blk_read(s->blk_cur, sec, dp, secn) < 0) {
> +            if (!dp || blk_pread(s->blk_cur, offset, dp, size) < 0) {
>                  result = 1;
>              }
>          } else {
>              if (sec + secn > s->secs_cur) {
>                  result = 1;
>              } else {
> -                dp = (uint8_t *)s->current + (sec << 9);
> +                dp = (uint8_t *)s->current + offset;
>              }
>          }
>          if (!result) {
> @@ -278,7 +281,7 @@ static inline int onenand_prog_main(OneNANDState *s, int sec, int secn,
>                  dp[i] &= sp[i];
>              }
>              if (s->blk_cur) {
> -                result = blk_write(s->blk_cur, sec, dp, secn) < 0;
> +                result = blk_pwrite(s->blk_cur, offset, dp, size, 0) < 0;
>              }
>          }
>          if (dp && s->blk_cur) {
> @@ -295,7 +298,8 @@ static inline int onenand_load_spare(OneNANDState *s, int sec, int secn,
>      uint8_t buf[512];
> 
>      if (s->blk_cur) {
> -        if (blk_read(s->blk_cur, s->secs_cur + (sec >> 5), buf, 1) < 0) {
> +        int32_t offset = (s->secs_cur + (sec >> 5)) << BDRV_SECTOR_BITS;

Here you have 32 bits (though still signed). In any case, some
consistency couldn't hurt.

> +        if (blk_pread(s->blk_cur, offset, buf, BDRV_SECTOR_SIZE) < 0) {
>              return 1;
>          }
>          memcpy(dest, buf + ((sec & 31) << 4), secn << 4);
> @@ -304,7 +308,7 @@ static inline int onenand_load_spare(OneNANDState *s, int sec, int secn,
>      } else {
>          memcpy(dest, s->current + (s->secs_cur << 9) + (sec << 4), secn << 4);
>      }
> - 
> +
>      return 0;
>  }
> 
> @@ -315,10 +319,11 @@ static inline int onenand_prog_spare(OneNANDState *s, int sec, int secn,
>      if (secn > 0) {
>          const uint8_t *sp = (const uint8_t *)src;
>          uint8_t *dp = 0, *dpp = 0;
> +        uint64_t offset = (s->secs_cur + (sec >> 5)) << BDRV_SECTOR_BITS;

Oh, nice, we have an unsigned one, too! :-)

Kevin

Re: [Qemu-devel] [PATCH v4 03/14] nand: Switch to byte-based block access

[Kevin Wolf]

Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
> Sector-based blk_write() should die; switch to byte-based
> blk_pwrite() instead.  Likewise for blk_read().
> 
> This file is doing some complex computations to map various
> flash page sizes (256, 512, and 2048) atop generic uses of
> 512-byte sector operations.  Perhaps someone will want to tidy
> up the file for fewer gymnastics in managing addresses and
> offsets, and less wasteful visits of 256-byte pages, but it
> was out of scope for this series, where I just went with the
> mechanical conversion.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  hw/block/nand.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index 29c6596..2703ff4 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
> @@ -663,7 +663,8 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s)
>          sector = SECTOR(s->addr);
>          off = (s->addr & PAGE_MASK) + s->offset;
>          soff = SECTOR_OFFSET(s->addr);
> -        if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS) < 0) {
> +        if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> +                      PAGE_SECTORS << BDRV_SECTOR_BITS) < 0) {
>              printf("%s: read error in sector %" PRIu64 "\n", __func__, sector);
>              return;
>          }
> @@ -675,21 +676,24 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s)
>                              MIN(OOB_SIZE, off + s->iolen - PAGE_SIZE));
>          }
> 
> -        if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS) < 0) {
> +        if (blk_pwrite(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> +                       PAGE_SECTORS << BDRV_SECTOR_BITS, 0) < 0) {
>              printf("%s: write error in sector %" PRIu64 "\n", __func__, sector);
>          }
>      } else {
>          off = PAGE_START(s->addr) + (s->addr & PAGE_MASK) + s->offset;
>          sector = off >> 9;
>          soff = off & 0x1ff;
> -        if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
> +        if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> +                     (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
>              printf("%s: read error in sector %" PRIu64 "\n", __func__, sector);
>              return;
>          }
> 
>          mem_and(iobuf + soff, s->io, s->iolen);
> 
> -        if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
> +        if (blk_write(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> +                      (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS, 0) < 0) {

You forgot to actually change which function is called here.

Kevin

Re: [Qemu-devel] [PATCH v4 06/14] sd: Switch to byte-based block access

[Kevin Wolf]

Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
> Sector-based blk_write() should die; switch to byte-based
> blk_pwrite() instead.  Likewise for blk_read().
> 
> Greatly simplifies the code, now that we let the block layer
> take care of alignment and read-modify-write on our behalf :)
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  hw/sd/sd.c | 46 +++-------------------------------------------
>  1 file changed, 3 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index b66e5d2..3c2f2f1 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1577,57 +1577,17 @@ send_response:
> 
>  static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
>  {
> -    uint64_t end = addr + len;
> -
>      DPRINTF("sd_blk_read: addr = 0x%08llx, len = %d\n",
>              (unsigned long long) addr, len);
> -    if (!sd->blk || blk_read(sd->blk, addr >> 9, sd->buf, 1) < 0) {
> +    if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
>          fprintf(stderr, "sd_blk_read: read error on host side\n");
> -        return;
>      }
> -
> -    if (end > (addr & ~511) + 512) {
> -        memcpy(sd->data, sd->buf + (addr & 511), 512 - (addr & 511));
> -
> -        if (blk_read(sd->blk, end >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_read: read error on host side\n");
> -            return;
> -        }
> -        memcpy(sd->data + 512 - (addr & 511), sd->buf, end & 511);
> -    } else
> -        memcpy(sd->data, sd->buf + (addr & 511), len);
>  }

We can remove sd->buf from SDState, it should be unused now.

I'm not sure why a temporary buffer was ever included in the VMState,
but I guess we can make it a VMSTATE_UNUSED_BUFFER().

>  static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>  {
> -    uint64_t end = addr + len;
> -
> -    if ((addr & 511) || len < 512)
> -        if (!sd->blk || blk_read(sd->blk, addr >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: read error on host side\n");
> -            return;
> -        }
> -
> -    if (end > (addr & ~511) + 512) {
> -        memcpy(sd->buf + (addr & 511), sd->data, 512 - (addr & 511));
> -        if (blk_write(sd->blk, addr >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: write error on host side\n");
> -            return;
> -        }
> -
> -        if (blk_read(sd->blk, end >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: read error on host side\n");
> -            return;
> -        }
> -        memcpy(sd->buf, sd->data + 512 - (addr & 511), end & 511);
> -        if (blk_write(sd->blk, end >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: write error on host side\n");
> -        }
> -    } else {
> -        memcpy(sd->buf + (addr & 511), sd->data, len);
> -        if (!sd->blk || blk_write(sd->blk, addr >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: write error on host side\n");
> -        }
> +    if (!sd->blk || blk_pwrite(sd->blk, addr, sd->buf, len, 0) < 0) {

I said "should" instead of "is" above because this line is buggy.
sd->data should probably be used instead.

Kevin

Re: [Qemu-devel] [PATCH v4 03/14] nand: Switch to byte-based block access

[Eric Blake]

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

On 05/02/2016 09:35 AM, Kevin Wolf wrote:
> Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
>> Sector-based blk_write() should die; switch to byte-based
>> blk_pwrite() instead.  Likewise for blk_read().
>>
>> This file is doing some complex computations to map various
>> flash page sizes (256, 512, and 2048) atop generic uses of
>> 512-byte sector operations.  Perhaps someone will want to tidy
>> up the file for fewer gymnastics in managing addresses and
>> offsets, and less wasteful visits of 256-byte pages, but it
>> was out of scope for this series, where I just went with the
>> mechanical conversion.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  hw/block/nand.c | 36 +++++++++++++++++++++++-------------
>>  1 file changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/block/nand.c b/hw/block/nand.c
>> index 29c6596..2703ff4 100644
>> --- a/hw/block/nand.c

>>      } else {
>>          off = PAGE_START(s->addr) + (s->addr & PAGE_MASK) + s->offset;
>>          sector = off >> 9;
>>          soff = off & 0x1ff;
>> -        if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
>> +        if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
>> +                     (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {

Botched indentation here, too.

>>              printf("%s: read error in sector %" PRIu64 "\n", __func__, sector);
>>              return;
>>          }
>>
>>          mem_and(iobuf + soff, s->io, s->iolen);
>>
>> -        if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
>> +        if (blk_write(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
>> +                      (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS, 0) < 0) {
> 
> You forgot to actually change which function is called here.

I _did_ warn that some of my patches were not compile tested (wonder why
I forgot to warn on this one, when I did warn on 4/14).  Of course,
patch 14/14 would have caused a compile error if I had been compiling
this. So, what exactly do I need to start compiling these files, and
ensure I'm not doing dead code edits?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 04/14] onenand: Switch to byte-based block access

[Eric Blake]

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

On 05/02/2016 09:35 AM, Kevin Wolf wrote:
> Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
>> Sector-based blk_write() should die; switch to byte-based
>> blk_pwrite() instead.  Likewise for blk_read().
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> Not compile tested - I'm not sure what else I'd need in my environment
>> to actually test this one.  I have:
>> Fedora 23, dnf builddep qemu
>> ./configure --enable-kvm --enable-system --disable-user --target-list=x86_64-softmmu,ppc64-softmmu --enable-debug
>> ---
>>  hw/block/onenand.c | 36 ++++++++++++++++++++++--------------
>>  1 file changed, 22 insertions(+), 14 deletions(-)
>>

>> @@ -257,19 +259,20 @@ static inline int onenand_prog_main(OneNANDState *s, int sec, int secn,
>>      int result = 0;
>>
>>      if (secn > 0) {
>> -        uint32_t size = (uint32_t)secn * 512;
>> +        uint32_t size = (uint32_t)secn << BDRV_SECTOR_BITS;
>> +        int64_t offset = sec << BDRV_SECTOR_BITS;
> 
> I'm not completely happy with the types here.
> 
> First of all, why signed? More importantly, though, sec is an int. I'm
> not sure if we should cast it to uint64_t before shifting (I'm unsure
> because this device seems to supports only sizes that fit in a uint32_t
> anyway), but if we don't, wouldn't it make things more obvious if offset
> were a uint32_t, too?

Hmm. I guess sec can't be negative, but I didn't check whether sec can
ever be greater than 0x7fffffff/BDRV_SECTOR_SIZE.  Depending on that
answer determines whether the shift here overflows - but you are right
that if it CAN overflow 32 bits, then we MUST cast sec to a 64-bit type
PRIOR to the shift, not just merely assign it to a 64-bit value; and if
CAN'T overflow, then a 32-bit type is sufficient to hold the answer.
You're also right that unsigned is nicer in general for sizes that
shouldn't be negative.

> 
> And if we decide for casting, there are more places in this patch where
> an int is shifted.

Good catch.  I guess I have to audit things more closely before
respinning the series.

>> @@ -295,7 +298,8 @@ static inline int onenand_load_spare(OneNANDState *s, int sec, int secn,
>>      uint8_t buf[512];
>>
>>      if (s->blk_cur) {
>> -        if (blk_read(s->blk_cur, s->secs_cur + (sec >> 5), buf, 1) < 0) {
>> +        int32_t offset = (s->secs_cur + (sec >> 5)) << BDRV_SECTOR_BITS;
> 
> Here you have 32 bits (though still signed). In any case, some
> consistency couldn't hurt.

That's certainly true, no matter what type I ultimately pick.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 04/14] onenand: Switch to byte-based block access

[Eric Blake]

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

On 05/02/2016 03:16 PM, Eric Blake wrote:
>> First of all, why signed? More importantly, though, sec is an int. I'm
>> not sure if we should cast it to uint64_t before shifting (I'm unsure
>> because this device seems to supports only sizes that fit in a uint32_t
>> anyway), but if we don't, wouldn't it make things more obvious if offset
>> were a uint32_t, too?
> 
> Hmm. I guess sec can't be negative, but I didn't check whether sec can
> ever be greater than 0x7fffffff/BDRV_SECTOR_SIZE.  Depending on that
> answer determines whether the shift here overflows - but you are right
> that if it CAN overflow 32 bits, then we MUST cast sec to a 64-bit type
> PRIOR to the shift, not just merely assign it to a 64-bit value; and if
> CAN'T overflow, then a 32-bit type is sufficient to hold the answer.
> You're also right that unsigned is nicer in general for sizes that
> shouldn't be negative.

Okay, auditing wasn't as hard as I feared:

static int onenand_initfn(SysBusDevice *sbd)
...
    uint32_t size = 1 << (24 + ((s->id.dev >> 4) & 7));
...
    s->blocks = size >> BLOCK_SHIFT;
    s->secs = size >> 9;

so the maximum sec should ever be is 0x80000000 >> 9. I'll stick with
uint32_t (since that's what the init function used), and maybe add an
assert that we aren't overflowing.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 03/14] nand: Switch to byte-based block access

[Kevin Wolf]

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

Am 02.05.2016 um 23:09 hat Eric Blake geschrieben:
> On 05/02/2016 09:35 AM, Kevin Wolf wrote:
> > Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
> >> Sector-based blk_write() should die; switch to byte-based
> >> blk_pwrite() instead.  Likewise for blk_read().
> >>
> >> This file is doing some complex computations to map various
> >> flash page sizes (256, 512, and 2048) atop generic uses of
> >> 512-byte sector operations.  Perhaps someone will want to tidy
> >> up the file for fewer gymnastics in managing addresses and
> >> offsets, and less wasteful visits of 256-byte pages, but it
> >> was out of scope for this series, where I just went with the
> >> mechanical conversion.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >> ---
> >>  hw/block/nand.c | 36 +++++++++++++++++++++++-------------
> >>  1 file changed, 23 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/block/nand.c b/hw/block/nand.c
> >> index 29c6596..2703ff4 100644
> >> --- a/hw/block/nand.c
> 
> >>      } else {
> >>          off = PAGE_START(s->addr) + (s->addr & PAGE_MASK) + s->offset;
> >>          sector = off >> 9;
> >>          soff = off & 0x1ff;
> >> -        if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
> >> +        if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> >> +                     (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
> 
> Botched indentation here, too.
> 
> >>              printf("%s: read error in sector %" PRIu64 "\n", __func__, sector);
> >>              return;
> >>          }
> >>
> >>          mem_and(iobuf + soff, s->io, s->iolen);
> >>
> >> -        if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
> >> +        if (blk_write(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> >> +                      (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS, 0) < 0) {
> > 
> > You forgot to actually change which function is called here.
> 
> I _did_ warn that some of my patches were not compile tested (wonder why
> I forgot to warn on this one, when I did warn on 4/14).  Of course,
> patch 14/14 would have caused a compile error if I had been compiling
> this. So, what exactly do I need to start compiling these files, and
> ensure I'm not doing dead code edits?

I just did a full compile (i.e. no --target-list), that covered it. This
is the only thing I saw, though maybe I missed a warning because somehow
-Werror didn't seem to be in effect. I did the test build with clang,
maybe configure behaves different there.

Kevin

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