On Mon, 2019-11-25 at 19:45 +0100, Max Reitz wrote:
> On 22.11.19 15:22, Maxim Levitsky wrote:
> > Hi!
> >
> > This is the second version of the proposed QMP API for key management,
> > after discussion with Keven and Max.
> >
> > Will this work?
> >
(I'll drop the -x prefix if you like), and
wire it
to luks and qcow2 driver to implement qmp based encryption slot management
also using
the code from patches 1,2, and also add a bunch of iotests to cover this.
Best regards,
Maxim Levitsky
Maxim Levitsky (13):
qcrypto: add ge
rename the write_func to create_write_func, and init_func to create_init_func.
This is preparation for other write_func that will be used to update the
encryption keys.
No functional changes
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
block/crypto.c | 25
This will be used first to implement luks keyslot management.
block_crypto_amend_opts_init will be used to convert
qemu-img cmdline to QCryptoBlockAmendOptions
Signed-off-by: Maxim Levitsky
---
block/crypto.c | 17 +
block/crypto.h | 3 +++
crypto/block.c
'force' option will be used for some unsafe amend operations.
This includes things like erasing last keyslot in luks based formats
(which destroys the data, unless the master key is backed up
by external means), but that _might_ be desired result.
Signed-off-by: Maxim Levitsky
R
it in each action option list.
In future it might be useful to remove some options which are
not supported anyway from amend list, which currently
cause an error message if amended.
Signed-off-by: Maxim Levitsky
---
block/qcow2.c | 160 +-
include
Now that we have all the infrastructure in place,
wire it in the qcow2 driver and expose this to the user.
Signed-off-by: Maxim Levitsky
---
block/qcow2.c | 101 +++---
1 file changed, 79 insertions(+), 22 deletions(-)
diff --git a/block/qcow2.c b
This allows more tests to be able to have same output on both qcow2 luks
encrypted images
and raw luks images
Signed-off-by: Maxim Levitsky
---
tests/qemu-iotests/087.out | 6 +++---
tests/qemu-iotests/134.out | 2 +-
tests/qemu-iotests/158.out | 4 ++--
tests/qemu-iotests
blockdev-amend will be used similiar to blockdev-create
to allow on the fly changes of the structure of the format based block devices.
Current plan is to first support encryption keyslot management for luks
based formats (raw and embedded in qcow2)
Signed-off-by: Maxim Levitsky
---
block
Currently the implementation only supports amending the encryption
options, unlike the qemu-img version
Signed-off-by: Maxim Levitsky
---
block/qcow2.c| 39 +++
qapi/block-core.json | 16 +++-
2 files changed, 54 insertions(+), 1 deletion
This commit adds two tests that cover the
new blockdev-amend functionality of luks and qcow2 driver
Signed-off-by: Maxim Levitsky
---
tests/qemu-iotests/302 | 284 +
tests/qemu-iotests/302.out | 40 ++
tests/qemu-iotests/303 | 235
ange for the idea of
unsharing read, rather that write permission which allows
to avoid cases when the other user had opened the image read-only.
Signed-off-by: Maxim Levitsky
---
block/crypto.c | 130 +++--
block/crypto.h | 31
2 files changed,
This commit adds two tests, which test the new amend interface
of both luks raw images and qcow2 luks encrypted images.
Signed-off-by: Maxim Levitsky
---
tests/qemu-iotests/300 | 207 +
tests/qemu-iotests/300.out | 99 ++
tests/qemu
Next few patches will expose that functionality
to the user.
Signed-off-by: Maxim Levitsky
---
crypto/block-luks.c | 374 +++-
qapi/crypto.json| 50 +-
2 files changed, 421 insertions(+), 3 deletions(-)
diff --git a/crypto/block-luks.c b/crypto
Signed-off-by: Maxim Levitsky
---
block/crypto.c | 70
qapi/block-core.json | 14 -
2 files changed, 64 insertions(+), 20 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index 081880bced..6836337863 100644
--- a/block/crypto.c
-lks33yi2/src'
> make: *** [docker-run-test-quick@centos7] Error 2
>
> real11m29.420s
> user 0m8.596s
>
>
> The full log is available at
> http://patchew.org/logs/20200114193350.10830-1-mlevi...@redhat.com/testing.docker-quick@centos7/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-de...@redhat.com
Hi, this is my fault. I made tiny change in the error message, and didn't update
the output of 300 test.
I'll fix that in next version of the patches.
Best regards,
Maxim Levitsky
Checking commit fcf88585 (block/crypto: implement blockdev-amend)
> 12/13 Checking commit 005f7d83f052 (block/qcow2: implement blockdev-amend)
> 13/13 Checking commit c97e00f6078b (iotests: add tests for blockdev-amend)
> WARNING: added, moved or deleted file(s), does MAINTAINERS n
following commit:
> > > >
> > > > 4d7c487eac1652dfe4498fe84f32900ad461d61b is the first bad commit
> > > > commit 4d7c487eac1652dfe4498fe84f32900ad461d61b
> > > > Author: Max Reitz
> > > > Date: Wed Jul 24 19:12:29 2019 +0200
> >
+# if not given will use the same secret as one
>
> "as the one"?
Yea, this is wrong wording, I'll drop those words. Thanks.
>
> > +# that was used to open the image
> > +#
> > +# Since: 5.0
> > +##
> > +{ 'struct': 'QCryptoBlockAmendOptionsLUKS',
> > + 'data' : {
> > +'keys': ['LUKSKeyslotUpdate'],
> > + '*unlock-secret' : 'str' } }
> > +
> >
> >
> > ##
> > @@ -324,4 +372,4 @@
> > 'base': 'QCryptoBlockOptionsBase',
> > 'discriminator': 'format',
> > 'data': {
> > -} }
> > + 'luks': 'QCryptoBlockAmendOptionsLUKS' } }
Thanks for review,
Best regards,
Maxim Levitsky
On Tue, 2020-01-21 at 08:59 +0100, Markus Armbruster wrote:
> Maxim Levitsky writes:
>
> > blockdev-amend will be used similiar to blockdev-create
> > to allow on the fly changes of the structure of the format based block
> > devices.
> >
> > Current plan is
On Thu, 2020-07-16 at 14:33 +0300, Maxim Levitsky wrote:
> This is basically the same thing as commit
> 'crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails'
> does but for qcow2 files to ensure that we don't leave qcow2 files
> when creation fails
Then block driver clients (like scsi devices that you mention, nbd, etc)
can choose which limit to use, depending on which IO api they use.
The scsi-generic, and scsi-block can use the SG_IO limits,
while the rest can use the normal (unlimited for file I/O) limits, including
the NBD.
Best re
set it to, is probably
> beyond what I can take.
Implementation wise, I think I can do this, but I'll wait few days for the
feedback on my proposal.
Thanks for finding this bug!
Best regards,
Maxim Levitsky
>
> IMHO my patches should be merged first (they are really
device file is a character device.
>
> So is this path ever taken, or can we just replace it all with the ioctl?
>
> (Before 867eccfed84, this function was used for all host devices, which
> might explain why the code even exists.)
>
> Max
I have another proposal which I am currently evaluating.
How about we drop all the SG_IO limits code alltogher from the raw driver, and
instead just let the scsi drivers (scsi-block and scsi-generic) query
the device directly, since I don't think that the kernel (I will double check
this)?
Best regards,
Maxim Levitsky
On Thu, 2020-09-17 at 16:22 +0300, Maxim Levitsky wrote:
> On Thu, 2020-09-17 at 15:16 +0200, Max Reitz wrote:
> > On 12.08.20 00:51, Dmitry Fomichev wrote:
> > > If scsi-generic driver is in use, an SG node can be specified in
> > > the command line in place of a regula
Soon, a device removal might only happen on RCU callback execution.
This is okay for device-del which provides a DEVICE_DELETED event,
but not for the failure case of device-add. To avoid changing
monitor semantics, just drain all pending RCU callbacks on error.
Signed-off-by: Maxim Levitsky
This change will allow us to convert the bus children list to RCU,
while not changing the logic of this function
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-2-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.
This is a very small first step to make this code thread safe.
Suggested-by: Paolo Bonzini
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-5-mlevi...@redhat.com>
[Use RCU_READ_LOCK_GUARD in more places, adjust testcase now that
t
ned-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-6-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/core/qdev.c | 19 ++-
include/hw/qdev-core.h | 2 ++
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/hw/
qtest_qmp_event_ref a function which only scans
the buffer that qtest_qmp_receive stores the events to.
This is intended for callers that are only interested in events that were
received during the last call to the qtest_qmp_receive.
Suggested-by: Paolo Bonzini
Signed-off-by: Maxim Levitsky
---
tests/qtest
In the next patch a new version of qtest_qmp_receive will be
reintroduced that will buffer received qmp events for later
consumption in qtest_qmp_eventwait_ref
No functional change intended.
Suggested-by: Paolo Bonzini
Signed-off-by: Maxim Levitsky
---
tests/qtest/ahci-test.c| 4
_del since it is fixed now by my qtest patches.
The iotest 67 still fails with this, something that Paulo
is looking to fix before merging this.
Best regards,
Maxim Levitsky
Maxim Levitsky (10):
qtest: rename qtest_qmp_receive to qtest_qmp_receive_dict
qtest: Reintroduce qtest_qmp_recei
The only remaining users of qtest_qmp_receive_dict are tests
that fuzz the QMP protocol.
Tested with 'make check-qtest'.
Signed-off-by: Maxim Levitsky
---
tests/qtest/ahci-test.c | 4 +-
tests/qtest/device-plug-test.c | 2 +-
tests/qtest/drive_del-test.c| 9 ++---
t
Bonzini
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-7-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.c | 83 +-
1 file changed, 53 insertions(+), 30 deletions(-)
diff --gi
From: Paolo Bonzini
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.c | 122 -
1 file changed, 75 insertions(+), 47 deletions(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 3284a5d1fb..94921c04b1 100644
--- a/hw/scsi/scsi-bus.c
+++ b
This will help us to avoid the scsi device disappearing
after we took a reference to it.
It doesn't by itself forbid case when we try to access
an unrealized device
Suggested-by: Stefan Hajnoczi
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259
From: Paolo Bonzini
Check if an address is free on the bus before plugging in the
device. This makes it possible to do the check without any
side effects, and to detect the problem early without having
to do it in the realize callback.
Signed-off-by: Paolo Bonzini
---
hw/core/qdev.c |
Add scsi_device_get which finds the scsi device
and takes a reference to it.
Suggested-by: Stefan Hajnoczi
Signed-off-by: Maxim Levitsky
Message-Id: <20200913160259.32145-8-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.c | 11 +++
include/hw/scsi/
can
avoid iterating twice, and therefore we avoid this race.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1866707
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-10-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.
On Tue, 2020-10-06 at 14:56 +0200, Paolo Bonzini wrote:
> On 06/10/20 14:38, Maxim Levitsky wrote:
> > The only remaining users of qtest_qmp_receive_dict are tests
> > that fuzz the QMP protocol.
> >
> > Tested with 'make check-qtest'.
>
> Probably the
On Tue, 2020-09-01 at 15:30 +0200, Alberto Garcia wrote:
> On Thu 16 Jul 2020 01:33:59 PM CEST, Maxim Levitsky wrote:
> > if (ret < 0) {
> > +
> > +Error *local_delete_err = NULL;
> > +int r_del = bdrv_co_del
Use the bdrv_co_delete_file interface to delete the underlying
file if qcow2 initilization fails (e.g due to bad encryption secret)
This gives the qcow2 the same treatment as to luks.
V2: added a patch to fix a memory leak.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1845353
Maxim
In the case when underlying block device doesn't support the
bdrv_co_delete_file interface, an 'Error' wasn't freed.
Signed-off-by: Maxim Levitsky
---
block/crypto.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/crypto.c b/block/crypto.c
index 0807557763..9b6
If the qcow initialization fails after we created the storage file,
we should remove it to avoid leaving stale files around.
We already do this for luks raw images.
Signed-off-by: Maxim Levitsky
---
block/qcow2.c | 12
1 file changed, 12 insertions(+)
diff --git a/block/qcow2.c b
On Tue, 2020-10-13 at 14:26 +0200, Alberto Garcia wrote:
> On Sun 11 Oct 2020 12:21:35 PM CEST, Maxim Levitsky wrote:
> > In the case when underlying block device doesn't support the
> > bdrv_co_delete_file interface, an 'Error' wasn't freed.
>
By a mistake I added the pending events in a wrong order.
Fix this by using g_list_append.
Signed-off-by: Maxim Levitsky
---
tests/qtest/libqtest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 08929f5ff6..bd96cb6fdd
Just few fixes, for some stuff that slipped thorough.
Tested with make check, and qcow2/raw/nbd iotests.
Best regards,
Maxim Levitsky
Maxim Levitsky (4):
qdev: Fix two typos
libqtest: fix the order of buffered events
libqtest: fix memory leak in the qtest_qmp_event_ref
iotests
Signed-off-by: Maxim Levitsky
---
include/hw/qdev-core.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 868973319e..3761186804 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -163,8 +163,8
The g_list_remove_link doesn't free the link element,
opposed to what I thought.
Switch to g_list_delete_link that does free it.
Also refactor the code a bit.
Thanks for Max Reitz for helping me with this.
Signed-off-by: Maxim Levitsky
---
tests/qtest/libqtest.c | 11 ---
1
The recent changes that brought RCU delayed device deletion,
broke few tests and this test breakage went unnoticed.
Fix this test by rewriting it in python
(which allows to wait for DEVICE_DELETED events before continuing).
Signed-off-by: Maxim Levitsky
---
tests/qemu-iotests/240 | 228
On Thu, 2020-11-12 at 17:04 +0200, Maxim Levitsky wrote:
> On Thu, 2020-11-12 at 07:45 -0600, Eric Blake wrote:
> > On 11/12/20 6:40 AM, Peter Lieven wrote:
> >
> > > > /*
> > > > - * Avoid that s->sector_next_statu
which I think is similar to this patch series.
Sorry that I kind of forgot about it for too much time.
https://www.mail-archive.com/qemu-devel@nongnu.org/msg768261.html
I'll need some time to swap-in this area so that I could compare our
patches to see if we missed something.
Best regards,
Maxim Levitsky
_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
> > major(st.st_rdev), minor(st.st_rdev));
> > sysfd = open(sysfspath, O_RDONLY);
> >
>
>
Other than that, this is the same as the patch from Tom Yan:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg768262.html
In this version he does check if the SG_GET_SG_TABLESIZE is defined, so
you might want to do this as well.
Best regards,
Maxim Levitsky
something
as I haven't touched this area for a long time.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
> ---
> block/block-backend.c | 12
> block/file-posix.c | 2 +-
> block/io.c | 1 +
> hw
l.max_iov = ret;
> }
Roughly speaking this looks correct, but I might have missed something as well.
This is roughly the same as patches from Tom Yan which I carried in my series
https://www.mail-archive.com/qemu-devel@nongnu.org/msg768258.html
https://www.mail-archive.com/qemu-devel@nongnu.org/msg768262.html
I like a bit more how he created separate functions for /dev/sg and for all
other block devices.
Please take a look.
Also not related to this patch, you are missing my fix I did to the VPD limit
emulation, please consider taking
it into the series:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg768260.html
Best regards,
Maxim Levitsky
the max iovec number.
https://www.mail-archive.com/qemu-devel@nongnu.org/msg768264.html
Best regards,
Maxim Levitsky
> }
> }
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 98c30c5d5c..82e1e2ee79 100644
> --- a/hw/scsi/scsi-g
OMEM and -ENOSPC
and recycle the mappings in both cases?
I think that would work on both old and new kernels.
What do you think?
Best regards,
Maxim Levitsky
>
> Should I check uname(2)'s utsname.release[]? Is it reliable?
>
> > The block driver started to mis
emu-iotests/122
> +++ b/tests/qemu-iotests/122
> @@ -67,7 +67,7 @@ echo
> _make_test_img -b "$TEST_IMG".base -F $IMGFMT
>
> $QEMU_IO -c "write -P 0 0 3M" "$TEST_IMG" 2>&1 | _filter_qemu_io |
> _filter_testdir
> -$QEMU_IMG convert -O $IMGFMT -B "$TEST_IMG".base -o backing_fmt=$IMGFMT \
> +$QEMU_IMG convert -O $IMGFMT -B "$TEST_IMG".base -F $IMGFMT \
> "$TEST_IMG" "$TEST_IMG".orig
> $QEMU_IO -c "read -P 0 0 3M" "$TEST_IMG".orig 2>&1 | _filter_qemu_io |
> _filter_testdir
> $QEMU_IMG convert -O $IMGFMT -c -B "$TEST_IMG".base -o backing_fmt=$IMGFMT \
I have seen that warning few times already in my scripts, so good to have this
option.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
On Mon, 2020-06-08 at 14:14 +0200, Max Reitz wrote:
> On 08.06.20 11:40, Maxim Levitsky wrote:
> > This implements the encryption key management using the generic code in
> > qcrypto layer and exposes it to the user via qemu-img
> >
> > This code adds anoth
27; \
You sometimes use $SED and sometimes sed. Is this intentional?
> +| grep -ae
> "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
> +| $SED "${filename_filters[@]}" \
> +-e 's/^\(fmt\)/0-\1/' \
> +-e 's/^\(size\)/1-\1/' \
> +-e 's/^\(backing\)/2-\1/' \
> +-e 's/^\(data_file\)/3-\1/' \
> +-e 's/^\(encryption\)/4-\1/' \
> +-e 's/^\(encrypt\.format\)/5-\1/' \
> +-e 's/^\(encrypt\.key-secret\)/6-\1/' \
> +-e 's/^\(encrypt\.iter-time\)/7-\1/' \
> +-e 's/^\(preallocation\)/8-\1/' \
All right, I understand this now, but do we have to do this?
Maybe it is better to just update the outputs once to avoid keeping
the custom sort order?
> +| sort \
> +| $SED -e 's/^[0-9]-//' \
> +| tr '\n\0' ' \n' \
> +| $SED -e 's/^ *$//' -e 's/ *$//'
> +)
For the above bash pipeline overall: It was hard to decipher :-), but I must
admit
I learned something from it.
> +
> +echo -n "$qmp_pre"
> +if [ -n "$options" ]; then
> +echo "$filename_part, $options"
> +elif [ -n "$filename_part" ]; then
> +echo "$filename_part"
> +fi
> +echo -n "$qmp_post"
> }
>
> _filter_img_create_size()
Overall I like the idea of this patch.
Best regards,
Maxim Levitsky
On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
> From: Maxim Levitsky
>
> This allows more tests to be able to have same output on both qcow2 luks
> encrypted images
> and raw luks images
>
> Signed-off-by: Maxim Levitsky
> Signed-off-by: Max Reitz
> ---
&g
On Wed, 2020-06-17 at 15:50 +0200, Max Reitz wrote:
> On 17.06.20 13:46, Maxim Levitsky wrote:
> > On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
> > > Right now, _filter_img_create just filters out everything that looks
> > > format-dependent, and applies some fi
rep -ae
> "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
> +| $SED "${filename_filters[@]}" \
> +-e 's/^\(fmt\)/0-\1/' \
> +-e 's/^\(size\)/1-\1/' \
> +-e 's/^\(backing\)/2-\1/' \
> +-e 's/^\(data_file\)/3-\1/' \
> +-e 's/^\(encryption\)/4-\1/' \
> +-e 's/^\(encrypt\.format\)/5-\1/' \
> +-e 's/^\(encrypt\.key-secret\)/6-\1/' \
> +-e 's/^\(encrypt\.iter-time\)/7-\1/' \
> +-e 's/^\(preallocation\)/8-\1/' \
> +| sort \
> +| $SED -e 's/^[0-9]-//' \
> +| tr '\n\0' ' \n' \
> +| $SED -e 's/^ *$//' -e 's/ *$//'
> +)
> +
> +echo "$filename_part, $options"
> +}
> +
> +# Filter the "Formatting..." line in QMP output (leaving the QMP output
> +# untouched)
> +# (In contrast to _filter_img_create(), this function does not support
> +# multi-line Formatting output)
> +_filter_img_create_in_qmp()
> +{
> +while read -r line; do
> +if echo "$line" | grep -q '^Formatting'; then
> +echo "$line" | _filter_img_create
> +else
> +echo "$line"
> +fi
> +done
> }
Good as well.
>
> _filter_img_create_size()
Looks good now,
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
n get away with changing
> as few reference outputs in this patch here as possible.
>
> Signed-off-by: Max Reitz
I went over this patch again, and it looks OK, but
I might have missed something.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
> ---
> tests
all the patches applied,a
and with --disable-nettle --disable-gcrypt
And now all my tests are skipped with this nice message:
"No crypto library supporting PBKDF in this build: Function not implemented"
Thank you very very much for implementing this!
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
universal_newlines=True)
> -exitcode = subp.wait()
> -if exitcode < 0:
> - sys.stderr.write('qemu-img received signal %i: %s\n'
> - % (-exitcode, ' '.join(qemu_img_args + list(args
> -return subp.communicate()[0]
> +return qemu_img_pipe_and_status(*args)[0]
>
> def qemu_img_log(*args):
> result = qemu_img_pipe(*args)
You made me learn a bit about python type hints, and I don't regret it :-)
Looks OK.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
o me that we can have a situation when luks driver is
blacklisted
(via block driver blacklist "--block-drv-whitelist=") then this test.
THe whilelist only affects the qmp it seems so this check doesn't catch it,
plus you could have case when luks driver is blacklisted but qcow2 isn't
When I build qemu with
'--block-drv-whitelist='raw,qcow2' I was able to break iotests 295 296
But this is such a non issue, that I am just noting for reference.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
ts
> +if imgfmt == 'luks':
> +verify_working_luks()
> return
>
> not_sup = supported_fmts and (imgfmt not in supported_fmts)
> if not_sup or (imgfmt in unsupported_fmts):
> notrun('not suitable for this image forma
On Thu, 2020-06-25 at 14:55 +0200, Max Reitz wrote:
> From: Maxim Levitsky
>
> This commit adds two tests, which test the new amend interface
> of both luks raw images and qcow2 luks encrypted images.
>
> Signed-off-by: Maxim Levitsky
> Reviewed-by: Daniel P. Berrang
On Thu, 2020-06-25 at 14:55 +0200, Max Reitz wrote:
> From: Maxim Levitsky
>
> This commit adds two tests that cover the
> new blockdev-amend functionality of luks and qcow2 driver
>
> Signed-off-by: Maxim Levitsky
> Reviewed-by: Daniel P. Berrangé
> [mreitz: Let 29
On Tue, 2020-06-30 at 10:56 +0200, Max Reitz wrote:
> On 29.06.20 14:05, Maxim Levitsky wrote:
> > On Thu, 2020-06-25 at 14:55 +0200, Max Reitz wrote:
> > > From: Maxim Levitsky
> > >
> > > This commit adds two tests, which test the new amend interface
>
ither if this only serves to complicate things these days. If sending
> > separate pull requests directly to Peter would make things easier, I
> > certainly wouldn't object.
> >
>
> I don't think there is any reason to by-pass your tree. I think the
> volume would need to increase even further for that to make sense.
>
It my fault as well - I need to get back to reviewing these.
(I'll review few of them today I hope)
Best regards,
Maxim Levitsky
ches and ask him to respin.
> > While this is a valid outcome, if we can avoid it it will save all of us
> > review time.
>
> If Klaus wants to do that, fine with me. I'm just trying to find the
> easiest solution for all of us.
>
> > > It would be good to have at least one review, though.
> >
> > Maxim catched this issue, I'd feel safer if he acks your pre-merge queue.
>
> Ok. Maxim, can you please review this series then?
>
> Kevin
I am slowly getting through the heap of the patches trying to understand the
current state of things.
I will start reviewing all these patches today.
Best regards,
Maxim Levitsky
+#define NVME_CAP_SET_PMRS(cap, val) (cap |= (uint64_t)(val & CAP_PMR_MASK)
> \
> << CAP_PMR_SHIFT)
> +#define NVME_CAP_SET_CMBS(cap, val) (cap |= (uint64_t)(val & CAP_CMB_MASK)
> \
> +
On Wed, 2020-07-08 at 18:11 +0300, Maxim Levitsky wrote:
> On Tue, 2020-07-07 at 21:40 +0100, Peter Maydell wrote:
> > On Mon, 6 Jul 2020 at 11:04, Max Reitz wrote:
> > > The following changes since commit
> > > eb6490f544388dd24c0d054a96dd304bc7284450:
> >
On Wed, 2020-07-08 at 14:33 +0200, Gerd Hoffmann wrote:
> On Mon, Jun 08, 2020 at 12:40:27PM +0300, Maxim Levitsky wrote:
> > blockdev-amend will be used similiar to blockdev-create
> > to allow on the fly changes of the structure of the format based block
> > devices.
>
d from BlockDriverInfo
> > - Fix qcow2 preallocation when the image size is not a multiple of the
> > cluster size
> > - Fix in block-copy code
> >
>
>
> Applied, thanks.
>
> Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
> fo
On Wed, 2020-07-08 at 16:23 +0200, Gerd Hoffmann wrote:
> On Wed, Jul 08, 2020 at 04:06:45PM +0300, Maxim Levitsky wrote:
> > On Wed, 2020-07-08 at 14:33 +0200, Gerd Hoffmann wrote:
> > > On Mon, Jun 08, 2020 at 12:40:27PM +0300, Maxim Levitsky wrote:
> > > > blockd
#x27; not found or not supported",
> fmt);
Yep, this looks like a real bug, sorry about that.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
This is basically the same thing as commit
'crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails'
does but for qcow2 files to ensure that we don't leave qcow2 files
when creation fails.
Signed-off-by: Maxim Levitsky
---
block/qcow2.c | 11 +++
1 f
Test that we can't write-share raw luks images by default,
but we still can with share-rw=on
Signed-off-by: Maxim Levitsky
---
tests/qemu-iotests/296 | 44 +-
tests/qemu-iotests/296.out | 12 +--
2 files changed, 53 insertions(+), 3 dele
A rebase gone wrong, and I ended up allowing a luks image
to be opened at the same time by two VMs without any warnings/overrides.
Fix that and also add an iotest to prevent this from happening.
Best regards,
Maxim Levisky
Maxim Levitsky (2):
block/crypto: disallow write sharing by
ot;)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1857490
Signed-off-by: Maxim Levitsky
---
block/crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/crypto.c b/block/crypto.c
index 8725c1bc02..0807557763 100644
--- a/block/crypto.c
+++ b/block/crypt
en the write-zero command can have special 'deallocate' flag that hints the
controller
to discard the sectors.
So woudn't discarding the clusters have theoretical risk of introducing garbage
there?
Best regards,
Maxim Levitsky
On Wed, 2020-07-22 at 19:14 +0200, Kevin Wolf wrote:
> Am 22.07.2020 um 19:01 hat Maxim Levitsky geschrieben:
> > On Mon, 2020-07-20 at 15:18 +0200, Kevin Wolf wrote:
> > > qcow2 version 2 images don't support the zero flag for clusters, so for
> > > write_zeroes
On Thu, 2020-07-16 at 14:33 +0300, Maxim Levitsky wrote:
> This is basically the same thing as commit
> 'crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails'
> does but for qcow2 files to ensure that we don't leave qcow2 files
> when creation fails
static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> return NVME_INVALID_FIELD | NVME_DNR;
> }
>
> -req->cqe.result = result;
> +req->cqe.result = cpu_to_le32(result);
> return NVME_SUCCESS;
> }
>
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
uot;nsid %"PRIu32""
> pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write
> cache, result=%s"
> pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
> pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested
> cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
> pci_nvme_setfeat_timestamp(uint64_t ts) "set feature timestamp = 0x%"PRIx64""
> pci_nvme_getfeat_timestamp(uint64_t ts) "get feature timestamp = 0x%"PRIx64""
> +pci_nvme_enqueue_req_completion(uint16_t cid, uint16_t cqid, uint16_t
> status) "cid %"PRIu16" cqid %"PRIu16" status 0x%"PRIx16""
> +pci_nvme_mmio_read(uint64_t addr) "addr 0x%"PRIx64""
> +pci_nvme_mmio_write(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data
> 0x%"PRIx64""
> +pci_nvme_mmio_doorbell_cq(uint16_t cqid, uint16_t new_head) "cqid %"PRIu16"
> new_head %"PRIu16""
> +pci_nvme_mmio_doorbell_sq(uint16_t sqid, uint16_t new_tail) "cqid %"PRIu16"
> new_tail %"PRIu16""
> pci_nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO,
> interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> pci_nvme_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO,
> interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> pci_nvme_mmio_cfg(uint64_t data) "wrote MMIO, config controller
> config=0x%"PRIx64""
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
NvmeIdCtrlOncs {
> NVME_ONCS_TIMESTAMP = 1 << 6,
> };
>
> +enum NvmeIdCtrlFrmw {
> +NVME_FRMW_SLOT1_RO = 1 << 0,
> +};
> +
> #define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
> #define NVME_CTRL_SQES_MAX(sqes) (((sqes) >> 4) & 0xf)
> #define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
Looks good,
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
quot;"
> pci_nvme_err_invalid_setfeat(uint32_t dw10) "invalid set features,
> dw10=0x%"PRIx32""
> +pci_nvme_err_invalid_log_page(uint16_t cid, uint16_t lid) "cid %"PRIu16" lid
> 0x%"PRIx16""
> pci_nvme_err_startfail_cq(void) "nvme_start
; Signed-off-by: Klaus Jensen
> Acked-by: Keith Busch
> Reviewed-by: Maxim Levitsky
> Reviewed-by: Dmitry Fomichev
> ---
> hw/block/nvme.c | 180 --
> hw/block/nvme.h | 10 ++-
> hw/block/trace-events | 9 +++
> inc
into the nvme device specific header file as it is the only
> user of the structure. Also, remove the unused members.
Agree.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
>
> Signed-off-by: Klaus Jensen
> Reviewed-by: Dmitry Fomichev
> ---
> hw/block/nvm
NvmeRequest *req)
>
> break;
> case NVME_VOLATILE_WRITE_CACHE:
> +if (!(dw11 & 0x1) && blk_enable_write_cache(n->conf.blk)) {
> +blk_flush(n->conf.blk);
> +}
> +
> blk_set_enable_write_cache(n->conf.blk, dw
CtrlLpa {
> #define NVME_INTC_THR(intc) (intc & 0xff)
> #define NVME_INTC_TIME(intc)((intc >> 8) & 0xff)
>
> +#define NVME_INTVC_NOCOALESCING (0x1 << 16)
> +
> #define NVME_TEMP_THSEL(temp) ((temp >> 20) & 0x3)
> #define NVME_TEMP_THSEL_OVER 0x0
> #define NVME_TEMP_THSEL_UNDER 0x1
> @@ -899,9 +903,13 @@ enum NvmeFeatureIds {
> NVME_WRITE_ATOMICITY= 0xa,
> NVME_ASYNCHRONOUS_EVENT_CONF= 0xb,
> NVME_TIMESTAMP = 0xe,
> -NVME_SOFTWARE_PROGRESS_MARKER = 0x80
> +NVME_SOFTWARE_PROGRESS_MARKER = 0x80,
> +NVME_FID_MAX= 0x100,
> };
>
> +#define NVME_GETSETFEAT_FID_MASK 0xff
> +#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
> +
> typedef struct NvmeRangeType {
> uint8_t type;
> uint8_t attributes;
Looks good,
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
E= 1 << 2,
> +} NvmeFeatureCap;
> +
> +typedef enum NvmeGetFeatureSelect {
> +NVME_GETFEAT_SELECT_CURRENT = 0x0,
> +NVME_GETFEAT_SELECT_DEFAULT = 0x1,
> +NVME_GETFEAT_SELECT_SAVED = 0x2,
> +NVME_GETFEAT_SELECT_CAP = 0x3,
> +} NvmeGetFeatureSelect;
> +
> #define NVME_GETSETFEAT_FID_MASK 0xff
> #define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
>
> +#define NVME_GETFEAT_SELECT_SHIFT 8
> +#define NVME_GETFEAT_SELECT_MASK 0x7
> +#define NVME_GETFEAT_SELECT(dw10) \
> +((dw10 >> NVME_GETFEAT_SELECT_SHIFT) & NVME_GETFEAT_SELECT_MASK)
> +
> +#define NVME_SETFEAT_SAVE_SHIFT 31
> +#define NVME_SETFEAT_SAVE_MASK 0x1
> +#define NVME_SETFEAT_SAVE(dw10) \
> +((dw10 >> NVME_SETFEAT_SAVE_SHIFT) & NVME_SETFEAT_SAVE_MASK)
OK.
> +
> typedef struct NvmeRangeType {
> uint8_t type;
> uint8_t attributes;
> @@ -926,6 +949,8 @@ typedef struct NvmeLBAF {
> uint8_t rp;
> } NvmeLBAF;
>
> +#define NVME_NSID_BROADCAST 0x
Cool, you probably want eventually to go over code and
change all places that use the number to the define.
(No need to do this now)
> +
> typedef struct NvmeIdNs {
> uint64_tnsze;
> uint64_tncap;
Overall looks OK, other that nitpick about that goto so
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
2""
> pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> +pci_nvme_identify_ns_descr_list(uint32_t ns) "nsid %"PRIu32""
> pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae,
> uint32_t len, uint64_t off) "cid %"PRIu16&qu
so
> specified
> + * in the spec (NVM Express v1.3d, Section 5.15.4).
> + */
> +if (min_nsid >= NVME_NSID_BROADCAST - 1) {
> +return NVME_INVALID_NSID | NVME_DNR;
> +}
> +
> list = g_malloc0(data_len);
> for (i = 0; i < n->num
eof(id->subnqn), subnqn, '\0');
> +g_free(subnqn);
> +
> id->psd[0].mp = cpu_to_le16(0x9c4);
> id->psd[0].enlat = cpu_to_le32(0x10);
> id->psd[0].exlat = cpu_to_le32(0x4);
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
index 7b7303cab1dd..f3b2d004e078 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -33,6 +33,8 @@ pci_nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ
> vector %u"
> pci_nvme_irq_pin(void) "pulsing IRQ pin"
> pci_nvme_irq_masked(void) "IRQ is masked"
> pci_nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64"
> prp2=0x%"PRIx64""
> +pci_nvme_map_addr(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len
> %"PRIu64""
> +pci_nvme_map_addr_cmb(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len
> %"PRIu64""
> pci_nvme_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode)
> "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
> pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode) "cid
> %"PRIu16" sqid %"PRIu16" opc 0x%"PRIx8""
> pci_nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count,
> uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
Looks good. I could have missed something, but compared to older version of
similiar code I reviewed,
this looks much better and easy to t understand.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
On Mon, 2020-07-20 at 13:37 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Refactor the nvme_dma_{read,write}_prp functions into a common function
> taking a DMADirection parameter.
>
> Signed-off-by: Klaus Jensen
> Reviewed-by: Maxim Levitsky
> ---
501 - 600 of 959 matches
Mail list logo
