Re: [RFC PATCH v2 02/78] block: add fallthrough pseudo-keyword

[Eric Blake]

On Fri, Oct 13, 2023 at 10:56:29AM +0300, Emmanouil Pitsidianakis wrote:
> In preparation of raising -Wimplicit-fallthrough to 5, replace all
> fall-through comments with the fallthrough attribute pseudo-keyword.
> 
> Signed-off-by: Emmanouil Pitsidianakis 
> ---
>  block/block-copy.c|  1 +
>  block/file-posix.c|  1 +
>  block/io.c|  1 +
>  block/iscsi.c |  1 +
>  block/qcow2-cluster.c |  5 -
>  block/vhdx.c  | 17 +
>  6 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 1c60368d72..b4ceb6a079 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
...
>  case COPY_RANGE_FULL:
>  ret = bdrv_co_copy_range(s->source, offset, s->target, offset, 
> nbytes,
>   0, s->write_flags);
>  if (ret >= 0) {
>  /* Successful copy-range, increase chunk size.  */
>  *method = COPY_RANGE_FULL;
>  return 0;
>  }
>  
>  trace_block_copy_copy_range_fail(s, offset, ret);
>  *method = COPY_READ_WRITE;
>  /* Fall through to read+write with allocated buffer */
> +fallthrough;
>  
>  case COPY_READ_WRITE_CLUSTER:
>  case COPY_READ_WRITE:

I like how you kept the comments.

> +++ b/block/qcow2-cluster.c
> @@ -1327,36 +1327,39 @@ static int coroutine_fn 
> calculate_l2_meta(BlockDriverState *bs,
>  /*
>   * Returns true if writing to the cluster pointed to by @l2_entry
>   * requires a new allocation (that is, if the cluster is unallocated
>   * or has refcount > 1 and therefore cannot be written in-place).
>   */
>  static bool cluster_needs_new_alloc(BlockDriverState *bs, uint64_t l2_entry)
>  {
>  switch (qcow2_get_cluster_type(bs, l2_entry)) {
>  case QCOW2_CLUSTER_NORMAL:
> +fallthrough;
>  case QCOW2_CLUSTER_ZERO_ALLOC:

Why is this one needed?  It looks two case labels for the same code is
okay; the fallthrough attribute is only needed once a case label is no
lonter empty.

>  if (l2_entry & QCOW_OFLAG_COPIED) {
>  return false;
>  }
> -/* fallthrough */
> +fallthrough;

This one makes sense.

>  case QCOW2_CLUSTER_UNALLOCATED:
> +fallthrough;
>  case QCOW2_CLUSTER_COMPRESSED:
> +fallthrough;

These two also look spurious.

>  case QCOW2_CLUSTER_ZERO_PLAIN:
>  return true;
>  default:
>  abort();
>  }
>  }
...
> +++ b/block/vhdx.c
> @@ -1176,60 +1176,65 @@ static int coroutine_fn GRAPH_RDLOCK
>  vhdx_co_readv(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>QEMUIOVector *qiov)
...
>  /* check the payload block state */
>  switch (s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) {
> -case PAYLOAD_BLOCK_NOT_PRESENT: /* fall through */
> +case PAYLOAD_BLOCK_NOT_PRESENT:
> +fallthrough;
>  case PAYLOAD_BLOCK_UNDEFINED:
> +fallthrough;
>  case PAYLOAD_BLOCK_UNMAPPED:
> +fallthrough;
>  case PAYLOAD_BLOCK_UNMAPPED_v095:
> +fallthrough;

All four of these look spurious; although the old comment is also
spurious, so I'd be happy with deleting it without replacement.

>  case PAYLOAD_BLOCK_ZERO:
>  /* return zero */
>  qemu_iovec_memset(_qiov, 0, 0, sinfo.bytes_avail);
>  break;
>  case PAYLOAD_BLOCK_FULLY_PRESENT:
>  qemu_co_mutex_unlock(>lock);
>  ret = bdrv_co_preadv(bs->file, sinfo.file_offset,
>   sinfo.sectors_avail * BDRV_SECTOR_SIZE,
>   _qiov, 0);
>  qemu_co_mutex_lock(>lock);
>  if (ret < 0) {
>  goto exit;
>  }
>  break;
>  case PAYLOAD_BLOCK_PARTIALLY_PRESENT:
>  /* we don't yet support difference files, fall through
>   * to error */
> +fallthrough;
>  default:

But keeping this one because of the comment is reasonable.

...
>  switch (bat_state) {
>  case PAYLOAD_BLOCK_ZERO:
>  /* in this case, we need to preserve zero writes for
>   * data that is not part of this write, so we must pad
>   * the rest of the buffer to zeroes */
>  use_zero_buffers = true;
> -/* fall through */
> -case PAYLOAD_BLOCK_NOT_PRESENT: /* fall through */
> +fallthrough;
> +case PAYLOAD_BLOCK_NOT_PRESENT:

This one is necessary;

> +fallthrough;
>  case PAYLOAD_BLOCK_UNMAPPED:
> +fallthrough;
>  case PAYLOAD_BLOCK_UNMAPPED_v095:
> +fallthrough;
>  case 

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

[Juan Quintela]

Juan Quintela  wrote:
> Use blocked-mirror with NBD instead.
>
> Signed-off-by: Juan Quintela 
> Acked-by: Stefan Hajnoczi 
> Reviewed-by: Thomas Huth 
> Reviewed-by: Markus Armbruster 

Hi Kevin and Stefan

Can we change the iotest output to fix this?

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}}


I really want to give the warnings two users.

I guess that you want to test blk migration.

What do you recommend?

Later, Juan.

Re: [PATCH 7/7] hw/usb: Declare link using static DEFINE_PROP_LINK() macro

[Mark Cave-Ayland]

On 17/10/2023 15:01, Philippe Mathieu-Daudé wrote:


Pull the 'dma' property to the core XHCI type, declare
its link statically using DEFINE_PROP_LINK().

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/usb/hcd-xhci-sysbus.c | 4 
  hw/usb/hcd-xhci.c| 2 ++
  2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-xhci-sysbus.c b/hw/usb/hcd-xhci-sysbus.c
index faf57b4797..15983d0b96 100644
--- a/hw/usb/hcd-xhci-sysbus.c
+++ b/hw/usb/hcd-xhci-sysbus.c
@@ -60,10 +60,6 @@ static void xhci_sysbus_instance_init(Object *obj)
  object_initialize_child(obj, "xhci-core", >xhci, TYPE_XHCI);
  qdev_alias_all_properties(DEVICE(>xhci), obj);
  
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,

- (Object **)>xhci.dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
  s->xhci.intr_update = NULL;
  s->xhci.intr_raise = xhci_sysbus_intr_raise;
  }
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4b60114207..012a6f3644 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3638,6 +3638,8 @@ static Property xhci_properties[] = {
  DEFINE_PROP_UINT32("p3",XHCIState, numports_3, 4),
  DEFINE_PROP_LINK("host",XHCIState, hostOpaque, TYPE_DEVICE,
   DeviceState *),
+DEFINE_PROP_LINK("dma", XHCIState, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
  DEFINE_PROP_END_OF_LIST(),
  };


I'm slightly unsure about this one: does pulling the "dma" property into the core 
type cause any issues if the property is left unset for any non-sysbus xhci users?



ATB,

Mark.

Re: [PATCH 6/7] hw/net: Declare link using static DEFINE_PROP_LINK() macro

[Mark Cave-Ayland]

On 17/10/2023 15:01, Philippe Mathieu-Daudé wrote:


Declare link statically using DEFINE_PROP_LINK().

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/net/cadence_gem.c | 7 ++-
  1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index f445d8bb5e..37e209cda6 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1654,11 +1654,6 @@ static void gem_init(Object *obj)
"enet", sizeof(s->regs));
  
  sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);

-
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,
- (Object **)>dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
  }
  
  static const VMStateDescription vmstate_cadence_gem = {

@@ -1691,6 +1686,8 @@ static Property gem_properties[] = {
num_type2_screeners, 4),
  DEFINE_PROP_UINT16("jumbo-max-len", CadenceGEMState,
 jumbo_max_len, 10240),
+DEFINE_PROP_LINK("dma", CadenceGEMState, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
  DEFINE_PROP_END_OF_LIST(),
  };


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 5/7] hw/dma: Declare link using static DEFINE_PROP_LINK() macro

[Mark Cave-Ayland]

On 17/10/2023 15:01, Philippe Mathieu-Daudé wrote:


Declare link statically using DEFINE_PROP_LINK().

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/dma/xilinx_axidma.c |  6 ++
  hw/dma/xlnx-zdma.c |  7 ++-
  hw/dma/xlnx_csu_dma.c  | 13 -
  3 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 12c90267df..0ae056ed06 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -577,10 +577,6 @@ static void xilinx_axidma_init(Object *obj)
  object_initialize_child(OBJECT(s), "axistream-control-connected-target",
  >rx_control_dev,
  TYPE_XILINX_AXI_DMA_CONTROL_STREAM);
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,
- (Object **)>dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
  
  sysbus_init_irq(sbd, >streams[0].irq);

  sysbus_init_irq(sbd, >streams[1].irq);
@@ -596,6 +592,8 @@ static Property axidma_properties[] = {
   tx_data_dev, TYPE_STREAM_SINK, StreamSink *),
  DEFINE_PROP_LINK("axistream-control-connected", XilinxAXIDMA,
   tx_control_dev, TYPE_STREAM_SINK, StreamSink *),
+DEFINE_PROP_LINK("dma", XilinxAXIDMA, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
  DEFINE_PROP_END_OF_LIST(),
  };
  
diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c

index 4eb7f66e9f..84c0083013 100644
--- a/hw/dma/xlnx-zdma.c
+++ b/hw/dma/xlnx-zdma.c
@@ -795,11 +795,6 @@ static void zdma_init(Object *obj)
TYPE_XLNX_ZDMA, ZDMA_R_MAX * 4);
  sysbus_init_mmio(sbd, >iomem);
  sysbus_init_irq(sbd, >irq_zdma_ch_imr);
-
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,
- (Object **)>dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
  }
  
  static const VMStateDescription vmstate_zdma = {

@@ -817,6 +812,8 @@ static const VMStateDescription vmstate_zdma = {
  
  static Property zdma_props[] = {

  DEFINE_PROP_UINT32("bus-width", XlnxZDMA, cfg.bus_width, 64),
+DEFINE_PROP_LINK("dma", XlnxZDMA, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
  DEFINE_PROP_END_OF_LIST(),
  };
  
diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c

index 88002698a1..e89089821a 100644
--- a/hw/dma/xlnx_csu_dma.c
+++ b/hw/dma/xlnx_csu_dma.c
@@ -702,6 +702,10 @@ static Property xlnx_csu_dma_properties[] = {
   * which channel the device is connected to.
   */
  DEFINE_PROP_BOOL("is-dst", XlnxCSUDMA, is_dst, true),
+DEFINE_PROP_LINK("stream-connected-dma", XlnxCSUDMA, tx_dev,
+ TYPE_STREAM_SINK, StreamSink *),
+DEFINE_PROP_LINK("dma", XlnxCSUDMA, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
  DEFINE_PROP_END_OF_LIST(),
  };
  
@@ -728,15 +732,6 @@ static void xlnx_csu_dma_init(Object *obj)
  
  memory_region_init(>iomem, obj, TYPE_XLNX_CSU_DMA,

 XLNX_CSU_DMA_R_MAX * 4);
-
-object_property_add_link(obj, "stream-connected-dma", TYPE_STREAM_SINK,
- (Object **)>tx_dev,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,
- (Object **)>dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
  }
  
  static const TypeInfo xlnx_csu_dma_info = {


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 4/7] hw/scsi/virtio-scsi: Use VIRTIO_SCSI_COMMON() macro

[Mark Cave-Ayland]

On 17/10/2023 15:01, Philippe Mathieu-Daudé wrote:


Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/scsi/virtio-scsi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 45b95ea070..fa53f0902c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
  
  static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)

  {
-VirtIOSCSICommon *vs = >parent_obj;
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
  SCSIDevice *d;
  int rc;


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 3/7] hw/display/virtio-gpu: Use VIRTIO_DEVICE() macro

[Mark Cave-Ayland]

On 17/10/2023 15:01, Philippe Mathieu-Daudé wrote:


Access QOM parent with the proper QOM VIRTIO_DEVICE() macro.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/display/virtio-gpu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 93857ad523..51cb517999 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1132,7 +1132,7 @@ static void virtio_gpu_ctrl_bh(void *opaque)
  VirtIOGPU *g = opaque;
  VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
  
-vgc->handle_ctrl(>parent_obj.parent_obj, g->ctrl_vq);

+vgc->handle_ctrl(VIRTIO_DEVICE(g), g->ctrl_vq);
  }
  
  static void virtio_gpu_handle_cursor(VirtIODevice *vdev, VirtQueue *vq)


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 2/7] hw/block/vhost-user-blk: Use DEVICE() / VIRTIO_DEVICE() macros

[Mark Cave-Ayland]

On 17/10/2023 15:01, Philippe Mathieu-Daudé wrote:


Access QOM parent with the proper QOM [VIRTIO_]DEVICE() macros.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/block/vhost-user-blk.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..4b37e26120 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -405,7 +405,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent 
event)
  
  static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)

  {
-DeviceState *dev = >parent_obj.parent_obj;
+DeviceState *dev = DEVICE(s);
  int ret;
  
  s->connected = false;

@@ -423,7 +423,7 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, 
Error **errp)
  assert(s->connected);
  
  ret = vhost_dev_get_config(>dev, (uint8_t *)>blkcfg,

-   s->parent_obj.config_len, errp);
+   VIRTIO_DEVICE(s)->config_len, errp);
  if (ret < 0) {
  qemu_chr_fe_disconnect(>chardev);
  vhost_dev_cleanup(>dev);


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH v3 0/8] qemu-img: rebase: add compression support

[Kevin Wolf]

Am 19.09.2023 um 18:57 hat Andrey Drobyshev geschrieben:
> v2 --> v3:
>  * Patch 3/8: fixed logic in the if statement, so that we align on blk
>when blk_old_backing == NULL;
>  * Patch 4/8: comment fix;
>  * Patch 5/8: comment fix; dropped redundant "if (blk_new_backing)"
>statements.
> 
> v2: https://lists.nongnu.org/archive/html/qemu-block/2023-09/msg00448.html
> 
> Andrey Drobyshev (8):
>   qemu-img: rebase: stop when reaching EOF of old backing file
>   qemu-iotests: 024: add rebasing test case for overlay_size >
> backing_size
>   qemu-img: rebase: use backing files' BlockBackend for buffer alignment
>   qemu-img: add chunk size parameter to compare_buffers()
>   qemu-img: rebase: avoid unnecessary COW operations
>   iotests/{024, 271}: add testcases for qemu-img rebase
>   qemu-img: add compression option to rebase subcommand
>   iotests: add tests for "qemu-img rebase" with compression

Thanks, applied to the block branch.

Kevin

Re: [PATCH v3 4/8] qemu-img: add chunk size parameter to compare_buffers()

[Kevin Wolf]

Am 19.09.2023 um 18:58 hat Andrey Drobyshev geschrieben:
> Add @chsize param to the function which, if non-zero, would represent
> the chunk size to be used for comparison.  If it's zero, then
> BDRV_SECTOR_SIZE is used as default chunk size, which is the previous
> behaviour.
> 
> In particular, we're going to use this param in img_rebase() to make the
> write requests aligned to a predefined alignment value.
> 
> Signed-off-by: Andrey Drobyshev 
> Reviewed-by: Eric Blake 
> Reviewed-by: Hanna Czenczek 
> ---
>  qemu-img.c | 24 +++-
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 4dc91505bf..0f67b021f7 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1274,23 +1274,29 @@ static int is_allocated_sectors_min(const uint8_t 
> *buf, int n, int *pnum,
>  }
>  
>  /*
> - * Compares two buffers sector by sector. Returns 0 if the first
> - * sector of each buffer matches, non-zero otherwise.
> + * Compares two buffers chunk by chunk, where @chsize is the chunk size.
> + * If @chsize is 0, default chunk size of BDRV_SECTOR_SIZE is used.
> + * Returns 0 if the first chunk of each buffer matches, non-zero otherwise.
>   *
> - * pnum is set to the sector-aligned size of the buffer prefix that
> - * has the same matching status as the first sector.
> + * @pnum is set to the size of the buffer prefix aligned to @chsize that
> + * has the same matching status as the first chunk.
>   */
>  static int compare_buffers(const uint8_t *buf1, const uint8_t *buf2,
> -   int64_t bytes, int64_t *pnum)
> +   int64_t bytes, uint64_t chsize, int64_t *pnum)

uint64_t might be too large, depending on the platform. memcmp() takes
size_t, so in theory we can get truncation.

I assume that we won't see a problem in practice because you'll never
pass that large values, but I'd prefer making chsize size_t here.

Kevin

Re: [PULL 00/38] Migration 20231017 patches

[Stefan Hajnoczi]

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any 
user-visible changes.


signature.asc
Description: PGP signature

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

[Vladimir Sementsov-Ogievskiy]

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 
---
 blockdev.c | 56 
 qapi/block-core.json   | 83 ++
 stubs/blk-by-qdev-id.c |  9 +
 stubs/meson.build  |  1 +
 4 files changed, 149 insertions(+)
 create mode 100644 stubs/blk-by-qdev-id.c

diff --git a/blockdev.c b/blockdev.c
index 49d0aab356..255f4971ec 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3700,6 +3700,62 @@ void qmp_x_blockdev_set_iothread(const char *node_name, 
StrOrNull *iothread,
 aio_context_release(old_context);
 }
 
+void qmp_x_blockdev_replace(BlockdevReplace *repl, Error **errp)
+{
+BdrvChild *child = NULL;
+BlockDriverState *new_child_bs;
+
+if (repl->parent_type == BLOCK_PARENT_TYPE_DRIVER) {
+BlockDriverState *parent_bs;
+
+parent_bs = bdrv_find_node(repl->u.driver.node_name);
+if (!parent_bs) {
+error_setg(errp, "Block driver node with node-name '%s' not "
+   "found", repl->u.driver.node_name);
+return;
+}
+
+child = bdrv_find_child(parent_bs, repl->u.driver.child);
+if (!child) {
+error_setg(errp, "Block driver node '%s' doesn't have child "
+   "named '%s'", repl->u.driver.node_name,
+   repl->u.driver.child);
+return;
+}
+} else {
+/* Other types are similar, they work through blk */
+BlockBackend *blk;
+bool is_qdev = repl->parent_type == BLOCK_PARENT_TYPE_QDEV;
+const char *id =
+is_qdev ? repl->u.qdev.qdev_id : repl->u.export.export_id;
+
+assert(is_qdev || repl->parent_type == BLOCK_PARENT_TYPE_EXPORT);
+
+blk = is_qdev ? blk_by_qdev_id(id, errp) : blk_by_export_id(id, errp);
+if (!blk) {
+return;
+}
+
+child = blk_root(blk);
+if (!child) {
+error_setg(errp, "%s '%s' is empty, nothing to replace",
+   is_qdev ? "Device" : "Export", id);
+return;
+}
+}
+
+assert(child);
+assert(child->bs);
+
+new_child_bs = bdrv_find_node(repl->new_child);
+if (!new_child_bs) {
+error_setg(errp, "Node '%s' not found", repl->new_child);
+return;
+}
+
+bdrv_replace_child_bs(child, new_child_bs, errp);
+}
+
 QemuOptsList qemu_common_drive_opts = {
 .name = "drive",
 .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
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 

[PATCH v8 2/7] block/export: add blk_by_export_id()

[Vladimir Sementsov-Ogievskiy]

We need it for further blockdev-replace functionality.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/export/export.c   | 18 ++
 include/sysemu/block-backend-global-state.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/block/export/export.c b/block/export/export.c
index a8f274e526..d68c5d78ca 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -375,3 +375,21 @@ BlockExportInfoList *qmp_query_block_exports(Error **errp)
 
 return head;
 }
+
+BlockBackend *blk_by_export_id(const char *id, Error **errp)
+{
+BlockExport *exp;
+
+exp = blk_exp_find(id);
+if (exp == NULL) {
+error_setg(errp, "Export '%s' not found", id);
+return NULL;
+}
+
+if (!exp->blk) {
+error_setg(errp, "Export '%s' is empty", id);
+return NULL;
+}
+
+return exp->blk;
+}
diff --git a/include/sysemu/block-backend-global-state.h 
b/include/sysemu/block-backend-global-state.h
index ccb35546a1..410d0cc5c7 100644
--- a/include/sysemu/block-backend-global-state.h
+++ b/include/sysemu/block-backend-global-state.h
@@ -74,6 +74,7 @@ void blk_detach_dev(BlockBackend *blk, DeviceState *dev);
 DeviceState *blk_get_attached_dev(BlockBackend *blk);
 BlockBackend *blk_by_dev(void *dev);
 BlockBackend *blk_by_qdev_id(const char *id, Error **errp);
+BlockBackend *blk_by_export_id(const char *id, Error **errp);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
 
 void blk_activate(BlockBackend *blk, Error **errp);
-- 
2.34.1

[PATCH v8 6/7] iotests.py: introduce VM.assert_edges_list() method

[Vladimir Sementsov-Ogievskiy]

Add an alternative method to check block graph, to be used in further
commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 17 +
 1 file changed, 17 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e5c5798c71..638105cdc7 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1101,6 +1101,23 @@ def check_bitmap_status(self, node_name, bitmap_name, 
fields):
 
 return fields.items() <= ret.items()
 
+def get_block_graph(self):
+"""
+Returns block graph in form of edges list, where each edge is a tuple:
+  (parent_node_name, child_name, child_node_name)
+"""
+graph = self.qmp('x-debug-query-block-graph')['return']
+
+nodes = {n['id']: n['name'] for n in graph['nodes']}
+# Check that all names are unique:
+assert len(set(nodes.values())) == len(nodes)
+
+return [(nodes[e['parent']], e['name'], nodes[e['child']])
+for e in graph['edges']]
+
+def assert_edges_list(self, edges):
+assert sorted(edges) == sorted(self.get_block_graph())
+
 def assert_block_path(self, root, path, expected_node, graph=None):
 """
 Check whether the node under the given path in the block graph
-- 
2.34.1

[PATCH v8 7/7] iotests: add filter-insertion

[Vladimir Sementsov-Ogievskiy]

Demonstrate new blockdev-replace API for filter insertion and removal.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/tests/filter-insertion | 236 ++
 tests/qemu-iotests/tests/filter-insertion.out |   5 +
 2 files changed, 241 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/filter-insertion
 create mode 100644 tests/qemu-iotests/tests/filter-insertion.out

diff --git a/tests/qemu-iotests/tests/filter-insertion 
b/tests/qemu-iotests/tests/filter-insertion
new file mode 100755
index 00..be9a7780bb
--- /dev/null
+++ b/tests/qemu-iotests/tests/filter-insertion
@@ -0,0 +1,236 @@
+#!/usr/bin/env python3
+#
+# Tests for inserting and removing filters in a block graph.
+#
+# Copyright (c) 2022 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+
+import iotests
+from iotests import qemu_img_create, try_remove
+
+
+disk = os.path.join(iotests.test_dir, 'disk')
+sock = os.path.join(iotests.sock_dir, 'sock')
+size = 1024 * 1024
+
+
+class TestFilterInsertion(iotests.QMPTestCase):
+def setUp(self):
+qemu_img_create('-f', iotests.imgfmt, disk, str(size))
+
+self.vm = iotests.VM()
+self.vm.launch()
+
+self.vm.cmd('blockdev-add', {
+'node-name': 'disk0',
+'driver': 'qcow2',
+'file': {
+'node-name': 'file0',
+'driver': 'file',
+'filename': disk
+}
+})
+
+def tearDown(self):
+self.vm.shutdown()
+os.remove(disk)
+try_remove(sock)
+
+def test_simple_insertion(self):
+vm = self.vm
+
+vm.cmd('blockdev-add', {
+'node-name': 'filter',
+'driver': 'blkdebug',
+'image': 'file0'
+})
+
+vm.cmd('x-blockdev-replace', {
+'parent-type': 'driver',
+'node-name': 'disk0',
+'child': 'file',
+'new-child': 'filter'
+})
+
+# Filter inserted:
+# disk0 -file-> filter -file-> file0
+vm.assert_edges_list([
+('disk0', 'file', 'filter'),
+('filter', 'image', 'file0')
+])
+
+vm.cmd('x-blockdev-replace', {
+'parent-type': 'driver',
+'node-name': 'disk0',
+'child': 'file',
+'new-child': 'file0'
+})
+
+# Filter replaced, but still exists:
+# dik0 -file-> file0 <-file- filter
+vm.assert_edges_list([
+('disk0', 'file', 'file0'),
+('filter', 'image', 'file0')
+])
+
+vm.cmd('blockdev-del', node_name='filter')
+
+# Filter removed
+# dik0 -file-> file0
+vm.assert_edges_list([
+('disk0', 'file', 'file0')
+])
+
+def test_insert_under_qdev(self):
+vm = self.vm
+
+vm.cmd('device_add', driver='virtio-scsi')
+vm.cmd('device_add', id='sda', driver='scsi-hd',
+ drive='disk0')
+
+vm.cmd('blockdev-add', {
+'node-name': 'filter',
+'driver': 'compress',
+'file': 'disk0'
+})
+
+vm.cmd('x-blockdev-replace', {
+'parent-type': 'qdev',
+'qdev-id': 'sda',
+'new-child': 'filter'
+})
+
+# Filter inserted:
+# sda -root-> filter -file-> disk0 -file-> file0
+vm.assert_edges_list([
+# parent_node_name, child_name, child_node_name
+('sda', 'root', 'filter'),
+('filter', 'file', 'disk0'),
+('disk0', 'file', 'file0'),
+])
+
+vm.cmd('x-blockdev-replace', {
+'parent-type': 'qdev',
+'qdev-id': 'sda',
+'new-child': 'disk0'
+})
+vm.cmd('blockdev-del', node_name='filter')
+
+# Filter removed:
+# sda -root-> disk0 -file-> file0
+vm.assert_edges_list([
+# parent_node_name, child_name, child_node_name
+('sda', 'root', 'disk0'),
+('disk0', 'file', 'file0'),
+])
+
+def test_insert_under_nbd_export(self):
+vm = self.vm
+
+vm.cmd('nbd-server-start',
+ addr={'type': 'unix', 'data': {'path': sock}})
+vm.cmd('block-export-add', id='exp1', type='nbd',
+ node_name='disk0', 

[PATCH v8 5/7] block: bdrv_get_xdbg_block_graph(): report export ids

[Vladimir Sementsov-Ogievskiy]

Currently for block exports we report empty blk names. That's not good.
Let's try to find corresponding block export and report its id.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c |  4 
 block/export/export.c   | 13 +
 include/block/export.h  |  1 +
 stubs/blk-exp-find-by-blk.c |  9 +
 stubs/meson.build   |  1 +
 5 files changed, 28 insertions(+)
 create mode 100644 stubs/blk-exp-find-by-blk.c

diff --git a/block.c b/block.c
index 6215ee9f28..e0b83e6181 100644
--- a/block.c
+++ b/block.c
@@ -6429,7 +6429,11 @@ XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp)
 for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
 char *allocated_name = NULL;
 const char *name = blk_name(blk);
+BlockExport *exp = blk_exp_find_by_blk(blk);
 
+if (!*name && exp) {
+name = exp->id;
+}
 if (!*name) {
 name = allocated_name = blk_get_attached_dev_id(blk);
 }
diff --git a/block/export/export.c b/block/export/export.c
index d68c5d78ca..192ae79a24 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -60,6 +60,19 @@ BlockExport *blk_exp_find(const char *id)
 return NULL;
 }
 
+BlockExport *blk_exp_find_by_blk(BlockBackend *blk)
+{
+BlockExport *exp;
+
+QLIST_FOREACH(exp, _exports, next) {
+if (exp->blk == blk) {
+return exp;
+}
+}
+
+return NULL;
+}
+
 static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
 {
 int i;
diff --git a/include/block/export.h b/include/block/export.h
index f2fe0f8078..16863d37cf 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -82,6 +82,7 @@ struct BlockExport {
 
 BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
 BlockExport *blk_exp_find(const char *id);
+BlockExport *blk_exp_find_by_blk(BlockBackend *blk);
 void blk_exp_ref(BlockExport *exp);
 void blk_exp_unref(BlockExport *exp);
 void blk_exp_request_shutdown(BlockExport *exp);
diff --git a/stubs/blk-exp-find-by-blk.c b/stubs/blk-exp-find-by-blk.c
new file mode 100644
index 00..2fc1da953b
--- /dev/null
+++ b/stubs/blk-exp-find-by-blk.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+#include "sysemu/block-backend.h"
+#include "block/export.h"
+
+BlockExport *blk_exp_find_by_blk(BlockBackend *blk)
+{
+return NULL;
+}
+
diff --git a/stubs/meson.build b/stubs/meson.build
index 6ff8db71f9..7524a8f90e 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -2,6 +2,7 @@ stub_ss.add(files('bdrv-next-monitor-owned.c'))
 stub_ss.add(files('blk-commit-all.c'))
 stub_ss.add(files('blk-exp-close-all.c'))
 stub_ss.add(files('blk-by-qdev-id.c'))
+stub_ss.add(files('blk-exp-find-by-blk.c'))
 stub_ss.add(files('blockdev-close-all-bdrv-states.c'))
 stub_ss.add(files('change-state-handler.c'))
 stub_ss.add(files('cmos.c'))
-- 
2.34.1

[PATCH v8 3/7] block: make bdrv_find_child() function public

[Vladimir Sementsov-Ogievskiy]

To be reused soon for blockdev-replace functionality.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c  | 13 +
 blockdev.c   | 14 --
 include/block/block_int-io.h |  2 ++
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index f9cf05ddcf..6215ee9f28 100644
--- a/block.c
+++ b/block.c
@@ -8378,6 +8378,19 @@ int bdrv_make_empty(BdrvChild *c, Error **errp)
 return 0;
 }
 
+BdrvChild *bdrv_find_child(BlockDriverState *parent_bs, const char *child_name)
+{
+BdrvChild *child;
+
+QLIST_FOREACH(child, _bs->children, next) {
+if (strcmp(child->name, child_name) == 0) {
+return child;
+}
+}
+
+return NULL;
+}
+
 /*
  * Return the child that @bs acts as an overlay for, and from which data may be
  * copied in COW or COR operations.  Usually this is the backing file.
diff --git a/blockdev.c b/blockdev.c
index a01c62596b..49d0aab356 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3587,20 +3587,6 @@ out:
 aio_context_release(aio_context);
 }
 
-static BdrvChild * GRAPH_RDLOCK
-bdrv_find_child(BlockDriverState *parent_bs, const char *child_name)
-{
-BdrvChild *child;
-
-QLIST_FOREACH(child, _bs->children, next) {
-if (strcmp(child->name, child_name) == 0) {
-return child;
-}
-}
-
-return NULL;
-}
-
 void qmp_x_blockdev_change(const char *parent, const char *child,
const char *node, Error **errp)
 {
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index 34eac72d7a..656037a49d 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -130,6 +130,8 @@ bdrv_co_refresh_total_sectors(BlockDriverState *bs, int64_t 
hint);
 int co_wrapper_mixed_bdrv_rdlock
 bdrv_refresh_total_sectors(BlockDriverState *bs, int64_t hint);
 
+BdrvChild * GRAPH_RDLOCK
+bdrv_find_child(BlockDriverState *parent_bs, const char *child_name);
 BdrvChild *bdrv_cow_child(BlockDriverState *bs);
 BdrvChild *bdrv_filter_child(BlockDriverState *bs);
 BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs);
-- 
2.34.1

[PATCH v8 0/7] blockdev-replace

[Vladimir Sementsov-Ogievskiy]

Hi all!

This series presents a new command blockdev-replace, which helps to
insert/remove filters anywhere in the block graph. It can:

 - replace qdev block-node by qdev-id
 - replace export block-node by export-id
 - replace any child of parent block-node by node-name and child name

So insertions is done in two steps:

1. blockdev_add (create filter node, unparented)

[some parent]  [new filter]
 |   |
 V   V
[some child   ]

2. blockdev-replace (replace child by the filter)

[some parent]
 | 
 V
[new filter]
 |
 V
[some child]

And removal is done in reverse order:

1. blockdev-replace (go back to picture 1)
2. blockdev_del (remove filter node)


Ideally, we to do both operations (add + replace or replace + del) in a
transaction, but that would be another series.

v8: - rebase on master
- add documentation
- also don't use "preallocate" filter in a test, as we don't support
removal of this filter for now. Preallocate filter is rather
unusual, see discussion here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg994945.html

Vladimir Sementsov-Ogievskiy (7):
  block-backend: blk_root(): drop const specifier on return type
  block/export: add blk_by_export_id()
  block: make bdrv_find_child() function public
  qapi: add x-blockdev-replace command
  block: bdrv_get_xdbg_block_graph(): report export ids
  iotests.py: introduce VM.assert_edges_list() method
  iotests: add filter-insertion

 block.c   |  17 ++
 block/block-backend.c |   2 +-
 block/export/export.c |  31 +++
 blockdev.c|  70 --
 include/block/block_int-io.h  |   2 +
 include/block/export.h|   1 +
 include/sysemu/block-backend-global-state.h   |   3 +-
 qapi/block-core.json  |  83 ++
 stubs/blk-by-qdev-id.c|   9 +
 stubs/blk-exp-find-by-blk.c   |   9 +
 stubs/meson.build |   2 +
 tests/qemu-iotests/iotests.py |  17 ++
 tests/qemu-iotests/tests/filter-insertion | 236 ++
 tests/qemu-iotests/tests/filter-insertion.out |   5 +
 14 files changed, 471 insertions(+), 16 deletions(-)
 create mode 100644 stubs/blk-by-qdev-id.c
 create mode 100644 stubs/blk-exp-find-by-blk.c
 create mode 100755 tests/qemu-iotests/tests/filter-insertion
 create mode 100644 tests/qemu-iotests/tests/filter-insertion.out

-- 
2.34.1

[PATCH v8 1/7] block-backend: blk_root(): drop const specifier on return type

[Vladimir Sementsov-Ogievskiy]

We'll need get non-const child pointer for graph modifications in
further commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/block-backend.c   | 2 +-
 include/sysemu/block-backend-global-state.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 39aac1bbce..fdf7597277 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2897,7 +2897,7 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, 
int64_t off_in,
   bytes, read_flags, write_flags);
 }
 
-const BdrvChild *blk_root(BlockBackend *blk)
+BdrvChild *blk_root(BlockBackend *blk)
 {
 GLOBAL_STATE_CODE();
 return blk->root;
diff --git a/include/sysemu/block-backend-global-state.h 
b/include/sysemu/block-backend-global-state.h
index 49c12b0fa9..ccb35546a1 100644
--- a/include/sysemu/block-backend-global-state.h
+++ b/include/sysemu/block-backend-global-state.h
@@ -126,7 +126,7 @@ void blk_set_force_allow_inactivate(BlockBackend *blk);
 bool blk_register_buf(BlockBackend *blk, void *host, size_t size, Error 
**errp);
 void blk_unregister_buf(BlockBackend *blk, void *host, size_t size);
 
-const BdrvChild *blk_root(BlockBackend *blk);
+BdrvChild *blk_root(BlockBackend *blk);
 
 int blk_make_empty(BlockBackend *blk, Error **errp);
 
-- 
2.34.1

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

[David Woodhouse]

On Tue, 2023-10-17 at 12:21 +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 
> 
> > @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error 
> > **errp)
> >  XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
> >  XenBlockVdev *vdev = >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)) {
> 
> I like your optimism that we can handle a million disks. :-)

Heh, yeah. Given that there *is* a limit, setting it lower seemed a bit
arbitrary.

For consoles I picked 100 instead of letting it go all the way to
INT_MAX, and in a patch set soon to be posted I'll do the same for the
xen-net-device as I convert it.

Even with a limit of 100, having that many devices *WITHOUT SPECIFYING
WHICH ONE IS WHICH* seems a bit many!

FWIW I've changed it to check for the existence of the *frontend* nodes
(the ones which are visible to the guest). Currently it checks for the
backend nodes, but those might be in different places.

> I haven't reviewed the Xen part in detail, but the patch looks fine on
> the block layer side.
> 
> Acked-by: Kevin Wolf 

Thanks.


smime.p7s
Description: S/MIME cryptographic signature

[PATCH v6 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 

Signed-off-by: Juan Quintela 
Acked-by: Stefan Hajnoczi 
Reviewed-by: Markus Armbruster 
---
 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 1312c563bb..a48591c049 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -479,3 +479,13 @@ 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
+++ b/migration/block.c
@@ -731,6 

[PATCH v6 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  | 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.
+
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, );
 if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index 6ba5e145ac..e54baf9102 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1603,6 +1603,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 v6 1/5] migration: Print block status when needed

[Juan Quintela]

The new line was only printed when command options were used.  When we
used migration parameters and capabilities, it wasn't.

Signed-off-by: Juan Quintela 
Reviewed-by: Fabiano Rosas 
---
 migration/migration-hmp-cmds.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index d206700a43..a82597f18e 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -30,6 +30,7 @@
 #include "sysemu/runstate.h"
 #include "ui/qemu-spice.h"
 #include "sysemu/sysemu.h"
+#include "options.h"
 #include "migration.h"
 
 static void migration_global_dump(Monitor *mon)
@@ -696,7 +697,6 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict 
*qdict)
 typedef struct HMPMigrationStatus {
 QEMUTimer *timer;
 Monitor *mon;
-bool is_block_migration;
 } HMPMigrationStatus;
 
 static void hmp_migrate_status_cb(void *opaque)
@@ -722,7 +722,7 @@ static void hmp_migrate_status_cb(void *opaque)
 
 timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 
1000);
 } else {
-if (status->is_block_migration) {
+if (migrate_block()) {
 monitor_printf(status->mon, "\n");
 }
 if (info->error_desc) {
@@ -762,7 +762,6 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 
 status = g_malloc0(sizeof(*status));
 status->mon = mon;
-status->is_block_migration = blk || inc;
 status->timer = timer_new_ms(QEMU_CLOCK_REALTIME, 
hmp_migrate_status_cb,
   status);
 timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
-- 
2.41.0

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

[Juan Quintela]

Use blocked-mirror with NBD instead.

Signed-off-by: Juan Quintela 
Acked-by: Stefan Hajnoczi 
Reviewed-by: Thomas Huth 
Reviewed-by: Markus Armbruster 
---
 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 b51136f50a..1312c563bb 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -470,3 +470,12 @@ 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, );
 if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index e54baf9102..16d4602e52 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1608,6 +1608,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 v6 5/5] migration: Deprecate old compression method

[Juan Quintela]

Signed-off-by: Juan Quintela 
Acked-by: Stefan Hajnoczi 
Acked-by: Peter Xu 
Reviewed-by: Markus Armbruster 
---
 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 a48591c049..0df9b738e0 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -489,3 +489,11 @@ 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':  { 'type': 

[PATCH v6 0/5] Migration deprecated parts

[Juan Quintela]

Based on: Message-ID: <20231017083003.15951-1-quint...@redhat.com>
  Migration 20231017 patches

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):
  migration: Print block status when needed
  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  | 36 +
 qapi/migration.json| 93 ++
 migration/block.c  |  3 ++
 migration/migration-hmp-cmds.c | 15 --
 migration/migration.c  | 10 
 migration/options.c| 22 +++-
 6 files changed, 153 insertions(+), 26 deletions(-)

-- 
2.41.0

Re: [PATCH 1/7] hw/virtio/virtio-pmem: Replace impossible check by assertion

[Manos Pitsidianakis]

On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé  wrote:

The get_memory_region() handler is used when (un)plugging the
device, which can only occur *after* it is realized.

virtio_pmem_realize() ensure the instance can not be realized
without 'memdev'. Remove the superfluous check, replacing it
by an assertion.

Signed-off-by: Philippe Mathieu-Daudé 
---
hw/virtio/virtio-pmem.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
index c3512c2dae..cc24812d2e 100644
--- a/hw/virtio/virtio-pmem.c
+++ b/hw/virtio/virtio-pmem.c
@@ -147,10 +147,7 @@ static void virtio_pmem_fill_device_info(const VirtIOPMEM 
*pmem,
static MemoryRegion *virtio_pmem_get_memory_region(VirtIOPMEM *pmem,
   Error **errp)
{
-if (!pmem->memdev) {
-error_setg(errp, "'%s' property must be set", VIRTIO_PMEM_MEMDEV_PROP);
-return NULL;
-}
+assert(pmem->memdev);

return >memdev->mr;
}
--
2.41.0



Reviewed-by: Manos Pitsidianakis 

Re: [PATCH 4/7] hw/scsi/virtio-scsi: Use VIRTIO_SCSI_COMMON() macro

[Manos Pitsidianakis]

On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé  wrote:

Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.

Signed-off-by: Philippe Mathieu-Daudé 
---
hw/scsi/virtio-scsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 45b95ea070..fa53f0902c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)

static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
{
-VirtIOSCSICommon *vs = >parent_obj;
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
SCSIDevice *d;
int rc;

--
2.41.0




Reviewed-by: Manos Pitsidianakis 

Re: [PATCH 3/7] hw/display/virtio-gpu: Use VIRTIO_DEVICE() macro

[Manos Pitsidianakis]

On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé  wrote:

Access QOM parent with the proper QOM VIRTIO_DEVICE() macro.

Signed-off-by: Philippe Mathieu-Daudé 
---
hw/display/virtio-gpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 93857ad523..51cb517999 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1132,7 +1132,7 @@ static void virtio_gpu_ctrl_bh(void *opaque)
VirtIOGPU *g = opaque;
VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);

-vgc->handle_ctrl(>parent_obj.parent_obj, g->ctrl_vq);
+vgc->handle_ctrl(VIRTIO_DEVICE(g), g->ctrl_vq);
}

static void virtio_gpu_handle_cursor(VirtIODevice *vdev, VirtQueue *vq)
--
2.41.0




Reviewed-by: Manos Pitsidianakis 

Re: [PATCH 2/7] hw/block/vhost-user-blk: Use DEVICE() / VIRTIO_DEVICE() macros

[Manos Pitsidianakis]

On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé  wrote:

Access QOM parent with the proper QOM [VIRTIO_]DEVICE() macros.

Signed-off-by: Philippe Mathieu-Daudé 
---
hw/block/vhost-user-blk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..4b37e26120 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -405,7 +405,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent 
event)

static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
{
-DeviceState *dev = >parent_obj.parent_obj;
+DeviceState *dev = DEVICE(s);
int ret;

s->connected = false;
@@ -423,7 +423,7 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, 
Error **errp)
assert(s->connected);

ret = vhost_dev_get_config(>dev, (uint8_t *)>blkcfg,
-   s->parent_obj.config_len, errp);
+   VIRTIO_DEVICE(s)->config_len, errp);
if (ret < 0) {
qemu_chr_fe_disconnect(>chardev);
vhost_dev_cleanup(>dev);
--
2.41.0




Reviewed-by: Manos Pitsidianakis 

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

[Juan Quintela]

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.
>>  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.

>> +"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 v5 1/7] migration: Print block status when needed

[Fabiano Rosas]

Juan Quintela  writes:

> The new line was only printed when command options were used.  When we
> used migration parameters and capabilities, it wasn't.
>
> Signed-off-by: Juan Quintela 

Reviewed-by: Fabiano Rosas 

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

[Vladimir Sementsov-Ogievskiy]

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 
---
  hw/virtio/virtio-pci.c |  9 +
  include/monitor/qdev.h |  1 +
  monitor/monitor.c  |  1 +
  qapi/qdev.json | 22 ++
  softmmu/qdev-monitor.c |  5 +
  5 files changed, 38 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index dd4620462b..f24f8ff03d 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -23,6 +23,7 @@
  #include "hw/boards.h"
  #include "hw/virtio/virtio.h"
  #include "migration/qemu-file-types.h"
+#include "monitor/qdev.h"
  #include "hw/pci/pci.h"
  #include "hw/pci/pci_bus.h"
  #include "hw/qdev-properties.h"
@@ -541,6 +542,10 @@ static uint64_t virtio_pci_config_read(void *opaque, 
hwaddr addr,
  }
  addr -= config;
  
+if (vdev->generation > 0) {

+qdev_config_read_event(DEVICE(proxy));
+}
+
  switch (size) {
  case 1:
  val = virtio_config_readb(vdev, addr);
@@ -1728,6 +1733,10 @@ static uint64_t virtio_pci_device_read(void *opaque, 
hwaddr addr,
  return UINT64_MAX;
  }
  
+if (vdev->generation > 0) {

+qdev_config_read_event(DEVICE(proxy));
+}
+
  switch (size) {
  case 1:
  val = virtio_config_modern_readb(vdev, addr);
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 949a3672cb..f0b0eab07e 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -39,6 +39,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
  const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
  
  void qdev_hotplug_device_on_event(DeviceState *dev);

+void qdev_config_read_event(DeviceState *dev);
  
  DeviceAndPath *qdev_new_device_and_path(DeviceState *dev);
  
diff --git a/monitor/monitor.c b/monitor/monitor.c

index 941f87815a..f8aa91b190 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -315,6 +315,7 @@ static MonitorQAPIEventConf 
monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
  [QAPI_EVENT_QUORUM_FAILURE]= { 1000 * SCALE_MS },
  [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
  [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
+[QAPI_EVENT_X_CONFIG_READ]   = { 300 * SCALE_MS },
  };
  
  /*

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-".

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



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.

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->canonical_path);
+}




--
Best regards,
Vladimir

Re: [PATCH v5 4/7] migration: Deprecate block migration

[Juan Quintela]

Markus Armbruster  wrote:
> Juan Quintela  writes:
>
>>  #
>> +# @deprecated: Member @block-incremental is deprecated. Use
>
> Two spaces between sentences for consistency, please.

Done.  At least here I did the copy and paste right.

>> diff --git a/migration/block.c b/migration/block.c
>> index b60698d6e2..7682f4fbd2 100644
>> --- a/migration/block.c
>> +++ b/migration/block.c
>> @@ -731,6 +731,9 @@ static int block_save_setup(QEMUFile *f, void *opaque)
>>  trace_migration_block_save("setup", block_mig_state.submitted,
>> block_mig_state.transferred);
>>  
>> +warn_report("block migration is deprecated.  Use blockdev-mirror with"
>> +"NBD instead.");
>
>warn_report("block migration is deprecated;"
>" use blockdev-mirror with NBD instead.");

Done.

>> +if (new_caps[MIGRATION_CAPABILITY_BLOCK]) {
>> +warn_report("Block migration is deprecated. "
>> +"Use blockdev-mirror with NBD instead.");
>
> Likewise.
>
>> +}
>>  
>>  #ifndef CONFIG_REPLICATION
>>  if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
>> @@ -1386,6 +1391,8 @@ static void migrate_params_apply(MigrateSetParameters 
>> *params, Error **errp)
>>  }
>>  
>>  if (params->has_block_incremental) {
>> +warn_report("Block migration is deprecated. "
>> +"Use blockdev-mirror with NBD instead.");
>
> Likewise.
>
>>  s->parameters.block_incremental = params->block_incremental;
>>  }
>>  if (params->has_multifd_channels) {
>
> Other than that
> Reviewed-by: Markus Armbruster 

Thanks.

Re: [PATCH v5 3/7] migration: migrate 'blk' command option is deprecated.

[Juan Quintela]

Markus Armbruster  wrote:
> Juan Quintela  writes:
>
>> Use blocked-mirror with NBD instead.
>>
>> Signed-off-by: Juan Quintela 
>> Acked-by: Stefan Hajnoczi 
>> Reviewed-by: Thomas Huth 
>>
>> ---
>>
>> Improve documentation and style (markus)
>> ---
>>  docs/about/deprecated.rst  | 10 ++
>>  qapi/migration.json|  6 --
>>  migration/migration-hmp-cmds.c |  5 +
>>  migration/migration.c  |  5 +
>>  4 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index fc6adf1dea..0149f040b6 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -469,3 +469,13 @@ 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..59a07b50f0 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1527,7 +1527,8 @@
>>  # Features:
>>  #
>>  # @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
>> -# NBD instead.
>> +# NBD instead.  Member @blk is deprecated.  Use blockdev-mirror
>> +# with NBD instead.
>
> Better:
>
># @deprecated: Members @inc and @blk are deprecated.  Use
># blockdev-mirror with NBD instead.

Fixed.

>>  #
>>  # Returns: nothing on success
>>  #
>> @@ -1550,7 +1551,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 fee7079afa..bfeb1a476a 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)
>>  " instead.");
>>  }
>>  
>> +if (blk) {
>> +warn_report("option '-b' is deprecated.  Use 'blockdev-mirror + 
>> NBD'"
>> +" instead.");
>
>warn_report("option '-b' is deprecated;"
>" use blockdev-mirror with NBD instead.");

Fixed (removed the trailing dot)

>> +}
>> +
>>  qmp_migrate(uri, !!blk, blk, !!inc, inc,
>>  false, false, true, resume, );
>>  if (hmp_handle_error(mon, err)) {
>> diff --git a/migration/migration.c b/migration/migration.c
>> index b8b3ba58df..4da7fcfe0f 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1608,6 +1608,11 @@ static bool migrate_prepare(MigrationState *s, bool 
>> blk, bool blk_inc,
>>  " NBD instead");
>>  }
>>  
>> +if (blk) {
>> +warn_report("capability 'blk is deprecated.  Use blockdev-mirror 
>> with"
>> +" NBD instead");
>> +}
>
> Capability?  Isn't this a parameter?
>
> "'blk" lacks a closing single quote.

You are right.  You need to set the 'blk capability for substitution,
but this is a parameter.  My fault.

> I figure we want
>
>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 "
>
> Other than that
> Reviewed-by: Markus Armbruster 

Thanks,

Re: [PATCH 2/4] qapi: introduce device-sync-config

[Vladimir Sementsov-Ogievskiy]

On 17.10.23 17:57, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


Add command to sync config from vhost-user backend to the device. It
may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
triggered interrupt to the guest or just not available (not supported
by vhost-user server).

Signed-off-by: Vladimir Sementsov-Ogievskiy 


[...]


diff --git a/qapi/qdev.json b/qapi/qdev.json
index fa80694735..2468f8bddf 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -315,3 +315,17 @@
  # Since: 8.2
  ##
  { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' }
+
+##
+# @x-device-sync-config:
+#
+# Sync config from backend to the guest.


"Sync" is not a word; "synchronize" is :)


Seems, I learn English from code :)




+#
+# @id: the device's ID or QOM path
+#
+# Returns: Nothing on success
+#  If @id is not a valid device, DeviceNotFound


Why not GenericError?


I just designed the command looking at device_del. device_del reports 
DeviceNotFound in this case. GenericError is OK for me, if you think it's 
better even in this case. I remember now that everything except GenericError is 
not recommended.




+#
+# Since: 8.2
+##
+{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} }


The commit message above and the error message below talk about command
device-sync-config, but you actually name it x-device-sync-config.

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".

We tend to eschew abbreviations in QAPI schema names.
device-synchronize-config is quite a mouthful, though.  What do you
think?


OK for me.

Hmm, could I ask here, is "config" a word?)) device-synchronize-configuration 
would become a precedent, I'm afraid)




diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 19c31446d8..b6da24389f 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error 
**errp)
  return hotplug_handler_get_state(hotplug_ctrl, dev, errp);
  }
  
+int qdev_sync_config(DeviceState *dev, Error **errp)

+{
+DeviceClass *dc = DEVICE_GET_CLASS(dev);
+
+if (!dc->sync_config) {
+error_setg(errp, "device-sync-config is not supported for '%s'",
+   object_get_typename(OBJECT(dev)));
+return -ENOTSUP;
+}
+
+return dc->sync_config(dev, errp);
+}
+
+void qmp_x_device_sync_config(const char *id, Error **errp)
+{
+DeviceState *dev = find_device_state(id, errp);


Not your patch's fault, but here goes anyway: when @id refers to a
non-device, find_device_state() fails with "is not a hotpluggable
device".  "hotpluggable" is misleading.


Hmm. Thanks, OK, I'll rework it somehow in v2.




+if (!dev) {
+return;
+}
+
+qdev_sync_config(dev, errp);
+}
+
  void hmp_device_add(Monitor *mon, const QDict *qdict)
  {
  Error *err = NULL;




--
Best regards,
Vladimir

Re: [PATCH v5 2/7] 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 
>> Signed-off-by: Juan Quintela 
>>
>> ---
>>
>> Improve documentation and style (thanks Markus)
>> ---
>>  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..fee7079afa 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 + 
>> NBD'"
>> +" instead.");
>
> Convention: an error or warning message is a single phrase, with no
> newline or trailing punctuation.  The simplest way to conform to it is
> something like
>
>warn_report("option '-i' is deprecated;"
>" use blockdev-mirror with NBD instead.");

then the trailing dot is not needed, right?

>> +}
>> +
>>  qmp_migrate(uri, !!blk, blk, !!inc, inc,
>>  false, false, true, resume, );
>>  if (hmp_handle_error(mon, err)) {
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 6ba5e145ac..b8b3ba58df 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1603,6 +1603,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");
>
> Likewise.
>
>> +}
>> +
>>  if (resume) {
>>  if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>>  error_setg(errp, "Cannot resume if there is no "
>
> Other than that
> Reviewed-by: Markus Armbruster 

OK, fixing it.

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

[Markus Armbruster]

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 
> ---
>  hw/virtio/virtio-pci.c |  9 +
>  include/monitor/qdev.h |  1 +
>  monitor/monitor.c  |  1 +
>  qapi/qdev.json | 22 ++
>  softmmu/qdev-monitor.c |  5 +
>  5 files changed, 38 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index dd4620462b..f24f8ff03d 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -23,6 +23,7 @@
>  #include "hw/boards.h"
>  #include "hw/virtio/virtio.h"
>  #include "migration/qemu-file-types.h"
> +#include "monitor/qdev.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/qdev-properties.h"
> @@ -541,6 +542,10 @@ static uint64_t virtio_pci_config_read(void *opaque, 
> hwaddr addr,
>  }
>  addr -= config;
>  
> +if (vdev->generation > 0) {
> +qdev_config_read_event(DEVICE(proxy));
> +}
> +
>  switch (size) {
>  case 1:
>  val = virtio_config_readb(vdev, addr);
> @@ -1728,6 +1733,10 @@ static uint64_t virtio_pci_device_read(void *opaque, 
> hwaddr addr,
>  return UINT64_MAX;
>  }
>  
> +if (vdev->generation > 0) {
> +qdev_config_read_event(DEVICE(proxy));
> +}
> +
>  switch (size) {
>  case 1:
>  val = virtio_config_modern_readb(vdev, addr);
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 949a3672cb..f0b0eab07e 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -39,6 +39,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>  const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>  
>  void qdev_hotplug_device_on_event(DeviceState *dev);
> +void qdev_config_read_event(DeviceState *dev);
>  
>  DeviceAndPath *qdev_new_device_and_path(DeviceState *dev);
>  
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 941f87815a..f8aa91b190 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -315,6 +315,7 @@ static MonitorQAPIEventConf 
> monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>  [QAPI_EVENT_QUORUM_FAILURE]= { 1000 * SCALE_MS },
>  [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
>  [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
> +[QAPI_EVENT_X_CONFIG_READ]   = { 300 * SCALE_MS },
>  };
>  
>  /*
> 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.

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".

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

> 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->canonical_path);
> +}

Re: [PATCH 2/4] qapi: introduce device-sync-config

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> Add command to sync config from vhost-user backend to the device. It
> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
> triggered interrupt to the guest or just not available (not supported
> by vhost-user server).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

[...]

> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index fa80694735..2468f8bddf 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -315,3 +315,17 @@
>  # Since: 8.2
>  ##
>  { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' }
> +
> +##
> +# @x-device-sync-config:
> +#
> +# Sync config from backend to the guest.

"Sync" is not a word; "synchronize" is :)

> +#
> +# @id: the device's ID or QOM path
> +#
> +# Returns: Nothing on success
> +#  If @id is not a valid device, DeviceNotFound

Why not GenericError?  

> +#
> +# Since: 8.2
> +##
> +{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} }

The commit message above and the error message below talk about command
device-sync-config, but you actually name it x-device-sync-config.

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".

We tend to eschew abbreviations in QAPI schema names.
device-synchronize-config is quite a mouthful, though.  What do you
think?

> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 19c31446d8..b6da24389f 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error 
> **errp)
>  return hotplug_handler_get_state(hotplug_ctrl, dev, errp);
>  }
>  
> +int qdev_sync_config(DeviceState *dev, Error **errp)
> +{
> +DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +
> +if (!dc->sync_config) {
> +error_setg(errp, "device-sync-config is not supported for '%s'",
> +   object_get_typename(OBJECT(dev)));
> +return -ENOTSUP;
> +}
> +
> +return dc->sync_config(dev, errp);
> +}
> +
> +void qmp_x_device_sync_config(const char *id, Error **errp)
> +{
> +DeviceState *dev = find_device_state(id, errp);

Not your patch's fault, but here goes anyway: when @id refers to a
non-device, find_device_state() fails with "is not a hotpluggable
device".  "hotpluggable" is misleading.

> +if (!dev) {
> +return;
> +}
> +
> +qdev_sync_config(dev, errp);
> +}
> +
>  void hmp_device_add(Monitor *mon, const QDict *qdict)
>  {
>  Error *err = NULL;

Re: deadlock when using iothread during backup_clean()

[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.

> > #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_); \
> > +}  \
> >  aio_poll(ctx_, true);  \
> > +if (unlock && ctx_) {  \
> > +aio_context_acquire(ctx_); \
> > +}  \
> >   

Re: [PATCH 0/7] hw: Few more QOM/QDev cleanups

[Michael S. Tsirkin]

On Tue, Oct 17, 2023 at 04:01:43PM +0200, Philippe Mathieu-Daudé wrote:
> - Remove a pointless check,
> - Use QOM cast macros,
> - Declare QDev links statically using DEFINE_PROP_LINK()

virtio things
Reviewed-by: Michael S. Tsirkin 



> Philippe Mathieu-Daudé (7):
>   hw/virtio/virtio-pmem: Replace impossible check by assertion
>   hw/block/vhost-user-blk: Use DEVICE() / VIRTIO_DEVICE() macros
>   hw/display/virtio-gpu: Use VIRTIO_DEVICE() macro
>   hw/scsi/virtio-scsi: Use VIRTIO_SCSI_COMMON() macro
>   hw/dma: Declare link using static DEFINE_PROP_LINK() macro
>   hw/net: Declare link using static DEFINE_PROP_LINK() macro
>   hw/usb: Declare link using static DEFINE_PROP_LINK() macro
> 
>  hw/block/vhost-user-blk.c |  4 ++--
>  hw/display/virtio-gpu.c   |  2 +-
>  hw/dma/xilinx_axidma.c|  6 ++
>  hw/dma/xlnx-zdma.c|  7 ++-
>  hw/dma/xlnx_csu_dma.c | 13 -
>  hw/net/cadence_gem.c  |  7 ++-
>  hw/scsi/virtio-scsi.c |  2 +-
>  hw/usb/hcd-xhci-sysbus.c  |  4 
>  hw/usb/hcd-xhci.c |  2 ++
>  hw/virtio/virtio-pmem.c   |  5 +
>  10 files changed, 17 insertions(+), 35 deletions(-)
> 
> -- 
> 2.41.0

[PATCH 7/7] hw/usb: Declare link using static DEFINE_PROP_LINK() macro

[Philippe Mathieu-Daudé]

Pull the 'dma' property to the core XHCI type, declare
its link statically using DEFINE_PROP_LINK().

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/usb/hcd-xhci-sysbus.c | 4 
 hw/usb/hcd-xhci.c| 2 ++
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-xhci-sysbus.c b/hw/usb/hcd-xhci-sysbus.c
index faf57b4797..15983d0b96 100644
--- a/hw/usb/hcd-xhci-sysbus.c
+++ b/hw/usb/hcd-xhci-sysbus.c
@@ -60,10 +60,6 @@ static void xhci_sysbus_instance_init(Object *obj)
 object_initialize_child(obj, "xhci-core", >xhci, TYPE_XHCI);
 qdev_alias_all_properties(DEVICE(>xhci), obj);
 
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,
- (Object **)>xhci.dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
 s->xhci.intr_update = NULL;
 s->xhci.intr_raise = xhci_sysbus_intr_raise;
 }
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4b60114207..012a6f3644 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3638,6 +3638,8 @@ static Property xhci_properties[] = {
 DEFINE_PROP_UINT32("p3",XHCIState, numports_3, 4),
 DEFINE_PROP_LINK("host",XHCIState, hostOpaque, TYPE_DEVICE,
  DeviceState *),
+DEFINE_PROP_LINK("dma", XHCIState, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
 DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

[PATCH 6/7] hw/net: Declare link using static DEFINE_PROP_LINK() macro

[Philippe Mathieu-Daudé]

Declare link statically using DEFINE_PROP_LINK().

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/net/cadence_gem.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index f445d8bb5e..37e209cda6 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1654,11 +1654,6 @@ static void gem_init(Object *obj)
   "enet", sizeof(s->regs));
 
 sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);
-
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,
- (Object **)>dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
 }
 
 static const VMStateDescription vmstate_cadence_gem = {
@@ -1691,6 +1686,8 @@ static Property gem_properties[] = {
   num_type2_screeners, 4),
 DEFINE_PROP_UINT16("jumbo-max-len", CadenceGEMState,
jumbo_max_len, 10240),
+DEFINE_PROP_LINK("dma", CadenceGEMState, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
 DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

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

[Markus Armbruster]

Juan Quintela  writes:

> Signed-off-by: Juan Quintela 
> Acked-by: Stefan Hajnoczi 
> Acked-by: Peter Xu 
> ---
>  docs/about/deprecated.rst |  8 
>  qapi/migration.json   | 79 +--
>  migration/options.c   | 13 +++
>  3 files changed, 72 insertions(+), 28 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 5eaf096040..f46baf9ee9 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -489,3 +489,11 @@ 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 c7633b22c0..834506a02b 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.

Two spaces between sentences for consistency, please.

>  #
>  # 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.

Likewise.

>  #
>  # @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.

Likewise.

>  #
>  # @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',
> @@ -885,16 +898,16 @@
>  # @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: 

[PATCH 4/7] hw/scsi/virtio-scsi: Use VIRTIO_SCSI_COMMON() macro

[Philippe Mathieu-Daudé]

Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/scsi/virtio-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 45b95ea070..fa53f0902c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
 
 static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq 
*req)
 {
-VirtIOSCSICommon *vs = >parent_obj;
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
 SCSIDevice *d;
 int rc;
 
-- 
2.41.0

[PATCH 1/7] hw/virtio/virtio-pmem: Replace impossible check by assertion

[Philippe Mathieu-Daudé]

The get_memory_region() handler is used when (un)plugging the
device, which can only occur *after* it is realized.

virtio_pmem_realize() ensure the instance can not be realized
without 'memdev'. Remove the superfluous check, replacing it
by an assertion.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/virtio/virtio-pmem.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
index c3512c2dae..cc24812d2e 100644
--- a/hw/virtio/virtio-pmem.c
+++ b/hw/virtio/virtio-pmem.c
@@ -147,10 +147,7 @@ static void virtio_pmem_fill_device_info(const VirtIOPMEM 
*pmem,
 static MemoryRegion *virtio_pmem_get_memory_region(VirtIOPMEM *pmem,
Error **errp)
 {
-if (!pmem->memdev) {
-error_setg(errp, "'%s' property must be set", VIRTIO_PMEM_MEMDEV_PROP);
-return NULL;
-}
+assert(pmem->memdev);
 
 return >memdev->mr;
 }
-- 
2.41.0

[PATCH 5/7] hw/dma: Declare link using static DEFINE_PROP_LINK() macro

[Philippe Mathieu-Daudé]

Declare link statically using DEFINE_PROP_LINK().

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/dma/xilinx_axidma.c |  6 ++
 hw/dma/xlnx-zdma.c |  7 ++-
 hw/dma/xlnx_csu_dma.c  | 13 -
 3 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 12c90267df..0ae056ed06 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -577,10 +577,6 @@ static void xilinx_axidma_init(Object *obj)
 object_initialize_child(OBJECT(s), "axistream-control-connected-target",
 >rx_control_dev,
 TYPE_XILINX_AXI_DMA_CONTROL_STREAM);
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,
- (Object **)>dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
 
 sysbus_init_irq(sbd, >streams[0].irq);
 sysbus_init_irq(sbd, >streams[1].irq);
@@ -596,6 +592,8 @@ static Property axidma_properties[] = {
  tx_data_dev, TYPE_STREAM_SINK, StreamSink *),
 DEFINE_PROP_LINK("axistream-control-connected", XilinxAXIDMA,
  tx_control_dev, TYPE_STREAM_SINK, StreamSink *),
+DEFINE_PROP_LINK("dma", XilinxAXIDMA, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
index 4eb7f66e9f..84c0083013 100644
--- a/hw/dma/xlnx-zdma.c
+++ b/hw/dma/xlnx-zdma.c
@@ -795,11 +795,6 @@ static void zdma_init(Object *obj)
   TYPE_XLNX_ZDMA, ZDMA_R_MAX * 4);
 sysbus_init_mmio(sbd, >iomem);
 sysbus_init_irq(sbd, >irq_zdma_ch_imr);
-
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,
- (Object **)>dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
 }
 
 static const VMStateDescription vmstate_zdma = {
@@ -817,6 +812,8 @@ static const VMStateDescription vmstate_zdma = {
 
 static Property zdma_props[] = {
 DEFINE_PROP_UINT32("bus-width", XlnxZDMA, cfg.bus_width, 64),
+DEFINE_PROP_LINK("dma", XlnxZDMA, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
index 88002698a1..e89089821a 100644
--- a/hw/dma/xlnx_csu_dma.c
+++ b/hw/dma/xlnx_csu_dma.c
@@ -702,6 +702,10 @@ static Property xlnx_csu_dma_properties[] = {
  * which channel the device is connected to.
  */
 DEFINE_PROP_BOOL("is-dst", XlnxCSUDMA, is_dst, true),
+DEFINE_PROP_LINK("stream-connected-dma", XlnxCSUDMA, tx_dev,
+ TYPE_STREAM_SINK, StreamSink *),
+DEFINE_PROP_LINK("dma", XlnxCSUDMA, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -728,15 +732,6 @@ static void xlnx_csu_dma_init(Object *obj)
 
 memory_region_init(>iomem, obj, TYPE_XLNX_CSU_DMA,
XLNX_CSU_DMA_R_MAX * 4);
-
-object_property_add_link(obj, "stream-connected-dma", TYPE_STREAM_SINK,
- (Object **)>tx_dev,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
-object_property_add_link(obj, "dma", TYPE_MEMORY_REGION,
- (Object **)>dma_mr,
- qdev_prop_allow_set_link_before_realize,
- OBJ_PROP_LINK_STRONG);
 }
 
 static const TypeInfo xlnx_csu_dma_info = {
-- 
2.41.0

[PATCH 3/7] hw/display/virtio-gpu: Use VIRTIO_DEVICE() macro

[Philippe Mathieu-Daudé]

Access QOM parent with the proper QOM VIRTIO_DEVICE() macro.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/display/virtio-gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 93857ad523..51cb517999 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1132,7 +1132,7 @@ static void virtio_gpu_ctrl_bh(void *opaque)
 VirtIOGPU *g = opaque;
 VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
 
-vgc->handle_ctrl(>parent_obj.parent_obj, g->ctrl_vq);
+vgc->handle_ctrl(VIRTIO_DEVICE(g), g->ctrl_vq);
 }
 
 static void virtio_gpu_handle_cursor(VirtIODevice *vdev, VirtQueue *vq)
-- 
2.41.0

[PATCH 2/7] hw/block/vhost-user-blk: Use DEVICE() / VIRTIO_DEVICE() macros

[Philippe Mathieu-Daudé]

Access QOM parent with the proper QOM [VIRTIO_]DEVICE() macros.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/vhost-user-blk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..4b37e26120 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -405,7 +405,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent 
event)
 
 static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
 {
-DeviceState *dev = >parent_obj.parent_obj;
+DeviceState *dev = DEVICE(s);
 int ret;
 
 s->connected = false;
@@ -423,7 +423,7 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, 
Error **errp)
 assert(s->connected);
 
 ret = vhost_dev_get_config(>dev, (uint8_t *)>blkcfg,
-   s->parent_obj.config_len, errp);
+   VIRTIO_DEVICE(s)->config_len, errp);
 if (ret < 0) {
 qemu_chr_fe_disconnect(>chardev);
 vhost_dev_cleanup(>dev);
-- 
2.41.0

[PATCH 0/7] hw: Few more QOM/QDev cleanups

[Philippe Mathieu-Daudé]

- Remove a pointless check,
- Use QOM cast macros,
- Declare QDev links statically using DEFINE_PROP_LINK()

Philippe Mathieu-Daudé (7):
  hw/virtio/virtio-pmem: Replace impossible check by assertion
  hw/block/vhost-user-blk: Use DEVICE() / VIRTIO_DEVICE() macros
  hw/display/virtio-gpu: Use VIRTIO_DEVICE() macro
  hw/scsi/virtio-scsi: Use VIRTIO_SCSI_COMMON() macro
  hw/dma: Declare link using static DEFINE_PROP_LINK() macro
  hw/net: Declare link using static DEFINE_PROP_LINK() macro
  hw/usb: Declare link using static DEFINE_PROP_LINK() macro

 hw/block/vhost-user-blk.c |  4 ++--
 hw/display/virtio-gpu.c   |  2 +-
 hw/dma/xilinx_axidma.c|  6 ++
 hw/dma/xlnx-zdma.c|  7 ++-
 hw/dma/xlnx_csu_dma.c | 13 -
 hw/net/cadence_gem.c  |  7 ++-
 hw/scsi/virtio-scsi.c |  2 +-
 hw/usb/hcd-xhci-sysbus.c  |  4 
 hw/usb/hcd-xhci.c |  2 ++
 hw/virtio/virtio-pmem.c   |  5 +
 10 files changed, 17 insertions(+), 35 deletions(-)

-- 
2.41.0

Re: [PATCH v5 4/7] migration: Deprecate block migration

[Markus Armbruster]

Juan Quintela  writes:

> 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 
>
> Signed-off-by: Juan Quintela 
> Acked-by: Stefan Hajnoczi 
> ---
>  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 0149f040b6..5eaf096040 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -479,3 +479,13 @@ 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 59a07b50f0..c7633b22c0 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

Two spaces between sentences for consistency, please.

> +# 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

Likewise.

> +# 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

Likewise.

> +# 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',
> +  

Re: [PATCH v5 3/7] migration: migrate 'blk' command option is deprecated.

[Markus Armbruster]

Juan Quintela  writes:

> Use blocked-mirror with NBD instead.
>
> Signed-off-by: Juan Quintela 
> Acked-by: Stefan Hajnoczi 
> Reviewed-by: Thomas Huth 
>
> ---
>
> Improve documentation and style (markus)
> ---
>  docs/about/deprecated.rst  | 10 ++
>  qapi/migration.json|  6 --
>  migration/migration-hmp-cmds.c |  5 +
>  migration/migration.c  |  5 +
>  4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index fc6adf1dea..0149f040b6 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -469,3 +469,13 @@ 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..59a07b50f0 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1527,7 +1527,8 @@
>  # Features:
>  #
>  # @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
> -# NBD instead.
> +# NBD instead.  Member @blk is deprecated.  Use blockdev-mirror
> +# with NBD instead.

Better:

   # @deprecated: Members @inc and @blk are deprecated.  Use
   # blockdev-mirror with NBD instead.

>  #
>  # Returns: nothing on success
>  #
> @@ -1550,7 +1551,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 fee7079afa..bfeb1a476a 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)
>  " instead.");
>  }
>  
> +if (blk) {
> +warn_report("option '-b' is deprecated.  Use 'blockdev-mirror + NBD'"
> +" instead.");

   warn_report("option '-b' is deprecated;"
   " use blockdev-mirror with NBD instead.");

> +}
> +
>  qmp_migrate(uri, !!blk, blk, !!inc, inc,
>  false, false, true, resume, );
>  if (hmp_handle_error(mon, err)) {
> diff --git a/migration/migration.c b/migration/migration.c
> index b8b3ba58df..4da7fcfe0f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1608,6 +1608,11 @@ static bool migrate_prepare(MigrationState *s, bool 
> blk, bool blk_inc,
>  " NBD instead");
>  }
>  
> +if (blk) {
> +warn_report("capability 'blk is deprecated.  Use blockdev-mirror 
> with"
> +" NBD instead");
> +}

Capability?  Isn't this a parameter?

"'blk" lacks a closing single quote.

I figure we want

   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 "

Other than that
Reviewed-by: Markus Armbruster 

Re: [PATCH v5 2/7] 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 
> Signed-off-by: Juan Quintela 
>
> ---
>
> Improve documentation and style (thanks Markus)
> ---
>  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..fee7079afa 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 + NBD'"
> +" instead.");

Convention: an error or warning message is a single phrase, with no
newline or trailing punctuation.  The simplest way to conform to it is
something like

   warn_report("option '-i' is deprecated;"
   " use blockdev-mirror with NBD instead.");

> +}
> +
>  qmp_migrate(uri, !!blk, blk, !!inc, inc,
>  false, false, true, resume, );
>  if (hmp_handle_error(mon, err)) {
> diff --git a/migration/migration.c b/migration/migration.c
> index 6ba5e145ac..b8b3ba58df 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1603,6 +1603,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");

Likewise.

> +}
> +
>  if (resume) {
>  if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>  error_setg(errp, "Cannot resume if there is no "

Other than that
Reviewed-by: Markus Armbruster 

Re: [PULL 1/1] virtio-blk: don't start dataplane during the stop of dataplane

[Stefan Hajnoczi]

On Tue, Oct 17, 2023 at 12:02:26PM +0200, Kevin Wolf wrote:
> Am 17.10.2023 um 11:01 hat Fiona Ebner geschrieben:
> > Am 16.10.23 um 21:40 schrieb Stefan Hajnoczi:
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index 39e7f23fab..c2d59389cb 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -1166,7 +1166,7 @@ static void virtio_blk_handle_output(VirtIODevice 
> > > *vdev, VirtQueue *vq)
> > >  {
> > >  VirtIOBlock *s = (VirtIOBlock *)vdev;
> > >  
> > > -if (s->dataplane && !s->dataplane_started) {
> > > +if (s->dataplane && !s->dataplane_started && !s->stopping) {
> > 
> > Isn't 'stopping' a property of VirtIOBlockDataPlane? Here, s is VirtIOBlock.
> 
> Indeed, this patch doesn't even build for me.
> 
> However, even if we wrote !s->dataplane->stopping, would it really be
> right to be handling I/O in the main thread while the dataplane hasn't
> stopped yet? At least without all the multiqueue changes, it's not
> obvious to me that it can't cause problems. Unfortunately, the commit
> message doesn't say anything about why it's safe.

Thanks for pointing these things out, Fiona and Kevin. I've dropped the
patch for now.

Stefan


signature.asc
Description: PGP signature

Re: deadlock when using iothread during backup_clean()

[Fiona Ebner]

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
> #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_); \
> +}  \
>  aio_poll(ctx_, true);  \
> +if (unlock && ctx_) {  \
> +aio_context_acquire(ctx_); \
> +}  \
>  waited_ = true;\
>  }  \
>  } else {   \
> 
> But I think we're actually not very far from removing the AioContext
> lock, so if we can't find an easy fix in the meantime, waiting for that
> could be a realistic option.
> 

Glad to hear :) Do you think it will be in time for QEMU 8.2? Otherwise,
I'll go ahead and send what I have for fixing the deadlocks from this
mail thread in the following days. The stuck guest IO can happen even

Re: [PULL 11/38] tests/qtest: migration-test: Add tests for file-based migration

[Juan Quintela]

Fabiano Rosas  wrote:
> Juan Quintela  writes:
>

[..]

>>>> I am resubmitting with this change.
>>>>
>>>> But I think we need to change this:
>>>>
>>>>>> +g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, 
>>>>>> FILE_TEST_FILENAME);
>>>>>> +size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
>>>>>> +uintptr_t *addr, *p;
>>>>
>>>> I think we should change the test so the file is 64 bits on every
>>>> architecture.
>>>> Then we can cast to void * or uintptr_t as needed.
>>>
>>> Hm, I don't get what you mean here. What needs to be 64 bits?
>>
>> size_t is 32 bits on 32bits host, and 64 bits on 64 bits host.
>> uintprt_t is the same.
>
> Right, I have thought of that when writing this fix yesterday, but I
> dismissed it because I thought we were never have a 32bit host running
> these tests.
>
>> So using explicit sizes:
>>
>> static void file_offset_finish_hook(QTestState *from, QTestState *to,
>> void *opaque)
>> {
>> #if defined(__linux__)
>> g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, 
>> FILE_TEST_FILENAME);
>> size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
>> uint64_t *addr, *p;
>> int fd;
>>
>> fd = open(path, O_RDONLY);
>> g_assert(fd != -1);
>> addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
>> g_assert(addr != MAP_FAILED);
>>
>> /*
>>  * Ensure the skipped offset contains zeros and the migration
>>  * stream starts at the right place.
>>  */
>> p = addr;
>> while (p < (uintprt_t)addr + FILE_TEST_OFFSET) {
>> g_assert(*p == 0);
>>     p++;
>> }
>> g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC);
>>
>> munmap(addr, size);
>> close(fd);
>> #endif
>> }
>>
>> This is completely untested, but it should make sure that we are reading
>> 64bits integers in both 32 and 64 bits hosts, no?
>
> Looks like it. I can give it a try and send an update as a separate
> patch.

Send the update against migration-20231017 please.
That branch is on github and the patches are on the PULL request on the
list.

Thanks, Juan.

Re: [PULL 11/38] tests/qtest: migration-test: Add tests for file-based migration

[Fabiano Rosas]

Juan Quintela  writes:

> Fabiano Rosas  wrote:
>> Juan Quintela  writes:
>>
>>> Fabiano Rosas  wrote:
>>> D> Juan Quintela  writes:

> From: Fabiano Rosas 
>
> Add basic tests for file-based migration.
>
> Note that we cannot use test_precopy_common because that routine
> expects it to be possible to run the migration live. With the file
> transport there is no live migration because we must wait for the
> source to finish writing the migration data to the file before the
> destination can start reading. Add a new migration function
> specifically to handle the file migration.
>
> Reviewed-by: Peter Xu 
> Reviewed-by: Juan Quintela 
> Signed-off-by: Fabiano Rosas 
> Signed-off-by: Juan Quintela 
> Message-ID: <20230712190742.22294-7-faro...@suse.de>
>>>
> +static void file_offset_finish_hook(QTestState *from, QTestState *to,
> +void *opaque)
> +{
> +#if defined(__linux__)
> +g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, 
> FILE_TEST_FILENAME);
> +size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
> +uintptr_t *addr, *p;
> +int fd;
> +
> +fd = open(path, O_RDONLY);
> +g_assert(fd != -1);
> +addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
> +g_assert(addr != MAP_FAILED);
> +
> +/*
> + * Ensure the skipped offset contains zeros and the migration
> + * stream starts at the right place.
> + */
> +p = addr;
> +while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) {
> +g_assert(*p == 0);
> +p++;
> +}
> +g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);

 This truncates to 32-bits, so it breaks on a BE host. We need this:

 -->8--
 From ea0c2d1c988add48d9754891a9fc7f6854a9718a Mon Sep 17 00:00:00 2001
 From: Fabiano Rosas 
 Date: Mon, 16 Oct 2023 15:21:49 -0300
 Subject: [PATCH] fixup! tests/qtest: migration-test: Add tests for 
 file-based
  migration

 ---
  tests/qtest/migration-test.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
 index da02b6d692..e1c110537b 100644
 --- a/tests/qtest/migration-test.c
 +++ b/tests/qtest/migration-test.c
 @@ -1966,7 +1966,7 @@ static void file_offset_finish_hook(QTestState 
 *from, QTestState *to,
  g_assert(*p == 0);
  p++;
  }
 -g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
 +g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC);
  
  munmap(addr, size);
  close(fd);
>>>
>>> I am resubmitting with this change.
>>>
>>> But I think we need to change this:
>>>
> +g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, 
> FILE_TEST_FILENAME);
> +size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
> +uintptr_t *addr, *p;
>>>
>>> I think we should change the test so the file is 64 bits on every
>>> architecture.
>>> Then we can cast to void * or uintptr_t as needed.
>>
>> Hm, I don't get what you mean here. What needs to be 64 bits?
>
> size_t is 32 bits on 32bits host, and 64 bits on 64 bits host.
> uintprt_t is the same.

Right, I have thought of that when writing this fix yesterday, but I
dismissed it because I thought we were never have a 32bit host running
these tests.

> So using explicit sizes:
>
> static void file_offset_finish_hook(QTestState *from, QTestState *to,
> void *opaque)
> {
> #if defined(__linux__)
> g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, 
> FILE_TEST_FILENAME);
> size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
> uint64_t *addr, *p;
> int fd;
>
> fd = open(path, O_RDONLY);
> g_assert(fd != -1);
> addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
> g_assert(addr != MAP_FAILED);
>
> /*
>  * Ensure the skipped offset contains zeros and the migration
>  * stream starts at the right place.
>  */
> p = addr;
> while (p < (uintprt_t)addr + FILE_TEST_OFFSET) {
> g_assert(*p == 0);
> p++;
> }
> g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC);
>
> munmap(addr, size);
> close(fd);
> #endif
> }
>
> This is completely untested, but it should make sure that we are reading
> 64bits integers in both 32 and 64 bits hosts, no?

Looks like it. I can give it a try and send an update as a separate
patch.

Re: [PATCH 2/2] virtio: Drop out of coroutine context in virtio_load()

[Juan Quintela]

Michael Tokarev  wrote:
> 05.09.2023 17:50, Kevin Wolf wrote:
>> virtio_load() as a whole should run in coroutine context because it
>> reads from the migration stream and we don't want this to block.
>> However, it calls virtio_set_features_nocheck() and devices don't
>> expect their .set_features callback to run in a coroutine and therefore
>> call functions that may not be called in coroutine context. To fix this,
>> drop out of coroutine context for calling virtio_set_features_nocheck().
> ...
>> Cc: qemu-sta...@nongnu.org
>> Buglink: https://issues.redhat.com/browse/RHEL-832
>> Signed-off-by: Kevin Wolf 
>
> It looks like this change caused an interesting regression,
>   https://gitlab.com/qemu-project/qemu/-/issues/1933
> at least in -stable.  Can you take a look please?
>
> BTW, Kevin, do you have account @gitlab?

Dunno what is going on here, but failing postcopy is weird.

2023-10-12T06:23:44.354387Z qemu-system-x86_64: warning: TSC frequency mismatch 
between VM (2892749 kHz) and host (279 kHz), and TSC scaling unavailable 
2023-10-12T06:23:44.354538Z qemu-system-x86_64: warning: TSC frequency mismatch 
between VM (2892749 kHz) and host (279 kHz), and TSC scaling unavailable

I hope/guess that the problem is not TSC related?

i.e. does other tests work between this two machines?

Once discarding that, we get on source:

2023-10-12 06:23:43.412+: initiating migration

2023-10-12T06:23:44.362392Z qemu-system-x86_64: failed to save
SaveStateEntry with id(name): 3(ram): -5

So migration was aborted, and -5 is EIO on my system.
So we are having trouble here with a write() somewhere.

Later, Juan.

> Thanks,
>
> /mjt

[PATCH v3 4/6] block/nvme: nvme_process_completion() fix bound for cid

[Vladimir Sementsov-Ogievskiy]

NVMeQueuePair::reqs has length NVME_NUM_REQS, which less than
NVME_QUEUE_SIZE by 1.

Fixes: 1086e95da17050 ("block/nvme: switch to a NVMeRequest freelist")
Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Maksim Davydov 
---
 block/nvme.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index b6e95f0b7e..0faedf3072 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -416,9 +416,10 @@ static bool nvme_process_completion(NVMeQueuePair *q)
 q->cq_phase = !q->cq_phase;
 }
 cid = le16_to_cpu(c->cid);
-if (cid == 0 || cid > NVME_QUEUE_SIZE) {
-warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", "
-"queue size: %u", cid, NVME_QUEUE_SIZE);
+if (cid == 0 || cid > NVME_NUM_REQS) {
+warn_report("NVMe: Unexpected CID in completion queue: %" PRIu32
+", should be within: 1..%u inclusively", cid,
+NVME_NUM_REQS);
 continue;
 }
 trace_nvme_complete_command(s, q->index, cid);
-- 
2.34.1

Re: [PULL 11/38] tests/qtest: migration-test: Add tests for file-based migration

[Juan Quintela]

Fabiano Rosas  wrote:
> Juan Quintela  writes:
>
>> Fabiano Rosas  wrote:
>> D> Juan Quintela  writes:
>>>
 From: Fabiano Rosas 

 Add basic tests for file-based migration.

 Note that we cannot use test_precopy_common because that routine
 expects it to be possible to run the migration live. With the file
 transport there is no live migration because we must wait for the
 source to finish writing the migration data to the file before the
 destination can start reading. Add a new migration function
 specifically to handle the file migration.

 Reviewed-by: Peter Xu 
 Reviewed-by: Juan Quintela 
 Signed-off-by: Fabiano Rosas 
 Signed-off-by: Juan Quintela 
 Message-ID: <20230712190742.22294-7-faro...@suse.de>
>>
 +static void file_offset_finish_hook(QTestState *from, QTestState *to,
 +void *opaque)
 +{
 +#if defined(__linux__)
 +g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, 
 FILE_TEST_FILENAME);
 +size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
 +uintptr_t *addr, *p;
 +int fd;
 +
 +fd = open(path, O_RDONLY);
 +g_assert(fd != -1);
 +addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
 +g_assert(addr != MAP_FAILED);
 +
 +/*
 + * Ensure the skipped offset contains zeros and the migration
 + * stream starts at the right place.
 + */
 +p = addr;
 +while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) {
 +g_assert(*p == 0);
 +p++;
 +}
 +g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
>>>
>>> This truncates to 32-bits, so it breaks on a BE host. We need this:
>>>
>>> -->8--
>>> From ea0c2d1c988add48d9754891a9fc7f6854a9718a Mon Sep 17 00:00:00 2001
>>> From: Fabiano Rosas 
>>> Date: Mon, 16 Oct 2023 15:21:49 -0300
>>> Subject: [PATCH] fixup! tests/qtest: migration-test: Add tests for 
>>> file-based
>>>  migration
>>>
>>> ---
>>>  tests/qtest/migration-test.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>>> index da02b6d692..e1c110537b 100644
>>> --- a/tests/qtest/migration-test.c
>>> +++ b/tests/qtest/migration-test.c
>>> @@ -1966,7 +1966,7 @@ static void file_offset_finish_hook(QTestState *from, 
>>> QTestState *to,
>>>  g_assert(*p == 0);
>>>  p++;
>>>  }
>>> -g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
>>> +g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC);
>>>  
>>>  munmap(addr, size);
>>>  close(fd);
>>
>> I am resubmitting with this change.
>>
>> But I think we need to change this:
>>
 +g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, 
 FILE_TEST_FILENAME);
 +size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
 +uintptr_t *addr, *p;
>>
>> I think we should change the test so the file is 64 bits on every
>> architecture.
>> Then we can cast to void * or uintptr_t as needed.
>
> Hm, I don't get what you mean here. What needs to be 64 bits?

size_t is 32 bits on 32bits host, and 64 bits on 64 bits host.
uintprt_t is the same.

So using explicit sizes:

static void file_offset_finish_hook(QTestState *from, QTestState *to,
void *opaque)
{
#if defined(__linux__)
g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
uint64_t *addr, *p;
int fd;

fd = open(path, O_RDONLY);
g_assert(fd != -1);
addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
g_assert(addr != MAP_FAILED);

/*
 * Ensure the skipped offset contains zeros and the migration
 * stream starts at the right place.
 */
p = addr;
while (p < (uintprt_t)addr + FILE_TEST_OFFSET) {
g_assert(*p == 0);
p++;
}
g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC);

munmap(addr, size);
close(fd);
#endif
}

This is completely untested, but it should make sure that we are reading
64bits integers in both 32 and 64 bits hosts, no?

And yes, for migration, in case of doubt, we use 64bits.  I know it is
unfair for 32 bits host architectures, but they basically don't exist
anymore.

Later, Juan.

Re: [PULL 11/38] tests/qtest: migration-test: Add tests for file-based migration

[Fabiano Rosas]

Juan Quintela  writes:

> Fabiano Rosas  wrote:
> D> Juan Quintela  writes:
>>
>>> From: Fabiano Rosas 
>>>
>>> Add basic tests for file-based migration.
>>>
>>> Note that we cannot use test_precopy_common because that routine
>>> expects it to be possible to run the migration live. With the file
>>> transport there is no live migration because we must wait for the
>>> source to finish writing the migration data to the file before the
>>> destination can start reading. Add a new migration function
>>> specifically to handle the file migration.
>>>
>>> Reviewed-by: Peter Xu 
>>> Reviewed-by: Juan Quintela 
>>> Signed-off-by: Fabiano Rosas 
>>> Signed-off-by: Juan Quintela 
>>> Message-ID: <20230712190742.22294-7-faro...@suse.de>
>
>>> +static void file_offset_finish_hook(QTestState *from, QTestState *to,
>>> +void *opaque)
>>> +{
>>> +#if defined(__linux__)
>>> +g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, 
>>> FILE_TEST_FILENAME);
>>> +size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
>>> +uintptr_t *addr, *p;
>>> +int fd;
>>> +
>>> +fd = open(path, O_RDONLY);
>>> +g_assert(fd != -1);
>>> +addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
>>> +g_assert(addr != MAP_FAILED);
>>> +
>>> +/*
>>> + * Ensure the skipped offset contains zeros and the migration
>>> + * stream starts at the right place.
>>> + */
>>> +p = addr;
>>> +while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) {
>>> +g_assert(*p == 0);
>>> +p++;
>>> +}
>>> +g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
>>
>> This truncates to 32-bits, so it breaks on a BE host. We need this:
>>
>> -->8--
>> From ea0c2d1c988add48d9754891a9fc7f6854a9718a Mon Sep 17 00:00:00 2001
>> From: Fabiano Rosas 
>> Date: Mon, 16 Oct 2023 15:21:49 -0300
>> Subject: [PATCH] fixup! tests/qtest: migration-test: Add tests for file-based
>>  migration
>>
>> ---
>>  tests/qtest/migration-test.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index da02b6d692..e1c110537b 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -1966,7 +1966,7 @@ static void file_offset_finish_hook(QTestState *from, 
>> QTestState *to,
>>  g_assert(*p == 0);
>>  p++;
>>  }
>> -g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
>> +g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC);
>>  
>>  munmap(addr, size);
>>  close(fd);
>
> I am resubmitting with this change.
>
> But I think we need to change this:
>
>>> +g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, 
>>> FILE_TEST_FILENAME);
>>> +size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
>>> +uintptr_t *addr, *p;
>
> I think we should change the test so the file is 64 bits on every
> architecture.
> Then we can cast to void * or uintptr_t as needed.

Hm, I don't get what you mean here. What needs to be 64 bits?

Re: deadlock when using iothread during backup_clean()

[Kevin Wolf]

Am 17.10.2023 um 12:18 hat Fiona Ebner geschrieben:
> Am 06.10.23 um 14:18 schrieb Fiona Ebner:
> > Am 04.10.23 um 19:08 schrieb Vladimir Sementsov-Ogievskiy:
> >> On 28.09.23 11:06, Fiona Ebner wrote:
> >>> For fixing the backup cancel deadlock, I tried the following:
> >>>
>  diff --git a/blockjob.c b/blockjob.c
>  index 58c5d64539..fd6132ebfe 100644
>  --- a/blockjob.c
>  +++ b/blockjob.c
>  @@ -198,7 +198,9 @@ void block_job_remove_all_bdrv(BlockJob *job)
> * one to make sure that such a concurrent access does not attempt
> * to process an already freed BdrvChild.
> */
>  +aio_context_release(job->job.aio_context);
>    bdrv_graph_wrlock(NULL);
>  +aio_context_acquire(job->job.aio_context);
>    while (job->nodes) {
>    GSList *l = job->nodes;
>    BdrvChild *c = l->data;
> >>>
> >>> but it's not enough unfortunately. And I don't just mean with the later
> >>> deadlock during bdrv_close() (via bdrv_cbw_drop()) as mentioned in the
> >>> other mail.
> >>>
> >>> Even when I got lucky and that deadlock didn't trigger by chance or with
> >>> an additional change to try and avoid that one
> >>>
>  diff --git a/block.c b/block.c
>  index e7f349b25c..02d2c4e777 100644
>  --- a/block.c
>  +++ b/block.c
>  @@ -5165,7 +5165,7 @@ static void bdrv_close(BlockDriverState *bs)
>    bs->drv = NULL;
>    }
>    -bdrv_graph_wrlock(NULL);
>  +bdrv_graph_wrlock(bs);
>    QLIST_FOREACH_SAFE(child, >children, next, next) {
>    bdrv_unref_child(bs, child);
>    }
> >>>
> >>> often guest IO would get completely stuck after canceling the backup.
> >>> There's nothing obvious to me in the backtraces at that point, but it
> >>> seems the vCPU and main threads running like usual, while the IO thread
> >>> is stuck in aio_poll(), i.e. never returns from the __ppoll() call. This
> >>> would happen with both, a VirtIO SCSI and a VirtIO block disk and with
> >>> both aio=io_uring and aio=threads.
> >>
> >> When IO is stuck, it may be helpful to look at bs->tracked_requests list
> >> in gdb. If there are requests, each one has field .co, which points to
> >> the coroutine of request.
> > 
> > After the IO was stuck in the guest, I used bdrv_next_all_states() to
> > iterate over the states and there's only the bdrv_raw and the
> > bdrv_host_device. For both, tracked_requests was empty.
> > 
> > What is also very interesting is that the IO isn't always dead
> > immediately. It can be that the fio command still runs with lower speed
> > for a while (sometimes even up to about a minute, but most often about
> > 10-15 seconds or so). During that time, I still can see calls to
> > virtio_scsi_handle_cmd() and blk_aio_write_entry(). Then they suddenly stop.
> > 
> >>>
> >>> I should also mention I'm using
> >>>
>  fio --name=file --size=4k --direct=1 --rw=randwrite --bs=4k
>  --ioengine=psync --numjobs=5 --runtime=6000 --time_based
> >>>
> >>> inside the guest during canceling of the backup.
> > 
> > One single time, it got stuck polling while canceling [0]. I usually
> > need to do the backup+cancel a few times, because the IO doesn't get
> > stuck each time, so this was a subsequent invocation. The reentrancy to
> > virtio_scsi_handle_cmd()/etc. seems strange at a first glance. Can that
> > be normal?
> > 
> 
> 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.

> 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.

But I think we're actually not very far from removing the AioContext
lock, so if we can't find an easy fix in the meantime, waiting for that
could be a realistic option.

> I'm still trying to figure out what happens in the cases where 

[PATCH v5 3/7] migration: migrate 'blk' command option is deprecated.

[Juan Quintela]

Use blocked-mirror with NBD instead.

Signed-off-by: Juan Quintela 
Acked-by: Stefan Hajnoczi 
Reviewed-by: Thomas Huth 

---

Improve documentation and style (markus)
---
 docs/about/deprecated.rst  | 10 ++
 qapi/migration.json|  6 --
 migration/migration-hmp-cmds.c |  5 +
 migration/migration.c  |  5 +
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index fc6adf1dea..0149f040b6 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -469,3 +469,13 @@ 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..59a07b50f0 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1527,7 +1527,8 @@
 # Features:
 #
 # @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
-# NBD instead.
+# NBD instead.  Member @blk is deprecated.  Use blockdev-mirror
+# with NBD instead.
 #
 # Returns: nothing on success
 #
@@ -1550,7 +1551,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 fee7079afa..bfeb1a476a 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)
 " instead.");
 }
 
+if (blk) {
+warn_report("option '-b' is deprecated.  Use 'blockdev-mirror + NBD'"
+" instead.");
+}
+
 qmp_migrate(uri, !!blk, blk, !!inc, inc,
 false, false, true, resume, );
 if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index b8b3ba58df..4da7fcfe0f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1608,6 +1608,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, 
bool blk_inc,
 " NBD instead");
 }
 
+if (blk) {
+warn_report("capability '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 v5 1/7] migration: Print block status when needed

[Juan Quintela]

The new line was only printed when command options were used.  When we
used migration parameters and capabilities, it wasn't.

Signed-off-by: Juan Quintela 
---
 migration/migration-hmp-cmds.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index d206700a43..a82597f18e 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -30,6 +30,7 @@
 #include "sysemu/runstate.h"
 #include "ui/qemu-spice.h"
 #include "sysemu/sysemu.h"
+#include "options.h"
 #include "migration.h"
 
 static void migration_global_dump(Monitor *mon)
@@ -696,7 +697,6 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict 
*qdict)
 typedef struct HMPMigrationStatus {
 QEMUTimer *timer;
 Monitor *mon;
-bool is_block_migration;
 } HMPMigrationStatus;
 
 static void hmp_migrate_status_cb(void *opaque)
@@ -722,7 +722,7 @@ static void hmp_migrate_status_cb(void *opaque)
 
 timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 
1000);
 } else {
-if (status->is_block_migration) {
+if (migrate_block()) {
 monitor_printf(status->mon, "\n");
 }
 if (info->error_desc) {
@@ -762,7 +762,6 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 
 status = g_malloc0(sizeof(*status));
 status->mon = mon;
-status->is_block_migration = blk || inc;
 status->timer = timer_new_ms(QEMU_CLOCK_REALTIME, 
hmp_migrate_status_cb,
   status);
 timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
-- 
2.41.0

[PATCH v5 0/7] Migration deprecated parts

[Juan Quintela]

Based on: Message-ID: <20231017083003.15951-1-quint...@redhat.com>
  Migration 20231017 patches

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 (7):
  migration: Print block status when needed
  migration: migrate 'inc' command option is deprecated.
  migration: migrate 'blk' command option is deprecated.
  migration: Deprecate block migration
  migration: Deprecate old compression method
  [RFC] migration: Make -i/-b an error for hmp and qmp
  [RFC] migration: Remove helpers needed for -i/-b migrate options

 docs/about/deprecated.rst  |  36 +++
 qapi/migration.json| 110 -
 migration/migration.h  |   4 --
 migration/options.h|   6 --
 migration/block.c  |   3 +
 migration/migration-hmp-cmds.c |  18 --
 migration/migration.c  |  35 ---
 migration/options.c|  63 +++
 8 files changed, 165 insertions(+), 110 deletions(-)

-- 
2.41.0

[PATCH v5 2/7] migration: migrate 'inc' command option is deprecated.

[Juan Quintela]

Use blockdev-mirror with NBD instead.

Reviewed-by: Thomas Huth 
Acked-by: Stefan Hajnoczi 
Signed-off-by: Juan Quintela 

---

Improve documentation and style (thanks Markus)
---
 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..fee7079afa 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 + NBD'"
+" instead.");
+}
+
 qmp_migrate(uri, !!blk, blk, !!inc, inc,
 false, false, true, resume, );
 if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index 6ba5e145ac..b8b3ba58df 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1603,6 +1603,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 v5 6/7] [RFC] migration: Make -i/-b an error for hmp and qmp

[Juan Quintela]

[DON'T MERGE]

Block migration is obsolete, users should use blockdev-mirror
instead.
Make it one error to set them.

Signed-off-by: Juan Quintela 
---
 migration/migration-hmp-cmds.c | 13 +++--
 migration/migration.c  | 33 ++---
 2 files changed, 13 insertions(+), 33 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index bfeb1a476a..0acc866c79 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -746,16 +746,17 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 Error *err = NULL;
 
 if (inc) {
-warn_report("option '-i' is deprecated.  Use 'blockdev-mirror + NBD'"
-" instead.");
+monitor_printf(mon, "option '-i' is deprecated.  Use"
+   " 'blockdev-mirror + NBD' instead.");
+return;
 }
 
 if (blk) {
-warn_report("option '-b' is deprecated.  Use 'blockdev-mirror + NBD'"
-" instead.");
+monitor_printf(mon, "option '-b' is deprecated.  Use"
+   " 'blockdev-mirror + NBD' instead.");
+return;
 }
-
-qmp_migrate(uri, !!blk, blk, !!inc, inc,
+qmp_migrate(uri, false, false, false, false,
 false, false, true, resume, );
 if (hmp_handle_error(mon, err)) {
 return;
diff --git a/migration/migration.c b/migration/migration.c
index 4da7fcfe0f..9a695299ba 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1601,16 +1601,16 @@ bool migration_is_blocked(Error **errp)
 static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
 bool resume, Error **errp)
 {
-Error *local_err = NULL;
-
 if (blk_inc) {
-warn_report("parameter 'inc' is deprecated.  Use blockdev-mirror with"
-" NBD instead");
+error_setg(errp, "parameter 'inc' is deprecated.  Use blockdev-mirror"
+   " with NBD instead");
+return false;
 }
 
 if (blk) {
-warn_report("capability 'blk is deprecated.  Use blockdev-mirror with"
-" NBD instead");
+error_setg(errp, "capability 'blk is deprecated.  Use blockdev-mirror"
+   " with NBD instead");
+return false;
 }
 
 if (resume) {
@@ -1662,27 +1662,6 @@ static bool migrate_prepare(MigrationState *s, bool blk, 
bool blk_inc,
 return false;
 }
 
-if (blk || blk_inc) {
-if (migrate_colo()) {
-error_setg(errp, "No disk migration is required in COLO mode");
-return false;
-}
-if (migrate_block() || migrate_block_incremental()) {
-error_setg(errp, "Command options are incompatible with "
-   "current migration capabilities");
-return false;
-}
-if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, _err)) {
-error_propagate(errp, local_err);
-return false;
-}
-s->must_remove_block_options = true;
-}
-
-if (blk_inc) {
-migrate_set_block_incremental(true);
-}
-
 if (migrate_init(s, errp)) {
 return false;
 }
-- 
2.41.0

[PATCH v5 4/7] 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 

Signed-off-by: Juan Quintela 
Acked-by: Stefan Hajnoczi 
---
 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 0149f040b6..5eaf096040 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -479,3 +479,13 @@ 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 59a07b50f0..c7633b22c0 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..7682f4fbd2 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -731,6 +731,9 @@ static int 

[PATCH v5 7/7] [RFC] migration: Remove helpers needed for -i/-b migrate options

[Juan Quintela]

[DON'T MERGE]

We were abusing capabilities and parameters to implement -i/-b.
Previous patch convert that options into one error.  Remove all the
helpers needed to implement them.

Signed-off-by: Juan Quintela 
---
 migration/migration.h |  4 
 migration/options.h   |  6 --
 migration/migration.c |  2 --
 migration/options.c   | 41 -
 4 files changed, 53 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index ae82004892..937a3534c5 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -382,10 +382,6 @@ struct MigrationState {
 /* mutex to protect errp */
 QemuMutex error_mutex;
 
-/* Do we have to clean up -b/-i from old migrate parameters */
-/* This feature is deprecated and will be removed */
-bool must_remove_block_options;
-
 /*
  * Global switch on whether we need to store the global state
  * during migration.
diff --git a/migration/options.h b/migration/options.h
index 237f2d6b4a..e41ea38322 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -62,7 +62,6 @@ bool migrate_tls(void);
 /* capabilities helpers */
 
 bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
-bool migrate_cap_set(int cap, bool value, Error **errp);
 
 /* parameters */
 
@@ -93,14 +92,9 @@ const char *migrate_tls_creds(void);
 const char *migrate_tls_hostname(void);
 uint64_t migrate_xbzrle_cache_size(void);
 
-/* parameters setters */
-
-void migrate_set_block_incremental(bool value);
-
 /* parameters helpers */
 
 bool migrate_params_check(MigrationParameters *params, Error **errp);
 void migrate_params_init(MigrationParameters *params);
-void block_cleanup_parameters(void);
 
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 9a695299ba..9792bd98ca 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1205,7 +1205,6 @@ static void migrate_fd_cleanup(MigrationState *s)
 error_report_err(error_copy(s->error));
 }
 notifier_list_notify(_state_notifiers, s);
-block_cleanup_parameters();
 yank_unregister_instance(MIGRATION_YANK_INSTANCE);
 }
 
@@ -1715,7 +1714,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
"a valid migration protocol");
 migrate_set_state(>state, MIGRATION_STATUS_SETUP,
   MIGRATION_STATUS_FAILED);
-block_cleanup_parameters();
 }
 
 if (local_err) {
diff --git a/migration/options.c b/migration/options.c
index 7cb99a82a5..3be7e35f40 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -631,26 +631,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, 
Error **errp)
 return true;
 }
 
-bool migrate_cap_set(int cap, bool value, Error **errp)
-{
-MigrationState *s = migrate_get_current();
-bool new_caps[MIGRATION_CAPABILITY__MAX];
-
-if (migration_is_running(s->state)) {
-error_setg(errp, QERR_MIGRATION_ACTIVE);
-return false;
-}
-
-memcpy(new_caps, s->capabilities, sizeof(new_caps));
-new_caps[cap] = value;
-
-if (!migrate_caps_check(s->capabilities, new_caps, errp)) {
-return false;
-}
-s->capabilities[cap] = value;
-return true;
-}
-
 MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
 {
 MigrationCapabilityStatusList *head = NULL, **tail = 
@@ -877,29 +857,8 @@ uint64_t migrate_xbzrle_cache_size(void)
 return s->parameters.xbzrle_cache_size;
 }
 
-/* parameter setters */
-
-void migrate_set_block_incremental(bool value)
-{
-MigrationState *s = migrate_get_current();
-
-s->parameters.block_incremental = value;
-}
-
 /* parameters helpers */
 
-void block_cleanup_parameters(void)
-{
-MigrationState *s = migrate_get_current();
-
-if (s->must_remove_block_options) {
-/* setting to false can never fail */
-migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, false, _abort);
-migrate_set_block_incremental(false);
-s->must_remove_block_options = false;
-}
-}
-
 AnnounceParameters *migrate_announce_params(void)
 {
 static AnnounceParameters ap;
-- 
2.41.0

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

[Juan Quintela]

Signed-off-by: Juan Quintela 
Acked-by: Stefan Hajnoczi 
Acked-by: Peter Xu 
---
 docs/about/deprecated.rst |  8 
 qapi/migration.json   | 79 +--
 migration/options.c   | 13 +++
 3 files changed, 72 insertions(+), 28 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 5eaf096040..f46baf9ee9 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -489,3 +489,11 @@ 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 c7633b22c0..834506a02b 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',
@@ -885,16 +898,16 @@
 # @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
@@ -1023,7 +1036,9 @@
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated. Use
-# 

Re: [PULL 00/25] Python patches

[Vladimir Sementsov-Ogievskiy]

On 17.10.23 02:44, John Snow wrote:

On Mon, Oct 16, 2023 at 3:21 PM Stefan Hajnoczi  wrote:


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any 
user-visible changes.


Hi Vladimir: all done!



Thanks!


I've created a MR to backport your changes to the standalone repo
here: https://gitlab.com/qemu-project/python-qemu-qmp/-/merge_requests/30


Looks good to me.


- please give it a quick glance to make sure I didn't butcher anything
in transit.

Thanks,
--js



--
Best regards,
Vladimir

Re: [RFC PATCH v3 40/78] hw/sd/sdhci.c: add fallthrough pseudo-keyword

[Philippe Mathieu-Daudé]

On 13/10/23 10:46, Emmanouil Pitsidianakis wrote:

In preparation of raising -Wimplicit-fallthrough to 5, replace all
fall-through comments with the fallthrough attribute pseudo-keyword.

Signed-off-by: Emmanouil Pitsidianakis 
---
  hw/sd/sdhci.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

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 

> @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error 
> **errp)
>  XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
>  XenBlockVdev *vdev = >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)) {

I like your optimism that we can handle a million disks. :-)

I haven't reviewed the Xen part in detail, but the patch looks fine on
the block layer side.

Acked-by: Kevin Wolf 

Re: deadlock when using iothread during backup_clean()

[Fiona Ebner]

Am 06.10.23 um 14:18 schrieb Fiona Ebner:
> Am 04.10.23 um 19:08 schrieb Vladimir Sementsov-Ogievskiy:
>> On 28.09.23 11:06, Fiona Ebner wrote:
>>> For fixing the backup cancel deadlock, I tried the following:
>>>
 diff --git a/blockjob.c b/blockjob.c
 index 58c5d64539..fd6132ebfe 100644
 --- a/blockjob.c
 +++ b/blockjob.c
 @@ -198,7 +198,9 @@ void block_job_remove_all_bdrv(BlockJob *job)
* one to make sure that such a concurrent access does not attempt
* to process an already freed BdrvChild.
*/
 +aio_context_release(job->job.aio_context);
   bdrv_graph_wrlock(NULL);
 +aio_context_acquire(job->job.aio_context);
   while (job->nodes) {
   GSList *l = job->nodes;
   BdrvChild *c = l->data;
>>>
>>> but it's not enough unfortunately. And I don't just mean with the later
>>> deadlock during bdrv_close() (via bdrv_cbw_drop()) as mentioned in the
>>> other mail.
>>>
>>> Even when I got lucky and that deadlock didn't trigger by chance or with
>>> an additional change to try and avoid that one
>>>
 diff --git a/block.c b/block.c
 index e7f349b25c..02d2c4e777 100644
 --- a/block.c
 +++ b/block.c
 @@ -5165,7 +5165,7 @@ static void bdrv_close(BlockDriverState *bs)
   bs->drv = NULL;
   }
   -bdrv_graph_wrlock(NULL);
 +bdrv_graph_wrlock(bs);
   QLIST_FOREACH_SAFE(child, >children, next, next) {
   bdrv_unref_child(bs, child);
   }
>>>
>>> often guest IO would get completely stuck after canceling the backup.
>>> There's nothing obvious to me in the backtraces at that point, but it
>>> seems the vCPU and main threads running like usual, while the IO thread
>>> is stuck in aio_poll(), i.e. never returns from the __ppoll() call. This
>>> would happen with both, a VirtIO SCSI and a VirtIO block disk and with
>>> both aio=io_uring and aio=threads.
>>
>> When IO is stuck, it may be helpful to look at bs->tracked_requests list
>> in gdb. If there are requests, each one has field .co, which points to
>> the coroutine of request.
> 
> After the IO was stuck in the guest, I used bdrv_next_all_states() to
> iterate over the states and there's only the bdrv_raw and the
> bdrv_host_device. For both, tracked_requests was empty.
> 
> What is also very interesting is that the IO isn't always dead
> immediately. It can be that the fio command still runs with lower speed
> for a while (sometimes even up to about a minute, but most often about
> 10-15 seconds or so). During that time, I still can see calls to
> virtio_scsi_handle_cmd() and blk_aio_write_entry(). Then they suddenly stop.
> 
>>>
>>> I should also mention I'm using
>>>
 fio --name=file --size=4k --direct=1 --rw=randwrite --bs=4k
 --ioengine=psync --numjobs=5 --runtime=6000 --time_based
>>>
>>> inside the guest during canceling of the backup.
> 
> One single time, it got stuck polling while canceling [0]. I usually
> need to do the backup+cancel a few times, because the IO doesn't get
> stuck each time, so this was a subsequent invocation. The reentrancy to
> virtio_scsi_handle_cmd()/etc. seems strange at a first glance. Can that
> be normal?
> 

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()

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.

I'm still trying to figure out what happens in the cases where the guest
IO gets stuck. I should mention that I'm not experiencing the issues
when disabling graph locking. It's rather obvious for the deadlocks, but
I also can't reproduce the issues with stuck guest IO. Did it on top of
QEMU 8.1.1, because stuff like bdrv_schedule_unref() came in later and I
didn't want to mess that up :)

> Best Regards,
> Fiona
> 
> [0]:
> 
>> Thread 3 (Thread 0x7ff7f28946c0 (LWP 1815909) "qemu-system-x86"):
>> #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 

Re: [PATCH v2 3/3] hw/nvme: Add SPDM over DOE support

[Jonathan Cameron via]

On Tue, 17 Oct 2023 15:21:55 +1000
Alistair Francis  wrote:

> From: Wilfred Mallawa 
> 
> Setup Data Object Exchance (DOE) as an extended capability for the NVME
> controller and connect SPDM to it (CMA) to it.
> 
> Signed-off-by: Wilfred Mallawa 
> Signed-off-by: Alistair Francis 
> ---
>  docs/specs/index.rst|   1 +
>  docs/specs/spdm.rst | 114 
>  include/hw/pci/pci_device.h |   5 ++
>  include/hw/pci/pcie_doe.h   |   3 +
>  hw/nvme/ctrl.c  |  53 +
>  5 files changed, 176 insertions(+)
>  create mode 100644 docs/specs/spdm.rst
> 
> diff --git a/docs/specs/index.rst b/docs/specs/index.rst
> index e58be38c41..c398541388 100644
> --- a/docs/specs/index.rst
> +++ b/docs/specs/index.rst
> @@ -24,3 +24,4 @@ guest hardware that is specific to QEMU.
> acpi_erst
> sev-guest-firmware
> fw_cfg
> +   spdm
> diff --git a/docs/specs/spdm.rst b/docs/specs/spdm.rst
> new file mode 100644
> index 00..dfdc3cbb4d
> --- /dev/null
> +++ b/docs/specs/spdm.rst
> @@ -0,0 +1,114 @@
> +==
> +QEMU Security Protocols and Data Models (SPDM) Support
> +==
> +
> +SPDM enables authentication, attestation and key exchange to assist in
> +providing infrastructure security enablement. It's a standard published
> +by the `DMTF`_.
> +
> +QEMU supports connecting to a SPDM Responder implementation. This allows an
> +external application to emulate the SPDM Responder logic for an SPDM device.
> +
> +Setting up a SPDM server
> +
> +
> +When using QEMU with SPDM devices QEMU will connect to a server which
> +implements the SPDM functionality.
> +
> +SPDM-Utils
> +--
> +
> +You can use `SPDM Utils`_ to emulate a Responder.
> +
> +SPDM-Utils is a Linux applications to manage, test and develop devices
> +supporting DMTF Security Protocol and Data Model (SPDM). It is written in 
> Rust
> +and utilises libspdm.
> +
> +To use SPDM-Utils you will need to do the followoing:

Spell check needed. following

> +
> + 1. `Build SPDM Utils`_
> + 2. `Generate the certificates`_
> + 3. `Run it as a server`_
> +
> +spdm-emu
> +
> +
> +You can use `spdm emu`_ to model the
> +SPDM responder.
> +
> +.. code-block:: shell
> +
> +$ cd spdm-emu
> +$ git submodule init; git submodule update --recursive
> +$ mkdir build; cd build
> +$ cmake -DARCH=x64 -DTOOLCHAIN=GCC -DTARGET=Debug -DCRYPTO=openssl ..
> +$ make -j32
> +$ make copy_sample_key # Build certificates, required for SPDM 
> authentication.
> +
> +It is worth noting that the certificates should be in compliance with
> +PCIe r6.1 sec 6.31.3. This means you will need to add the following to
> +openssl.cnf
> +
> +.. code-block::
> +
> +subjectAltName = 
> otherName:2.23.147;UTF8:Vendor=1b36:Device=0010:CC=010802:REV=02:SSVID=1af4:SSID=1100
> +2.23.147 = ASN1:OID:2.23.147
> +
> +and then manually regenerate some certificates with:
> +
> +.. code-block:: shell
> +
> +openssl req -nodes -newkey ec:param.pem -keyout end_responder.key -out 
> end_responder.req -sha384 -batch -subj "/CN=DMTF libspdm ECP384 responder 
> cert"

For these no need to have on oneline maybe some \ ?
to make it easier to read if someone looks at the rst file.

> +openssl x509 -req -in end_responder.req -out end_responder.cert -CA 
> inter.cert -CAkey inter.key -sha384 -days 3650 -set_serial 3 -extensions 
> v3_end -extfile ../openssl.cnf
> +openssl asn1parse -in end_responder.cert -out end_responder.cert.der
> +cat ca.cert.der inter.cert.der end_responder.cert.der > 
> bundle_responder.certchain.der
> +

Otherwise this all looks good to me.

Reviewed-by: Jonathan Cameron 

Re: [PATCH v2 2/3] backends: Initial support for SPDM socket support

[Jonathan Cameron via]

On Tue, 17 Oct 2023 15:21:54 +1000
Alistair Francis  wrote:

> From: Huai-Cheng Kuo 
> 
> SPDM enables authentication, attestation and key exchange to assist in
> providing infrastructure security enablement. It's a standard published
> by the DMTF [1].
> 
> SPDM supports multiple transports, including PCIe DOE and MCTP.
> This patch adds support to QEMU to connect to an external SPDM
> instance.
> 
> SPDM support can be added to any QEMU device by exposing a
> TCP socket to a SPDM server. The server can then implement the SPDM
> decoding/encoding support, generally using libspdm [2].
> 
> This is similar to how the current TPM implementation works and means
> that the heavy lifting of setting up certificate chains, capabilities,
> measurements and complex crypto can be done outside QEMU by a well
> supported and tested library.
> 
> 1: https://www.dmtf.org/standards/SPDM
> 2: https://github.com/DMTF/libspdm
> 
> Signed-off-by: Huai-Cheng Kuo 
> Signed-off-by: Chris Browy 
> Co-developed-by: Jonathan Cameron 
> Signed-off-by: Jonathan Cameron 
> [ Changes by WM
>  - Bug fixes from testing
> ]
> Signed-off-by: Wilfred Mallawa 
> [ Changes by AF:
>  - Convert to be more QEMU-ified
>  - Move to backends as it isn't PCIe specific
> ]
> Signed-off-by: Alistair Francis 

LGTM.  Will be interesting to see how this evolves as we put more
requirements on it.

Given I already signed off, I won't give another tag as that would be
extremely confusing.

Re: [PULL 1/1] virtio-blk: don't start dataplane during the stop of dataplane

[Kevin Wolf]

Am 17.10.2023 um 11:01 hat Fiona Ebner geschrieben:
> Am 16.10.23 um 21:40 schrieb Stefan Hajnoczi:
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 39e7f23fab..c2d59389cb 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -1166,7 +1166,7 @@ static void virtio_blk_handle_output(VirtIODevice 
> > *vdev, VirtQueue *vq)
> >  {
> >  VirtIOBlock *s = (VirtIOBlock *)vdev;
> >  
> > -if (s->dataplane && !s->dataplane_started) {
> > +if (s->dataplane && !s->dataplane_started && !s->stopping) {
> 
> Isn't 'stopping' a property of VirtIOBlockDataPlane? Here, s is VirtIOBlock.

Indeed, this patch doesn't even build for me.

However, even if we wrote !s->dataplane->stopping, would it really be
right to be handling I/O in the main thread while the dataplane hasn't
stopped yet? At least without all the multiqueue changes, it's not
obvious to me that it can't cause problems. Unfortunately, the commit
message doesn't say anything about why it's safe.

Kevin

Re: [PATCH v2 1/3] hw/pci: Add all Data Object Types defined in PCIe r6.0

[Jonathan Cameron via]

On Tue, 17 Oct 2023 15:21:53 +1000
Alistair Francis  wrote:

> Add all of the defined protocols/features from the PCIe-SIG r6.0
> "Table 6-32 PCI-SIG defined Data Object Types (Vendor ID = 0001h)"
> table.
> 
> Signed-off-by: Alistair Francis 
Reviewed-by: Jonathan Cameron 

If you feel like adding the PCIe r6.1 extras you could, but not necessary
at this point though I can see they might become relevant fairly soon,
particularly async messages and the connection ID stuff.

> ---
>  include/hw/pci/pcie_doe.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/hw/pci/pcie_doe.h b/include/hw/pci/pcie_doe.h
> index 87dc17dcef..15d94661f9 100644
> --- a/include/hw/pci/pcie_doe.h
> +++ b/include/hw/pci/pcie_doe.h
> @@ -46,6 +46,8 @@ REG32(PCI_DOE_CAP_STATUS, 0)
>  
>  /* PCI-SIG defined Data Object Types - r6.0 Table 6-32 */
>  #define PCI_SIG_DOE_DISCOVERY   0x00
> +#define PCI_SIG_DOE_CMA 0x01
> +#define PCI_SIG_DOE_SECURED_CMA 0x02
>  
>  #define PCI_DOE_DW_SIZE_MAX (1 << 18)
>  #define PCI_DOE_PROTOCOL_NUM_MAX256

Re: [PULL 1/1] virtio-blk: don't start dataplane during the stop of dataplane

[Fiona Ebner]

Am 16.10.23 um 21:40 schrieb Stefan Hajnoczi:
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 39e7f23fab..c2d59389cb 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1166,7 +1166,7 @@ static void virtio_blk_handle_output(VirtIODevice 
> *vdev, VirtQueue *vq)
>  {
>  VirtIOBlock *s = (VirtIOBlock *)vdev;
>  
> -if (s->dataplane && !s->dataplane_started) {
> +if (s->dataplane && !s->dataplane_started && !s->stopping) {

Isn't 'stopping' a property of VirtIOBlockDataPlane? Here, s is VirtIOBlock.

Best Regards,
Fiona

Re: [PATCH 2/2] virtio: Drop out of coroutine context in virtio_load()

[Kevin Wolf]

Am 17.10.2023 um 07:19 hat Michael Tokarev geschrieben:
> 05.09.2023 17:50, Kevin Wolf wrote:
> > virtio_load() as a whole should run in coroutine context because it
> > reads from the migration stream and we don't want this to block.
> > 
> > However, it calls virtio_set_features_nocheck() and devices don't
> > expect their .set_features callback to run in a coroutine and therefore
> > call functions that may not be called in coroutine context. To fix this,
> > drop out of coroutine context for calling virtio_set_features_nocheck().
> ...
> > Cc: qemu-sta...@nongnu.org
> > Buglink: https://issues.redhat.com/browse/RHEL-832
> > Signed-off-by: Kevin Wolf 
> 
> It looks like this change caused an interesting regression,
>   https://gitlab.com/qemu-project/qemu/-/issues/1933
> at least in -stable.  Can you take a look please?

Huh?! This is an interesting one indeed.

I can't see any direct connection between the patch and this regression.
Random memory corruption is the only explanation I have. But I'm not
sure how this patch could cause it, it's quite simple.

The next step is probably trying to find a simple reproducer on the QEMU
level. And then maybe valgrind or we could get stack traces for the
call to virtio_set_features_nocheck_maybe_co(). Also the stack trace for
the crash and maybe the content of 's' would be interesting - we can ask
the reporter for that, the core dump should be enough for that.

Another potentially interesting question is whether after yielding, the
coroutine is indeed reentered from the aio_co_wake() call in the patch
or if something else wakes it up. If it were the latter, that could
explain memory corruption.

> BTW, Kevin, do you have account @gitlab?

Yes, @kmwolf.

Kevin

[PULL 16/38] migration/rdma: Unfold ram_control_after_iterate()

[Juan Quintela]

Once there:
- Remove unused data parameter
- unfold it in its callers
- change all callers to call qemu_rdma_registration_stop()
- We need to call QIO_CHANNEL_RDMA() after we check for migrate_rdma()

Reviewed-by: Li Zhijian 
Signed-off-by: Juan Quintela 
Message-ID: <20231011203527.9061-4-quint...@redhat.com>
---
 migration/qemu-file.h |  2 --
 migration/rdma.h  |  3 +++
 migration/qemu-file.c | 12 
 migration/ram.c   | 17 ++---
 migration/rdma.c  |  9 -
 5 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index d6a370c569..35e671a01e 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -55,7 +55,6 @@ typedef int (QEMURamSaveFunc)(QEMUFile *f,
   size_t size);
 
 typedef struct QEMUFileHooks {
-QEMURamHookFunc *after_ram_iterate;
 QEMURamHookFunc *hook_ram_load;
 QEMURamSaveFunc *save_page;
 } QEMUFileHooks;
@@ -126,7 +125,6 @@ void qemu_fflush(QEMUFile *f);
 void qemu_file_set_blocking(QEMUFile *f, bool block);
 int qemu_file_get_to_fd(QEMUFile *f, int fd, size_t size);
 
-void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
 void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
 
 /* Whenever this is found in the data stream, the flags
diff --git a/migration/rdma.h b/migration/rdma.h
index 670c67a8cb..c13b94c782 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -25,8 +25,11 @@ void rdma_start_incoming_migration(const char *host_port, 
Error **errp);
 
 #ifdef CONFIG_RDMA
 int qemu_rdma_registration_start(QEMUFile *f, uint64_t flags);
+int qemu_rdma_registration_stop(QEMUFile *f, uint64_t flags);
 #else
 static inline
 int qemu_rdma_registration_start(QEMUFile *f, uint64_t flags) { return 0; }
+static inline
+int qemu_rdma_registration_stop(QEMUFile *f, uint64_t flags) { return 0; }
 #endif
 #endif
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 5e2d73fd68..e7dba2a849 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -298,18 +298,6 @@ void qemu_fflush(QEMUFile *f)
 f->iovcnt = 0;
 }
 
-void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
-{
-int ret = 0;
-
-if (f->hooks && f->hooks->after_ram_iterate) {
-ret = f->hooks->after_ram_iterate(f, flags, NULL);
-if (ret < 0) {
-qemu_file_set_error(f, ret);
-}
-}
-}
-
 void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data)
 {
 if (f->hooks && f->hooks->hook_ram_load) {
diff --git a/migration/ram.c b/migration/ram.c
index 6592431a4e..f1ddc1f9fa 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3065,7 +3065,11 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 if (ret < 0) {
 qemu_file_set_error(f, ret);
 }
-ram_control_after_iterate(f, RAM_CONTROL_SETUP);
+
+ret = qemu_rdma_registration_stop(f, RAM_CONTROL_SETUP);
+if (ret < 0) {
+qemu_file_set_error(f, ret);
+}
 
 migration_ops = g_malloc0(sizeof(MigrationOps));
 migration_ops->ram_save_target_page = ram_save_target_page_legacy;
@@ -3187,7 +3191,10 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
  * Must occur before EOS (or any QEMUFile operation)
  * because of RDMA protocol.
  */
-ram_control_after_iterate(f, RAM_CONTROL_ROUND);
+ret = qemu_rdma_registration_stop(f, RAM_CONTROL_ROUND);
+if (ret < 0) {
+qemu_file_set_error(f, ret);
+}
 
 out:
 if (ret >= 0
@@ -3260,7 +3267,11 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
 qemu_mutex_unlock(>bitmap_mutex);
 
 ram_flush_compressed_data(rs);
-ram_control_after_iterate(f, RAM_CONTROL_FINISH);
+
+int ret = qemu_rdma_registration_stop(f, RAM_CONTROL_FINISH);
+if (ret < 0) {
+qemu_file_set_error(f, ret);
+}
 }
 
 if (ret < 0) {
diff --git a/migration/rdma.c b/migration/rdma.c
index 3d74ad6db0..4b32d375ec 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3878,20 +3878,20 @@ int qemu_rdma_registration_start(QEMUFile *f, uint64_t 
flags)
  * Inform dest that dynamic registrations are done for now.
  * First, flush writes, if any.
  */
-static int qemu_rdma_registration_stop(QEMUFile *f,
-   uint64_t flags, void *data)
+int qemu_rdma_registration_stop(QEMUFile *f, uint64_t flags)
 {
-QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
+QIOChannelRDMA *rioc;
 Error *err = NULL;
 RDMAContext *rdma;
 RDMAControlHeader head = { .len = 0, .repeat = 1 };
 int ret;
 
-if (migration_in_postcopy()) {
+if (!migrate_rdma() || migration_in_postcopy()) {
 return 0;
 }
 
 RCU_READ_LOCK_GUARD();
+rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
 rdma = qatomic_rcu_read(>rdmaout);
 if (!rdma) {
 return -1;
@@ -3999,7 +3999,6 @@ static const QEMUFileHooks rdma_read_hooks = {
 };
 
 static const 

[PULL 30/38] multifd: reset next_packet_len after sending pages

[Juan Quintela]

From: Elena Ufimtseva 

Sometimes multifd sends just sync packet with no pages
(normal_num is 0). In this case the old value is being
preserved and being accounted for while only packet_len
is being transferred.
Reset it to 0 after sending and accounting for.

Signed-off-by: Elena Ufimtseva 
Reviewed-by: Fabiano Rosas 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231011184358.97349-5-elena.ufimts...@oracle.com>
---
 migration/multifd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/multifd.c b/migration/multifd.c
index e6e0013c16..c45f5015f8 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -730,6 +730,7 @@ static void *multifd_send_thread(void *opaque)
p->next_packet_size + p->packet_len);
 stat64_add(_stats.transferred,
p->next_packet_size + p->packet_len);
+p->next_packet_size = 0;
 qemu_mutex_lock(>mutex);
 p->pending_job--;
 qemu_mutex_unlock(>mutex);
-- 
2.41.0

[PULL 32/38] migration/ram: Remove RAMState from xbzrle_cache_zero_page

[Juan Quintela]

From: Fabiano Rosas 

'rs' is not used in that function. It's a leftover from commit
9360447d34 ("ram: Use MigrationStats for statistics").

Reviewed-by: Peter Xu 
Signed-off-by: Fabiano Rosas 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231011184604.32364-4-faro...@suse.de>
---
 migration/ram.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 2bd51d6bb5..535172d9be 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -570,7 +570,6 @@ void mig_throttle_counter_reset(void)
 /**
  * xbzrle_cache_zero_page: insert a zero page in the XBZRLE cache
  *
- * @rs: current RAM state
  * @current_addr: address for the zero page
  *
  * Update the xbzrle cache to reflect a page that's been sent as all 0.
@@ -579,7 +578,7 @@ void mig_throttle_counter_reset(void)
  * As a bonus, if the page wasn't in the cache it gets added so that
  * when a small write is made into the 0'd page it gets XBZRLE sent.
  */
-static void xbzrle_cache_zero_page(RAMState *rs, ram_addr_t current_addr)
+static void xbzrle_cache_zero_page(ram_addr_t current_addr)
 {
 /* We don't care if this fails to allocate a new cache page
  * as long as it updated an old one */
@@ -2146,7 +2145,7 @@ static int ram_save_target_page_legacy(RAMState *rs, 
PageSearchStatus *pss)
  */
 if (rs->xbzrle_started) {
 XBZRLE_cache_lock();
-xbzrle_cache_zero_page(rs, block->offset + offset);
+xbzrle_cache_zero_page(block->offset + offset);
 XBZRLE_cache_unlock();
 }
 return res;
-- 
2.41.0

[PULL 23/38] migration/rdma: Check sooner if we are in postcopy for save_page()

[Juan Quintela]

Reviewed-by: Peter Xu 
Reviewed-by: Li Zhijian 
Signed-off-by: Juan Quintela 
Message-ID: <20231011203527.9061-11-quint...@redhat.com>
---
 migration/rdma.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index c147c94b08..e973579a52 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3240,10 +3240,6 @@ static int qemu_rdma_save_page(QEMUFile *f, ram_addr_t 
block_offset,
 RDMAContext *rdma;
 int ret;
 
-if (migration_in_postcopy()) {
-return RAM_SAVE_CONTROL_NOT_SUPP;
-}
-
 RCU_READ_LOCK_GUARD();
 rdma = qatomic_rcu_read(>rdmaout);
 
@@ -3317,7 +3313,7 @@ err:
 int rdma_control_save_page(QEMUFile *f, ram_addr_t block_offset,
ram_addr_t offset, size_t size)
 {
-if (!migrate_rdma()) {
+if (!migrate_rdma() || migration_in_postcopy()) {
 return RAM_SAVE_CONTROL_NOT_SUPP;
 }
 
-- 
2.41.0

[PULL 20/38] qemu-file: Remove QEMUFileHooks

[Juan Quintela]

The only user was rdma, and its use is gone.

Reviewed-by: Peter Xu 
Reviewed-by: Li Zhijian 
Signed-off-by: Juan Quintela 
Message-ID: <20231011203527.9061-8-quint...@redhat.com>
---
 migration/qemu-file.h | 4 
 migration/qemu-file.c | 6 --
 migration/rdma.c  | 9 -
 3 files changed, 19 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 60510a2819..0b22d8335f 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -36,12 +36,8 @@
 #define RAM_CONTROL_ROUND 1
 #define RAM_CONTROL_FINISH3
 
-typedef struct QEMUFileHooks {
-} QEMUFileHooks;
-
 QEMUFile *qemu_file_new_input(QIOChannel *ioc);
 QEMUFile *qemu_file_new_output(QIOChannel *ioc);
-void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks);
 int qemu_fclose(QEMUFile *f);
 
 /*
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 745eaf7a5b..3fb25148d1 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -38,7 +38,6 @@
 #define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)
 
 struct QEMUFile {
-const QEMUFileHooks *hooks;
 QIOChannel *ioc;
 bool is_writable;
 
@@ -133,11 +132,6 @@ QEMUFile *qemu_file_new_input(QIOChannel *ioc)
 return qemu_file_new_impl(ioc, false);
 }
 
-void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
-{
-f->hooks = hooks;
-}
-
 /*
  * Get last error for stream f with optional Error*
  *
diff --git a/migration/rdma.c b/migration/rdma.c
index f66bd939d7..9883b0a250 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -4003,13 +4003,6 @@ err:
 return -1;
 }
 
-static const QEMUFileHooks rdma_read_hooks = {
-};
-
-static const QEMUFileHooks rdma_write_hooks = {
-};
-
-
 static void qio_channel_rdma_finalize(Object *obj)
 {
 QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(obj);
@@ -4061,7 +4054,6 @@ static QEMUFile *rdma_new_input(RDMAContext *rdma)
 rioc->file = qemu_file_new_input(QIO_CHANNEL(rioc));
 rioc->rdmain = rdma;
 rioc->rdmaout = rdma->return_path;
-qemu_file_set_hooks(rioc->file, _read_hooks);
 
 return rioc->file;
 }
@@ -4073,7 +4065,6 @@ static QEMUFile *rdma_new_output(RDMAContext *rdma)
 rioc->file = qemu_file_new_output(QIO_CHANNEL(rioc));
 rioc->rdmaout = rdma;
 rioc->rdmain = rdma->return_path;
-qemu_file_set_hooks(rioc->file, _write_hooks);
 
 return rioc->file;
 }
-- 
2.41.0

[PULL 26/38] migration/rdma: Remove all "ret" variables that are used only once

[Juan Quintela]

Change code that is:

int ret;
...

ret = foo();
if (ret[ < 0]?) {

to:

if (foo()[ < 0]) {

Reviewed-by: Fabiano Rosas 
Reviewed-by: Li Zhijian 
Signed-off-by: Juan Quintela 
Message-ID: <20231011203527.9061-14-quint...@redhat.com>
---
 migration/rdma.c | 29 -
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 09015fbd1a..2a1852ec7f 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1107,7 +1107,6 @@ err_alloc_pd_cq:
 static int qemu_rdma_alloc_qp(RDMAContext *rdma)
 {
 struct ibv_qp_init_attr attr = { 0 };
-int ret;
 
 attr.cap.max_send_wr = RDMA_SIGNALED_SEND_MAX;
 attr.cap.max_recv_wr = 3;
@@ -1117,8 +1116,7 @@ static int qemu_rdma_alloc_qp(RDMAContext *rdma)
 attr.recv_cq = rdma->recv_cq;
 attr.qp_type = IBV_QPT_RC;
 
-ret = rdma_create_qp(rdma->cm_id, rdma->pd, );
-if (ret < 0) {
+if (rdma_create_qp(rdma->cm_id, rdma->pd, ) < 0) {
 return -1;
 }
 
@@ -1130,8 +1128,8 @@ static int qemu_rdma_alloc_qp(RDMAContext *rdma)
 static bool rdma_support_odp(struct ibv_context *dev)
 {
 struct ibv_device_attr_ex attr = {0};
-int ret = ibv_query_device_ex(dev, NULL, );
-if (ret) {
+
+if (ibv_query_device_ex(dev, NULL, )) {
 return false;
 }
 
@@ -1508,7 +1506,6 @@ static int qemu_rdma_wait_comp_channel(RDMAContext *rdma,
struct ibv_comp_channel *comp_channel)
 {
 struct rdma_cm_event *cm_event;
-int ret;
 
 /*
  * Coroutine doesn't start until migration_fd_process_incoming()
@@ -1544,8 +1541,7 @@ static int qemu_rdma_wait_comp_channel(RDMAContext *rdma,
 }
 
 if (pfds[1].revents) {
-ret = rdma_get_cm_event(rdma->channel, _event);
-if (ret < 0) {
+if (rdma_get_cm_event(rdma->channel, _event) < 0) {
 return -1;
 }
 
@@ -2317,12 +2313,10 @@ static int qemu_rdma_write(RDMAContext *rdma,
 uint64_t current_addr = block_offset + offset;
 uint64_t index = rdma->current_index;
 uint64_t chunk = rdma->current_chunk;
-int ret;
 
 /* If we cannot merge it, we flush the current buffer first. */
 if (!qemu_rdma_buffer_mergeable(rdma, current_addr, len)) {
-ret = qemu_rdma_write_flush(rdma, errp);
-if (ret < 0) {
+if (qemu_rdma_write_flush(rdma, errp) < 0) {
 return -1;
 }
 rdma->current_length = 0;
@@ -2936,7 +2930,6 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
 static int qemu_rdma_drain_cq(RDMAContext *rdma)
 {
 Error *err = NULL;
-int ret;
 
 if (qemu_rdma_write_flush(rdma, ) < 0) {
 error_report_err(err);
@@ -2944,8 +2937,7 @@ static int qemu_rdma_drain_cq(RDMAContext *rdma)
 }
 
 while (rdma->nb_sent) {
-ret = qemu_rdma_block_for_wrid(rdma, RDMA_WRID_RDMA_WRITE, NULL);
-if (ret < 0) {
+if (qemu_rdma_block_for_wrid(rdma, RDMA_WRID_RDMA_WRITE, NULL) < 0) {
 error_report("rdma migration: complete polling error!");
 return -1;
 }
@@ -3323,12 +3315,10 @@ static void rdma_accept_incoming_migration(void 
*opaque);
 static void rdma_cm_poll_handler(void *opaque)
 {
 RDMAContext *rdma = opaque;
-int ret;
 struct rdma_cm_event *cm_event;
 MigrationIncomingState *mis = migration_incoming_get_current();
 
-ret = rdma_get_cm_event(rdma->channel, _event);
-if (ret < 0) {
+if (rdma_get_cm_event(rdma->channel, _event) < 0) {
 error_report("get_cm_event failed %d", errno);
 return;
 }
@@ -4053,14 +4043,11 @@ static QEMUFile *rdma_new_output(RDMAContext *rdma)
 static void rdma_accept_incoming_migration(void *opaque)
 {
 RDMAContext *rdma = opaque;
-int ret;
 QEMUFile *f;
 Error *local_err = NULL;
 
 trace_qemu_rdma_accept_incoming_migration();
-ret = qemu_rdma_accept(rdma);
-
-if (ret < 0) {
+if (qemu_rdma_accept(rdma) < 0) {
 error_report("RDMA ERROR: Migration initialization failed");
 return;
 }
-- 
2.41.0

[PULL 31/38] migration/ram: Refactor precopy ram loading code

[Juan Quintela]

From: Nikolay Borisov 

Extract the ramblock parsing code into a routine that operates on the
sequence of headers from the stream and another the parses the
individual ramblock. This makes ram_load_precopy() easier to
comprehend.

Signed-off-by: Nikolay Borisov 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Peter Xu 
Reviewed-by: Juan Quintela 
Signed-off-by: Fabiano Rosas 
Signed-off-by: Juan Quintela 
Message-ID: <20231011184604.32364-3-faro...@suse.de>
---
 migration/ram.c | 146 +++-
 1 file changed, 82 insertions(+), 64 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index a9bc6ae1f1..2bd51d6bb5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3884,6 +3884,85 @@ void colo_flush_ram_cache(void)
 trace_colo_flush_ram_cache_end();
 }
 
+static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
+{
+int ret = 0;
+/* ADVISE is earlier, it shows the source has the postcopy capability on */
+bool postcopy_advised = migration_incoming_postcopy_advised();
+
+assert(block);
+
+if (!qemu_ram_is_migratable(block)) {
+error_report("block %s should not be migrated !", block->idstr);
+return -EINVAL;
+}
+
+if (length != block->used_length) {
+Error *local_err = NULL;
+
+ret = qemu_ram_resize(block, length, _err);
+if (local_err) {
+error_report_err(local_err);
+}
+}
+/* For postcopy we need to check hugepage sizes match */
+if (postcopy_advised && migrate_postcopy_ram() &&
+block->page_size != qemu_host_page_size) {
+uint64_t remote_page_size = qemu_get_be64(f);
+if (remote_page_size != block->page_size) {
+error_report("Mismatched RAM page size %s "
+ "(local) %zd != %" PRId64, block->idstr,
+ block->page_size, remote_page_size);
+ret = -EINVAL;
+}
+}
+if (migrate_ignore_shared()) {
+hwaddr addr = qemu_get_be64(f);
+if (migrate_ram_is_ignored(block) &&
+block->mr->addr != addr) {
+error_report("Mismatched GPAs for block %s "
+ "%" PRId64 "!= %" PRId64, block->idstr,
+ (uint64_t)addr, (uint64_t)block->mr->addr);
+ret = -EINVAL;
+}
+}
+ret = rdma_block_notification_handle(f, block->idstr);
+if (ret < 0) {
+qemu_file_set_error(f, ret);
+}
+
+return ret;
+}
+
+static int parse_ramblocks(QEMUFile *f, ram_addr_t total_ram_bytes)
+{
+int ret = 0;
+
+/* Synchronize RAM block list */
+while (!ret && total_ram_bytes) {
+RAMBlock *block;
+char id[256];
+ram_addr_t length;
+int len = qemu_get_byte(f);
+
+qemu_get_buffer(f, (uint8_t *)id, len);
+id[len] = 0;
+length = qemu_get_be64(f);
+
+block = qemu_ram_block_by_name(id);
+if (block) {
+ret = parse_ramblock(f, block, length);
+} else {
+error_report("Unknown ramblock \"%s\", cannot accept "
+ "migration", id);
+ret = -EINVAL;
+}
+total_ram_bytes -= length;
+}
+
+return ret;
+}
+
 /**
  * ram_load_precopy: load pages in precopy case
  *
@@ -3898,14 +3977,13 @@ static int ram_load_precopy(QEMUFile *f)
 {
 MigrationIncomingState *mis = migration_incoming_get_current();
 int flags = 0, ret = 0, invalid_flags = 0, len = 0, i = 0;
-/* ADVISE is earlier, it shows the source has the postcopy capability on */
-bool postcopy_advised = migration_incoming_postcopy_advised();
+
 if (!migrate_compress()) {
 invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
 }
 
 while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
-ram_addr_t addr, total_ram_bytes;
+ram_addr_t addr;
 void *host = NULL, *host_bak = NULL;
 uint8_t ch;
 
@@ -3976,67 +4054,7 @@ static int ram_load_precopy(QEMUFile *f)
 
 switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
 case RAM_SAVE_FLAG_MEM_SIZE:
-/* Synchronize RAM block list */
-total_ram_bytes = addr;
-while (!ret && total_ram_bytes) {
-RAMBlock *block;
-char id[256];
-ram_addr_t length;
-
-len = qemu_get_byte(f);
-qemu_get_buffer(f, (uint8_t *)id, len);
-id[len] = 0;
-length = qemu_get_be64(f);
-
-block = qemu_ram_block_by_name(id);
-if (block && !qemu_ram_is_migratable(block)) {
-error_report("block %s should not be migrated !", id);
-ret = -EINVAL;
-} else if (block) {
-if (length != block->used_length) {
-Error *local_err = NULL;
-
-ret = qemu_ram_resize(block, length,
-   

[PULL 25/38] migration/rdma: Declare for index variables local

[Juan Quintela]

Declare all variables that are only used inside a for loop inside the
for statement.

This makes clear that they are not used outside of the for loop.

Reviewed-by: Fabiano Rosas 
Reviewed-by: Li Zhijian 
Signed-off-by: Juan Quintela 
Message-ID: <20231011203527.9061-13-quint...@redhat.com>
---
 migration/rdma.c | 44 ++--
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 5f6a771e8e..09015fbd1a 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -559,10 +559,8 @@ static void rdma_add_block(RDMAContext *rdma, const char 
*block_name,
 local->block = g_new0(RDMALocalBlock, local->nb_blocks + 1);
 
 if (local->nb_blocks) {
-int x;
-
 if (rdma->blockmap) {
-for (x = 0; x < local->nb_blocks; x++) {
+for (int x = 0; x < local->nb_blocks; x++) {
 g_hash_table_remove(rdma->blockmap,
 (void *)(uintptr_t)old[x].offset);
 g_hash_table_insert(rdma->blockmap,
@@ -649,15 +647,12 @@ static void rdma_delete_block(RDMAContext *rdma, 
RDMALocalBlock *block)
 {
 RDMALocalBlocks *local = >local_ram_blocks;
 RDMALocalBlock *old = local->block;
-int x;
 
 if (rdma->blockmap) {
 g_hash_table_remove(rdma->blockmap, (void *)(uintptr_t)block->offset);
 }
 if (block->pmr) {
-int j;
-
-for (j = 0; j < block->nb_chunks; j++) {
+for (int j = 0; j < block->nb_chunks; j++) {
 if (!block->pmr[j]) {
 continue;
 }
@@ -687,7 +682,7 @@ static void rdma_delete_block(RDMAContext *rdma, 
RDMALocalBlock *block)
 block->block_name = NULL;
 
 if (rdma->blockmap) {
-for (x = 0; x < local->nb_blocks; x++) {
+for (int x = 0; x < local->nb_blocks; x++) {
 g_hash_table_remove(rdma->blockmap,
 (void *)(uintptr_t)old[x].offset);
 }
@@ -705,7 +700,7 @@ static void rdma_delete_block(RDMAContext *rdma, 
RDMALocalBlock *block)
 memcpy(local->block + block->index, old + (block->index + 1),
 sizeof(RDMALocalBlock) *
 (local->nb_blocks - (block->index + 1)));
-for (x = block->index; x < local->nb_blocks - 1; x++) {
+for (int x = block->index; x < local->nb_blocks - 1; x++) {
 local->block[x].index--;
 }
 }
@@ -725,7 +720,7 @@ static void rdma_delete_block(RDMAContext *rdma, 
RDMALocalBlock *block)
 local->nb_blocks--;
 
 if (local->nb_blocks && rdma->blockmap) {
-for (x = 0; x < local->nb_blocks; x++) {
+for (int x = 0; x < local->nb_blocks; x++) {
 g_hash_table_insert(rdma->blockmap,
 (void *)(uintptr_t)local->block[x].offset,
 >block[x]);
@@ -828,12 +823,12 @@ static int qemu_rdma_broken_ipv6_kernel(struct 
ibv_context *verbs, Error **errp)
  * Otherwise, there are no guarantees until the bug is fixed in linux.
  */
 if (!verbs) {
-int num_devices, x;
+int num_devices;
 struct ibv_device **dev_list = ibv_get_device_list(_devices);
 bool roce_found = false;
 bool ib_found = false;
 
-for (x = 0; x < num_devices; x++) {
+for (int x = 0; x < num_devices; x++) {
 verbs = ibv_open_device(dev_list[x]);
 /*
  * ibv_open_device() is not documented to set errno.  If
@@ -925,7 +920,6 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, Error 
**errp)
 char port_str[16];
 struct rdma_cm_event *cm_event;
 char ip[40] = "unknown";
-struct rdma_addrinfo *e;
 
 if (rdma->host == NULL || !strcmp(rdma->host, "")) {
 error_setg(errp, "RDMA ERROR: RDMA hostname has not been set");
@@ -957,7 +951,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, Error 
**errp)
 }
 
 /* Try all addresses, saving the first error in @err */
-for (e = res; e != NULL; e = e->ai_next) {
+for (struct rdma_addrinfo *e = res; e != NULL; e = e->ai_next) {
 Error **local_errp = err ? NULL : 
 
 inet_ntop(e->ai_family,
@@ -2777,7 +2771,7 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
 RDMAContext *rdma;
 int ret;
 ssize_t done = 0;
-size_t i, len;
+size_t len;
 
 RCU_READ_LOCK_GUARD();
 rdma = qatomic_rcu_read(>rdmaout);
@@ -2803,7 +2797,7 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
 return -1;
 }
 
-for (i = 0; i < niov; i++) {
+for (int i = 0; i < niov; i++) {
 size_t remaining = iov[i].iov_len;
 uint8_t * data = (void *)iov[i].iov_base;
 while (remaining) {
@@ -2866,7 +2860,7 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
 RDMAControlHeader head;
 int ret;
 ssize_t done = 0;
-size_t i, len;
+size_t len;
 
 

[PULL 34/38] migration/ram: Move xbzrle zero page handling into save_zero_page

[Juan Quintela]

From: Fabiano Rosas 

It makes a bit more sense to have the zero page handling of xbzrle
right where we save the zero page.

Also invert the exit condition to remove one level of indentation
which makes the next patch easier to grasp.

Reviewed-by: Peter Xu 
Signed-off-by: Fabiano Rosas 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231011184604.32364-6-faro...@suse.de>
---
 migration/ram.c | 35 ---
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 2ec28c4507..229cad5c74 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1168,21 +1168,34 @@ static int save_zero_page_to_file(PageSearchStatus 
*pss, RAMBlock *block,
  *
  * Returns the number of pages written.
  *
+ * @rs: current RAM state
  * @pss: current PSS channel
  * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
-static int save_zero_page(PageSearchStatus *pss, RAMBlock *block,
+static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
   ram_addr_t offset)
 {
 int len = save_zero_page_to_file(pss, block, offset);
 
-if (len) {
-stat64_add(_stats.zero_pages, 1);
-ram_transferred_add(len);
-return 1;
+if (!len) {
+return -1;
 }
-return -1;
+
+stat64_add(_stats.zero_pages, 1);
+ram_transferred_add(len);
+
+/*
+ * Must let xbzrle know, otherwise a previous (now 0'd) cached
+ * page would be stale.
+ */
+if (rs->xbzrle_started) {
+XBZRLE_cache_lock();
+xbzrle_cache_zero_page(block->offset + offset);
+XBZRLE_cache_unlock();
+}
+
+return 1;
 }
 
 /*
@@ -2139,16 +2152,8 @@ static int ram_save_target_page_legacy(RAMState *rs, 
PageSearchStatus *pss)
 return 1;
 }
 
-res = save_zero_page(pss, block, offset);
+res = save_zero_page(rs, pss, block, offset);
 if (res > 0) {
-/* Must let xbzrle know, otherwise a previous (now 0'd) cached
- * page would be stale
- */
-if (rs->xbzrle_started) {
-XBZRLE_cache_lock();
-xbzrle_cache_zero_page(block->offset + offset);
-XBZRLE_cache_unlock();
-}
 return res;
 }
 
-- 
2.41.0

[PULL 28/38] migration: check for rate_limit_max for RATE_LIMIT_DISABLED

[Juan Quintela]

From: Elena Ufimtseva 

In migration rate limiting atomic operations are used
to read the rate limit variables and transferred bytes and
they are expensive. Check first if rate_limit_max is equal
to RATE_LIMIT_DISABLED and return false immediately if so.

Note that with this patch we will also will stop flushing
by not calling qemu_fflush() from migration_transferred_bytes()
if the migration rate is not exceeded.
This should be fine since migration thread calls in the loop
migration_update_counters from migration_rate_limit() that
calls the migration_transferred_bytes() and flushes there.

Signed-off-by: Elena Ufimtseva 
Reviewed-by: Fabiano Rosas 
Reviewed-by: Peter Xu 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231011184358.97349-2-elena.ufimts...@oracle.com>
---
 migration/migration-stats.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 84e11e6dd8..4cc989d975 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -24,14 +24,15 @@ bool migration_rate_exceeded(QEMUFile *f)
 return true;
 }
 
+uint64_t rate_limit_max = migration_rate_get();
+if (rate_limit_max == RATE_LIMIT_DISABLED) {
+return false;
+}
+
 uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start);
 uint64_t rate_limit_current = migration_transferred_bytes(f);
 uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
-uint64_t rate_limit_max = stat64_get(_stats.rate_limit_max);
 
-if (rate_limit_max == RATE_LIMIT_DISABLED) {
-return false;
-}
 if (rate_limit_max > 0 && rate_limit_used > rate_limit_max) {
 return true;
 }
-- 
2.41.0

[PULL 36/38] migration/multifd: Remove direct "socket" references

[Juan Quintela]

From: Fabiano Rosas 

We're about to enable support for other transports in multifd, so
remove direct references to sockets.

Signed-off-by: Fabiano Rosas 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231012134343.23757-2-faro...@suse.de>
---
 migration/multifd.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index c45f5015f8..8e9a5ee394 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -510,6 +510,11 @@ static void multifd_send_terminate_threads(Error *err)
 }
 }
 
+static int multifd_send_channel_destroy(QIOChannel *send)
+{
+return socket_send_channel_destroy(send);
+}
+
 void multifd_save_cleanup(void)
 {
 int i;
@@ -532,7 +537,7 @@ void multifd_save_cleanup(void)
 if (p->registered_yank) {
 migration_ioc_unregister_yank(p->c);
 }
-socket_send_channel_destroy(p->c);
+multifd_send_channel_destroy(p->c);
 p->c = NULL;
 qemu_mutex_destroy(>mutex);
 qemu_sem_destroy(>sem);
@@ -890,20 +895,25 @@ static void 
multifd_new_send_channel_cleanup(MultiFDSendParams *p,
 static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
 {
 MultiFDSendParams *p = opaque;
-QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
+QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
 Error *local_err = NULL;
 
 trace_multifd_new_send_channel_async(p->id);
 if (!qio_task_propagate_error(task, _err)) {
-p->c = sioc;
+p->c = ioc;
 qio_channel_set_delay(p->c, false);
 p->running = true;
-if (multifd_channel_connect(p, sioc, local_err)) {
+if (multifd_channel_connect(p, ioc, local_err)) {
 return;
 }
 }
 
-multifd_new_send_channel_cleanup(p, sioc, local_err);
+multifd_new_send_channel_cleanup(p, ioc, local_err);
+}
+
+static void multifd_new_send_channel_create(gpointer opaque)
+{
+socket_send_channel_create(multifd_new_send_channel_async, opaque);
 }
 
 int multifd_save_setup(Error **errp)
@@ -952,7 +962,7 @@ int multifd_save_setup(Error **errp)
 p->write_flags = 0;
 }
 
-socket_send_channel_create(multifd_new_send_channel_async, p);
+multifd_new_send_channel_create(p);
 }
 
 for (i = 0; i < thread_count; i++) {
-- 
2.41.0

[PULL 24/38] migration/rdma: Use i as for index instead of idx

[Juan Quintela]

Once there, all the uses are local to the for, so declare the variable
inside the for statement.

Reviewed-by: Fabiano Rosas 
Reviewed-by: Li Zhijian 
Signed-off-by: Juan Quintela 
Message-ID: <20231011203527.9061-12-quint...@redhat.com>
---
 migration/rdma.c | 49 ++--
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index e973579a52..5f6a771e8e 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2354,7 +2354,6 @@ static int qemu_rdma_write(RDMAContext *rdma,
 static void qemu_rdma_cleanup(RDMAContext *rdma)
 {
 Error *err = NULL;
-int idx;
 
 if (rdma->cm_id && rdma->connected) {
 if ((rdma->errored ||
@@ -2381,12 +2380,12 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
 g_free(rdma->dest_blocks);
 rdma->dest_blocks = NULL;
 
-for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
-if (rdma->wr_data[idx].control_mr) {
+for (int i = 0; i < RDMA_WRID_MAX; i++) {
+if (rdma->wr_data[i].control_mr) {
 rdma->total_registrations--;
-ibv_dereg_mr(rdma->wr_data[idx].control_mr);
+ibv_dereg_mr(rdma->wr_data[i].control_mr);
 }
-rdma->wr_data[idx].control_mr = NULL;
+rdma->wr_data[i].control_mr = NULL;
 }
 
 if (rdma->local_ram_blocks.block) {
@@ -2452,7 +2451,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
 
 static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp)
 {
-int ret, idx;
+int ret;
 
 /*
  * Will be validated against destination's actual capabilities
@@ -2480,18 +2479,17 @@ static int qemu_rdma_source_init(RDMAContext *rdma, 
bool pin_all, Error **errp)
 
 /* Build the hash that maps from offset to RAMBlock */
 rdma->blockmap = g_hash_table_new(g_direct_hash, g_direct_equal);
-for (idx = 0; idx < rdma->local_ram_blocks.nb_blocks; idx++) {
+for (int i = 0; i < rdma->local_ram_blocks.nb_blocks; i++) {
 g_hash_table_insert(rdma->blockmap,
-(void *)(uintptr_t)rdma->local_ram_blocks.block[idx].offset,
->local_ram_blocks.block[idx]);
+(void *)(uintptr_t)rdma->local_ram_blocks.block[i].offset,
+>local_ram_blocks.block[i]);
 }
 
-for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
-ret = qemu_rdma_reg_control(rdma, idx);
+for (int i = 0; i < RDMA_WRID_MAX; i++) {
+ret = qemu_rdma_reg_control(rdma, i);
 if (ret < 0) {
-error_setg(errp,
-   "RDMA ERROR: rdma migration: error registering %d 
control!",
-   idx);
+error_setg(errp, "RDMA ERROR: rdma migration: error "
+   "registering %d control!", i);
 goto err_rdma_source_init;
 }
 }
@@ -2625,16 +2623,16 @@ err_rdma_source_connect:
 static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
 {
 Error *err = NULL;
-int ret, idx;
+int ret;
 struct rdma_cm_id *listen_id;
 char ip[40] = "unknown";
 struct rdma_addrinfo *res, *e;
 char port_str[16];
 int reuse = 1;
 
-for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
-rdma->wr_data[idx].control_len = 0;
-rdma->wr_data[idx].control_curr = NULL;
+for (int i = 0; i < RDMA_WRID_MAX; i++) {
+rdma->wr_data[i].control_len = 0;
+rdma->wr_data[i].control_curr = NULL;
 }
 
 if (!rdma->host || !rdma->host[0]) {
@@ -2723,11 +2721,9 @@ err_dest_init_create_listen_id:
 static void qemu_rdma_return_path_dest_init(RDMAContext *rdma_return_path,
 RDMAContext *rdma)
 {
-int idx;
-
-for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
-rdma_return_path->wr_data[idx].control_len = 0;
-rdma_return_path->wr_data[idx].control_curr = NULL;
+for (int i = 0; i < RDMA_WRID_MAX; i++) {
+rdma_return_path->wr_data[i].control_len = 0;
+rdma_return_path->wr_data[i].control_curr = NULL;
 }
 
 /*the CM channel and CM id is shared*/
@@ -3376,7 +3372,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
 struct rdma_cm_event *cm_event;
 struct ibv_context *verbs;
 int ret;
-int idx;
 
 ret = rdma_get_cm_event(rdma->channel, _event);
 if (ret < 0) {
@@ -3462,10 +3457,10 @@ static int qemu_rdma_accept(RDMAContext *rdma)
 
 qemu_rdma_init_ram_blocks(rdma);
 
-for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
-ret = qemu_rdma_reg_control(rdma, idx);
+for (int i = 0; i < RDMA_WRID_MAX; i++) {
+ret = qemu_rdma_reg_control(rdma, i);
 if (ret < 0) {
-error_report("rdma: error registering %d control", idx);
+error_report("rdma: error registering %d control", i);
 goto err_rdma_dest_wait;
 }
 }
-- 
2.41.0

[PULL 13/38] migration: Non multifd migration don't care about multifd flushes

[Juan Quintela]

RDMA was having trouble because
migrate_multifd_flush_after_each_section() can only be true or false,
but we don't want to send any flush when we are not in multifd
migration.

CC: Fabiano Rosas 
Reviewed-by: Li Zhijian 
Reviewed-by: Peter Xu 
Signed-off-by: Juan Quintela 
Message-ID: <20231011205548.10571-2-quint...@redhat.com>
---
 migration/ram.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index d3d9c8b65b..acb8f95f00 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1395,7 +1395,8 @@ static int find_dirty_block(RAMState *rs, 
PageSearchStatus *pss)
 pss->page = 0;
 pss->block = QLIST_NEXT_RCU(pss->block, next);
 if (!pss->block) {
-if (!migrate_multifd_flush_after_each_section()) {
+if (migrate_multifd() &&
+!migrate_multifd_flush_after_each_section()) {
 QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
 int ret = multifd_send_sync_main(f);
 if (ret < 0) {
@@ -3072,7 +3073,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 return ret;
 }
 
-if (!migrate_multifd_flush_after_each_section()) {
+if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
 qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
 }
 
@@ -3184,7 +3185,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 out:
 if (ret >= 0
 && migration_is_setup_or_active(migrate_get_current()->state)) {
-if (migrate_multifd_flush_after_each_section()) {
+if (migrate_multifd() && migrate_multifd_flush_after_each_section()) {
 ret = 
multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
 if (ret < 0) {
 return ret;
@@ -3261,7 +3262,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
 return ret;
 }
 
-if (!migrate_multifd_flush_after_each_section()) {
+if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
 qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
 }
 qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
@@ -3768,7 +3769,8 @@ int ram_load_postcopy(QEMUFile *f, int channel)
 break;
 case RAM_SAVE_FLAG_EOS:
 /* normal exit */
-if (migrate_multifd_flush_after_each_section()) {
+if (migrate_multifd() &&
+migrate_multifd_flush_after_each_section()) {
 multifd_recv_sync_main();
 }
 break;
@@ -4046,7 +4048,8 @@ static int ram_load_precopy(QEMUFile *f)
 break;
 case RAM_SAVE_FLAG_EOS:
 /* normal exit */
-if (migrate_multifd_flush_after_each_section()) {
+if (migrate_multifd() &&
+migrate_multifd_flush_after_each_section()) {
 multifd_recv_sync_main();
 }
 break;
-- 
2.41.0

[PULL 10/38] tests/qtest/migration: Add a test for the analyze-migration script

[Juan Quintela]

From: Fabiano Rosas 

Add a smoke test that migrates to a file and gives it to the
script. It should catch the most annoying errors such as changes in
the ram flags.

After code has been merged it becomes way harder to figure out what is
causing the script to fail, the person making the change is the most
likely to know right away what the problem is.

Signed-off-by: Fabiano Rosas 
Acked-by: Thomas Huth 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231009184326.15777-7-faro...@suse.de>
---
 tests/qtest/migration-test.c | 60 
 tests/qtest/meson.build  |  2 ++
 2 files changed, 62 insertions(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 8eb2053dbb..cef5081f8c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -66,6 +66,8 @@ static bool got_dst_resume;
  */
 #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
 
+#define ANALYZE_SCRIPT "scripts/analyze-migration.py"
+
 #if defined(__linux__)
 #include 
 #include 
@@ -1501,6 +1503,61 @@ static void test_baddest(void)
 test_migrate_end(from, to, false);
 }
 
+#ifndef _WIN32
+static void test_analyze_script(void)
+{
+MigrateStart args = {
+.opts_source = "-uuid ----",
+};
+QTestState *from, *to;
+g_autofree char *uri = NULL;
+g_autofree char *file = NULL;
+int pid, wstatus;
+const char *python = g_getenv("PYTHON");
+
+if (!python) {
+g_test_skip("PYTHON variable not set");
+return;
+}
+
+/* dummy url */
+if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
+return;
+}
+
+/*
+ * Setting these two capabilities causes the "configuration"
+ * vmstate to include subsections for them. The script needs to
+ * parse those subsections properly.
+ */
+migrate_set_capability(from, "validate-uuid", true);
+migrate_set_capability(from, "x-ignore-shared", true);
+
+file = g_strdup_printf("%s/migfile", tmpfs);
+uri = g_strdup_printf("exec:cat > %s", file);
+
+migrate_ensure_converge(from);
+migrate_qmp(from, uri, "{}");
+wait_for_migration_complete(from);
+
+pid = fork();
+if (!pid) {
+close(1);
+open("/dev/null", O_WRONLY);
+execl(python, python, ANALYZE_SCRIPT, "-f", file, NULL);
+g_assert_not_reached();
+}
+
+g_assert(waitpid(pid, , 0) == pid);
+if (WIFEXITED(wstatus) && WEXITSTATUS(wstatus) != 0) {
+g_test_message("Failed to analyze the migration stream");
+g_test_fail();
+}
+test_migrate_end(from, to, false);
+cleanup("migfile");
+}
+#endif
+
 static void test_precopy_common(MigrateCommon *args)
 {
 QTestState *from, *to;
@@ -2837,6 +2894,9 @@ int main(int argc, char **argv)
 }
 
 qtest_add_func("/migration/bad_dest", test_baddest);
+#ifndef _WIN32
+qtest_add_func("/migration/analyze-script", test_analyze_script);
+#endif
 qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
 qtest_add_func("/migration/precopy/unix/xbzrle", test_precopy_unix_xbzrle);
 /*
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 66795cfcd2..d6022ebd64 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -357,6 +357,8 @@ foreach dir : target_dirs
 test_deps += [qsd]
   endif
 
+  qtest_env.set('PYTHON', python.full_path())
+
   foreach test : target_qtests
 # Executables are shared across targets, declare them only the first time 
we
 # encounter them
-- 
2.41.0

[PULL 02/38] migration: Use g_autofree to simplify ram_dirty_bitmap_reload()

[Juan Quintela]

From: Philippe Mathieu-Daudé 

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Markus Armbruster 
Reviewed-by: Fabiano Rosas 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231011023627.86691-1-phi...@linaro.org>
---
 migration/ram.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 2f5ce4d60b..24d91de8b3 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4159,7 +4159,8 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock 
*block)
 int ret = -EINVAL;
 /* from_dst_file is always valid because we're within rp_thread */
 QEMUFile *file = s->rp_state.from_dst_file;
-unsigned long *le_bitmap, nbits = block->used_length >> TARGET_PAGE_BITS;
+g_autofree unsigned long *le_bitmap = NULL;
+unsigned long nbits = block->used_length >> TARGET_PAGE_BITS;
 uint64_t local_size = DIV_ROUND_UP(nbits, 8);
 uint64_t size, end_mark;
 RAMState *rs = ram_state;
@@ -4188,8 +4189,7 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock 
*block)
 error_report("%s: ramblock '%s' bitmap size mismatch "
  "(0x%"PRIx64" != 0x%"PRIx64")", __func__,
  block->idstr, size, local_size);
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 
 size = qemu_get_buffer(file, (uint8_t *)le_bitmap, local_size);
@@ -4200,15 +4200,13 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock 
*block)
 error_report("%s: read bitmap failed for ramblock '%s': %d"
  " (size 0x%"PRIx64", got: 0x%"PRIx64")",
  __func__, block->idstr, ret, local_size, size);
-ret = -EIO;
-goto out;
+return -EIO;
 }
 
 if (end_mark != RAMBLOCK_RECV_BITMAP_ENDING) {
 error_report("%s: ramblock '%s' end mark incorrect: 0x%"PRIx64,
  __func__, block->idstr, end_mark);
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 
 /*
@@ -4240,10 +4238,7 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock 
*block)
  */
 migration_rp_kick(s);
 
-ret = 0;
-out:
-g_free(le_bitmap);
-return ret;
+return 0;
 }
 
 static int ram_resume_prepare(MigrationState *s, void *opaque)
-- 
2.41.0

[PULL 06/38] migration: Fix analyze-migration.py 'configuration' parsing

[Juan Quintela]

From: Fabiano Rosas 

The 'configuration' state subsections are currently not being parsed
and the script fails when analyzing an aarch64 stream:

Traceback (most recent call last):
  File "./scripts/analyze-migration.py", line 625, in 
dump.read(dump_memory = args.memory)
  File "./scripts/analyze-migration.py", line 571, in read
raise Exception("Unknown section type: %d" % section_type)
Exception: Unknown section type: 5

Signed-off-by: Fabiano Rosas 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231009184326.15777-3-faro...@suse.de>
---
 scripts/analyze-migration.py | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index 082424558b..24687db497 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -261,12 +261,21 @@ def getDict(self):
 
 
 class ConfigurationSection(object):
-def __init__(self, file):
+def __init__(self, file, desc):
 self.file = file
+self.desc = desc
 
 def read(self):
-name_len = self.file.read32()
-name = self.file.readstr(len = name_len)
+if self.desc:
+version_id = self.desc['version']
+section = VMSDSection(self.file, version_id, self.desc,
+  'configuration')
+section.read()
+else:
+# backward compatibility for older streams that don't have
+# the configuration section in the json
+name_len = self.file.read32()
+name = self.file.readstr(len = name_len)
 
 class VMSDFieldGeneric(object):
 def __init__(self, desc, file):
@@ -532,7 +541,8 @@ def read(self, desc_only = False, dump_memory = False, 
write_memory = False):
 if section_type == self.QEMU_VM_EOF:
 break
 elif section_type == self.QEMU_VM_CONFIGURATION:
-section = ConfigurationSection(file)
+config_desc = self.vmsd_desc.get('configuration')
+section = ConfigurationSection(file, config_desc)
 section.read()
 elif section_type == self.QEMU_VM_SECTION_START or section_type == 
self.QEMU_VM_SECTION_FULL:
 section_id = file.read32()
-- 
2.41.0

[PULL 21/38] migration/rdma: Move rdma constants from qemu-file.h to rdma.h

[Juan Quintela]

Reviewed-by: Peter Xu 
Reviewed-by: Li Zhijian 
Signed-off-by: Juan Quintela 
Message-ID: <20231011203527.9061-9-quint...@redhat.com>
---
 migration/qemu-file.h | 17 -
 migration/rdma.h  | 16 
 migration/ram.c   |  2 +-
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 0b22d8335f..a29c37b0d0 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -29,13 +29,6 @@
 #include "exec/cpu-common.h"
 #include "io/channel.h"
 
-/*
- * Constants used by ram_control_* hooks
- */
-#define RAM_CONTROL_SETUP 0
-#define RAM_CONTROL_ROUND 1
-#define RAM_CONTROL_FINISH3
-
 QEMUFile *qemu_file_new_input(QIOChannel *ioc);
 QEMUFile *qemu_file_new_output(QIOChannel *ioc);
 int qemu_fclose(QEMUFile *f);
@@ -101,16 +94,6 @@ void qemu_fflush(QEMUFile *f);
 void qemu_file_set_blocking(QEMUFile *f, bool block);
 int qemu_file_get_to_fd(QEMUFile *f, int fd, size_t size);
 
-/* Whenever this is found in the data stream, the flags
- * will be passed to ram_control_load_hook in the incoming-migration
- * side. This lets before_ram_iterate/after_ram_iterate add
- * transport-specific sections to the RAM migration data.
- */
-#define RAM_SAVE_FLAG_HOOK 0x80
-
-#define RAM_SAVE_CONTROL_NOT_SUPP -1000
-#define RAM_SAVE_CONTROL_DELAYED  -2000
-
 QIOChannel *qemu_file_get_ioc(QEMUFile *file);
 
 #endif
diff --git a/migration/rdma.h b/migration/rdma.h
index 09a16c1e3c..1ff3718a76 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -24,6 +24,22 @@ void rdma_start_outgoing_migration(void *opaque, const char 
*host_port,
 
 void rdma_start_incoming_migration(const char *host_port, Error **errp);
 
+/*
+ * Constants used by rdma return codes
+ */
+#define RAM_CONTROL_SETUP 0
+#define RAM_CONTROL_ROUND 1
+#define RAM_CONTROL_FINISH3
+
+/*
+ * Whenever this is found in the data stream, the flags
+ * will be passed to rdma functions in the incoming-migration
+ * side.
+ */
+#define RAM_SAVE_FLAG_HOOK 0x80
+
+#define RAM_SAVE_CONTROL_NOT_SUPP -1000
+#define RAM_SAVE_CONTROL_DELAYED  -2000
 
 #ifdef CONFIG_RDMA
 int qemu_rdma_registration_handle(QEMUFile *f);
diff --git a/migration/ram.c b/migration/ram.c
index 3b4b09f6ff..6a4aed2a75 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -89,7 +89,7 @@
 #define RAM_SAVE_FLAG_EOS  0x10
 #define RAM_SAVE_FLAG_CONTINUE 0x20
 #define RAM_SAVE_FLAG_XBZRLE   0x40
-/* 0x80 is reserved in qemu-file.h for RAM_SAVE_FLAG_HOOK */
+/* 0x80 is reserved in rdma.h for RAM_SAVE_FLAG_HOOK */
 #define RAM_SAVE_FLAG_COMPRESS_PAGE0x100
 #define RAM_SAVE_FLAG_MULTIFD_FLUSH0x200
 /* We can't use any flag that is bigger than 0x200 */
-- 
2.41.0

[PULL 29/38] multifd: fix counters in multifd_send_thread

[Juan Quintela]

From: Elena Ufimtseva 

Previous commit cbec7eb76879d419e7dbf531ee2506ec0722e825
"migration/multifd: Compute transferred bytes correctly"
removed accounting for packet_len in non-rdma
case, but the next_packet_size only accounts for pages, not for
the header packet (normal_pages * PAGE_SIZE) that is being sent
as iov[0]. The packet_len part should be added to account for
the size of MultiFDPacket and the array of the offsets.

Signed-off-by: Elena Ufimtseva 
Reviewed-by: Fabiano Rosas 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231011184358.97349-4-elena.ufimts...@oracle.com>
---
 migration/multifd.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 0f6b203877..e6e0013c16 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -714,8 +714,6 @@ static void *multifd_send_thread(void *opaque)
 if (ret != 0) {
 break;
 }
-stat64_add(_stats.multifd_bytes, p->packet_len);
-stat64_add(_stats.transferred, p->packet_len);
 } else {
 /* Send header using the same writev call */
 p->iov[0].iov_len = p->packet_len;
@@ -728,8 +726,10 @@ static void *multifd_send_thread(void *opaque)
 break;
 }
 
-stat64_add(_stats.multifd_bytes, p->next_packet_size);
-stat64_add(_stats.transferred, p->next_packet_size);
+stat64_add(_stats.multifd_bytes,
+   p->next_packet_size + p->packet_len);
+stat64_add(_stats.transferred,
+   p->next_packet_size + p->packet_len);
 qemu_mutex_lock(>mutex);
 p->pending_job--;
 qemu_mutex_unlock(>mutex);
-- 
2.41.0

[PULL 27/38] migration: Improve json and formatting

[Juan Quintela]

Reviewed-by: Markus Armbruster 
Signed-off-by: Juan Quintela 
Message-ID: <20231013104736.31722-2-quint...@redhat.com>
---
 qapi/migration.json | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 360e609f66..db3df12d6c 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -73,7 +73,7 @@
 { 'struct': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
'duplicate': 'int',
-   'skipped': { 'type': 'int', 'features': ['deprecated'] },
+   'skipped': { 'type': 'int', 'features': [ 'deprecated' ] },
'normal': 'int',
'normal-bytes': 'int', 'dirty-pages-rate': 'int',
'mbps': 'number', 'dirty-sync-count': 'int',
@@ -440,10 +440,9 @@
 # compress and xbzrle are both on, compress only takes effect in
 # the ram bulk stage, after that, it will be disabled and only
 # xbzrle takes effect, this can help to minimize migration
-# traffic.  The feature is disabled by default.  (since 2.4 )
+# traffic.  The feature is disabled by default.  (since 2.4)
 #
-# @events: generate events for each migration state change (since 2.4
-# )
+# @events: generate events for each migration state change (since 2.4)
 #
 # @auto-converge: If enabled, QEMU will automatically throttle down
 # the guest to speed up convergence of RAM migration.  (since 1.6)
-- 
2.41.0

[PULL 09/38] migration: Fix analyze-migration read operation signedness

[Juan Quintela]

From: Fabiano Rosas 

The migration code uses unsigned values for 16, 32 and 64-bit
operations. Fix the script to do the same.

This was causing an issue when parsing the migration stream generated
on the ppc64 target because one of instance_ids was larger than the
32bit signed maximum:

Traceback (most recent call last):
  File "/home/fabiano/kvm/qemu/build/scripts/analyze-migration.py", line 658, 
in 
dump.read(dump_memory = args.memory)
  File "/home/fabiano/kvm/qemu/build/scripts/analyze-migration.py", line 592, 
in read
classdesc = self.section_classes[section_key]
KeyError: ('spapr_iommu', -2147483648)

Signed-off-by: Fabiano Rosas 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
Message-ID: <20231009184326.15777-6-faro...@suse.de>
---
 scripts/analyze-migration.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index 56ab04dd2d..de506cb8bf 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -38,13 +38,13 @@ def __init__(self, filename):
 self.file = open(self.filename, "rb")
 
 def read64(self):
-return int.from_bytes(self.file.read(8), byteorder='big', signed=True)
+return int.from_bytes(self.file.read(8), byteorder='big', signed=False)
 
 def read32(self):
-return int.from_bytes(self.file.read(4), byteorder='big', signed=True)
+return int.from_bytes(self.file.read(4), byteorder='big', signed=False)
 
 def read16(self):
-return int.from_bytes(self.file.read(2), byteorder='big', signed=True)
+return int.from_bytes(self.file.read(2), byteorder='big', signed=False)
 
 def read8(self):
 return int.from_bytes(self.file.read(1), byteorder='big', signed=True)
-- 
2.41.0