[Qemu-devel] [PATCH v2 0/4] block: improve luks driver perf & switch to byte APIs

[Qemu-devel] [PATCH v2 0/4] block: improve luks driver perf & switch to byte APIs

[Daniel P. Berrange]

This series includes a previously posted patch that improves performance
of the luks crypto driver:

  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00781.html

And then adds three patches that switch over to use byte based APIs for
I/O, rather than the legacy sector based APIs.

Daniel P. Berrange (4):
  block: use 1 MB bounce buffers for crypto instead of 16KB
  block: use BDRV_SECTOR_SIZE in crypto driver
  block: convert crypto driver to bdrv_co_preadv|pwritev
  block: convert qcrypto_block_encrypt|decrypt to take bytes offset

 block/crypto.c         | 119 +++++++++++++++++++++++++------------------------
 block/qcow.c           |   7 ++-
 block/qcow2-cluster.c  |   8 ++--
 block/qcow2.c          |   4 +-
 crypto/block-luks.c    |  12 +++--
 crypto/block-qcow.c    |  12 +++--
 crypto/block.c         |  14 +++---
 crypto/blockpriv.h     |   4 +-
 include/crypto/block.h |  14 +++---
 9 files changed, 104 insertions(+), 90 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v2 1/4] block: use 1 MB bounce buffers for crypto instead of 16KB

[Daniel P. Berrange]

Using 16KB bounce buffers creates a significant performance
penalty for I/O to encrypted volumes on storage which high
I/O latency (rotating rust & network drives), because it
triggers lots of fairly small I/O operations.

On tests with rotating rust, and cache=none|directsync,
write speed increased from 2MiB/s to 32MiB/s, on a par
with that achieved by the in-kernel luks driver. With
other cache modes the in-kernel driver is still notably
faster because it is able to report completion of the
I/O request before any encryption is done, while the
in-QEMU driver must encrypt the data before completion.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/crypto.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 58ef6f2f52..cc8afe0e0d 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -379,7 +379,7 @@ static void block_crypto_close(BlockDriverState *bs)
 }
 
 
-#define BLOCK_CRYPTO_MAX_SECTORS 32
+#define BLOCK_CRYPTO_MAX_SECTORS 2048
 
 static coroutine_fn int
 block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
@@ -396,9 +396,8 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
-    /* Bounce buffer so we have a linear mem region for
-     * entire sector. XXX optimize so we avoid bounce
-     * buffer in case that qiov->niov == 1
+    /* Bounce buffer because we don't wish to expose cipher text
+     * in qiov which points to guest memory.
      */
     cipher_data =
         qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
@@ -464,9 +463,8 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
-    /* Bounce buffer so we have a linear mem region for
-     * entire sector. XXX optimize so we avoid bounce
-     * buffer in case that qiov->niov == 1
+    /* Bounce buffer because we're not permitted to touch
+     * contents of qiov - it points to guest memory.
      */
     cipher_data =
         qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
-- 
2.13.5

[Qemu-devel] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

[Daniel P. Berrange]

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/crypto.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index cc8afe0e0d..e63f094379 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -392,16 +392,16 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
     QEMUIOVector hd_qiov;
     int ret = 0;
     size_t payload_offset =
-        qcrypto_block_get_payload_offset(crypto->block) / 512;
+        qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
     /* Bounce buffer because we don't wish to expose cipher text
      * in qiov which points to guest memory.
      */
-    cipher_data =
-        qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
-                                              qiov->size));
+    cipher_data = qemu_try_blockalign(
+        bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * BDRV_SECTOR_SIZE,
+                          qiov->size));
     if (cipher_data == NULL) {
         ret = -ENOMEM;
         goto cleanup;
@@ -415,7 +415,7 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
         }
 
         qemu_iovec_reset(&hd_qiov);
-        qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512);
+        qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
 
         ret = bdrv_co_readv(bs->file,
                             payload_offset + sector_num,
@@ -426,18 +426,18 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
 
         if (qcrypto_block_decrypt(crypto->block,
                                   sector_num,
-                                  cipher_data, cur_nr_sectors * 512,
+                                  cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE,
                                   NULL) < 0) {
             ret = -EIO;
             goto cleanup;
         }
 
         qemu_iovec_from_buf(qiov, bytes_done,
-                            cipher_data, cur_nr_sectors * 512);
+                            cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
 
         remaining_sectors -= cur_nr_sectors;
         sector_num += cur_nr_sectors;
-        bytes_done += cur_nr_sectors * 512;
+        bytes_done += cur_nr_sectors * BDRV_SECTOR_SIZE;
     }
 
  cleanup:
@@ -459,16 +459,16 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
     QEMUIOVector hd_qiov;
     int ret = 0;
     size_t payload_offset =
-        qcrypto_block_get_payload_offset(crypto->block) / 512;
+        qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
     /* Bounce buffer because we're not permitted to touch
      * contents of qiov - it points to guest memory.
      */
-    cipher_data =
-        qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
-                                              qiov->size));
+    cipher_data = qemu_try_blockalign(
+        bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * BDRV_SECTOR_SIZE,
+                          qiov->size));
     if (cipher_data == NULL) {
         ret = -ENOMEM;
         goto cleanup;
@@ -482,18 +482,18 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
         }
 
         qemu_iovec_to_buf(qiov, bytes_done,
-                          cipher_data, cur_nr_sectors * 512);
+                          cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
 
         if (qcrypto_block_encrypt(crypto->block,
                                   sector_num,
-                                  cipher_data, cur_nr_sectors * 512,
+                                  cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE,
                                   NULL) < 0) {
             ret = -EIO;
             goto cleanup;
         }
 
         qemu_iovec_reset(&hd_qiov);
-        qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512);
+        qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
 
         ret = bdrv_co_writev(bs->file,
                              payload_offset + sector_num,
@@ -504,7 +504,7 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
 
         remaining_sectors -= cur_nr_sectors;
         sector_num += cur_nr_sectors;
-        bytes_done += cur_nr_sectors * 512;
+        bytes_done += cur_nr_sectors * BDRV_SECTOR_SIZE;
     }
 
  cleanup:
-- 
2.13.5

[Qemu-devel] [PATCH v2 3/4] block: convert crypto driver to bdrv_co_preadv|pwritev

[Daniel P. Berrange]

Make the crypto driver implement the bdrv_co_preadv|pwritev
callbacks,  and also use bdrv_co_preadv|pwritev for I/O
with the protocol driver beneath.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/crypto.c | 103 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 53 insertions(+), 50 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index e63f094379..40daa77188 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -380,19 +380,23 @@ static void block_crypto_close(BlockDriverState *bs)
 
 
 #define BLOCK_CRYPTO_MAX_SECTORS 2048
+#define BLOCK_CRYPTO_MAX_LENGTH (BLOCK_CRYPTO_MAX_SECTORS * BDRV_SECTOR_SIZE)
 
 static coroutine_fn int
-block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
-                      int remaining_sectors, QEMUIOVector *qiov)
+block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+                       QEMUIOVector *qiov, int flags)
 {
     BlockCrypto *crypto = bs->opaque;
-    int cur_nr_sectors; /* number of sectors in current iteration */
+    uint64_t cur_bytes; /* number of bytes in current iteration */
     uint64_t bytes_done = 0;
     uint8_t *cipher_data = NULL;
     QEMUIOVector hd_qiov;
     int ret = 0;
-    size_t payload_offset =
-        qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
+    size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
+    uint64_t sector_num = offset / BDRV_SECTOR_SIZE;
+
+    assert((offset % BDRV_SECTOR_SIZE) == 0);
+    assert((bytes % BDRV_SECTOR_SIZE) == 0);
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
@@ -400,44 +404,39 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
      * in qiov which points to guest memory.
      */
     cipher_data = qemu_try_blockalign(
-        bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * BDRV_SECTOR_SIZE,
-                          qiov->size));
+        bs->file->bs, MIN(BLOCK_CRYPTO_MAX_LENGTH, qiov->size));
     if (cipher_data == NULL) {
         ret = -ENOMEM;
         goto cleanup;
     }
 
-    while (remaining_sectors) {
-        cur_nr_sectors = remaining_sectors;
+    while (bytes) {
+        cur_bytes = bytes;
 
-        if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {
-            cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;
+        if (cur_bytes > BLOCK_CRYPTO_MAX_LENGTH) {
+            cur_bytes = BLOCK_CRYPTO_MAX_LENGTH;
         }
 
         qemu_iovec_reset(&hd_qiov);
-        qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
+        qemu_iovec_add(&hd_qiov, cipher_data, cur_bytes);
 
-        ret = bdrv_co_readv(bs->file,
-                            payload_offset + sector_num,
-                            cur_nr_sectors, &hd_qiov);
+        ret = bdrv_co_preadv(bs->file, payload_offset + offset + bytes_done,
+                             cur_bytes, &hd_qiov, 0);
         if (ret < 0) {
             goto cleanup;
         }
 
-        if (qcrypto_block_decrypt(crypto->block,
-                                  sector_num,
-                                  cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE,
-                                  NULL) < 0) {
+        if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
+                                  cur_bytes, NULL) < 0) {
             ret = -EIO;
             goto cleanup;
         }
 
-        qemu_iovec_from_buf(qiov, bytes_done,
-                            cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
+        qemu_iovec_from_buf(qiov, bytes_done, cipher_data, cur_bytes);
 
-        remaining_sectors -= cur_nr_sectors;
-        sector_num += cur_nr_sectors;
-        bytes_done += cur_nr_sectors * BDRV_SECTOR_SIZE;
+        sector_num += cur_bytes / BDRV_SECTOR_SIZE;
+        bytes -= cur_bytes;
+        bytes_done += cur_bytes;
     }
 
  cleanup:
@@ -449,17 +448,20 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
 
 
 static coroutine_fn int
-block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
-                       int remaining_sectors, QEMUIOVector *qiov)
+block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+                        QEMUIOVector *qiov, int flags)
 {
     BlockCrypto *crypto = bs->opaque;
-    int cur_nr_sectors; /* number of sectors in current iteration */
+    uint64_t cur_bytes; /* number of bytes in current iteration */
     uint64_t bytes_done = 0;
     uint8_t *cipher_data = NULL;
     QEMUIOVector hd_qiov;
     int ret = 0;
-    size_t payload_offset =
-        qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
+    size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
+    uint64_t sector_num = offset / BDRV_SECTOR_SIZE;
+
+    assert((offset % BDRV_SECTOR_SIZE) == 0);
+    assert((bytes % BDRV_SECTOR_SIZE) == 0);
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
@@ -467,44 +469,39 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
      * contents of qiov - it points to guest memory.
      */
     cipher_data = qemu_try_blockalign(
-        bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * BDRV_SECTOR_SIZE,
-                          qiov->size));
+        bs->file->bs, MIN(BLOCK_CRYPTO_MAX_LENGTH, qiov->size));
     if (cipher_data == NULL) {
         ret = -ENOMEM;
         goto cleanup;
     }
 
-    while (remaining_sectors) {
-        cur_nr_sectors = remaining_sectors;
+    while (bytes) {
+        cur_bytes = bytes;
 
-        if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {
-            cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;
+        if (cur_bytes > BLOCK_CRYPTO_MAX_LENGTH) {
+            cur_bytes = BLOCK_CRYPTO_MAX_LENGTH;
         }
 
-        qemu_iovec_to_buf(qiov, bytes_done,
-                          cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
+        qemu_iovec_to_buf(qiov, bytes_done, cipher_data, cur_bytes);
 
-        if (qcrypto_block_encrypt(crypto->block,
-                                  sector_num,
-                                  cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE,
-                                  NULL) < 0) {
+        if (qcrypto_block_encrypt(crypto->block, sector_num, cipher_data,
+                                  cur_bytes, NULL) < 0) {
             ret = -EIO;
             goto cleanup;
         }
 
         qemu_iovec_reset(&hd_qiov);
-        qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
+        qemu_iovec_add(&hd_qiov, cipher_data, cur_bytes);
 
-        ret = bdrv_co_writev(bs->file,
-                             payload_offset + sector_num,
-                             cur_nr_sectors, &hd_qiov);
+        ret = bdrv_co_pwritev(bs->file, payload_offset + offset + bytes_done,
+                              cur_bytes, &hd_qiov, 0);
         if (ret < 0) {
             goto cleanup;
         }
 
-        remaining_sectors -= cur_nr_sectors;
-        sector_num += cur_nr_sectors;
-        bytes_done += cur_nr_sectors * BDRV_SECTOR_SIZE;
+        sector_num += cur_bytes / BDRV_SECTOR_SIZE;
+        bytes -= cur_bytes;
+        bytes_done += cur_bytes;
     }
 
  cleanup:
@@ -514,6 +511,11 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }
 
+static void block_crypto_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    bs->bl.request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O */
+}
+
 
 static int64_t block_crypto_getlength(BlockDriverState *bs)
 {
@@ -611,8 +613,9 @@ BlockDriver bdrv_crypto_luks = {
     .bdrv_truncate      = block_crypto_truncate,
     .create_opts        = &block_crypto_create_opts_luks,
 
-    .bdrv_co_readv      = block_crypto_co_readv,
-    .bdrv_co_writev     = block_crypto_co_writev,
+    .bdrv_refresh_limits = block_crypto_refresh_limits,
+    .bdrv_co_preadv      = block_crypto_co_preadv,
+    .bdrv_co_pwritev     = block_crypto_co_pwritev,
     .bdrv_getlength     = block_crypto_getlength,
     .bdrv_get_info      = block_crypto_get_info_luks,
     .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
-- 
2.13.5

[Qemu-devel] [PATCH v2 4/4] block: convert qcrypto_block_encrypt|decrypt to take bytes offset

[Daniel P. Berrange]

Instead of sector offset, take the bytes offset when encrypting
or decrypting data.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/crypto.c         |  8 ++++----
 block/qcow.c           |  7 +++++--
 block/qcow2-cluster.c  |  8 +++-----
 block/qcow2.c          |  4 ++--
 crypto/block-luks.c    | 12 ++++++++----
 crypto/block-qcow.c    | 12 ++++++++----
 crypto/block.c         | 14 ++++++++------
 crypto/blockpriv.h     |  4 ++--
 include/crypto/block.h | 14 ++++++++------
 9 files changed, 48 insertions(+), 35 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 40daa77188..6b8d88efbc 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
             goto cleanup;
         }
 
-        if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
-                                  cur_bytes, NULL) < 0) {
+        if (qcrypto_block_decrypt(crypto->block, offset + bytes_done,
+                                  cipher_data, cur_bytes, NULL) < 0) {
             ret = -EIO;
             goto cleanup;
         }
@@ -484,8 +484,8 @@ block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
 
         qemu_iovec_to_buf(qiov, bytes_done, cipher_data, cur_bytes);
 
-        if (qcrypto_block_encrypt(crypto->block, sector_num, cipher_data,
-                                  cur_bytes, NULL) < 0) {
+        if (qcrypto_block_encrypt(crypto->block, offset + bytes_done,
+                                  cipher_data, cur_bytes, NULL) < 0) {
             ret = -EIO;
             goto cleanup;
         }
diff --git a/block/qcow.c b/block/qcow.c
index c08cdc4a7b..c2a446e824 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
                         if (i < n_start || i >= n_end) {
                             Error *err = NULL;
                             memset(s->cluster_data, 0x00, 512);
-                            if (qcrypto_block_encrypt(s->crypto, start_sect + i,
+                            if (qcrypto_block_encrypt(s->crypto,
+                                                      start_sect + i *
+                                                      BDRV_SECTOR_SIZE,
                                                       s->cluster_data,
                                                       BDRV_SECTOR_SIZE,
                                                       &err) < 0) {
@@ -636,7 +638,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
             }
             if (bs->encrypted) {
                 assert(s->crypto);
-                if (qcrypto_block_decrypt(s->crypto, sector_num, buf,
+                if (qcrypto_block_decrypt(s->crypto,
+                                          sector_num * BDRV_SECTOR_SIZE, buf,
                                           n * BDRV_SECTOR_SIZE, &err) < 0) {
                     goto fail;
                 }
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f06c08f64c..85ffc33c2c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -396,15 +396,13 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
 {
     if (bytes && bs->encrypted) {
         BDRVQcow2State *s = bs->opaque;
-        int64_t sector = (s->crypt_physical_offset ?
+        int64_t offset = (s->crypt_physical_offset ?
                           (cluster_offset + offset_in_cluster) :
-                          (src_cluster_offset + offset_in_cluster))
-                         >> BDRV_SECTOR_BITS;
+                          (src_cluster_offset + offset_in_cluster));
         assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
         assert((bytes & ~BDRV_SECTOR_MASK) == 0);
         assert(s->crypto);
-        if (qcrypto_block_encrypt(s->crypto, sector, buffer,
-                                  bytes, NULL) < 0) {
+        if (qcrypto_block_encrypt(s->crypto, offset, buffer, bytes, NULL) < 0) {
             return false;
         }
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 40ba26c111..1c9f6c8f7d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1824,7 +1824,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
                 if (qcrypto_block_decrypt(s->crypto,
                                           (s->crypt_physical_offset ?
                                            cluster_offset + offset_in_cluster :
-                                           offset) >> BDRV_SECTOR_BITS,
+                                           offset),
                                           cluster_data,
                                           cur_bytes,
                                           &err) < 0) {
@@ -1961,7 +1961,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
             if (qcrypto_block_encrypt(s->crypto,
                                       (s->crypt_physical_offset ?
                                        cluster_offset + offset_in_cluster :
-                                       offset) >> BDRV_SECTOR_BITS,
+                                       offset),
                                       cluster_data,
                                       cur_bytes, &err) < 0) {
                 error_free(err);
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index afb8543108..672b0986e0 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1403,29 +1403,33 @@ static void qcrypto_block_luks_cleanup(QCryptoBlock *block)
 
 static int
 qcrypto_block_luks_decrypt(QCryptoBlock *block,
-                           uint64_t startsector,
+                           uint64_t offset,
                            uint8_t *buf,
                            size_t len,
                            Error **errp)
 {
+    assert(!(offset % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
+    assert(!(len % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
     return qcrypto_block_decrypt_helper(block->cipher,
                                         block->niv, block->ivgen,
                                         QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
-                                        startsector, buf, len, errp);
+                                        offset, buf, len, errp);
 }
 
 
 static int
 qcrypto_block_luks_encrypt(QCryptoBlock *block,
-                           uint64_t startsector,
+                           uint64_t offset,
                            uint8_t *buf,
                            size_t len,
                            Error **errp)
 {
+    assert(!(offset % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
+    assert(!(len % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
     return qcrypto_block_encrypt_helper(block->cipher,
                                         block->niv, block->ivgen,
                                         QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
-                                        startsector, buf, len, errp);
+                                        offset, buf, len, errp);
 }
 
 
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index a456fe338b..9d2efa27d6 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -142,29 +142,33 @@ qcrypto_block_qcow_cleanup(QCryptoBlock *block)
 
 static int
 qcrypto_block_qcow_decrypt(QCryptoBlock *block,
-                           uint64_t startsector,
+                           uint64_t offset,
                            uint8_t *buf,
                            size_t len,
                            Error **errp)
 {
+    assert(!(offset % QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
+    assert(!(len % QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
     return qcrypto_block_decrypt_helper(block->cipher,
                                         block->niv, block->ivgen,
                                         QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
-                                        startsector, buf, len, errp);
+                                        offset, buf, len, errp);
 }
 
 
 static int
 qcrypto_block_qcow_encrypt(QCryptoBlock *block,
-                           uint64_t startsector,
+                           uint64_t offset,
                            uint8_t *buf,
                            size_t len,
                            Error **errp)
 {
+    assert(!(offset % QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
+    assert(!(len % QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
     return qcrypto_block_encrypt_helper(block->cipher,
                                         block->niv, block->ivgen,
                                         QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
-                                        startsector, buf, len, errp);
+                                        offset, buf, len, errp);
 }
 
 
diff --git a/crypto/block.c b/crypto/block.c
index b097d451af..84ef5d870a 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -127,22 +127,22 @@ QCryptoBlockInfo *qcrypto_block_get_info(QCryptoBlock *block,
 
 
 int qcrypto_block_decrypt(QCryptoBlock *block,
-                          uint64_t startsector,
+                          uint64_t offset,
                           uint8_t *buf,
                           size_t len,
                           Error **errp)
 {
-    return block->driver->decrypt(block, startsector, buf, len, errp);
+    return block->driver->decrypt(block, offset, buf, len, errp);
 }
 
 
 int qcrypto_block_encrypt(QCryptoBlock *block,
-                          uint64_t startsector,
+                          uint64_t offset,
                           uint8_t *buf,
                           size_t len,
                           Error **errp)
 {
-    return block->driver->encrypt(block, startsector, buf, len, errp);
+    return block->driver->encrypt(block, offset, buf, len, errp);
 }
 
 
@@ -188,13 +188,14 @@ int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
                                  size_t niv,
                                  QCryptoIVGen *ivgen,
                                  int sectorsize,
-                                 uint64_t startsector,
+                                 uint64_t offset,
                                  uint8_t *buf,
                                  size_t len,
                                  Error **errp)
 {
     uint8_t *iv;
     int ret = -1;
+    uint64_t startsector = offset / sectorsize;
 
     iv = niv ? g_new0(uint8_t, niv) : NULL;
 
@@ -237,13 +238,14 @@ int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
                                  size_t niv,
                                  QCryptoIVGen *ivgen,
                                  int sectorsize,
-                                 uint64_t startsector,
+                                 uint64_t offset,
                                  uint8_t *buf,
                                  size_t len,
                                  Error **errp)
 {
     uint8_t *iv;
     int ret = -1;
+    uint64_t startsector = offset / sectorsize;
 
     iv = niv ? g_new0(uint8_t, niv) : NULL;
 
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 0edb810e22..53e66e58fb 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -81,7 +81,7 @@ int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
                                  size_t niv,
                                  QCryptoIVGen *ivgen,
                                  int sectorsize,
-                                 uint64_t startsector,
+                                 uint64_t offset,
                                  uint8_t *buf,
                                  size_t len,
                                  Error **errp);
@@ -90,7 +90,7 @@ int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
                                  size_t niv,
                                  QCryptoIVGen *ivgen,
                                  int sectorsize,
-                                 uint64_t startsector,
+                                 uint64_t offset,
                                  uint8_t *buf,
                                  size_t len,
                                  Error **errp);
diff --git a/include/crypto/block.h b/include/crypto/block.h
index f0e543bee1..2dea165a8a 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -161,18 +161,19 @@ QCryptoBlockInfo *qcrypto_block_get_info(QCryptoBlock *block,
 /**
  * @qcrypto_block_decrypt:
  * @block: the block encryption object
- * @startsector: the sector from which @buf was read
+ * @offset: the position at which @iov was read
  * @buf: the buffer to decrypt
  * @len: the length of @buf in bytes
  * @errp: pointer to a NULL-initialized error object
  *
  * Decrypt @len bytes of cipher text in @buf, writing
- * plain text back into @buf
+ * plain text back into @buf. @len and @offset must be
+ * a multiple of the encryption format sector size.
  *
  * Returns 0 on success, -1 on failure
  */
 int qcrypto_block_decrypt(QCryptoBlock *block,
-                          uint64_t startsector,
+                          uint64_t offset,
                           uint8_t *buf,
                           size_t len,
                           Error **errp);
@@ -180,18 +181,19 @@ int qcrypto_block_decrypt(QCryptoBlock *block,
 /**
  * @qcrypto_block_encrypt:
  * @block: the block encryption object
- * @startsector: the sector to which @buf will be written
+ * @offset: the position at which @iov will be written
  * @buf: the buffer to decrypt
  * @len: the length of @buf in bytes
  * @errp: pointer to a NULL-initialized error object
  *
  * Encrypt @len bytes of plain text in @buf, writing
- * cipher text back into @buf
+ * cipher text back into @buf. @len and @offset must be
+ * a multiple of the encryption format sector size.
  *
  * Returns 0 on success, -1 on failure
  */
 int qcrypto_block_encrypt(QCryptoBlock *block,
-                          uint64_t startsector,
+                          uint64_t offset,
                           uint8_t *buf,
                           size_t len,
                           Error **errp);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

[Eric Blake]

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

On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/crypto.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 3/4] block: convert crypto driver to bdrv_co_preadv|pwritev

[Eric Blake]

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

On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> Make the crypto driver implement the bdrv_co_preadv|pwritev
> callbacks,  and also use bdrv_co_preadv|pwritev for I/O
> with the protocol driver beneath.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/crypto.c | 103 +++++++++++++++++++++++++++++----------------------------
>  1 file changed, 53 insertions(+), 50 deletions(-)
> 

>  static coroutine_fn int
> -block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> -                      int remaining_sectors, QEMUIOVector *qiov)
> +block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> +                       QEMUIOVector *qiov, int flags)

>  {
>      BlockCrypto *crypto = bs->opaque;
> -    int cur_nr_sectors; /* number of sectors in current iteration */
> +    uint64_t cur_bytes; /* number of bytes in current iteration */
>      uint64_t bytes_done = 0;
>      uint8_t *cipher_data = NULL;
>      QEMUIOVector hd_qiov;
>      int ret = 0;
> -    size_t payload_offset =
> -        qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
> +    size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);

Pre-existing: is size_t the right type, or can we overflow a 64-bit
offset on a 32-bit host?

> +    uint64_t sector_num = offset / BDRV_SECTOR_SIZE;
> +
> +    assert((offset % BDRV_SECTOR_SIZE) == 0);
> +    assert((bytes % BDRV_SECTOR_SIZE) == 0);

The osdep.h macros might be nicer than open-coding; furthermore, if
desired, you could shorten to:

assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));


>  
>  static coroutine_fn int
> -block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
> -                       int remaining_sectors, QEMUIOVector *qiov)
> +block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> +                        QEMUIOVector *qiov, int flags)
>  {

Hmm - you don't set supported_write_flags.  But presumably, if the
underlying BDS supports BDRV_REQUEST_FUA, then crypto can likewise
support that flag by passing it through to the underlying device after
encryption.

>      BlockCrypto *crypto = bs->opaque;
> -    int cur_nr_sectors; /* number of sectors in current iteration */
> +    uint64_t cur_bytes; /* number of bytes in current iteration */
>      uint64_t bytes_done = 0;
>      uint8_t *cipher_data = NULL;
>      QEMUIOVector hd_qiov;
>      int ret = 0;
> -    size_t payload_offset =
> -        qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
> +    size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
> +    uint64_t sector_num = offset / BDRV_SECTOR_SIZE;
> +
> +    assert((offset % BDRV_SECTOR_SIZE) == 0);
> +    assert((bytes % BDRV_SECTOR_SIZE) == 0);

Same comment as for read.

> @@ -611,8 +613,9 @@ BlockDriver bdrv_crypto_luks = {
>      .bdrv_truncate      = block_crypto_truncate,
>      .create_opts        = &block_crypto_create_opts_luks,
>  
> -    .bdrv_co_readv      = block_crypto_co_readv,
> -    .bdrv_co_writev     = block_crypto_co_writev,
> +    .bdrv_refresh_limits = block_crypto_refresh_limits,
> +    .bdrv_co_preadv      = block_crypto_co_preadv,
> +    .bdrv_co_pwritev     = block_crypto_co_pwritev,
>      .bdrv_getlength     = block_crypto_getlength,
>      .bdrv_get_info      = block_crypto_get_info_luks,
>      .bdrv_get_specific_info = block_crypto_get_specific_info_luks,

Looks weird when = isn't consistently aligned, but we use more than one
space.  My preference is to just use one space everywhere, as adding a
longer name to the list doesn't require a mass re-format of all other
entries; but I'm not opposed when people like the aligned = for
legibility.  So up to you if you do anything in response to my nit.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 4/4] block: convert qcrypto_block_encrypt|decrypt to take bytes offset

[Eric Blake]

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

On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> Instead of sector offset, take the bytes offset when encrypting
> or decrypting data.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/crypto.c         |  8 ++++----
>  block/qcow.c           |  7 +++++--
>  block/qcow2-cluster.c  |  8 +++-----
>  block/qcow2.c          |  4 ++--
>  crypto/block-luks.c    | 12 ++++++++----
>  crypto/block-qcow.c    | 12 ++++++++----
>  crypto/block.c         | 14 ++++++++------
>  crypto/blockpriv.h     |  4 ++--
>  include/crypto/block.h | 14 ++++++++------
>  9 files changed, 48 insertions(+), 35 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 40daa77188..6b8d88efbc 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
>              goto cleanup;
>          }
>  
> -        if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
> -                                  cur_bytes, NULL) < 0) {
> +        if (qcrypto_block_decrypt(crypto->block, offset + bytes_done,
> +                                  cipher_data, cur_bytes, NULL) < 0) {

How close are we to getting rid of even needing 'sector_num' as a variable?

> +++ b/block/qcow.c
> @@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>                          if (i < n_start || i >= n_end) {
>                              Error *err = NULL;
>                              memset(s->cluster_data, 0x00, 512);
> -                            if (qcrypto_block_encrypt(s->crypto, start_sect + i,
> +                            if (qcrypto_block_encrypt(s->crypto,
> +                                                      start_sect + i *
> +                                                      BDRV_SECTOR_SIZE,

Umm, not the same.  You want (start_sect + i) * BDRV_SECTOR_SIZE.

> +++ b/block/qcow2-cluster.c
> @@ -396,15 +396,13 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>  {
>      if (bytes && bs->encrypted) {
>          BDRVQcow2State *s = bs->opaque;
> -        int64_t sector = (s->crypt_physical_offset ?
> +        int64_t offset = (s->crypt_physical_offset ?
>                            (cluster_offset + offset_in_cluster) :
> -                          (src_cluster_offset + offset_in_cluster))
> -                         >> BDRV_SECTOR_BITS;
> +                          (src_cluster_offset + offset_in_cluster));
>          assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>          assert((bytes & ~BDRV_SECTOR_MASK) == 0);

Pre-existing, but we could use osdep.h macros here, as in QEMU_IS_ALIGNED().

> +++ b/crypto/block-luks.c
> @@ -1403,29 +1403,33 @@ static void qcrypto_block_luks_cleanup(QCryptoBlock *block)
>  
>  static int
>  qcrypto_block_luks_decrypt(QCryptoBlock *block,
> -                           uint64_t startsector,
> +                           uint64_t offset,
>                             uint8_t *buf,
>                             size_t len,
>                             Error **errp)
>  {
> +    assert(!(offset % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
> +    assert(!(len % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));

Again, QEMU_IS_ALIGNED() might be easier to read - but this time, it's
in code you're adding.

Looking forward to v3.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 4/4] block: convert qcrypto_block_encrypt|decrypt to take bytes offset

[Daniel P. Berrange]

On Thu, Aug 31, 2017 at 10:17:43AM -0500, Eric Blake wrote:
> On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> > Instead of sector offset, take the bytes offset when encrypting
> > or decrypting data.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  block/crypto.c         |  8 ++++----
> >  block/qcow.c           |  7 +++++--
> >  block/qcow2-cluster.c  |  8 +++-----
> >  block/qcow2.c          |  4 ++--
> >  crypto/block-luks.c    | 12 ++++++++----
> >  crypto/block-qcow.c    | 12 ++++++++----
> >  crypto/block.c         | 14 ++++++++------
> >  crypto/blockpriv.h     |  4 ++--
> >  include/crypto/block.h | 14 ++++++++------
> >  9 files changed, 48 insertions(+), 35 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 40daa77188..6b8d88efbc 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> >              goto cleanup;
> >          }
> >  
> > -        if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
> > -                                  cur_bytes, NULL) < 0) {
> > +        if (qcrypto_block_decrypt(crypto->block, offset + bytes_done,
> > +                                  cipher_data, cur_bytes, NULL) < 0) {
> 
> How close are we to getting rid of even needing 'sector_num' as a variable?

I thought there was more usage, but I just realized we can in fact
remove it in this patch.

> 
> > +++ b/block/qcow.c
> > @@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
> >                          if (i < n_start || i >= n_end) {
> >                              Error *err = NULL;
> >                              memset(s->cluster_data, 0x00, 512);
> > -                            if (qcrypto_block_encrypt(s->crypto, start_sect + i,
> > +                            if (qcrypto_block_encrypt(s->crypto,
> > +                                                      start_sect + i *
> > +                                                      BDRV_SECTOR_SIZE,
> 
> Umm, not the same.  You want (start_sect + i) * BDRV_SECTOR_SIZE.

Heh, oppps.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

[Kevin Wolf]

Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/crypto.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)

I'm actually not sure about this one. Anything that is left after
patch 3 is probably not the arbitrary unit that qemu uses internally
for some interfaces, but the unit in which data is encrypted.

Basically, if just for fun we ever changed the unit of things like
bdrv_write() from 512 to 4096, then everything that needs to or at least
can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
needs to stay on 512 (like I suspect most of the occurrences in the
crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).

Kevin

Re: [Qemu-devel] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

[Daniel P. Berrange]

On Tue, Sep 05, 2017 at 11:52:15AM +0200, Kevin Wolf wrote:
> Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  block/crypto.c | 32 ++++++++++++++++----------------
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> I'm actually not sure about this one. Anything that is left after
> patch 3 is probably not the arbitrary unit that qemu uses internally
> for some interfaces, but the unit in which data is encrypted.
> 
> Basically, if just for fun we ever changed the unit of things like
> bdrv_write() from 512 to 4096, then everything that needs to or at least
> can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
> needs to stay on 512 (like I suspect most of the occurrences in the
> crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).

Yeah for sure LUKSv1 & legacy qcow2 encryption need to stay 512
forever, so if BDRV_SECTOR_SIZE were to change that would be a
problem.

I wrote this on the assumption that we would never change BDRV_SECTOR_SIZE
though. If we did need different sector sizes in the block layer I figured
it would surely end up being a dynamic property per disk, rather than just
changing the compile time constant. So from that POV I thought it ok to
use BDRV_SECTOR_SIZE.

Perhaps I should instead add a qcrypto_block_get_sector_size() API though
and use that, so we can fetch the sector size per encryption scheme in
case we ever get a scheme using a non-512 sector size for encryption.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

[Kevin Wolf]

Am 05.09.2017 um 12:05 hat Daniel P. Berrange geschrieben:
> On Tue, Sep 05, 2017 at 11:52:15AM +0200, Kevin Wolf wrote:
> > Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > ---
> > >  block/crypto.c | 32 ++++++++++++++++----------------
> > >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > I'm actually not sure about this one. Anything that is left after
> > patch 3 is probably not the arbitrary unit that qemu uses internally
> > for some interfaces, but the unit in which data is encrypted.
> > 
> > Basically, if just for fun we ever changed the unit of things like
> > bdrv_write() from 512 to 4096, then everything that needs to or at least
> > can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
> > needs to stay on 512 (like I suspect most of the occurrences in the
> > crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).
> 
> Yeah for sure LUKSv1 & legacy qcow2 encryption need to stay 512
> forever, so if BDRV_SECTOR_SIZE were to change that would be a
> problem.
> 
> I wrote this on the assumption that we would never change BDRV_SECTOR_SIZE
> though. If we did need different sector sizes in the block layer I figured
> it would surely end up being a dynamic property per disk, rather than just
> changing the compile time constant. So from that POV I thought it ok to
> use BDRV_SECTOR_SIZE.

Yes, I agree that we'll never BDRV_SECTOR_SIZE in practice. I just use
this as a way to check whether this is really BDRV_SECTOR_SIZE, or
whether we have two different quantities that just happen to be both
512.

For example, QDICT_BUCKET_MAX is also 512, but that doesn't make it
right to use here, even though it works and is unlikely to change.

What we may actually want to do at some point (won't be in the near
future, though) is removing BDRV_SECTOR_SIZE because it refers to an
arbitrary unit in APIs and we're in the process of making everything
byte-based instead. This is another way to check whether some value is
BDRV_SECTOR_SIZE: If it will remain even after removing APIs with
sector-based parameters, it's probably something different.

> Perhaps I should instead add a qcrypto_block_get_sector_size() API though
> and use that, so we can fetch the sector size per encryption scheme in
> case we ever get a scheme using a non-512 sector size for encryption.

If you have a use for it, sure. Otherwise, I'd just suggest introducing
another constant for now.

Kevin

Re: [Qemu-devel] [PATCH v2 3/4] block: convert crypto driver to bdrv_co_preadv|pwritev

[Daniel P. Berrange]

On Thu, Aug 31, 2017 at 10:08:00AM -0500, Eric Blake wrote:
> On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> > Make the crypto driver implement the bdrv_co_preadv|pwritev
> > callbacks,  and also use bdrv_co_preadv|pwritev for I/O
> > with the protocol driver beneath.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  block/crypto.c | 103 +++++++++++++++++++++++++++++----------------------------
> >  1 file changed, 53 insertions(+), 50 deletions(-)
> > 
> 
> >  static coroutine_fn int
> > -block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> > -                      int remaining_sectors, QEMUIOVector *qiov)
> > +block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> > +                       QEMUIOVector *qiov, int flags)
> 
> >  {
> >      BlockCrypto *crypto = bs->opaque;
> > -    int cur_nr_sectors; /* number of sectors in current iteration */
> > +    uint64_t cur_bytes; /* number of bytes in current iteration */
> >      uint64_t bytes_done = 0;
> >      uint8_t *cipher_data = NULL;
> >      QEMUIOVector hd_qiov;
> >      int ret = 0;
> > -    size_t payload_offset =
> > -        qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
> > +    size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
> 
> Pre-existing: is size_t the right type, or can we overflow a 64-bit
> offset on a 32-bit host?

No, it is bad. I'm fixing that as a separate patch, since it is a good
to cleanup.

> 
> > +    uint64_t sector_num = offset / BDRV_SECTOR_SIZE;
> > +
> > +    assert((offset % BDRV_SECTOR_SIZE) == 0);
> > +    assert((bytes % BDRV_SECTOR_SIZE) == 0);
> 
> The osdep.h macros might be nicer than open-coding; furthermore, if
> desired, you could shorten to:
> 
> assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));

Yep, makes sense.

> >  static coroutine_fn int
> > -block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
> > -                       int remaining_sectors, QEMUIOVector *qiov)
> > +block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> > +                        QEMUIOVector *qiov, int flags)
> >  {
> 
> Hmm - you don't set supported_write_flags.  But presumably, if the
> underlying BDS supports BDRV_REQUEST_FUA, then crypto can likewise
> support that flag by passing it through to the underlying device after
> encryption.

Something to be added as a separate patch

> > @@ -611,8 +613,9 @@ BlockDriver bdrv_crypto_luks = {
> >      .bdrv_truncate      = block_crypto_truncate,
> >      .create_opts        = &block_crypto_create_opts_luks,
> >  
> > -    .bdrv_co_readv      = block_crypto_co_readv,
> > -    .bdrv_co_writev     = block_crypto_co_writev,
> > +    .bdrv_refresh_limits = block_crypto_refresh_limits,
> > +    .bdrv_co_preadv      = block_crypto_co_preadv,
> > +    .bdrv_co_pwritev     = block_crypto_co_pwritev,
> >      .bdrv_getlength     = block_crypto_getlength,
> >      .bdrv_get_info      = block_crypto_get_info_luks,
> >      .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
> 
> Looks weird when = isn't consistently aligned, but we use more than one
> space.  My preference is to just use one space everywhere, as adding a
> longer name to the list doesn't require a mass re-format of all other
> entries; but I'm not opposed when people like the aligned = for
> legibility.  So up to you if you do anything in response to my nit.

Normally I stick with one space, so don't know what I was thinking
when i wrote this.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|