Re: [Qemu-block] [PATCH] nvme: Add support for Read Data and Write Data in CMBs.

[Keith Busch]

On Tue, Jun 13, 2017 at 04:08:35AM -0600, sba...@raithlin.com wrote:
> From: Stephen Bates 
> 
> Add the ability for the NVMe model to support both the RDS and WDS
> modes in the Controller Memory Buffer.
> 
> Although not currently supported in the upstreamed Linux kernel a fork
> with support exists [1] and user-space test programs that build on
> this also exist [2].
> 
> Useful for testing CMB functionality in preperation for real CMB
> enabled NVMe devices (coming soon).
> 
> [1] https://github.com/sbates130272/linux-p2pmem
> [2] https://github.com/sbates130272/p2pmem-test
> 
> Signed-off-by: Stephen Bates 
> Reviewed-by: Logan Gunthorpe 

This looks good to me.

Reviewed-by: Keith Busch 

[Qemu-block] [PATCH v9 17/20] block: remove all encryption handling APIs

[Daniel P. Berrange]

Now that all encryption keys must be provided upfront via
the QCryptoSecret API and associated block driver properties
there is no need for any explicit encryption handling APIs
in the block layer. Encryption can be handled transparently
within the block driver. We only retain an API for querying
whether an image is encrypted or not, since that is a
potentially useful piece of metadata to report to the user.

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 block.c   | 77 +--
 block/crypto.c|  1 -
 block/qapi.c  |  2 +-
 block/qcow.c  |  8 -
 block/qcow2.c |  1 -
 blockdev.c| 37 ++-
 hmp-commands.hx   |  2 ++
 include/block/block.h |  3 --
 include/block/block_int.h |  1 -
 include/qapi/error.h  |  1 -
 qapi/block-core.json  | 37 ++-
 qapi/common.json  |  5 +--
 12 files changed, 16 insertions(+), 159 deletions(-)

diff --git a/block.c b/block.c
index fa1d06d..440649c 100644
--- a/block.c
+++ b/block.c
@@ -2569,15 +2569,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 goto close_and_fail;
 }
 
-if (!bdrv_key_required(bs)) {
-bdrv_parent_cb_change_media(bs, true);
-} else if (!runstate_check(RUN_STATE_PRELAUNCH)
-   && !runstate_check(RUN_STATE_INMIGRATE)
-   && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
-error_setg(errp,
-   "Guest must be stopped for opening of encrypted image");
-goto close_and_fail;
-}
+bdrv_parent_cb_change_media(bs, true);
 
 QDECREF(options);
 
@@ -3068,7 +3060,6 @@ static void bdrv_close(BlockDriverState *bs)
 bs->backing_format[0] = '\0';
 bs->total_sectors = 0;
 bs->encrypted = false;
-bs->valid_key = false;
 bs->sg = false;
 QDECREF(bs->options);
 QDECREF(bs->explicit_options);
@@ -3498,72 +3489,6 @@ bool bdrv_is_encrypted(BlockDriverState *bs)
 return bs->encrypted;
 }
 
-bool bdrv_key_required(BlockDriverState *bs)
-{
-BdrvChild *backing = bs->backing;
-
-if (backing && backing->bs->encrypted && !backing->bs->valid_key) {
-return true;
-}
-return (bs->encrypted && !bs->valid_key);
-}
-
-int bdrv_set_key(BlockDriverState *bs, const char *key)
-{
-int ret;
-if (bs->backing && bs->backing->bs->encrypted) {
-ret = bdrv_set_key(bs->backing->bs, key);
-if (ret < 0)
-return ret;
-if (!bs->encrypted)
-return 0;
-}
-if (!bs->encrypted) {
-return -EINVAL;
-} else if (!bs->drv || !bs->drv->bdrv_set_key) {
-return -ENOMEDIUM;
-}
-ret = bs->drv->bdrv_set_key(bs, key);
-if (ret < 0) {
-bs->valid_key = false;
-} else if (!bs->valid_key) {
-/* call the change callback now, we skipped it on open */
-bs->valid_key = true;
-bdrv_parent_cb_change_media(bs, true);
-}
-return ret;
-}
-
-/*
- * Provide an encryption key for @bs.
- * If @key is non-null:
- * If @bs is not encrypted, fail.
- * Else if the key is invalid, fail.
- * Else set @bs's key to @key, replacing the existing key, if any.
- * If @key is null:
- * If @bs is encrypted and still lacks a key, fail.
- * Else do nothing.
- * On failure, store an error object through @errp if non-null.
- */
-void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
-{
-if (key) {
-if (!bdrv_is_encrypted(bs)) {
-error_setg(errp, "Node '%s' is not encrypted",
-  bdrv_get_device_or_node_name(bs));
-} else if (bdrv_set_key(bs, key) < 0) {
-error_setg(errp, QERR_INVALID_PASSWORD);
-}
-} else {
-if (bdrv_key_required(bs)) {
-error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
-  "'%s' (%s) is encrypted",
-  bdrv_get_device_or_node_name(bs),
-  bdrv_get_encrypted_filename(bs));
-}
-}
-}
-
 const char *bdrv_get_format_name(BlockDriverState *bs)
 {
 return bs->drv ? bs->drv->format_name : NULL;
diff --git a/block/crypto.c b/block/crypto.c
index da4be74..3ad4b20 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -308,7 +308,6 @@ static int block_crypto_open_generic(QCryptoBlockFormat 
format,
 }
 
 bs->encrypted = true;
-bs->valid_key = true;
 
 ret = 0;
  cleanup:
diff --git a/block/qapi.c b/block/qapi.c
index a40922e..9d724c2 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -45,7 +45,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 info->ro = bs->read_only;
 info->drv= g_strdup(bs->drv->format_name);
 info->encrypted  = bs->encrypted;
-   

[Qemu-block] [PATCH v9 19/20] qcow2: report encryption specific image information

[Daniel P. Berrange]

Currently 'qemu-img info' reports a simple "encrypted: yes"
field. This is not very useful now that qcow2 can support
multiple encryption formats. Users want to know which format
is in use and some data related to it.

Wire up usage of the qcrypto_block_get_info() method so that
'qemu-img info' can report about the encryption format
and parameters in use

  $ qemu-img create \
  --object secret,id=sec0,data=123456 \
  -o encrypt.format=luks,encrypt.key-secret=sec0 \
  -f qcow2 demo.qcow2 1G
  Formatting 'demo.qcow2', fmt=qcow2 size=1073741824 \
  encryption=off encrypt.format=luks encrypt.key-secret=sec0 \
  cluster_size=65536 lazy_refcounts=off refcount_bits=16

  $ qemu-img info demo.qcow2
  image: demo.qcow2
  file format: qcow2
  virtual size: 1.0G (1073741824 bytes)
  disk size: 480K
  encrypted: yes
  cluster_size: 65536
  Format specific information:
  compat: 1.1
  lazy refcounts: false
  refcount bits: 16
  encrypt:
  ivgen alg: plain64
  hash alg: sha256
  cipher alg: aes-256
  uuid: 3fa930c4-58c8-4ef7-b3c5-314bb5af21f3
  format: luks
  cipher mode: xts
  slots:
  [0]:
  active: true
  iters: 1839058
  key offset: 4096
  stripes: 4000
  [1]:
  active: false
  key offset: 262144
  [2]:
  active: false
  key offset: 520192
  [3]:
  active: false
  key offset: 778240
  [4]:
  active: false
  key offset: 1036288
  [5]:
  active: false
  key offset: 1294336
  [6]:
  active: false
  key offset: 1552384
  [7]:
  active: false
  key offset: 1810432
  payload offset: 2068480
  master key iters: 438487
  corrupt: false

With the legacy "AES" encryption we just report the format
name

  $ qemu-img create \
  --object secret,id=sec0,data=123456 \
  -o encrypt.format=aes,encrypt.key-secret=sec0 \
  -f qcow2 demo.qcow2 1G
  Formatting 'demo.qcow2', fmt=qcow2 size=1073741824 \
  encryption=off encrypt.format=aes encrypt.key-secret=sec0 \
  cluster_size=65536 lazy_refcounts=off refcount_bits=16

  $ ./qemu-img info demo.qcow2
  image: demo.qcow2
  file format: qcow2
  virtual size: 1.0G (1073741824 bytes)
  disk size: 196K
  encrypted: yes
  cluster_size: 65536
  Format specific information:
  compat: 1.1
  lazy refcounts: false
  refcount bits: 16
  encrypt:
  format: aes
  corrupt: false

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 block/qcow2.c| 32 +++-
 qapi/block-core.json | 27 ++-
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 19bc69c..a9d0b55 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3199,8 +3199,14 @@ static int qcow2_get_info(BlockDriverState *bs, 
BlockDriverInfo *bdi)
 static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
-ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
+ImageInfoSpecific *spec_info;
+QCryptoBlockInfo *encrypt_info = NULL;
 
+if (s->crypto != NULL) {
+encrypt_info = qcrypto_block_get_info(s->crypto, _abort);
+}
+
+spec_info = g_new(ImageInfoSpecific, 1);
 *spec_info = (ImageInfoSpecific){
 .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
 .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1),
@@ -3227,6 +3233,30 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
 assert(false);
 }
 
+if (encrypt_info) {
+ImageInfoSpecificQCow2Encryption *qencrypt =
+g_new(ImageInfoSpecificQCow2Encryption, 1);
+switch (encrypt_info->format) {
+case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_AES;
+qencrypt->u.aes = encrypt_info->u.qcow;
+break;
+case Q_CRYPTO_BLOCK_FORMAT_LUKS:
+qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_LUKS;
+qencrypt->u.luks = encrypt_info->u.luks;
+break;
+default:
+abort();
+}
+/* Since we did shallow copy above, erase any pointers
+ * in the original info */
+memset(_info->u, 0, sizeof(encrypt_info->u));
+qapi_free_QCryptoBlockInfo(encrypt_info);
+
+spec_info->u.qcow2.data->has_encrypt = true;
+spec_info->u.qcow2.data->encrypt = qencrypt;
+}
+
 return spec_info;
 }
 
diff --git a/qapi/block-core.json 

[Qemu-block] [PATCH v9 16/20] block: rip out all traces of password prompting

[Daniel P. Berrange]

Now that qcow & qcow2 are wired up to get encryption keys
via the QCryptoSecret object, nothing is relying on the
interactive prompting for passwords. All the code related
to password prompting can thus be ripped out.

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 hmp.c | 31 -
 include/monitor/monitor.h |  7 -
 include/qemu/osdep.h  |  2 --
 monitor.c | 68 ---
 qapi-schema.json  | 10 +--
 qemu-img.c| 31 -
 qemu-io.c | 20 --
 qmp.c | 12 +
 util/oslib-posix.c| 66 -
 util/oslib-win32.c| 24 -
 10 files changed, 2 insertions(+), 269 deletions(-)

diff --git a/hmp.c b/hmp.c
index 8c72c58..435cb31 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1085,37 +1085,12 @@ void hmp_ringbuf_read(Monitor *mon, const QDict *qdict)
 g_free(data);
 }
 
-static void hmp_cont_cb(void *opaque, int err)
-{
-if (!err) {
-qmp_cont(NULL);
-}
-}
-
-static bool key_is_missing(const BlockInfo *bdev)
-{
-return (bdev->inserted && bdev->inserted->encryption_key_missing);
-}
-
 void hmp_cont(Monitor *mon, const QDict *qdict)
 {
-BlockInfoList *bdev_list, *bdev;
 Error *err = NULL;
 
-bdev_list = qmp_query_block(NULL);
-for (bdev = bdev_list; bdev; bdev = bdev->next) {
-if (key_is_missing(bdev->value)) {
-monitor_read_block_device_key(mon, bdev->value->device,
-  hmp_cont_cb, NULL);
-goto out;
-}
-}
-
 qmp_cont();
 hmp_handle_error(mon, );
-
-out:
-qapi_free_BlockInfoList(bdev_list);
 }
 
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
@@ -1738,12 +1713,6 @@ void hmp_change(Monitor *mon, const QDict *qdict)
 qmp_blockdev_change_medium(true, device, false, NULL, target,
!!arg, arg, !!read_only, read_only_mode,
);
-if (err &&
-error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
-error_free(err);
-monitor_read_block_device_key(mon, device, NULL, NULL);
-return;
-}
 }
 
 hmp_handle_error(mon, );
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index d2b3aaf..83ea4a1 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -23,13 +23,6 @@ void monitor_cleanup(void);
 int monitor_suspend(Monitor *mon);
 void monitor_resume(Monitor *mon);
 
-int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
-BlockCompletionFunc *completion_cb,
-void *opaque);
-int monitor_read_block_device_key(Monitor *mon, const char *device,
-  BlockCompletionFunc *completion_cb,
-  void *opaque);
-
 int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
 int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp);
 
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index fb008a2..a5982ef 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -457,8 +457,6 @@ void qemu_set_tty_echo(int fd, bool echo);
 void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus,
  Error **errp);
 
-int qemu_read_password(char *buf, int buf_size);
-
 /**
  * qemu_get_pid_name:
  * @pid: pid of a process
diff --git a/monitor.c b/monitor.c
index fcf4fad..7647291 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4123,74 +4123,6 @@ void monitor_cleanup(void)
 qemu_mutex_unlock(_lock);
 }
 
-static void bdrv_password_cb(void *opaque, const char *password,
- void *readline_opaque)
-{
-Monitor *mon = opaque;
-BlockDriverState *bs = readline_opaque;
-int ret = 0;
-Error *local_err = NULL;
-
-bdrv_add_key(bs, password, _err);
-if (local_err) {
-error_report_err(local_err);
-ret = -EPERM;
-}
-if (mon->password_completion_cb)
-mon->password_completion_cb(mon->password_opaque, ret);
-
-monitor_read_command(mon, 1);
-}
-
-int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
-BlockCompletionFunc *completion_cb,
-void *opaque)
-{
-int err;
-
-monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
-   bdrv_get_encrypted_filename(bs));
-
-mon->password_completion_cb = completion_cb;
-mon->password_opaque = opaque;
-
-err = monitor_read_password(mon, bdrv_password_cb, bs);
-
-if (err && completion_cb)
-completion_cb(opaque, err);
-
-return err;
-}
-
-int 

[Qemu-block] [PATCH v9 15/20] iotests: enable tests 134 and 158 to work with qcow (v1)

[Daniel P. Berrange]

The 138 and 158 iotests exercise the legacy qcow2 aes encryption
code path and they work fine with qcow v1 too.

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/134 | 2 +-
 tests/qemu-iotests/158 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/134 b/tests/qemu-iotests/134
index f851d92..9914415 100755
--- a/tests/qemu-iotests/134
+++ b/tests/qemu-iotests/134
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-_supported_fmt qcow2
+_supported_fmt qcow qcow2
 _supported_proto generic
 _unsupported_proto vxhs
 _supported_os Linux
diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
index e280b79..823c120 100755
--- a/tests/qemu-iotests/158
+++ b/tests/qemu-iotests/158
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-_supported_fmt qcow2
+_supported_fmt qcow qcow2
 _supported_proto generic
 _unsupported_proto vxhs
 _supported_os Linux
-- 
2.9.3

[Qemu-block] [PATCH v9 11/20] qcow2: convert QCow2 to use QCryptoBlock for encryption

[Daniel P. Berrange]

This converts the qcow2 driver to make use of the QCryptoBlock
APIs for encrypting image content, using the legacy QCow2 AES
scheme.

With this change it is now required to use the QCryptoSecret
object for providing passwords, instead of the current block
password APIs / interactive prompting.

  $QEMU \
-object secret,id=sec0,filename=/home/berrange/encrypted.pw \
-drive file=/home/berrange/encrypted.qcow2,encrypt.key-secret=sec0

The test 087 could be simplified since there is no longer a
difference in behaviour when using blockdev_add with encrypted
images for the running vs stopped CPU state.

Reviewed-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 block/qcow2-cluster.c  |  47 +-
 block/qcow2.c  | 226 ++---
 block/qcow2.h  |   5 +-
 qapi/block-core.json   |  27 +-
 tests/qemu-iotests/049 |   2 +-
 tests/qemu-iotests/049.out |   4 +-
 tests/qemu-iotests/082.out |  27 ++
 tests/qemu-iotests/087 |  28 +++---
 tests/qemu-iotests/087.out |  12 +--
 tests/qemu-iotests/134 |  18 +++-
 tests/qemu-iotests/134.out |  10 +-
 tests/qemu-iotests/158 |  19 ++--
 tests/qemu-iotests/158.out |  14 +--
 tests/qemu-iotests/common  |  10 +-
 14 files changed, 263 insertions(+), 186 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 6400147..c4a256d 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -357,47 +357,6 @@ static int count_contiguous_clusters_unallocated(int 
nb_clusters,
 return i;
 }
 
-/* The crypt function is compatible with the linux cryptoloop
-   algorithm for < 4 GB images. */
-int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
-  uint8_t *buf, int nb_sectors, bool enc,
-  Error **errp)
-{
-union {
-uint64_t ll[2];
-uint8_t b[16];
-} ivec;
-int i;
-int ret;
-
-for(i = 0; i < nb_sectors; i++) {
-ivec.ll[0] = cpu_to_le64(sector_num);
-ivec.ll[1] = 0;
-if (qcrypto_cipher_setiv(s->cipher,
- ivec.b, G_N_ELEMENTS(ivec.b),
- errp) < 0) {
-return -1;
-}
-if (enc) {
-ret = qcrypto_cipher_encrypt(s->cipher,
- buf, buf,
- 512,
- errp);
-} else {
-ret = qcrypto_cipher_decrypt(s->cipher,
- buf, buf,
- 512,
- errp);
-}
-if (ret < 0) {
-return -1;
-}
-sector_num++;
-buf += 512;
-}
-return 0;
-}
-
 static int coroutine_fn do_perform_cow(BlockDriverState *bs,
uint64_t src_cluster_offset,
uint64_t cluster_offset,
@@ -438,11 +397,11 @@ static int coroutine_fn do_perform_cow(BlockDriverState 
*bs,
 Error *err = NULL;
 int64_t sector = (src_cluster_offset + offset_in_cluster)
  >> BDRV_SECTOR_BITS;
-assert(s->cipher);
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
-if (qcow2_encrypt_sectors(s, sector, iov.iov_base,
-  bytes >> BDRV_SECTOR_BITS, true, ) < 0) {
+assert(s->crypto);
+if (qcrypto_block_encrypt(s->crypto, sector, iov.iov_base,
+  bytes, ) < 0) {
 ret = -EIO;
 error_free(err);
 goto out;
diff --git a/block/qcow2.c b/block/qcow2.c
index b985ba7..9edd0a0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -37,6 +37,9 @@
 #include "qemu/option_int.h"
 #include "qemu/cutils.h"
 #include "qemu/bswap.h"
+#include "qapi/opts-visitor.h"
+#include "qapi-visit.h"
+#include "block/crypto.h"
 
 /*
   Differences with QCOW:
@@ -461,6 +464,7 @@ static QemuOptsList qcow2_runtime_opts = {
 .type = QEMU_OPT_NUMBER,
 .help = "Clean unused cache entries after this time (in seconds)",
 },
+BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET("encrypt."),
 { /* end of list */ }
 },
 };
@@ -585,6 +589,7 @@ typedef struct Qcow2ReopenState {
 int overlap_check;
 bool discard_passthrough[QCOW2_DISCARD_MAX];
 uint64_t cache_clean_interval;
+QCryptoBlockOpenOptions *crypto_opts; /* Disk encryption runtime options */
 } Qcow2ReopenState;
 
 static int qcow2_update_options_prepare(BlockDriverState *bs,
@@ -598,9 +603,14 @@ static int qcow2_update_options_prepare(BlockDriverState 
*bs,
 int overlap_check_template = 0;
 uint64_t l2_cache_size, refcount_cache_size;
 int i;
+const 

[Qemu-block] [PATCH v9 18/20] block: pass option prefix down to crypto layer

[Daniel P. Berrange]

While the crypto layer uses a fixed option name "key-secret",
the upper block layer may have a prefix on the options. e.g.
"encrypt.key-secret", in order to avoid clashes between crypto
option names & other block option names. To ensure the crypto
layer can report accurate error messages, we must tell it what
option name prefix was used.

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c| 4 ++--
 block/qcow.c  | 7 ---
 block/qcow2.c | 8 
 crypto/block-luks.c   | 8 ++--
 crypto/block-qcow.c   | 8 ++--
 crypto/block.c| 6 --
 crypto/blockpriv.h| 2 ++
 include/crypto/block.h| 6 +-
 tests/test-crypto-block.c | 8 
 9 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 3ad4b20..c561cba 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -296,7 +296,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat 
format,
 if (flags & BDRV_O_NO_IO) {
 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
 }
-crypto->block = qcrypto_block_open(open_opts,
+crypto->block = qcrypto_block_open(open_opts, NULL,
block_crypto_read_func,
bs,
cflags,
@@ -340,7 +340,7 @@ static int block_crypto_create_generic(QCryptoBlockFormat 
format,
 return -1;
 }
 
-crypto = qcrypto_block_create(create_opts,
+crypto = qcrypto_block_create(create_opts, NULL,
   block_crypto_init_func,
   block_crypto_write_func,
   ,
diff --git a/block/qcow.c b/block/qcow.c
index fb5a11f..fcd4715 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -208,8 +208,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, 
int flags,
 if (flags & BDRV_O_NO_IO) {
 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
 }
-s->crypto = qcrypto_block_open(crypto_opts, NULL, NULL,
-   cflags, errp);
+s->crypto = qcrypto_block_open(crypto_opts, "encrypt.",
+   NULL, NULL, cflags, errp);
 if (!s->crypto) {
 ret = -EINVAL;
 goto fail;
@@ -869,7 +869,8 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 goto exit;
 }
 
-crypto = qcrypto_block_create(crypto_opts, NULL, NULL, NULL, errp);
+crypto = qcrypto_block_create(crypto_opts, "encrypt.",
+  NULL, NULL, NULL, errp);
 if (!crypto) {
 ret = -EINVAL;
 goto exit;
diff --git a/block/qcow2.c b/block/qcow2.c
index 9ab361c..19bc69c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -279,7 +279,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
 if (flags & BDRV_O_NO_IO) {
 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
 }
-s->crypto = qcrypto_block_open(s->crypto_opts,
+s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
qcow2_crypto_hdr_read_func,
bs, cflags, errp);
 if (!s->crypto) {
@@ -1313,8 +1313,8 @@ static int qcow2_do_open(BlockDriverState *bs, QDict 
*options, int flags,
 if (flags & BDRV_O_NO_IO) {
 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
 }
-s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL,
-   cflags, errp);
+s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
+   NULL, NULL, cflags, errp);
 if (!s->crypto) {
 ret = -EINVAL;
 goto fail;
@@ -2274,7 +2274,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, 
const char *encryptfmt,
 }
 s->crypt_method_header = fmt;
 
-crypto = qcrypto_block_create(cryptoopts,
+crypto = qcrypto_block_create(cryptoopts, "encrypt.",
   qcow2_crypto_hdr_init_func,
   qcow2_crypto_hdr_write_func,
   bs, errp);
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 2b97d89..afb8543 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -638,6 +638,7 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
 static int
 qcrypto_block_luks_open(QCryptoBlock *block,
 QCryptoBlockOpenOptions *options,
+const char *optprefix,
 QCryptoBlockReadFunc readfunc,
 void *opaque,

[Qemu-block] [PATCH v9 20/20] docs: document encryption options for qcow, qcow2 and luks

[Daniel P. Berrange]

Expand the image format docs to cover the new options for
the qcow, qcow2 and luks disk image formats

Reviewed-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 qemu-doc.texi | 123 ++
 1 file changed, 115 insertions(+), 8 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index 965ba59..3c3081f 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -541,10 +541,20 @@ File name of a base image (see @option{create} subcommand)
 @item backing_fmt
 Image format of the base image
 @item encryption
-If this option is set to @code{on}, the image is encrypted with 128-bit 
AES-CBC.
+This option is deprecated and equivalent to @code{encrypt.format=aes}
 
-The use of encryption in qcow and qcow2 images is considered to be flawed by
-modern cryptography standards, suffering from a number of design problems:
+@item encrypt.format
+
+If this is set to @code{luks}, it requests that the qcow2 payload (not
+qcow2 header) be encrypted using the LUKS format. The passphrase to
+use to unlock the LUKS key slot is given by the @code{encrypt.key-secret}
+parameter. LUKS encryption parameters can be tuned with the other
+@code{encrypt.*} parameters.
+
+If this is set to @code{aes}, the image is encrypted with 128-bit AES-CBC.
+The encryption key is given by the @code{encrypt.key-secret} parameter.
+This encryption format is considered to be flawed by modern cryptography
+standards, suffering from a number of design problems:
 
 @itemize @minus
 @item The AES-CBC cipher is used with predictable initialization vectors based
@@ -559,10 +569,45 @@ original file must then be securely erased using a 
program like shred,
 though even this is ineffective with many modern storage technologies.
 @end itemize
 
-Use of qcow / qcow2 encryption with QEMU is deprecated, and support for
-it will go away in a future release.  Users are recommended to use an
-alternative encryption technology such as the Linux dm-crypt / LUKS
-system.
+The use of this is no longer supported in system emulators. Support only
+remains in the command line utilities, for the purposes of data liberation
+and interoperability with old versions of QEMU. The @code{luks} format
+should be used instead.
+
+@item encrypt.key-secret
+
+Provides the ID of a @code{secret} object that contains the passphrase
+(@code{encrypt.format=luks}) or encryption key (@code{encrypt.format=aes}).
+
+@item encrypt.cipher-alg
+
+Name of the cipher algorithm and key length. Currently defaults
+to @code{aes-256}. Only used when @code{encrypt.format=luks}.
+
+@item encrypt.cipher-mode
+
+Name of the encryption mode to use. Currently defaults to @code{xts}.
+Only used when @code{encrypt.format=luks}.
+
+@item encrypt.ivgen-alg
+
+Name of the initialization vector generator algorithm. Currently defaults
+to @code{plain64}. Only used when @code{encrypt.format=luks}.
+
+@item encrypt.ivgen-hash-alg
+
+Name of the hash algorithm to use with the initialization vector generator
+(if required). Defaults to @code{sha256}. Only used when 
@code{encrypt.format=luks}.
+
+@item encrypt.hash-alg
+
+Name of the hash algorithm to use for PBKDF algorithm
+Defaults to @code{sha256}. Only used when @code{encrypt.format=luks}.
+
+@item encrypt.iter-time
+
+Amount of time, in milliseconds, to use for PBKDF algorithm per key slot.
+Defaults to @code{2000}. Only used when @code{encrypt.format=luks}.
 
 @item cluster_size
 Changes the qcow2 cluster size (must be between 512 and 2M). Smaller cluster
@@ -637,7 +682,69 @@ Supported options:
 @item backing_file
 File name of a base image (see @option{create} subcommand)
 @item encryption
-If this option is set to @code{on}, the image is encrypted.
+This option is deprecated and equivalent to @code{encrypt.format=aes}
+
+@item encrypt.format
+If this is set to @code{aes}, the image is encrypted with 128-bit AES-CBC.
+The encryption key is given by the @code{encrypt.key-secret} parameter.
+This encryption format is considered to be flawed by modern cryptography
+standards, suffering from a number of design problems enumerated previously
+against the @code{qcow2} image format.
+
+The use of this is no longer supported in system emulators. Support only
+remains in the command line utilities, for the purposes of data liberation
+and interoperability with old versions of QEMU.
+
+Users requiring native encryption should use the @code{qcow2} format
+instead with @code{encrypt.format=luks}.
+
+@item encrypt.key-secret
+
+Provides the ID of a @code{secret} object that contains the encryption
+key (@code{encrypt.format=aes}).
+
+@end table
+
+@item luks
+
+LUKS v1 encryption format, compatible with Linux dm-crypt/cryptsetup
+
+Supported options:
+@table @code
+
+@item key-secret
+
+Provides the ID of a @code{secret} object that contains the passphrase.
+
+@item cipher-alg
+
+Name of the cipher algorithm and key length. Currently 

[Qemu-block] [PATCH v9 14/20] qcow2: add iotests to cover LUKS encryption support

[Daniel P. Berrange]

This extends the 087 iotest to cover LUKS encryption when doing
blockdev-add.

Two further tests are added to validate read/write of LUKS
encrypted images with a single file and with a backing file.

Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/087 | 35 ++-
 tests/qemu-iotests/087.out | 14 +++-
 tests/qemu-iotests/188 | 76 
 tests/qemu-iotests/188.out | 18 ++
 tests/qemu-iotests/189 | 86 ++
 tests/qemu-iotests/189.out | 26 ++
 tests/qemu-iotests/group   |  2 ++
 7 files changed, 255 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/188
 create mode 100644 tests/qemu-iotests/188.out
 create mode 100755 tests/qemu-iotests/189
 create mode 100644 tests/qemu-iotests/189.out

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 1d595b2..f8e4903 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -119,7 +119,7 @@ run_qemu .
+#
+
+# creator
+owner=berra...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=16M
+
+SECRET="secret,id=sec0,data=astrochicken"
+SECRETALT="secret,id=sec0,data=platypus"
+
+_make_test_img --object $SECRET -o 
"encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10" $size
+
+IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"
+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+echo
+echo "== reading whole image =="
+$QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts $IMGSPEC | 
_filter_qemu_io | _filter_testdir
+
+echo
+echo "== rewriting whole image =="
+$QEMU_IO --object $SECRET -c "write -P 0xa 0 $size" --image-opts $IMGSPEC | 
_filter_qemu_io | _filter_testdir
+
+echo
+echo "== verify pattern =="
+$QEMU_IO --object $SECRET -c "read -P 0xa 0 $size"  --image-opts $IMGSPEC | 
_filter_qemu_io | _filter_testdir
+
+echo
+echo "== verify open failure with wrong password =="
+$QEMU_IO --object $SECRETALT -c "read -P 0xa 0 $size" --image-opts $IMGSPEC | 
_filter_qemu_io | _filter_testdir
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/188.out b/tests/qemu-iotests/188.out
new file mode 100644
index 000..8af24e5
--- /dev/null
+++ b/tests/qemu-iotests/188.out
@@ -0,0 +1,18 @@
+QA output created by 188
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10
+
+== reading whole image ==
+read 16777216/16777216 bytes at offset 0
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting whole image ==
+wrote 16777216/16777216 bytes at offset 0
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify pattern ==
+read 16777216/16777216 bytes at offset 0
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify open failure with wrong password ==
+can't open: Invalid password, cannot unlock any keyslot
+*** done
diff --git a/tests/qemu-iotests/189 b/tests/qemu-iotests/189
new file mode 100755
index 000..54ad980
--- /dev/null
+++ b/tests/qemu-iotests/189
@@ -0,0 +1,86 @@
+#!/bin/bash
+#
+# Test encrypted read/write using backing files
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=berra...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=16M
+TEST_IMG_BASE=$TEST_IMG.base
+SECRET0="secret,id=sec0,data=astrochicken"
+SECRET1="secret,id=sec1,data=furby"
+
+TEST_IMG_SAVE=$TEST_IMG
+TEST_IMG=$TEST_IMG_BASE
+echo "== create base =="
+_make_test_img --object $SECRET0 -o 

[Qemu-block] [PATCH v9 13/20] qcow2: add support for LUKS encryption format

[Daniel P. Berrange]

This adds support for using LUKS as an encryption format
with the qcow2 file, using the new encrypt.format parameter
to request "luks" format. e.g.

  # qemu-img create --object secret,data=123456,id=sec0 \
   -f qcow2 -o encrypt.format=luks,encrypt.key-secret=sec0 \
   test.qcow2 10G

The legacy "encryption=on" parameter still results in
creation of the old qcow2 AES format (and is equivalent
to the new 'encryption-format=aes'). e.g. the following are
equivalent:

  # qemu-img create --object secret,data=123456,id=sec0 \
   -f qcow2 -o encryption=on,encrypt.key-secret=sec0 \
   test.qcow2 10G

 # qemu-img create --object secret,data=123456,id=sec0 \
   -f qcow2 -o encryption-format=aes,encrypt.key-secret=sec0 \
   test.qcow2 10G

With the LUKS format it is necessary to store the LUKS
partition header and key material in the QCow2 file. This
data can be many MB in size, so cannot go into the QCow2
header region directly. Thus the spec defines a FDE
(Full Disk Encryption) header extension that specifies
the offset of a set of clusters to hold the FDE headers,
as well as the length of that region. The LUKS header is
thus stored in these extra allocated clusters before the
main image payload.

Aside from all the cryptographic differences implied by
use of the LUKS format, there is one further key difference
between the use of legacy AES and LUKS encryption in qcow2.
For LUKS, the initialiazation vectors are generated using
the host physical sector as the input, rather than the
guest virtual sector. This guarantees unique initialization
vectors for all sectors when qcow2 internal snapshots are
used, thus giving stronger protection against watermarking
attacks.

Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 block/qcow2-cluster.c  |   4 +-
 block/qcow2-refcount.c |  10 ++
 block/qcow2.c  | 268 ++--
 block/qcow2.h  |   9 ++
 qapi/block-core.json   |   5 +-
 tests/qemu-iotests/082.out | 270 -
 6 files changed, 478 insertions(+), 88 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index c4a256d..edec6a7 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -395,7 +395,9 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,
 
 if (bs->encrypted) {
 Error *err = NULL;
-int64_t sector = (src_cluster_offset + offset_in_cluster)
+int64_t sector = (s->crypt_physical_offset ?
+  (cluster_offset + offset_in_cluster) :
+  (src_cluster_offset + offset_in_cluster))
  >> BDRV_SECTOR_BITS;
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7c06061..81c22e6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1856,6 +1856,16 @@ static int calculate_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
 return ret;
 }
 
+/* encryption */
+if (s->crypto_header.length) {
+ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
+s->crypto_header.offset,
+s->crypto_header.length);
+if (ret < 0) {
+return ret;
+}
+}
+
 return check_refblocks(bs, res, fix, rebuild, refcount_table, nb_clusters);
 }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 9edd0a0..bb6e7c1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -66,6 +66,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_END 0
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
 
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -80,6 +81,86 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 }
 
 
+static ssize_t qcow2_crypto_hdr_read_func(QCryptoBlock *block, size_t offset,
+  uint8_t *buf, size_t buflen,
+  void *opaque, Error **errp)
+{
+BlockDriverState *bs = opaque;
+BDRVQcow2State *s = bs->opaque;
+ssize_t ret;
+
+if ((offset + buflen) > s->crypto_header.length) {
+error_setg(errp, "Request for data outside of extension header");
+return -1;
+}
+
+ret = bdrv_pread(bs->file,
+ s->crypto_header.offset + offset, buf, buflen);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not read encryption header");
+return -1;
+}
+return ret;
+}
+
+
+static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t 
headerlen,
+  void *opaque, Error **errp)
+{
+  

[Qemu-block] [PATCH v9 07/20] block: deprecate "encryption=on" in favor of "encrypt.format=aes"

[Daniel P. Berrange]

Historically the qcow & qcow2 image formats supported a property
"encryption=on" to enable their built-in AES encryption. We'll
soon be supporting LUKS for qcow2, so need a more general purpose
way to enable encryption, with a choice of formats.

This introduces an "encrypt.format" option, which will later be
joined by a number of other "encrypt.XXX" options. The use of
a "encrypt." prefix instead of "encrypt-" is done to facilitate
mapping to a nested QAPI schema at later date.

e.g. the preferred syntax is now

  qemu-img create -f qcow2 -o encrypt.format=aes demo.qcow2

Signed-off-by: Daniel P. Berrange 
---
 block/qcow.c   | 34 +---
 block/qcow2.c  | 36 +
 include/block/block_int.h  |  2 +-
 qemu-img.c |  4 +-
 tests/qemu-iotests/049.out | 98 +++---
 tests/qemu-iotests/082.out | 95 
 tests/qemu-iotests/085.out | 38 +-
 tests/qemu-iotests/144.out |  4 +-
 8 files changed, 192 insertions(+), 119 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 49871fb..3047a61 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -803,10 +803,11 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 uint8_t *tmp;
 int64_t total_size = 0;
 char *backing_file = NULL;
-int flags = 0;
 Error *local_err = NULL;
 int ret;
 BlockBackend *qcow_blk;
+const char *encryptfmt = NULL;
+char *buf;
 
 /* Read out options */
 total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
@@ -818,8 +819,18 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 }
 
 backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
-if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-flags |= BLOCK_FLAG_ENCRYPT;
+encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
+if (encryptfmt) {
+buf = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT);
+if (buf != NULL) {
+g_free(buf);
+error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
+   BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
+ret = -EINVAL;
+goto cleanup;
+}
+} else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
+encryptfmt = "aes";
 }
 
 ret = bdrv_create_file(filename, opts, _err);
@@ -873,7 +884,13 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 l1_size = (total_size + (1LL << shift) - 1) >> shift;
 
 header.l1_table_offset = cpu_to_be64(header_size);
-if (flags & BLOCK_FLAG_ENCRYPT) {
+if (encryptfmt) {
+if (!g_str_equal(encryptfmt, "aes")) {
+error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
+   encryptfmt);
+ret = -EINVAL;
+goto exit;
+}
 header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
 } else {
 header.crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
@@ -1047,8 +1064,13 @@ static QemuOptsList qcow_create_opts = {
 {
 .name = BLOCK_OPT_ENCRYPT,
 .type = QEMU_OPT_BOOL,
-.help = "Encrypt the image",
-.def_value_str = "off"
+.help = "Encrypt the image with format 'aes'. (Deprecated "
+"in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",
+},
+{
+.name = BLOCK_OPT_ENCRYPT_FORMAT,
+.type = QEMU_OPT_STRING,
+.help = "Encrypt the image, format choices: 'aes'",
 },
 { /* end of list */ }
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index b3ba5da..652e6a4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2100,7 +2100,7 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
  const char *backing_file, const char *backing_format,
  int flags, size_t cluster_size, PreallocMode prealloc,
  QemuOpts *opts, int version, int refcount_order,
- Error **errp)
+ const char *encryptfmt, Error **errp)
 {
 int cluster_bits;
 QDict *options;
@@ -2229,7 +2229,13 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
 .header_length  = cpu_to_be32(sizeof(*header)),
 };
 
-if (flags & BLOCK_FLAG_ENCRYPT) {
+if (encryptfmt) {
+if (!g_str_equal(encryptfmt, "aes")) {
+error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
+   encryptfmt);
+ret = -EINVAL;
+goto out;
+}
 header->crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
 } else {
 header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
@@ -2358,6 +2364,7 @@ static int qcow2_create(const char *filename, 

[Qemu-block] [PATCH v9 12/20] qcow2: extend specification to cover LUKS encryption

[Daniel P. Berrange]

Update the qcow2 specification to describe how the LUKS header is
placed inside a qcow2 file, when using LUKS encryption for the
qcow2 payload instead of the legacy AES-CBC encryption

Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 docs/specs/qcow2.txt | 103 +++
 1 file changed, 103 insertions(+)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0..886a546 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -45,6 +45,7 @@ The first cluster of a qcow2 image contains the file header:
  32 - 35:   crypt_method
 0 for no encryption
 1 for AES encryption
+2 for LUKS encryption
 
  36 - 39:   l1_size
 Number of entries in the active L1 table
@@ -135,6 +136,7 @@ be stored. Each extension has a structure like the 
following:
 0xE2792ACA - Backing file format name
 0x6803f857 - Feature name table
 0x23852875 - Bitmaps extension
+0x0537be77 - Full disk encryption header pointer
 other  - Unknown header extension, can be safely
  ignored
 
@@ -207,6 +209,107 @@ The fields of the bitmaps extension are:
Offset into the image file at which the bitmap directory
starts. Must be aligned to a cluster boundary.
 
+== Full disk encryption header pointer ==
+
+The full disk encryption header must be present if, and only if, the
+'crypt_method' header requires metadata. Currently this is only true
+of the 'LUKS' crypt method. The header extension must be absent for
+other methods.
+
+This header provides the offset at which the crypt method can store
+its additional data, as well as the length of such data.
+
+Byte  0 -  7:   Offset into the image file at which the encryption
+header starts in bytes. Must be aligned to a cluster
+boundary.
+Byte  8 - 15:   Length of the written encryption header in bytes.
+Note actual space allocated in the qcow2 file may
+be larger than this value, since it will be rounded
+to the nearest multiple of the cluster size. Any
+unused bytes in the allocated space will be initialized
+to 0.
+
+For the LUKS crypt method, the encryption header works as follows.
+
+The first 592 bytes of the header clusters will contain the LUKS
+partition header. This is then followed by the key material data areas.
+The size of the key material data areas is determined by the number of
+stripes in the key slot and key size. Refer to the LUKS format
+specification ('docs/on-disk-format.pdf' in the cryptsetup source
+package) for details of the LUKS partition header format.
+
+In the LUKS partition header, the "payload-offset" field will be
+calculated as normal for the LUKS spec. ie the size of the LUKS
+header, plus key material regions, plus padding, relative to the
+start of the LUKS header. This offset value is not required to be
+qcow2 cluster aligned. Its value is currently never used in the
+context of qcow2, since the qcow2 file format itself defines where
+the real payload offset is, but none the less a valid payload offset
+should always be present.
+
+In the LUKS key slots header, the "key-material-offset" is relative
+to the start of the LUKS header clusters in the qcow2 container,
+not the start of the qcow2 file.
+
+Logically the layout looks like
+
+  +-+
+  | QCow2 header|
+  | QCow2 header extension X|
+  | QCow2 header extension FDE  |
+  | QCow2 header extension ...  |
+  | QCow2 header extension Z|
+  +-+
+  | other QCow2 tables  |
+  . .
+  . .
+  +-+
+  | +-+ |
+  | | LUKS partition header   | |
+  | +-+ |
+  | | LUKS key material 1 | |
+  | +-+ |
+  | | LUKS key material 2 | |
+  | +-+ |
+  | | LUKS key material ...   | |
+  | +-+ |
+  | | LUKS key material 8 | |
+  | +-+ |
+  +-+
+  | QCow2 cluster payload   |
+  . .
+  . .
+  . .
+  | |
+  +-+
+
+== Data encryption ==
+
+When an encryption method is requested in the header, the image payload
+data must be encrypted/decrypted on every write/read. The image headers
+and metadata are never 

[Qemu-block] [PATCH v9 10/20] qcow2: make qcow2_encrypt_sectors encrypt in place

[Daniel P. Berrange]

Instead of requiring separate input/output buffers for
encrypting data, change qcow2_encrypt_sectors() to assume
use of a single buffer, encrypting in place. The current
callers all used the same buffer for input/output already.

Reviewed-by: Eric Blake 
Reviewed-by: Fam Zheng 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 block/qcow2-cluster.c | 17 ++---
 block/qcow2.c |  4 ++--
 block/qcow2.h |  3 +--
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index d779ea1..6400147 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -358,11 +358,9 @@ static int count_contiguous_clusters_unallocated(int 
nb_clusters,
 }
 
 /* The crypt function is compatible with the linux cryptoloop
-   algorithm for < 4 GB images. NOTE: out_buf == in_buf is
-   supported */
+   algorithm for < 4 GB images. */
 int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
-  uint8_t *out_buf, const uint8_t *in_buf,
-  int nb_sectors, bool enc,
+  uint8_t *buf, int nb_sectors, bool enc,
   Error **errp)
 {
 union {
@@ -382,14 +380,12 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
 }
 if (enc) {
 ret = qcrypto_cipher_encrypt(s->cipher,
- in_buf,
- out_buf,
+ buf, buf,
  512,
  errp);
 } else {
 ret = qcrypto_cipher_decrypt(s->cipher,
- in_buf,
- out_buf,
+ buf, buf,
  512,
  errp);
 }
@@ -397,8 +393,7 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
 return -1;
 }
 sector_num++;
-in_buf += 512;
-out_buf += 512;
+buf += 512;
 }
 return 0;
 }
@@ -446,7 +441,7 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,
 assert(s->cipher);
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
-if (qcow2_encrypt_sectors(s, sector, iov.iov_base, iov.iov_base,
+if (qcow2_encrypt_sectors(s, sector, iov.iov_base,
   bytes >> BDRV_SECTOR_BITS, true, ) < 0) {
 ret = -EIO;
 error_free(err);
diff --git a/block/qcow2.c b/block/qcow2.c
index 652e6a4..b985ba7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1543,7 +1543,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
 Error *err = NULL;
 if (qcow2_encrypt_sectors(s, offset >> BDRV_SECTOR_BITS,
-  cluster_data, cluster_data,
+  cluster_data,
   cur_bytes >> BDRV_SECTOR_BITS,
   false, ) < 0) {
 error_free(err);
@@ -1639,7 +1639,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 qemu_iovec_to_buf(_qiov, 0, cluster_data, hd_qiov.size);
 
 if (qcow2_encrypt_sectors(s, offset >> BDRV_SECTOR_BITS,
-  cluster_data, cluster_data,
+  cluster_data,
   cur_bytes >>BDRV_SECTOR_BITS,
   true, ) < 0) {
 error_free(err);
diff --git a/block/qcow2.h b/block/qcow2.h
index 1801dc3..6b3d5de 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -538,8 +538,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t 
min_size,
 int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
 int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
-  uint8_t *out_buf, const uint8_t *in_buf,
-  int nb_sectors, bool enc, Error **errp);
+  uint8_t *buf, int nb_sectors, bool enc, Error 
**errp);
 
 int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
  unsigned int *bytes, uint64_t *cluster_offset);
-- 
2.9.3

[Qemu-block] [PATCH v9 08/20] qcow: make encrypt_sectors encrypt in place

[Daniel P. Berrange]

Instead of requiring separate input/output buffers for
encrypting data, change encrypt_sectors() to assume
use of a single buffer, encrypting in place. One current
caller uses the same buffer for input/output already
and the other two callers are easily converted to do so.

Reviewed-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
Reviewed-by: Kevin Wolf 
Signed-off-by: Daniel P. Berrange 
---
 block/qcow.c | 45 +++--
 1 file changed, 15 insertions(+), 30 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 3047a61..2306e4f 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -322,11 +322,10 @@ static int qcow_set_key(BlockDriverState *bs, const char 
*key)
 }
 
 /* The crypt function is compatible with the linux cryptoloop
-   algorithm for < 4 GB images. NOTE: out_buf == in_buf is
-   supported */
+   algorithm for < 4 GB images. */
 static int encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
-   uint8_t *out_buf, const uint8_t *in_buf,
-   int nb_sectors, bool enc, Error **errp)
+   uint8_t *buf, int nb_sectors, bool enc,
+   Error **errp)
 {
 union {
 uint64_t ll[2];
@@ -345,14 +344,12 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t 
sector_num,
 }
 if (enc) {
 ret = qcrypto_cipher_encrypt(s->cipher,
- in_buf,
- out_buf,
+ buf, buf,
  512,
  errp);
 } else {
 ret = qcrypto_cipher_decrypt(s->cipher,
- in_buf,
- out_buf,
+ buf, buf,
  512,
  errp);
 }
@@ -360,8 +357,7 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t 
sector_num,
 return -1;
 }
 sector_num++;
-in_buf += 512;
-out_buf += 512;
+buf += 512;
 }
 return 0;
 }
@@ -481,13 +477,12 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
 uint64_t start_sect;
 assert(s->cipher);
 start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
-memset(s->cluster_data + 512, 0x00, 512);
 for(i = 0; i < s->cluster_sectors; i++) {
 if (i < n_start || i >= n_end) {
 Error *err = NULL;
+memset(s->cluster_data, 0x00, 512);
 if (encrypt_sectors(s, start_sect + i,
-s->cluster_data,
-s->cluster_data + 512, 1,
+s->cluster_data, 1,
 true, ) < 0) {
 error_free(err);
 errno = EIO;
@@ -665,7 +660,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, 
int64_t sector_num,
 }
 if (bs->encrypted) {
 assert(s->cipher);
-if (encrypt_sectors(s, sector_num, buf, buf,
+if (encrypt_sectors(s, sector_num, buf,
 n, false, ) < 0) {
 goto fail;
 }
@@ -700,9 +695,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState 
*bs, int64_t sector_num,
 BDRVQcowState *s = bs->opaque;
 int index_in_cluster;
 uint64_t cluster_offset;
-const uint8_t *src_buf;
 int ret = 0, n;
-uint8_t *cluster_data = NULL;
 struct iovec hd_iov;
 QEMUIOVector hd_qiov;
 uint8_t *buf;
@@ -710,7 +703,9 @@ static coroutine_fn int qcow_co_writev(BlockDriverState 
*bs, int64_t sector_num,
 
 s->cluster_cache_offset = -1; /* disable compressed cache */
 
-if (qiov->niov > 1) {
+/* We must always copy the iov when encrypting, so we
+ * don't modify the original data buffer during encryption */
+if (bs->encrypted || qiov->niov > 1) {
 buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
 if (buf == NULL) {
 return -ENOMEM;
@@ -740,21 +735,14 @@ static coroutine_fn int qcow_co_writev(BlockDriverState 
*bs, int64_t sector_num,
 if (bs->encrypted) {
 Error *err = NULL;
 assert(s->cipher);
-if (!cluster_data) {
-cluster_data = g_malloc0(s->cluster_size);
-}
-if (encrypt_sectors(s, sector_num, cluster_data, buf,
-n, true, ) < 0) {

[Qemu-block] [PATCH v9 03/20] qcow: document another weakness of qcow AES encryption

[Daniel P. Berrange]

Document that use of guest virtual sector numbers as the basis for
the initialization vectors is a potential weakness, when combined
with internal snapshots or multiple images using the same passphrase.
This fixes the formatting of the itemized list too.

Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 qemu-img.texi | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/qemu-img.texi b/qemu-img.texi
index 5b925ec..f335139 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -567,16 +567,29 @@ The use of encryption in qcow and qcow2 images is 
considered to be flawed by
 modern cryptography standards, suffering from a number of design problems:
 
 @itemize @minus
-@item The AES-CBC cipher is used with predictable initialization vectors based
+@item
+The AES-CBC cipher is used with predictable initialization vectors based
 on the sector number. This makes it vulnerable to chosen plaintext attacks
 which can reveal the existence of encrypted data.
-@item The user passphrase is directly used as the encryption key. A poorly
+@item
+The user passphrase is directly used as the encryption key. A poorly
 chosen or short passphrase will compromise the security of the encryption.
-@item In the event of the passphrase being compromised there is no way to
+@item
+In the event of the passphrase being compromised there is no way to
 change the passphrase to protect data in any qcow images. The files must
 be cloned, using a different encryption passphrase in the new file. The
 original file must then be securely erased using a program like shred,
 though even this is ineffective with many modern storage technologies.
+@item
+Initialization vectors used to encrypt sectors are based on the
+guest virtual sector number, instead of the host physical sector. When
+a disk image has multiple internal snapshots this means that data in
+multiple physical sectors is encrypted with the same initialization
+vector. With the CBC mode, this opens the possibility of watermarking
+attacks if the attack can collect multiple sectors encrypted with the
+same IV and some predictable data. Having multiple qcow2 images with
+the same passphrase also exposes this weakness since the passphrase
+is directly used as the key.
 @end itemize
 
 Use of qcow / qcow2 encryption is thus strongly discouraged. Users are
-- 
2.9.3

[Qemu-block] [PATCH v9 04/20] qcow: require image size to be > 1 for new images

[Daniel P. Berrange]

The qcow driver refuses to open images which are less than
2 bytes in size, but will happily create such images. Add
a check in the create path to avoid this discrepancy.

Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 block/qcow.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/qcow.c b/block/qcow.c
index 7bd94dc..49871fb 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -811,6 +811,12 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 /* Read out options */
 total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
   BDRV_SECTOR_SIZE);
+if (total_size == 0) {
+error_setg(errp, "Image size is too small, cannot be zero length");
+ret = -EINVAL;
+goto cleanup;
+}
+
 backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
 if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
 flags |= BLOCK_FLAG_ENCRYPT;
-- 
2.9.3

[Qemu-block] [PATCH v9 01/20] block: expose crypto option names / defs to other drivers

[Daniel P. Berrange]

The block/crypto.c defines a set of QemuOpts that provide
parameters for encryption. This will also be needed by
the qcow/qcow2 integration, so expose the relevant pieces
in a new block/crypto.h header. Some helper methods taking
QemuOpts are changed to take QDict to simplify usage in
other places.

Reviewed-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c | 82 +---
 block/crypto.h | 91 ++
 2 files changed, 117 insertions(+), 56 deletions(-)
 create mode 100644 block/crypto.h

diff --git a/block/crypto.c b/block/crypto.c
index 10e5ddc..ea40ba4 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -24,16 +24,10 @@
 #include "sysemu/block-backend.h"
 #include "crypto/block.h"
 #include "qapi/opts-visitor.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qapi-visit.h"
 #include "qapi/error.h"
-
-#define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret"
-#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg"
-#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode"
-#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG "ivgen-alg"
-#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg"
-#define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"
-#define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
+#include "block/crypto.h"
 
 typedef struct BlockCrypto BlockCrypto;
 
@@ -135,11 +129,7 @@ static QemuOptsList block_crypto_runtime_opts_luks = {
 .name = "crypto",
 .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head),
 .desc = {
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,
-.type = QEMU_OPT_STRING,
-.help = "ID of the secret that provides the encryption key",
-},
+BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET,
 { /* end of list */ }
 },
 };
@@ -154,49 +144,21 @@ static QemuOptsList block_crypto_create_opts_luks = {
 .type = QEMU_OPT_SIZE,
 .help = "Virtual disk size"
 },
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,
-.type = QEMU_OPT_STRING,
-.help = "ID of the secret that provides the encryption key",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG,
-.type = QEMU_OPT_STRING,
-.help = "Name of encryption cipher algorithm",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE,
-.type = QEMU_OPT_STRING,
-.help = "Name of encryption cipher mode",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG,
-.type = QEMU_OPT_STRING,
-.help = "Name of IV generator algorithm",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG,
-.type = QEMU_OPT_STRING,
-.help = "Name of IV generator hash algorithm",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_HASH_ALG,
-.type = QEMU_OPT_STRING,
-.help = "Name of encryption hash algorithm",
-},
-{
-.name = BLOCK_CRYPTO_OPT_LUKS_ITER_TIME,
-.type = QEMU_OPT_NUMBER,
-.help = "Time to spend in PBKDF in milliseconds",
-},
+BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET,
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG,
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE,
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG,
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG,
+BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG,
+BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME,
 { /* end of list */ }
 },
 };
 
 
-static QCryptoBlockOpenOptions *
+QCryptoBlockOpenOptions *
 block_crypto_open_opts_init(QCryptoBlockFormat format,
-QemuOpts *opts,
+QDict *opts,
 Error **errp)
 {
 Visitor *v;
@@ -206,7 +168,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 ret = g_new0(QCryptoBlockOpenOptions, 1);
 ret->format = format;
 
-v = opts_visitor_new(opts);
+v = qobject_input_visitor_new_keyval(QOBJECT(opts));
 
 visit_start_struct(v, NULL, NULL, 0, _err);
 if (local_err) {
@@ -240,9 +202,9 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 }
 
 
-static QCryptoBlockCreateOptions *
+QCryptoBlockCreateOptions *
 block_crypto_create_opts_init(QCryptoBlockFormat format,
-  QemuOpts *opts,
+  QDict *opts,
   Error **errp)
 {
 Visitor *v;
@@ -252,7 +214,7 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
 ret = g_new0(QCryptoBlockCreateOptions, 1);
 ret->format = format;
 
-v = opts_visitor_new(opts);
+v = qobject_input_visitor_new_keyval(QOBJECT(opts));
 
 visit_start_struct(v, NULL, 

[Qemu-block] [PATCH v9 09/20] qcow: convert QCow to use QCryptoBlock for encryption

[Daniel P. Berrange]

This converts the qcow driver to make use of the QCryptoBlock
APIs for encrypting image content. This is only wired up to
permit use of the legacy QCow encryption format. Users who wish
to have the strong LUKS format should switch to qcow2 instead.

With this change it is now required to use the QCryptoSecret
object for providing passwords, instead of the current block
password APIs / interactive prompting.

  $QEMU \
-object secret,id=sec0,filename=/home/berrange/encrypted.pw \
-drive file=/home/berrange/encrypted.qcow,encrypt.format=qcow,\
   encrypt.key-secret=sec0

Though note that running QEMU system emulators with the AES
encryption is no longer supported, so while the above syntax
is valid, QEMU will refuse to actually run the VM in this
particular example.

Likewise when creating images with the legacy AES-CBC format

  qemu-img create -f qcow \
--object secret,id=sec0,filename=/home/berrange/encrypted.pw \
-o encrypt.format=aes,encrypt.key-secret=sec0 \
/home/berrange/encrypted.qcow 64M

Reviewed-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c   |  10 +++
 block/crypto.h   |  20 --
 block/qcow.c | 198 +--
 qapi/block-core.json |  38 +-
 4 files changed, 158 insertions(+), 108 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 9df1e5d..da4be74 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -181,6 +181,11 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 v, >u.luks, _err);
 break;
 
+case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+visit_type_QCryptoBlockOptionsQCow_members(
+v, >u.qcow, _err);
+break;
+
 default:
 error_setg(_err, "Unsupported block format %d", format);
 break;
@@ -227,6 +232,11 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
 v, >u.luks, _err);
 break;
 
+case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+visit_type_QCryptoBlockOptionsQCow_members(
+v, >u.qcow, _err);
+break;
+
 default:
 error_setg(_err, "Unsupported block format %d", format);
 break;
diff --git a/block/crypto.h b/block/crypto.h
index 3430dcd..0f985ea 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -21,6 +21,19 @@
 #ifndef BLOCK_CRYPTO_H__
 #define BLOCK_CRYPTO_H__
 
+#define BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix, helpstr)\
+{   \
+.name = prefix BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET,\
+.type = QEMU_OPT_STRING,\
+.help = helpstr,\
+}
+
+#define BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET "key-secret"
+
+#define BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET(prefix)\
+BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix, \
+"ID of the secret that provides the AES encryption key")
+
 #define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret"
 #define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg"
 #define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode"
@@ -30,11 +43,8 @@
 #define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
 
 #define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(prefix)\
-{   \
-.name = prefix BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,\
-.type = QEMU_OPT_STRING,\
-.help = "ID of the secret that provides the keyslot passphrase", \
-}
+BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix, \
+"ID of the secret that provides the keyslot passphrase")
 
 #define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(prefix)   \
 {  \
diff --git a/block/qcow.c b/block/qcow.c
index 2306e4f..3dca60c 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -31,8 +31,10 @@
 #include "qemu/bswap.h"
 #include 
 #include "qapi/qmp/qerror.h"
-#include "crypto/cipher.h"
+#include "qapi/qmp/qstring.h"
+#include "crypto/block.h"
 #include "migration/blocker.h"
+#include "block/crypto.h"
 
 /**/
 /* QEMU COW block driver with compression and encryption support */
@@ -77,7 +79,7 @@ typedef struct BDRVQcowState {
 uint8_t *cluster_cache;
 uint8_t *cluster_data;
 uint64_t cluster_cache_offset;
-QCryptoCipher *cipher; /* NULL if no key yet */
+QCryptoBlock *crypto; /* Disk encryption format driver */
 uint32_t crypt_method_header;
 CoMutex lock;
 Error *migration_blocker;
@@ -97,6 +99,15 @@ static int qcow_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 return 0;
 }
 
+static QemuOptsList qcow_runtime_opts = 

[Qemu-block] [PATCH v9 06/20] iotests: skip 048 with qcow which doesn't support resize

[Daniel P. Berrange]

Test 048 is designed to verify data preservation during an
image resize. The qcow (v1) format impl has never supported
resize so always fails.

Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/048 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
index 203c04f..9ed04a0 100755
--- a/tests/qemu-iotests/048
+++ b/tests/qemu-iotests/048
@@ -46,7 +46,7 @@ _compare()
 . ./common.filter
 . ./common.pattern
 
-_supported_fmt raw qcow qcow2 qed luks
+_supported_fmt raw qcow2 qed luks
 _supported_proto file
 _supported_os Linux
 
-- 
2.9.3

[Qemu-block] [PATCH v9 02/20] block: add ability to set a prefix for opt names

[Daniel P. Berrange]

When integrating the crypto support with qcow/qcow2, we don't
want to use the bare LUKS option names "hash-alg", "key-secret",
etc. We need to namespace them to match the nested QAPI schema.

e.g. "encrypt.hash-alg", "encrypt.key-secret"

so that they don't clash with any general qcow options at a later
date.

Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c | 16 
 block/crypto.h | 40 
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index ea40ba4..9df1e5d 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -129,7 +129,7 @@ static QemuOptsList block_crypto_runtime_opts_luks = {
 .name = "crypto",
 .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head),
 .desc = {
-BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET,
+BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""),
 { /* end of list */ }
 },
 };
@@ -144,13 +144,13 @@ static QemuOptsList block_crypto_create_opts_luks = {
 .type = QEMU_OPT_SIZE,
 .help = "Virtual disk size"
 },
-BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET,
-BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG,
-BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE,
-BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG,
-BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG,
-BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG,
-BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME,
+BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(""),
 { /* end of list */ }
 },
 };
diff --git a/block/crypto.h b/block/crypto.h
index c0e9b54..3430dcd 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -29,51 +29,51 @@
 #define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"
 #define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET\
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(prefix)\
 {   \
-.name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,   \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,\
 .type = QEMU_OPT_STRING,\
 .help = "ID of the secret that provides the keyslot passphrase", \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG   \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(prefix)   \
 {  \
-.name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG,  \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG,   \
 .type = QEMU_OPT_STRING,   \
 .help = "Name of encryption cipher algorithm", \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE \
-{ \
-.name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE,\
-.type = QEMU_OPT_STRING,  \
-.help = "Name of encryption cipher mode", \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE(prefix)  \
+{  \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE,  \
+.type = QEMU_OPT_STRING,   \
+.help = "Name of encryption cipher mode",  \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG   \
-{ \
-.name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG,  \
-.type = QEMU_OPT_STRING,  \
-.help = "Name of IV generator algorithm", \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG(prefix) \
+{   \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG, \
+.type = QEMU_OPT_STRING,\
+.help = "Name of IV generator algorithm",   \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG\
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(prefix)\
 {   \
-.name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG,   \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG,\
 .type = QEMU_OPT_STRING,\
 .help = "Name of IV generator hash algorithm",  \
 }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG   \
+#define 

[Qemu-block] [PATCH v9 05/20] iotests: skip 042 with qcow which dosn't support zero sized images

[Daniel P. Berrange]

Test 042 is designed to verify operation with zero sized images.
Such images are not supported with qcow (v1), so this test has
always failed.

Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/042 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/042 b/tests/qemu-iotests/042
index 351b283..a53e7cb 100755
--- a/tests/qemu-iotests/042
+++ b/tests/qemu-iotests/042
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-_supported_fmt qcow2 qcow qed
+_supported_fmt qcow2 qed
 _supported_proto file
 _supported_os Linux
 
-- 
2.9.3

[Qemu-block] [PATCH v9 00/20] Convert QCow[2] to QCryptoBlock & add LUKS support

[Daniel P. Berrange]

Previously posted:

 v1: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00201.html
 v2: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05147.html
 v3: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05671.html
 v4: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02293.html
 v5: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04653.html
 v6: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg04561.html
 v7: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05818.html
 v8: https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg00308.html

NB in v9 i've removed Reviewed-by from patch 07/20, since the
changes had a ripple effect into the test suite, due to need to
remove the "def_value_str" in the QemuOpts list.


This series is a continuation of previous work to support LUKS in
QEMU. The existing merged code supports LUKS as a standalone
driver which can be layered over/under any other QEMU block device
driver. This works well when using LUKS over protocol drivers (file,
rbd, iscsi, etc, etc), but has some downsides when combined with
format drivers like qcow2.

If you layer LUKS under qcow2 (eg qcow2 -> luks -> file) then you
cannot get any information about the qcow2 file without first
decrypting it, as both the header and payload are encrypted.

If you layer LUKS over qcow2 (eg luks -> qcow2 -> file) then you
cannot distinguish between a qcow2 file where the guest has done
LUKS encryption from a qcow2 file which qemu has done encryption.
More seriously, when encrypting sectors the guest virtual sector
is used as the input for deriving the initialization vectors.
When internal snapshots are used, this means that multiple sectors
in the qcow2 file may be encrypted with the same initialization
vector. This is a security weakness when combined with certain
cryptographic modes.

Integrating LUKS natively into qcow2 allows us to combine the
best aspects of both layering strategies above. In particular
the header remains unecrypted, but initialization vectors are
generated using physical sector numbers preserving security
when snapshots are used. This is a change from previous postings
of this work, where the IVs were (incorrectly) generated based
on the virtual disk sector.

In a previous posting of this work, Fam had suggested that we
do integration by layering luks over qcow2, but having QEMU
block layer automatically create the luks driver above qcow2
based on the qcow2 header crypt_method field. This is not
possible though, because such a scheme would suffer from the
problem of IVs being generated from the virtual disk sector
instead of physical disk sector. So having LUKS specific
code in the qcow2 block driver is unavoidable. In comparison
to the previous posting though, the amount of code in qcow2.c
has been reduced by allowing re-use of code from block/crypto.c
for handling QemuOpts -> QAPI conversion. So extra lines of
code in qcow2 to support LUKS is < 200.

I have also split the changes to qcow2 up into 2 patches. The
first patch simply introduces use of the QCryptoBlock framework
to qcow2 for the existing (deprecated) AES-CBC encryption method.
The second patch wires up the LUKS support for qcow2. This makes
it clearer which parts of the changes are related to plain code
refactoring, vs enabling the new features. Specifically we can
now see that the LUKS enablement in qcow2 has this footprint:

Changed in v9:

 - Ensure encryption=off conflicts with encryption.format=XXX (Max)
 - Fix mistaken syntax in commit message examples (Max)
 - Fix typo in spec (Eric)
 - Ensure variable decl is at start of method (Max)
 - Use abort() instead of assert(false)  (Max)

Changed in v8:

 - Fix leak of encryptopts in qcow driver (Alberto)
 - Remove some error_propagate calls (Alberto)
 - Clarify payload offset in spec (Eric)
 - Mention AES deprecation in spec (Eric)
 - Misc typos in spec (Eric)
 - Use error_abort querying specific info (Eric)
 - Document 'encrypt' qapi field (Eric)
 - Resolve conflict in iotests 087

Changed in v7:

 - Add encryption info to 'qemu-img info' output
 - List new encryption parameters in QEMU manual docs.
 - Extend copyright date to include 2017 (Eric)
 - Avoid local error object when not needed (Alberto)
 - Ensure to set 'ret' to an errno value (Alberto)
 - Fix leak of crypto options in qcow (Alberto)
 - Use american spelling of 'favor' (Eric)
 - Fix encryption format name in qapi (Alberto)
 - Fix incorrect option name prefix
 - Rename new iotests to avoid clash

Changed in v6:

 - Changed QAPI / QemuOpts design to use nested struct/union
   for all encryption parameters (Eric/Kevin)
 - Fix cleanup during error conditions (Alberto)

Changed in v5:

 - Remove accidental use of tabs in spec (Alberto)
 - Clarify payload-offset position semantics (Alberto)
 - Fix leak of QemuOpts in qcow v1 (Alberto)
 - Avoid overwriting existing Error ** (Alberto)
 - Reuse string -> enum convertor for qcow2 encryption format (Alberto)
 - Fix 

Re: [Qemu-block] [PATCH v2 00/31] qed: Convert to coroutines

[Stefan Hajnoczi]

On Fri, Jun 16, 2017 at 07:36:45PM +0200, Kevin Wolf wrote:
> The qed block driver is one of the last remaining block drivers that use the
> AIO callback style interfaces. This series converts it to the coroutine model
> that other drivers are using and removes some AIO functions from the block
> layer API afterwards.
> 
> If this isn't compelling enough, the diffstat should speak for itself.
> 
> This series is relatively long, but it consists mostly of mechanical
> conversions of one function per patch, so it should be easy to review.
> 
> v2:
> - Add coroutine_fn markers [Stefan, Paolo]
> - Use bdrv_co_*() instead of bdrv_*() in coroutine_fns
> - Use ACB on stack in qed_co_request [Paolo]
> - Updated some comments [Paolo]
> - Unplug earlier in qed_clear_need_check() [Stefan]
> - Removed now unused trace events [Stefan]
> - Improved commit message of patch creating qed_aio_write_cow() [Eric]
> 
> Kevin Wolf (31):
>   qed: Use bottom half to resume waiting requests
>   qed: Make qed_read_table() synchronous
>   qed: Remove callback from qed_read_table()
>   qed: Remove callback from qed_read_l2_table()
>   qed: Remove callback from qed_find_cluster()
>   qed: Make qed_read_backing_file() synchronous
>   qed: Make qed_copy_from_backing_file() synchronous
>   qed: Remove callback from qed_copy_from_backing_file()
>   qed: Make qed_write_header() synchronous
>   qed: Remove callback from qed_write_header()
>   qed: Make qed_write_table() synchronous
>   qed: Remove GenericCB
>   qed: Remove callback from qed_write_table()
>   qed: Make qed_aio_read_data() synchronous
>   qed: Make qed_aio_write_main() synchronous
>   qed: Inline qed_commit_l2_update()
>   qed: Add return value to qed_aio_write_l1_update()
>   qed: Add return value to qed_aio_write_l2_update()
>   qed: Add return value to qed_aio_write_main()
>   qed: Add return value to qed_aio_write_cow()
>   qed: Add return value to qed_aio_write_inplace/alloc()
>   qed: Add return value to qed_aio_read/write_data()
>   qed: Remove ret argument from qed_aio_next_io()
>   qed: Remove recursion in qed_aio_next_io()
>   qed: Implement .bdrv_co_readv/writev
>   qed: Use CoQueue for serialising allocations
>   qed: Simplify request handling
>   qed: Use a coroutine for need_check_timer
>   qed: Add coroutine_fn to I/O path functions
>   qed: Use bdrv_co_* for coroutine_fns
>   block: Remove bdrv_aio_readv/writev/flush()
> 
>  block/Makefile.objs   |   2 +-
>  block/io.c| 171 ---
>  block/qed-cluster.c   | 124 
>  block/qed-gencb.c |  33 ---
>  block/qed-table.c | 261 ++---
>  block/qed.c   | 773 
> +++---
>  block/qed.h   |  54 +---
>  block/trace-events|   3 -
>  include/block/block.h |   8 -
>  9 files changed, 438 insertions(+), 991 deletions(-)
>  delete mode 100644 block/qed-gencb.c
> 
> -- 
> 1.8.3.1
> 

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img: don't shadow opts variable in img_dd()

[Max Reitz]

On 2017-06-19 17:00, Stefan Hajnoczi wrote:
> It's confusing when two different variables have the same name in one
> function.
> 
> Cc: Reda Sallahi 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  qemu-img.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 0ad698d..c285c2f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4249,15 +4249,12 @@ static int img_dd(int argc, char **argv)
>  case 'U':
>  force_share = true;
>  break;
> -case OPTION_OBJECT: {
> -QemuOpts *opts;
> -opts = qemu_opts_parse_noisily(_object_opts,
> -   optarg, true);
> -if (!opts) {
> +case OPTION_OBJECT:
> +if (!qemu_opts_parse_noisily(_object_opts, optarg, true)) {
>  ret = -1;
>  goto out;
>  }
> -}   break;
> +break;
>  case OPTION_IMAGE_OPTS:
>  image_opts = true;
>  break;

Hm, I basically reverted such a style in commit
3258b91141090b05edcaab8f1d1dd355ca91b49a. I find it confusing to use the
same variable for two different things.

I agree that shadowing is bad, though. How about just renaming this
variable here?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 00/16] block: Preallocated truncate

[Stefan Hajnoczi]

On Tue, Jun 13, 2017 at 10:20:51PM +0200, Max Reitz wrote:
> === Series dependencies ===
> 
> This series depends on v7 of Stefan's series
> "qemu-img: add measure sub-command"
> (http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg03035.html).
> 
> 
> === Actual cover letter ===
> 
> This series adds preallocation to bdrv_truncate() and subsequently
> qemu-img resize. This is implemented for qcow2 and raw only, just like
> preallocation for newly created images is. There is no runtime interface
> for this new parameter (yet).
> 
> 
> v4:
> - Patch 4:
>   - Note bug fix in commit message [Eric]
>   - "...may result in *slightly* more data being allocated..." [Eric]
> - Patch 11:
>   - Drop the "image may now occupy more space than necessary" note --
> that's always the case when preallocation failed, and I don't really
> want to mention it everywhere
>   - Flush metadata after preallocation [Stefan]
> 
> 
> git-backport-diff against v3:
> 
> Key:
> [] : patches are identical
> [] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, 
> respectively
> 
> 001/16:[] [--] 'block: Add PreallocMode to BD.bdrv_truncate()'
> 002/16:[] [--] 'block: Add PreallocMode to bdrv_truncate()'
> 003/16:[] [--] 'block: Add PreallocMode to blk_truncate()'
> 004/16:[0002] [FC] 'qemu-img: Expose PreallocMode for resizing'
> 005/16:[] [--] 'block/file-posix: Small fixes in raw_create()'
> 006/16:[] [--] 'block/file-posix: Extract raw_regular_truncate()'
> 007/16:[] [--] 'block/file-posix: Generalize raw_regular_truncate'
> 008/16:[] [--] 'block/file-posix: Preallocation for truncate'
> 009/16:[] [--] 'block/qcow2: Generalize preallocate()'
> 010/16:[] [--] 'block/qcow2: Lock s->lock in preallocate()'
> 011/16:[0013] [FC] 'block/qcow2: Metadata preallocation for truncate'
> 012/16:[] [--] 'block/qcow2: Add qcow2_refcount_area()'
> 013/16:[] [--] 'block/qcow2: Rename "fail_block" to just "fail"'
> 014/16:[] [--] 'block/qcow2: falloc/full preallocating growth'
> 015/16:[] [--] 'iotests: Add preallocated resize test for raw'
> 016/16:[] [--] 'iotests: Add preallocated growth test for qcow2'
> 
> 
> Max Reitz (16):
>   block: Add PreallocMode to BD.bdrv_truncate()
>   block: Add PreallocMode to bdrv_truncate()
>   block: Add PreallocMode to blk_truncate()
>   qemu-img: Expose PreallocMode for resizing
>   block/file-posix: Small fixes in raw_create()
>   block/file-posix: Extract raw_regular_truncate()
>   block/file-posix: Generalize raw_regular_truncate
>   block/file-posix: Preallocation for truncate
>   block/qcow2: Generalize preallocate()
>   block/qcow2: Lock s->lock in preallocate()
>   block/qcow2: Metadata preallocation for truncate
>   block/qcow2: Add qcow2_refcount_area()
>   block/qcow2: Rename "fail_block" to just "fail"
>   block/qcow2: falloc/full preallocating growth
>   iotests: Add preallocated resize test for raw
>   iotests: Add preallocated growth test for qcow2
> 
>  block/qcow2.h  |   9 +
>  include/block/block.h  |   3 +-
>  include/block/block_int.h  |   3 +-
>  include/sysemu/block-backend.h |   3 +-
>  block.c|   5 +-
>  block/blkdebug.c   |   5 +-
>  block/block-backend.c  |   5 +-
>  block/commit.c |   4 +-
>  block/crypto.c |   4 +-
>  block/file-posix.c | 201 +
>  block/file-win32.c |   9 +-
>  block/gluster.c|   8 +-
>  block/iscsi.c  |   9 +-
>  block/mirror.c |   3 +-
>  block/nfs.c|   9 +-
>  block/parallels.c  |  13 +-
>  block/qcow.c   |   8 +-
>  block/qcow2-refcount.c | 277 +++--
>  block/qcow2.c  | 205 +++---
>  block/qed.c|  11 +-
>  block/raw-format.c |   5 +-
>  block/rbd.c|   9 +-
>  block/sheepdog.c   |  11 +-
>  block/vdi.c|   3 +-
>  block/vhdx-log.c   |   2 +-
>  block/vhdx.c   |   8 +-
>  block/vmdk.c   |   7 +-
>  block/vpc.c|   2 +-
>  blockdev.c |   2 +-
>  qemu-img.c |  33 +++-
>  qemu-io-cmds.c |   2 +-
>  qemu-img.texi  |   7 +-
>  tests/qemu-iotests/044.out |   2 +-
>  tests/qemu-iotests/106 |  92 ++
>  tests/qemu-iotests/106.out |  50 ++
>  tests/qemu-iotests/125 | 130 ++
>  tests/qemu-iotests/125.out | 386 
> +
>  tests/qemu-iotests/group   |   2 +
>  38 files changed, 1313 insertions(+), 234 deletions(-)
>  create mode 100755 tests/qemu-iotests/106
>  create mode 

[Qemu-block] [PATCH] qemu-img: don't shadow opts variable in img_dd()

[Stefan Hajnoczi]

It's confusing when two different variables have the same name in one
function.

Cc: Reda Sallahi 
Signed-off-by: Stefan Hajnoczi 
---
 qemu-img.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 0ad698d..c285c2f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4249,15 +4249,12 @@ static int img_dd(int argc, char **argv)
 case 'U':
 force_share = true;
 break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(_object_opts,
-   optarg, true);
-if (!opts) {
+case OPTION_OBJECT:
+if (!qemu_opts_parse_noisily(_object_opts, optarg, true)) {
 ret = -1;
 goto out;
 }
-}   break;
+break;
 case OPTION_IMAGE_OPTS:
 image_opts = true;
 break;
-- 
2.9.4

Re: [Qemu-block] [PATCH 2/4] qapi: Add qobject_is_equal()

[Max Reitz]

On 2017-06-19 15:55, Kevin Wolf wrote:
> Am 14.06.2017 um 17:31 hat Max Reitz geschrieben:
>> This generic function (along with its implementations for different
>> types) determines whether two QObjects are equal.
>>
>> Signed-off-by: Max Reitz 
> 
>> diff --git a/qobject/qdict.c b/qobject/qdict.c
>> index 88e2ecd..2ed57ca 100644
>> --- a/qobject/qdict.c
>> +++ b/qobject/qdict.c
>> @@ -410,6 +410,30 @@ void qdict_del(QDict *qdict, const char *key)
>>  }
>>  
>>  /**
>> + * qdict_is_equal(): Tests whether the two QDicts are equal
> 
> This should specify what equality means for QDicts: Do their entries
> have to point to the same objects, or is it enough if the objects
> compare equal? It seems you're trying to implement the lattter.

Yes, I'll add a comment.

>> + */
>> +bool qdict_is_equal(const QObject *x, const QObject *y)
>> +{
>> +const QDict *dict_x = qobject_to_qdict(x), *dict_y = 
>> qobject_to_qdict(y);
>> +const QDictEntry *e;
>> +
>> +if (qdict_size(dict_x) != qdict_size(dict_y)) {
>> +return false;
>> +}
>> +
>> +for (e = qdict_first(dict_x); e; e = qdict_next(dict_x, e)) {
>> +const QObject *obj_x = qdict_entry_value(e);
>> +const QObject *obj_y = qdict_get(dict_y, qdict_entry_key(e));
>> +
>> +if (!qobject_is_equal(obj_x, obj_y)) {
>> +return false;
>> +}
>> +}
>> +
>> +return true;
>> +}
> 
> If I'm not mistaken, this doesn't actually check equality, but that the
> entries of x are a subset of the entries of y.

That's why I'm checking that x and y have the same size before.

>> +/**
>>   * qdict_destroy_obj(): Free all the memory allocated by a QDict
>>   */
>>  void qdict_destroy_obj(QObject *obj)
>> diff --git a/qobject/qlist.c b/qobject/qlist.c
>> index 86b60cb..b3033c6 100644
>> --- a/qobject/qlist.c
>> +++ b/qobject/qlist.c
>> @@ -140,6 +140,31 @@ QList *qobject_to_qlist(const QObject *obj)
>>  }
>>  
>>  /**
>> + * qlist_is_equal(): Tests whether the two QLists are equal
> 
> Like for QDict, this isn't a complete description.

Yes, I'll add a comment here, too.

> Your implementation seems to check that both lists contain objects that
> compare equal at each index (i.e. order matters).

Of course it does because lists are not sets but arrays (in contrast to
dicts).

Max

>> + */
>> +bool qlist_is_equal(const QObject *x, const QObject *y)
>> +{
>> +const QList *list_x = qobject_to_qlist(x), *list_y = 
>> qobject_to_qlist(y);
>> +const QListEntry *entry_x, *entry_y;
>> +
>> +entry_x = qlist_first(list_x);
>> +entry_y = qlist_first(list_y);
>> +
>> +while (entry_x && entry_y) {
>> +if (!qobject_is_equal(qlist_entry_obj(entry_x),
>> +  qlist_entry_obj(entry_y)))
>> +{
>> +return false;
>> +}
>> +
>> +entry_x = qlist_next(entry_x);
>> +entry_y = qlist_next(entry_y);
>> +}
>> +
>> +return !entry_x && !entry_y;
>> +}
>> +
>> +/**
>>   * qlist_destroy_obj(): Free all the memory allocated by a QList
>>   */
> 
> Kevin
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 4/4] iotests: Add test for non-string option reopening

[Kevin Wolf]

Am 14.06.2017 um 17:31 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH 3/4] block: qobject_is_equal() in bdrv_reopen_prepare()

[Kevin Wolf]

Am 14.06.2017 um 17:31 hat Max Reitz geschrieben:
> Currently, bdrv_reopen_prepare() assumes that all BDS options are
> strings. However, this is not the case if the BDS has been created
> through the json: pseudo-protocol or blockdev-add.
> 
> Note that the user-invokable reopen command is an HMP command, so you
> can only specify strings there. Therefore, specifying a non-string
> option with the "same" value as it was when originally created will now
> return an error because the values are supposedly similar (and there is
> no way for the user to circumvent this but to just not specify the
> option again -- however, this is still strictly better than just
> crashing).
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v8 19/20] qcow2: report encryption specific image information

[Daniel P. Berrange]

On Wed, Jun 07, 2017 at 07:38:44PM +0200, Max Reitz wrote:
> On 2017-06-01 19:27, Daniel P. Berrange wrote:
> > Currently 'qemu-img info' reports a simple "encrypted: yes"
> > field. This is not very useful now that qcow2 can support
> > multiple encryption formats. Users want to know which format
> > is in use and some data related to it.
> > 
> > Wire up usage of the qcrypto_block_get_info() method so that
> > 'qemu-img info' can report about the encryption format
> > and parameters in use
> > 
> >   $ qemu-img create \
> >   --object secret,id=sec0,data=123456 \
> >   -o encrypt.format=luks,encrypt.key-secret=sec0 \
> >   -f qcow2 demo.qcow2 1G
> >   Formatting 'demo.qcow2', fmt=qcow2 size=1073741824 \
> >   encryption=off encrypt.format=luks encrypt.key-secret=sec0 \
> >   cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > 
> >   $ qemu-img info demo.qcow2
> >   image: demo.qcow2
> >   file format: qcow2
> >   virtual size: 1.0G (1073741824 bytes)
> >   disk size: 480K
> >   encrypted: yes
> >   cluster_size: 65536
> >   Format specific information:
> >   compat: 1.1
> >   lazy refcounts: false
> >   refcount bits: 16
> >   encrypt:
> >   ivgen alg: plain64
> >   hash alg: sha256
> >   cipher alg: aes-256
> >   uuid: 3fa930c4-58c8-4ef7-b3c5-314bb5af21f3
> >   format: luks
> >   cipher mode: xts
> >   slots:
> >   [0]:
> >   active: true
> >   iters: 1839058
> >   key offset: 4096
> >   stripes: 4000
> >   [1]:
> >   active: false
> >   key offset: 262144
> >   [2]:
> >   active: false
> >   key offset: 520192
> >   [3]:
> >   active: false
> >   key offset: 778240
> >   [4]:
> >   active: false
> >   key offset: 1036288
> >   [5]:
> >   active: false
> >   key offset: 1294336
> >   [6]:
> >   active: false
> >   key offset: 1552384
> >   [7]:
> >   active: false
> >   key offset: 1810432
> >   payload offset: 2068480
> >   master key iters: 438487
> >   corrupt: false
> > 
> > With the legacy "AES" encryption we just report the format
> > name
> > 
> >   $ qemu-img create \
> >   --object secret,id=sec0,data=123456 \
> >   -o encrypt.format=aes,encrypt.key-secret=sec0 \
> >   -f qcow2 demo.qcow2 1G
> >   Formatting 'demo.qcow2', fmt=qcow2 size=1073741824 \
> >   encryption=off encrypt.format=aes encrypt.key-secret=sec0 \
> >   cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > 
> >   $ ./qemu-img info demo.qcow2
> >   image: demo.qcow2
> >   file format: qcow2
> >   virtual size: 1.0G (1073741824 bytes)
> >   disk size: 196K
> >   encrypted: yes
> >   cluster_size: 65536
> >   Format specific information:
> >   compat: 1.1
> >   lazy refcounts: false
> >   refcount bits: 16
> >   encrypt:
> >   format: aes
> >   corrupt: false
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block/qcow2.c| 32 +++-
> >  qapi/block-core.json | 27 ++-
> >  2 files changed, 57 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 58da658..a8a23af 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> 
> [...]
> 
> > @@ -3224,6 +3230,30 @@ static ImageInfoSpecific 
> > *qcow2_get_specific_info(BlockDriverState *bs)
> >  assert(false);
> >  }
> >  
> > +if (encrypt_info) {
> > +ImageInfoSpecificQCow2Encryption *qencrypt =
> > +g_new(ImageInfoSpecificQCow2Encryption, 1);
> > +switch (encrypt_info->format) {
> > +case Q_CRYPTO_BLOCK_FORMAT_QCOW:
> > +qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_AES;
> > +qencrypt->u.aes = encrypt_info->u.qcow;
> > +break;
> > +case Q_CRYPTO_BLOCK_FORMAT_LUKS:
> > +qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_LUKS;
> > +qencrypt->u.luks = encrypt_info->u.luks;
> > +break;
> > +default:
> > +assert(false);
> 
> I'd rather like this to be either a plain abort() or a
> g_asert_not_reached(); the latter is more expressive, and the former
> will work even with NDEBUG.

Its very annoying that g_assert_not_reached() can be turned into
a no-op as that would lead to bad code, so I'll make it abort().

> I know we already have an assert(false) in this function, but I'd assert
> this is just wrong.

Agreed.

> With this changed (or with me convinced that we should just use
> assert(false)):
> 
> Reviewed-by: Max Reitz 
> 



Regards,
Daniel
-- 
|: 

Re: [Qemu-block] [PATCH v8 13/20] qcow2: add support for LUKS encryption format

[Daniel P. Berrange]

On Wed, Jun 07, 2017 at 07:19:28PM +0200, Max Reitz wrote:
> On 2017-06-01 19:27, Daniel P. Berrange wrote:
> > This adds support for using LUKS as an encryption format
> > with the qcow2 file, using the new encrypt.format parameter
> > to request "luks" format. e.g.
> > 
> >   # qemu-img create --object secret,data=123456,id=sec0 \
> >-f qcow2 -o encrypt.format=luks,encrypt.key-secret=sec0 \
> >test.qcow2 10G
> > 
> > The legacy "encryption=on" parameter still results in
> > creation of the old qcow2 AES format (and is equivalent
> > to the new 'encryption-format=aes'). e.g. the following are
> > equivalent:
> > 
> >   # qemu-img create --object secret,data=123456,id=sec0 \
> >-f qcow2 -o encryption=on,encrypt.key-secret=sec0 \
> >test.qcow2 10G
> > 
> >  # qemu-img create --object secret,data=123456,id=sec0 \
> >-f qcow2 -o encryption-format=aes,encrypt.key-secret=sec0 \
> >test.qcow2 10G
> > 
> > With the LUKS format it is necessary to store the LUKS
> > partition header and key material in the QCow2 file. This
> > data can be many MB in size, so cannot go into the QCow2
> > header region directly. Thus the spec defines a FDE
> > (Full Disk Encryption) header extension that specifies
> > the offset of a set of clusters to hold the FDE headers,
> > as well as the length of that region. The LUKS header is
> > thus stored in these extra allocated clusters before the
> > main image payload.
> > 
> > Aside from all the cryptographic differences implied by
> > use of the LUKS format, there is one further key difference
> > between the use of legacy AES and LUKS encryption in qcow2.
> > For LUKS, the initialiazation vectors are generated using
> > the host physical sector as the input, rather than the
> > guest virtual sector. This guarantees unique initialization
> > vectors for all sectors when qcow2 internal snapshots are
> > used, thus giving stronger protection against watermarking
> > attacks.
> > 
> > Reviewed-by: Eric Blake 
> > Reviewed-by: Alberto Garcia 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block/qcow2-cluster.c  |   4 +-
> >  block/qcow2-refcount.c |  10 ++
> >  block/qcow2.c  | 267 
> > ++--
> >  block/qcow2.h  |   9 ++
> >  qapi/block-core.json   |   5 +-
> >  tests/qemu-iotests/082.out | 270 
> > -
> >  6 files changed, 477 insertions(+), 88 deletions(-)
> 
> [...]
> 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 38c0420..30d0343 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> 
> [...]
> 
> > @@ -2072,22 +2253,30 @@ static int qcow2_set_up_encryption(BlockDriverState 
> > *bs, const char *encryptfmt,
> >  qdict_extract_subqdict(options, , "encrypt.");
> >  QDECREF(options);
> >  
> > -if (!g_str_equal(encryptfmt, "aes")) {
> > -error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
> > -   encryptfmt);
> > -ret = -EINVAL;
> > -goto out;
> > +int fmt = qcow2_crypt_method_from_format(encryptfmt);
> 
> Because I love nit picking: Variable declarations should be at the start
> of the surrounding block.

Opps, dunno what I was thinking there.

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-block] [PATCH v8 09/20] qcow: convert QCow to use QCryptoBlock for encryption

[Daniel P. Berrange]

On Wed, Jun 07, 2017 at 06:55:39PM +0200, Max Reitz wrote:
> On 2017-06-01 19:27, Daniel P. Berrange wrote:
> > This converts the qcow driver to make use of the QCryptoBlock
> > APIs for encrypting image content. This is only wired up to
> > permit use of the legacy QCow encryption format. Users who wish
> > to have the strong LUKS format should switch to qcow2 instead.
> > 
> > With this change it is now required to use the QCryptoSecret
> > object for providing passwords, instead of the current block
> > password APIs / interactive prompting.
> > 
> 
> Beware, nit picks incoming:
> 
> >   $QEMU \
> > -object secret,id=sec0,filename=/home/berrange/encrypted.pw \> 
> > -drive file=/home/berrange/encrypted.qcow,encrypt.format=qcow,\
> 
> encrypt.format should be "aes".
> 
> >encrypt.key-secret=sec0
> 
> This doesn't work at all, though, because:
> 
> Use of AES-CBC encrypted qcow images is no longer supported in system
> emulators
> You can use 'qemu-img convert' to convert your image to an alternative
> supported format, such as unencrypted qcow, or raw with the LUKS format
> instead.

Good point. I'll leave this example here, since it is
useful to illustrate the overall syntax approach, but
I'll add a note that this example won't let you run
the VM

> > Likewise when creating images with the legacy AES-CBC format
> > 
> >   qemu-img create -f qcow \
> > -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
> 
> Should be --object.

Yep

> 
> > -o encrypt.format=aes,encrypt.key-secret=sec0 \
> > /home/berrange/encrypted.qcow
> 
> There should be a size here to make it work.

Ok


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-block] [PATCH v8 07/20] block: deprecate "encryption=on" in favor of "encrypt.format=aes"

[Daniel P. Berrange]

On Wed, Jun 07, 2017 at 06:40:32PM +0200, Max Reitz wrote:
> On 2017-06-01 19:27, Daniel P. Berrange wrote:
> > Historically the qcow & qcow2 image formats supported a property
> > "encryption=on" to enable their built-in AES encryption. We'll
> > soon be supporting LUKS for qcow2, so need a more general purpose
> > way to enable encryption, with a choice of formats.
> > 
> > This introduces an "encrypt.format" option, which will later be
> > joined by a number of other "encrypt.XXX" options. The use of
> > a "encrypt." prefix instead of "encrypt-" is done to facilitate
> > mapping to a nested QAPI schema at later date.
> > 
> > e.g. the preferred syntax is now
> > 
> >   qemu-img create -f qcow2 -o encrypt.format=aes demo.qcow2
> > 
> > Reviewed-by: Eric Blake 
> > Reviewed-by: Alberto Garcia 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block/qcow.c   | 30 ++---
> >  block/qcow2.c  | 33 +++
> >  include/block/block_int.h  |  2 +-
> >  qemu-img.c |  4 ++-
> >  tests/qemu-iotests/082.out | 81 
> > ++
> >  5 files changed, 110 insertions(+), 40 deletions(-)
> > 
> > diff --git a/block/qcow.c b/block/qcow.c
> > index 6738bc7..42f83b2 100644
> > --- a/block/qcow.c
> > +++ b/block/qcow.c
> 
> [...]
> 
> > @@ -818,8 +818,16 @@ static int qcow_create(const char *filename, QemuOpts 
> > *opts, Error **errp)
> >  }
> >  
> >  backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> > -if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
> > -flags |= BLOCK_FLAG_ENCRYPT;
> > +encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
> > +if (encryptfmt) {
> > +if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
> 
> You should probably just use qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT)
> here, because otherwise you can do this:
> 
> $ ./qemu-img create -f qcow -o encryption=off,encrypt.format=aes \
> foo.qcow 64M
> Formatting 'foo.qcow', fmt=qcow size=67108864 encryption=off
> encrypt.format=aes

Yes, will fix it as you suggest.


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-block] [PATCH 2/4] qapi: Add qobject_is_equal()

[Kevin Wolf]

Am 14.06.2017 um 17:31 hat Max Reitz geschrieben:
> This generic function (along with its implementations for different
> types) determines whether two QObjects are equal.
> 
> Signed-off-by: Max Reitz 

> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 88e2ecd..2ed57ca 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -410,6 +410,30 @@ void qdict_del(QDict *qdict, const char *key)
>  }
>  
>  /**
> + * qdict_is_equal(): Tests whether the two QDicts are equal

This should specify what equality means for QDicts: Do their entries
have to point to the same objects, or is it enough if the objects
compare equal? It seems you're trying to implement the lattter.

> + */
> +bool qdict_is_equal(const QObject *x, const QObject *y)
> +{
> +const QDict *dict_x = qobject_to_qdict(x), *dict_y = qobject_to_qdict(y);
> +const QDictEntry *e;
> +
> +if (qdict_size(dict_x) != qdict_size(dict_y)) {
> +return false;
> +}
> +
> +for (e = qdict_first(dict_x); e; e = qdict_next(dict_x, e)) {
> +const QObject *obj_x = qdict_entry_value(e);
> +const QObject *obj_y = qdict_get(dict_y, qdict_entry_key(e));
> +
> +if (!qobject_is_equal(obj_x, obj_y)) {
> +return false;
> +}
> +}
> +
> +return true;
> +}

If I'm not mistaken, this doesn't actually check equality, but that the
entries of x are a subset of the entries of y.

> +/**
>   * qdict_destroy_obj(): Free all the memory allocated by a QDict
>   */
>  void qdict_destroy_obj(QObject *obj)
> diff --git a/qobject/qlist.c b/qobject/qlist.c
> index 86b60cb..b3033c6 100644
> --- a/qobject/qlist.c
> +++ b/qobject/qlist.c
> @@ -140,6 +140,31 @@ QList *qobject_to_qlist(const QObject *obj)
>  }
>  
>  /**
> + * qlist_is_equal(): Tests whether the two QLists are equal

Like for QDict, this isn't a complete description.

Your implementation seems to check that both lists contain objects that
compare equal at each index (i.e. order matters).

> + */
> +bool qlist_is_equal(const QObject *x, const QObject *y)
> +{
> +const QList *list_x = qobject_to_qlist(x), *list_y = qobject_to_qlist(y);
> +const QListEntry *entry_x, *entry_y;
> +
> +entry_x = qlist_first(list_x);
> +entry_y = qlist_first(list_y);
> +
> +while (entry_x && entry_y) {
> +if (!qobject_is_equal(qlist_entry_obj(entry_x),
> +  qlist_entry_obj(entry_y)))
> +{
> +return false;
> +}
> +
> +entry_x = qlist_next(entry_x);
> +entry_y = qlist_next(entry_y);
> +}
> +
> +return !entry_x && !entry_y;
> +}
> +
> +/**
>   * qlist_destroy_obj(): Free all the memory allocated by a QList
>   */

Kevin

[Qemu-block] [PATCH v4 6/7] qcow2: Pass a QEMUIOVector to do_perform_cow_{read, write}()

[Alberto Garcia]

Instead of passing a single buffer pointer to do_perform_cow_write(),
pass a QEMUIOVector. This will allow us to merge the write requests
for the COW regions and the actual data into a single one.

Although do_perform_cow_read() does not strictly need to change its
API, we're doing it here as well for consistency.

Signed-off-by: Alberto Garcia 
Reviewed-by: Kevin Wolf 
---
 block/qcow2-cluster.c | 51 ---
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 20fb531932..3ac26d6bf7 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -406,19 +406,14 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
 static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
 uint64_t src_cluster_offset,
 unsigned offset_in_cluster,
-uint8_t *buffer,
-unsigned bytes)
+QEMUIOVector *qiov)
 {
-QEMUIOVector qiov;
-struct iovec iov = { .iov_base = buffer, .iov_len = bytes };
 int ret;
 
-if (bytes == 0) {
+if (qiov->size == 0) {
 return 0;
 }
 
-qemu_iovec_init_external(, , 1);
-
 BLKDBG_EVENT(bs->file, BLKDBG_COW_READ);
 
 if (!bs->drv) {
@@ -430,7 +425,7 @@ static int coroutine_fn 
do_perform_cow_read(BlockDriverState *bs,
  * which can lead to deadlock when block layer copy-on-read is enabled.
  */
 ret = bs->drv->bdrv_co_preadv(bs, src_cluster_offset + offset_in_cluster,
-  bytes, , 0);
+  qiov->size, qiov, 0);
 if (ret < 0) {
 return ret;
 }
@@ -462,28 +457,23 @@ static bool coroutine_fn 
do_perform_cow_encrypt(BlockDriverState *bs,
 static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
  uint64_t cluster_offset,
  unsigned offset_in_cluster,
- uint8_t *buffer,
- unsigned bytes)
+ QEMUIOVector *qiov)
 {
-QEMUIOVector qiov;
-struct iovec iov = { .iov_base = buffer, .iov_len = bytes };
 int ret;
 
-if (bytes == 0) {
+if (qiov->size == 0) {
 return 0;
 }
 
-qemu_iovec_init_external(, , 1);
-
 ret = qcow2_pre_write_overlap_check(bs, 0,
-cluster_offset + offset_in_cluster, bytes);
+cluster_offset + offset_in_cluster, qiov->size);
 if (ret < 0) {
 return ret;
 }
 
 BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
 ret = bdrv_co_pwritev(bs->file, cluster_offset + offset_in_cluster,
-  bytes, , 0);
+  qiov->size, qiov, 0);
 if (ret < 0) {
 return ret;
 }
@@ -780,6 +770,7 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
 unsigned data_bytes = end->offset - (start->offset + start->nb_bytes);
 bool merge_reads;
 uint8_t *start_buffer, *end_buffer;
+QEMUIOVector qiov;
 int ret;
 
 assert(start->nb_bytes <= UINT_MAX - end->nb_bytes);
@@ -816,22 +807,25 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
*m)
 /* The part of the buffer where the end region is located */
 end_buffer = start_buffer + buffer_size - end->nb_bytes;
 
+qemu_iovec_init(, 1);
+
 qemu_co_mutex_unlock(>lock);
 /* First we read the existing data from both COW regions. We
  * either read the whole region in one go, or the start and end
  * regions separately. */
 if (merge_reads) {
-ret = do_perform_cow_read(bs, m->offset, start->offset,
-  start_buffer, buffer_size);
+qemu_iovec_add(, start_buffer, buffer_size);
+ret = do_perform_cow_read(bs, m->offset, start->offset, );
 } else {
-ret = do_perform_cow_read(bs, m->offset, start->offset,
-  start_buffer, start->nb_bytes);
+qemu_iovec_add(, start_buffer, start->nb_bytes);
+ret = do_perform_cow_read(bs, m->offset, start->offset, );
 if (ret < 0) {
 goto fail;
 }
 
-ret = do_perform_cow_read(bs, m->offset, end->offset,
-  end_buffer, end->nb_bytes);
+qemu_iovec_reset();
+qemu_iovec_add(, end_buffer, end->nb_bytes);
+ret = do_perform_cow_read(bs, m->offset, end->offset, );
 }
 if (ret < 0) {
 goto fail;
@@ -849,14 +843,16 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
*m)
 }
 
 /* And now we can write everything */
-ret = do_perform_cow_write(bs, m->alloc_offset, start->offset,
-   

[Qemu-block] [PATCH v4 0/7] Reduce the number of I/O ops when doing COW

[Alberto Garcia]

Hi all,

here's a patch series that rewrites the copy-on-write code in the
qcow2 driver to reduce the number of I/O operations.

This is version v4, please refer to the original e-mail for a complete
description:

https://lists.gnu.org/archive/html/qemu-block/2017-05/msg00882.html

Regards,

Berto

v4:
- Patch 6: Initialize the QEMUIOVector with only 1 iovecs [Kevin]
- Patch 7: Initialize the QEMUIOVector with 2 + m->data_qiov->niov [Kevin]
- Patch 7: Rename can_merge_cow() to merge_cow() [Kevin]

v3: https://lists.gnu.org/archive/html/qemu-block/2017-06/msg00472.html
- Patch 4: Over-allocate the read buffer in perform_cow() so both COW
   sectors are optimally aligned [Eric]

v2: https://lists.gnu.org/archive/html/qemu-block/2017-06/msg00227.html
- Patch 1: Update commit message [Eric]
- Patch 7: Make sure that the number of iovs does not exceed IOV_MAX [Anton]
- Patch 7: Don't add zero-length buffers to the qiov in perform_cow()

v1: https://lists.gnu.org/archive/html/qemu-block/2017-05/msg00882.html
- Initial version

Output of git-backport-diff against v3:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/7:[] [--] 'qcow2: Remove unused Error variable in do_perform_cow()'
002/7:[] [--] 'qcow2: Use unsigned int for both members of Qcow2COWRegion'
003/7:[] [--] 'qcow2: Make perform_cow() call do_perform_cow() twice'
004/7:[] [--] 'qcow2: Split do_perform_cow() into _read(), _encrypt() and 
_write()'
005/7:[] [--] 'qcow2: Allow reading both COW regions with only one request'
006/7:[0002] [FC] 'qcow2: Pass a QEMUIOVector to do_perform_cow_{read,write}()'
007/7:[0008] [FC] 'qcow2: Merge the writing of the COW regions with the guest 
data'

Alberto Garcia (7):
  qcow2: Remove unused Error variable in do_perform_cow()
  qcow2: Use unsigned int for both members of Qcow2COWRegion
  qcow2: Make perform_cow() call do_perform_cow() twice
  qcow2: Split do_perform_cow() into _read(), _encrypt() and _write()
  qcow2: Allow reading both COW regions with only one request
  qcow2: Pass a QEMUIOVector to do_perform_cow_{read,write}()
  qcow2: Merge the writing of the COW regions with the guest data

 block/qcow2-cluster.c | 201 ++
 block/qcow2.c |  64 +---
 block/qcow2.h |  11 ++-
 3 files changed, 216 insertions(+), 60 deletions(-)

-- 
2.11.0

[Qemu-block] [PATCH v4 4/7] qcow2: Split do_perform_cow() into _read(), _encrypt() and _write()

[Alberto Garcia]

This patch splits do_perform_cow() into three separate functions to
read, encrypt and write the COW regions.

perform_cow() can now read both regions first, then encrypt them and
finally write them to disk. The memory allocation is also done in
this function now, using one single buffer large enough to hold both
regions.

Signed-off-by: Alberto Garcia 
Reviewed-by: Kevin Wolf 
---
 block/qcow2-cluster.c | 117 +-
 1 file changed, 87 insertions(+), 30 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4c03639a72..3c9ace8a96 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -403,34 +403,26 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
 return 0;
 }
 
-static int coroutine_fn do_perform_cow(BlockDriverState *bs,
-   uint64_t src_cluster_offset,
-   uint64_t cluster_offset,
-   unsigned offset_in_cluster,
-   unsigned bytes)
+static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
+uint64_t src_cluster_offset,
+unsigned offset_in_cluster,
+uint8_t *buffer,
+unsigned bytes)
 {
-BDRVQcow2State *s = bs->opaque;
 QEMUIOVector qiov;
-struct iovec iov;
+struct iovec iov = { .iov_base = buffer, .iov_len = bytes };
 int ret;
 
 if (bytes == 0) {
 return 0;
 }
 
-iov.iov_len = bytes;
-iov.iov_base = qemu_try_blockalign(bs, iov.iov_len);
-if (iov.iov_base == NULL) {
-return -ENOMEM;
-}
-
 qemu_iovec_init_external(, , 1);
 
 BLKDBG_EVENT(bs->file, BLKDBG_COW_READ);
 
 if (!bs->drv) {
-ret = -ENOMEDIUM;
-goto out;
+return -ENOMEDIUM;
 }
 
 /* Call .bdrv_co_readv() directly instead of using the public block-layer
@@ -440,39 +432,63 @@ static int coroutine_fn do_perform_cow(BlockDriverState 
*bs,
 ret = bs->drv->bdrv_co_preadv(bs, src_cluster_offset + offset_in_cluster,
   bytes, , 0);
 if (ret < 0) {
-goto out;
+return ret;
 }
 
-if (bs->encrypted) {
+return 0;
+}
+
+static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
+uint64_t src_cluster_offset,
+unsigned offset_in_cluster,
+uint8_t *buffer,
+unsigned bytes)
+{
+if (bytes && bs->encrypted) {
+BDRVQcow2State *s = bs->opaque;
 int64_t sector = (src_cluster_offset + offset_in_cluster)
  >> BDRV_SECTOR_BITS;
 assert(s->cipher);
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
-if (qcow2_encrypt_sectors(s, sector, iov.iov_base, iov.iov_base,
+if (qcow2_encrypt_sectors(s, sector, buffer, buffer,
   bytes >> BDRV_SECTOR_BITS, true, NULL) < 0) {
-ret = -EIO;
-goto out;
+return false;
 }
 }
+return true;
+}
+
+static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
+ uint64_t cluster_offset,
+ unsigned offset_in_cluster,
+ uint8_t *buffer,
+ unsigned bytes)
+{
+QEMUIOVector qiov;
+struct iovec iov = { .iov_base = buffer, .iov_len = bytes };
+int ret;
+
+if (bytes == 0) {
+return 0;
+}
+
+qemu_iovec_init_external(, , 1);
 
 ret = qcow2_pre_write_overlap_check(bs, 0,
 cluster_offset + offset_in_cluster, bytes);
 if (ret < 0) {
-goto out;
+return ret;
 }
 
 BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
 ret = bdrv_co_pwritev(bs->file, cluster_offset + offset_in_cluster,
   bytes, , 0);
 if (ret < 0) {
-goto out;
+return ret;
 }
 
-ret = 0;
-out:
-qemu_vfree(iov.iov_base);
-return ret;
+return 0;
 }
 
 
@@ -760,22 +776,62 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
*m)
 BDRVQcow2State *s = bs->opaque;
 Qcow2COWRegion *start = >cow_start;
 Qcow2COWRegion *end = >cow_end;
+unsigned buffer_size;
+uint8_t *start_buffer, *end_buffer;
 int ret;
 
+assert(start->nb_bytes <= UINT_MAX - end->nb_bytes);
+
 if (start->nb_bytes == 0 && end->nb_bytes == 0) {
 return 0;
 }
 
+/* Reserve a buffer large enough to store the data from both the
+ * 

[Qemu-block] [PATCH v4 7/7] qcow2: Merge the writing of the COW regions with the guest data

[Alberto Garcia]

If the guest tries to write data that results on the allocation of a
new cluster, instead of writing the guest data first and then the data
from the COW regions, write everything together using one single I/O
operation.

This can improve the write performance by 25% or more, depending on
several factors such as the media type, the cluster size and the I/O
request size.

Signed-off-by: Alberto Garcia 
Reviewed-by: Kevin Wolf 
---
 block/qcow2-cluster.c | 40 
 block/qcow2.c | 64 +++
 block/qcow2.h |  7 ++
 3 files changed, 91 insertions(+), 20 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 3ac26d6bf7..01f210187c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -776,6 +776,7 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
 assert(start->nb_bytes <= UINT_MAX - end->nb_bytes);
 assert(start->nb_bytes + end->nb_bytes <= UINT_MAX - data_bytes);
 assert(start->offset + start->nb_bytes <= end->offset);
+assert(!m->data_qiov || m->data_qiov->size == data_bytes);
 
 if (start->nb_bytes == 0 && end->nb_bytes == 0) {
 return 0;
@@ -807,7 +808,7 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
 /* The part of the buffer where the end region is located */
 end_buffer = start_buffer + buffer_size - end->nb_bytes;
 
-qemu_iovec_init(, 1);
+qemu_iovec_init(, 2 + (m->data_qiov ? m->data_qiov->niov : 0));
 
 qemu_co_mutex_unlock(>lock);
 /* First we read the existing data from both COW regions. We
@@ -842,17 +843,36 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
*m)
 }
 }
 
-/* And now we can write everything */
-qemu_iovec_reset();
-qemu_iovec_add(, start_buffer, start->nb_bytes);
-ret = do_perform_cow_write(bs, m->alloc_offset, start->offset, );
-if (ret < 0) {
-goto fail;
+/* And now we can write everything. If we have the guest data we
+ * can write everything in one single operation */
+if (m->data_qiov) {
+qemu_iovec_reset();
+if (start->nb_bytes) {
+qemu_iovec_add(, start_buffer, start->nb_bytes);
+}
+qemu_iovec_concat(, m->data_qiov, 0, data_bytes);
+if (end->nb_bytes) {
+qemu_iovec_add(, end_buffer, end->nb_bytes);
+}
+/* NOTE: we have a write_aio blkdebug event here followed by
+ * a cow_write one in do_perform_cow_write(), but there's only
+ * one single I/O operation */
+BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
+ret = do_perform_cow_write(bs, m->alloc_offset, start->offset, );
+} else {
+/* If there's no guest data then write both COW regions separately */
+qemu_iovec_reset();
+qemu_iovec_add(, start_buffer, start->nb_bytes);
+ret = do_perform_cow_write(bs, m->alloc_offset, start->offset, );
+if (ret < 0) {
+goto fail;
+}
+
+qemu_iovec_reset();
+qemu_iovec_add(, end_buffer, end->nb_bytes);
+ret = do_perform_cow_write(bs, m->alloc_offset, end->offset, );
 }
 
-qemu_iovec_reset();
-qemu_iovec_add(, end_buffer, end->nb_bytes);
-ret = do_perform_cow_write(bs, m->alloc_offset, end->offset, );
 fail:
 qemu_co_mutex_lock(>lock);
 
diff --git a/block/qcow2.c b/block/qcow2.c
index b3ba5daa93..328b1d4fb5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1575,6 +1575,44 @@ fail:
 return ret;
 }
 
+/* Check if it's possible to merge a write request with the writing of
+ * the data from the COW regions */
+static bool merge_cow(uint64_t offset, unsigned bytes,
+  QEMUIOVector *hd_qiov, QCowL2Meta *l2meta)
+{
+QCowL2Meta *m;
+
+for (m = l2meta; m != NULL; m = m->next) {
+/* If both COW regions are empty then there's nothing to merge */
+if (m->cow_start.nb_bytes == 0 && m->cow_end.nb_bytes == 0) {
+continue;
+}
+
+/* The data (middle) region must be immediately after the
+ * start region */
+if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
+continue;
+}
+
+/* The end region must be immediately after the data (middle)
+ * region */
+if (m->offset + m->cow_end.offset != offset + bytes) {
+continue;
+}
+
+/* Make sure that adding both COW regions to the QEMUIOVector
+ * does not exceed IOV_MAX */
+if (hd_qiov->niov > IOV_MAX - 2) {
+continue;
+}
+
+m->data_qiov = hd_qiov;
+return true;
+}
+
+return false;
+}
+
 static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
  uint64_t bytes, QEMUIOVector *qiov,
  int flags)
@@ -1657,16 +1695,22 @@ static 

[Qemu-block] [PATCH v4 2/7] qcow2: Use unsigned int for both members of Qcow2COWRegion

[Alberto Garcia]

Qcow2COWRegion has two attributes:

- The offset of the COW region from the start of the first cluster
  touched by the I/O request. Since it's always going to be positive
  and the maximum request size is at most INT_MAX, we can use a
  regular unsigned int to store this offset.

- The size of the COW region in bytes. This is guaranteed to be >= 0,
  so we should use an unsigned type instead.

In x86_64 this reduces the size of Qcow2COWRegion from 16 to 8 bytes.
It will also help keep some assertions simpler now that we know that
there are no negative numbers.

The prototype of do_perform_cow() is also updated to reflect these
changes.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Reviewed-by: Kevin Wolf 
---
 block/qcow2-cluster.c | 4 ++--
 block/qcow2.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index d1c419f52b..a86c5a75a9 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -406,8 +406,8 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
 static int coroutine_fn do_perform_cow(BlockDriverState *bs,
uint64_t src_cluster_offset,
uint64_t cluster_offset,
-   int offset_in_cluster,
-   int bytes)
+   unsigned offset_in_cluster,
+   unsigned bytes)
 {
 BDRVQcow2State *s = bs->opaque;
 QEMUIOVector qiov;
diff --git a/block/qcow2.h b/block/qcow2.h
index 1801dc30dc..c26ee0a33d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -301,10 +301,10 @@ typedef struct Qcow2COWRegion {
  * Offset of the COW region in bytes from the start of the first cluster
  * touched by the request.
  */
-uint64_toffset;
+unsignedoffset;
 
 /** Number of bytes to copy */
-int nb_bytes;
+unsignednb_bytes;
 } Qcow2COWRegion;
 
 /**
-- 
2.11.0

[Qemu-block] [PATCH v4 5/7] qcow2: Allow reading both COW regions with only one request

[Alberto Garcia]

Reading both COW regions requires two separate requests, but it's
perfectly possible to merge them and perform only one. This generally
improves performance, particularly on rotating disk drives. The
downside is that the data in the middle region is read but discarded.

This patch takes a conservative approach and only merges reads when
the size of the middle region is <= 16KB.

Signed-off-by: Alberto Garcia 
Reviewed-by: Kevin Wolf 
---
 block/qcow2-cluster.c | 51 ++-
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 3c9ace8a96..20fb531932 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -777,20 +777,38 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
*m)
 Qcow2COWRegion *start = >cow_start;
 Qcow2COWRegion *end = >cow_end;
 unsigned buffer_size;
+unsigned data_bytes = end->offset - (start->offset + start->nb_bytes);
+bool merge_reads;
 uint8_t *start_buffer, *end_buffer;
 int ret;
 
 assert(start->nb_bytes <= UINT_MAX - end->nb_bytes);
+assert(start->nb_bytes + end->nb_bytes <= UINT_MAX - data_bytes);
+assert(start->offset + start->nb_bytes <= end->offset);
 
 if (start->nb_bytes == 0 && end->nb_bytes == 0) {
 return 0;
 }
 
-/* Reserve a buffer large enough to store the data from both the
- * start and end COW regions. Add some padding in the middle if
- * necessary to make sure that the end region is optimally aligned */
-buffer_size = QEMU_ALIGN_UP(start->nb_bytes, bdrv_opt_mem_align(bs)) +
-end->nb_bytes;
+/* If we have to read both the start and end COW regions and the
+ * middle region is not too large then perform just one read
+ * operation */
+merge_reads = start->nb_bytes && end->nb_bytes && data_bytes <= 16384;
+if (merge_reads) {
+buffer_size = start->nb_bytes + data_bytes + end->nb_bytes;
+} else {
+/* If we have to do two reads, add some padding in the middle
+ * if necessary to make sure that the end region is optimally
+ * aligned. */
+size_t align = bdrv_opt_mem_align(bs);
+assert(align > 0 && align <= UINT_MAX);
+assert(QEMU_ALIGN_UP(start->nb_bytes, align) <=
+   UINT_MAX - end->nb_bytes);
+buffer_size = QEMU_ALIGN_UP(start->nb_bytes, align) + end->nb_bytes;
+}
+
+/* Reserve a buffer large enough to store all the data that we're
+ * going to read */
 start_buffer = qemu_try_blockalign(bs, buffer_size);
 if (start_buffer == NULL) {
 return -ENOMEM;
@@ -799,15 +817,22 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
*m)
 end_buffer = start_buffer + buffer_size - end->nb_bytes;
 
 qemu_co_mutex_unlock(>lock);
-/* First we read the existing data from both COW regions */
-ret = do_perform_cow_read(bs, m->offset, start->offset,
-  start_buffer, start->nb_bytes);
-if (ret < 0) {
-goto fail;
-}
+/* First we read the existing data from both COW regions. We
+ * either read the whole region in one go, or the start and end
+ * regions separately. */
+if (merge_reads) {
+ret = do_perform_cow_read(bs, m->offset, start->offset,
+  start_buffer, buffer_size);
+} else {
+ret = do_perform_cow_read(bs, m->offset, start->offset,
+  start_buffer, start->nb_bytes);
+if (ret < 0) {
+goto fail;
+}
 
-ret = do_perform_cow_read(bs, m->offset, end->offset,
-  end_buffer, end->nb_bytes);
+ret = do_perform_cow_read(bs, m->offset, end->offset,
+  end_buffer, end->nb_bytes);
+}
 if (ret < 0) {
 goto fail;
 }
-- 
2.11.0

[Qemu-block] [PATCH v4 3/7] qcow2: Make perform_cow() call do_perform_cow() twice

[Alberto Garcia]

Instead of calling perform_cow() twice with a different COW region
each time, call it just once and make perform_cow() handle both
regions.

This patch simply moves code around. The next one will do the actual
reordering of the COW operations.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Reviewed-by: Kevin Wolf 
---
 block/qcow2-cluster.c | 38 +++---
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index a86c5a75a9..4c03639a72 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -414,6 +414,10 @@ static int coroutine_fn do_perform_cow(BlockDriverState 
*bs,
 struct iovec iov;
 int ret;
 
+if (bytes == 0) {
+return 0;
+}
+
 iov.iov_len = bytes;
 iov.iov_base = qemu_try_blockalign(bs, iov.iov_len);
 if (iov.iov_base == NULL) {
@@ -751,31 +755,40 @@ uint64_t 
qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
 return cluster_offset;
 }
 
-static int perform_cow(BlockDriverState *bs, QCowL2Meta *m, Qcow2COWRegion *r)
+static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
 {
 BDRVQcow2State *s = bs->opaque;
+Qcow2COWRegion *start = >cow_start;
+Qcow2COWRegion *end = >cow_end;
 int ret;
 
-if (r->nb_bytes == 0) {
+if (start->nb_bytes == 0 && end->nb_bytes == 0) {
 return 0;
 }
 
 qemu_co_mutex_unlock(>lock);
-ret = do_perform_cow(bs, m->offset, m->alloc_offset, r->offset, 
r->nb_bytes);
+ret = do_perform_cow(bs, m->offset, m->alloc_offset,
+ start->offset, start->nb_bytes);
+if (ret < 0) {
+goto fail;
+}
+
+ret = do_perform_cow(bs, m->offset, m->alloc_offset,
+ end->offset, end->nb_bytes);
+
+fail:
 qemu_co_mutex_lock(>lock);
 
-if (ret < 0) {
-return ret;
-}
-
 /*
  * Before we update the L2 table to actually point to the new cluster, we
  * need to be sure that the refcounts have been increased and COW was
  * handled.
  */
-qcow2_cache_depends_on_flush(s->l2_table_cache);
+if (ret == 0) {
+qcow2_cache_depends_on_flush(s->l2_table_cache);
+}
 
-return 0;
+return ret;
 }
 
 int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
@@ -795,12 +808,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
QCowL2Meta *m)
 }
 
 /* copy content of unmodified sectors */
-ret = perform_cow(bs, m, >cow_start);
-if (ret < 0) {
-goto err;
-}
-
-ret = perform_cow(bs, m, >cow_end);
+ret = perform_cow(bs, m);
 if (ret < 0) {
 goto err;
 }
-- 
2.11.0

[Qemu-block] [PATCH v4 1/7] qcow2: Remove unused Error variable in do_perform_cow()

[Alberto Garcia]

We are using the return value of qcow2_encrypt_sectors() to detect
problems but we are throwing away the returned Error since we have no
way to report it to the user. Therefore we can simply get rid of the
local Error variable and pass NULL instead.

Alternatively we could try to figure out a way to pass the original
error instead of simply returning -EIO, but that would be more
invasive, so let's keep the current approach.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Reviewed-by: Kevin Wolf 
---
 block/qcow2-cluster.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index d779ea19cf..d1c419f52b 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -440,16 +440,14 @@ static int coroutine_fn do_perform_cow(BlockDriverState 
*bs,
 }
 
 if (bs->encrypted) {
-Error *err = NULL;
 int64_t sector = (src_cluster_offset + offset_in_cluster)
  >> BDRV_SECTOR_BITS;
 assert(s->cipher);
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
 if (qcow2_encrypt_sectors(s, sector, iov.iov_base, iov.iov_base,
-  bytes >> BDRV_SECTOR_BITS, true, ) < 0) {
+  bytes >> BDRV_SECTOR_BITS, true, NULL) < 0) {
 ret = -EIO;
-error_free(err);
 goto out;
 }
 }
-- 
2.11.0

Re: [Qemu-block] [PATCH 1/4] qapi/qnull: Add own header

[Kevin Wolf]

Am 14.06.2017 um 17:30 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v2 7/7] qcow2: Merge the writing of the COW regions with the guest data

[Alberto Garcia]

On Fri 16 Jun 2017 05:31:42 PM CEST, Kevin Wolf wrote:
>> +/* Make sure that adding both COW regions to the QEMUIOVector
>> + * does not exceed IOV_MAX */
>> +if (hd_qiov->niov > IOV_MAX - 2) {
>> +continue;
>> +}
>> +
>> +m->data_qiov = hd_qiov;
>
> I don't think having this side effect is good for a function that is
> called can_xyz(). I'd either call it merge_cow() or move the
> assignment to the caller.

You're right, I think I prefer to move the assignment to the caller for
now.

Berto

Re: [Qemu-block] [PATCH v2] live-block-ops.txt: Rename, rewrite, and improve it

[Kashyap Chamarthy]

On Fri, Jun 16, 2017 at 04:41:20PM +0100, Stephen Finucane wrote:
> On Fri, 2017-06-16 at 16:51 +0200, Kashyap Chamarthy wrote:

[...]

> As requested, a couple of rST pointers below that will help you if/when you
> switch to Sphinx. I've only focused on the design aspect, not the content.

Thanks for the Sphinx / rST review.  Eric Blake is reviewing the
content, and probably others will chime in, too.

[...]

> > +Copyright (C) 2017 Red Hat Inc.
> > +
> > +This work is licensed under the terms of the GNU GPL, version 2 or
> > +later.  See the COPYING file in the top-level directory.
> > +
> > +---
> > +
> 
> This information doesn't need to be output in the web version, IMO. If write 
> it
> like a comment, it will only be visible in the source. See what we do in OVS
> docs [1] for an example.
> 
> [1] 
> https://raw.githubusercontent.com/openvswitch/ovs/master/Documentation/index.rst

Yep, that's useful.  Fixed in v3.

[...]

> > +NB: The file ``qapi/block-core.json`` in the QEMU source tree has the
> > +canonical QEMU API (QAPI) schema documentation for the QMP primitives
> > +discussed here.
> > +
> 
> You might consider using admonitions here and elsewhere. This would make sense
> as a 'note' or 'important' directive:
> 
>   .. note::
> 
>   The file ``qapi/block-core.json`` ...

Yes, done.

> > +
> > +.. contents::
> 
> This can probably go if/when Sphinx is integrated - Sphinx includes a ToC in
> the sidebar by default. Perhaps include a TODO to remove this?
> 
>   .. TODO(kashyap): Remove this when Sphinx is integrated

Useful, done.

[...]

> > +(1) Directional: 'base' and 'top'.  Given the simple disk image chain
> > +above, image [A] can be referred to as 'base', and image [B] as
> > +'top'.  (This terminology can be seen in in QAPI schema file,
> > +block-core.json.)
> 
> This looks really like a definition list, which is rST are written like so:
> 
>   term
> 
> Detailed description of the term here...
> 
> So this would become:
> 
>   Directional
> 
> 'base' and 'top'. Given...

Yeah, I tried it, but it's just creating needless blank lines for me in
the HTML rendering.  So I stuck with using numbers.

[...]

> > +NB: The base disk image can be raw format; however, all the overlay
> > +files must be of QCOW2 format.
> 
> .. important::

Yes, noted.

[...]

> > +Brief overview of live block QMP primitives
> > +---

[...]

> Definition list?

Not quite.  It's more a quick overview.
 
> > +
> > +
> > +.. _`Interacting with a QEMU instance`:
> 
> If you're not linking to this, you don't need to include this. The 'contents'
> directive will automatically insert an anchor for each heading.

Yeah, I actually did link to to further below, hence I retained it.

[...]

> > +The ``-blockdev`` command-line option, used above, is available from
> > +QEMU 2.9 onwards.  In the above invocation, notice the 'node-name'
> 
> ``node-name``?

Yes.

> 
> > +parameter that is used to refer to the disk image a.qcow2 ('node-A') --
> 
> ``a.qcow2``?

Perhaps.

[...]

> > +NB: In the event we have to repeat a certain QMP command, we will: for
> > +the first occurrence of it, show the the ``qmp-shell`` invocation,
> > +*and* the corresponding raw JSON QMP syntax; but for subsequent
> > +invocations, present just the ``qmp-shell`` syntax, and omit the
> > +equivalent JSON output.
> 
> .. important::

Yeah, it's more of a ".. note::", will fix.

[...]

> > +Here, "node-A" is the name QEMU internally uses to refer to the base
> > +image [A] -- it is the backing file, based on which the overlay image,
> > +[B], is created.
> 
> I guess you should probably use ``[A]`` here to preserve formatting

Hmm, noted.

[...]

> > +
> > +A note on points-in-time vs file names
> > +--
> > +
> > +In our disk disk image chain:
> > +
> > +::
> 
> repeated word and no need for ':\n\n::' - you can just use '::'.
> 
>   In our disk image chain::
> 
> ditto for the rest of the file

Ah, indeed.  I recall using it in the past, but forgot.  Avoids needless
blank lines.  Will fix throughout.

[...]

> > +
> > +
> > +Live block streaming --- ``block-stream``
> > +-
> > +
> > +The ``block-stream`` command allows you to do live copy data from backing
> > +files into overlay images.
> > +
> > +Given our original example disk image chain from earlier:
> > +
> > +::
> > +
> > +[A] <-- [B] <-- [C] <-- [D]
> > +
> > +The disk image chain can be shortened in one of the following different
> > +ways (not an exhaustive list).
> > +
> 
> Maybe you should include an anchor here, so you can link to it below.

Yes, makes sense.  I know by "link to it below", you mean link to it in
the next where I refer to these.

[...]

> > +[The invocation for rest of the cases, discussed in the previous
> > +section, is omitted for brevity.]
> 
> This looks like a:
> 
>   .. note::

Yes; will fix.

[...]

-- 
/kashyap

Re: [Qemu-block] [Question] How can we confirm hot-plug disk succesfully?

[Kevin Wolf]

Am 19.06.2017 um 12:27 hat Xie Changlong geschrieben:
> 在 6/19/2017 3:27 PM, Kevin Wolf 写道:
> >Am 18.06.2017 um 09:21 hat Xie Changlong geschrieben:
> >>In device hot-remove scenario, if we don't probe acpiphp module on
> >>the guest, 'device_del' will never emit DEVICE_DELETED event(because
> >>guest will not write to __EJ0) . So we can confirm that hot-remove
> >>failed. But IIUC, there is no event such as DEVICE_ADDED, so
> >>1) How can we confirm hotplug('device_add') successfully?
> >>2) It seems when hot-plug disk, we don't care acpiphp module status
> >>on the guest, am I right?
> >>3) Why there is no DEVICE_ADDED like event?
> >
> >device_add doesn't need guest cooperation, so it is immediately
> >completed when the QMP command returns success.
> >
> 
> That's what i though too. But I have a question, if we don't proble
> acpiphp on the guest, and execute below commands:
> 
> Hot plug:
> (qemu) drive_add 0
> file=/resource/changlox/test.raw,id=drive-virtio-disk1,if=none
> (qemu) device_add virtio-blk-pci,drive=drive-virtio-disk1,id=virtio-disk1
> 
> Hot remove:
> (qemu) device_del virtio-disk1
> (qemu) drive_del drive-virtio-disk1
> (qemu) qom-list /machine/peripheral
> type (string)
> virtio-disk1 (child)
> (qemu) info block
> foo (#block122): suse1.qcow2 (qcow2)
> Cache mode:   writeback, direct
> Backing file: suse.qcow2.orgin (chain depth: 1)
> 
> ide1-cd0: [not inserted]
> Removable device: not locked, tray closed
> 
> floppy0: [not inserted]
> Removable device: not locked, tray closed
> 
> sd0: [not inserted]
> Removable device: not locked, tray closed
> (qemu) drive_add 0
> file=/resource/changlox/test.raw,id=drive-virtio-disk1,if=none
> Duplicate ID 'drive-virtio-disk1' for drive
> 
> 'info block' shows nothing, but we can't add drive who's id
> is'drive-virtio-disk1' too.

Yes, the old BlockBackend is only fully freed when the guest actually
unplugs the device. Specifically, we would have to free the QemuOpts
in DriveInfo that keeps the ID reserved. Currently, it is only freed
when the BlockBackend is destroyed:

blockdev_auto_del()
blk_unref()
blk_delete()
drive_info_del()

We can't free the DriveInfo earlier because it's still needed while the
guest device is still alive.

I'm not sure, but it may be possible to free just the QemuOpts in
monitor_remove_blk(), so that the ID can immediately be reused.

Markus, would you know?

> There is a more serious situation, we
> could *never* destory device memory with 'device_del', it's memory
> leak to me if the guest os never support hot-plug and the user don't
> know this information.

The user sees that they never get a DEVICE_REMOVED event, so in some way
the do know about it.

Kevin

Re: [Qemu-block] [Question] How can we confirm hot-plug disk succesfully?

[Xie Changlong]

在 6/19/2017 3:27 PM, Kevin Wolf 写道:

Am 18.06.2017 um 09:21 hat Xie Changlong geschrieben:

In device hot-remove scenario, if we don't probe acpiphp module on
the guest, 'device_del' will never emit DEVICE_DELETED event(because
guest will not write to __EJ0) . So we can confirm that hot-remove
failed. But IIUC, there is no event such as DEVICE_ADDED, so
1) How can we confirm hotplug('device_add') successfully?
2) It seems when hot-plug disk, we don't care acpiphp module status
on the guest, am I right?
3) Why there is no DEVICE_ADDED like event?


device_add doesn't need guest cooperation, so it is immediately
completed when the QMP command returns success.



That's what i though too. But I have a question, if we don't proble 
acpiphp on the guest, and execute below commands:


Hot plug:
(qemu) drive_add 0 
file=/resource/changlox/test.raw,id=drive-virtio-disk1,if=none

(qemu) device_add virtio-blk-pci,drive=drive-virtio-disk1,id=virtio-disk1

Hot remove:
(qemu) device_del virtio-disk1
(qemu) drive_del drive-virtio-disk1
(qemu) qom-list /machine/peripheral
type (string)
virtio-disk1 (child)
(qemu) info block
foo (#block122): suse1.qcow2 (qcow2)
Cache mode:   writeback, direct
Backing file: suse.qcow2.orgin (chain depth: 1)

ide1-cd0: [not inserted]
Removable device: not locked, tray closed

floppy0: [not inserted]
Removable device: not locked, tray closed

sd0: [not inserted]
Removable device: not locked, tray closed
(qemu) drive_add 0 
file=/resource/changlox/test.raw,id=drive-virtio-disk1,if=none

Duplicate ID 'drive-virtio-disk1' for drive

'info block' shows nothing, but we can't add drive who's id 
is'drive-virtio-disk1' too。 There is a more serious situation, we could 
*never* destory device memory with 'device_del', it's memory leak to me 
if the guest os never support hot-plug and the user don't know this 
information.





Kevin



--
Thanks
-Xie

Re: [Qemu-block] [Question] How can we confirm hot-plug disk succesfully?

[Kevin Wolf]

Am 18.06.2017 um 09:21 hat Xie Changlong geschrieben:
> In device hot-remove scenario, if we don't probe acpiphp module on
> the guest, 'device_del' will never emit DEVICE_DELETED event(because
> guest will not write to __EJ0) . So we can confirm that hot-remove
> failed. But IIUC, there is no event such as DEVICE_ADDED, so
> 1) How can we confirm hotplug('device_add') successfully?
> 2) It seems when hot-plug disk, we don't care acpiphp module status
> on the guest, am I right?
> 3) Why there is no DEVICE_ADDED like event?

device_add doesn't need guest cooperation, so it is immediately
completed when the QMP command returns success.

Kevin