Re: [Qemu-block] [Qemu-devel] [PATCH 0/5] crypto threads
Patchew URL: https://patchew.org/QEMU/20181204184657.78028-1-vsement...@virtuozzo.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-quick@centos7 SHOW_ENV=1 J=8 === TEST SCRIPT END === libpmem support no libudev no WARNING: Use of SDL 1.2 is deprecated and will be removed in WARNING: future releases. Please switch to using SDL 2.0 NOTE: cross-compilers enabled: 'cc' GEN x86_64-softmmu/config-devices.mak.tmp --- CC tests/usb-hcd-uhci-test.o CC tests/usb-hcd-xhci-test.o /tmp/qemu-test/src/tests/test-crypto-block.c: In function 'test_block': /tmp/qemu-test/src/tests/test-crypto-block.c:310:30: error: passing argument 6 of 'qcrypto_block_open' makes integer from pointer without a cast [-Werror] NULL); ^ In file included from /tmp/qemu-test/src/tests/test-crypto-block.c:24:0: /tmp/qemu-test/src/include/crypto/block.h:107:15: note: expected 'int' but argument is of type 'void *' QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, ^ /tmp/qemu-test/src/tests/test-crypto-block.c:310:30: error: too few arguments to function 'qcrypto_block_open' NULL); ^ In file included from /tmp/qemu-test/src/tests/test-crypto-block.c:24:0: /tmp/qemu-test/src/include/crypto/block.h:107:15: note: declared here QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, ^ /tmp/qemu-test/src/tests/test-crypto-block.c:318:30: error: passing argument 6 of 'qcrypto_block_open' makes integer from pointer without a cast [-Werror] _abort); ^ In file included from /tmp/qemu-test/src/tests/test-crypto-block.c:24:0: /tmp/qemu-test/src/include/crypto/block.h:107:15: note: expected 'int' but argument is of type 'struct Error **' QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, ^ /tmp/qemu-test/src/tests/test-crypto-block.c:318:30: error: too few arguments to function 'qcrypto_block_open' _abort); ^ In file included from /tmp/qemu-test/src/tests/test-crypto-block.c:24:0: /tmp/qemu-test/src/include/crypto/block.h:107:15: note: declared here QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, ^ /tmp/qemu-test/src/tests/test-crypto-block.c:332:30: error: passing argument 6 of 'qcrypto_block_open' makes integer from pointer without a cast [-Werror] _abort); ^ In file included from /tmp/qemu-test/src/tests/test-crypto-block.c:24:0: /tmp/qemu-test/src/include/crypto/block.h:107:15: note: expected 'int' but argument is of type 'struct Error **' QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, ^ /tmp/qemu-test/src/tests/test-crypto-block.c:332:30: error: too few arguments to function 'qcrypto_block_open' _abort); ^ In file included from /tmp/qemu-test/src/tests/test-crypto-block.c:24:0: The full log is available at http://patchew.org/logs/20181204184657.78028-1-vsement...@virtuozzo.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[Qemu-block] [PATCH for-3.1? 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create
Free block->cipher and block->ivgen on error path. Signed-off-by: Vladimir Sementsov-Ogievskiy --- crypto/block-luks.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crypto/block-luks.c b/crypto/block-luks.c index 5738124773..51e24d23ca 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -1341,6 +1341,9 @@ qcrypto_block_luks_create(QCryptoBlock *block, qcrypto_ivgen_free(ivgen); qcrypto_cipher_free(cipher); +qcrypto_cipher_free(block->cipher); +qcrypto_ivgen_free(block->ivgen); + g_free(luks); return -1; } -- 2.18.0
Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create
On Tue, 4 Dec 2018 at 18:50, Vladimir Sementsov-Ogievskiy wrote: > > Free block->cipher and block->ivgen on error path. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > crypto/block-luks.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > index 5738124773..51e24d23ca 100644 > --- a/crypto/block-luks.c > +++ b/crypto/block-luks.c > @@ -1341,6 +1341,9 @@ qcrypto_block_luks_create(QCryptoBlock *block, > qcrypto_ivgen_free(ivgen); > qcrypto_cipher_free(cipher); > > +qcrypto_cipher_free(block->cipher); > +qcrypto_ivgen_free(block->ivgen); > + > g_free(luks); > return -1; > } > -- Too late for 3.1, I'm afraid. You should probably add the cc tag for stable. thanks -- PMM
[Qemu-block] [PATCH 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
qcrypto_block_encrypt_helper and qcrypto_block_decrypt_helper are
almost identical, let's reduce code duplication and simplify further
improvements.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
crypto/block.c | 81 +++---
1 file changed, 31 insertions(+), 50 deletions(-)
diff --git a/crypto/block.c b/crypto/block.c
index e59d1140fe..f4101f0841 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -190,14 +190,21 @@ void qcrypto_block_free(QCryptoBlock *block)
}
-int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- Error **errp)
+typedef int (*QCryptoCipherEncryptFunc)(QCryptoCipher *cipher,
+const void *in,
+void *out,
+size_t len,
+Error **errp);
+
+static int do_qcrypto_block_encrypt(QCryptoCipher *cipher,
+size_t niv,
+QCryptoIVGen *ivgen,
+int sectorsize,
+uint64_t offset,
+uint8_t *buf,
+size_t len,
+QCryptoCipherEncryptFunc func,
+Error **errp)
{
uint8_t *iv;
int ret = -1;
@@ -226,8 +233,7 @@ int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
}
nbytes = len > sectorsize ? sectorsize : len;
-if (qcrypto_cipher_decrypt(cipher, buf, buf,
- nbytes, errp) < 0) {
+if (func(cipher, buf, buf, nbytes, errp) < 0) {
goto cleanup;
}
@@ -243,7 +249,7 @@ int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
}
-int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
+int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
size_t niv,
QCryptoIVGen *ivgen,
int sectorsize,
@@ -252,45 +258,20 @@ int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
size_t len,
Error **errp)
{
-uint8_t *iv;
-int ret = -1;
-uint64_t startsector = offset / sectorsize;
-
-assert(QEMU_IS_ALIGNED(offset, sectorsize));
-assert(QEMU_IS_ALIGNED(len, sectorsize));
-
-iv = niv ? g_new0(uint8_t, niv) : NULL;
-
-while (len > 0) {
-size_t nbytes;
-if (niv) {
-if (qcrypto_ivgen_calculate(ivgen,
-startsector,
-iv, niv,
-errp) < 0) {
-goto cleanup;
-}
-
-if (qcrypto_cipher_setiv(cipher,
- iv, niv,
- errp) < 0) {
-goto cleanup;
-}
-}
-
-nbytes = len > sectorsize ? sectorsize : len;
-if (qcrypto_cipher_encrypt(cipher, buf, buf,
- nbytes, errp) < 0) {
-goto cleanup;
-}
+return do_qcrypto_block_encrypt(cipher, niv, ivgen, sectorsize, offset,
+buf, len, qcrypto_cipher_decrypt, errp);
+}
-startsector++;
-buf += nbytes;
-len -= nbytes;
-}
-ret = 0;
- cleanup:
-g_free(iv);
-return ret;
+int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp)
+{
+return do_qcrypto_block_encrypt(cipher, niv, ivgen, sectorsize, offset,
+buf, len, qcrypto_cipher_encrypt, errp);
}
--
2.18.0
[Qemu-block] [PATCH 3/5] crypto/block: rename qcrypto_block_*crypt_helper
Rename qcrypto_block_*crypt_helper to qcrypto_cipher_*crypt_helper, as
it's not about QCryptoBlock. This is needed to introduce
qcrypto_block_*crypt_helper in the next commit, which will have
QCryptoBlock pointer and than will be able to use additional fields of
it, which in turn will be used to implement thread-safe QCryptoBlock
operations.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
crypto/blockpriv.h | 34 +-
crypto/block-luks.c | 44 +-
crypto/block-qcow.c | 16 ++---
crypto/block.c | 58 ++---
4 files changed, 76 insertions(+), 76 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 41840abcec..347a7c010a 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -78,22 +78,22 @@ struct QCryptoBlockDriver {
};
-int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- Error **errp);
-
-int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- Error **errp);
+int qcrypto_cipher_decrypt_helper(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp);
+
+int qcrypto_cipher_encrypt_helper(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp);
#endif /* QCRYPTO_BLOCKPRIV_H */
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 51e24d23ca..72dd51051d 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -504,14 +504,14 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
* to reset the encryption cipher every time the master
* key crosses a sector boundary.
*/
-if (qcrypto_block_decrypt_helper(cipher,
- niv,
- ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- 0,
- splitkey,
- splitkeylen,
- errp) < 0) {
+if (qcrypto_cipher_decrypt_helper(cipher,
+ niv,
+ ivgen,
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ 0,
+ splitkey,
+ splitkeylen,
+ errp) < 0) {
goto cleanup;
}
@@ -1219,12 +1219,12 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Now we encrypt the split master key with the key generated
* from the user's password, before storing it */
-if (qcrypto_block_encrypt_helper(cipher, block->niv, ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- 0,
- splitkey,
- splitkeylen,
- errp) < 0) {
+if (qcrypto_cipher_encrypt_helper(cipher, block->niv, ivgen,
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ 0,
+ splitkey,
+ splitkeylen,
+ errp) < 0) {
goto error;
}
@@ -1409,10 +1409,10 @@ qcrypto_block_luks_decrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
-return qcrypto_block_decrypt_helper(block->cipher,
-block->niv, block->ivgen,
-QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
-
[Qemu-block] [PATCH 0/5] crypto threads
Hi all. These series are preliminary step before moving encryption code in qcow2 to the threads. The first attempt of doing it is on list ([PATCH 00/11] qcow2: encryption threads : https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00729.html) But it's approach with multiplying the whole QCryptoBlock per thread was rejected. So, here is a solution to maintain multitasking inside QCryptoBlock. Patch 01 may be considered as 3.1-rc4 material, others are ofcourse for 4.0. Vladimir Sementsov-Ogievskiy (5): crypto/block-luks: fix memory leak in qcrypto_block_luks_create crypto/block: refactor qcrypto_block_*crypt_helper functions crypto/block: rename qcrypto_block_*crypt_helper crypto/block: introduce qcrypto_block_*crypt_helper functions crypto: support multiple threads accessing one QCryptoBlock crypto/blockpriv.h| 42 ++-- include/crypto/block.h| 16 ++- block/crypto.c| 1 + block/qcow.c | 2 +- block/qcow2.c | 4 +- crypto/block-luks.c | 60 +-- crypto/block-qcow.c | 26 ++--- crypto/block.c| 210 -- tests/test-crypto-block.c | 2 + 9 files changed, 257 insertions(+), 106 deletions(-) -- 2.18.0
[Qemu-block] [PATCH 5/5] crypto: support multiple threads accessing one QCryptoBlock
The two thing that should be handled are cipher and ivgen. For ivgen
the solution is just mutex, as iv calculations should not be long in
comparison with encryption/decryption. And for cipher let's just keep
per-thread ciphers.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
crypto/blockpriv.h| 16 -
include/crypto/block.h| 16 -
block/crypto.c| 1 +
block/qcow.c | 2 +-
block/qcow2.c | 4 +-
crypto/block-luks.c | 25 +++
crypto/block-qcow.c | 20 +++---
crypto/block.c| 135 --
tests/test-crypto-block.c | 2 +
9 files changed, 176 insertions(+), 45 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index e9fe3e5687..86dae49210 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -22,6 +22,7 @@
#define QCRYPTO_BLOCKPRIV_H
#include "crypto/block.h"
+#include "qemu/thread.h"
typedef struct QCryptoBlockDriver QCryptoBlockDriver;
@@ -31,8 +32,12 @@ struct QCryptoBlock {
const QCryptoBlockDriver *driver;
void *opaque;
-QCryptoCipher *cipher;
+QCryptoCipher **ciphers;
+int n_ciphers;
+int n_free_ciphers;
QCryptoIVGen *ivgen;
+QemuMutex mutex;
+
QCryptoHashAlgorithm kdfhash;
size_t niv;
uint64_t payload_offset; /* In bytes */
@@ -46,6 +51,7 @@ struct QCryptoBlockDriver {
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
+int n_threads,
Error **errp);
int (*create)(QCryptoBlock *block,
@@ -110,4 +116,12 @@ int qcrypto_block_encrypt_helper(QCryptoBlock *block,
size_t len,
Error **errp);
+int qcrypto_block_init_cipher(QCryptoBlock *block,
+ QCryptoCipherAlgorithm alg,
+ QCryptoCipherMode mode,
+ const uint8_t *key, size_t nkey,
+ int n_threads, Error **errp);
+
+void qcrypto_block_free_cipher(QCryptoBlock *block);
+
#endif /* QCRYPTO_BLOCKPRIV_H */
diff --git a/include/crypto/block.h b/include/crypto/block.h
index cd18f46d56..d54044323a 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -75,6 +75,8 @@ typedef enum {
* @readfunc: callback for reading data from the volume
* @opaque: data to pass to @readfunc
* @flags: bitmask of QCryptoBlockOpenFlags values
+ * @n_threads: prepare block to multi tasking with up to
+ * @n_threads threads
* @errp: pointer to a NULL-initialized error object
*
* Create a new block encryption object for an existing
@@ -107,6 +109,7 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions
*options,
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
+ int n_threads,
Error **errp);
/**
@@ -202,12 +205,23 @@ int qcrypto_block_encrypt(QCryptoBlock *block,
* qcrypto_block_get_cipher:
* @block: the block encryption object
*
- * Get the cipher to use for payload encryption
+ * Get the cipher to use for payload encryption. Must be paired with
+ * qcrypto_block_put_cipher.
*
* Returns: the cipher object
*/
QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block);
+/**
+ * qcrypto_block_put_cipher:
+ * @block: the block encryption object
+ *
+ * Put cipher back to the block. Must follow qcrypto_block_get_cipher.
+ *
+ * Returns: the cipher object
+ */
+void qcrypto_block_put_cipher(QCryptoBlock *block, QCryptoCipher *cipher);
+
/**
* qcrypto_block_get_ivgen:
* @block: the block encryption object
diff --git a/block/crypto.c b/block/crypto.c
index 33ee01bebd..f0a5f6b987 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -229,6 +229,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat
format,
block_crypto_read_func,
bs,
cflags,
+ 1,
errp);
if (!crypto->block) {
diff --git a/block/qcow.c b/block/qcow.c
index 4518cb4c35..0a235bf393 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -213,7 +213,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options,
int flags,
cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
}
s->crypto = qcrypto_block_open(crypto_opts, "encrypt.",
- NULL, NULL, cflags, errp);
+ NULL, NULL, cflags, 1, errp);
if (!s->crypto) {
ret = -EINVAL;
goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index 991d6ac91b..bc8868c36a 100644
--- a/block/qcow2.c
+++
[Qemu-block] [PATCH 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions
Introduce QCryptoBlock-based functions and use them where possible.
This is needed to implement thread-safe encrypt/decrypt operations.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
crypto/blockpriv.h | 14 ++
crypto/block-luks.c | 14 ++
crypto/block-qcow.c | 14 ++
crypto/block.c | 26 ++
4 files changed, 52 insertions(+), 16 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 347a7c010a..e9fe3e5687 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -96,4 +96,18 @@ int qcrypto_cipher_encrypt_helper(QCryptoCipher *cipher,
size_t len,
Error **errp);
+int qcrypto_block_decrypt_helper(QCryptoBlock *block,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp);
+
+int qcrypto_block_encrypt_helper(QCryptoBlock *block,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp);
+
#endif /* QCRYPTO_BLOCKPRIV_H */
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 72dd51051d..bbffd80fff 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1409,10 +1409,9 @@ qcrypto_block_luks_decrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
-return qcrypto_cipher_decrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- offset, buf, len, errp);
+return qcrypto_block_decrypt_helper(block,
+QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+offset, buf, len, errp);
}
@@ -1425,10 +1424,9 @@ qcrypto_block_luks_encrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
-return qcrypto_cipher_encrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- offset, buf, len, errp);
+return qcrypto_block_encrypt_helper(block,
+QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+offset, buf, len, errp);
}
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 536ef4ee98..36bf5f09b7 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -152,10 +152,9 @@ qcrypto_block_qcow_decrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
-return qcrypto_cipher_decrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
- offset, buf, len, errp);
+return qcrypto_block_decrypt_helper(block,
+QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
+offset, buf, len, errp);
}
@@ -168,10 +167,9 @@ qcrypto_block_qcow_encrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
-return qcrypto_cipher_encrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
- offset, buf, len, errp);
+return qcrypto_block_encrypt_helper(block,
+QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
+offset, buf, len, errp);
}
diff --git a/crypto/block.c b/crypto/block.c
index 540b27e581..3edd9ec251 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -275,3 +275,29 @@ int qcrypto_cipher_encrypt_helper(QCryptoCipher *cipher,
return do_qcrypto_cipher_encrypt(cipher, niv, ivgen, sectorsize, offset,
buf, len, qcrypto_cipher_encrypt, errp);
}
+
+
+int qcrypto_block_decrypt_helper(QCryptoBlock *block,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp)
+{
+
Re: [Qemu-block] [PATCH 03/18] xen: introduce 'xen-qdisk'
> -Original Message-
> From: Qemu-devel [mailto:qemu-devel-
> bounces+paul.durrant=citrix@nongnu.org] On Behalf Of Paul Durrant
> Sent: 04 December 2018 15:20
> To: Anthony Perard
> Cc: Kevin Wolf ; Stefano Stabellini
> ; qemu-block@nongnu.org; qemu-de...@nongnu.org;
> Max Reitz ; xen-de...@lists.xenproject.org
> Subject: Re: [Qemu-devel] [PATCH 03/18] xen: introduce 'xen-qdisk'
>
> > -Original Message-
> > From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> > Sent: 29 November 2018 16:06
> > To: Paul Durrant
> > Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen-
> > de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> > ; Stefano Stabellini
> > Subject: Re: [PATCH 03/18] xen: introduce 'xen-qdisk'
> >
> > On Wed, Nov 21, 2018 at 03:11:56PM +, Paul Durrant wrote:
> > > This patch adds a new XenDevice: 'xen-qdisk' [1]. This will eventually
> > > replace the 'xen_disk' legacy PV backend but it is illustrative to
> build
> > > up the implementation incrementally, along with the XenBus/XenDevice
> > > framework. Subsequent patches will therefore add to this device's
> > > implementation as new features are added to the framework.
> > >
> > > After this patch has been applied it is possible to instantiate a new
> > > 'xen-qdisk' device with a single 'vdev' parameter, which accepts
> values
> > > adhering to the Xen VBD naming scheme [2]. For example, a command-line
> > > instantiation of a xen-qdisk can be done with an argument similar to
> the
> > > following:
> > >
> > > -device xen-qdisk,vdev=hda
> >
> > That works when QEMU boot, but doing the same thing once the guest have
> > booted, via QMP, doesn't. Here is the result (tested in qmp-shell):
> >
> > (QEMU) device_add driver=xen-qdisk vdev=hda
> > {
> > "error": {
> > "class": "GenericError",
> > "desc": "Bus 'xen-bus.0' does not support hotplugging"
> > }
> > }
> >
> > That's probably why I've asked about the hotplug capability on the
> > previous patch.
> >
>
> Ok. I've added the hotplug now so I'll make sure QMP DTRT.
>
> > > The implementation of the vdev parameter formulates the appropriate
> VBD
> > > number for use in the PV protocol.
> > >
> > > [1] The name 'qdisk' as always been the name given to the QEMU
> > > implementation of the Xen PV block protocol backend implementation
> > > [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/man/xen-
> vbd-
> > interface.markdown.7
> >
> > Maybe a link to the generated docs would be better:
> > https://xenbits.xen.org/docs/unstable/man/xen-vbd-interface.7.html
> >
>
> Ok.
>
> > Also, it would be useful to have the same link in the source code.
> >
>
> Yes, I'll add a comment.
>
> > > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > > new file mode 100644
> > > index 00..72122073f7
> > > --- /dev/null
> > > +++ b/hw/block/xen-qdisk.c
> > [...]
> > > +static char *disk_to_vbd_name(unsigned int disk)
> > > +{
> > > +unsigned int len = DIV_ROUND_UP(disk, 26);
> > > +char *name = g_malloc0(len + 1);
> > > +
> > > +do {
> > > +name[len--] = 'a' + (disk % 26);
> > > +disk /= 26;
> > > +} while (disk != 0);
> > > +assert(len == 0);
> > > +
> > > +return name;
> > > +}
> >
> > That function doesn't work.
> >
> > For a simple xvdp, (so disk==15), it return "", I mean "\0p".
> >
> > For a more complicated 'xvdbhwza', we have len == 22901. And the assert
> > failed.
> >
> > Maybe the recursing algo in libxl would be fine, with a buffer that is
> > big enough, and could probably be on the stack (in _get_vdev).
>
> I used libxl__device_disk_dev_number() as my model (as well as cross-
> checking with the spec), but I guess a recursing algorithm would be
> neater.
>
> >
> > > +
> > > +switch (vdev->type) {
> > > +case XEN_QDISK_VDEV_TYPE_DP:
> > > +case XEN_QDISK_VDEV_TYPE_XVD:
> > > +if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> > > +vdev->number = (202 << 8) | (vdev->disk << 4) |
> > > +vdev->partition;
> > > +} else if (vdev->disk < (1 << 20) && vdev->partition < (1 <<
> > 8)) {
> > > +vdev->number = (1 << 28) | (vdev->disk << 8) |
> > > +vdev->partition;
> > > +} else {
> > > +goto invalid;
> > > +}
> > > +break;
> > > +
> > > +case XEN_QDISK_VDEV_TYPE_HD:
> > > +if ((vdev->disk == 0 || vdev->disk == 1) &&
> > > +vdev->partition < (1 << 4)) {
> >
> > I think that should be:
> >
> > vdev->partition < (1 << 6)
> >
> > Because hd disk have 0..63 partitions.
>
> Yes, I must have typo-ed it...
>
> >
> > > +vdev->number = (3 << 8) | (vdev->disk << 6) | vdev-
> > >partition;
> > > +} else if ((vdev->disk == 2 || vdev->disk == 3) &&
> > > + vdev->partition < (1 << 4)) {
> >
> > same here.
>
> ...and then cut'n'pasted.
>
> >
> > > +vdev->number = (22 << 8) | ((vdev->disk
Re: [Qemu-block] [PATCH 16/18] xen: automatically create XenQdiskDevice-s
On Wed, Nov 21, 2018 at 03:12:09PM +, Paul Durrant wrote:
> This patch adds a creator function for XenQdiskDevice-s so that they can
> be created automatically when the Xen toolstack instantiates a new
> PV backend. When the XenQdiskDevice is created this way it is also
> necessary to create a drive which matches the configuration that the Xen
> toolstack has written into xenstore. This drive is marked 'auto_del' so
> that it will be removed when the XenQdiskDevice is destroyed. Also, for
> compatibilitye with the legacy 'xen_disk' implementation, an iothread
> is automatically created for the new XenQdiskDevice. This will also be
> removed when he XenQdiskDevice is destroyed.
"the XenQdiskDevice"
[...]
> +qemu_opt_set(drive_opts, "file.locking", "off", _err);
That looks new, compared to the xen_disk.c implementation. Maybe that
should be mention in the commit message.
[..]
> +static void xen_qdisk_device_create(BusState *bus, const char *name,
> +QDict *opts, Error **errp)
> +{
[...]
> +iothread = iothread_create(vdev, _abort);
I would just propagate the error, since iothread could fail for external
reason. No need to crash qemu while a VM is running.
> +
> +dev = qdev_create(bus, TYPE_XEN_QDISK_DEVICE);
> +
> +qdev_prop_set_string(dev, "vdev", vdev);
> +
> +if (XEN_QDISK_DEVICE(dev)->vdev.number != number) {
> +error_setg(errp, "invalid dev parameter '%s'", vdev);
> +goto unref;
> +}
> +
> +qdev_prop_set_drive(dev, "drive", blk, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +error_prepend(errp, "failed to set 'drive': ");
> +goto unref;
> +}
> +
> +XEN_QDISK_DEVICE(dev)->auto_iothread = iothread;
> +
> +qdev_init_nofail(dev);
That function shouldn't be use during hotplug. But I'm not sure what
should be done instead, probably object_property_set_bool(..., true
"realized") and check for error.
Thanks,
--
Anthony PERARD
Re: [Qemu-block] [PATCH 17/18] MAINTAINERS: add myself as a Xen maintainer
On Wed, Nov 21, 2018 at 03:12:10PM +, Paul Durrant wrote: > I have made many significant contributions to the Xen code in QEMU, > particularly the recent patches introducing a new PV device framework. > I intend to make further significant contributions, porting other PV back- > ends to the new framework with the intent of eventually removing the > legacy code. It therefore seems reasonable that I become a maintiner of > the Xen code. > > Signed-off-by: Paul Durrant With the typo fixed: Acked-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH 03/18] xen: introduce 'xen-qdisk'
> -Original Message-
> From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> Sent: 04 December 2018 15:49
> To: Paul Durrant
> Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen-
> de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> ; Stefano Stabellini
> Subject: Re: [PATCH 03/18] xen: introduce 'xen-qdisk'
>
> On Tue, Dec 04, 2018 at 03:20:04PM +, Paul Durrant wrote:
> > > > +static char *disk_to_vbd_name(unsigned int disk)
> > > > +{
> > > > +unsigned int len = DIV_ROUND_UP(disk, 26);
> > > > +char *name = g_malloc0(len + 1);
> > > > +
> > > > +do {
> > > > +name[len--] = 'a' + (disk % 26);
> > > > +disk /= 26;
> > > > +} while (disk != 0);
> > > > +assert(len == 0);
> > > > +
> > > > +return name;
> > > > +}
> > >
> > > That function doesn't work.
> > >
> > > For a simple xvdp, (so disk==15), it return "", I mean "\0p".
> > >
> > > For a more complicated 'xvdbhwza', we have len == 22901. And the
> assert
> > > failed.
> > >
> > > Maybe the recursing algo in libxl would be fine, with a buffer that is
> > > big enough, and could probably be on the stack (in _get_vdev).
> >
> > I used libxl__device_disk_dev_number() as my model (as well as cross-
> checking with the spec), but I guess a recursing algorithm would be
> neater.
>
> There is libxl__devid_to_vdev() actually just after ;-) which calls
> encode_disk_name which is recursing.
Ah, I'll look for that. Currently having trouble reconciling the 'tq' -> 536
mapping in the doc.
Paul
>
> > > > diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> > > > new file mode 100644
> > > > index 00..ade0866037
> > > > --- /dev/null
> > > > +++ b/include/hw/xen/xen-qdisk.h
> > > > @@ -0,0 +1,38 @@
> > > > +/*
> > > > + * Copyright (c) Citrix Systems Inc.
> > > > + * All rights reserved.
> > > > + */
> > > > +
> > > > +#ifndef HW_XEN_QDISK_H
> > > > +#define HW_XEN_QDISK_H
> > > > +
> > > > +#include "hw/xen/xen-bus.h"
> > > > +
> > > > +typedef enum XenQdiskVdevType {
> > > > +XEN_QDISK_VDEV_TYPE_DP,
> > >
> > > Maybe we could set type_dp value to 1, so that, when vdev->type isn't
> > > set, we can detect it later.
> >
> > Rather than having the 'valid' bool? Yes, that would work.
>
> Well, the 'valid' bool doesn't seems to always be check so it would be
> better. e.g. xen_qdisk_get_vdev() doesn't check `valid` before
> genereting a string. Then xen_qdisk_set_vdev could set `type` to invalid
> when it is invalid.
>
> --
> Anthony PERARD
Re: [Qemu-block] [PATCH 03/18] xen: introduce 'xen-qdisk'
On Tue, Dec 04, 2018 at 03:20:04PM +, Paul Durrant wrote:
> > > +static char *disk_to_vbd_name(unsigned int disk)
> > > +{
> > > +unsigned int len = DIV_ROUND_UP(disk, 26);
> > > +char *name = g_malloc0(len + 1);
> > > +
> > > +do {
> > > +name[len--] = 'a' + (disk % 26);
> > > +disk /= 26;
> > > +} while (disk != 0);
> > > +assert(len == 0);
> > > +
> > > +return name;
> > > +}
> >
> > That function doesn't work.
> >
> > For a simple xvdp, (so disk==15), it return "", I mean "\0p".
> >
> > For a more complicated 'xvdbhwza', we have len == 22901. And the assert
> > failed.
> >
> > Maybe the recursing algo in libxl would be fine, with a buffer that is
> > big enough, and could probably be on the stack (in _get_vdev).
>
> I used libxl__device_disk_dev_number() as my model (as well as cross-checking
> with the spec), but I guess a recursing algorithm would be neater.
There is libxl__devid_to_vdev() actually just after ;-) which calls
encode_disk_name which is recursing.
> > > diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> > > new file mode 100644
> > > index 00..ade0866037
> > > --- /dev/null
> > > +++ b/include/hw/xen/xen-qdisk.h
> > > @@ -0,0 +1,38 @@
> > > +/*
> > > + * Copyright (c) Citrix Systems Inc.
> > > + * All rights reserved.
> > > + */
> > > +
> > > +#ifndef HW_XEN_QDISK_H
> > > +#define HW_XEN_QDISK_H
> > > +
> > > +#include "hw/xen/xen-bus.h"
> > > +
> > > +typedef enum XenQdiskVdevType {
> > > +XEN_QDISK_VDEV_TYPE_DP,
> >
> > Maybe we could set type_dp value to 1, so that, when vdev->type isn't
> > set, we can detect it later.
>
> Rather than having the 'valid' bool? Yes, that would work.
Well, the 'valid' bool doesn't seems to always be check so it would be
better. e.g. xen_qdisk_get_vdev() doesn't check `valid` before
genereting a string. Then xen_qdisk_set_vdev could set `type` to invalid
when it is invalid.
--
Anthony PERARD
Re: [Qemu-block] [PATCH 15/18] xen: add a mechanism to automatically create XenDevice-s...
On Wed, Nov 21, 2018 at 03:12:08PM +, Paul Durrant wrote:
> +xen_backend_device_create(BUS(xenbus), type, name, opts, _err);
> +qobject_unref(opts);
> +
> +if (local_err) {
> +const char *msg = error_get_pretty(local_err);
> +
> +error_report("failed to create '%s' device '%s': %s", type, name,
> + msg);
> +error_free(local_err);
You can use error_reportf_err() instead of those three calls. I may have
only suggest error_report_err in a previous patch, but error_reportf_err
does the error_prepend as well.
> +}
> +}
> +
> +static void xen_bus_type_enumerate(XenBus *xenbus, const char *type)
> +{
> +char *domain_path = g_strdup_printf("backend/%s/%u", type, xen_domid);
> +char **backend;
> +unsigned int i, n;
> +
> +trace_xen_bus_type_enumerate(type);
> +
> +backend = xs_directory(xenbus->xsh, XBT_NULL, domain_path, );
> +if (!backend) {
domain_path isn't free here, you probably want a `goto out` which would
free everything.
> +return;
> +}
> +
> @@ -193,6 +302,17 @@ static void xen_bus_realize(BusState *bus, Error **errp)
> notifier_list_init(>watch_notifiers);
> qemu_set_fd_handler(xs_fileno(xenbus->xsh), xen_bus_watch, NULL,
> xenbus);
> +
> +module_call_init(MODULE_INIT_XEN_BACKEND);
> +
> +xenbus->backend_watch =
> +xen_bus_add_watch(xenbus, "", /* domain root node */
> + "backend", xen_bus_enumerate, xenbus, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +error_prepend(errp, "failed to set up enumeration watch: ");
You should use error_propagate_prepend instead
error_propagate;error_prepend. And it looks like there is the same
mistake in other patches that I haven't notice.
Also you probably want goto fail here.
> +static void xen_device_backend_changed(void *opaque)
> +{
> +XenDevice *xendev = opaque;
> +const char *type = object_get_typename(OBJECT(xendev));
> +enum xenbus_state state;
> +unsigned int online;
> +
> +trace_xen_device_backend_changed(type, xendev->name);
> +
> +if (xen_device_backend_scanf(xendev, "state", "%u", ) != 1) {
> +state = XenbusStateUnknown;
> +}
> +
> +xen_device_backend_set_state(xendev, state);
It's kind of weird to set the internal state base on the external one
that something else may have modified. Shouldn't we check that it is
fine for something else to modify the state and that it is a correct
transition?
Also aren't we going in a loop by having QEMU set the state, then the
watch fires again? (Not really a loop since the function _set_state
check for changes.
Also maybe we should watch for the state changes only when something
else like libxl creates (ask for) the backend, and ignore changes when
QEMU did it itself.
> +
> +if (xen_device_backend_scanf(xendev, "online", "%u", ) != 1) {
> +online = 0;
> +}
> +
> +xen_device_backend_set_online(xendev, !!online);
> +
> +/*
> + * If a backend is still 'online' then its state should be cycled
> + * back round to InitWait in order for a new frontend instance to
> + * connect. This may happen when, for example, a frontend driver is
> + * re-installed or updated.
> + * If a backend id not 'online' then the device should be destroyed.
s/id/is/
> + */
> +if (xendev->backend_online &&
> +xendev->backend_state == XenbusStateClosed) {
> +xen_device_backend_set_state(xendev, XenbusStateInitWait);
> +} else if (!xendev->backend_online &&
> + (xendev->backend_state == XenbusStateClosed ||
> +xendev->backend_state == XenbusStateInitialising ||
> +xendev->backend_state == XenbusStateInitWait ||
> +xendev->backend_state == XenbusStateUnknown)) {
> +object_unparent(OBJECT(xendev));
> +}
> +}
> +
> static void xen_device_backend_create(XenDevice *xendev, Error **errp)
> {
> XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> @@ -289,12 +463,38 @@ static void xen_device_backend_create(XenDevice
> *xendev, Error **errp)
> error_propagate(errp, local_err);
> error_prepend(errp, "failed to create backend: ");
It looks like there is a missing return here.
> }
> +
> +xendev->backend_state_watch =
> +xen_bus_add_watch(xenbus, xendev->backend_path,
> + "state", xen_device_backend_changed,
> + xendev, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +error_prepend(errp, "failed to watch backend state: ");
You should return here, as local_err mustn't be reused.
> +}
> +
> +xendev->backend_online_watch =
> +xen_bus_add_watch(xenbus, xendev->backend_path,
> + "online", xen_device_backend_changed,
> + xendev, _err);
> +if
Re: [Qemu-block] [Qemu-devel] [PATCH for-next 0/6] qemu-img: Add salvaging mode to convert
On 12/3/18 9:39 PM, no-re...@patchew.org wrote: Patchew URL: https://patchew.org/QEMU/20181203175211.8230-1-mre...@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 fatal: unable to access 'https://github.com/patchew-project/qemu/': Operation timed out after 38 milliseconds with 0 out of 0 bytes received False negative. It would be nice to figure out why patchew had a connection problem, but it would also be nice if patchew didn't send out failure notices when the failure is due to a git clone issue unrelated to the patch itself. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@redhat.com -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH 03/18] xen: introduce 'xen-qdisk'
> -Original Message-
> From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> Sent: 29 November 2018 16:06
> To: Paul Durrant
> Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen-
> de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> ; Stefano Stabellini
> Subject: Re: [PATCH 03/18] xen: introduce 'xen-qdisk'
>
> On Wed, Nov 21, 2018 at 03:11:56PM +, Paul Durrant wrote:
> > This patch adds a new XenDevice: 'xen-qdisk' [1]. This will eventually
> > replace the 'xen_disk' legacy PV backend but it is illustrative to build
> > up the implementation incrementally, along with the XenBus/XenDevice
> > framework. Subsequent patches will therefore add to this device's
> > implementation as new features are added to the framework.
> >
> > After this patch has been applied it is possible to instantiate a new
> > 'xen-qdisk' device with a single 'vdev' parameter, which accepts values
> > adhering to the Xen VBD naming scheme [2]. For example, a command-line
> > instantiation of a xen-qdisk can be done with an argument similar to the
> > following:
> >
> > -device xen-qdisk,vdev=hda
>
> That works when QEMU boot, but doing the same thing once the guest have
> booted, via QMP, doesn't. Here is the result (tested in qmp-shell):
>
> (QEMU) device_add driver=xen-qdisk vdev=hda
> {
> "error": {
> "class": "GenericError",
> "desc": "Bus 'xen-bus.0' does not support hotplugging"
> }
> }
>
> That's probably why I've asked about the hotplug capability on the
> previous patch.
>
Ok. I've added the hotplug now so I'll make sure QMP DTRT.
> > The implementation of the vdev parameter formulates the appropriate VBD
> > number for use in the PV protocol.
> >
> > [1] The name 'qdisk' as always been the name given to the QEMU
> > implementation of the Xen PV block protocol backend implementation
> > [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/man/xen-vbd-
> interface.markdown.7
>
> Maybe a link to the generated docs would be better:
> https://xenbits.xen.org/docs/unstable/man/xen-vbd-interface.7.html
>
Ok.
> Also, it would be useful to have the same link in the source code.
>
Yes, I'll add a comment.
> > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > new file mode 100644
> > index 00..72122073f7
> > --- /dev/null
> > +++ b/hw/block/xen-qdisk.c
> [...]
> > +static char *disk_to_vbd_name(unsigned int disk)
> > +{
> > +unsigned int len = DIV_ROUND_UP(disk, 26);
> > +char *name = g_malloc0(len + 1);
> > +
> > +do {
> > +name[len--] = 'a' + (disk % 26);
> > +disk /= 26;
> > +} while (disk != 0);
> > +assert(len == 0);
> > +
> > +return name;
> > +}
>
> That function doesn't work.
>
> For a simple xvdp, (so disk==15), it return "", I mean "\0p".
>
> For a more complicated 'xvdbhwza', we have len == 22901. And the assert
> failed.
>
> Maybe the recursing algo in libxl would be fine, with a buffer that is
> big enough, and could probably be on the stack (in _get_vdev).
I used libxl__device_disk_dev_number() as my model (as well as cross-checking
with the spec), but I guess a recursing algorithm would be neater.
>
> > +
> > +switch (vdev->type) {
> > +case XEN_QDISK_VDEV_TYPE_DP:
> > +case XEN_QDISK_VDEV_TYPE_XVD:
> > +if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> > +vdev->number = (202 << 8) | (vdev->disk << 4) |
> > +vdev->partition;
> > +} else if (vdev->disk < (1 << 20) && vdev->partition < (1 <<
> 8)) {
> > +vdev->number = (1 << 28) | (vdev->disk << 8) |
> > +vdev->partition;
> > +} else {
> > +goto invalid;
> > +}
> > +break;
> > +
> > +case XEN_QDISK_VDEV_TYPE_HD:
> > +if ((vdev->disk == 0 || vdev->disk == 1) &&
> > +vdev->partition < (1 << 4)) {
>
> I think that should be:
>
> vdev->partition < (1 << 6)
>
> Because hd disk have 0..63 partitions.
Yes, I must have typo-ed it...
>
> > +vdev->number = (3 << 8) | (vdev->disk << 6) | vdev-
> >partition;
> > +} else if ((vdev->disk == 2 || vdev->disk == 3) &&
> > + vdev->partition < (1 << 4)) {
>
> same here.
...and then cut'n'pasted.
>
> > +vdev->number = (22 << 8) | ((vdev->disk - 2) << 6) |
> > +vdev->partition;
> > +} else {
> > +goto invalid;
> > +}
> > +break;
> > +
> > +case XEN_QDISK_VDEV_TYPE_SD:
> > +if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> > +vdev->number = (8 << 8) | (vdev->disk << 4) | vdev-
> >partition;
> > +} else {
> > +goto invalid;
> > +}
> > +break;
> > +
> > +default:
> > +goto invalid;
> > +}
> > +
> > +g_free(str);
> > +vdev->valid = true;
> > +return;
> > +
> > +invalid:
> > +error_setg(errp, "invalid virtual disk specifier");
> > +
Re: [Qemu-block] [PATCH 07/18] xen: add event channel interface for XenDevice-s
On Mon, Dec 03, 2018 at 04:24:24PM +, Anthony PERARD wrote:
> On Wed, Nov 21, 2018 at 03:12:00PM +, Paul Durrant wrote:
> > +static void xen_device_event(void *opaque)
> > +{
> > +XenDevice *xendev = opaque;
> > +unsigned long port = xenevtchn_pending(xendev->xeh);
> > +
> > +notifier_list_notify(>event_notifiers, (void *)port);
>
> I wonder if a Notifier is a good fit for XenDevice, like here for the
> events or the xenstore watches in previous patches, as NotifierLists are
> normaly used when every Notifiers want to do something, but here there
> is only one that is going to do something. But I guess it might not be
> much better to write a loop in here rather than use the one in
> notifier_list_notify.
I've seen that you use GHashTable in a following patch, wouldn't that be
useful to use for xenstore watches and evtchn events as well?
--
Anthony PERARD
Re: [Qemu-block] [qemu-s390x] [Qemu-devel] [PULL 2/2] iotests: simple mirror test with kvm on 1G image
On 04.12.2018 14:46, Christian Borntraeger wrote:
> FWIW, this testcase fails with current qemu master on s390:
>
> QEMU --
> "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../s390x-softmmu/qemu-system-s390x"
> -nodefaults -machine accel=qtest
> QEMU_IMG --
> "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-img"
> QEMU_IO --
> "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-io" --cache
> writeback -f qcow2
> QEMU_NBD --
> "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-nbd"
> IMGFMT-- qcow2 (compat=1.1)
> IMGPROTO -- file
> PLATFORM -- Linux/s390x s38lp08 4.19.0+
> TEST_DIR -- /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/scratch
> SOCKET_SCM_HELPER --
> /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/socket_scm_helper
> 235
> [failed, exit status 1] - output mismatch (see 235.out.bad)
> --- /home/cborntra/REPOS/qemu/tests/qemu-iotests/235.out 2018-12-04
> 14:44:27.913714608 +0100
> +++ /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/235.out.bad
> 2018-12-04 14:44:51.512958864 +0100
> @@ -1,3 +1,14 @@
> -{"return": {}}
> -{"return": {}}
> -{"return": {}}
> +Traceback (most recent call last):
> + File "235", line 54, in
> +vm.launch()
> + File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py",
> line 295, in launch
> +self._launch()
> + File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py",
> line 321, in _launch
> +self._post_launch()
> + File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py",
> line 266, in _post_launch
> +self._qmp.accept()
> + File
> "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line
> 155, in accept
> +self.__sock, _ = self.__sock.accept()
> + File "/usr/lib64/python2.7/socket.py", line 206, in accept
> +sock, addr = self._sock.accept()
> +socket.timeout: timed out
>
forgot to cc Vladimir
>
> On 03.12.2018 17:58, Kevin Wolf wrote:
>> From: Vladimir Sementsov-Ogievskiy
>>
>> This test is broken without previous commit fixing dead-lock in mirror.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> Signed-off-by: Max Reitz
>> Acked-by: Vladimir Sementsov-Ogievskiy
>> Signed-off-by: Kevin Wolf
>> ---
>> tests/qemu-iotests/235 | 76 ++
>> tests/qemu-iotests/235.out | 3 ++
>> tests/qemu-iotests/group | 1 +
>> 3 files changed, 80 insertions(+)
>> create mode 100755 tests/qemu-iotests/235
>> create mode 100644 tests/qemu-iotests/235.out
>>
>> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
>> new file mode 100755
>> index 00..da044ed34e
>> --- /dev/null
>> +++ b/tests/qemu-iotests/235
>> @@ -0,0 +1,76 @@
>> +#!/usr/bin/env python
>> +#
>> +# Simple mirror test
>> +#
>> +# Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
>> +#
>> +# 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
Re: [Qemu-block] [Qemu-devel] [PULL 2/2] iotests: simple mirror test with kvm on 1G image
FWIW, this testcase fails with current qemu master on s390:
QEMU --
"/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../s390x-softmmu/qemu-system-s390x"
-nodefaults -machine accel=qtest
QEMU_IMG --
"/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-img"
QEMU_IO --
"/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-io" --cache
writeback -f qcow2
QEMU_NBD --
"/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-nbd"
IMGFMT-- qcow2 (compat=1.1)
IMGPROTO -- file
PLATFORM -- Linux/s390x s38lp08 4.19.0+
TEST_DIR -- /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/scratch
SOCKET_SCM_HELPER --
/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/socket_scm_helper
235
[failed, exit status 1] - output mismatch (see 235.out.bad)
--- /home/cborntra/REPOS/qemu/tests/qemu-iotests/235.out2018-12-04
14:44:27.913714608 +0100
+++ /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/235.out.bad
2018-12-04 14:44:51.512958864 +0100
@@ -1,3 +1,14 @@
-{"return": {}}
-{"return": {}}
-{"return": {}}
+Traceback (most recent call last):
+ File "235", line 54, in
+vm.launch()
+ File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py",
line 295, in launch
+self._launch()
+ File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py",
line 321, in _launch
+self._post_launch()
+ File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py",
line 266, in _post_launch
+self._qmp.accept()
+ File
"/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line
155, in accept
+self.__sock, _ = self.__sock.accept()
+ File "/usr/lib64/python2.7/socket.py", line 206, in accept
+sock, addr = self._sock.accept()
+socket.timeout: timed out
On 03.12.2018 17:58, Kevin Wolf wrote:
> From: Vladimir Sementsov-Ogievskiy
>
> This test is broken without previous commit fixing dead-lock in mirror.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> Signed-off-by: Max Reitz
> Acked-by: Vladimir Sementsov-Ogievskiy
> Signed-off-by: Kevin Wolf
> ---
> tests/qemu-iotests/235 | 76 ++
> tests/qemu-iotests/235.out | 3 ++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 80 insertions(+)
> create mode 100755 tests/qemu-iotests/235
> create mode 100644 tests/qemu-iotests/235.out
>
> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> new file mode 100755
> index 00..da044ed34e
> --- /dev/null
> +++ b/tests/qemu-iotests/235
> @@ -0,0 +1,76 @@
> +#!/usr/bin/env python
> +#
> +# Simple mirror test
> +#
> +# Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
> +#
> +# 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
Re: [Qemu-block] [PATCH 14/18] xen: add implementations of xen-qdisk connect and disconnect functions...
On Wed, Nov 21, 2018 at 03:12:07PM +, Paul Durrant wrote:
> diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> index 35f7b70480..8c88393832 100644
> --- a/hw/block/xen-qdisk.c
> +++ b/hw/block/xen-qdisk.c
> static void xen_qdisk_connect(XenQdiskDevice *qdiskdev, Error **errp)
> {
> XenQdiskVdev *vdev = >vdev;
> +XenDevice *xendev = XEN_DEVICE(qdiskdev);
> +unsigned int order, nr_ring_ref, *ring_ref, event_channel, protocol;
> +char *str;
>
> trace_xen_qdisk_connect(vdev->disk, vdev->partition);
> +
> +if (xen_device_frontend_scanf(xendev, "ring-page-order", "%u",
> + ) != 1) {
> +nr_ring_ref = 1;
> +ring_ref = g_new(unsigned int, nr_ring_ref);
> +
> +if (xen_device_frontend_scanf(xendev, "ring-ref", "%u",
> + _ref[0]) != 1) {
> +error_setg(errp, "failed to read ring-ref");
Don't you need to free `ring_ref`?
> +return;
> +}
[...]
> diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> index ade0866037..d7dd2bf0ee 100644
> --- a/include/hw/xen/xen-qdisk.h
> +++ b/include/hw/xen/xen-qdisk.h
> @@ -6,7 +6,15 @@
> #ifndef HW_XEN_QDISK_H
> #define HW_XEN_QDISK_H
>
> +#include "hw/xen/xen.h"
> #include "hw/xen/xen-bus.h"
> +#include "hw/block/block.h"
> +#include "hw/block/xen_blkif.h"
> +#include "hw/block/dataplane/xen-qdisk.h"
> +#include "sysemu/blockdev.h"
> +#include "sysemu/iothread.h"
> +#include "sysemu/block-backend.h"
> +#include "sysemu/iothread.h"
You don't need that many includes, especially not iothread.h twice ;-).
I think those new includes would be enough:
#include "hw/block/block.h"; for BlockConf
#include "sysemu/iothread.h"
#include "hw/block/dataplane/xen-qdisk.h"
>
> typedef enum XenQdiskVdevType {
> XEN_QDISK_VDEV_TYPE_DP,
> @@ -33,6 +41,10 @@ typedef struct XenQdiskDevice XenQdiskDevice;
> struct XenQdiskDevice {
> XenDevice xendev;
> XenQdiskVdev vdev;
> +BlockConf conf;
> +unsigned int max_ring_page_order;
> +IOThread *iothread;
> +XenQdiskDataPlane *dataplane;
> };
>
> #endif /* HW_XEN_QDISK_H */
--
Anthony PERARD
Re: [Qemu-block] [PATCH 13/18] xen: purge 'blk' and 'ioreq' from function names in dataplane/xen-qdisk.c
On Wed, Nov 21, 2018 at 03:12:06PM +, Paul Durrant wrote: > This is a purely cosmetic patch that purges remaining use of 'blk' and > 'ioreq' in local function names. > > No functional change. > > Signed-off-by: Paul Durrant I don't think it's a good idee to use function names that could be use elsewhere, don't have a namespace. It makes it more difficult to figure out which function is called by just searching for the function name. Could you had a prefix? Maybe xendisk_ or xen_disk or xen_qdisk or xen_block or ..., so we can have xendisk_start_request, or xendisk_request_start. I don't have a preference beside staying away from generic names. Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH 12/18] xen: remove 'ioreq' struct/varable/field names from dataplane/xen-qdisk.c
On Wed, Nov 21, 2018 at 03:12:05PM +, Paul Durrant wrote: > This is a purely cosmetic patch that purges the name 'ioreq' from struct, > variable and field names. (This name has been problematic for a long time > as 'ioreq' is the name used for generic I/O requests coming from Xen). > The patch replaces 'struct ioreq' with a new 'XenQdiskRequest' type and > 'ioreq' field/variable names with 'request', and then does necessary > fix-up to adhere to coding style. > > Function names are not modified by this patch. Yhey will be dealt with in s/Yhey/They/ > a subsequent patch. > > No functional change. > > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH 11/18] xen: remove 'XenBlkDev' and 'blkdev' names from dataplane/xen-qdisk
On Wed, Nov 21, 2018 at 03:12:04PM +, Paul Durrant wrote: > This is a purely cosmetic patch that substitutes the old 'struct XenBlkDev' > name with 'XenQdiskDataPlane' and 'blkdev' field/variable names with > 'dataplane', and then does necessary fix-up to adhere to coding style. > > No functional change. > > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD -- Anthony PERARD
