Re: [PATCH v4 9/9] iotests: rename and move 169 and 199 tests
On 5/27/20 2:50 PM, Vladimir Sementsov-Ogievskiy wrote: > > Hmm. Actually, I think, it's not a problem to continue support ranges of > tests for number test names, if you need it. > > Note a new paramter --start-from, which is here to re-run failed > ./check run from the middle of the process, is it your use-case or not? Yeah, that'll solve that for me! I pretty much care only about excluding tests sometimes, and running ranges was just a way to do that.
Re: [PATCH v7 28/32] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()
On Thu 28 May 2020 09:11:07 PM CEST, Eric Blake wrote: >> I think the problem also exists in the current code (without my >> patches). If you zeroize 10 clusters and the last one is compressed >> you have to repeat the request after having zeroized 9 clusters. > > Hmm. In the pre-patch code, qcow2_co_pwrite_zeroes() calls > qcow2_cluster_zeroize() which can fail with -ENOTSUP up front, but not > after the fact. Once it starts the while loop over clusters, its use > of zero_in_l2_slice() handles compressed clusters just fine; You're right, complete compressed clusters can always be handled, the problem is just when there's subclusters. > But isn't this something we could solve recursively? Instead of > returning -ENOTSUP, we could have zero_in_l2_slice() call > bdrv_pwrite_zeroes() on the (sub-)clusters associated with a > compressed cluster. I suppose we could, as long as BDRV_REQ_NO_FALLBACK is not used. Berto
Re: [PATCH 5/5] virtio: enable VIRTIO_F_RING_PACKED for all devices
On Fri, May 29, 2020 at 03:15:59PM +0800, Jason Wang wrote:
>
> On 2020/5/23 上午1:17, Stefan Hajnoczi wrote:
> > The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
> > ring instead of a split avail/used ring design. There are CPU cache
> > advantages to this layout and it is also suited better to hardware
> > implementation.
> >
> > The vhost-net backend has already supported packed virtqueues for some
> > time. Performance benchmarks show that virtio-blk performance on NVMe
> > drives is also improved.
> >
> > Go ahead and enable this feature for all VIRTIO devices. Keep it
> > disabled for QEMU 5.0 and earlier machine types.
> >
> > Signed-off-by: Stefan Hajnoczi
> > ---
> > include/hw/virtio/virtio.h | 2 +-
> > hw/core/machine.c | 18 +-
> > 2 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index b69d517496..fd5b4a2044 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -292,7 +292,7 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> > DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
> > VIRTIO_F_IOMMU_PLATFORM, false), \
> > DEFINE_PROP_BIT64("packed", _state, _field, \
> > - VIRTIO_F_RING_PACKED, false)
> > + VIRTIO_F_RING_PACKED, true)
> > hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> > bool virtio_queue_enabled(VirtIODevice *vdev, int n);
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index bb3a7b18b1..3598c3c825 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -28,7 +28,23 @@
> > #include "hw/mem/nvdimm.h"
> > #include "migration/vmstate.h"
> > -GlobalProperty hw_compat_5_0[] = {};
> > +GlobalProperty hw_compat_5_0[] = {
> > +{ "vhost-user-blk", "packed", "off" },
> > +{ "vhost-user-fs-device", "packed", "off" },
> > +{ "vhost-vsock-device", "packed", "off" },
> > +{ "virtio-9p-device", "packed", "off" },
> > +{ "virtio-balloon-device", "packed", "off" },
> > +{ "virtio-blk-device", "packed", "off" },
> > +{ "virtio-crypto-device", "packed", "off" },
> > +{ "virtio-gpu-device", "packed", "off" },
> > +{ "virtio-input-device", "packed", "off" },
> > +{ "virtio-iommu-device", "packed", "off" },
> > +{ "virtio-net-device", "packed", "off" },
> > +{ "virtio-pmem", "packed", "off" },
> > +{ "virtio-rng-device", "packed", "off" },
> > +{ "virtio-scsi-common", "packed", "off" },
> > +{ "virtio-serial-device", "packed", "off" },
>
>
> Missing "vhost-user-gpu" here?
Thanks, you're right.
I'll see if virtio-gpu-base works. If not it will be necessary to add
all the derived classes. The same is true for virtio-scsi-common, I'd
better check it works correctly!
Stefan
signature.asc
Description: PGP signature
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
On Wed, May 27, 2020 at 04:28:41PM +0200, Marc-André Lureau wrote: > Hi Stefan > > On Fri, May 22, 2020 at 7:18 PM Stefan Hajnoczi wrote: > > > > Many vhost devices in QEMU currently do not involve the device backend > > in feature negotiation. This seems fine at first glance for device types > > without their own feature bits (virtio-net has many but other device > > types have none). > > > > This overlooks the fact that QEMU's virtqueue implementation and the > > device backend's implementation may support different features. QEMU > > must not report features to the guest that the the device backend > > doesn't support. > > > > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many > > existing vhost device backends do not. When the user sets packed=on the > > device backend breaks. This should have been handled gracefully by > > feature negotiation instead. > > > > Introduce vhost_get_default_features() and update all vhost devices in > > QEMU to involve the device backend in feature negotiation. > > > > This patch fixes the following error: > > > > $ x86_64-softmmu/qemu-system-x86_64 \ > > -drive if=virtio,file=test.img,format=raw \ > > -chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \ > > -device vhost-user-blk-pci,chardev=char0,packed=on \ > > -object memory-backend-memfd,size=1G,share=on,id=ram0 \ > > -M accel=kvm,memory-backend=ram0 > > qemu-system-x86_64: Failed to set msg fds. > > qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0) > > > > The vhost-user-blk backend failed as follows: > > > > $ ./vhost-user-blk --socket-path=/tmp/vhost-user-blk.sock -b test2.img > > vu_panic: virtio: zero sized buffers are not allowed > > virtio-blk request missing headers > > > > Signed-off-by: Stefan Hajnoczi > > --- > > include/hw/virtio/vhost.h| 1 + > > include/hw/virtio/virtio-gpu.h | 2 ++ > > include/sysemu/cryptodev-vhost.h | 11 +++ > > backends/cryptodev-vhost.c | 19 +++ > > hw/display/vhost-user-gpu.c | 17 + > > hw/display/virtio-gpu-base.c | 2 +- > > hw/input/vhost-user-input.c | 9 + > > hw/virtio/vhost-user-fs.c| 5 +++-- > > hw/virtio/vhost-vsock.c | 5 +++-- > > hw/virtio/vhost.c| 22 ++ > > hw/virtio/virtio-crypto.c| 3 ++- > > 11 files changed, 90 insertions(+), 6 deletions(-) > > > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > > index 085450c6f8..d2e54dd4a8 100644 > > --- a/include/hw/virtio/vhost.h > > +++ b/include/hw/virtio/vhost.h > > @@ -112,6 +112,7 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, > > VirtIODevice *vdev, int n, > >bool mask); > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int > > *feature_bits, > > uint64_t features); > > +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t > > features); > > void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits, > > uint64_t features); > > bool vhost_has_free_slot(void); > > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h > > index 6dd57f2025..41d270d80e 100644 > > --- a/include/hw/virtio/virtio-gpu.h > > +++ b/include/hw/virtio/virtio-gpu.h > > @@ -192,6 +192,8 @@ bool virtio_gpu_base_device_realize(DeviceState *qdev, > > void virtio_gpu_base_reset(VirtIOGPUBase *g); > > void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g, > > struct virtio_gpu_resp_display_info *dpy_info); > > +uint64_t virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t > > features, > > + Error **errp); > > > > /* virtio-gpu.c */ > > void virtio_gpu_ctrl_response(VirtIOGPU *g, > > diff --git a/include/sysemu/cryptodev-vhost.h > > b/include/sysemu/cryptodev-vhost.h > > index f42824fbde..e629446bfb 100644 > > --- a/include/sysemu/cryptodev-vhost.h > > +++ b/include/sysemu/cryptodev-vhost.h > > @@ -122,6 +122,17 @@ int cryptodev_vhost_start(VirtIODevice *dev, int > > total_queues); > > */ > > void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues); > > > > +/** > > + * cryptodev_vhost_get_features: > > + * @dev: the virtio crypto object > > + * @requested_features: the features being offered > > + * > > + * Returns: the requested features bits that are supported by the vhost > > device, > > + * or the original request feature bits if vhost is disabled > > + * > > + */ > > +uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t > > features); > > + > > /** > > * cryptodev_vhost_virtqueue_mask: > > * @dev: the virtio crypto object > > diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c > > index 8337c9a495..5f5a4fda7b 100644 > > --- a/backends/cryptodev-vhost.c > > +++ b/backends/cryptodev-vhost.c > > @@ -266,6 +266,20 @@ void cry
Re: [PATCH v7 32/32] iotests: Add tests for qcow2 images with extended L2 entries
On 5/29/20 10:07 AM, Alberto Garcia wrote: On Wed 27 May 2020 08:30:06 PM CEST, Eric Blake wrote: +offset=$(($offset + 8)) +bitmap=`peek_file_be "$TEST_IMG" $offset 8` + +expected_bitmap=0 +for bit in $expected_alloc; do +expected_bitmap=$(($expected_bitmap | (1 << $bit))) +done +for bit in $expected_zero; do +expected_bitmap=$(($expected_bitmap | (1 << (32 + $bit +done +expected_bitmap=`printf "%llu" $expected_bitmap` Dead statement - expected_bitmap is already a 64-bit decimal number without reprinting it to itself. Not quite... it seems that simply expanding the variable treats the value as signed so echo $((1 << 63)) returns INT64_MIN. The printf call makes it unsigned, Ah, yes, then that makes sense. Still, you could shave a fork or comment the action by doing: printf -v expected_bitmap %llu $expected_bitmap # convert to unsigned but even though I tried that in a 32-bit system and it works now I'm actually wondering about the portability of the whole thing. Bash supports 64-bit numbers even on 32-bit platforms, and has done for years. Since we are running the test only under bash, that's all the more we have to worry about. Looking at the source it seems that bash uses intmax_t: https://git.savannah.gnu.org/cgit/bash.git/tree/variables.h?h=bash-5.0#n68 But if this is a problem then peek_file_* would also be affected, it also uses printf %llu and a few iotests are already reading 64bit values (grep 'peek_file_.* 8'). In cases where a negative number is read but the 64-bit pattern is the same, it doesn't matter; but here, you are using it for output, and so you do want the unsigned representation instead of bash's default signed representation. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v7 32/32] iotests: Add tests for qcow2 images with extended L2 entries
On Wed 27 May 2020 08:30:06 PM CEST, Eric Blake wrote: >> +offset=$(($offset + 8)) >> +bitmap=`peek_file_be "$TEST_IMG" $offset 8` >> + >> +expected_bitmap=0 >> +for bit in $expected_alloc; do >> +expected_bitmap=$(($expected_bitmap | (1 << $bit))) >> +done >> +for bit in $expected_zero; do >> +expected_bitmap=$(($expected_bitmap | (1 << (32 + $bit >> +done >> +expected_bitmap=`printf "%llu" $expected_bitmap` > > Dead statement - expected_bitmap is already a 64-bit decimal number > without reprinting it to itself. Not quite... it seems that simply expanding the variable treats the value as signed so echo $((1 << 63)) returns INT64_MIN. The printf call makes it unsigned, but even though I tried that in a 32-bit system and it works now I'm actually wondering about the portability of the whole thing. Looking at the source it seems that bash uses intmax_t: https://git.savannah.gnu.org/cgit/bash.git/tree/variables.h?h=bash-5.0#n68 But if this is a problem then peek_file_* would also be affected, it also uses printf %llu and a few iotests are already reading 64bit values (grep 'peek_file_.* 8'). Berto
[PATCH] qemu-img: Fix doc typo for 'bitmap' subcommand
Prefer a consistent naming for the --merge argument. Fixes: 3b51ab4bf Signed-off-by: Eric Blake --- I'm happy for this to go in through my bitmaps queue or through the trivial tree, whichever picks it up first. docs/tools/qemu-img.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index 69cd9a30373a..7f0737488ade 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -300,7 +300,7 @@ Command description: ``--disable`` to change *BITMAP* to stop recording future edits. - ``--merge`` to merge the contents of *SOURCE_BITMAP* into *BITMAP*. + ``--merge`` to merge the contents of the *SOURCE* bitmap into *BITMAP*. Additional options include ``-g`` which sets a non-default *GRANULARITY* for ``--add``, and ``-b`` and ``-F`` which select an -- 2.26.2
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
* Stefan Hajnoczi (stefa...@redhat.com) wrote: > On Fri, May 29, 2020 at 03:04:54PM +0800, Jason Wang wrote: > > > > On 2020/5/23 上午1:17, Stefan Hajnoczi wrote: > > > Many vhost devices in QEMU currently do not involve the device backend > > > in feature negotiation. This seems fine at first glance for device types > > > without their own feature bits (virtio-net has many but other device > > > types have none). > > > > > > This overlooks the fact that QEMU's virtqueue implementation and the > > > device backend's implementation may support different features. QEMU > > > must not report features to the guest that the the device backend > > > doesn't support. > > > > > > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many > > > existing vhost device backends do not. When the user sets packed=on the > > > device backend breaks. This should have been handled gracefully by > > > feature negotiation instead. > > > > > > Introduce vhost_get_default_features() and update all vhost devices in > > > QEMU to involve the device backend in feature negotiation. > > > > > > This patch fixes the following error: > > > > > >$ x86_64-softmmu/qemu-system-x86_64 \ > > >-drive if=virtio,file=test.img,format=raw \ > > >-chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \ > > >-device vhost-user-blk-pci,chardev=char0,packed=on \ > > >-object memory-backend-memfd,size=1G,share=on,id=ram0 \ > > >-M accel=kvm,memory-backend=ram0 > > >qemu-system-x86_64: Failed to set msg fds. > > >qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0) > > > > > > It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED > > into user_feature_bits in vhost-user-blk.c. > > > > And for the rest, we need require them to call vhost_get_features() with the > > proper feature bits that needs to be acked by the backend. > > There is a backwards-compatibility problem: we cannot call > vhost_get_features() with the full set of feature bits that each device > type supports because existing vhost-user backends don't advertise > features properly. QEMU disables features not advertised by the > vhost-user backend. > > For example, if we add VIRTIO_RING_F_EVENT_IDX then it will always be > disabled for existing libvhost-user backends because they don't > advertise this bit :(. > > The reason I introduced vhost_get_default_features() is to at least > salvage VIRTIO_F_IOMMU_PLATFORM and VIRTIO_F_RING_PACKED. These bits can > be safely negotiated for all devices. > > Any new transport/vring VIRTIO feature bits can also be added to > vhost_get_default_features() safely. > > If a vhost-user device wants to use device type-specific feature bits > then it really needs to call vhost_get_features() directly as you > suggest. But it's important to check whether existing vhost-user > backends actually advertise them - because if they don't advertise them > but rely on them then we'll break existing backends. What about adding a VHOST_USER_PROTOCOL_F_BACKEND_F to indicate the backend knows how to advertise the backend features. (Although my understanding of virtio feature flags is thin) Dave > Would you prefer a different approach? > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index aff98a0ede..f8a144dcd0 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -48,6 +48,23 @@ static unsigned int used_memslots; > > > static QLIST_HEAD(, vhost_dev) vhost_devices = > > > QLIST_HEAD_INITIALIZER(vhost_devices); > > > +/* > > > + * Feature bits that device backends must explicitly report. Feature > > > bits not > > > + * listed here maybe set by QEMU without checking with the device > > > backend. > > > + * Ideally all feature bits would be listed here but existing vhost > > > device > > > + * implementations do not explicitly report bits like > > > VIRTIO_F_VERSION_1, so we > > > + * can only assume they are supported. > > > > > > For most backends, we care about the features for datapath. So this is not > > true at least for networking device, since VIRTIO_F_VERSION_1 have impact on > > the length of vnet header length. > > The net device is in good shape and doesn't use vhost_default_feature_bits[]. > > vhost_default_feature_bits[] is for devices that haven't been > negotiating feature bits properly. The goal is start involving them in > feature negotiation without breaking existing backends. > > Would you like me to rephrase this comment in some way? > > > > + * > > > + * New feature bits added to the VIRTIO spec should usually be included > > > here > > > + * so that existing vhost device backends that do not support them yet > > > continue > > > + * to work. > > > > > > It actually depends on the type of backend. > > > > Kernel vhost-net does not validate GSO features since qemu can talk directly > > to TAP and vhost doesn't report those features. But for vhost-user GSO > > features must be valida
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
On Fri, May 29, 2020 at 03:04:54PM +0800, Jason Wang wrote: > > On 2020/5/23 上午1:17, Stefan Hajnoczi wrote: > > Many vhost devices in QEMU currently do not involve the device backend > > in feature negotiation. This seems fine at first glance for device types > > without their own feature bits (virtio-net has many but other device > > types have none). > > > > This overlooks the fact that QEMU's virtqueue implementation and the > > device backend's implementation may support different features. QEMU > > must not report features to the guest that the the device backend > > doesn't support. > > > > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many > > existing vhost device backends do not. When the user sets packed=on the > > device backend breaks. This should have been handled gracefully by > > feature negotiation instead. > > > > Introduce vhost_get_default_features() and update all vhost devices in > > QEMU to involve the device backend in feature negotiation. > > > > This patch fixes the following error: > > > >$ x86_64-softmmu/qemu-system-x86_64 \ > >-drive if=virtio,file=test.img,format=raw \ > >-chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \ > >-device vhost-user-blk-pci,chardev=char0,packed=on \ > >-object memory-backend-memfd,size=1G,share=on,id=ram0 \ > >-M accel=kvm,memory-backend=ram0 > >qemu-system-x86_64: Failed to set msg fds. > >qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0) > > > It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED > into user_feature_bits in vhost-user-blk.c. > > And for the rest, we need require them to call vhost_get_features() with the > proper feature bits that needs to be acked by the backend. There is a backwards-compatibility problem: we cannot call vhost_get_features() with the full set of feature bits that each device type supports because existing vhost-user backends don't advertise features properly. QEMU disables features not advertised by the vhost-user backend. For example, if we add VIRTIO_RING_F_EVENT_IDX then it will always be disabled for existing libvhost-user backends because they don't advertise this bit :(. The reason I introduced vhost_get_default_features() is to at least salvage VIRTIO_F_IOMMU_PLATFORM and VIRTIO_F_RING_PACKED. These bits can be safely negotiated for all devices. Any new transport/vring VIRTIO feature bits can also be added to vhost_get_default_features() safely. If a vhost-user device wants to use device type-specific feature bits then it really needs to call vhost_get_features() directly as you suggest. But it's important to check whether existing vhost-user backends actually advertise them - because if they don't advertise them but rely on them then we'll break existing backends. Would you prefer a different approach? > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index aff98a0ede..f8a144dcd0 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -48,6 +48,23 @@ static unsigned int used_memslots; > > static QLIST_HEAD(, vhost_dev) vhost_devices = > > QLIST_HEAD_INITIALIZER(vhost_devices); > > +/* > > + * Feature bits that device backends must explicitly report. Feature bits > > not > > + * listed here maybe set by QEMU without checking with the device backend. > > + * Ideally all feature bits would be listed here but existing vhost device > > + * implementations do not explicitly report bits like VIRTIO_F_VERSION_1, > > so we > > + * can only assume they are supported. > > > For most backends, we care about the features for datapath. So this is not > true at least for networking device, since VIRTIO_F_VERSION_1 have impact on > the length of vnet header length. The net device is in good shape and doesn't use vhost_default_feature_bits[]. vhost_default_feature_bits[] is for devices that haven't been negotiating feature bits properly. The goal is start involving them in feature negotiation without breaking existing backends. Would you like me to rephrase this comment in some way? > > + * > > + * New feature bits added to the VIRTIO spec should usually be included > > here > > + * so that existing vhost device backends that do not support them yet > > continue > > + * to work. > > > It actually depends on the type of backend. > > Kernel vhost-net does not validate GSO features since qemu can talk directly > to TAP and vhost doesn't report those features. But for vhost-user GSO > features must be validated by qemu since qemu can't see what is behind > vhost-user. Maybe the comment should say "New transport/vring feature bits". I'm thinking about things like VIRTIO_F_RING_PACKED that are not device-specific but require backend support. The GSO features you mentioned are device-specific. Devices that want to let the backend advertise device-specific features cannot use vhost_default_feature_bits[]. > > + */ > > +static const int vhost_default_feature_bi
Re: [PULL v3 7/7] qemu-img: Add bitmap sub-command
On 5/19/20 12:57 PM, Eric Blake wrote: Include actions for --add, --remove, --clear, --enable, --disable, and --merge (note that --clear is a bit of fluff, because the same can be accomplished by removing a bitmap and then adding a new one in its place, but it matches what QMP commands exist). Listing is omitted, because it does not require a bitmap name and because it was already possible with 'qemu-img info'. A single command line can play one or more bitmap commands in sequence on the same bitmap name (although all added bitmaps share the same granularity, and and all merged bitmaps come from the same source file). Merge defaults to other bitmaps in the primary image, but can also be told to merge bitmaps from a distinct image. While backporting this, I noticed: +++ b/docs/tools/qemu-img.rst @@ -281,6 +281,30 @@ Command description: For write tests, by default a buffer filled with zeros is written. This can be overridden with a pattern byte specified by *PATTERN*. +.. option:: bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b SOURCE_FILE [-F SOURCE_FMT]] [-g GRANULARITY] [--object OBJECTDEF] [--image-opts | -f FMT] FILENAME BITMAP Here, it is spelled --merge SOURCE, + ``--merge`` to merge the contents of *SOURCE_BITMAP* into *BITMAP*. but here, it is spelled SOURCE_BITMAP, + + Additional options include ``-g`` which sets a non-default + *GRANULARITY* for ``--add``, and ``-b`` and ``-F`` which select an + alternative source file for all *SOURCE* bitmaps used by + ``--merge``. and once again SOURCE. I'll submit a followup patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 00/22] Fix error handling during bitmap postcopy
29.05.2020 14:58, Eric Blake wrote: On 4/2/20 2:42 AM, Vladimir Sementsov-Ogievskiy wrote: Ping! It's a fix, but not a degradation and I'm afraid too big for 5.0. Still, I think I should ping it anyway. John, I'm afraid, that this all is for your branch :) Just noticing this thread, now that we've shuffled bitmaps maintainers. Is there anything here that we still need to include in 5.1? Yes, we need the whole series. 17.02.2020 18:02, Vladimir Sementsov-Ogievskiy wrote: Original idea of bitmaps postcopy migration is that bitmaps are non critical data, and their loss is not serious problem. So, using postcopy method on any failure we should just drop unfinished bitmaps and continue guest execution. However, it doesn't work so. It crashes, fails, it goes to postcopy-recovery feature. It does anything except for behavior we want. These series fixes at least some problems with error handling during bitmaps migration postcopy. v1 was "[PATCH 0/7] Fix crashes on early shutdown during bitmaps postcopy" v2: Most of patches are new or changed a lot. Only patches 06,07 mostly unchanged, just rebased on refactorings. Vladimir Sementsov-Ogievskiy (22): migration/block-dirty-bitmap: fix dirty_bitmap_mig_before_vm_start migration/block-dirty-bitmap: rename state structure types migration/block-dirty-bitmap: rename dirty_bitmap_mig_cleanup migration/block-dirty-bitmap: move mutex init to dirty_bitmap_mig_init migration/block-dirty-bitmap: refactor state global variables migration/block-dirty-bitmap: rename finish_lock to just lock migration/block-dirty-bitmap: simplify dirty_bitmap_load_complete migration/block-dirty-bitmap: keep bitmap state for all bitmaps migration/block-dirty-bitmap: relax error handling in incoming part migration/block-dirty-bitmap: cancel migration on shutdown migration/savevm: don't worry if bitmap migration postcopy failed qemu-iotests/199: fix style qemu-iotests/199: drop extra constraints qemu-iotests/199: better catch postcopy time qemu-iotests/199: improve performance: set bitmap by discard qemu-iotests/199: change discard patterns qemu-iotests/199: increase postcopy period python/qemu/machine: add kill() method qemu-iotests/199: prepare for new test-cases addition qemu-iotests/199: check persistent bitmaps qemu-iotests/199: add early shutdown case to bitmaps postcopy qemu-iotests/199: add source-killed case to bitmaps postcopy Cc: John Snow Cc: Vladimir Sementsov-Ogievskiy Cc: Stefan Hajnoczi Cc: Fam Zheng Cc: Juan Quintela Cc: "Dr. David Alan Gilbert" Cc: Eduardo Habkost Cc: Cleber Rosa Cc: Kevin Wolf Cc: Max Reitz Cc: qemu-block@nongnu.org Cc: qemu-de...@nongnu.org Cc: qemu-sta...@nongnu.org # for patch 01 migration/migration.h | 3 +- migration/block-dirty-bitmap.c | 444 + migration/migration.c | 15 +- migration/savevm.c | 37 ++- python/qemu/machine.py | 12 +- tests/qemu-iotests/199 | 244 ++ tests/qemu-iotests/199.out | 4 +- 7 files changed, 529 insertions(+), 230 deletions(-) -- Best regards, Vladimir
Re: [PATCH v2 00/22] Fix error handling during bitmap postcopy
On 4/2/20 2:42 AM, Vladimir Sementsov-Ogievskiy wrote: Ping! It's a fix, but not a degradation and I'm afraid too big for 5.0. Still, I think I should ping it anyway. John, I'm afraid, that this all is for your branch :) Just noticing this thread, now that we've shuffled bitmaps maintainers. Is there anything here that we still need to include in 5.1? 17.02.2020 18:02, Vladimir Sementsov-Ogievskiy wrote: Original idea of bitmaps postcopy migration is that bitmaps are non critical data, and their loss is not serious problem. So, using postcopy method on any failure we should just drop unfinished bitmaps and continue guest execution. However, it doesn't work so. It crashes, fails, it goes to postcopy-recovery feature. It does anything except for behavior we want. These series fixes at least some problems with error handling during bitmaps migration postcopy. v1 was "[PATCH 0/7] Fix crashes on early shutdown during bitmaps postcopy" v2: Most of patches are new or changed a lot. Only patches 06,07 mostly unchanged, just rebased on refactorings. Vladimir Sementsov-Ogievskiy (22): migration/block-dirty-bitmap: fix dirty_bitmap_mig_before_vm_start migration/block-dirty-bitmap: rename state structure types migration/block-dirty-bitmap: rename dirty_bitmap_mig_cleanup migration/block-dirty-bitmap: move mutex init to dirty_bitmap_mig_init migration/block-dirty-bitmap: refactor state global variables migration/block-dirty-bitmap: rename finish_lock to just lock migration/block-dirty-bitmap: simplify dirty_bitmap_load_complete migration/block-dirty-bitmap: keep bitmap state for all bitmaps migration/block-dirty-bitmap: relax error handling in incoming part migration/block-dirty-bitmap: cancel migration on shutdown migration/savevm: don't worry if bitmap migration postcopy failed qemu-iotests/199: fix style qemu-iotests/199: drop extra constraints qemu-iotests/199: better catch postcopy time qemu-iotests/199: improve performance: set bitmap by discard qemu-iotests/199: change discard patterns qemu-iotests/199: increase postcopy period python/qemu/machine: add kill() method qemu-iotests/199: prepare for new test-cases addition qemu-iotests/199: check persistent bitmaps qemu-iotests/199: add early shutdown case to bitmaps postcopy qemu-iotests/199: add source-killed case to bitmaps postcopy Cc: John Snow Cc: Vladimir Sementsov-Ogievskiy Cc: Stefan Hajnoczi Cc: Fam Zheng Cc: Juan Quintela Cc: "Dr. David Alan Gilbert" Cc: Eduardo Habkost Cc: Cleber Rosa Cc: Kevin Wolf Cc: Max Reitz Cc: qemu-block@nongnu.org Cc: qemu-de...@nongnu.org Cc: qemu-sta...@nongnu.org # for patch 01 migration/migration.h | 3 +- migration/block-dirty-bitmap.c | 444 + migration/migration.c | 15 +- migration/savevm.c | 37 ++- python/qemu/machine.py | 12 +- tests/qemu-iotests/199 | 244 ++ tests/qemu-iotests/199.out | 4 +- 7 files changed, 529 insertions(+), 230 deletions(-) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v8 2/8] block: consolidate blocksize properties consistency checks
On Fri, May 29, 2020 at 11:53:26AM +0200, Markus Armbruster wrote:
> Roman Kagan writes:
>
> > Several block device properties related to blocksize configuration must
> > be in certain relationship WRT each other: physical block must be no
> > smaller than logical block; min_io_size, opt_io_size, and
> > discard_granularity must be a multiple of a logical block.
> >
> > To ensure these requirements are met, add corresponding consistency
> > checks to blkconf_blocksizes, adjusting its signature to communicate
> > possible error to the caller. Also remove the now redundant consistency
> > checks from the specific devices.
> >
> > Signed-off-by: Roman Kagan
> > Reviewed-by: Eric Blake
> > Reviewed-by: Paul Durrant
> > ---
> > include/hw/block/block.h | 2 +-
> > hw/block/block.c | 30 +-
> > hw/block/fdc.c | 5 -
> > hw/block/nvme.c| 5 -
> > hw/block/swim.c| 5 -
> > hw/block/virtio-blk.c | 7 +--
> > hw/block/xen-block.c | 6 +-
> > hw/ide/qdev.c | 5 -
> > hw/scsi/scsi-disk.c| 12 +---
> > hw/usb/dev-storage.c | 5 -
> > tests/qemu-iotests/172.out | 2 +-
> > 11 files changed, 58 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index d7246f3862..784953a237 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void
> > *buf, hwaddr size,
> > bool blkconf_geometry(BlockConf *conf, int *trans,
> >unsigned cyls_max, unsigned heads_max, unsigned
> > secs_max,
> >Error **errp);
> > -void blkconf_blocksizes(BlockConf *conf);
> > +bool blkconf_blocksizes(BlockConf *conf, Error **errp);
> > bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> > bool resizable, Error **errp);
> >
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index bf56c7612b..b22207c921 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void
> > *buf, hwaddr size,
> > return true;
> > }
> >
> > -void blkconf_blocksizes(BlockConf *conf)
> > +bool blkconf_blocksizes(BlockConf *conf, Error **errp)
> > {
> > BlockBackend *blk = conf->blk;
> > BlockSizes blocksizes;
> > @@ -83,6 +83,34 @@ void blkconf_blocksizes(BlockConf *conf)
> > conf->logical_block_size = BDRV_SECTOR_SIZE;
> > }
> > }
> > +
> > +if (conf->logical_block_size > conf->physical_block_size) {
> > +error_setg(errp,
> > + "logical_block_size > physical_block_size not
> > supported");
> > +return false;
> > +}
>
> Pardon me if this has been answered already for prior revisions: do we
> really support physical block sizes that are not a multiple of the
> logical block size?
Both physical and logical block sizes are required to be powers of two,
so the former is certain to be a multiple of the latter.
> > +
> > +if (!QEMU_IS_ALIGNED(conf->min_io_size, conf->logical_block_size)) {
> > +error_setg(errp,
> > + "min_io_size must be a multiple of logical_block_size");
> > +return false;
> > +}
> > +
> > +if (!QEMU_IS_ALIGNED(conf->opt_io_size, conf->logical_block_size)) {
> > +error_setg(errp,
> > + "opt_io_size must be a multiple of logical_block_size");
> > +return false;
> > +}
> > +
> > +if (conf->discard_granularity != -1 &&
> > +!QEMU_IS_ALIGNED(conf->discard_granularity,
> > + conf->logical_block_size)) {
> > +error_setg(errp, "discard_granularity must be "
> > + "a multiple of logical_block_size");
> > +return false;
> > +}
> > +
> > +return true;
> > }
> >
> > bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index c5fb9d6ece..8eda572ef4 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -554,7 +554,10 @@ static void floppy_drive_realize(DeviceState *qdev,
> > Error **errp)
> > read_only = !blk_bs(dev->conf.blk) ||
> > blk_is_read_only(dev->conf.blk);
> > }
> >
> > -blkconf_blocksizes(&dev->conf);
> > +if (!blkconf_blocksizes(&dev->conf, errp)) {
> > +return;
> > +}
> > +
> > if (dev->conf.logical_block_size != 512 ||
> > dev->conf.physical_block_size != 512)
> > {
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 2f3100e56c..672650e162 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1390,7 +1390,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error
> > **errp)
> > host_memory_backend_set_mapped(n->pmrdev, true);
> > }
> >
> > -blkc
Re: [PATCH v7 00/14] LUKS: encryption slot management using amend interface
On Mon, May 18, 2020 at 03:20:27PM +0300, Maxim Levitsky wrote: > Hi! > Here is the updated series of my patches, incorporating all the feedback I > received. > > This implements the API interface that we agreed upon except that I merged the > LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise > I need nested unions which are not supported currently by QAPI parser. > This didn't change the API and thus once support for nested unions is there, > it can always be implemented in backward compatible way. > > I hope that this series will finally be considered for merging, since I am > somewhat running > out of time to finish this task. > > Patches are strictly divided by topic to 3 groups, and each group depends on > former groups. > > * Patches 1,2 implement qcrypto generic amend interface, including definition > of structs used in crypto.json and implement this in luks crypto driver > Nothing is exposed to the user at this stage > > * Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based > encryption slot management > for luks and for qcow2, and add a bunch of iotests to cover that. > > * Patches 10-13 add x-blockdev-amend (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. > > Tested with -raw,-qcow2,-nbd and -luks iotests and 'make check' > > Changes from V4: > * Addresed feedback on patch 2 from Daniel (thanks!) > * Gave real numbers to the tests > * Updated the copyright in amend.c to RedHat > * Rebased and adjusted the python iotests to latest changes in iotest > infrastructure > > Changes from V5: > * Updated all QMP docs to state that all added QMP features are since 5.1 > * Rebased to latest master > > Changes from V6: > * Addressed most of the review feedback from Max Reitz > * Rebased on latest qemu master It needs another rebase as there's some conflicts now. In any case, all the patches have my R-B already, and I don't have any further comments to add. 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: [PATCH v8 2/8] block: consolidate blocksize properties consistency checks
Roman Kagan writes:
> Several block device properties related to blocksize configuration must
> be in certain relationship WRT each other: physical block must be no
> smaller than logical block; min_io_size, opt_io_size, and
> discard_granularity must be a multiple of a logical block.
>
> To ensure these requirements are met, add corresponding consistency
> checks to blkconf_blocksizes, adjusting its signature to communicate
> possible error to the caller. Also remove the now redundant consistency
> checks from the specific devices.
>
> Signed-off-by: Roman Kagan
> Reviewed-by: Eric Blake
> Reviewed-by: Paul Durrant
> ---
> include/hw/block/block.h | 2 +-
> hw/block/block.c | 30 +-
> hw/block/fdc.c | 5 -
> hw/block/nvme.c| 5 -
> hw/block/swim.c| 5 -
> hw/block/virtio-blk.c | 7 +--
> hw/block/xen-block.c | 6 +-
> hw/ide/qdev.c | 5 -
> hw/scsi/scsi-disk.c| 12 +---
> hw/usb/dev-storage.c | 5 -
> tests/qemu-iotests/172.out | 2 +-
> 11 files changed, 58 insertions(+), 26 deletions(-)
>
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index d7246f3862..784953a237 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void
> *buf, hwaddr size,
> bool blkconf_geometry(BlockConf *conf, int *trans,
>unsigned cyls_max, unsigned heads_max, unsigned
> secs_max,
>Error **errp);
> -void blkconf_blocksizes(BlockConf *conf);
> +bool blkconf_blocksizes(BlockConf *conf, Error **errp);
> bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> bool resizable, Error **errp);
>
> diff --git a/hw/block/block.c b/hw/block/block.c
> index bf56c7612b..b22207c921 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void
> *buf, hwaddr size,
> return true;
> }
>
> -void blkconf_blocksizes(BlockConf *conf)
> +bool blkconf_blocksizes(BlockConf *conf, Error **errp)
> {
> BlockBackend *blk = conf->blk;
> BlockSizes blocksizes;
> @@ -83,6 +83,34 @@ void blkconf_blocksizes(BlockConf *conf)
> conf->logical_block_size = BDRV_SECTOR_SIZE;
> }
> }
> +
> +if (conf->logical_block_size > conf->physical_block_size) {
> +error_setg(errp,
> + "logical_block_size > physical_block_size not supported");
> +return false;
> +}
Pardon me if this has been answered already for prior revisions: do we
really support physical block sizes that are not a multiple of the
logical block size?
> +
> +if (!QEMU_IS_ALIGNED(conf->min_io_size, conf->logical_block_size)) {
> +error_setg(errp,
> + "min_io_size must be a multiple of logical_block_size");
> +return false;
> +}
> +
> +if (!QEMU_IS_ALIGNED(conf->opt_io_size, conf->logical_block_size)) {
> +error_setg(errp,
> + "opt_io_size must be a multiple of logical_block_size");
> +return false;
> +}
> +
> +if (conf->discard_granularity != -1 &&
> +!QEMU_IS_ALIGNED(conf->discard_granularity,
> + conf->logical_block_size)) {
> +error_setg(errp, "discard_granularity must be "
> + "a multiple of logical_block_size");
> +return false;
> +}
> +
> +return true;
> }
>
> bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index c5fb9d6ece..8eda572ef4 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -554,7 +554,10 @@ static void floppy_drive_realize(DeviceState *qdev,
> Error **errp)
> read_only = !blk_bs(dev->conf.blk) ||
> blk_is_read_only(dev->conf.blk);
> }
>
> -blkconf_blocksizes(&dev->conf);
> +if (!blkconf_blocksizes(&dev->conf, errp)) {
> +return;
> +}
> +
> if (dev->conf.logical_block_size != 512 ||
> dev->conf.physical_block_size != 512)
> {
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 2f3100e56c..672650e162 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1390,7 +1390,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error
> **errp)
> host_memory_backend_set_mapped(n->pmrdev, true);
> }
>
> -blkconf_blocksizes(&n->conf);
> +if (!blkconf_blocksizes(&n->conf, errp)) {
> +return;
> +}
> +
> if (!blkconf_apply_backend_options(&n->conf,
> blk_is_read_only(n->conf.blk),
> false, errp)) {
> return;
> diff --git a/hw/block/swim.c b/hw/block/swim.c
> index 8f124782f4..74f56e8f46 100644
> --- a/hw/block/swim.c
> +++ b/hw/block/swim.c
> @@ -189,7 +189,10 @@ static void swim_drive_
Re: [PATCH v4 0/6] scripts: More Python fixes
On 5/12/20 12:32 PM, Philippe Mathieu-Daudé wrote: > Trivial Python3 fixes, again... > > Since v3: > - Fixed missing scripts/qemugdb/timers.py (kwolf) > - Cover more scripts > - Check for __main__ in few scripts > > Since v2: > - Remove patch updating MAINTAINERS > > Since v1: > - Added Alex Bennée A-b tags > - Addressed John Snow review comments > - Use /usr/bin/env > - Do not modify os.path (dropped last patch) > > Philippe Mathieu-Daudé (6): > scripts/qemugdb: Remove shebang header > scripts/qemu-gdb: Use Python 3 interpreter > scripts/qmp: Use Python 3 interpreter > scripts/kvm/vmxcap: Use Python 3 interpreter and add pseudo-main() > scripts/modules/module_block: Use Python 3 interpreter & add > pseudo-main > tests/migration/guestperf: Use Python 3 interpreter > > scripts/kvm/vmxcap | 7 --- > scripts/modules/module_block.py| 31 +++--- > scripts/qemu-gdb.py| 4 ++-- > scripts/qemugdb/__init__.py| 3 +-- > scripts/qemugdb/aio.py | 3 +-- > scripts/qemugdb/coroutine.py | 3 +-- > scripts/qemugdb/mtree.py | 4 +--- > scripts/qemugdb/tcg.py | 1 - > scripts/qemugdb/timers.py | 1 - > scripts/qmp/qom-get| 2 +- > scripts/qmp/qom-list | 2 +- > scripts/qmp/qom-set| 2 +- > scripts/qmp/qom-tree | 2 +- > tests/migration/guestperf-batch.py | 2 +- > tests/migration/guestperf-plot.py | 2 +- > tests/migration/guestperf.py | 2 +- > 16 files changed, 33 insertions(+), 38 deletions(-) Thanks, applied to my python-next tree: https://gitlab.com/philmd/qemu/commits/python-next
Re: [PATCH] python: remove more instances of sys.version_info
On 5/14/20 5:52 AM, John Snow wrote: > We guarantee 3.5+ everywhere; remove more dead checks. In general, try > to avoid using version checks and instead prefer to attempt behavior > when possible. > > Signed-off-by: John Snow > --- > scripts/analyze-migration.py | 5 - > scripts/decodetree.py| 25 +--- > scripts/qmp/qmp-shell| 3 --- > tests/docker/docker.py | 5 +++-- > tests/qemu-iotests/nbd-fault-injector.py | 5 + > 5 files changed, 13 insertions(+), 30 deletions(-) Thanks, applied to my python-next tree: https://gitlab.com/philmd/qemu/commits/python-next
Re: [PATCH 1/7] block/nvme: poll queues without q->lock
On Thu, May 28, 2020 at 04:23:50PM +0100, Stefan Hajnoczi wrote:
> On Mon, May 25, 2020 at 10:07:13AM +0200, Sergio Lopez wrote:
> > On Tue, May 19, 2020 at 06:11:32PM +0100, Stefan Hajnoczi wrote:
> > > A lot of CPU time is spent simply locking/unlocking q->lock during
> > > polling. Check for completion outside the lock to make q->lock disappear
> > > from the profile.
> > >
> > > Signed-off-by: Stefan Hajnoczi
> > > ---
> > > block/nvme.c | 12
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/block/nvme.c b/block/nvme.c
> > > index eb2f54dd9d..7eb4512666 100644
> > > --- a/block/nvme.c
> > > +++ b/block/nvme.c
> > > @@ -512,6 +512,18 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
> > >
> > > for (i = 0; i < s->nr_queues; i++) {
> > > NVMeQueuePair *q = s->queues[i];
> > > +const size_t cqe_offset = q->cq.head * NVME_CQ_ENTRY_BYTES;
> > > +NvmeCqe *cqe = (NvmeCqe *)&q->cq.queue[cqe_offset];
> > > +
> > > +/*
> > > + * q->lock isn't needed for checking completion because
> > > + * nvme_process_completion() only runs in the event loop thread
> > > and
> > > + * cannot race with itself.
> > > + */
> > > +if ((le16_to_cpu(cqe->status) & 0x1) == q->cq_phase) {
> > > +continue;
> > > +}
> > > +
> >
> > IIUC, this is introducing an early check of the phase bit to determine
> > if there is something new in the queue.
> >
> > I'm fine with this optimization, but I have the feeling that the
> > comment doesn't properly describe it.
>
> I'm not sure I understand. The comment explains why it's safe not to
> take q->lock. Normally it would be taken. Without the comment readers
> could be confused why we ignore the locking rules here.
>
> As for documenting the cqe->status expression itself, I didn't think of
> explaining it since it's part of the theory of operation of this device.
> Any polling driver will do this, there's nothing QEMU-specific or
> unusual going on here.
>
> Would you like me to expand the comment explaining that NVMe polling
> consists of checking the phase bit of the latest cqe to check for
> readiness?
>
> Or maybe I misunderstood? :)
I was thinking of something like "Do an early check for
completions. We don't need q->lock here because
nvme_process_completion() only runs (...)"
Sergio.
signature.asc
Description: PGP signature
Re: [PATCH 5/5] virtio: enable VIRTIO_F_RING_PACKED for all devices
On 2020/5/23 上午1:17, Stefan Hajnoczi wrote:
The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
ring instead of a split avail/used ring design. There are CPU cache
advantages to this layout and it is also suited better to hardware
implementation.
The vhost-net backend has already supported packed virtqueues for some
time. Performance benchmarks show that virtio-blk performance on NVMe
drives is also improved.
Go ahead and enable this feature for all VIRTIO devices. Keep it
disabled for QEMU 5.0 and earlier machine types.
Signed-off-by: Stefan Hajnoczi
---
include/hw/virtio/virtio.h | 2 +-
hw/core/machine.c | 18 +-
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b69d517496..fd5b4a2044 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -292,7 +292,7 @@ typedef struct VirtIORNGConf VirtIORNGConf;
DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
VIRTIO_F_IOMMU_PLATFORM, false), \
DEFINE_PROP_BIT64("packed", _state, _field, \
- VIRTIO_F_RING_PACKED, false)
+ VIRTIO_F_RING_PACKED, true)
hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
bool virtio_queue_enabled(VirtIODevice *vdev, int n);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index bb3a7b18b1..3598c3c825 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,7 +28,23 @@
#include "hw/mem/nvdimm.h"
#include "migration/vmstate.h"
-GlobalProperty hw_compat_5_0[] = {};
+GlobalProperty hw_compat_5_0[] = {
+{ "vhost-user-blk", "packed", "off" },
+{ "vhost-user-fs-device", "packed", "off" },
+{ "vhost-vsock-device", "packed", "off" },
+{ "virtio-9p-device", "packed", "off" },
+{ "virtio-balloon-device", "packed", "off" },
+{ "virtio-blk-device", "packed", "off" },
+{ "virtio-crypto-device", "packed", "off" },
+{ "virtio-gpu-device", "packed", "off" },
+{ "virtio-input-device", "packed", "off" },
+{ "virtio-iommu-device", "packed", "off" },
+{ "virtio-net-device", "packed", "off" },
+{ "virtio-pmem", "packed", "off" },
+{ "virtio-rng-device", "packed", "off" },
+{ "virtio-scsi-common", "packed", "off" },
+{ "virtio-serial-device", "packed", "off" },
Missing "vhost-user-gpu" here?
I try to do something like this in the past but give up since I end up
with similar list.
It would be better to consider something more smart, probably need some
refactor for a common parent class.
Thanks
+};
const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
GlobalProperty hw_compat_4_2[] = {
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
On 2020/5/23 上午1:17, Stefan Hajnoczi wrote:
Many vhost devices in QEMU currently do not involve the device backend
in feature negotiation. This seems fine at first glance for device types
without their own feature bits (virtio-net has many but other device
types have none).
This overlooks the fact that QEMU's virtqueue implementation and the
device backend's implementation may support different features. QEMU
must not report features to the guest that the the device backend
doesn't support.
For example, QEMU supports VIRTIO 1.1 packed virtqueues while many
existing vhost device backends do not. When the user sets packed=on the
device backend breaks. This should have been handled gracefully by
feature negotiation instead.
Introduce vhost_get_default_features() and update all vhost devices in
QEMU to involve the device backend in feature negotiation.
This patch fixes the following error:
$ x86_64-softmmu/qemu-system-x86_64 \
-drive if=virtio,file=test.img,format=raw \
-chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \
-device vhost-user-blk-pci,chardev=char0,packed=on \
-object memory-backend-memfd,size=1G,share=on,id=ram0 \
-M accel=kvm,memory-backend=ram0
qemu-system-x86_64: Failed to set msg fds.
qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0)
It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED
into user_feature_bits in vhost-user-blk.c.
And for the rest, we need require them to call vhost_get_features() with
the proper feature bits that needs to be acked by the backend.
The vhost-user-blk backend failed as follows:
$ ./vhost-user-blk --socket-path=/tmp/vhost-user-blk.sock -b test2.img
vu_panic: virtio: zero sized buffers are not allowed
virtio-blk request missing headers
Signed-off-by: Stefan Hajnoczi
---
include/hw/virtio/vhost.h| 1 +
include/hw/virtio/virtio-gpu.h | 2 ++
include/sysemu/cryptodev-vhost.h | 11 +++
backends/cryptodev-vhost.c | 19 +++
hw/display/vhost-user-gpu.c | 17 +
hw/display/virtio-gpu-base.c | 2 +-
hw/input/vhost-user-input.c | 9 +
hw/virtio/vhost-user-fs.c| 5 +++--
hw/virtio/vhost-vsock.c | 5 +++--
hw/virtio/vhost.c| 22 ++
hw/virtio/virtio-crypto.c| 3 ++-
11 files changed, 90 insertions(+), 6 deletions(-)
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 085450c6f8..d2e54dd4a8 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -112,6 +112,7 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
VirtIODevice *vdev, int n,
bool mask);
uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
uint64_t features);
+uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features);
void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
uint64_t features);
bool vhost_has_free_slot(void);
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 6dd57f2025..41d270d80e 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -192,6 +192,8 @@ bool virtio_gpu_base_device_realize(DeviceState *qdev,
void virtio_gpu_base_reset(VirtIOGPUBase *g);
void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g,
struct virtio_gpu_resp_display_info *dpy_info);
+uint64_t virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
+ Error **errp);
/* virtio-gpu.c */
void virtio_gpu_ctrl_response(VirtIOGPU *g,
diff --git a/include/sysemu/cryptodev-vhost.h b/include/sysemu/cryptodev-vhost.h
index f42824fbde..e629446bfb 100644
--- a/include/sysemu/cryptodev-vhost.h
+++ b/include/sysemu/cryptodev-vhost.h
@@ -122,6 +122,17 @@ int cryptodev_vhost_start(VirtIODevice *dev, int
total_queues);
*/
void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues);
+/**
+ * cryptodev_vhost_get_features:
+ * @dev: the virtio crypto object
+ * @requested_features: the features being offered
+ *
+ * Returns: the requested features bits that are supported by the vhost device,
+ * or the original request feature bits if vhost is disabled
+ *
+ */
+uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features);
+
/**
* cryptodev_vhost_virtqueue_mask:
* @dev: the virtio crypto object
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index 8337c9a495..5f5a4fda7b 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -266,6 +266,20 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int
total_queues)
assert(r >= 0);
}
+uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features)
+{
+VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
+Cr
