* Change the qcow2_co_{encrypt|decrypt} to just receive full host and
  guest offsets and use this function directly instead of calling
  do_perform_cow_encrypt (which is removed by that patch).

* Adjust qcow2_co_encdec to take full host and guest offsets as well.

* Document the qcow2_co_{encrypt|decrypt} arguments
  to prevent the bug fixed in former commit from hopefully
  happening again.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/qcow2-cluster.c | 41 ++++++++++------------------
 block/qcow2-threads.c | 63 +++++++++++++++++++++++++++++++++----------
 block/qcow2.c         |  5 ++--
 block/qcow2.h         |  8 +++---
 4 files changed, 70 insertions(+), 47 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index bfeb0241d7..a2d4909024 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -462,28 +462,6 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
     return 0;
 }
 
-static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
-                                                uint64_t src_cluster_offset,
-                                                uint64_t cluster_offset,
-                                                unsigned offset_in_cluster,
-                                                uint8_t *buffer,
-                                                unsigned bytes)
-{
-    if (bytes && bs->encrypted) {
-        BDRVQcow2State *s = bs->opaque;
-        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
-        assert((bytes & ~BDRV_SECTOR_MASK) == 0);
-        assert(s->crypto);
-        if (qcow2_co_encrypt(bs,
-                start_of_cluster(s, cluster_offset + offset_in_cluster),
-                src_cluster_offset + offset_in_cluster,
-                buffer, bytes) < 0) {
-            return false;
-        }
-    }
-    return true;
-}
-
 static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
                                              uint64_t cluster_offset,
                                              unsigned offset_in_cluster,
@@ -891,11 +869,20 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
 
     /* Encrypt the data if necessary before writing it */
     if (bs->encrypted) {
-        if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
-                                    start->offset, start_buffer,
-                                    start->nb_bytes) ||
-            !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
-                                    end->offset, end_buffer, end->nb_bytes)) {
+        ret = qcow2_co_encrypt(bs,
+                               m->alloc_offset + start->offset,
+                               m->offset + start->offset,
+                               start_buffer, start->nb_bytes);
+        if (ret < 0) {
+            ret = -EIO;
+            goto fail;
+        }
+
+        ret = qcow2_co_encrypt(bs,
+                               m->alloc_offset + end->offset,
+                               m->offset + end->offset,
+                               end_buffer, end->nb_bytes);
+        if (ret < 0) {
             ret = -EIO;
             goto fail;
         }
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3b1e63fe41..8f5a0d1ebe 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -234,35 +234,70 @@ static int qcow2_encdec_pool_func(void *opaque)
 }
 
 static int coroutine_fn
-qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
-                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
+qcow2_co_encdec(BlockDriverState *bs, uint64_t host_offset,
+                uint64_t guest_offset, void *buf, size_t len,
+                Qcow2EncDecFunc func)
 {
     BDRVQcow2State *s = bs->opaque;
     Qcow2EncDecData arg = {
         .block = s->crypto,
-        .offset = s->crypt_physical_offset ?
-                      file_cluster_offset + offset_into_cluster(s, offset) :
-                      offset,
+        .offset = s->crypt_physical_offset ? host_offset : guest_offset,
         .buf = buf,
         .len = len,
         .func = func,
     };
 
-    return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
+    assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE));
+    assert(s->crypto);
+
+    return len == 0 ? 0 : qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
 }
 
+/*
+ * qcow2_co_encrypt()
+ *
+ * Encrypts one or more contiguous aligned sectors
+ *
+ * @host_offset - underlying storage offset of the first sector of the
+ * data to be encrypted
+ *
+ * @guest_offset - guest (virtual) offset of the first sector of the
+ * data to be encrypted
+ *
+ * @buf - buffer with the data to encrypt, that after encryption
+ *        will be written to the underlying storage device at
+ *        @host_offset
+ *
+ * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
+ *
+ * Depending on the encryption method, @host_offset and/or @guest_offset
+ * may be used for generating the initialization vector for
+ * encryption.
+ *
+ * Note that while the whole range must be aligned on sectors, it
+ * does not have to be aligned on clusters and can also cross cluster
+ * boundaries
+ */
 int coroutine_fn
-qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len)
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len)
 {
-    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
-                             qcrypto_block_encrypt);
+    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
+                           qcrypto_block_encrypt);
 }
 
+/*
+ * qcow2_co_decrypt()
+ *
+ * Decrypts one or more contiguous aligned sectors
+ * Similar to qcow2_co_encrypt
+ */
 int coroutine_fn
-qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len)
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len)
 {
-    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
-                             qcrypto_block_decrypt);
+    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
+                           qcrypto_block_decrypt);
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 57734f20cf..ac768092bb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2069,7 +2069,8 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
 
                 assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
                 assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
-                if (qcow2_co_decrypt(bs, cluster_offset, offset,
+                if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
+                                     offset,
                                      cluster_data, cur_bytes) < 0) {
                     ret = -EIO;
                     goto fail;
@@ -2288,7 +2289,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
             qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
                               cluster_data, cur_bytes);
 
-            if (qcow2_co_encrypt(bs, cluster_offset, offset,
+            if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
                                  cluster_data, cur_bytes) < 0) {
                 ret = -EIO;
                 goto out_unlocked;
diff --git a/block/qcow2.h b/block/qcow2.h
index 998bcdaef1..a488d761ff 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -758,10 +758,10 @@ ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
                     const void *src, size_t src_size);
 int coroutine_fn
-qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len);
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len);
 int coroutine_fn
-qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len);
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len);
 
 #endif
-- 
2.17.2

On 15.09.19 22:36, Maxim Levitsky wrote:
> * Change the qcow2_co_{encrypt|decrypt} to just receive full host and
>   guest offsets and use this function directly instead of calling
>   do_perform_cow_encrypt (which is removed by that patch).
> 
> * Adjust qcow2_co_encdec to take full host and guest offsets as well.
> 
> * Document the qcow2_co_{encrypt|decrypt} arguments
>   to prevent the bug fixed in former commit from hopefully
>   happening again.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/qcow2-cluster.c | 41 ++++++++++------------------
>  block/qcow2-threads.c | 63 +++++++++++++++++++++++++++++++++----------
>  block/qcow2.c         |  5 ++--
>  block/qcow2.h         |  8 +++---
>  4 files changed, 70 insertions(+), 47 deletions(-)

Looks good to me, but there are conflicts with two series I’ve already
taken to my branch: Namely “qcow2: async handling of fragmented io” by
Vladimir and “Alignment checks cleanup​” by Nir.

Unfortunately, those conflicts (while not difficult to resolve) are not
quite trivial, so I’d rather not resolve them myself.

I’ll send a pull request today, but until they are in master, you could
rebase on my branch (either of
- https://git.xanclic.moe/XanClic/qemu.git block
- https://github.com/XanClic/qemu.git block
, whichever you prefer that works).

Max

15.09.2019 23:36, Maxim Levitsky wrote:
> * Change the qcow2_co_{encrypt|decrypt} to just receive full host and
>    guest offsets and use this function directly instead of calling
>    do_perform_cow_encrypt (which is removed by that patch).
> 
> * Adjust qcow2_co_encdec to take full host and guest offsets as well.
> 
> * Document the qcow2_co_{encrypt|decrypt} arguments
>    to prevent the bug fixed in former commit from hopefully
>    happening again.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>   block/qcow2-cluster.c | 41 ++++++++++------------------
>   block/qcow2-threads.c | 63 +++++++++++++++++++++++++++++++++----------
>   block/qcow2.c         |  5 ++--
>   block/qcow2.h         |  8 +++---
>   4 files changed, 70 insertions(+), 47 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index bfeb0241d7..a2d4909024 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c

[..]

>   static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
>                                                uint64_t cluster_offset,
>                                                unsigned offset_in_cluster,
> @@ -891,11 +869,20 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
>   
>       /* Encrypt the data if necessary before writing it */
>       if (bs->encrypted) {
> -        if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -                                    start->offset, start_buffer,
> -                                    start->nb_bytes) ||
> -            !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -                                    end->offset, end_buffer, end->nb_bytes)) {
> +        ret = qcow2_co_encrypt(bs,
> +                               m->alloc_offset + start->offset,
> +                               m->offset + start->offset,
> +                               start_buffer, start->nb_bytes);
> +        if (ret < 0) {
> +            ret = -EIO;
> +            goto fail;

Just go to fail I think, don't reassign ret variable.

> +        }
> +
> +        ret = qcow2_co_encrypt(bs,
> +                               m->alloc_offset + end->offset,
> +                               m->offset + end->offset,
> +                               end_buffer, end->nb_bytes);
> +        if (ret < 0) {
>               ret = -EIO;

and here.

with these two places fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

(I think, these simple changes may be applied while queuing, so you don't
need to resend, if there no other reasons)




-- 
Best regards,
Vladimir