Re: [PATCH v5 5/7] migration: Deprecate old compression method

[Markus Armbruster]

Juan Quintela  writes:

> Markus Armbruster  wrote:
>> Juan Quintela  writes:
>>
>>> Signed-off-by: Juan Quintela 
>>> Acked-by: Stefan Hajnoczi 
>>> Acked-by: Peter Xu 
>
>
>>>  # @deprecated: Member @disk is deprecated because block migration is.
>>> +# Member @compression is deprecated because it is unreliable and
>>> +# untested. It is recommended to use multifd migration, which
>>> +# offers an alternative compression implementation that is
>>> +# reliable and tested.
>>
>> Two spaces between sentences for consistency, please.
>
> I have reviewed all the patches again.  Let's hope that I didn't miss
> one.
>
>>>  # @announce-step: Increase in delay (in milliseconds) between
>>>  # subsequent packets in the announcement (Since 4.0)
>>>  #
>>> -# @compress-level: compression level
>>> +# @compress-level: compression level.
>>>  #
>>> -# @compress-threads: compression thread count
>>> +# @compress-threads: compression thread count.
>>>  #
>>>  # @compress-wait-thread: Controls behavior when all compression
>>>  # threads are currently busy.  If true (default), wait for a free
>>>  # compression thread to become available; otherwise, send the page
>>> -# uncompressed.  (Since 3.1)
>>> +# uncompressed. (Since 3.1)
>>>  #
>>> -# @decompress-threads: decompression thread count
>>> +# @decompress-threads: decompression thread count.
>>>  #
>>>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>>>  # bytes_xfer_period to trigger throttling.  It is expressed as
>>
>> Unrelated.
>
> Rebases are bad for you O:-)
>
>>> @@ -1023,7 +1036,9 @@
>>>  # Features:
>>>  #
>>>  # @deprecated: Member @block-incremental is deprecated. Use
>>> -# blockdev-mirror with NBD instead.
>>> +# blockdev-mirror with NBD instead. Members @compress-level,
>>> +# @compress-threads, @decompress-threads and @compress-wait-thread
>>> +# are deprecated because @compression is deprecated.
>>
>> Two spaces between sentences for consistency, please.
>
> Done.
>>> @@ -1078,7 +1097,7 @@
>>>  # Example:
>>>  #
>>>  # -> { "execute": "migrate-set-parameters" ,
>>> -#  "arguments": { "compress-level": 1 } }
>>> +#  "arguments": { "multifd-channels": 5 } }
>>>  # <- { "return": {} }
>>>  ##
>>
>> Thanks for taking care of updating the example!
>
> You are welcome.  grep for all occurences of compress-level and friends
> has its advantages.
>
>>>  # @compress-wait-thread: Controls behavior when all compression
>>>  # threads are currently busy.  If true (default), wait for a free
>>>  # compression thread to become available; otherwise, send the page
>>> -# uncompressed.  (Since 3.1)
>>> +# uncompressed. (Since 3.1)
>>>  #
>>> -# @decompress-threads: decompression thread count
>>> +# @decompress-threads: decompression thread count.
>>>  #
>>>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>>>  # bytes_xfer_period to trigger throttling.  It is expressed as
>>
>> Unrelated.
>
> I have removed the periods.
>
> But I have a question, why the descriptions that are less than one line
> don't have period and the other have it.

When the description consists of multiple sentences, we obviously need
to end them with punctuation.

Sometimes the description isn't a sentence, just a phrase,
e.g. "decompression thread count".  No punctuation then.

Sometimes it's a single sentence.  Then we roll dice.

>>>  if (params->has_compress_level) {
>>> +warn_report("Old compression is deprecated. "
>>> +"Use multifd compression methods instead.");
>>>  s->parameters.compress_level = params->compress_level;
>>>  }
>>>  
>>>  if (params->has_compress_threads) {
>>> +warn_report("Old compression is deprecated. "
>>> +"Use multifd compression methods instead.");
>>>  s->parameters.compress_threads = params->compress_threads;
>>>  }
>>>  
>>>  if (params->has_compress_wait_thread) {
>>> +warn_report("Old compression is deprecated. "
>>> +"Use multifd compression methods instead.");
>>>  s->parameters.compress_wait_thread = params->compress_wait_thread;
>>>  }
>>>  
>>>  if (params->has_decompress_threads) {
>>> +warn_report("Old compression is deprecated. "
>
> Once here, I did s/Old/old/
>
> as all your examples of description start with lowercase.

Yes, please.

>>> +"Use multifd compression methods instead.");
>>>  s->parameters.decompress_threads = params->decompress_threads;
>>>  }
>>
>> Other than that
>> Reviewed-by: Markus Armbruster 
>
> Thanks for your patience.

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Igor Mammedov]

On Mon, 16 Oct 2023 16:19:08 +0100
David Woodhouse  wrote:

> From: David Woodhouse 
> 

is this index a user (guest) visible?

> There's no need to force the user to assign a vdev. We can automatically
> assign one, starting at xvda and searching until we find the first disk
> name that's unused.
> 
> This means we can now allow '-drive if=xen,file=xxx' to work without an
> explicit separate -driver argument, just like if=virtio.
> 
> Signed-off-by: David Woodhouse 
> ---
>  blockdev.c   | 15 ---
>  hw/block/xen-block.c | 25 +
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 325b7a3bef..9dec4c9c74 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -255,13 +255,13 @@ void drive_check_orphaned(void)
>   * Ignore default drives, because we create certain default
>   * drives unconditionally, then leave them unclaimed.  Not the
>   * users fault.
> - * Ignore IF_VIRTIO, because it gets desugared into -device,
> - * so we can leave failing to -device.
> + * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
> + * -device, so we can leave failing to -device.
>   * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
>   * available for device_add is a feature.
>   */
>  if (dinfo->is_default || dinfo->type == IF_VIRTIO
> -|| dinfo->type == IF_NONE) {
> +|| dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
>  continue;
>  }
>  if (!blk_get_attached_dev(blk)) {
> @@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type,
>  qemu_opt_set(devopts, "driver", "virtio-blk", &error_abort);
>  qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
>   &error_abort);
> +} else if (type == IF_XEN) {
> +QemuOpts *devopts;
> +devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> +   &error_abort);
> +qemu_opt_set(devopts, "driver",
> + (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
> + &error_abort);
> +qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> + &error_abort);
>  }
>  
>  filename = qemu_opt_get(legacy_opts, "file");
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 9262338535..20fa783cbe 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error 
> **errp)
>  XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
>  XenBlockVdev *vdev = &blockdev->props.vdev;
>  
> +if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> +char name[11];
> +int disk = 0;
> +unsigned long idx;
> +
> +/* Find an unoccupied device name */
> +while (disk < (1 << 20)) {
> +if (disk < (1 << 4)) {
> +idx = (202 << 8) | (disk << 4);
> +} else {
> +idx = (1 << 28) | (disk << 8);
> +}
> +snprintf(name, sizeof(name), "%lu", idx);
> +if (!xen_backend_exists("qdisk", name)) {
> +vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
> +vdev->partition = 0;
> +vdev->disk = disk;
> +vdev->number = idx;
> +return g_strdup(name);
> +}
> +disk++;
> +}
> +error_setg(errp, "cannot find device vdev for block device");
> +return NULL;
> +}
>  return g_strdup_printf("%lu", vdev->number);
>  }
>  

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote:
> On Mon, 16 Oct 2023 16:19:08 +0100
> David Woodhouse  wrote:
> 
> > From: David Woodhouse 
> > 
> 
> is this index a user (guest) visible?

Yes. It defines what block device (e.g. /dev/xvda) the disk appears as
in the guest. In the common case, it literally encodes the Linux
major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc.

Previously we had to explicitly set it for each disk in Qemu:

  -drive file=disk1.img,id=drv1 -device xen-disk,drive=drv1,vdev=xvda
  -drive file=disk2.img,id=drv2 -device xen-disk,drive=drv2,vdev=xvdb

Now we can just do

  -drive file=disk1.img,if=xen -drive file-disk2.img,if=xen

(We could go further and make if=xen the default for Xen guests too,
but that doesn't work right now because xen-block will barf on the
default provided CD-ROM when it's empty. It doesn't handle empty
devices. And if I work around that, then `-hda disk1.img` would work on
the command line... but would make it appear as /dev/xvda instead of
/dev/hda, and I don't know how I feel about that.)

[root@localhost ~]# xenstore-ls  -f device/vbd
device/vbd = ""
device/vbd/51712 = ""
device/vbd/51712/backend = "/local/domain/0/backend/qdisk/1/51712"
device/vbd/51712/backend-id = "0"
device/vbd/51712/device-type = "disk"
device/vbd/51712/event-channel = "8"
device/vbd/51712/feature-persistent = "1"
device/vbd/51712/protocol = "x86_64-abi"
device/vbd/51712/ring-ref = "8"
device/vbd/51712/state = "4"
device/vbd/51712/virtual-device = "51712"

> 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Signed-off-by: David Woodhouse 
> > ---
> >  blockdev.c   | 15 ---
> >  hw/block/xen-block.c | 25 +
> >  2 files changed, 37 insertions(+), 3 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 325b7a3bef..9dec4c9c74 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -255,13 +255,13 @@ void drive_check_orphaned(void)
> >   * Ignore default drives, because we create certain default
> >   * drives unconditionally, then leave them unclaimed.  Not the
> >   * users fault.
> > - * Ignore IF_VIRTIO, because it gets desugared into -device,
> > - * so we can leave failing to -device.
> > + * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
> > + * -device, so we can leave failing to -device.
> >   * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
> >   * available for device_add is a feature.
> >   */
> >  if (dinfo->is_default || dinfo->type == IF_VIRTIO
> > -    || dinfo->type == IF_NONE) {
> > +    || dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
> >  continue;
> >  }
> >  if (!blk_get_attached_dev(blk)) {
> > @@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> > BlockInterfaceType block_default_type,
> >  qemu_opt_set(devopts, "driver", "virtio-blk", &error_abort);
> >  qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> >   &error_abort);
> > +    } else if (type == IF_XEN) {
> > +    QemuOpts *devopts;
> > +    devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> > +   &error_abort);
> > +    qemu_opt_set(devopts, "driver",
> > + (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
> > + &error_abort);
> > +    qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> > + &error_abort);
> >  }
> >  
> >  filename = qemu_opt_get(legacy_opts, "file");
> > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> > index 9262338535..20fa783cbe 100644
> > --- a/hw/block/xen-block.c
> > +++ b/hw/block/xen-block.c
> > @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error 
> > **errp)
> >  XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
> >  XenBlockVdev *vdev = &blockdev->props.vdev;
> >  
> > +    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> > +    char name[11];
> > +    int disk = 0;
> > +    unsigned long idx;
> > +
> > +    /* Find an unoccupied device name */
> > +    while (disk < (1 << 20)) {
> > +    if (disk < (1 << 4)) {
> > +    idx = (202 << 8) | (disk << 4);
> > +    } else {
> > +    idx = (1 << 28) | (disk << 8);
> > +    }
> > +    snprintf(name, sizeof(name), "%lu", idx);
> > +    if (!xen_backend_exists("qdisk", name)) {
> > +    vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
> > +    vdev->partition = 0;
> >

Re: [PATCH v8 3/5] vhost-user-scsi: support reconnect to backend

[Manos Pitsidianakis]

If the backend crashes and restarts, the device is broken.
This patch adds reconnect for vhost-user-scsi.

This patch also improves the error messages, and reports some silent 
errors.


Tested with spdk backend.

Signed-off-by: Li Feng 
---


Reviewed-by: Manos Pitsidianakis 

Re: [PATCH v8 0/5] Implement reconnect for vhost-user-scsi

[Li Feng]

Hello Guys,
Ping… 
These patches have been waiting for a long time. Can they be merged?

Best Regards, 
li

> On 9 Oct 2023, at 12:46 PM, Li Feng  wrote:
> 
> Changes for v8:
> - [PATCH 3/5] vhost-user-scsi: support reconnect to backend
>  - Fix code style suggested by Manos Pitsidianakis
> - [PATCH 4/5] vhost-user-scsi: start vhost when guest kicks
>  - Use 'DEVICE()' macro in vhost_user_scsi_handle_output to replace the
>'parent_obj.parent_obj.parent_obj.parent_obj'.
> 
> Changes for v7:
> - [PATCH 3/5] vhost-user-scsi: support reconnect to backend
>  - Add reporting the error in vhost-scsi;
>  - Rebase to master and fix the conflict.
> - Add "Reviewed-by" tags.
> 
> Changes for v6:
> - [PATCH] vhost-user: fix lost reconnect
>  - Fix missing assign event_cb.
> 
> Changes for v5:
> - No logic has been changed, just move part of the code from patch 4 to patch 
> 5.
> 
> Changes for v4:
> - Merge
>  https://lore.kernel.org/all/20230830045722.611224-1-fen...@smartx.com/ to
>  this series.
> - Add ERRP_GUARD in vhost_user_scsi_realize;
> - Reword the commit messages.
> 
> Changes for v3:
> - Split the vhost_user_scsi_handle_output to a separate patch;
> - Move the started_vu from vhost scsi common header to vhost-user-scsi header;
> - Fix a log print error;
> 
> Changes for v2:
> - Split the v1 patch to small separate patchset;
> - New patch for fixing fd leak, which has sent to reviewers in another
>  mail;
> - Implement the `vhost_user_scsi_handle_output`;
> - Add the started_vu safe check;
> - Fix error handler;
> - Check the inflight before set/get inflight fd.
> 
> Li Feng (5):
>  vhost-user-common: send get_inflight_fd once
>  vhost: move and rename the conn retry times
>  vhost-user-scsi: support reconnect to backend
>  vhost-user-scsi: start vhost when guest kicks
>  vhost-user: fix lost reconnect
> 
> hw/block/vhost-user-blk.c |   6 +-
> hw/scsi/vhost-scsi-common.c   |  47 ++---
> hw/scsi/vhost-scsi.c  |   6 +-
> hw/scsi/vhost-user-scsi.c | 250 +++---
> hw/virtio/vhost-user-gpio.c   |   5 +-
> hw/virtio/vhost-user.c|  10 +-
> include/hw/virtio/vhost-scsi-common.h |   2 +-
> include/hw/virtio/vhost-user-scsi.h   |   6 +
> include/hw/virtio/vhost-user.h|   3 +-
> include/hw/virtio/vhost.h |   2 +
> 10 files changed, 277 insertions(+), 60 deletions(-)
> 
> -- 
> 2.41.0
> 

Re: [PATCH v6 2/5] migration: migrate 'inc' command option is deprecated.

[Markus Armbruster]

Juan Quintela  writes:

> Use blockdev-mirror with NBD instead.
>
> Reviewed-by: Thomas Huth 
> Acked-by: Stefan Hajnoczi 
> Reviewed-by: Markus Armbruster 
> Signed-off-by: Juan Quintela 
> ---
>  docs/about/deprecated.rst  | 9 +
>  qapi/migration.json| 8 +++-
>  migration/migration-hmp-cmds.c | 5 +
>  migration/migration.c  | 5 +
>  4 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 2febd2d12f..b51136f50a 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -461,3 +461,12 @@ Migration
>  ``skipped`` field in Migration stats has been deprecated.  It hasn't
>  been used for more than 10 years.
>  
> +``inc`` migrate command option (since 8.2)
> +''
> +
> +Use blockdev-mirror with NBD instead.
> +
> +As an intermediate step the ``inc`` functionality can be achieved by
> +setting the ``block-incremental`` migration parameter to ``true``.
> +But this parameter is also deprecated.
> +

If you need to respin for some other reason, drop the blank line at end
of file.  Same in later patches.

[...]

Re: [PATCH v8 0/5] Implement reconnect for vhost-user-scsi

[Michael S. Tsirkin]

Queued. Thanks!
On Wed, Oct 18, 2023 at 04:26:10PM +0800, Li Feng wrote:
> Hello Guys,
> 
> Ping… 
> 
> These patches have been waiting for a long time. Can they be merged?
> 
> 
> Best Regards, 
> 
> li
> 
> 
> On 9 Oct 2023, at 12:46 PM, Li Feng  wrote:
> 
> Changes for v8:
> - [PATCH 3/5] vhost-user-scsi: support reconnect to backend
>  - Fix code style suggested by Manos Pitsidianakis
> - [PATCH 4/5] vhost-user-scsi: start vhost when guest kicks
>  - Use 'DEVICE()' macro in vhost_user_scsi_handle_output to replace the
>'parent_obj.parent_obj.parent_obj.parent_obj'.
> 
> Changes for v7:
> - [PATCH 3/5] vhost-user-scsi: support reconnect to backend
>  - Add reporting the error in vhost-scsi;
>  - Rebase to master and fix the conflict.
> - Add "Reviewed-by" tags.
> 
> Changes for v6:
> - [PATCH] vhost-user: fix lost reconnect
>  - Fix missing assign event_cb.
> 
> Changes for v5:
> - No logic has been changed, just move part of the code from patch 4 to
> patch 5.
> 
> Changes for v4:
> - Merge
>  https://lore.kernel.org/all/20230830045722.611224-1-fen...@smartx.com/ to
>  this series.
> - Add ERRP_GUARD in vhost_user_scsi_realize;
> - Reword the commit messages.
> 
> Changes for v3:
> - Split the vhost_user_scsi_handle_output to a separate patch;
> - Move the started_vu from vhost scsi common header to vhost-user-scsi
> header;
> - Fix a log print error;
> 
> Changes for v2:
> - Split the v1 patch to small separate patchset;
> - New patch for fixing fd leak, which has sent to reviewers in another
>  mail;
> - Implement the `vhost_user_scsi_handle_output`;
> - Add the started_vu safe check;
> - Fix error handler;
> - Check the inflight before set/get inflight fd.
> 
> Li Feng (5):
>  vhost-user-common: send get_inflight_fd once
>  vhost: move and rename the conn retry times
>  vhost-user-scsi: support reconnect to backend
>  vhost-user-scsi: start vhost when guest kicks
>  vhost-user: fix lost reconnect
> 
> hw/block/vhost-user-blk.c |   6 +-
> hw/scsi/vhost-scsi-common.c   |  47 ++---
> hw/scsi/vhost-scsi.c  |   6 +-
> hw/scsi/vhost-user-scsi.c | 250 +++---
> hw/virtio/vhost-user-gpio.c   |   5 +-
> hw/virtio/vhost-user.c|  10 +-
> include/hw/virtio/vhost-scsi-common.h |   2 +-
> include/hw/virtio/vhost-user-scsi.h   |   6 +
> include/hw/virtio/vhost-user.h|   3 +-
> include/hw/virtio/vhost.h |   2 +
> 10 files changed, 277 insertions(+), 60 deletions(-)
> 
> --
> 2.41.0
> 
> 
> 

Re: [PATCH 4/4] qapi: introduce CONFIG_READ event

[Vladimir Sementsov-Ogievskiy]

On 18.10.23 09:47, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


On 17.10.23 18:00, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


Send a new event when guest reads virtio-pci config after
virtio_notify_config() call.

That's useful to check that guest fetched modified config, for example
after resizing disk backend.

Signed-off-by: Vladimir Sementsov-Ogievskiy 


[...]


diff --git a/qapi/qdev.json b/qapi/qdev.json
index 2468f8bddf..37a8785b81 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -329,3 +329,25 @@
   # Since: 8.2
   ##
   { 'command': 'x-device-sync-config', 'data': {'id': 'str'} }
+
+##
+# @X_CONFIG_READ:
+#
+# Emitted whenever guest reads virtio device config after config change.
+#
+# @device: device name
+#
+# @path: device path
+#
+# Since: 5.0.1-24
+#
+# Example:
+#
+# <- { "event": "X_CONFIG_READ",
+#  "data": { "device": "virtio-net-pci-0",
+#"path": "/machine/peripheral/virtio-net-pci-0" },
+#  "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+#
+##
+{ 'event': 'X_CONFIG_READ',
+  'data': { '*device': 'str', 'path': 'str' } }


The commit message talks about event CONFIG_READ, but you actually name
it x-device-sync-config.


will fix


I figure you use x- to signify "unstable".  Please use feature flag
'unstable' for that.  See docs/devel/qapi-code-gen.rst section
"Features", in particular "Special features", and also the note on x- in
section "Naming rules and reserved names".


OK, will do.

Hmm, it say

Names beginning with ``x-`` used to signify "experimental".  This
convention has been replaced by special feature "unstable".

"replaced".. So, I should use "unstable" flag without "x-" prefix? Can't find an example. Seems 
"unstable" always used together with "x-".


True.

The "x-" prefix originated with qdev properties.  First use might be
commit f0c07c7c7b4.  The convention wasn't documented then, but QOM/qdev
properties have always been a documentation wasteland.  It then spread
to other places, and eventually to the QAPI schema.  Where we try pretty
hard to document things properly.  We documented the "x-" prefix in
commit e790e666518:

 Any name (command, event, type, field, or enum value) beginning with
 "x-" is marked experimental, and may be withdrawn or changed
 incompatibly in a future release.

Minor pain point: when things grow up from experimental to stable, we
have to rename.

The convention didn't stop us from naming non-experimental things x-FOO,
e.g. QOM property "x-origin" in commit 6105683da35.  Made it to the QAPI
schema in commit 8825587b53c.  Point is: the prefix isn't a reliable
marker for "unstable".

Since I needed a reliable marker for my "set policy for unstable
interfaces" feature (see CLI option -compat), I created special feature
flag "unstable", and dropped the "x-" convention for the QAPI schema.

Renaming existing "x-" names felt like pointless churn, so I didn't.

I'm not objecting to new names starting with "x-".  Nor am I objecting
to feature 'unstable' on names that don't start with "x-".

I guess "x-" remains just fine for things we don't intend to make stable
at some point.  The "x-" can remind humans "this is unstable" better
than a feature flag can (for machines, it's the other way round).

For things we do intend (hope?) to make stable, I wouldn't bother with
the "x-".

Clearer now?


Yes, thanks.

x- seems safer for management tool that doesn't know about "unstable" 
properties..

But on the other hand, changing from x- to no-prefix is already done when the 
feature is stable, and thouse who use it already use the latest version of 
interface, so, removing the prefix is just extra work.

So, I think, I'd go without prefix.




Also, nothing said about events. Is using "X_" wrong idea? Should it be 
x-SOME_EVENT instead?


Since this is the first unstable event, there is no precedent.  Let's
use no prefix, and move on.


The name CONFIG_READ feels overly generic for something that makes sense
only with virtio devices.


Hmm, right.. I think, we can say same thing about DEVICE_UNPLUG_GUEST_ERROR.


That one came to be as a generalization of existing MEM_UNPLUG_ERROR and
a concrete need to signal CPU unplug errors.  Demonstrates "unplug guest
errors" can happen for different kinds of devices.  So we went with a
generic event we can use for all of them.

This doesn't seem to be the case for this patch's event.  Thoughts?


Right.. VIRTIO_CONFIG_READ maybe?




So, what about DEVICE_GUEST_READ_CONFIG ?




diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index b485375049..d0f022e925 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1252,3 +1252,8 @@ void qdev_hotplug_device_on_event(DeviceState *dev)
   dev->device_on_event_sent = true;
   qapi_event_send_x_device_on(dev->id, dev->canonical_path);
   }
+
+void qdev_config_read_event(DeviceState *dev)
+{
+qapi_event_send_x_config_read(dev->id, dev->ca

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Kevin Wolf]

Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> From: David Woodhouse 
> 
> There's no need to force the user to assign a vdev. We can automatically
> assign one, starting at xvda and searching until we find the first disk
> name that's unused.
> 
> This means we can now allow '-drive if=xen,file=xxx' to work without an
> explicit separate -driver argument, just like if=virtio.
> 
> Signed-off-by: David Woodhouse 

Actually, how does this play together with xen_config_dev_blk()? This
looks like it tried to implement a very similar thing (which is IF_XEN
even already existed).

Are we now trying to attach each if=xen disk twice in the 'xenpv'
machine? Or if something prevents this, is it dead code?

I think in both cases, we would want to delete that function and remove
the loop that calls it in xen_init_pv()?

Kevin

Re: [PATCH v3 6/9] qapi/block-core: use JobType for BlockJobInfo's type

[Markus Armbruster]

Fiona Ebner  writes:

> In preparation to turn BlockJobInfo into a union with @type as the
> discriminator. That requires it to be an enum.
>
> No functional change is intended.
>
> Signed-off-by: Fiona Ebner 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Replacing str by enum makes sense whether we need enum for a union or
not.

Reviewed-by: Markus Armbruster 

Re: [PATCH v3 5/9] mirror: implement mirror_change method

[Markus Armbruster]

Fiona Ebner  writes:

> which allows switching the @copy-mode from 'background' to
> 'write-blocking'.
>
> This is useful for management applications, so they can start out in
> background mode to avoid limiting guest write speed and switch to
> active mode when certain criteria are fulfilled.
>
> In presence of an iothread, the copy_mode member is now shared between
> the iothread and the main thread, so turn accesses to it atomic.
>
> Signed-off-by: Fiona Ebner 

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c6f31a9399..01427c259a 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3044,6 +3044,17 @@
>  { 'command': 'block-job-finalize', 'data': { 'id': 'str' },
>'allow-preconfig': true }
>  
> +##
> +# @BlockJobChangeOptionsMirror:
> +#
> +# @copy-mode: Switch to this copy mode. Currenlty, only the switch

Typo: Currently

Also, two spaces between sentences for consistency, please.

> +# from 'background' to 'write-blocking' is implemented.
> +#
> +# Since: 8.2
> +##
> +{ 'struct': 'BlockJobChangeOptionsMirror',
> +  'data': { 'copy-mode' : 'MirrorCopyMode' } }
> +
>  ##
>  # @BlockJobChangeOptions:
>  #
> @@ -3058,7 +3069,7 @@
>  { 'union': 'BlockJobChangeOptions',
>'base': { 'id': 'str', 'type': 'JobType' },
>'discriminator': 'type',
> -  'data': {} }
> +  'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }
>  
>  ##
>  # @block-job-change:

Re: [PATCH v3 0/9] mirror: allow switching from background to active mode

[Markus Armbruster]

Fiona Ebner  writes:

> Changes in v3:
> * unlock the job mutex when calling the new block job driver
>   'query' handler
> * squash patch adapting iotest output into patch that changes the
>   output
> * turn accesses to copy_mode and actively_synced atomic
> * slightly rework error handling in mirror_change
>
> Changes in v2:
> * move bitmap to filter which allows to avoid draining when
>   changing the copy mode
> * add patch to determine copy_to_target only once
> * drop patches returning redundant information upon query
> * update QEMU version in QAPI
> * update indentation in QAPI
> * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
>   doc comments to conform to current conventions"))
> * add patch to adapt iotest output
>
> Discussion of v2:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02290.html
>
> Discussion of v1:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg07216.html
>
> With active mode, the guest write speed is limited by the synchronous
> writes to the mirror target. For this reason, management applications
> might want to start out in background mode and only switch to active
> mode later, when certain conditions are met. This series adds a
> block-job-change QMP command to achieve that, as well as
> job-type-specific information when querying block jobs, which
> can be used to decide when the switch should happen.
>
> For now, only the direction background -> active is supported.
>
> The information added upon querying is whether the target is actively
> synced, the total data sent, and the remaining dirty bytes.
>
> Initially, I tried to go for a more general 'job-change' command, but
> to avoid mutual inclusion of block-core.json and job.json, more
> preparation would be required.

Can you elaborate a bit?  A more generic command is preferable, and we
need to understand what it would take.

Re: deadlock when using iothread during backup_clean()

[Fiona Ebner]

Am 17.10.23 um 16:20 schrieb Kevin Wolf:
> Am 17.10.2023 um 15:37 hat Fiona Ebner geschrieben:
>> Am 17.10.23 um 14:12 schrieb Kevin Wolf:
>>> Am 17.10.2023 um 12:18 hat Fiona Ebner geschrieben:
 I ran into similar issues now with mirror, (both deadlocks and stuck
 guest IO at other times), and interestingly, also during job start.

 Also had a backtrace similar to [0] once, so I took a closer look.
 Probably was obvious to others already, but for the record:

 1. the graph is locked by the main thread
 2. the iothread holds the AioContext lock
 3. the main thread waits on the AioContext lock
 4. the iothread waits for coroutine spawned by blk_is_available()
>>>
>>> Where does this blk_is_available() in the iothread come from? Having it
>>> wait without dropping the AioContext lock sounds like something that
>>> we'd want to avoid. Ideally, devices using iothreads shouldn't use
>>> synchronous requests at all, but I think scsi-disk might have some of
>>> them.
>>>
>>
>> It's part of the request handling in virtio-scsi:
>>
>>> #0  0x7ff7f5f55136 in __ppoll (fds=0x7ff7e40030c0, nfds=8, 
>>> timeout=, sigmask=0x0) at 
>>> ../sysdeps/unix/sysv/linux/ppoll.c:42
>>> #1  0x5587132615ab in qemu_poll_ns (fds=0x7ff7e40030c0, nfds=8, 
>>> timeout=-1) at ../util/qemu-timer.c:339
>>> #2  0x55871323e8b1 in fdmon_poll_wait (ctx=0x55871598d5e0, 
>>> ready_list=0x7ff7f288ebe0, timeout=-1) at ../util/fdmon-poll.c:79
>>> #3  0x55871323e1ed in aio_poll (ctx=0x55871598d5e0, blocking=true) at 
>>> ../util/aio-posix.c:670
>>> #4  0x558713089efa in bdrv_poll_co (s=0x7ff7f288ec90) at 
>>> /home/febner/repos/qemu/block/block-gen.h:43
>>> #5  0x55871308c362 in blk_is_available (blk=0x55871599e2f0) at 
>>> block/block-gen.c:1426
>>> #6  0x558712f6843b in virtio_scsi_ctx_check (s=0x558716c049c0, 
>>> d=0x55871581cd30) at ../hw/scsi/virtio-scsi.c:290
> 
> Oh... So essentially for an assertion.
> 
> I wonder if the blk_is_available() check introduced in 2a2d69f490c is
> even necessary any more, because BlockBackend has its own AioContext
> now. And if blk_bs(blk) != NULL isn't what we actually want to check if
> the check is necessary, because calling bdrv_is_inserted() doesn't seem
> to have been intended. blk_bs() wouldn't have to poll.
> 

Could virtio_scsi_hotunplug() be an issue with removing or modifying the
check? There's a call there which sets the blk's AioContext to
qemu_get_aio_context(). Or are we sure that the assert in
virtio_scsi_ctx_check() can't be reached after that?

>>> #7  0x558712f697e4 in virtio_scsi_handle_cmd_req_prepare 
>>> (s=0x558716c049c0, req=0x7ff7e400b650) at ../hw/scsi/virtio-scsi.c:788
>>> #8  0x558712f699b0 in virtio_scsi_handle_cmd_vq (s=0x558716c049c0, 
>>> vq=0x558716c0d2a8) at ../hw/scsi/virtio-scsi.c:831
>>> #9  0x558712f69bcb in virtio_scsi_handle_cmd (vdev=0x558716c049c0, 
>>> vq=0x558716c0d2a8) at ../hw/scsi/virtio-scsi.c:867
>>> #10 0x558712f96812 in virtio_queue_notify_vq (vq=0x558716c0d2a8) at 
>>> ../hw/virtio/virtio.c:2263
>>> #11 0x558712f99b75 in virtio_queue_host_notifier_read 
>>> (n=0x558716c0d31c) at ../hw/virtio/virtio.c:3575
>>> #12 0x55871323d8b5 in aio_dispatch_handler (ctx=0x55871598d5e0, 
>>> node=0x558716771000) at ../util/aio-posix.c:372
>>> #13 0x55871323d988 in aio_dispatch_ready_handlers (ctx=0x55871598d5e0, 
>>> ready_list=0x7ff7f288eeb0) at ../util/aio-posix.c:401
>>
>>
 As for why it doesn't progress, blk_co_is_available_entry() uses
 bdrv_graph_co_rdlock() and can't get it, because the main thread has the
 write lock. Should be fixed once the AioContext locks are gone, but not
 sure what should be done to avoid it until then.
>>>
>>> Then the nested event loop in blk_is_available() would probably be
>>> enough to make progress, yes.
>>>
>>> Maybe we could actually drop the lock (and immediately reacquire it) in
>>> AIO_WAIT_WHILE() even if we're in the home thread? That should give the
>>> main thread a chance to make progress.
>>
>> Seems to work :) I haven't run into the issue with the following change
>> anymore, but I have to say, running into that specific deadlock only
>> happened every 10-15 tries or so before. Did 30 tests now. But
>> unfortunately, the stuck IO issue is still there.
>>
>>> diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
>>> index 5449b6d742..da159501ca 100644
>>> --- a/include/block/aio-wait.h
>>> +++ b/include/block/aio-wait.h
>>> @@ -88,7 +88,13 @@ extern AioWait global_aio_wait;
>>>  smp_mb__after_rmw();   \
>>>  if (ctx_ && in_aio_context_home_thread(ctx_)) {\
>>>  while ((cond)) {   \
>>> +if (unlock && ctx_) {  \
>>> +aio_context_release(ctx_); \
>>> +}  

Re: [PATCH v3 0/9] mirror: allow switching from background to active mode

[Fiona Ebner]

Am 18.10.23 um 11:41 schrieb Markus Armbruster:
> Fiona Ebner  writes:
>>
>> Initially, I tried to go for a more general 'job-change' command, but
>> to avoid mutual inclusion of block-core.json and job.json, more
>> preparation would be required.
> 
> Can you elaborate a bit?  A more generic command is preferable, and we
> need to understand what it would take.
> 

Yes, I did so during the discussion of v2:

https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02993.html

Best Regards,
Fiona

Re: [PATCH v6 2/5] migration: migrate 'inc' command option is deprecated.

[Juan Quintela]

Markus Armbruster  wrote:
> Juan Quintela  writes:
>
>> Use blockdev-mirror with NBD instead.
>>
>> Reviewed-by: Thomas Huth 
>> Acked-by: Stefan Hajnoczi 
>> Reviewed-by: Markus Armbruster 
>> Signed-off-by: Juan Quintela 
>> ---
>>  docs/about/deprecated.rst  | 9 +
>>  qapi/migration.json| 8 +++-
>>  migration/migration-hmp-cmds.c | 5 +
>>  migration/migration.c  | 5 +
>>  4 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index 2febd2d12f..b51136f50a 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -461,3 +461,12 @@ Migration
>>  ``skipped`` field in Migration stats has been deprecated.  It hasn't
>>  been used for more than 10 years.
>>  
>> +``inc`` migrate command option (since 8.2)
>> +''
>> +
>> +Use blockdev-mirror with NBD instead.
>> +
>> +As an intermediate step the ``inc`` functionality can be achieved by
>> +setting the ``block-incremental`` migration parameter to ``true``.
>> +But this parameter is also deprecated.
>> +
>
> If you need to respin for some other reason, drop the blank line at end
> of file.  Same in later patches.
>
> [...]

Done.

There is a tool, git-am maybe, that complains if files don't end in a
blank line.

You can't have happy everybody.

Later, Juan.

[PATCH v7 4/4] migration: Deprecate old compression method

[Juan Quintela]

Acked-by: Stefan Hajnoczi 
Acked-by: Peter Xu 
Reviewed-by: Markus Armbruster 
Signed-off-by: Juan Quintela 
---
 docs/about/deprecated.rst |  8 +
 qapi/migration.json   | 63 ++-
 migration/options.c   | 13 
 3 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 7ae872162d..e7f17827d3 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -488,3 +488,11 @@ devices or none.
 Please see "QMP invocation for live storage migration with
 ``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst
 for a detailed explanation.
+
+old compression method (since 8.2)
+''
+
+Compression method fails too much.  Too many races.  We are going to
+remove it if nobody fixes it.  For starters, migration-test
+compression tests are disabled becase they fail randomly.  If you need
+compression, use multifd compression methods.
diff --git a/qapi/migration.json b/qapi/migration.json
index e3b00a215b..e6610af428 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -272,6 +272,10 @@
 # Features:
 #
 # @deprecated: Member @disk is deprecated because block migration is.
+# Member @compression is deprecated because it is unreliable and
+# untested.  It is recommended to use multifd migration, which
+# offers an alternative compression implementation that is
+# reliable and tested.
 #
 # Since: 0.14
 ##
@@ -289,7 +293,7 @@
'*blocked-reasons': ['str'],
'*postcopy-blocktime': 'uint32',
'*postcopy-vcpu-blocktime': ['uint32'],
-   '*compression': 'CompressionStats',
+   '*compression': { 'type': 'CompressionStats', 'features': [ 
'deprecated' ] },
'*socket-address': ['SocketAddress'],
'*dirty-limit-throttle-time-per-round': 'uint64',
'*dirty-limit-ring-full-time': 'uint64'} }
@@ -530,7 +534,10 @@
 # Features:
 #
 # @deprecated: Member @block is deprecated.  Use blockdev-mirror with
-# NBD instead.
+# NBD instead.  Member @compression is deprecated because it is
+# unreliable and untested.  It is recommended to use multifd
+# migration, which offers an alternative compression
+# implementation that is reliable and tested.
 #
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
 #
@@ -538,7 +545,8 @@
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-   'compress', 'events', 'postcopy-ram',
+   { 'name': 'compress', 'features': [ 'deprecated' ] },
+   'events', 'postcopy-ram',
{ 'name': 'x-colo', 'features': [ 'unstable' ] },
'release-ram',
{ 'name': 'block', 'features': [ 'deprecated' ] },
@@ -844,7 +852,9 @@
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated.  Use
-# blockdev-mirror with NBD instead.
+# blockdev-mirror with NBD instead.  Members @compress-level,
+# @compress-threads, @decompress-threads and @compress-wait-thread
+# are deprecated because @compression is deprecated.
 #
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
@@ -854,8 +864,11 @@
 { 'enum': 'MigrationParameter',
   'data': ['announce-initial', 'announce-max',
'announce-rounds', 'announce-step',
-   'compress-level', 'compress-threads', 'decompress-threads',
-   'compress-wait-thread', 'throttle-trigger-threshold',
+   { 'name': 'compress-level', 'features': [ 'deprecated' ] },
+   { 'name': 'compress-threads', 'features': [ 'deprecated' ] },
+   { 'name': 'decompress-threads', 'features': [ 'deprecated' ] },
+   { 'name': 'compress-wait-thread', 'features': [ 'deprecated' ] },
+   'throttle-trigger-threshold',
'cpu-throttle-initial', 'cpu-throttle-increment',
'cpu-throttle-tailslow',
'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
@@ -1023,7 +1036,9 @@
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated.  Use
-# blockdev-mirror with NBD instead.
+# blockdev-mirror with NBD instead.  Members @compress-level,
+# @compress-threads, @decompress-threads and @compress-wait-thread
+# are deprecated because @compression is deprecated.
 #
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
@@ -1038,10 +1053,14 @@
 '*announce-max': 'size',
 '*announce-rounds': 'size',
 '*announce-step': 'size',
-'*compress-level': 'uint8',
-'*compress-threads': 'uint8',
-'*compress-wait-thread': 'bool',
-'*decompress-threads': 'uint8',
+'*compress-level': { 'type': 'uint8',
+ 'features': [ 'deprecated' ] },
+'*compress-threads': 

[PATCH v7 0/4] Migration deprecated parts

[Juan Quintela]

Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com>
  [PULL 00/11] Migration 20231018 patches

And here we are, at v7:
- drop black line at the end of deprecated.rst
- change qemu-iotest output due to warnings for deprecation.

The only real change is the output of the qemu-iotest.  That is the
reason why I maintained the reviewed-by.  But will be happy if anyone
of the block people ack the changes.

Thanks, Juan.

On this v6:
- Fixed Markus comments
- 1st patch is reviewed
- dropped the RFC ones.

Later, Juan.

On this v5:
- Rebased on top of last migration pull requesnt:

- address markus comments.  Basically we recommend always
  blockdev-mirror + NBD.  In deprecated.rst we also put the posiblity
  of using block-incremental and block, but we state that they are
  also deprecated.
  I know, I know, I deprecated them in the following patch.

- Dropped the removal of block-migration and block-incremental I am
  only interested in showing why I want to remove the -b/-i options.

Please review.

Later, Juan.

On this v4:
- addressed all markus comments.
- rebased on latest.
- improve formatting of migration.json
- print block migration status when needed.
- patches 7-10 are not mean to merge, they just show why we want to
  deprecate block migration and remove its support.
- Patch 7 just drop support for -i/-b and qmp equivalents.
- Patch 8 shows all the helpers and convolutions we need to have to
  support that -i and -d.
- patch 9 drops block-incremental migration support.
- patch 9 drops block migration support.

Please review.

Thanks, Juan.

On this v3:

- Rebase on top of upstream.
- Changed v8.1 to 8.2 (I left the reviewed by anyways)
- missing the block deprecation code, please.

Please, review.

Later, Juan.

On this v2:

- dropped -incoming  deprecation
  Paolo came with a better solution using keyvalues.

- skipped field is already ready for next pull request, so dropped.

- dropped the RFC bits, nermal PATCH.

- Assessed all the review comments.

- Added indentation of migration.json.

- Used the documentation pointer to substitute block migration.

Please review.

[v1]
Hi this series describe the migration parts that have to be deprecated.

- It is an rfc because I doubt that I did the deprecation process right. Hello 
Markus O:-)

- skipped field: It is older than me, I have never know what it stands
  for.  As far as I know it has always been zero.

- inc/blk migrate command options.  They are only used by block
  migration (that I deprecate on the following patch).  And they are really bad.
  grep must_remove_block_options.

- block migration.  block jobs, whatever they are called this week are
  way more flexible.  Current code works, but we broke it here and
  there, and really nobody has stand up to maintain it.  It is quite
  contained and can be left there.  Is anyone really using it?

- old compression method.  It don't work.  See last try from Lukas to
  make a test that works reliabely.  I failed with the same task years
  ago.  It is really slow, and if compression is good for you, multifd
  + zlib is going to perform/compress way more.

  I don't know what to do with this code, really.

  * Remove it for this release?  It don't work, and haven't work
reliabely in quite a few time.

  * Deprecate it and remove in another couple of releases, i.e. normal
deprecation.

  * Ideas?

- -incoming 

  if you need to set parameters (multifd cames to mind, and preempt has
  the same problem), you really needs to use defer.  So what should we do here?

  This part is not urget, because management apps have a working
  option that are already using "defer", and the code simplifacation
  if we remove it is not so big.  So we can leave it until 9.0 or
  whatever we think fit.

What do you think?

Later, Juan.

Juan Quintela (4):
  migration: migrate 'inc' command option is deprecated.
  migration: migrate 'blk' command option is deprecated.
  migration: Deprecate block migration
  migration: Deprecate old compression method

 docs/about/deprecated.rst  | 35 +
 qapi/migration.json| 93 ++
 migration/block.c  |  3 ++
 migration/migration-hmp-cmds.c | 10 
 migration/migration.c  | 10 
 migration/options.c| 22 +++-
 tests/qemu-iotests/183.out |  2 +
 7 files changed, 152 insertions(+), 23 deletions(-)

-- 
2.41.0

[PATCH v7 3/4] migration: Deprecate block migration

[Juan Quintela]

It is obsolete.  It is better to use driver-mirror with NBD instead.

CC: Kevin Wolf 
CC: Eric Blake 
CC: Stefan Hajnoczi 
CC: Hanna Czenczek 

Acked-by: Stefan Hajnoczi 
Reviewed-by: Markus Armbruster 
Signed-off-by: Juan Quintela 
---
 docs/about/deprecated.rst  | 10 ++
 qapi/migration.json| 29 -
 migration/block.c  |  3 +++
 migration/options.c|  9 -
 tests/qemu-iotests/183.out |  1 +
 5 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 98b0f14e69..7ae872162d 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -478,3 +478,13 @@ Use blockdev-mirror with NBD instead.
 As an intermediate step the ``blk`` functionality can be achieved by
 setting the ``block`` migration capability to ``true``.  But this
 capability is also deprecated.
+
+block migration (since 8.2)
+'''
+
+Block migration is too inflexible.  It needs to migrate all block
+devices or none.
+
+Please see "QMP invocation for live storage migration with
+``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst
+for a detailed explanation.
diff --git a/qapi/migration.json b/qapi/migration.json
index 3765c2b662..e3b00a215b 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -269,11 +269,15 @@
 # average memory load of the virtual CPU indirectly.  Note that
 # zero means guest doesn't dirty memory.  (Since 8.1)
 #
+# Features:
+#
+# @deprecated: Member @disk is deprecated because block migration is.
+#
 # Since: 0.14
 ##
 { 'struct': 'MigrationInfo',
   'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
-   '*disk': 'MigrationStats',
+   '*disk': { 'type': 'MigrationStats', 'features': [ 'deprecated' ] },
'*vfio': 'VfioStats',
'*xbzrle-cache': 'XBZRLECacheStats',
'*total-time': 'int',
@@ -525,6 +529,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block is deprecated.  Use blockdev-mirror with
+# NBD instead.
+#
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
 #
 # Since: 1.2
@@ -534,7 +541,8 @@
'compress', 'events', 'postcopy-ram',
{ 'name': 'x-colo', 'features': [ 'unstable' ] },
'release-ram',
-   'block', 'return-path', 'pause-before-switchover', 'multifd',
+   { 'name': 'block', 'features': [ 'deprecated' ] },
+   'return-path', 'pause-before-switchover', 'multifd',
'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
{ 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
'validate-uuid', 'background-snapshot',
@@ -835,6 +843,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block-incremental is deprecated.  Use
+# blockdev-mirror with NBD instead.
+#
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
 #
@@ -850,7 +861,7 @@
'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
'avail-switchover-bandwidth', 'downtime-limit',
{ 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
-   'block-incremental',
+   { 'name': 'block-incremental', 'features': [ 'deprecated' ] },
'multifd-channels',
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
@@ -1011,6 +1022,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block-incremental is deprecated.  Use
+# blockdev-mirror with NBD instead.
+#
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
 #
@@ -1040,7 +1054,8 @@
 '*downtime-limit': 'uint64',
 '*x-checkpoint-delay': { 'type': 'uint32',
  'features': [ 'unstable' ] },
-'*block-incremental': 'bool',
+'*block-incremental': { 'type': 'bool',
+'features': [ 'deprecated' ] },
 '*multifd-channels': 'uint8',
 '*xbzrle-cache-size': 'size',
 '*max-postcopy-bandwidth': 'size',
@@ -1225,6 +1240,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block-incremental is deprecated.  Use
+# blockdev-mirror with NBD instead.
+#
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
 #
@@ -1251,7 +1269,8 @@
 '*downtime-limit': 'uint64',
 '*x-checkpoint-delay': { 'type': 'uint32',
  'features': [ 'unstable' ] },
-'*block-incremental': 'bool',
+'*block-incremental': { 'type': 'bool',
+'features': [ 'deprecated' ] },
 '*multifd-channels': 'uint8',
 '*xbzrle-cache-size': 'size',
 '*max-postcopy-bandwidth': 'size',
diff --git a/migration/block.c b/migration/block.c
index b60698d6e2..acff

[PATCH v7 2/4] migration: migrate 'blk' command option is deprecated.

[Juan Quintela]

Use blocked-mirror with NBD instead.

Acked-by: Stefan Hajnoczi 
Reviewed-by: Thomas Huth 
Reviewed-by: Markus Armbruster 
Signed-off-by: Juan Quintela 
---
 docs/about/deprecated.rst  | 9 +
 qapi/migration.json| 7 ---
 migration/migration-hmp-cmds.c | 5 +
 migration/migration.c  | 5 +
 tests/qemu-iotests/183.out | 1 +
 5 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index fc6adf1dea..98b0f14e69 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -469,3 +469,12 @@ Use blockdev-mirror with NBD instead.
 As an intermediate step the ``inc`` functionality can be achieved by
 setting the ``block-incremental`` migration parameter to ``true``.
 But this parameter is also deprecated.
+
+``blk`` migrate command option (since 8.2)
+''
+
+Use blockdev-mirror with NBD instead.
+
+As an intermediate step the ``blk`` functionality can be achieved by
+setting the ``block`` migration capability to ``true``.  But this
+capability is also deprecated.
diff --git a/qapi/migration.json b/qapi/migration.json
index fa7f4f2575..3765c2b662 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1526,8 +1526,8 @@
 #
 # Features:
 #
-# @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
-# NBD instead.
+# @deprecated: Members @inc and @blk are deprecated.  Use
+# blockdev-mirror with NBD instead.
 #
 # Returns: nothing on success
 #
@@ -1550,7 +1550,8 @@
 # <- { "return": {} }
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool',
+  'data': {'uri': 'str',
+   '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
'*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
'*detach': 'bool', '*resume': 'bool' } }
 
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 83176f5bae..dfe98da355 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -750,6 +750,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 " use blockdev-mirror with NBD instead");
 }
 
+if (blk) {
+warn_report("option '-b' is deprecated;"
+" use blockdev-mirror with NBD instead");
+}
+
 qmp_migrate(uri, !!blk, blk, !!inc, inc,
 false, false, true, resume, &err);
 if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index 27145cd99e..79b742b98b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1609,6 +1609,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, 
bool blk_inc,
 " use blockdev-mirror with NBD instead");
 }
 
+if (blk) {
+warn_report("parameter 'blk' is deprecated;"
+" use blockdev-mirror with NBD instead");
+}
+
 if (resume) {
 if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
 error_setg(errp, "Cannot resume if there is no "
diff --git a/tests/qemu-iotests/183.out b/tests/qemu-iotests/183.out
index fd9c2e52a5..b49f491854 100644
--- a/tests/qemu-iotests/183.out
+++ b/tests/qemu-iotests/183.out
@@ -28,6 +28,7 @@ read 65536/65536 bytes at offset 0
 
 { 'execute': 'migrate',
'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } }
+warning: parameter 'blk' is deprecated; use blockdev-mirror with NBD instead
 {"return": {}}
 { 'execute': 'query-status' }
 {"return": {"status": "postmigrate", "singlestep": false, "running": false}}
-- 
2.41.0

[PATCH v7 1/4] migration: migrate 'inc' command option is deprecated.

[Juan Quintela]

Use blockdev-mirror with NBD instead.

Reviewed-by: Thomas Huth 
Acked-by: Stefan Hajnoczi 
Reviewed-by: Markus Armbruster 
Signed-off-by: Juan Quintela 
---
 docs/about/deprecated.rst  | 8 
 qapi/migration.json| 8 +++-
 migration/migration-hmp-cmds.c | 5 +
 migration/migration.c  | 5 +
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 2febd2d12f..fc6adf1dea 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -461,3 +461,11 @@ Migration
 ``skipped`` field in Migration stats has been deprecated.  It hasn't
 been used for more than 10 years.
 
+``inc`` migrate command option (since 8.2)
+''
+
+Use blockdev-mirror with NBD instead.
+
+As an intermediate step the ``inc`` functionality can be achieved by
+setting the ``block-incremental`` migration parameter to ``true``.
+But this parameter is also deprecated.
diff --git a/qapi/migration.json b/qapi/migration.json
index db3df12d6c..fa7f4f2575 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1524,6 +1524,11 @@
 #
 # @resume: resume one paused migration, default "off". (since 3.0)
 #
+# Features:
+#
+# @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
+# NBD instead.
+#
 # Returns: nothing on success
 #
 # Since: 0.14
@@ -1545,7 +1550,8 @@
 # <- { "return": {} }
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
+  'data': {'uri': 'str', '*blk': 'bool',
+   '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
'*detach': 'bool', '*resume': 'bool' } }
 
 ##
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index a82597f18e..83176f5bae 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -745,6 +745,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 const char *uri = qdict_get_str(qdict, "uri");
 Error *err = NULL;
 
+if (inc) {
+warn_report("option '-i' is deprecated;"
+" use blockdev-mirror with NBD instead");
+}
+
 qmp_migrate(uri, !!blk, blk, !!inc, inc,
 false, false, true, resume, &err);
 if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index 05c0b801ba..27145cd99e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1604,6 +1604,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, 
bool blk_inc,
 {
 Error *local_err = NULL;
 
+if (blk_inc) {
+warn_report("parameter 'inc' is deprecated;"
+" use blockdev-mirror with NBD instead");
+}
+
 if (resume) {
 if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
 error_setg(errp, "Cannot resume if there is no "
-- 
2.41.0

Re: [PATCH 4/4] qapi: introduce CONFIG_READ event

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> On 18.10.23 09:47, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>> 
>>> On 17.10.23 18:00, Markus Armbruster wrote:
 Vladimir Sementsov-Ogievskiy  writes:

> Send a new event when guest reads virtio-pci config after
> virtio_notify_config() call.
>
> That's useful to check that guest fetched modified config, for example
> after resizing disk backend.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> [...]
>> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2468f8bddf..37a8785b81 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -329,3 +329,25 @@
># Since: 8.2
>##
>{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} }
> +
> +##
> +# @X_CONFIG_READ:
> +#
> +# Emitted whenever guest reads virtio device config after config change.
> +#
> +# @device: device name
> +#
> +# @path: device path
> +#
> +# Since: 5.0.1-24
> +#
> +# Example:
> +#
> +# <- { "event": "X_CONFIG_READ",
> +#  "data": { "device": "virtio-net-pci-0",
> +#"path": "/machine/peripheral/virtio-net-pci-0" },
> +#  "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'X_CONFIG_READ',
> +  'data': { '*device': 'str', 'path': 'str' } }

 The commit message talks about event CONFIG_READ, but you actually name
 it x-device-sync-config.
>>>
>>> will fix
>>>
 I figure you use x- to signify "unstable".  Please use feature flag
 'unstable' for that.  See docs/devel/qapi-code-gen.rst section
 "Features", in particular "Special features", and also the note on x- in
 section "Naming rules and reserved names".
>>>
>>> OK, will do.
>>>
>>> Hmm, it say
>>>
>>> Names beginning with ``x-`` used to signify "experimental".  This
>>> convention has been replaced by special feature "unstable".
>>>
>>> "replaced".. So, I should use "unstable" flag without "x-" prefix? Can't 
>>> find an example. Seems "unstable" always used together with "x-".
>>
>> True.
>>
>> The "x-" prefix originated with qdev properties.  First use might be
>> commit f0c07c7c7b4.  The convention wasn't documented then, but QOM/qdev
>> properties have always been a documentation wasteland.  It then spread
>> to other places, and eventually to the QAPI schema.  Where we try pretty
>> hard to document things properly.  We documented the "x-" prefix in
>> commit e790e666518:
>>
>>  Any name (command, event, type, field, or enum value) beginning with
>>  "x-" is marked experimental, and may be withdrawn or changed
>>  incompatibly in a future release.
>>
>> Minor pain point: when things grow up from experimental to stable, we
>> have to rename.
>>
>> The convention didn't stop us from naming non-experimental things x-FOO,
>> e.g. QOM property "x-origin" in commit 6105683da35.  Made it to the QAPI
>> schema in commit 8825587b53c.  Point is: the prefix isn't a reliable
>> marker for "unstable".
>>
>> Since I needed a reliable marker for my "set policy for unstable
>> interfaces" feature (see CLI option -compat), I created special feature
>> flag "unstable", and dropped the "x-" convention for the QAPI schema.
>>
>> Renaming existing "x-" names felt like pointless churn, so I didn't.
>>
>> I'm not objecting to new names starting with "x-".  Nor am I objecting
>> to feature 'unstable' on names that don't start with "x-".
>>
>> I guess "x-" remains just fine for things we don't intend to make stable
>> at some point.  The "x-" can remind humans "this is unstable" better
>> than a feature flag can (for machines, it's the other way round).
>>
>> For things we do intend (hope?) to make stable, I wouldn't bother with
>> the "x-".
>>
>> Clearer now?
>
> Yes, thanks.
>
> x- seems safer for management tool that doesn't know about "unstable" 
> properties..

Easy, traditional, and unreliable :)

> But on the other hand, changing from x- to no-prefix is already done when the 
> feature is stable, and thouse who use it already use the latest version of 
> interface, so, removing the prefix is just extra work.

Exactly.

> So, I think, I'd go without prefix.

Makes sense.

>>> Also, nothing said about events. Is using "X_" wrong idea? Should it be 
>>> x-SOME_EVENT instead?
>>
>> Since this is the first unstable event, there is no precedent.  Let's
>> use no prefix, and move on.
>> 
 The name CONFIG_READ feels overly generic for something that makes sense
 only with virtio devices.
>>>
>>> Hmm, right.. I think, we can say same thing about DEVICE_UNPLUG_GUEST_ERROR.
>>
>> That one came to be as a generalization of existing MEM_UNPLUG_ERROR and
>> a concrete need to signal CPU unplug errors.  Demonstrates "unplug guest
>> errors" can happen for different kinds of devices.  So we went with a
>> generic event we can use for all of them.
>> This doesn't seem

Re: [PATCH v7 0/4] Migration deprecated parts

[Juan Quintela]

Juan Quintela  wrote:
> Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com>
>   [PULL 00/11] Migration 20231018 patches
>
> And here we are, at v7:
> - drop black line at the end of deprecated.rst
> - change qemu-iotest output due to warnings for deprecation.
>
> The only real change is the output of the qemu-iotest.  That is the
> reason why I maintained the reviewed-by.  But will be happy if anyone
> of the block people ack the changes.

I forgot to include the link to the CI of the previous failure.

https://gitlab.com/juan.quintela/qemu/-/jobs/5314070229

tput mismatch (see 
/builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad)
--- /builds/juan.quintela/qemu/tests/qemu-iotests/183.out
+++ 
/builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad
@@ -28,6 +28,8 @@
 { 'execute': 'migrate',
'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } }
+warning: parameter 'blk is deprecated; use blockdev-mirror with NBD instead
+warning: block migration is deprecated; use blockdev-mirror with NBD instead
 {"return": {}}
 { 'execute': 'query-status' }
 {"return": {"status": "postmigrate", "singlestep": false, "running":
 false}}



>
> Thanks, Juan.
>
> On this v6:
> - Fixed Markus comments
> - 1st patch is reviewed
> - dropped the RFC ones.
>
> Later, Juan.
>
> On this v5:
> - Rebased on top of last migration pull requesnt:
>
> - address markus comments.  Basically we recommend always
>   blockdev-mirror + NBD.  In deprecated.rst we also put the posiblity
>   of using block-incremental and block, but we state that they are
>   also deprecated.
>   I know, I know, I deprecated them in the following patch.
>
> - Dropped the removal of block-migration and block-incremental I am
>   only interested in showing why I want to remove the -b/-i options.
>
> Please review.
>
> Later, Juan.
>
> On this v4:
> - addressed all markus comments.
> - rebased on latest.
> - improve formatting of migration.json
> - print block migration status when needed.
> - patches 7-10 are not mean to merge, they just show why we want to
>   deprecate block migration and remove its support.
> - Patch 7 just drop support for -i/-b and qmp equivalents.
> - Patch 8 shows all the helpers and convolutions we need to have to
>   support that -i and -d.
> - patch 9 drops block-incremental migration support.
> - patch 9 drops block migration support.
>
> Please review.
>
> Thanks, Juan.
>
> On this v3:
>
> - Rebase on top of upstream.
> - Changed v8.1 to 8.2 (I left the reviewed by anyways)
> - missing the block deprecation code, please.
>
> Please, review.
>
> Later, Juan.
>
> On this v2:
>
> - dropped -incoming  deprecation
>   Paolo came with a better solution using keyvalues.
>
> - skipped field is already ready for next pull request, so dropped.
>
> - dropped the RFC bits, nermal PATCH.
>
> - Assessed all the review comments.
>
> - Added indentation of migration.json.
>
> - Used the documentation pointer to substitute block migration.
>
> Please review.
>
> [v1]
> Hi this series describe the migration parts that have to be deprecated.
>
> - It is an rfc because I doubt that I did the deprecation process right. 
> Hello Markus O:-)
>
> - skipped field: It is older than me, I have never know what it stands
>   for.  As far as I know it has always been zero.
>
> - inc/blk migrate command options.  They are only used by block
>   migration (that I deprecate on the following patch).  And they are really 
> bad.
>   grep must_remove_block_options.
>
> - block migration.  block jobs, whatever they are called this week are
>   way more flexible.  Current code works, but we broke it here and
>   there, and really nobody has stand up to maintain it.  It is quite
>   contained and can be left there.  Is anyone really using it?
>
> - old compression method.  It don't work.  See last try from Lukas to
>   make a test that works reliabely.  I failed with the same task years
>   ago.  It is really slow, and if compression is good for you, multifd
>   + zlib is going to perform/compress way more.
>
>   I don't know what to do with this code, really.
>
>   * Remove it for this release?  It don't work, and haven't work
> reliabely in quite a few time.
>
>   * Deprecate it and remove in another couple of releases, i.e. normal
> deprecation.
>
>   * Ideas?
>
> - -incoming 
>
>   if you need to set parameters (multifd cames to mind, and pree

Re: [PATCH v8 4/7] qapi: add x-blockdev-replace command

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> Add a command that can replace bs in following BdrvChild structures:
>
>  - qdev blk root child
>  - block-export blk root child
>  - any child of BlockDriverState selected by child-name
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 89751d81f2..cf92e0df8b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -6054,3 +6054,86 @@
>  ##
>  { 'struct': 'DummyBlockCoreForceArrays',
>'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
> +
> +##
> +# @BlockParentType:
> +#
> +# @qdev: block device, such as created by device_add, and denoted by
> +# qdev-id
> +#
> +# @driver: block driver node, such as created by blockdev-add, and
> +# denoted by node-name
> +#
> +# @export: block export, such created by block-export-add, and
> +# denoted by export-id
> +#
> +# Since 8.2
> +##
> +{ 'enum': 'BlockParentType',
> +  'data': ['qdev', 'driver', 'export'] }
> +
> +##
> +# @BdrvChildRefQdev:
> +#
> +# @qdev-id: the device's ID or QOM path
> +#
> +# Since 8.2
> +##
> +{ 'struct': 'BdrvChildRefQdev',
> +  'data': { 'qdev-id': 'str' } }
> +
> +##
> +# @BdrvChildRefExport:
> +#
> +# @export-id: block export identifier
> +#
> +# Since 8.2
> +##
> +{ 'struct': 'BdrvChildRefExport',
> +  'data': { 'export-id': 'str' } }
> +
> +##
> +# @BdrvChildRefDriver:
> +#
> +# @node-name: the node name of the parent block node
> +#
> +# @child: name of the child to be replaced, like "file" or "backing"
> +#
> +# Since 8.2
> +##
> +{ 'struct': 'BdrvChildRefDriver',
> +  'data': { 'node-name': 'str', 'child': 'str' } }
> +
> +##
> +# @BlockdevReplace:
> +#
> +# @parent-type: type of the parent, which child is to be replaced
> +#
> +# @new-child: new child for replacement
> +#
> +# Since 8.2
> +##
> +{ 'union': 'BlockdevReplace',
> +  'base': {
> +  'parent-type': 'BlockParentType',
> +  'new-child': 'str'
> +  },
> +  'discriminator': 'parent-type',
> +  'data': {
> +  'qdev': 'BdrvChildRefQdev',
> +  'export': 'BdrvChildRefExport',
> +  'driver': 'BdrvChildRefDriver'
> +  } }
> +
> +##
> +# @x-blockdev-replace:
> +#
> +# Replace a block-node associated with device (selected by
> +# @qdev-id) or with block-export (selected by @export-id) or
> +# any child of block-node (selected by @node-name and @child)
> +# with @new-child block-node.
> +#
> +# Since 8.2
> +##
> +{ 'command': 'x-blockdev-replace', 'boxed': true,
> +  'data': 'BlockdevReplace' }
> diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c
> new file mode 100644
> index 00..0e751ce4f7
> --- /dev/null
> +++ b/stubs/blk-by-qdev-id.c
> @@ -0,0 +1,9 @@
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "sysemu/block-backend.h"
> +
> +BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
> +{
> +error_setg(errp, "blk '%s' not found", id);

Is this expected to happen?

> +return NULL;
> +}

[...]

QAPI schema
Acked-by: Markus Armbruster 

Re: [PATCH 4/4] qapi: introduce CONFIG_READ event

[Michael S. Tsirkin]

On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
> > x- seems safer for management tool that doesn't know about "unstable" 
> > properties..
> 
> Easy, traditional, and unreliable :)

> > But on the other hand, changing from x- to no-prefix is already done when 
> > the feature is stable, and thouse who use it already use the latest version 
> > of interface, so, removing the prefix is just extra work.
> 
> Exactly.
> 

I think "x-" is still better for command line use of properties - we
don't have an API to mark things unstable there, do we?

-- 
MST

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

On Wed, 2023-10-18 at 10:52 +0200, Kevin Wolf wrote:
> Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> > From: David Woodhouse 
> > 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Signed-off-by: David Woodhouse 
> 
> Actually, how does this play together with xen_config_dev_blk()? This
> looks like it tried to implement a very similar thing (which is IF_XEN
> even already existed).

Ah yes, thanks for spotting that! I hadn't been looking at the xenfv

> Are we now trying to attach each if=xen disk twice in the 'xenpv'
> machine? Or if something prevents this, is it dead code.

I suspect we end up creating them twice (and probably thus failing to
lock the backing file).

The old Xen PV device drivers in Qemu, before Paul's XenDevice
conversion, were purely reactive. If they see nodes in XenStore
describing a backend which they should implement, they'd do so.

The code in xen_config_dev_blk() is *creating* those nodes for the
frontend (guest) and backend (host/qemu) to see, which prompts the
xen-block and xen-net drivers into action.

In the new XenDevice model, we can just instantiate a device (e.g.
qdev_new(TYPE_XEN_DISK_DEVICE) and its realize() method will create the
corresponding XenStore nodes (with some help from the generic XenBus
code for the common ones).

The new XenDevice/XenBus model will *also* react to nodes which it
discovers in XenStore, and instantiate the corresponding devices.

So I suspect what'll happen is xen_config_dev_blk() will create the
nodes starting at xvda (int vdev = 202 * 256 + 16 * disk->unit), and
later my new code will create a new set starting from xvdb or wherever
the next free one is.

> I think in both cases, we would want to delete that function and remove
> the loop that calls it in xen_init_pv()?

Yes, I think so. The xen_config_dev_blk() one can just die, as can
xen_config_dev_console() which isn't called from anywhere anyway.

The vkbd/vfb ones can stay until/unless those drivers are converted to
the new XenDevice model.

And xen_config_dev_nic() probably just needs to loop doing the same as
I did in pc_init_nic() in
https://lore.kernel.org/qemu-devel/20231017182545.97973-5-dw...@infradead.org/T/#u

+if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
+DeviceState *dev = qdev_new("xen-net-device");
+qdev_set_nic_properties(dev, nd);
+qdev_realize_and_unref(dev, xen_bus, &error_fatal);


... but this just reinforces what I said there about "if
qmp_device_add() can find the damn bus and do this right, why do we
have to litter it through platform code?"


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v7 0/4] Migration deprecated parts

[Daniel P . Berrangé]

On Wed, Oct 18, 2023 at 12:38:10PM +0200, Juan Quintela wrote:
> Juan Quintela  wrote:
> > Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com>
> >   [PULL 00/11] Migration 20231018 patches
> >
> > And here we are, at v7:
> > - drop black line at the end of deprecated.rst
> > - change qemu-iotest output due to warnings for deprecation.
> >
> > The only real change is the output of the qemu-iotest.  That is the
> > reason why I maintained the reviewed-by.  But will be happy if anyone
> > of the block people ack the changes.
> 
> I forgot to include the link to the CI of the previous failure.
> 
> https://gitlab.com/juan.quintela/qemu/-/jobs/5314070229
> 
> tput mismatch (see 
> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad)
> --- /builds/juan.quintela/qemu/tests/qemu-iotests/183.out
> +++ 
> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad
> @@ -28,6 +28,8 @@
>  { 'execute': 'migrate',
> 'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } }
> +warning: parameter 'blk is deprecated; use blockdev-mirror with NBD instead
> +warning: block migration is deprecated; use blockdev-mirror with NBD instead
>  {"return": {}}
>  { 'execute': 'query-status' }
>  {"return": {"status": "postmigrate", "singlestep": false, "running":
>  false}}

IIUC, the JSON bits are being written on stdout, and the warnings
are being written on stderr. The interleaving of the data is
potentially going to be non-deterministic in the .out file.
Generally you'd want a filter in the iotests that culls the
warning: lines to avoid this mixing of stdout/err streams.


With 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 4/4] qapi: introduce CONFIG_READ event

[Daniel P . Berrangé]

On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
> > > x- seems safer for management tool that doesn't know about "unstable" 
> > > properties..
> > 
> > Easy, traditional, and unreliable :)
> 
> > > But on the other hand, changing from x- to no-prefix is already
> > > done when the feature is stable, and thouse who use it already
> > > use the latest version of interface, so, removing the prefix is
> > > just extra work.
> > 
> > Exactly.
> > 
> 
> I think "x-" is still better for command line use of properties - we
> don't have an API to mark things unstable there, do we?

Personally I like to see "x-" prefix present *everywhere* there is
an unstable feature, and consider the need to rename when declaring
it stable to be good thing as it sets an easily identifiable line
in the sand and is self-evident to outside observers.

The self-documenting nature of the "x-" prefer is what makes it most
compelling to me. A patch submission, or command line invokation or
an example QMP command, or a bug report, that exhibit an 'x-' prefix
are an immediate red flag to anyone who sees them.

If someone sees a QMP comamnd / a typical giant QEMU command line,
they are never going to go look at the QAPI schema to check whether
any feature used had an 'unstable' marker. The 'unstable' marker
might as well not exist in most cases.

IOW, having the 'unstable' flag in the QAPI schema is great for machine
introspection, but it isn't a substitute for having an 'x-' prefix used
for the benefit of humans IMHO.

With 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 v7 0/4] Migration deprecated parts

[Juan Quintela]

Daniel P. Berrangé  wrote:
> On Wed, Oct 18, 2023 at 12:38:10PM +0200, Juan Quintela wrote:
>> Juan Quintela  wrote:
>> > Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com>
>> >   [PULL 00/11] Migration 20231018 patches
>> >
>> > And here we are, at v7:
>> > - drop black line at the end of deprecated.rst
>> > - change qemu-iotest output due to warnings for deprecation.
>> >
>> > The only real change is the output of the qemu-iotest.  That is the
>> > reason why I maintained the reviewed-by.  But will be happy if anyone
>> > of the block people ack the changes.
>> 
>> I forgot to include the link to the CI of the previous failure.
>> 
>> https://gitlab.com/juan.quintela/qemu/-/jobs/5314070229
>> 
>> tput mismatch (see 
>> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad)
>> --- /builds/juan.quintela/qemu/tests/qemu-iotests/183.out
>> +++ 
>> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad
>> @@ -28,6 +28,8 @@
>>  { 'execute': 'migrate',
>> 'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } }
>> +warning: parameter 'blk is deprecated; use blockdev-mirror with NBD instead
>> +warning: block migration is deprecated; use blockdev-mirror with NBD instead
>>  {"return": {}}
>>  { 'execute': 'query-status' }
>>  {"return": {"status": "postmigrate", "singlestep": false, "running":
>>  false}}
>
> IIUC, the JSON bits are being written on stdout, and the warnings
> are being written on stderr. The interleaving of the data is
> potentially going to be non-deterministic in the .out file.
> Generally you'd want a filter in the iotests that culls the
> warning: lines to avoid this mixing of stdout/err streams.

Thanks.

So here I am, going to v8 to create filters.

Later, Juan.

[PATCH v8 0/5] Migration deprecated parts

[Juan Quintela]

Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com>
  [PULL 00/11] Migration 20231018 patches

v8 now:
- drop changes to 183.out
- create _filter_migration_block_deprecated

And now it passes the iotests.

Please review patch 1.
Rest are unchanged from v6.

Later, Juan.

And here we are, at v7:
- drop black line at the end of deprecated.rst
- change qemu-iotest output due to warnings for deprecation.

The only real change is the output of the qemu-iotest.  That is the
reason why I maintained the reviewed-by.  But will be happy if anyone
of the block people ack the changes.

Thanks, Juan.

On this v6:
- Fixed Markus comments
- 1st patch is reviewed
- dropped the RFC ones.

Later, Juan.

On this v5:
- Rebased on top of last migration pull requesnt:

- address markus comments.  Basically we recommend always
  blockdev-mirror + NBD.  In deprecated.rst we also put the posiblity
  of using block-incremental and block, but we state that they are
  also deprecated.
  I know, I know, I deprecated them in the following patch.

- Dropped the removal of block-migration and block-incremental I am
  only interested in showing why I want to remove the -b/-i options.

Please review.

Later, Juan.

On this v4:
- addressed all markus comments.
- rebased on latest.
- improve formatting of migration.json
- print block migration status when needed.
- patches 7-10 are not mean to merge, they just show why we want to
  deprecate block migration and remove its support.
- Patch 7 just drop support for -i/-b and qmp equivalents.
- Patch 8 shows all the helpers and convolutions we need to have to
  support that -i and -d.
- patch 9 drops block-incremental migration support.
- patch 9 drops block migration support.

Please review.

Thanks, Juan.

On this v3:

- Rebase on top of upstream.
- Changed v8.1 to 8.2 (I left the reviewed by anyways)
- missing the block deprecation code, please.

Please, review.

Later, Juan.

On this v2:

- dropped -incoming  deprecation
  Paolo came with a better solution using keyvalues.

- skipped field is already ready for next pull request, so dropped.

- dropped the RFC bits, nermal PATCH.

- Assessed all the review comments.

- Added indentation of migration.json.

- Used the documentation pointer to substitute block migration.

Please review.

[v1]
Hi this series describe the migration parts that have to be deprecated.

- It is an rfc because I doubt that I did the deprecation process right. Hello 
Markus O:-)

- skipped field: It is older than me, I have never know what it stands
  for.  As far as I know it has always been zero.

- inc/blk migrate command options.  They are only used by block
  migration (that I deprecate on the following patch).  And they are really bad.
  grep must_remove_block_options.

- block migration.  block jobs, whatever they are called this week are
  way more flexible.  Current code works, but we broke it here and
  there, and really nobody has stand up to maintain it.  It is quite
  contained and can be left there.  Is anyone really using it?

- old compression method.  It don't work.  See last try from Lukas to
  make a test that works reliabely.  I failed with the same task years
  ago.  It is really slow, and if compression is good for you, multifd
  + zlib is going to perform/compress way more.

  I don't know what to do with this code, really.

  * Remove it for this release?  It don't work, and haven't work
reliabely in quite a few time.

  * Deprecate it and remove in another couple of releases, i.e. normal
deprecation.

  * Ideas?

- -incoming 

  if you need to set parameters (multifd cames to mind, and preempt has
  the same problem), you really needs to use defer.  So what should we do here?

  This part is not urget, because management apps have a working
  option that are already using "defer", and the code simplifacation
  if we remove it is not so big.  So we can leave it until 9.0 or
  whatever we think fit.

What do you think?

Later, Juan.

Juan Quintela (5):
  qemu-iotests: Filter warnings about block migration being deprecated
  migration: migrate 'inc' command option is deprecated.
  migration: migrate 'blk' command option is deprecated.
  migration: Deprecate block migration
  migration: Deprecate old compression method

 docs/about/deprecated.rst| 35 
 qapi/migration.json  | 93 
 migration/block.c|  3 ++
 migration/migration-hmp-cmds.c   | 10 
 migration/migration.c| 10 
 migration/options.c  | 22 +++-
 tests/qemu-iotests/183   |  2 +-
 tests/qemu-iotests/common.filter |  7 +++
 8 files changed, 158 insertions(+), 24 deletions(-)

-- 
2.41.0

[PATCH v8 3/5] migration: migrate 'blk' command option is deprecated.

[Juan Quintela]

Use blocked-mirror with NBD instead.

Acked-by: Stefan Hajnoczi 
Reviewed-by: Thomas Huth 
Reviewed-by: Markus Armbruster 
Signed-off-by: Juan Quintela 
---
 docs/about/deprecated.rst  | 9 +
 qapi/migration.json| 7 ---
 migration/migration-hmp-cmds.c | 5 +
 migration/migration.c  | 5 +
 4 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index fc6adf1dea..98b0f14e69 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -469,3 +469,12 @@ Use blockdev-mirror with NBD instead.
 As an intermediate step the ``inc`` functionality can be achieved by
 setting the ``block-incremental`` migration parameter to ``true``.
 But this parameter is also deprecated.
+
+``blk`` migrate command option (since 8.2)
+''
+
+Use blockdev-mirror with NBD instead.
+
+As an intermediate step the ``blk`` functionality can be achieved by
+setting the ``block`` migration capability to ``true``.  But this
+capability is also deprecated.
diff --git a/qapi/migration.json b/qapi/migration.json
index fa7f4f2575..3765c2b662 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1526,8 +1526,8 @@
 #
 # Features:
 #
-# @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
-# NBD instead.
+# @deprecated: Members @inc and @blk are deprecated.  Use
+# blockdev-mirror with NBD instead.
 #
 # Returns: nothing on success
 #
@@ -1550,7 +1550,8 @@
 # <- { "return": {} }
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool',
+  'data': {'uri': 'str',
+   '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
'*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
'*detach': 'bool', '*resume': 'bool' } }
 
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 83176f5bae..dfe98da355 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -750,6 +750,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 " use blockdev-mirror with NBD instead");
 }
 
+if (blk) {
+warn_report("option '-b' is deprecated;"
+" use blockdev-mirror with NBD instead");
+}
+
 qmp_migrate(uri, !!blk, blk, !!inc, inc,
 false, false, true, resume, &err);
 if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index 27145cd99e..79b742b98b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1609,6 +1609,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, 
bool blk_inc,
 " use blockdev-mirror with NBD instead");
 }
 
+if (blk) {
+warn_report("parameter 'blk' is deprecated;"
+" use blockdev-mirror with NBD instead");
+}
+
 if (resume) {
 if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
 error_setg(errp, "Cannot resume if there is no "
-- 
2.41.0

[PATCH v8 1/5] qemu-iotests: Filter warnings about block migration being deprecated

[Juan Quintela]

Create a new filter that removes the two warnings for test 183.

Signed-off-by: Juan Quintela 
---
 tests/qemu-iotests/183   | 2 +-
 tests/qemu-iotests/common.filter | 7 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
index ee62939e72..b85770458e 100755
--- a/tests/qemu-iotests/183
+++ b/tests/qemu-iotests/183
@@ -90,7 +90,7 @@ echo
 reply="$(_send_qemu_cmd $src \
 "{ 'execute': 'migrate',
'arguments': { 'uri': 'unix:${MIG_SOCKET}', 'blk': true } }" \
-'return\|error')"
+'return\|error' | _filter_migration_block_deprecated)"
 echo "$reply"
 if echo "$reply" | grep "compiled without old-style" > /dev/null; then
 _notrun "migrate -b support not compiled in"
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index fc3c64bcb8..2846c83808 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -359,5 +359,12 @@ _filter_qcow2_compression_type_bit()
 -e 's/\(incompatible_features.*\), 3\(,.*\)/\1\2/'
 }
 
+# filter warnings caused for block migration deprecation
+_filter_migration_block_deprecated()
+{
+gsed -e '/warning: parameter .blk. is deprecated; use blockdev-mirror with 
NBD instead/d' \
+ -e '/warning: block migration is deprecated; use blockdev-mirror with 
NBD instead/d'
+}
+
 # make sure this script returns success
 true
-- 
2.41.0

[PATCH v8 5/5] migration: Deprecate old compression method

[Juan Quintela]

Acked-by: Stefan Hajnoczi 
Acked-by: Peter Xu 
Reviewed-by: Markus Armbruster 
Signed-off-by: Juan Quintela 
---
 docs/about/deprecated.rst |  8 +
 qapi/migration.json   | 63 ++-
 migration/options.c   | 13 
 3 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 7ae872162d..e7f17827d3 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -488,3 +488,11 @@ devices or none.
 Please see "QMP invocation for live storage migration with
 ``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst
 for a detailed explanation.
+
+old compression method (since 8.2)
+''
+
+Compression method fails too much.  Too many races.  We are going to
+remove it if nobody fixes it.  For starters, migration-test
+compression tests are disabled becase they fail randomly.  If you need
+compression, use multifd compression methods.
diff --git a/qapi/migration.json b/qapi/migration.json
index e3b00a215b..e6610af428 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -272,6 +272,10 @@
 # Features:
 #
 # @deprecated: Member @disk is deprecated because block migration is.
+# Member @compression is deprecated because it is unreliable and
+# untested.  It is recommended to use multifd migration, which
+# offers an alternative compression implementation that is
+# reliable and tested.
 #
 # Since: 0.14
 ##
@@ -289,7 +293,7 @@
'*blocked-reasons': ['str'],
'*postcopy-blocktime': 'uint32',
'*postcopy-vcpu-blocktime': ['uint32'],
-   '*compression': 'CompressionStats',
+   '*compression': { 'type': 'CompressionStats', 'features': [ 
'deprecated' ] },
'*socket-address': ['SocketAddress'],
'*dirty-limit-throttle-time-per-round': 'uint64',
'*dirty-limit-ring-full-time': 'uint64'} }
@@ -530,7 +534,10 @@
 # Features:
 #
 # @deprecated: Member @block is deprecated.  Use blockdev-mirror with
-# NBD instead.
+# NBD instead.  Member @compression is deprecated because it is
+# unreliable and untested.  It is recommended to use multifd
+# migration, which offers an alternative compression
+# implementation that is reliable and tested.
 #
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
 #
@@ -538,7 +545,8 @@
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-   'compress', 'events', 'postcopy-ram',
+   { 'name': 'compress', 'features': [ 'deprecated' ] },
+   'events', 'postcopy-ram',
{ 'name': 'x-colo', 'features': [ 'unstable' ] },
'release-ram',
{ 'name': 'block', 'features': [ 'deprecated' ] },
@@ -844,7 +852,9 @@
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated.  Use
-# blockdev-mirror with NBD instead.
+# blockdev-mirror with NBD instead.  Members @compress-level,
+# @compress-threads, @decompress-threads and @compress-wait-thread
+# are deprecated because @compression is deprecated.
 #
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
@@ -854,8 +864,11 @@
 { 'enum': 'MigrationParameter',
   'data': ['announce-initial', 'announce-max',
'announce-rounds', 'announce-step',
-   'compress-level', 'compress-threads', 'decompress-threads',
-   'compress-wait-thread', 'throttle-trigger-threshold',
+   { 'name': 'compress-level', 'features': [ 'deprecated' ] },
+   { 'name': 'compress-threads', 'features': [ 'deprecated' ] },
+   { 'name': 'decompress-threads', 'features': [ 'deprecated' ] },
+   { 'name': 'compress-wait-thread', 'features': [ 'deprecated' ] },
+   'throttle-trigger-threshold',
'cpu-throttle-initial', 'cpu-throttle-increment',
'cpu-throttle-tailslow',
'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
@@ -1023,7 +1036,9 @@
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated.  Use
-# blockdev-mirror with NBD instead.
+# blockdev-mirror with NBD instead.  Members @compress-level,
+# @compress-threads, @decompress-threads and @compress-wait-thread
+# are deprecated because @compression is deprecated.
 #
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
@@ -1038,10 +1053,14 @@
 '*announce-max': 'size',
 '*announce-rounds': 'size',
 '*announce-step': 'size',
-'*compress-level': 'uint8',
-'*compress-threads': 'uint8',
-'*compress-wait-thread': 'bool',
-'*decompress-threads': 'uint8',
+'*compress-level': { 'type': 'uint8',
+ 'features': [ 'deprecated' ] },
+'*compress-threads': 

[PATCH v8 2/5] migration: migrate 'inc' command option is deprecated.

[Juan Quintela]

Use blockdev-mirror with NBD instead.

Reviewed-by: Thomas Huth 
Acked-by: Stefan Hajnoczi 
Reviewed-by: Markus Armbruster 
Signed-off-by: Juan Quintela 
---
 docs/about/deprecated.rst  | 8 
 qapi/migration.json| 8 +++-
 migration/migration-hmp-cmds.c | 5 +
 migration/migration.c  | 5 +
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 2febd2d12f..fc6adf1dea 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -461,3 +461,11 @@ Migration
 ``skipped`` field in Migration stats has been deprecated.  It hasn't
 been used for more than 10 years.
 
+``inc`` migrate command option (since 8.2)
+''
+
+Use blockdev-mirror with NBD instead.
+
+As an intermediate step the ``inc`` functionality can be achieved by
+setting the ``block-incremental`` migration parameter to ``true``.
+But this parameter is also deprecated.
diff --git a/qapi/migration.json b/qapi/migration.json
index db3df12d6c..fa7f4f2575 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1524,6 +1524,11 @@
 #
 # @resume: resume one paused migration, default "off". (since 3.0)
 #
+# Features:
+#
+# @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
+# NBD instead.
+#
 # Returns: nothing on success
 #
 # Since: 0.14
@@ -1545,7 +1550,8 @@
 # <- { "return": {} }
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
+  'data': {'uri': 'str', '*blk': 'bool',
+   '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
'*detach': 'bool', '*resume': 'bool' } }
 
 ##
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index a82597f18e..83176f5bae 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -745,6 +745,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 const char *uri = qdict_get_str(qdict, "uri");
 Error *err = NULL;
 
+if (inc) {
+warn_report("option '-i' is deprecated;"
+" use blockdev-mirror with NBD instead");
+}
+
 qmp_migrate(uri, !!blk, blk, !!inc, inc,
 false, false, true, resume, &err);
 if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index 05c0b801ba..27145cd99e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1604,6 +1604,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, 
bool blk_inc,
 {
 Error *local_err = NULL;
 
+if (blk_inc) {
+warn_report("parameter 'inc' is deprecated;"
+" use blockdev-mirror with NBD instead");
+}
+
 if (resume) {
 if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
 error_setg(errp, "Cannot resume if there is no "
-- 
2.41.0

[PATCH v8 4/5] migration: Deprecate block migration

[Juan Quintela]

It is obsolete.  It is better to use driver-mirror with NBD instead.

CC: Kevin Wolf 
CC: Eric Blake 
CC: Stefan Hajnoczi 
CC: Hanna Czenczek 

Acked-by: Stefan Hajnoczi 
Reviewed-by: Markus Armbruster 
Signed-off-by: Juan Quintela 
---
 docs/about/deprecated.rst | 10 ++
 qapi/migration.json   | 29 -
 migration/block.c |  3 +++
 migration/options.c   |  9 -
 4 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 98b0f14e69..7ae872162d 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -478,3 +478,13 @@ Use blockdev-mirror with NBD instead.
 As an intermediate step the ``blk`` functionality can be achieved by
 setting the ``block`` migration capability to ``true``.  But this
 capability is also deprecated.
+
+block migration (since 8.2)
+'''
+
+Block migration is too inflexible.  It needs to migrate all block
+devices or none.
+
+Please see "QMP invocation for live storage migration with
+``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst
+for a detailed explanation.
diff --git a/qapi/migration.json b/qapi/migration.json
index 3765c2b662..e3b00a215b 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -269,11 +269,15 @@
 # average memory load of the virtual CPU indirectly.  Note that
 # zero means guest doesn't dirty memory.  (Since 8.1)
 #
+# Features:
+#
+# @deprecated: Member @disk is deprecated because block migration is.
+#
 # Since: 0.14
 ##
 { 'struct': 'MigrationInfo',
   'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
-   '*disk': 'MigrationStats',
+   '*disk': { 'type': 'MigrationStats', 'features': [ 'deprecated' ] },
'*vfio': 'VfioStats',
'*xbzrle-cache': 'XBZRLECacheStats',
'*total-time': 'int',
@@ -525,6 +529,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block is deprecated.  Use blockdev-mirror with
+# NBD instead.
+#
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
 #
 # Since: 1.2
@@ -534,7 +541,8 @@
'compress', 'events', 'postcopy-ram',
{ 'name': 'x-colo', 'features': [ 'unstable' ] },
'release-ram',
-   'block', 'return-path', 'pause-before-switchover', 'multifd',
+   { 'name': 'block', 'features': [ 'deprecated' ] },
+   'return-path', 'pause-before-switchover', 'multifd',
'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
{ 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
'validate-uuid', 'background-snapshot',
@@ -835,6 +843,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block-incremental is deprecated.  Use
+# blockdev-mirror with NBD instead.
+#
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
 #
@@ -850,7 +861,7 @@
'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
'avail-switchover-bandwidth', 'downtime-limit',
{ 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
-   'block-incremental',
+   { 'name': 'block-incremental', 'features': [ 'deprecated' ] },
'multifd-channels',
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
@@ -1011,6 +1022,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block-incremental is deprecated.  Use
+# blockdev-mirror with NBD instead.
+#
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
 #
@@ -1040,7 +1054,8 @@
 '*downtime-limit': 'uint64',
 '*x-checkpoint-delay': { 'type': 'uint32',
  'features': [ 'unstable' ] },
-'*block-incremental': 'bool',
+'*block-incremental': { 'type': 'bool',
+'features': [ 'deprecated' ] },
 '*multifd-channels': 'uint8',
 '*xbzrle-cache-size': 'size',
 '*max-postcopy-bandwidth': 'size',
@@ -1225,6 +1240,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block-incremental is deprecated.  Use
+# blockdev-mirror with NBD instead.
+#
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 # are experimental.
 #
@@ -1251,7 +1269,8 @@
 '*downtime-limit': 'uint64',
 '*x-checkpoint-delay': { 'type': 'uint32',
  'features': [ 'unstable' ] },
-'*block-incremental': 'bool',
+'*block-incremental': { 'type': 'bool',
+'features': [ 'deprecated' ] },
 '*multifd-channels': 'uint8',
 '*xbzrle-cache-size': 'size',
 '*max-postcopy-bandwidth': 'size',
diff --git a/migration/block.c b/migration/block.c
index b60698d6e2..acffe88f84 100644
--- a/migration/block.c
+

Re: [PATCH 4/4] qapi: introduce CONFIG_READ event

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
>> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
>> > > x- seems safer for management tool that doesn't know about "unstable" 
>> > > properties..
>> > 
>> > Easy, traditional, and unreliable :)
>> 
>> > > But on the other hand, changing from x- to no-prefix is already
>> > > done when the feature is stable, and thouse who use it already
>> > > use the latest version of interface, so, removing the prefix is
>> > > just extra work.
>> > 
>> > Exactly.
>> > 
>> 
>> I think "x-" is still better for command line use of properties - we
>> don't have an API to mark things unstable there, do we?
>
> Personally I like to see "x-" prefix present *everywhere* there is
> an unstable feature, and consider the need to rename when declaring
> it stable to be good thing as it sets an easily identifiable line
> in the sand and is self-evident to outside observers.
>
> The self-documenting nature of the "x-" prefer is what makes it most
> compelling to me. A patch submission, or command line invokation or
> an example QMP command, or a bug report, that exhibit an 'x-' prefix
> are an immediate red flag to anyone who sees them.

Except when it isn't, like in "x-origin".

> If someone sees a QMP comamnd / a typical giant QEMU command line,
> they are never going to go look at the QAPI schema to check whether
> any feature used had an 'unstable' marker. The 'unstable' marker
> might as well not exist in most cases.
>
> IOW, having the 'unstable' flag in the QAPI schema is great for machine
> introspection, but it isn't a substitute for having an 'x-' prefix used
> for the benefit of humans IMHO.

I'm not sure there's disagreement.  Quoting myself:

The "x-" can remind humans "this is unstable" better than a feature
flag can (for machines, it's the other way round).

CLI and HMP are for humans.  We continue to use "x-" there.

QMP is for machines.  The feature flag is the sole source of truth.
Additional use of "x-" is fine, but not required.

Re: [PATCH v8 4/7] qapi: add x-blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

On 18.10.23 13:45, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


Add a command that can replace bs in following BdrvChild structures:

  - qdev blk root child
  - block-export blk root child
  - any child of BlockDriverState selected by child-name

Signed-off-by: Vladimir Sementsov-Ogievskiy 


[..]


--- /dev/null
+++ b/stubs/blk-by-qdev-id.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "sysemu/block-backend.h"
+
+BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
+{
+error_setg(errp, "blk '%s' not found", id);


Is this expected to happen?


Yes, if call the command from qemu-storage-daemon, where qdev-monitor is not 
linked in.

Maybe, better message would be

   "devices are not supported"

Maybe, that possible to use some 'if': notation in qapi, to not include support 
for qdev into the new command, when it compiled into qemu-storage-daemon? Seems 
that would not be simple, as we also need to split compilation of the command 
somehow, now it compiled once both for qemu and qemu tools..




+return NULL;
+}


[...]

QAPI schema
Acked-by: Markus Armbruster 



--
Best regards,
Vladimir

Re: [PATCH 4/4] qapi: introduce CONFIG_READ event

[Daniel P . Berrangé]

On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
> >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
> >> > > x- seems safer for management tool that doesn't know about "unstable" 
> >> > > properties..
> >> > 
> >> > Easy, traditional, and unreliable :)
> >> 
> >> > > But on the other hand, changing from x- to no-prefix is already
> >> > > done when the feature is stable, and thouse who use it already
> >> > > use the latest version of interface, so, removing the prefix is
> >> > > just extra work.
> >> > 
> >> > Exactly.
> >> > 
> >> 
> >> I think "x-" is still better for command line use of properties - we
> >> don't have an API to mark things unstable there, do we?
> >
> > Personally I like to see "x-" prefix present *everywhere* there is
> > an unstable feature, and consider the need to rename when declaring
> > it stable to be good thing as it sets an easily identifiable line
> > in the sand and is self-evident to outside observers.
> >
> > The self-documenting nature of the "x-" prefer is what makes it most
> > compelling to me. A patch submission, or command line invokation or
> > an example QMP command, or a bug report, that exhibit an 'x-' prefix
> > are an immediate red flag to anyone who sees them.
> 
> Except when it isn't, like in "x-origin".
> 
> > If someone sees a QMP comamnd / a typical giant QEMU command line,
> > they are never going to go look at the QAPI schema to check whether
> > any feature used had an 'unstable' marker. The 'unstable' marker
> > might as well not exist in most cases.
> >
> > IOW, having the 'unstable' flag in the QAPI schema is great for machine
> > introspection, but it isn't a substitute for having an 'x-' prefix used
> > for the benefit of humans IMHO.
> 
> I'm not sure there's disagreement.  Quoting myself:
> 
> The "x-" can remind humans "this is unstable" better than a feature
> flag can (for machines, it's the other way round).
> 
> CLI and HMP are for humans.  We continue to use "x-" there.
> 
> QMP is for machines.  The feature flag is the sole source of truth.
> Additional use of "x-" is fine, but not required.

I guess we have different defintions of "for humans" in this context.
I consider QMP  data still relevant for humans, because humans are
reviewing patches to libvirt that add usage of QMP features, or
triaging bug reports that include examples of usage, and in both
cases it is pretty relevant to make unstable features stand out to
the human via the x- prefix IMHO.

With 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 4/4] qapi: introduce CONFIG_READ event

[Vladimir Sementsov-Ogievskiy]

On 18.10.23 15:02, Markus Armbruster wrote:

Daniel P. Berrangé  writes:


On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:

On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:

x- seems safer for management tool that doesn't know about "unstable" 
properties..


Easy, traditional, and unreliable :)



But on the other hand, changing from x- to no-prefix is already
done when the feature is stable, and thouse who use it already
use the latest version of interface, so, removing the prefix is
just extra work.


Exactly.



I think "x-" is still better for command line use of properties - we
don't have an API to mark things unstable there, do we?


Personally I like to see "x-" prefix present *everywhere* there is
an unstable feature, and consider the need to rename when declaring
it stable to be good thing as it sets an easily identifiable line
in the sand and is self-evident to outside observers.

The self-documenting nature of the "x-" prefer is what makes it most
compelling to me. A patch submission, or command line invokation or
an example QMP command, or a bug report, that exhibit an 'x-' prefix
are an immediate red flag to anyone who sees them.


Except when it isn't, like in "x-origin".



Interesting how many such stable x-FOO things we have? Probably we could 
deprecate and than rename them?


--
Best regards,
Vladimir

Re: [PATCH v8 4/7] qapi: add x-blockdev-replace command

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> On 18.10.23 13:45, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>> 
>>> Add a command that can replace bs in following BdrvChild structures:
>>>
>>>   - qdev blk root child
>>>   - block-export blk root child
>>>   - any child of BlockDriverState selected by child-name
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>
> [..]
>
>>> --- /dev/null
>>> +++ b/stubs/blk-by-qdev-id.c
>>> @@ -0,0 +1,9 @@
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +#include "sysemu/block-backend.h"
>>> +
>>> +BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
>>> +{
>>> +error_setg(errp, "blk '%s' not found", id);
>>
>> Is this expected to happen?
>
> Yes, if call the command from qemu-storage-daemon, where qdev-monitor is not 
> linked in.

It happens when you try to x-blockdev-replace with "parent-type":
"qdev".  Correct?

> Maybe, better message would be
>
>"devices are not supported"

Best to spell out which argument is the problem.

Stupidest solution that could possibly work:

"Parameter 'parent-type' does not accept value 'qdev'"

This is exactly what we'd get if we compiled out the parts that don't
make sense for qemu-storage-daemon.

> Maybe, that possible to use some 'if': notation in qapi, to not include 
> support for qdev into the new command, when it compiled into 
> qemu-storage-daemon? Seems that would not be simple, as we also need to split 
> compilation of the command somehow, now it compiled once both for qemu and 
> qemu tools..

That's precisely the problem.

Our reuse of parts of qemu-system-FOO's QAPI schema for
qemu-storage-daemon isn't pretty, but has worked for us so far.

>>> +return NULL;
>>> +}
>> [...]
>> QAPI schema
>> Acked-by: Markus Armbruster 
>> 

Re: [PATCH 4/4] qapi: introduce CONFIG_READ event

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berra...@redhat.com) wrote:
> On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote:
> > Daniel P. Berrangé  writes:
> > 
> > > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
> > >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
> > >> > > x- seems safer for management tool that doesn't know about 
> > >> > > "unstable" properties..
> > >> > 
> > >> > Easy, traditional, and unreliable :)
> > >> 
> > >> > > But on the other hand, changing from x- to no-prefix is already
> > >> > > done when the feature is stable, and thouse who use it already
> > >> > > use the latest version of interface, so, removing the prefix is
> > >> > > just extra work.
> > >> > 
> > >> > Exactly.
> > >> > 
> > >> 
> > >> I think "x-" is still better for command line use of properties - we
> > >> don't have an API to mark things unstable there, do we?
> > >
> > > Personally I like to see "x-" prefix present *everywhere* there is
> > > an unstable feature, and consider the need to rename when declaring
> > > it stable to be good thing as it sets an easily identifiable line
> > > in the sand and is self-evident to outside observers.
> > >
> > > The self-documenting nature of the "x-" prefer is what makes it most
> > > compelling to me. A patch submission, or command line invokation or
> > > an example QMP command, or a bug report, that exhibit an 'x-' prefix
> > > are an immediate red flag to anyone who sees them.
> > 
> > Except when it isn't, like in "x-origin".
> > 
> > > If someone sees a QMP comamnd / a typical giant QEMU command line,
> > > they are never going to go look at the QAPI schema to check whether
> > > any feature used had an 'unstable' marker. The 'unstable' marker
> > > might as well not exist in most cases.
> > >
> > > IOW, having the 'unstable' flag in the QAPI schema is great for machine
> > > introspection, but it isn't a substitute for having an 'x-' prefix used
> > > for the benefit of humans IMHO.
> > 
> > I'm not sure there's disagreement.  Quoting myself:
> > 
> > The "x-" can remind humans "this is unstable" better than a feature
> > flag can (for machines, it's the other way round).
> > 
> > CLI and HMP are for humans.  We continue to use "x-" there.
> > 
> > QMP is for machines.  The feature flag is the sole source of truth.
> > Additional use of "x-" is fine, but not required.
> 
> I guess we have different defintions of "for humans" in this context.
> I consider QMP  data still relevant for humans, because humans are
> reviewing patches to libvirt that add usage of QMP features, or
> triaging bug reports that include examples of usage, and in both
> cases it is pretty relevant to make unstable features stand out to
> the human via the x- prefix IMHO.

Using x- for events makes sense to me; the semantics of events can be
quite subtle; often you don't find out how broken they are until you
wire them through libvirt and up the stack; so it's not impossible
you might need to change it - but then without the x- the semantics
(rather than existence) of the event is carved in stone.

Dave

> With 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 :|
> 
-- 
 -Open up your eyes, open up your mind, open up your code ---   
/ Dr. David Alan Gilbert|   Running GNU/Linux   | Happy  \ 
\dave @ treblig.org |   | In Hex /
 \ _|_ http://www.treblig.org   |___/

Re: [PATCH v3 1/9] blockjob: introduce block-job-change QMP command

[Kevin Wolf]

Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
> which will allow changing job-type-specific options after job
> creation.
> 
> In the JobVerbTable, the same allow bits as for set-speed are used,
> because set-speed can be considered an existing change command.
> 
> Signed-off-by: Fiona Ebner 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

> diff --git a/job.c b/job.c
> index 72d57f0934..99a2e54b54 100644
> --- a/job.c
> +++ b/job.c
> @@ -80,6 +80,7 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = {
>  [JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0},
>  [JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0},
>  [JOB_VERB_DISMISS]  = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0},
> +[JOB_VERB_CHANGE]   = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0},
>  };

I'm not sure if I would have included JOB_STATUS_CREATED, i.e. before
the job has even started, but it's not necessarily a problem. The
implementation just need to be careful to work even in early stages. But
probably the early stages include some part of JOB_STATUS_RUNNING, too,
so they'd have to do this anyway.

Kevin

[PULL 73/83] vhost: move and rename the conn retry times

[Michael S. Tsirkin]

From: Li Feng 

Multiple devices need this macro, move it to a common header.

Signed-off-by: Li Feng 
Reviewed-by: Raphael Norwitz 
Message-Id: <20231009044735.941655-3-fen...@smartx.com>
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 include/hw/virtio/vhost.h   | 2 ++
 hw/block/vhost-user-blk.c   | 4 +---
 hw/virtio/vhost-user-gpio.c | 3 +--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 00e0a669b8..5e8183f64a 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -8,6 +8,8 @@
 #define VHOST_F_DEVICE_IOTLB 63
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
 
+#define VU_REALIZE_CONN_RETRIES 3
+
 /* Generic structures common for any vhost based device. */
 
 struct vhost_inflight {
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..3c69fa47d5 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -32,8 +32,6 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
 
-#define REALIZE_CONNECTION_RETRIES 3
-
 static const int user_feature_bits[] = {
 VIRTIO_BLK_F_SIZE_MAX,
 VIRTIO_BLK_F_SEG_MAX,
@@ -482,7 +480,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, 
Error **errp)
 s->inflight = g_new0(struct vhost_inflight, 1);
 s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
 
-retries = REALIZE_CONNECTION_RETRIES;
+retries = VU_REALIZE_CONN_RETRIES;
 assert(!*errp);
 do {
 if (*errp) {
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 3d7fae3984..fc784e4213 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -15,7 +15,6 @@
 #include "standard-headers/linux/virtio_ids.h"
 #include "trace.h"
 
-#define REALIZE_CONNECTION_RETRIES 3
 #define VHOST_NVQS 2
 
 /* Features required from VirtIO */
@@ -365,7 +364,7 @@ static void vu_gpio_device_realize(DeviceState *dev, Error 
**errp)
 qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, vu_gpio_event, NULL,
  dev, NULL, true);
 
-retries = REALIZE_CONNECTION_RETRIES;
+retries = VU_REALIZE_CONN_RETRIES;
 g_assert(!*errp);
 do {
 if (*errp) {
-- 
MST

[PULL 76/83] vhost-user: fix lost reconnect

[Michael S. Tsirkin]

From: Li Feng 

When the vhost-user is reconnecting to the backend, and if the vhost-user fails
at the get_features in vhost_dev_init(), then the reconnect will fail
and it will not be retriggered forever.

The reason is:
When the vhost-user fails at get_features, the vhost_dev_cleanup will be called
immediately.

vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.

The reconnect path is:
vhost_user_blk_event
   vhost_user_async_close(.. vhost_user_blk_disconnect ..)
 qemu_chr_fe_set_handlers <- clear the notifier callback
   schedule vhost_user_async_close_bh

The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
called, then the event fd callback will not be reinstalled.

All vhost-user devices have this issue, including vhost-user-blk/scsi.

With this patch, if the vdev->vdev is null, the fd callback will still
be reinstalled.

Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")

Signed-off-by: Li Feng 
Reviewed-by: Raphael Norwitz 
Message-Id: <20231009044735.941655-6-fen...@smartx.com>
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 include/hw/virtio/vhost-user.h |  3 ++-
 hw/block/vhost-user-blk.c  |  2 +-
 hw/scsi/vhost-user-scsi.c  |  3 ++-
 hw/virtio/vhost-user-gpio.c|  2 +-
 hw/virtio/vhost-user.c | 10 --
 5 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index 9f9ddf878d..6b06ecb1bd 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -106,6 +106,7 @@ typedef void (*vu_async_close_fn)(DeviceState *cb);
 
 void vhost_user_async_close(DeviceState *d,
 CharBackend *chardev, struct vhost_dev *vhost,
-vu_async_close_fn cb);
+vu_async_close_fn cb,
+IOEventHandler *event_cb);
 
 #endif
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 3c69fa47d5..95c758200d 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -391,7 +391,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent 
event)
 case CHR_EVENT_CLOSED:
 /* defer close until later to avoid circular close */
 vhost_user_async_close(dev, &s->chardev, &s->dev,
-   vhost_user_blk_disconnect);
+   vhost_user_blk_disconnect, 
vhost_user_blk_event);
 break;
 case CHR_EVENT_BREAK:
 case CHR_EVENT_MUX_IN:
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 258fba5c69..4486500cac 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -212,7 +212,8 @@ static void vhost_user_scsi_event(void *opaque, 
QEMUChrEvent event)
 case CHR_EVENT_CLOSED:
 /* defer close until later to avoid circular close */
 vhost_user_async_close(dev, &vs->conf.chardev, &vsc->dev,
-   vhost_user_scsi_disconnect);
+   vhost_user_scsi_disconnect,
+   vhost_user_scsi_event);
 break;
 case CHR_EVENT_BREAK:
 case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index fc784e4213..aff2d7eff6 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -289,7 +289,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
 case CHR_EVENT_CLOSED:
 /* defer close until later to avoid circular close */
 vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
-   vu_gpio_disconnect);
+   vu_gpio_disconnect, vu_gpio_event);
 break;
 case CHR_EVENT_BREAK:
 case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index f9414f03de..b8a7b5542d 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2756,6 +2756,7 @@ typedef struct {
 DeviceState *dev;
 CharBackend *cd;
 struct vhost_dev *vhost;
+IOEventHandler *event_cb;
 } VhostAsyncCallback;
 
 static void vhost_user_async_close_bh(void *opaque)
@@ -2770,7 +2771,10 @@ static void vhost_user_async_close_bh(void *opaque)
  */
 if (vhost->vdev) {
 data->cb(data->dev);
-}
+} else if (data->event_cb) {
+qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
+ NULL, data->dev, NULL, true);
+   }
 
 g_free(data);
 }
@@ -2782,7 +2786,8 @@ static void vhost_user_async_close_bh(void *opaque)
  */
 void vhost_user_async_close(DeviceState *d,
 CharBackend *chardev, struct vhost_dev *vhost,
-vu_async_close_fn cb)
+vu_async_close_fn cb,
+IOEventHandler *event_cb)
 {
 if (!runstate_check(RUN_STATE_SHUTDOWN)) {
 /*
@@ -2798,6 +

Re: [PATCH v3 5/9] mirror: implement mirror_change method

[Kevin Wolf]

Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
> which allows switching the @copy-mode from 'background' to
> 'write-blocking'.
> 
> This is useful for management applications, so they can start out in
> background mode to avoid limiting guest write speed and switch to
> active mode when certain criteria are fulfilled.
> 
> In presence of an iothread, the copy_mode member is now shared between
> the iothread and the main thread, so turn accesses to it atomic.
> 
> Signed-off-by: Fiona Ebner 
> ---
> 
> Changes in v3:
> * turn accesses to copy_mode atomic and...
> * ...slightly adapt error handling in mirror_change as a
>   consequence

It would be good to have a comment at the field declaration that it's
meant to be accessed with atomics.

As we don't have further synchonisation, is the idea that during the
switchover it basically doesn't matter if we read the old or the new
value?

After reading the whole patch, it seems that the field is only ever
written under the BQL, while iothreads only read it, and only once per
request (after the previous patch). This is why no further
synchonisation is needed. If other threads could write it, too,
mirror_change() would probably have to be more careful. As the code
depends on this, adding that to the comment would be useful, too.

>  block/mirror.c   | 33 ++---
>  qapi/block-core.json | 13 -
>  2 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 8992c09172..889cce5414 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1075,7 +1075,7 @@ static int coroutine_fn mirror_run(Job *job, Error 
> **errp)
>   */
>  job_transition_to_ready(&s->common.job);
>  }
> -if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
> +if (qatomic_read(&s->copy_mode) != MIRROR_COPY_MODE_BACKGROUND) {
>  s->actively_synced = true;
>  }

What resets s->actively_synced when we switch away from active mode?

>  
> @@ -1246,6 +1246,32 @@ static bool commit_active_cancel(Job *job, bool force)
>  return force || !job_is_ready(job);
>  }
>  
> +static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
> +  Error **errp)
> +{
> +MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
> +BlockJobChangeOptionsMirror *change_opts = &opts->u.mirror;
> +MirrorCopyMode current;

This is GLOBAL_STATE_CODE(), right? Let's be explicit about it.

> +
> +if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
> +return;
> +}
> +
> +if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
> +error_setg(errp, "Change to copy mode '%s' is not implemented",
> +   MirrorCopyMode_str(change_opts->copy_mode));
> +return;
> +}

Ah, ok, we don't even allow the switch I was wondering about above. What
would be needed, apart from removing this check, to make it work?

> +current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
> +  change_opts->copy_mode);
> +if (current != MIRROR_COPY_MODE_BACKGROUND) {
> +error_setg(errp, "Expected current copy mode '%s', got '%s'",
> +   MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
> +   MirrorCopyMode_str(current));
> +}

The error path is strange. We return an error, but the new mode is still
set. On the other hand, this is probably also the old mode unless
someone added a new value to the enum, so it didn't actually change. And
because this function is the only place that changes copy_mode and we're
holding the BQL, the case can't even happen and this could be an
assertion.

> +}
> +
>  static const BlockJobDriver mirror_job_driver = {
>  .job_driver = {
>  .instance_size  = sizeof(MirrorBlockJob),
> @@ -1260,6 +1286,7 @@ static const BlockJobDriver mirror_job_driver = {
>  .cancel = mirror_cancel,
>  },
>  .drained_poll   = mirror_drained_poll,
> +.change = mirror_change,
>  };

Kevin

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

On Wed, 2023-10-18 at 10:52 +0200, Kevin Wolf wrote:
> Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> > From: David Woodhouse 
> > 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Signed-off-by: David Woodhouse 
> 
> Actually, how does this play together with xen_config_dev_blk()? This
> looks like it tried to implement a very similar thing (which is IF_XEN
> even already existed).
> 
> Are we now trying to attach each if=xen disk twice in the 'xenpv'
> machine? Or if something prevents this, is it dead code?
> 
> I think in both cases, we would want to delete that function and remove
> the loop that calls it in xen_init_pv()?

I tested this with an xl config which looks a bit like this...

disk = [ 
"backendtype=qdisk,format=qcow2,vdev=xvda,access=rw,target=/var/lib/libvirt/images/fedora28.qcow2"
 ]
device_model_override = "/home/dwmw2/git/qemu/build/qemu-system-x86_64"
device_model_version = "qemu-xen"
device_model_args = [ 
"-trace","xen*","-chardev","pty,id=mon","-mon","mon","-drive","file=/var/lib/libvirt/images/solaris11.qcow2,format=qcow2,if=xen","-nic","user,model=xen"
 ]

The code in xen_config_dev_blk() scribbles over the disk that the
toolstack has configured for me, and adds that qcow2 file from the
'-drive' option on the command line, but in *raw* mode.

Then the new xen-disk support kicks in and finds a free vdev, and adds
the -drive properly /dev/xvdb (as qcow2).

So v2 of this patch will just rip out xen_config_dev_blk() as you
suggest.



smime.p7s
Description: S/MIME cryptographic signature