[PATCH 0/7] qcow2: make subclusters discardable
This series extends the discard operation for qcow2 driver so that it can be used to discard individial subclusters. This feature might prove useful and gain some host disk space when dealing with the guest TRIM/discard requests, especially with large clusters (1M, 2M) when subclusters are enabled. Andrey Drobyshev (7): qcow2: make function update_refcount_discard() global qcow2: add get_sc_range_info() helper for working with subcluster ranges qcow2: zeroize the entire cluster when there're no non-zero subclusters qcow2: make subclusters discardable qcow2: zero_l2_subclusters: fall through to discard operation when requested iotests/common.rc: add disk_usage function iotests/271: check disk usage on subcluster-based discard/unmap block/qcow2-cluster.c| 217 +-- block/qcow2-refcount.c | 8 +- block/qcow2.c| 8 +- block/qcow2.h| 2 + tests/qemu-iotests/250 | 5 - tests/qemu-iotests/271 | 25 +++- tests/qemu-iotests/271.out | 2 + tests/qemu-iotests/common.rc | 6 + 8 files changed, 226 insertions(+), 47 deletions(-) -- 2.39.3
[PATCH 5/7] qcow2: zero_l2_subclusters: fall through to discard operation when requested
When zeroizing subclusters within single cluster, detect usage of the
BDRV_REQ_MAY_UNMAP flag and fall through to the subcluster-based discard
operation, much like it's done with the cluster-based discards. That
way subcluster-aligned operations "qemu-io -c 'write -z -u ...'" will
lead to actual unmap.
Signed-off-by: Andrey Drobyshev
---
block/qcow2-cluster.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index cf40f2dc12..040251f2c3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2242,7 +2242,7 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
unsigned nb_subclusters, int flags)
{
BDRVQcow2State *s = bs->opaque;
-uint64_t new_l2_bitmap;
+uint64_t new_l2_bitmap, l2_bitmap_mask;
int ret, sc = offset_to_sc_index(s, offset);
SubClusterRangeInfo scri = { 0 };
@@ -2251,9 +2251,10 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t
offset,
goto out;
}
+l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
new_l2_bitmap = scri.l2_bitmap;
-new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
-new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
+new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
+new_l2_bitmap &= ~l2_bitmap_mask;
/*
* If there're no non-zero subclusters left, we might as well zeroize
@@ -2266,6 +2267,16 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t
offset,
1, flags);
}
+/*
+ * If the request allows discarding subclusters and they're actually
+ * allocated, we go down the discard path since after the discard
+ * operation the subclusters are going to be read as zeroes anyway.
+ */
+if ((flags & BDRV_REQ_MAY_UNMAP) && (scri.l2_bitmap & l2_bitmap_mask)) {
+return discard_l2_subclusters(bs, offset, nb_subclusters,
+ QCOW2_DISCARD_REQUEST, false, &scri);
+}
+
if (new_l2_bitmap != scri.l2_bitmap) {
set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap);
qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);
--
2.39.3
[PATCH 7/7] iotests/271: check disk usage on subcluster-based discard/unmap
Add _verify_du_delta() checker which is used to check that real disk
usage delta meets the expectations. For now we use it for checking that
subcluster-based discard/unmap operations lead to actual disk usage
decrease (i.e. PUNCH_HOLE operation is performed).
Also add separate test case for discarding particular subcluster within
one cluster.
Signed-off-by: Andrey Drobyshev
---
tests/qemu-iotests/271 | 25 -
tests/qemu-iotests/271.out | 2 ++
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/271 b/tests/qemu-iotests/271
index c7c2cadda0..5fcb209f5f 100755
--- a/tests/qemu-iotests/271
+++ b/tests/qemu-iotests/271
@@ -81,6 +81,15 @@ _verify_l2_bitmap()
fi
}
+# Check disk usage delta after a discard/unmap operation
+# _verify_du_delta $before $after $expected_delta
+_verify_du_delta()
+{
+if [ $(($1 - $2)) -ne $3 ]; then
+printf "ERROR: unexpected delta: $1 - $2 = $(($1 - $2)) != $3\n"
+fi
+}
+
# This should be called as _run_test c=XXX sc=XXX off=XXX len=XXX cmd=XXX
# c: cluster number (0 if unset)
# sc: subcluster number inside cluster @c (0 if unset)
@@ -198,9 +207,12 @@ for use_backing_file in yes no; do
alloc="$(seq 0 31)"; zero=""
_run_test sc=0 len=64k
-### Zero and unmap half of cluster #0 (this won't unmap it)
+### Zero and unmap half of cluster #0 (this will unmap it)
alloc="$(seq 16 31)"; zero="$(seq 0 15)"
+before=$(disk_usage "$TEST_IMG")
_run_test sc=0 len=32k cmd=unmap
+after=$(disk_usage "$TEST_IMG")
+_verify_du_delta $before $after 32768
### Zero and unmap cluster #0
alloc=""; zero="$(seq 0 31)"
@@ -447,7 +459,10 @@ for use_backing_file in yes no; do
# Subcluster-aligned request from clusters #12 to #14
alloc="$(seq 0 15)"; zero="$(seq 16 31)"
+before=$(disk_usage "$TEST_IMG")
_run_test c=12 sc=16 len=128k cmd=unmap
+after=$(disk_usage "$TEST_IMG")
+_verify_du_delta $before $after $((128 * 1024))
alloc=""; zero="$(seq 0 31)"
_verify_l2_bitmap 13
alloc="$(seq 16 31)"; zero="$(seq 0 15)"
@@ -528,6 +543,14 @@ for use_backing_file in yes no; do
else
_make_test_img -o extended_l2=on 1M
fi
+# Write cluster #0 and discard its subclusters #0-#3
+$QEMU_IO -c 'write -q 0 64k' "$TEST_IMG"
+before=$(disk_usage "$TEST_IMG")
+$QEMU_IO -c 'discard -q 0 8k' "$TEST_IMG"
+after=$(disk_usage "$TEST_IMG")
+_verify_du_delta $before $after 8192
+alloc="$(seq 4 31)"; zero="$(seq 0 3)"
+_verify_l2_bitmap 0
# Write clusters #0-#2 and then discard them
$QEMU_IO -c 'write -q 0 128k' "$TEST_IMG"
$QEMU_IO -c 'discard -q 0 128k' "$TEST_IMG"
diff --git a/tests/qemu-iotests/271.out b/tests/qemu-iotests/271.out
index 5be780de76..0da8d72cde 100644
--- a/tests/qemu-iotests/271.out
+++ b/tests/qemu-iotests/271.out
@@ -426,6 +426,7 @@ L2 entry #29: 0x
### Discarding clusters with non-zero bitmaps (backing file: yes) ###
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+L2 entry #0: 0x8005 0000
L2 entry #0: 0x
L2 entry #1: 0x
Image resized.
@@ -436,6 +437,7 @@ L2 entry #1: 0x
### Discarding clusters with non-zero bitmaps (backing file: no) ###
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+L2 entry #0: 0x8005 0000
L2 entry #0: 0x
L2 entry #1: 0x
Image resized.
--
2.39.3
[PATCH 2/7] qcow2: add get_sc_range_info() helper for working with subcluster ranges
This helper simply obtains the l2 table parameters of the cluster which
contains the given subclusters range. Right now this info is being
obtained and used by zero_l2_subclusters(). As we're about to introduce
the subclusters discard operation, this helper would let us avoid code
duplication.
Also introduce struct SubClusterRangeInfo, which would contain all the
needed params.
Signed-off-by: Andrey Drobyshev
---
block/qcow2-cluster.c | 90 +--
1 file changed, 62 insertions(+), 28 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 904f00d1b3..8801856b93 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -32,6 +32,13 @@
#include "qemu/memalign.h"
#include "trace.h"
+typedef struct SubClusterRangeInfo {
+uint64_t *l2_slice;
+int l2_index;
+uint64_t l2_entry;
+uint64_t l2_bitmap;
+} SubClusterRangeInfo;
+
int coroutine_fn qcow2_shrink_l1_table(BlockDriverState *bs,
uint64_t exact_size)
{
@@ -1892,6 +1899,50 @@ again:
return 0;
}
+static int get_sc_range_info(BlockDriverState *bs, uint64_t offset,
+ unsigned nb_subclusters,
+ SubClusterRangeInfo *scri)
+{
+BDRVQcow2State *s = bs->opaque;
+int ret, sc_cleared = 0, sc_index = offset_to_sc_index(s, offset);
+QCow2SubclusterType sctype;
+
+/* Here we only work with the subclusters within single cluster. */
+assert(nb_subclusters > 0 && nb_subclusters < s->subclusters_per_cluster);
+assert(sc_index + nb_subclusters <= s->subclusters_per_cluster);
+assert(offset_into_subcluster(s, offset) == 0);
+
+ret = get_cluster_table(bs, offset, &scri->l2_slice, &scri->l2_index);
+if (ret < 0) {
+return ret;
+}
+
+scri->l2_entry = get_l2_entry(s, scri->l2_slice, scri->l2_index);
+scri->l2_bitmap = get_l2_bitmap(s, scri->l2_slice, scri->l2_index);
+
+do {
+qcow2_get_subcluster_range_type(bs, scri->l2_entry, scri->l2_bitmap,
+sc_index, &sctype);
+if (ret < 0) {
+return ret;
+}
+
+switch (sctype) {
+case QCOW2_SUBCLUSTER_COMPRESSED:
+/* We cannot partially zeroize/discard compressed clusters. */
+return -ENOTSUP;
+case QCOW2_SUBCLUSTER_INVALID:
+return -EINVAL;
+default:
+break;
+}
+
+sc_cleared += ret;
+} while (sc_cleared < nb_subclusters);
+
+return 0;
+}
+
/*
* This discards as many clusters of nb_clusters as possible at once (i.e.
* all clusters in the same L2 slice) and returns the number of discarded
@@ -2097,44 +2148,27 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t
offset,
unsigned nb_subclusters)
{
BDRVQcow2State *s = bs->opaque;
-uint64_t *l2_slice;
-uint64_t old_l2_bitmap, l2_bitmap;
-int l2_index, ret, sc = offset_to_sc_index(s, offset);
+uint64_t new_l2_bitmap;
+int ret, sc = offset_to_sc_index(s, offset);
+SubClusterRangeInfo scri = { 0 };
-/* For full clusters use zero_in_l2_slice() instead */
-assert(nb_subclusters > 0 && nb_subclusters < s->subclusters_per_cluster);
-assert(sc + nb_subclusters <= s->subclusters_per_cluster);
-assert(offset_into_subcluster(s, offset) == 0);
-
-ret = get_cluster_table(bs, offset, &l2_slice, &l2_index);
+ret = get_sc_range_info(bs, offset, nb_subclusters, &scri);
if (ret < 0) {
-return ret;
-}
-
-switch (qcow2_get_cluster_type(bs, get_l2_entry(s, l2_slice, l2_index))) {
-case QCOW2_CLUSTER_COMPRESSED:
-ret = -ENOTSUP; /* We cannot partially zeroize compressed clusters */
goto out;
-case QCOW2_CLUSTER_NORMAL:
-case QCOW2_CLUSTER_UNALLOCATED:
-break;
-default:
-g_assert_not_reached();
}
-old_l2_bitmap = l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index);
-
-l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
-l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
+new_l2_bitmap = scri.l2_bitmap;
+new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
+new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
-if (old_l2_bitmap != l2_bitmap) {
-set_l2_bitmap(s, l2_slice, l2_index, l2_bitmap);
-qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
+if (new_l2_bitmap != scri.l2_bitmap) {
+set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap);
+qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);
}
ret = 0;
out:
-qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
+qcow2_cache_put(s->l2_table_cache, (void **) &scri.l2_slice);
return ret;
}
--
2.39.3
[PATCH 3/7] qcow2: zeroize the entire cluster when there're no non-zero subclusters
When zeroizing the last non-zero subclusters within single cluster, it
makes sense to go zeroize the entire cluster and go down zero_in_l2_slice()
path right away. That way we'd also update the corresponding refcount
table.
Signed-off-by: Andrey Drobyshev
---
block/qcow2-cluster.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8801856b93..7c6fa5524c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2145,7 +2145,7 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
static int coroutine_fn GRAPH_RDLOCK
zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
-unsigned nb_subclusters)
+unsigned nb_subclusters, int flags)
{
BDRVQcow2State *s = bs->opaque;
uint64_t new_l2_bitmap;
@@ -2161,6 +2161,17 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t
offset,
new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
+/*
+ * If there're no non-zero subclusters left, we might as well zeroize
+ * the entire cluster. That way we'd also update the refcount table.
+ */
+if ((new_l2_bitmap & QCOW_L2_BITMAP_ALL_ZEROES) ==
+QCOW_L2_BITMAP_ALL_ZEROES) {
+qcow2_cache_put(s->l2_table_cache, (void **) &scri.l2_slice);
+return zero_in_l2_slice(bs, QEMU_ALIGN_DOWN(offset, s->cluster_size),
+1, flags);
+}
+
if (new_l2_bitmap != scri.l2_bitmap) {
set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap);
qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);
@@ -2221,7 +2232,7 @@ int coroutine_fn
qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
if (head) {
ret = zero_l2_subclusters(bs, offset - head,
- size_to_subclusters(s, head));
+ size_to_subclusters(s, head), flags);
if (ret < 0) {
goto fail;
}
@@ -2242,7 +2253,8 @@ int coroutine_fn
qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
}
if (tail) {
-ret = zero_l2_subclusters(bs, end_offset, size_to_subclusters(s,
tail));
+ret = zero_l2_subclusters(bs, end_offset,
+ size_to_subclusters(s, tail), flags);
if (ret < 0) {
goto fail;
}
--
2.39.3
[PATCH 6/7] iotests/common.rc: add disk_usage function
Move the definition from iotests/250 to common.rc. This is used to
detect real disk usage of sparse files. In particular, we want to use
it for checking subclusters-based discards.
Signed-off-by: Andrey Drobyshev
---
tests/qemu-iotests/250 | 5 -
tests/qemu-iotests/common.rc | 6 ++
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/250 b/tests/qemu-iotests/250
index af48f83aba..c0a0dbc0ff 100755
--- a/tests/qemu-iotests/250
+++ b/tests/qemu-iotests/250
@@ -52,11 +52,6 @@ _unsupported_imgopts data_file
# bdrv_co_truncate(bs->file) call in qcow2_co_truncate(), which might succeed
# anyway.
-disk_usage()
-{
-du --block-size=1 $1 | awk '{print $1}'
-}
-
size=2100M
_make_test_img -o "cluster_size=1M,preallocation=metadata" $size
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 95c12577dd..5d2ea26c7f 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -140,6 +140,12 @@ _optstr_add()
fi
}
+# report real disk usage for sparse files
+disk_usage()
+{
+du --block-size=1 $1 | awk '{print $1}'
+}
+
# Set the variables to the empty string to turn Valgrind off
# for specific processes, e.g.
# $ VALGRIND_QEMU_IO= ./check -qcow2 -valgrind 015
--
2.39.3
[PATCH 1/7] qcow2: make function update_refcount_discard() global
We are going to need it for discarding separate subclusters. The
function itself doesn't do anything with the refcount tables, it simply
adds a discard request to the queue, so rename it to qcow2_queue_discard().
Signed-off-by: Andrey Drobyshev
---
block/qcow2-refcount.c | 8
block/qcow2.h | 2 ++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 0266542cee..2026f5fa21 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -754,8 +754,8 @@ void qcow2_process_discards(BlockDriverState *bs, int ret)
}
}
-static void update_refcount_discard(BlockDriverState *bs,
-uint64_t offset, uint64_t length)
+void qcow2_queue_discard(BlockDriverState *bs, uint64_t offset,
+ uint64_t length)
{
BDRVQcow2State *s = bs->opaque;
Qcow2DiscardRegion *d, *p, *next;
@@ -902,7 +902,7 @@ update_refcount(BlockDriverState *bs, int64_t offset,
int64_t length,
}
if (s->discard_passthrough[type]) {
-update_refcount_discard(bs, cluster_offset, s->cluster_size);
+qcow2_queue_discard(bs, cluster_offset, s->cluster_size);
}
}
}
@@ -3619,7 +3619,7 @@ qcow2_discard_refcount_block(BlockDriverState *bs,
uint64_t discard_block_offs)
/* discard refblock from the cache if refblock is cached */
qcow2_cache_discard(s->refcount_block_cache, refblock);
}
-update_refcount_discard(bs, discard_block_offs, s->cluster_size);
+qcow2_queue_discard(bs, discard_block_offs, s->cluster_size);
return 0;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index 29958c512b..75d6a1b61b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -891,6 +891,8 @@ int coroutine_fn qcow2_check_refcounts(BlockDriverState
*bs, BdrvCheckResult *re
BdrvCheckMode fix);
void qcow2_process_discards(BlockDriverState *bs, int ret);
+void qcow2_queue_discard(BlockDriverState *bs, uint64_t offset,
+ uint64_t length);
int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
int64_t size);
--
2.39.3
[PATCH 4/7] qcow2: make subclusters discardable
This commit makes the discard operation work on the subcluster level
rather than cluster level. It introduces discard_l2_subclusters()
function and makes use of it in qcow2 discard implementation, much like
it's done with zero_in_l2_slice() / zero_l2_subclusters(). It also
changes the qcow2 driver pdiscard_alignment to subcluster_size. That
way subcluster-aligned discards lead to actual fallocate(PUNCH_HOLE)
operation and free host disk space.
This feature will let us gain additional disk space on guest
TRIM/discard requests, especially when using large enough clusters
(1M, 2M) with subclusters enabled.
Signed-off-by: Andrey Drobyshev
---
block/qcow2-cluster.c | 100 --
block/qcow2.c | 8 ++--
2 files changed, 101 insertions(+), 7 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 7c6fa5524c..cf40f2dc12 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2042,6 +2042,74 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t
offset, uint64_t nb_clusters,
return nb_clusters;
}
+static int coroutine_fn GRAPH_RDLOCK
+discard_l2_subclusters(BlockDriverState *bs, uint64_t offset,
+ uint64_t nb_subclusters,
+ enum qcow2_discard_type type,
+ bool full_discard,
+ SubClusterRangeInfo *pscri)
+{
+BDRVQcow2State *s = bs->opaque;
+uint64_t new_l2_bitmap, l2_bitmap_mask;
+int ret, sc = offset_to_sc_index(s, offset);
+SubClusterRangeInfo scri = { 0 };
+
+if (!pscri) {
+ret = get_sc_range_info(bs, offset, nb_subclusters, &scri);
+if (ret < 0) {
+goto out;
+}
+} else {
+scri = *pscri;
+}
+
+l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
+new_l2_bitmap = scri.l2_bitmap;
+new_l2_bitmap &= ~l2_bitmap_mask;
+
+/*
+ * If there're no allocated subclusters left, we might as well discard
+ * the entire cluster. That way we'd also update the refcount table.
+ */
+if (!(new_l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC)) {
+return discard_in_l2_slice(bs,
+ QEMU_ALIGN_DOWN(offset, s->cluster_size),
+ 1, type, full_discard);
+}
+
+/*
+ * Full discard means we fall through to the backing file, thus we only
+ * need to mark the subclusters as deallocated.
+ *
+ * Non-full discard means subclusters should be explicitly marked as
+ * zeroes. In this case QCOW2 specification requires the corresponding
+ * allocation status bits to be unset as well. If the subclusters are
+ * deallocated in the first place and there's no backing, the operation
+ * can be skipped.
+ */
+if (!full_discard &&
+(bs->backing || scri.l2_bitmap & l2_bitmap_mask)) {
+new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
+}
+
+if (scri.l2_bitmap != new_l2_bitmap) {
+set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap);
+qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);
+}
+
+if (s->discard_passthrough[type]) {
+qcow2_queue_discard(bs, (scri.l2_entry & L2E_OFFSET_MASK) +
+offset_into_cluster(s, offset),
+nb_subclusters * s->subcluster_size);
+}
+
+ret = 0;
+out:
+qcow2_cache_put(s->l2_table_cache, (void **) &scri.l2_slice);
+
+return ret;
+}
+
int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, enum qcow2_discard_type type,
bool full_discard)
@@ -2049,19 +2117,36 @@ int qcow2_cluster_discard(BlockDriverState *bs,
uint64_t offset,
BDRVQcow2State *s = bs->opaque;
uint64_t end_offset = offset + bytes;
uint64_t nb_clusters;
+unsigned head, tail;
int64_t cleared;
int ret;
/* Caller must pass aligned values, except at image end */
-assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
-assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
+assert(QEMU_IS_ALIGNED(offset, s->subcluster_size));
+assert(QEMU_IS_ALIGNED(end_offset, s->subcluster_size) ||
end_offset == bs->total_sectors << BDRV_SECTOR_BITS);
-nb_clusters = size_to_clusters(s, bytes);
+head = MIN(end_offset, ROUND_UP(offset, s->cluster_size)) - offset;
+offset += head;
+
+tail = (end_offset >= bs->total_sectors << BDRV_SECTOR_BITS) ? 0 :
+ end_offset - MAX(offset, start_of_cluster(s, end_offset));
+end_offset -= tail;
s->cache_discards = true;
+if (head) {
+ret = discard_l2_subclusters(bs, offset - head,
+ size_to_subclusters(s, head), type,
+ full_discard, NULL);
+if (ret < 0) {
+goto fail;
+}
+}
+
/* Each L2 slice
Re: [PATCH v2 03/13] migration: Use vmstate_register_any() for isa-ide
Thomas Huth wrote: > On 20/10/2023 11.07, Juan Quintela wrote: >> Otherwise qom-test fails. >> ok 4 /i386/qom/x-remote >> qemu-system-i386: savevm_state_handler_insert: Detected duplicate >> SaveStateEntry: id=isa-ide, instance_id=0x0 >> Broken pipe >> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() >> tried to terminate QEMU process but encountered exit status 1 (expected 0) >> Aborted (core dumped) >> $ >> Reviewed-by: Stefan Berger >> Signed-off-by: Juan Quintela >> --- >> hw/ide/isa.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> diff --git a/hw/ide/isa.c b/hw/ide/isa.c >> index 95053e026f..ea60c08116 100644 >> --- a/hw/ide/isa.c >> +++ b/hw/ide/isa.c >> @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error >> **errp) >> ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); >> ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); >> ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); >> -vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); >> +vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s); >> ide_bus_register_restart_cb(&s->bus); >> } > > Would it make sense to use another unique ID of the device instead? E.g.: > > diff a/hw/ide/isa.c b/hw/ide/isa.c > --- a/hw/ide/isa.c > +++ b/hw/ide/isa.c > @@ -73,7 +73,9 @@ static void isa_ide_realizefn(DeviceState *dev, Error > **errp) > ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); > ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); > ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); > -vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); > +vmstate_register(VMSTATE_IF(dev), > + object_property_get_int(OBJECT(dev), "irq", > &error_abort), > + &vmstate_ide_isa, s); > ide_bus_register_restart_cb(&s->bus); > } >Thomas Ide is not my part of expertise. But anything that is different for each instantance is going to be good for me. The whole point of this series is to be able to test that there are no duplicates. Duplicates are one error when we do real migration. How we reach the goal of no duplicates doesn't matter to me. Later, Juan.
Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices
On Wed, 2023-10-18 at 11:52 +0100, David Woodhouse wrote:
>
> And xen_config_dev_nic() probably just needs to loop doing the same
> as
> I did in pc_init_nic() in
> https://lore.kernel.org/qemu-devel/20231017182545.97973-5-dw...@infradead.org/T/#u
>
> + if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-
> device"))) {
> + DeviceState *dev = qdev_new("xen-net-device");
> + qdev_set_nic_properties(dev, nd);
> + qdev_realize_and_unref(dev, xen_bus, &error_fatal);
>
>
> ... but this just reinforces what I said there about "if
> qmp_device_add() can find the damn bus and do this right, why do we
> have to litter it through platform code?"
I had a look through the network setup.
There are a bunch of platforms adding specific devices to their own
internal system bus, which often use nd_table[] directly. Sometimes
they do so whether it's been set up or now.
They can mostly be divided into two camps. Some of them will create
their NIC anyway, and will use a matching -nic configuration if it
exists. Others will only create their NIC if a corresponding -nic
configuration does exist.
This is fairly random, and perhaps platforms should be more consistent,
but it's best to avoid user-visible changes as much as possible while
doing the cleanup, so I've kept them as they were.
I've created qemu_configure_nic_device() and qemu_create_nic_device()
functions for those two use cases respectively, and most code which
directly accesses nd_table[] can be converted to those fairly simply:
https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/7b4fb6fc10a4
It means I can throw away the horrid parts of -nic support for the Xen
network device, which were in the pc and xenfv platforms, and replace
it with a trivial loop in xenbus_init():
+/* This is a bus-generic "configure all NICs on this bus type" */
+while ((qnic = qemu_create_nic_device("xen-net-device", true, "xen"))) {
+qdev_realize_and_unref(qnic, bus, &error_fatal);
Other than that one (which is cheating because there's only one type of
network device that can be instantiated on the XenBus), the only
remaining case is PCI. Most platforms just iterate over the -nic
configurations adding devices to a PCI bus of the platform's choice.
In some cases there's a special case, the first one goes at a specific
devfn and/or on a different bus.
There was a little more variation here... for example fuloong2e would
put the first nic (nd_table[0]) in slot 7 only if it's an rtl8139 (if
the model is unspecified). But PReP would put the first PCI NIC in slot
3 *regardless* of whether it's the expected PCNET or not.
I didn't faithfully preserve the behaviour there, because I don't think
it matters. They mostly look like this now (e.g. hw/sh4/r2d):
+nd = qemu_find_nic_info(mc->default_nic, true, NULL);
+if (nd) {
+pci_nic_init_nofail(nd, pci_bus, mc->default_nic, "2");
+}
+pci_init_nic_devices(pci_bus, mc->default_nic);
So they'll take the first NIC configuration which is of the expected
model (again, or unspecified model) and place that in the special slot,
and then put the rest of the devices wherever they land.
For the change in behaviour to *matter*, the user would have to
explicitly specify a NIC of the *non-default* type first, and then a
NIC of the default type. My new code will put the default-type NIC in
the "right place", while the old code mostly wouldn't. I think that's
an OK change to make.
My plan is to *remember* which NIC models are used for lookups during
the board in, so that qemu_show_nic_devices() can print them all at the
end.
Current WIP if anyone wants to take a quick look at
https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xenfv-net
but the weekend arrived quicker than I'd hoped, so I haven't quite got
it into shape to post yet. Hopefully it makes sense as an approach
though?
smime.p7s
Description: S/MIME cryptographic signature
Re: [PULL 00/17] Migration 20231020 patches
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PULL 00/46] Misc HW/UI patches for 2023-10-19
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH v2 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices
On 20/10/2023 11.07, Juan Quintela wrote: Just with make check I can see that we can have more than one of this devices, so use ANY. ok 5 /s390x/device/introspect/abstract-interfaces ... Broken pipe ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0) Aborted (core dumped) Reviewed-by: Stefan Berger Signed-off-by: Juan Quintela --- hw/s390x/s390-skeys.c| 3 ++- hw/s390x/s390-stattrib.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) Please use this patch series instead: https://lore.kernel.org/qemu-devel/20231020150554.664422-1-th...@redhat.com/ Thanks, Thomas
Re: [PATCH v2 04/13] migration: Use vmstate_register_any() for ipmi-bt*
On 20/10/2023 11.07, Juan Quintela wrote: Otherwise device-introspection-test fails. $ ./tests/qtest/device-introspect-test ... Broken pipe ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0) Aborted (core dumped) Reviewed-by: Stefan Berger Signed-off-by: Juan Quintela --- hw/ipmi/ipmi_bmc_extern.c | 2 +- hw/ipmi/ipmi_bmc_sim.c| 2 +- hw/ipmi/isa_ipmi_bt.c | 2 +- hw/ipmi/isa_ipmi_kcs.c| 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) Please check whether you could replace this by this patch instead: https://lore.kernel.org/qemu-devel/20231020145554.662751-1-th...@redhat.com/ Thanks, Thomas
Re: [PATCH v2 13/13] migration: Use vmstate_register_any() for vmware_vga
On 20/10/2023 11.07, Juan Quintela wrote: I have no idea if we can have more than one vmware_vga device, so play it safe. FWIW, it doesn't look like it's possible: $ ./qemu-system-x86_64 -device vmware-svga -device vmware-svga RAMBlock "vmsvga.fifo" already registered, abort! Aborted (core dumped) (NB: Aborting is very user-unfriendly here, but that's something for another patch...) Reviewed-by: Stefan Berger Signed-off-by: Juan Quintela --- hw/display/vmware_vga.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c index 09591fbd39..7490d43881 100644 --- a/hw/display/vmware_vga.c +++ b/hw/display/vmware_vga.c @@ -1264,7 +1264,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s, vga_common_init(&s->vga, OBJECT(dev), &error_fatal); vga_init(&s->vga, OBJECT(dev), address_space, io, true); -vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga); +vmstate_register_any(NULL, &vmstate_vga_common, &s->vga); s->new_depth = 32; } Reviewed-by: Thomas Huth
Re: [PATCH v2 03/13] migration: Use vmstate_register_any() for isa-ide
On 20/10/2023 11.07, Juan Quintela wrote: Otherwise qom-test fails. ok 4 /i386/qom/x-remote qemu-system-i386: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=isa-ide, instance_id=0x0 Broken pipe ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0) Aborted (core dumped) $ Reviewed-by: Stefan Berger Signed-off-by: Juan Quintela --- hw/ide/isa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ide/isa.c b/hw/ide/isa.c index 95053e026f..ea60c08116 100644 --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); -vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); +vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s); ide_bus_register_restart_cb(&s->bus); } Would it make sense to use another unique ID of the device instead? E.g.: diff a/hw/ide/isa.c b/hw/ide/isa.c --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -73,7 +73,9 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); -vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); +vmstate_register(VMSTATE_IF(dev), + object_property_get_int(OBJECT(dev), "irq", &error_abort), + &vmstate_ide_isa, s); ide_bus_register_restart_cb(&s->bus); } Thomas
Re: deadlock when using iothread during backup_clean()
Am 19.10.23 um 15:53 schrieb Fiona Ebner: > Am 19.10.23 um 14:14 schrieb Kevin Wolf: >> Am 18.10.2023 um 11:42 hat Fiona Ebner geschrieben: >>> Am 17.10.23 um 16:20 schrieb Kevin Wolf: Am 17.10.2023 um 15:37 hat Fiona Ebner geschrieben: > Am 17.10.23 um 14:12 schrieb Kevin Wolf: >> Am 17.10.2023 um 12:18 hat Fiona Ebner geschrieben: >>> I ran into similar issues now with mirror, (both deadlocks and stuck >>> guest IO at other times), and interestingly, also during job start. >>> >>> Also had a backtrace similar to [0] once, so I took a closer look. >>> Probably was obvious to others already, but for the record: >>> >>> 1. the graph is locked by the main thread >>> 2. the iothread holds the AioContext lock >>> 3. the main thread waits on the AioContext lock >>> 4. the iothread waits for coroutine spawned by blk_is_available() >> >> Where does this blk_is_available() in the iothread come from? Having it >> wait without dropping the AioContext lock sounds like something that >> we'd want to avoid. Ideally, devices using iothreads shouldn't use >> synchronous requests at all, but I think scsi-disk might have some of >> them. >> > > It's part of the request handling in virtio-scsi: > >> #0 0x7ff7f5f55136 in __ppoll (fds=0x7ff7e40030c0, nfds=8, >> timeout=, sigmask=0x0) at >> ../sysdeps/unix/sysv/linux/ppoll.c:42 >> #1 0x5587132615ab in qemu_poll_ns (fds=0x7ff7e40030c0, nfds=8, >> timeout=-1) at ../util/qemu-timer.c:339 >> #2 0x55871323e8b1 in fdmon_poll_wait (ctx=0x55871598d5e0, >> ready_list=0x7ff7f288ebe0, timeout=-1) at ../util/fdmon-poll.c:79 >> #3 0x55871323e1ed in aio_poll (ctx=0x55871598d5e0, blocking=true) >> at ../util/aio-posix.c:670 >> #4 0x558713089efa in bdrv_poll_co (s=0x7ff7f288ec90) at >> /home/febner/repos/qemu/block/block-gen.h:43 >> #5 0x55871308c362 in blk_is_available (blk=0x55871599e2f0) at >> block/block-gen.c:1426 >> #6 0x558712f6843b in virtio_scsi_ctx_check (s=0x558716c049c0, >> d=0x55871581cd30) at ../hw/scsi/virtio-scsi.c:290 Oh... So essentially for an assertion. I wonder if the blk_is_available() check introduced in 2a2d69f490c is even necessary any more, because BlockBackend has its own AioContext now. And if blk_bs(blk) != NULL isn't what we actually want to check if the check is necessary, because calling bdrv_is_inserted() doesn't seem to have been intended. blk_bs() wouldn't have to poll. >>> >>> Could virtio_scsi_hotunplug() be an issue with removing or modifying >>> the check? There's a call there which sets the blk's AioContext to >>> qemu_get_aio_context(). Or are we sure that the assert in >>> virtio_scsi_ctx_check() can't be reached after that? >> >> I think that would be the kind of bug that the assertion tries to >> catch, because then we would be sending requests to blk from a thread >> that doesn't match its AioContext (which will be allowed soon, but not >> quite yet). >> >> Before resetting the AioContext, virtio_scsi_hotunplug() calls >> qdev_simple_device_unplug_cb(), which unrealizes the SCSI device. This >> calls scsi_qdev_unrealize() -> scsi_device_purge_requests(), which in >> turn drains blk and cancels all pending requests. So there should be >> nothing left that could call into virtio_scsi_ctx_check() any more. >> >> The other argument is that after unrealize, virtio_scsi_device_get() >> would return NULL anyway, so even if a request were still pending, it >> would just fail instead of accessing the unplugged device. >> > > Okay, sounds like a way to get around that deadlock issue then :) > Unfortunately, scsi_dma_command() also has a blk_is_available() call and I ran into a similar deadlock with that once. > (...) > What does the stuck I/O look like? Is it stuck in the backend, i.e. the device started requests that never complete? Or stuck from the guest perspective, i.e. the device never checks for new requests? >>> >>> AFAICT, from the guest perspective. >>> I don't really have an idea immediately, we'd have to find out where the stuck I/O stops being processed. >>> >>> I've described it in an earlier mail in this thread: >>> https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg01900.html >>> >>> Quoting from there: >>> 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. >> >> And bs->in_flight and blk->in_flight are 0, too? >> > > Yes. And queued_requests in the BlockBackend is also empty. > >> Is anything quiesced? > > No. quiesce_counter is 0 for both BlockDriverState instances as well as > for the BlockBackend. quiesced_parent is false for both parents (i.e. > child_root for the bdrv_raw and child_of_bds for th
Re: [PATCH v2 00/22] qapi: Kill 'qapi/qmp/qerror.h' for good
Your patches are sane, the problem is they made me look at the code they patch, much of which is not fine. Let's chat off list to figure out how to best get your patches merged.
Re: [PATCH v2 04/13] migration: Use vmstate_register_any() for ipmi-bt*
On 20/10/2023 11.07, Juan Quintela wrote:
Otherwise device-introspection-test fails.
$ ./tests/qtest/device-introspect-test
...
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
hw/ipmi/ipmi_bmc_extern.c | 2 +-
hw/ipmi/ipmi_bmc_sim.c| 2 +-
hw/ipmi/isa_ipmi_bt.c | 2 +-
hw/ipmi/isa_ipmi_kcs.c| 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index e232d35ba2..324a2c8835 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -504,7 +504,7 @@ static void ipmi_bmc_extern_init(Object *obj)
IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
-vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
+vmstate_register_any(NULL, &vmstate_ipmi_bmc_extern, ibe);
}
This is an instance_init() function. We shouldn't register vmstate (and
timer) here, but do it in the realize() function instead.
static void ipmi_bmc_extern_finalize(Object *obj)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 905e091094..404db5d5bc 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -2188,7 +2188,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error
**errp)
ibs->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ipmi_timeout, ibs);
-vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
+vmstate_register_any(NULL, &vmstate_ipmi_sim, ibs);
}
Now here it's been done in the realize() function already ... did this
really cause trouble for you?
Anyway, I wonder why the first parameter is NULL here ... shouldn't this
point to dev instead?
static Property ipmi_sim_properties[] = {
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index a83e7243d6..afb76b548a 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -125,7 +125,7 @@ static void isa_ipmi_bt_init(Object *obj)
ipmi_bmc_find_and_link(obj, (Object **) &iib->bt.bmc);
-vmstate_register(NULL, 0, &vmstate_ISAIPMIBTDevice, iib);
+vmstate_register_any(NULL, &vmstate_ISAIPMIBTDevice, iib);
}
It's an instance_init() function again. This should be done in the realize()
instead.
static void *isa_ipmi_bt_get_backend_data(IPMIInterface *ii)
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index b2ed70b9da..5ab63b2fcf 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -132,7 +132,7 @@ static void isa_ipmi_kcs_init(Object *obj)
* IPMI device, so receive it, but transmit a different
* version.
*/
-vmstate_register(NULL, 0, &vmstate_ISAIPMIKCSDevice, iik);
+vmstate_register_any(NULL, &vmstate_ISAIPMIKCSDevice, iik);
}
Dito, this should be moved to realize().
Thomas
Re: [PATCH v2 19/22] qapi: Inline and remove QERR_PROPERTY_VALUE_OUT_OF_RANGE definition
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using sed, manually
> removing the definition in include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/qapi/qmp/qerror.h | 3 ---
> hw/intc/openpic.c | 3 ++-
> target/i386/cpu.c | 12
> util/block-helpers.c | 3 ++-
> 4 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 7862ac55a1..e094f13114 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,9 +17,6 @@
> * add new ones!
> */
>
> -#define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
> -"Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ",
> maximum: %" PRId64 ")"
> -
> #define QERR_QGA_COMMAND_FAILED \
> "Guest agent command failed, error was '%s'"
>
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index a6f91d4bcd..4f6ee930e2 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1535,7 +1535,8 @@ static void openpic_realize(DeviceState *dev, Error
> **errp)
> };
>
> if (opp->nb_cpus > MAX_CPU) {
> -error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> +error_setg(errp, "Property %s.%s doesn't take value %" PRId64
> + " (minimum: %" PRId64 ", maximum: %" PRId64 ")",
> TYPE_OPENPIC, "nb_cpus", (uint64_t)opp->nb_cpus,
> (uint64_t)0, (uint64_t)MAX_CPU);
> return;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index e5a14885ed..273f865228 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5086,7 +5086,8 @@ static void x86_cpuid_version_set_family(Object *obj,
> Visitor *v,
> return;
> }
> if (value < min || value > max) {
> -error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
> +error_setg(errp, "Property %s doesn't take value %" PRId64
> + " (minimum: %" PRId64 ", maximum: %" PRId64 ")",
> name ? name : "null", value, min, max);
> return;
> }
> @@ -5126,7 +5127,8 @@ static void x86_cpuid_version_set_model(Object *obj,
> Visitor *v,
> return;
> }
> if (value < min || value > max) {
> -error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
> +error_setg(errp, "Property %s doesn't take value %" PRId64
> + " (minimum: %" PRId64 ", maximum: %" PRId64 ")",
> name ? name : "null", value, min, max);
> return;
> }
> @@ -5161,7 +5163,8 @@ static void x86_cpuid_version_set_stepping(Object *obj,
> Visitor *v,
> return;
> }
> if (value < min || value > max) {
> -error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
> +error_setg(errp, "Property %s doesn't take value %" PRId64
> + " (minimum: %" PRId64 ", maximum: %" PRId64 ")",
> name ? name : "null", value, min, max);
> return;
> }
> @@ -5263,7 +5266,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor
> *v, const char *name,
> return;
> }
> if (value < min || value > max) {
> -error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
> +error_setg(errp, "Property %s doesn't take value %" PRId64
> + " (minimum: %" PRId64 ", maximum: %" PRId64 ")",
> name ? name : "null", value, min, max);
> return;
> }
> diff --git a/util/block-helpers.c b/util/block-helpers.c
> index c4851432f5..de94909bc4 100644
> --- a/util/block-helpers.c
> +++ b/util/block-helpers.c
> @@ -30,7 +30,8 @@ void check_block_size(const char *id, const char *name,
> int64_t value,
> {
> /* value of 0 means "unset" */
> if (value && (value < MIN_BLOCK_SIZE || value > MAX_BLOCK_SIZE)) {
> -error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
Three callers:
* set_blocksize()
Property setter. Good.
* vu_blk_exp_create() and vduse_blk_exp_create()
These check QMP arguments, i.e. *not* porperties. Misuse of
QERR_PROPERTY_VALUE_OUT_OF_RANGE.
> +error_setg(errp, "Property %s.%s doesn't take value %" PRId64
> + " (minimum: %" PRId64 ", maximum: %" PRId64 ")",
> id, name, value, MIN_BLOCK_SIZE, MAX_BLOCK_SIZE);
> return;
> }
Re: [PATCH v2 20/22] qapi: Inline and remove QERR_QGA_COMMAND_FAILED definition
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using the following
> coccinelle semantic patch:
>
> @match exists@
> expression errp;
> expression errmsg;
> @@
> error_setg(errp, QERR_QGA_COMMAND_FAILED, errmsg);
>
> @script:python strformat depends on match@
> errmsg << match.errmsg;
> fixedfmt; // new var
> @@
> # Format skipping '"'.
> fixedfmt = f'"Guest agent command failed, error was \'{errmsg[1:-1]}\'"'
> coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
> @replace@
> expression match.errp;
> expression match.errmsg;
> identifier strformat.fixedfmt;
> @@
> -error_setg(errp, QERR_QGA_COMMAND_FAILED, errmsg);
> +error_setg(errp, fixedfmt);
>
> then manually removing the definition in include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/qapi/qmp/qerror.h | 3 ---
> qga/commands-win32.c | 38 --
> qga/commands.c| 7 ---
> 3 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index e094f13114..840831cc6a 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,9 +17,6 @@
> * add new ones!
> */
>
> -#define QERR_QGA_COMMAND_FAILED \
> -"Guest agent command failed, error was '%s'"
> -
> #define QERR_UNSUPPORTED \
> "this feature or command is not currently supported"
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 946dbafbb6..aa8c9770d4 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -245,7 +245,8 @@ int64_t qmp_guest_file_open(const char *path, const char
> *mode, Error **errp)
>
> done:
> if (gerr) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> +error_setg(errp,
> + "Guest agent command failed, error was 'err -> messag'");
Oopsie :)
Two more below.
> g_error_free(gerr);
> }
> g_free(w_path);
> @@ -279,8 +280,8 @@ static void acquire_privilege(const char *name, Error
> **errp)
> TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &token))
> {
> if (!LookupPrivilegeValue(NULL, name, &priv.Privileges[0].Luid)) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> - "no luid for requested privilege");
> +error_setg(errp,
> + "Guest agent command failed, error was 'no luid for
> requested privilege'");
> goto out;
> }
>
I don't like this error message. I'm going to pretend I didn't see it.
[...]
Re: [PATCH v2 18/22] qapi: Inline and remove QERR_PROPERTY_VALUE_BAD definition
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Manual change. Remove the definition in
> include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/qapi/qmp/qerror.h | 3 ---
> hw/core/qdev-properties.c | 2 +-
> target/i386/cpu.c | 2 +-
> 3 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index b0f48f22fe..7862ac55a1 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,9 +17,6 @@
> * add new ones!
> */
>
> -#define QERR_PROPERTY_VALUE_BAD \
> -"Property '%s.%s' doesn't take value '%s'"
> -
> #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
> "Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ",
> maximum: %" PRId64 ")"
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 357b8761b5..44fc1686e0 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -682,7 +682,7 @@ void error_set_from_qdev_prop_error(Error **errp, int
> ret, Object *obj,
> break;
> default:
> case -EINVAL:
> -error_setg(errp, QERR_PROPERTY_VALUE_BAD,
> +error_setg(errp, "Property '%s.%s' doesn't take value '%s'",
> object_get_typename(obj), name, value);
> break;
> case -ENOENT:
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ed72883bf3..e5a14885ed 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5190,7 +5190,7 @@ static void x86_cpuid_set_vendor(Object *obj, const
> char *value,
> int i;
>
> if (strlen(value) != CPUID_VENDOR_SZ) {
> -error_setg(errp, QERR_PROPERTY_VALUE_BAD, "", "vendor", value);
Reporting the actual problem would be better: we need the value to be
exactly CPUID_VENDOR_SZ characters long.
> +error_setg(errp, "Property 'vendor' doesn't take value '%s'", value);
> return;
> }
Re: [PATCH v2 16/22] qapi: Inline QERR_MISSING_PARAMETER definition (constant parameter)
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using the following
> coccinelle semantic patches:
>
> @match@
> expression errp;
> constant param;
> @@
> error_setg(errp, QERR_MISSING_PARAMETER, param);
>
> @script:python strformat depends on match@
> param << match.param;
> fixedfmt; // new var
> @@
> if param[0] == '"': # Format skipping '"',
> fixedfmt = f'"Parameter \'{param[1:-1]}\' is missing"'
> else: # or use definition.
> fixedfmt = f'"Parameter " {param} " is missing"'
> coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
> @replace@
> expression match.errp;
> constant match.param;
> identifier strformat.fixedfmt;
> @@
> -error_setg(errp, QERR_MISSING_PARAMETER, param);
> +error_setg(errp, fixedfmt);
>
> and:
>
> @match@
> constant param;
> @@
> error_report(QERR_MISSING_PARAMETER, param);
>
> @script:python strformat depends on match@
> param << match.param;
> fixedfmt; // new var
> @@
> fixedfmt = f'"Parameter \'{param[1:-1]}\' is missing"'
> coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
> @replace@
> constant match.param;
> identifier strformat.fixedfmt;
> @@
> -error_report(QERR_MISSING_PARAMETER, param);
> +error_report(fixedfmt);
>
> Signed-off-by: Philippe Mathieu-Daudé
> Reviewed-by: Stefan Berger
> ---
> backends/dbus-vmstate.c| 2 +-
> block/gluster.c| 21 +++--
> block/monitor/block-hmp-cmds.c | 6 +++---
> dump/dump.c| 4 ++--
> hw/usb/redirect.c | 2 +-
> softmmu/qdev-monitor.c | 2 +-
> softmmu/tpm.c | 4 ++--
> softmmu/vl.c | 4 ++--
> ui/input-barrier.c | 2 +-
> ui/ui-qmp-cmds.c | 2 +-
> 10 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
> index 57369ec0f2..e781ded17c 100644
> --- a/backends/dbus-vmstate.c
> +++ b/backends/dbus-vmstate.c
> @@ -413,7 +413,7 @@ dbus_vmstate_complete(UserCreatable *uc, Error **errp)
> }
>
> if (!self->dbus_addr) {
> -error_setg(errp, QERR_MISSING_PARAMETER, "addr");
Misuse of QERR_MISSING_PARAMETER.
This function is interface UserCreatableClass method complete(), which
runs right after an object with this interface was created.
"Parameters" need not exist in this context.
The actual issue is property "addr" has not been set. So let's report
that: "property 'addr' is required".
Separate patch, to keep this one mechanical.
> +error_setg(errp, "Parameter 'addr' is missing");
> return;
> }
>
> diff --git a/block/gluster.c b/block/gluster.c
> index ad5fadbe79..8d97d698c3 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -530,20 +530,20 @@ static int
> qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>
> num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
> if (num_servers < 1) {
> -error_setg(&local_err, QERR_MISSING_PARAMETER, "server");
> +error_setg(&local_err, "Parameter 'server' is missing");
> goto out;
> }
>
> ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
> if (!ptr) {
> -error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_VOLUME);
> +error_setg(&local_err, "Parameter " GLUSTER_OPT_VOLUME " is
> missing");
> goto out;
> }
> gconf->volume = g_strdup(ptr);
>
> ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
> if (!ptr) {
> -error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_PATH);
> +error_setg(&local_err, "Parameter " GLUSTER_OPT_PATH " is missing");
> goto out;
> }
> gconf->path = g_strdup(ptr);
> @@ -562,7 +562,8 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
> *gconf,
>
> ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
> if (!ptr) {
> -error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
> +error_setg(&local_err,
> + "Parameter " GLUSTER_OPT_TYPE " is missing");
> error_append_hint(&local_err, GERR_INDEX_HINT, i);
> goto out;
>
> @@ -592,16 +593,16 @@ static int
> qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>
> ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
> if (!ptr) {
> -error_setg(&local_err, QERR_MISSING_PARAMETER,
> - GLUSTER_OPT_HOST);
> +error_setg(&local_err,
> + "Parameter " GLUSTER_OPT_HOST " is missing");
> error_append_hint(&
[PULL v3 06/46] MAINTAINERS: Extend entry to cover util/qemu-timer-common.c, too
From: Thomas Huth We already cover util/qemu-timer.c in MAINTAINERS - change this entry to util/qemu-timer*.c so that it covers util/qemu-timer-common.c, too. Signed-off-by: Thomas Huth Reviewed-by: Philippe Mathieu-Daudé Message-ID: <20231020062142.525405-1-th...@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index fe11c98669..1b2c5b9e76 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2937,7 +2937,7 @@ F: include/qemu/main-loop.h F: include/sysemu/runstate.h F: include/sysemu/runstate-action.h F: util/main-loop.c -F: util/qemu-timer.c +F: util/qemu-timer*.c F: system/vl.c F: system/main.c F: system/cpus.c -- 2.41.0
[PULL v3 00/46] Misc HW/UI patches for 2023-10-19
The following changes since commit 0d239e513e0117e66fa739fb71a43b9383a108ff: Merge tag 'pull-lu-20231018' of https://gitlab.com/rth7680/qemu into staging (2023-10-19 10:20:57 -0700) are available in the Git repository at: https://github.com/philmd/qemu.git tags/hw-misc-20231020 for you to fetch changes up to 9f1b100236223d073915b8eedac3089ec64f8a6e: ui/input: Constify QemuInputHandler structure (2023-10-20 14:46:07 +0200) Since v2: - Cc qemu-de...@nongnu.org Since v1: - Dropped PC_SPEAKER patch - Added MAINTAINERS::util/qemu-timer-common.c patch Misc hardware patch queue - MAINTAINERS updates (Zoltan, Thomas) - Fix cutils::get_relocated_path on Windows host (Akihiko) - Housekeeping in Memory APIs (Marc-André) - SDHCI fix for SDMA transfer (Lu, Jianxian) - Various QOM/QDev/SysBus cleanups (Philippe) - Constify QemuInputHandler structure (Philippe) Akihiko Odaki (1): cutils: Fix get_relocated_path on Windows BALATON Zoltan (1): MAINTAINERS: Split vt82c686 out of fuloong2e Lu Gao (1): hw/sd/sdhci: Block Size Register bits [14:12] is lost Luc Michel (1): mailmap: update email addresses for Luc Michel Marc-André Lureau (2): memory: drop needless argument memory: follow Error API guidelines Philippe Mathieu-Daudé (36): buildsys: Only display Objective-C information when Objective-C is used hw/mips/malta: Use sdram_type enum from 'hw/i2c/smbus_eeprom.h' hw/mips: Merge 'hw/mips/cpudevs.h' with 'target/mips/cpu.h' hw/misc/mips_itu: Declare itc_reconfigure() in 'hw/misc/mips_itu.h' hw/misc/mips_itu: Make MIPSITUState target agnostic hw/pci-host/sh_pcic: Declare CPU QOM types using DEFINE_TYPES() macro hw/pci-host/sh_pcic: Correct PCI host / devfn#0 function names hw/pci-host/sh_pcic: Replace magic value by proper definition hw/sparc64/ebus: Access memory regions via pci_address_space_io() hw/acpi/pcihp: Clean up global variable shadowing in acpi_pcihp_init() hw/pci: Clean up global variable shadowing of address_space_io variable hw/s390x: Clean up global variable shadowing in quiesce_powerdown_req() hw/intc/apic: Use ERRP_GUARD() in apic_common_realize() hw/ppc/spapr_vio: Realize SPAPR_VIO_BRIDGE device before accessing it hw/ppc/pnv_xscom: Rename pnv_xscom_realize(Error **) -> pnv_xscom_init() hw/ppc/pnv_xscom: Move sysbus_mmio_map() call within pnv_xscom_init() hw/ppc/pnv_xscom: Do not use SysBus API to map local MMIO region hw/ppc/pnv: Do not use SysBus API to map local MMIO region hw/intc/spapr_xive: Move sysbus_init_mmio() calls around hw/intc/spapr_xive: Do not use SysBus API to map local MMIO region hw/i386/amd_iommu: Do not use SysBus API to map local MMIO region hw/i386/intel_iommu: Do not use SysBus API to map local MMIO region hw/misc/allwinner-dramc: Move sysbus_mmio_map call from init -> realize hw/misc/allwinner-dramc: Do not use SysBus API to map local MMIO region hw/pci-host/bonito: Do not use SysBus API to map local MMIO region hw/acpi: Realize ACPI_GED sysbus device before accessing it hw/arm/virt: Realize ARM_GICV2M sysbus device before accessing it hw/isa: Realize ISA bridge device before accessing it hw/s390x/css-bridge: Realize sysbus device before accessing it 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 ui/input: Constify QemuInputHandler structure Thomas Huth (4): MAINTAINERS: Add hw/input/lasips2.c to the HPPA machine section MAINTAINERS: Add include/hw/intc/loongson_liointc.h to the Loongson-3 virt section MAINTAINERS: Add include/hw/openrisc/ to the OpenRISC section MAINTAINERS: Extend entry to cover util/qemu-timer-common.c, too MAINTAINERS | 18 +++-- meson.build | 6 ++- include/hw/acpi/pcihp.h | 2 +- include/hw/core/cpu.h | 4 +- include/hw/core/sysemu-cpu-ops.h| 2 +- include/hw/mips/cpudevs.h | 14 --- include/hw/misc/mips_itu.h | 4 +- include/hw/pci/pci.h| 9 ++--- include/hw/ppc/pnv_xscom.h | 2 +- include/hw/virtio/virtio-input.h| 2 +- include/sysemu/memory_mapping.h | 2 +- include/ui/input.h | 2 +- target/i386/cpu.h | 2 +- target/mips/cpu.h | 7 ++-- chardev/msmouse.c | 2 +- chardev/wctablet.c | 2 +- hw/acpi/pcihp.c | 5 +-- hw/arm/virt.c | 5 +-- hw/
Re: [PULL v2 00/46] Misc HW/UI patches for 2023-10-19
Grr, I forgot to Cc qemu-devel@ ... On 20/10/23 14:51, Philippe Mathieu-Daudé wrote: The following changes since commit 0d239e513e0117e66fa739fb71a43b9383a108ff: Merge tag 'pull-lu-20231018' of https://gitlab.com/rth7680/qemu into staging (2023-10-19 10:20:57 -0700) are available in the Git repository at: https://github.com/philmd/qemu.git tags/hw-misc-20231020 for you to fetch changes up to 9f1b100236223d073915b8eedac3089ec64f8a6e: ui/input: Constify QemuInputHandler structure (2023-10-20 14:46:07 +0200) Since v1: - Dropped PC_SPEAKER patch - Added MAINTAINERS::util/qemu-timer-common.c patch Misc hardware patch queue - MAINTAINERS updates (Zoltan, Thomas) - Fix cutils::get_relocated_path on Windows host (Akihiko) - Housekeeping in Memory APIs (Marc-André) - SDHCI fix for SDMA transfer (Lu, Jianxian) - Various QOM/QDev/SysBus cleanups (Philippe) - Constify QemuInputHandler structure (Philippe) Akihiko Odaki (1): cutils: Fix get_relocated_path on Windows BALATON Zoltan (1): MAINTAINERS: Split vt82c686 out of fuloong2e Lu Gao (1): hw/sd/sdhci: Block Size Register bits [14:12] is lost Luc Michel (1): mailmap: update email addresses for Luc Michel Marc-André Lureau (2): memory: drop needless argument memory: follow Error API guidelines Philippe Mathieu-Daudé (36): buildsys: Only display Objective-C information when Objective-C is used hw/mips/malta: Use sdram_type enum from 'hw/i2c/smbus_eeprom.h' hw/mips: Merge 'hw/mips/cpudevs.h' with 'target/mips/cpu.h' hw/misc/mips_itu: Declare itc_reconfigure() in 'hw/misc/mips_itu.h' hw/misc/mips_itu: Make MIPSITUState target agnostic hw/pci-host/sh_pcic: Declare CPU QOM types using DEFINE_TYPES() macro hw/pci-host/sh_pcic: Correct PCI host / devfn#0 function names hw/pci-host/sh_pcic: Replace magic value by proper definition hw/sparc64/ebus: Access memory regions via pci_address_space_io() hw/acpi/pcihp: Clean up global variable shadowing in acpi_pcihp_init() hw/pci: Clean up global variable shadowing of address_space_io variable hw/s390x: Clean up global variable shadowing in quiesce_powerdown_req() hw/intc/apic: Use ERRP_GUARD() in apic_common_realize() hw/ppc/spapr_vio: Realize SPAPR_VIO_BRIDGE device before accessing it hw/ppc/pnv_xscom: Rename pnv_xscom_realize(Error **) -> pnv_xscom_init() hw/ppc/pnv_xscom: Move sysbus_mmio_map() call within pnv_xscom_init() hw/ppc/pnv_xscom: Do not use SysBus API to map local MMIO region hw/ppc/pnv: Do not use SysBus API to map local MMIO region hw/intc/spapr_xive: Move sysbus_init_mmio() calls around hw/intc/spapr_xive: Do not use SysBus API to map local MMIO region hw/i386/amd_iommu: Do not use SysBus API to map local MMIO region hw/i386/intel_iommu: Do not use SysBus API to map local MMIO region hw/misc/allwinner-dramc: Move sysbus_mmio_map call from init -> realize hw/misc/allwinner-dramc: Do not use SysBus API to map local MMIO region hw/pci-host/bonito: Do not use SysBus API to map local MMIO region hw/acpi: Realize ACPI_GED sysbus device before accessing it hw/arm/virt: Realize ARM_GICV2M sysbus device before accessing it hw/isa: Realize ISA bridge device before accessing it hw/s390x/css-bridge: Realize sysbus device before accessing it 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 ui/input: Constify QemuInputHandler structure Thomas Huth (4): MAINTAINERS: Add hw/input/lasips2.c to the HPPA machine section MAINTAINERS: Add include/hw/intc/loongson_liointc.h to the Loongson-3 virt section MAINTAINERS: Add include/hw/openrisc/ to the OpenRISC section MAINTAINERS: Extend entry to cover util/qemu-timer-common.c, too MAINTAINERS | 18 +++-- meson.build | 6 ++- include/hw/acpi/pcihp.h | 2 +- include/hw/core/cpu.h | 4 +- include/hw/core/sysemu-cpu-ops.h| 2 +- include/hw/mips/cpudevs.h | 14 --- include/hw/misc/mips_itu.h | 4 +- include/hw/pci/pci.h| 9 ++--- include/hw/ppc/pnv_xscom.h | 2 +- include/hw/virtio/virtio-input.h| 2 +- include/sysemu/memory_mapping.h | 2 +- include/ui/input.h | 2 +- target/i386/cpu.h | 2 +- target/mips/cpu.h | 7 ++-- chardev/msmouse.c | 2 +- chardev/wctablet.c |
[PULL v2 06/46] MAINTAINERS: Extend entry to cover util/qemu-timer-common.c, too
From: Thomas Huth We already cover util/qemu-timer.c in MAINTAINERS - change this entry to util/qemu-timer*.c so that it covers util/qemu-timer-common.c, too. Signed-off-by: Thomas Huth Reviewed-by: Philippe Mathieu-Daudé Message-ID: <20231020062142.525405-1-th...@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index fe11c98669..1b2c5b9e76 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2937,7 +2937,7 @@ F: include/qemu/main-loop.h F: include/sysemu/runstate.h F: include/sysemu/runstate-action.h F: util/main-loop.c -F: util/qemu-timer.c +F: util/qemu-timer*.c F: system/vl.c F: system/main.c F: system/cpus.c -- 2.41.0
[PULL v2 00/46] Misc HW/UI patches for 2023-10-19
The following changes since commit 0d239e513e0117e66fa739fb71a43b9383a108ff: Merge tag 'pull-lu-20231018' of https://gitlab.com/rth7680/qemu into staging (2023-10-19 10:20:57 -0700) are available in the Git repository at: https://github.com/philmd/qemu.git tags/hw-misc-20231020 for you to fetch changes up to 9f1b100236223d073915b8eedac3089ec64f8a6e: ui/input: Constify QemuInputHandler structure (2023-10-20 14:46:07 +0200) Since v1: - Dropped PC_SPEAKER patch - Added MAINTAINERS::util/qemu-timer-common.c patch Misc hardware patch queue - MAINTAINERS updates (Zoltan, Thomas) - Fix cutils::get_relocated_path on Windows host (Akihiko) - Housekeeping in Memory APIs (Marc-André) - SDHCI fix for SDMA transfer (Lu, Jianxian) - Various QOM/QDev/SysBus cleanups (Philippe) - Constify QemuInputHandler structure (Philippe) Akihiko Odaki (1): cutils: Fix get_relocated_path on Windows BALATON Zoltan (1): MAINTAINERS: Split vt82c686 out of fuloong2e Lu Gao (1): hw/sd/sdhci: Block Size Register bits [14:12] is lost Luc Michel (1): mailmap: update email addresses for Luc Michel Marc-André Lureau (2): memory: drop needless argument memory: follow Error API guidelines Philippe Mathieu-Daudé (36): buildsys: Only display Objective-C information when Objective-C is used hw/mips/malta: Use sdram_type enum from 'hw/i2c/smbus_eeprom.h' hw/mips: Merge 'hw/mips/cpudevs.h' with 'target/mips/cpu.h' hw/misc/mips_itu: Declare itc_reconfigure() in 'hw/misc/mips_itu.h' hw/misc/mips_itu: Make MIPSITUState target agnostic hw/pci-host/sh_pcic: Declare CPU QOM types using DEFINE_TYPES() macro hw/pci-host/sh_pcic: Correct PCI host / devfn#0 function names hw/pci-host/sh_pcic: Replace magic value by proper definition hw/sparc64/ebus: Access memory regions via pci_address_space_io() hw/acpi/pcihp: Clean up global variable shadowing in acpi_pcihp_init() hw/pci: Clean up global variable shadowing of address_space_io variable hw/s390x: Clean up global variable shadowing in quiesce_powerdown_req() hw/intc/apic: Use ERRP_GUARD() in apic_common_realize() hw/ppc/spapr_vio: Realize SPAPR_VIO_BRIDGE device before accessing it hw/ppc/pnv_xscom: Rename pnv_xscom_realize(Error **) -> pnv_xscom_init() hw/ppc/pnv_xscom: Move sysbus_mmio_map() call within pnv_xscom_init() hw/ppc/pnv_xscom: Do not use SysBus API to map local MMIO region hw/ppc/pnv: Do not use SysBus API to map local MMIO region hw/intc/spapr_xive: Move sysbus_init_mmio() calls around hw/intc/spapr_xive: Do not use SysBus API to map local MMIO region hw/i386/amd_iommu: Do not use SysBus API to map local MMIO region hw/i386/intel_iommu: Do not use SysBus API to map local MMIO region hw/misc/allwinner-dramc: Move sysbus_mmio_map call from init -> realize hw/misc/allwinner-dramc: Do not use SysBus API to map local MMIO region hw/pci-host/bonito: Do not use SysBus API to map local MMIO region hw/acpi: Realize ACPI_GED sysbus device before accessing it hw/arm/virt: Realize ARM_GICV2M sysbus device before accessing it hw/isa: Realize ISA bridge device before accessing it hw/s390x/css-bridge: Realize sysbus device before accessing it 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 ui/input: Constify QemuInputHandler structure Thomas Huth (4): MAINTAINERS: Add hw/input/lasips2.c to the HPPA machine section MAINTAINERS: Add include/hw/intc/loongson_liointc.h to the Loongson-3 virt section MAINTAINERS: Add include/hw/openrisc/ to the OpenRISC section MAINTAINERS: Extend entry to cover util/qemu-timer-common.c, too MAINTAINERS | 18 +++-- meson.build | 6 ++- include/hw/acpi/pcihp.h | 2 +- include/hw/core/cpu.h | 4 +- include/hw/core/sysemu-cpu-ops.h| 2 +- include/hw/mips/cpudevs.h | 14 --- include/hw/misc/mips_itu.h | 4 +- include/hw/pci/pci.h| 9 ++--- include/hw/ppc/pnv_xscom.h | 2 +- include/hw/virtio/virtio-input.h| 2 +- include/sysemu/memory_mapping.h | 2 +- include/ui/input.h | 2 +- target/i386/cpu.h | 2 +- target/mips/cpu.h | 7 ++-- chardev/msmouse.c | 2 +- chardev/wctablet.c | 2 +- hw/acpi/pcihp.c | 5 +-- hw/arm/virt.c | 5 +-- hw/block/vhost-user-blk.c | 4 +- hw/char/e
Re: [PATCH v2 10/13] migration: Improve example and documentation of vmstate_register()
On 10/20/23 05:07, Juan Quintela wrote: Signed-off-by: Juan Quintela Reviewed-by: Stefan Berger --- docs/devel/migration.rst | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst index c3e1400c0c..bfd8710c95 100644 --- a/docs/devel/migration.rst +++ b/docs/devel/migration.rst @@ -165,13 +165,17 @@ An example (from hw/input/pckbd.c) } }; -We are declaring the state with name "pckbd". -The ``version_id`` is 3, and the fields are 4 uint8_t in a KBDState structure. -We registered this with: +We are declaring the state with name "pckbd". The ``version_id`` is +3, and there are 4 uint8_t fields in the KBDState structure. We +registered this ``VMSTATEDescription`` with one of the following +functions. The first one will generate a device ``instance_id`` +different for each registration. Use the second one if you already +have an id that is different for each instance of the device: .. code:: c -vmstate_register(NULL, 0, &vmstate_kbd, s); +vmstate_register_any(NULL, &vmstate_kbd, s); +vmstate_register(NULL, instance_id, &vmstate_kbd, s); For devices that are ``qdev`` based, we can register the device in the class init function:
Re: [PATCH v2 14/22] qapi: Inline and remove QERR_IO_ERROR definition
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using:
>
> $ sed -i -e 's/QERR_IO_ERROR/"An IO error has occurred"/' \
> $(git grep -wl QERR_IO_ERROR)
>
> then manually removing the definition in include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé
> Reviewed-by: Juan Quintela
> ---
> include/qapi/qmp/qerror.h | 3 ---
> block/vmdk.c | 8
> blockdev.c| 2 +-
> dump/win_dump.c | 4 ++--
> migration/savevm.c| 4 ++--
> softmmu/cpus.c| 4 ++--
> 6 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index ac727d1c2d..d95c4b84b9 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,9 +17,6 @@
> * add new ones!
> */
>
> -#define QERR_IO_ERROR \
> -"An IO error has occurred"
> -
> #define QERR_MIGRATION_ACTIVE \
> "There's a migration process in progress"
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index e90649c8bf..6779a181f0 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -2246,12 +2246,12 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize,
> bool flat, bool compress,
> /* write all the data */
> ret = blk_co_pwrite(blk, 0, sizeof(magic), &magic, 0);
> if (ret < 0) {
> -error_setg(errp, QERR_IO_ERROR);
As far as I can tell, blk_co_pwrite() returns a negative errno code.
Which we throw away, and claim "IO error". I expect that to be
misleading at least sometimes.
I suspect the other uses of QERR_IO_ERROR are similarly problematic more
often than not.
Not your patch's problem, of course.
> +error_setg(errp, "An IO error has occurred");
We should spell it "I/O", unless we're reporting trouble with Jupiter's
moon.
> goto exit;
> }
> ret = blk_co_pwrite(blk, sizeof(magic), sizeof(header), &header, 0);
> if (ret < 0) {
> -error_setg(errp, QERR_IO_ERROR);
> +error_setg(errp, "An IO error has occurred");
> goto exit;
> }
>
[...]
Re: [PATCH v2 11/22] qapi: Inline QERR_INVALID_PARAMETER_VALUE definition (constant value)
Philippe Mathieu-Daudé writes:
> vcpu_dirty_limit
>
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using the following
> coccinelle semantic patch:
>
> @match@
> expression errp;
> constant param;
> constant value;
> @@
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, param, value);
>
> @script:python strformat depends on match@
> param << match.param;
> value << match.value;
> fixedfmt; // new var
> @@
> fixedfmt = "\"Parameter '%s' expects %s\"" % (param[1:-1], value[1:-1])
> coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
> @replace@
> expression match.errp;
> constant match.param;
> constant match.value;
> identifier strformat.fixedfmt;
> @@
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, param, value);
> +error_setg(errp, fixedfmt);
>
> Then manually splitting lines over 90 characters.
>
> Reviewed-by: Juan Quintela
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> backends/cryptodev-vhost-user.c | 4 +-
> backends/rng-egd.c | 4 +-
> backends/rng-random.c | 3 +-
> block/quorum.c | 3 +-
> blockdev.c | 9 ++--
> blockjob.c | 3 +-
> chardev/char.c | 3 +-
> hw/usb/redirect.c | 4 +-
> migration/migration.c | 4 +-
> migration/options.c | 92 ++---
> migration/page_cache.c | 8 +--
> migration/ram.c | 4 +-
> monitor/fds.c | 8 +--
> monitor/qmp-cmds.c | 3 +-
> net/filter-buffer.c | 3 +-
> net/filter.c| 7 ++-
> net/net.c | 9 ++--
> qga/commands-win32.c| 4 +-
> qom/object_interfaces.c | 2 +-
> qom/qom-qmp-cmds.c | 7 ++-
> softmmu/balloon.c | 2 +-
> softmmu/cpus.c | 3 +-
> softmmu/qdev-monitor.c | 11 ++--
> ui/ui-qmp-cmds.c| 2 +-
> util/qemu-option.c | 3 +-
> 25 files changed, 89 insertions(+), 116 deletions(-)
>
> diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
> index c3283ba84a..d93ccd5528 100644
> --- a/backends/cryptodev-vhost-user.c
> +++ b/backends/cryptodev-vhost-user.c
> @@ -136,8 +136,8 @@ cryptodev_vhost_claim_chardev(CryptoDevBackendVhostUser
> *s,
> Chardev *chr;
>
> if (s->chr_name == NULL) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> - "chardev", "a valid character device");
> +error_setg(errp,
> + "Parameter 'chardev' expects a valid character device");
> return NULL;
> }
>
> diff --git a/backends/rng-egd.c b/backends/rng-egd.c
> index 684c3cf3d6..8f101afadc 100644
> --- a/backends/rng-egd.c
> +++ b/backends/rng-egd.c
> @@ -90,8 +90,8 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
> Chardev *chr;
>
> if (s->chr_name == NULL) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> - "chardev", "a valid character device");
> +error_setg(errp,
> + "Parameter 'chardev' expects a valid character device");
> return;
> }
>
> diff --git a/backends/rng-random.c b/backends/rng-random.c
> index 80eb5be138..9cb7d26cb5 100644
> --- a/backends/rng-random.c
> +++ b/backends/rng-random.c
> @@ -72,8 +72,7 @@ static void rng_random_opened(RngBackend *b, Error **errp)
> RngRandom *s = RNG_RANDOM(b);
>
> if (s->filename == NULL) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> - "filename", "a valid filename");
Misuse of QERR_INVALID_PARAMETER_VALUE.
This function is RngBackendClass method opened(), which runs within
interface UserCreatableClass method complete(), which runs right after
an object with this interface was created. "Parameters" need not exist
in this context.
The actual issue is property "filename" has not been set. So let's
report that: "property 'filename' is required".
Separate patch, to keep this one mechanical.
> +error_setg(errp, "Parameter 'filename' expects a valid filename");
> } else {
> s->fd = qemu_open_old(s->filename, O_RDONLY | O_NONBLOCK);
> if (s->fd == -1) {
> diff --git a/block/quorum.c b/block/quorum.c
> index 05220cab7f..8e9f279568 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -882,8 +882,7 @@ static int quorum_valid_threshold(int threshold, int
> num_children, Error **errp)
> {
>
> if (threshold < 1) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> - "vote-threshold", "a value >= 1");
> +error_setg(errp, "Parameter 'vote-threshol
Re: [PATCH 07/13] RFC migration: icp/server is a mess
On Fri Oct 20, 2023 at 6:33 PM AEST, Greg Kurz wrote:
> On Fri, 20 Oct 2023 17:49:38 +1000
> "Nicholas Piggin" wrote:
>
> > On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
> > > On Thu, 19 Oct 2023 21:08:25 +0200
> > > Juan Quintela wrote:
> > >
> > > > Current code does:
> > > > - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> > > > dependinfg on cpu number
> > > > - for newer machines, it register vmstate_icp with "icp/server" name
> > > > and instance 0
> > > > - now it unregisters "icp/server" for the 1st instance.
> > > >
> > > > This is wrong at many levels:
> > > > - we shouldn't have two VMSTATEDescriptions with the same name
> > > > - In case this is the only solution that we can came with, it needs to
> > > > be:
> > > > * register pre_2_10_vmstate_dummy_icp
> > > > * unregister pre_2_10_vmstate_dummy_icp
> > > > * register real vmstate_icp
> > > >
> > > > As the initialization of this machine is already complex enough, I
> > > > need help from PPC maintainers to fix this.
> > > >
> > > > Volunteers?
> > > >
> > > > CC: Cedric Le Goater
> > > > CC: Daniel Henrique Barboza
> > > > CC: David Gibson
> > > > CC: Greg Kurz
> > > >
> > > > Signed-off-by: Juan Quintela
> > > > ---
> > > > hw/ppc/spapr.c | 7 ++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index cb840676d3..8531d13492 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
> > > > *opaque)
> > > > }
> > > >
> > > > static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > > -.name = "icp/server",
> > > > +/*
> > > > + * Hack ahead. We can't have two devices with the same name and
> > > > + * instance id. So I rename this to pass make check.
> > > > + * Real help from people who knows the hardware is needed.
> > > > + */
> > > > +.name = "pre-2.10-icp/server",
> > > > .version_id = 1,
> > > > .minimum_version_id = 1,
> > > > .needed = pre_2_10_vmstate_dummy_icp_needed,
> > >
> > > I guess this fix is acceptable as well and a lot simpler than
> > > reverting the hack actually. Outcome is the same : drop
> > > compat with pseries-2.9 and older.
> > >
> > > Reviewed-by: Greg Kurz
> >
> > So the reason we can't have duplicate names registered, aside from it
> > surely going bad if we actually send or receive a stream at the point
> > they are registered, is the duplcate check introduced in patch 9? But
> > before that, this hack does seem to actually work because the duplicate
> > is unregistered right away.
> >
>
> Correct.
>
> > If I understand the workaround, there is an asymmetry in the migration
> > sequence in that receiving an unexpected object would cause a failure,
> > but going from newer to older would just skip some "expected" objects
> > and that didn't cause a problem. So you only have to deal with ignoring
> > the unexpected ones going form older to newer.
> >
>
> Correct.
>
> > Side question, is it possible to flag the problem of *not* receiving
> > an object that you did expect? That might be a source of bugs too.
> >
>
> AFAICR we try to only migrate state that differs from reset : the
> destination cannot really assume it will receive anything for a
> given device.
That's true, I guess you could always add some flag yourself if
you certainly need something.
>
> > Anyway, I wonder if we could fix this spapr problem by adding a special
> > case wild card instance matcher to ignore it? It's still a bit hacky
> > but maybe a bit nicer. I don't mind deprecating the machine soon if
> > you want to clear the wildcard hack away soon, but it would be nice to
> > separate the deprecation and removal from the fix, if possible.
> >
> > This patch is not tested but hopefully helps illustrate the idea.
> >
>
> I'm not sure this will fly with older QEMUs that don't know about
> VMSTATE_INSTANCE_ID_WILD... but I'll let Juan comment on that.
You could be right about that. He gave a simpler solution now
anyway.
Thanks,
Nick
Re: [PATCH v2 07/13] migration: Hack to maintain backwards compatibility for ppc
On Fri Oct 20, 2023 at 7:07 PM AEST, Juan Quintela wrote:
> Current code does:
> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> dependinfg on cpu number
> - for newer machines, it register vmstate_icp with "icp/server" name
> and instance 0
> - now it unregisters "icp/server" for the 1st instance.
>
> This is wrong at many levels:
> - we shouldn't have two VMSTATEDescriptions with the same name
> - In case this is the only solution that we can came with, it needs to
> be:
> * register pre_2_10_vmstate_dummy_icp
> * unregister pre_2_10_vmstate_dummy_icp
> * register real vmstate_icp
>
> Created vmstate_replace_hack_for_ppc() with warnings left and right
> that it is a hack.
Thanks. We'll look at deprecating 2.9 soon so this can all be removed.
Reviewed-by: Nicholas Piggin
>
> CC: Cedric Le Goater
> CC: Daniel Henrique Barboza
> CC: David Gibson
> CC: Greg Kurz
>
> Signed-off-by: Juan Quintela
> ---
> include/migration/vmstate.h | 11 +++
> hw/intc/xics.c | 17 +++--
> hw/ppc/spapr.c | 25 +++--
> migration/savevm.c | 18 ++
> 4 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9ca7e9cc48..65deaecc92 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1230,6 +1230,17 @@ static inline int vmstate_register(VMStateIf *obj, int
> instance_id,
>opaque, -1, 0, NULL);
> }
>
> +/**
> + * vmstate_replace_hack_for_ppc() - ppc used to abuse vmstate_register
> + *
> + * Don't even think about using this function in new code.
> + *
> + * Returns: 0 on success, -1 on failure
> + */
> +int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
> + const VMStateDescription *vmsd,
> + void *opaque);
> +
> /**
> * vmstate_register_any() - legacy function to register state
> * serialisation description and let the function choose the id
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index c7f8abd71e..a03979e72a 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -335,8 +335,21 @@ static void icp_realize(DeviceState *dev, Error **errp)
> return;
> }
> }
> -
> -vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> +/*
> + * The way that pre_2_10_icp is handling is really, really hacky.
> + * We used to have here this call:
> + *
> + * vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> + *
> + * But we were doing:
> + * pre_2_10_vmstate_register_dummy_icp()
> + * this vmstate_register()
> + * pre_2_10_vmstate_unregister_dummy_icp()
> + *
> + * So for a short amount of time we had to vmstate entries with
> + * the same name. This fixes it.
> + */
> +vmstate_replace_hack_for_ppc(NULL, icp->cs->cpu_index,
> &vmstate_icp_server, icp);
> }
>
> static void icp_unrealize(DeviceState *dev)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..a75cf475ad 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -143,6 +143,11 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
> *opaque)
> }
>
> static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> +/*
> + * Hack ahead. We can't have two devices with the same name and
> + * instance id. So I rename this to pass make check.
> + * Real help from people who knows the hardware is needed.
> + */
> .name = "icp/server",
> .version_id = 1,
> .minimum_version_id = 1,
> @@ -155,16 +160,32 @@ static const VMStateDescription
> pre_2_10_vmstate_dummy_icp = {
> },
> };
>
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * You have to remove vmstate_replace_hack_for_ppc() when you remove
> + * the machine types that need the following function.
> + */
> static void pre_2_10_vmstate_register_dummy_icp(int i)
> {
> vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
> (void *)(uintptr_t) i);
> }
>
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * You have to remove vmstate_replace_hack_for_ppc() when you remove
> + * the machine types that need the following function.
> + */
> static void pre_2_10_vmstate_unregister_dummy_icp(int i)
> {
> -vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
> - (void *)(uintptr_t) i);
> +/*
> + * This used to be:
> + *
> + *vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
> + * (void *)(uintptr_t) i);
> + */
> }
>
> int spapr_max_server_number(SpaprMachineState *spapr)
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 8622f229e5..d3a30686d4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
>
Re: [PATCH v2 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices
On 20/10/2023 11.54, Juan Quintela wrote:
Christian Borntraeger wrote:
Am 20.10.23 um 11:07 schrieb Juan Quintela:
Just with make check I can see that we can have more than one of this
devices, so use ANY.
ok 5 /s390x/device/introspect/abstract-interfaces
...
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195:
kill_qemu() tried to terminate QEMU process but encountered exit
status 1 (expected 0)
Aborted (core dumped)
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
hw/s390x/s390-skeys.c| 3 ++-
hw/s390x/s390-stattrib.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
Actually both devices should be theŕe only once - I think.
Reverting the patch (but with the check that we don't add duplicated
entries):
# Testing device 's390-skeys-qemu'
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:194: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
$
This is device-intraspect-test.
Somehow this function decides that you can hotplug this two s390
devices, if that is not the case, they need to be marked somehow not
hot-plugabble.
Sorry, no, it's not hot-plugging what is happening here, it's device
introspection. That means it should always be possible to create a second
instance of a device for introspection - it must just not be realized if
there can be only one instance.
Looking at the code here:
tatic inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
Error **errp)
{
S390SKeysState *ss = S390_SKEYS(obj);
/* Prevent double registration of savevm handler */
if (ss->migration_enabled == value) {
return;
}
ss->migration_enabled = value;
if (ss->migration_enabled) {
register_savevm_live(TYPE_S390_SKEYS, 0, 1,
&savevm_s390_storage_keys, ss);
} else {
unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
}
}
static void s390_skeys_instance_init(Object *obj)
{
object_property_add_bool(obj, "migration-enabled",
s390_skeys_get_migration_enabled,
s390_skeys_set_migration_enabled);
object_property_set_bool(obj, "migration-enabled", true, NULL);
}
I think the problem is the object_property_set_bool() in the
_instance_init() function. The setting of the property should maybe rather
happen during the realization instead?
Thomas
Re: [PULL 00/46] Misc HW/UI patches for 2023-10-19
On 19/10/23 23:17, Philippe Mathieu-Daudé wrote: The following changes since commit 0d239e513e0117e66fa739fb71a43b9383a108ff: Misc hardware patch queue - MAINTAINERS updates (Zoltan, Thomas) - Fix cutils::get_relocated_path on Windows host (Akihiko) - Housekeeping in Memory APIs (Marc-André) - SDHCI fix for SDMA transfer (Lu, Jianxian) - Various QOM/QDev/SysBus cleanups (Philippe) - Constify QemuInputHandler structure (Philippe) Stefan, a change has been requested for #30 "hw/audio/pcspk: Inline pcspk_init()", so please drop this pull request. Regards, Phil.
Re: [PATCH v2 10/22] qapi: Correct error message for 'vcpu_dirty_limit' parameter
Markus Armbruster wrote:
> Philippe Mathieu-Daudé writes:
>
>> QERR_INVALID_PARAMETER_VALUE is defined as:
>>
>> #define QERR_INVALID_PARAMETER_VALUE \
>> "Parameter '%s' expects %s"
>>
>> The current error is formatted as:
>>
>> "Parameter 'vcpu_dirty_limit' expects is invalid, it must greater then 1
>> MB/s"
>>
>> Replace by:
>>
>> "Parameter 'vcpu_dirty_limit' is invalid, it must greater then 1 MB/s"
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> migration/options.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index 1d1e1321b0..79fce0c3a9 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -1163,9 +1163,8 @@ bool migrate_params_check(MigrationParameters *params,
>> Error **errp)
>>
>> if (params->has_vcpu_dirty_limit &&
>> (params->vcpu_dirty_limit < 1)) {
>> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>> - "vcpu_dirty_limit",
>> - "is invalid, it must greater then 1 MB/s");
>> +error_setg(errp, "Parameter 'vcpu_dirty_limit' is invalid,"
>> + " it must greater then 1 MB/s");
>> return false;
>> }
>
> Make that "greater than", please.
>
> Arrgh, the unit is MB/s even in QMP:
>
> # @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration.
> # Defaults to 1. (Since 8.1)
>
> Should be Bytes. Escaped review, and now it's too late to fix.
I want a Time Machine.
I want a Time Machine.
Wait, if I had a Time Machine I would not be fixing old bugs O:-)
Later, Juan.
Re: [PATCH v2 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices
Christian Borntraeger wrote:
> Am 20.10.23 um 11:07 schrieb Juan Quintela:
>> Just with make check I can see that we can have more than one of this
>> devices, so use ANY.
>> ok 5 /s390x/device/introspect/abstract-interfaces
>> ...
>> Broken pipe
>> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195:
>> kill_qemu() tried to terminate QEMU process but encountered exit
>> status 1 (expected 0)
>> Aborted (core dumped)
>> Reviewed-by: Stefan Berger
>> Signed-off-by: Juan Quintela
>> ---
>> hw/s390x/s390-skeys.c| 3 ++-
>> hw/s390x/s390-stattrib.c | 3 ++-
>> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> Actually both devices should be theŕe only once - I think.
Reverting the patch (but with the check that we don't add duplicated
entries):
# Testing device 's390-skeys-qemu'
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:194: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
$
This is device-intraspect-test.
Somehow this function decides that you can hotplug this two s390
devices, if that is not the case, they need to be marked somehow not
hot-plugabble.
static void test_one_device(QTestState *qts, const char *type)
{
QDict *resp;
char *help, *escaped;
GRegex *comma;
g_test_message("Testing device '%s'", type);
resp = qtest_qmp(qts, "{'execute': 'device-list-properties',"
" 'arguments': {'typename': %s}}",
type);
qobject_unref(resp);
comma = g_regex_new(",", 0, 0, NULL);
escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0, NULL);
g_regex_unref(comma);
help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
g_free(help);
g_free(escaped);
}
Thanks, Juan.
>
>> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
>> index 5024faf411..ef089e1967 100644
>> --- a/hw/s390x/s390-skeys.c
>> +++ b/hw/s390x/s390-skeys.c
>> @@ -22,6 +22,7 @@
>> #include "sysemu/kvm.h"
>> #include "migration/qemu-file-types.h"
>> #include "migration/register.h"
>> +#include "migration/vmstate.h"
>> #define S390_SKEYS_BUFFER_SIZE (128 * KiB) /* Room for 128k
>> storage keys */
>> #define S390_SKEYS_SAVE_FLAG_EOS 0x01
>> @@ -457,7 +458,7 @@ static inline void
>> s390_skeys_set_migration_enabled(Object *obj, bool value,
>> ss->migration_enabled = value;
>> if (ss->migration_enabled) {
>> -register_savevm_live(TYPE_S390_SKEYS, 0, 1,
>> +register_savevm_live(TYPE_S390_SKEYS, VMSTATE_INSTANCE_ID_ANY, 1,
>>&savevm_s390_storage_keys, ss);
>> } else {
>> unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
>> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
>> index 220e845d12..055d382c3c 100644
>> --- a/hw/s390x/s390-stattrib.c
>> +++ b/hw/s390x/s390-stattrib.c
>> @@ -13,6 +13,7 @@
>> #include "qemu/units.h"
>> #include "migration/qemu-file.h"
>> #include "migration/register.h"
>> +#include "migration/vmstate.h"
>> #include "hw/s390x/storage-attributes.h"
>> #include "qemu/error-report.h"
>> #include "exec/ram_addr.h"
>> @@ -380,7 +381,7 @@ static void s390_stattrib_instance_init(Object *obj)
>> {
>> S390StAttribState *sas = S390_STATTRIB(obj);
>> -register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
>> +register_savevm_live(TYPE_S390_STATTRIB, VMSTATE_INSTANCE_ID_ANY, 0,
>>&savevm_s390_stattrib_handlers, sas);
>> object_property_add_bool(obj, "migration-enabled",
Re: [PATCH v2 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices
Am 20.10.23 um 11:07 schrieb Juan Quintela:
Just with make check I can see that we can have more than one of this
devices, so use ANY.
ok 5 /s390x/device/introspect/abstract-interfaces
...
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
hw/s390x/s390-skeys.c| 3 ++-
hw/s390x/s390-stattrib.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
Actually both devices should be theŕe only once - I think.
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5024faf411..ef089e1967 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -22,6 +22,7 @@
#include "sysemu/kvm.h"
#include "migration/qemu-file-types.h"
#include "migration/register.h"
+#include "migration/vmstate.h"
#define S390_SKEYS_BUFFER_SIZE (128 * KiB) /* Room for 128k storage keys */
#define S390_SKEYS_SAVE_FLAG_EOS 0x01
@@ -457,7 +458,7 @@ static inline void s390_skeys_set_migration_enabled(Object
*obj, bool value,
ss->migration_enabled = value;
if (ss->migration_enabled) {
-register_savevm_live(TYPE_S390_SKEYS, 0, 1,
+register_savevm_live(TYPE_S390_SKEYS, VMSTATE_INSTANCE_ID_ANY, 1,
&savevm_s390_storage_keys, ss);
} else {
unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 220e845d12..055d382c3c 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -13,6 +13,7 @@
#include "qemu/units.h"
#include "migration/qemu-file.h"
#include "migration/register.h"
+#include "migration/vmstate.h"
#include "hw/s390x/storage-attributes.h"
#include "qemu/error-report.h"
#include "exec/ram_addr.h"
@@ -380,7 +381,7 @@ static void s390_stattrib_instance_init(Object *obj)
{
S390StAttribState *sas = S390_STATTRIB(obj);
-register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
+register_savevm_live(TYPE_S390_STATTRIB, VMSTATE_INSTANCE_ID_ANY, 0,
&savevm_s390_stattrib_handlers, sas);
object_property_add_bool(obj, "migration-enabled",
[PATCH v2 12/13] migration: Use vmstate_register_any() for eeprom93xx
We can have more than one eeprom93xx.
For instance:
e100_nic_realize() -> eeprom93xx_new()
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
hw/nvram/eeprom93xx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/nvram/eeprom93xx.c b/hw/nvram/eeprom93xx.c
index 1081e2cc0d..57d63638d7 100644
--- a/hw/nvram/eeprom93xx.c
+++ b/hw/nvram/eeprom93xx.c
@@ -321,7 +321,7 @@ eeprom_t *eeprom93xx_new(DeviceState *dev, uint16_t nwords)
/* Output DO is tristate, read results in 1. */
eeprom->eedo = 1;
logout("eeprom = 0x%p, nwords = %u\n", eeprom, nwords);
-vmstate_register(VMSTATE_IF(dev), 0, &vmstate_eeprom, eeprom);
+vmstate_register_any(VMSTATE_IF(dev), &vmstate_eeprom, eeprom);
return eeprom;
}
--
2.41.0
[PATCH v2 09/13] migration: Check in savevm_state_handler_insert for dups
From: Peter Xu
Before finally register one SaveStateEntry, we detect for duplicated
entries. This could be helpful to notify us asap instead of get
silent migration failures which could be hard to diagnose.
For example, this patch will generate a message like this (if without
previous fixes on x2apic) as long as we wants to boot a VM instance
with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will
bail out even before VM starts:
savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic,
instance_id=0x0
Suggested-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
Reviewed-by: Juan Quintela
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Juan Quintela
---
migration/savevm.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/migration/savevm.c b/migration/savevm.c
index d3a30686d4..3e0ece84e8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -237,6 +237,8 @@ static SaveState savevm_state = {
.global_section_id = 0,
};
+static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id);
+
static bool should_validate_capability(int capability)
{
assert(capability >= 0 && capability < MIGRATION_CAPABILITY__MAX);
@@ -716,6 +718,18 @@ static void savevm_state_handler_insert(SaveStateEntry
*nse)
assert(priority <= MIG_PRI_MAX);
+/*
+ * This should never happen otherwise migration will probably fail
+ * silently somewhere because we can be wrongly applying one
+ * object properties upon another one. Bail out ASAP.
+ */
+if (find_se(nse->idstr, nse->instance_id)) {
+error_report("%s: Detected duplicate SaveStateEntry: "
+ "id=%s, instance_id=0x%"PRIx32, __func__,
+ nse->idstr, nse->instance_id);
+exit(EXIT_FAILURE);
+}
+
for (i = priority - 1; i >= 0; i--) {
se = savevm_state.handler_pri_head[i];
if (se != NULL) {
--
2.41.0
[PATCH v2 07/13] migration: Hack to maintain backwards compatibility for ppc
Current code does:
- register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
dependinfg on cpu number
- for newer machines, it register vmstate_icp with "icp/server" name
and instance 0
- now it unregisters "icp/server" for the 1st instance.
This is wrong at many levels:
- we shouldn't have two VMSTATEDescriptions with the same name
- In case this is the only solution that we can came with, it needs to
be:
* register pre_2_10_vmstate_dummy_icp
* unregister pre_2_10_vmstate_dummy_icp
* register real vmstate_icp
Created vmstate_replace_hack_for_ppc() with warnings left and right
that it is a hack.
CC: Cedric Le Goater
CC: Daniel Henrique Barboza
CC: David Gibson
CC: Greg Kurz
Signed-off-by: Juan Quintela
---
include/migration/vmstate.h | 11 +++
hw/intc/xics.c | 17 +++--
hw/ppc/spapr.c | 25 +++--
migration/savevm.c | 18 ++
4 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9ca7e9cc48..65deaecc92 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1230,6 +1230,17 @@ static inline int vmstate_register(VMStateIf *obj, int
instance_id,
opaque, -1, 0, NULL);
}
+/**
+ * vmstate_replace_hack_for_ppc() - ppc used to abuse vmstate_register
+ *
+ * Don't even think about using this function in new code.
+ *
+ * Returns: 0 on success, -1 on failure
+ */
+int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
+ const VMStateDescription *vmsd,
+ void *opaque);
+
/**
* vmstate_register_any() - legacy function to register state
* serialisation description and let the function choose the id
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index c7f8abd71e..a03979e72a 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -335,8 +335,21 @@ static void icp_realize(DeviceState *dev, Error **errp)
return;
}
}
-
-vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
+/*
+ * The way that pre_2_10_icp is handling is really, really hacky.
+ * We used to have here this call:
+ *
+ * vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
+ *
+ * But we were doing:
+ * pre_2_10_vmstate_register_dummy_icp()
+ * this vmstate_register()
+ * pre_2_10_vmstate_unregister_dummy_icp()
+ *
+ * So for a short amount of time we had to vmstate entries with
+ * the same name. This fixes it.
+ */
+vmstate_replace_hack_for_ppc(NULL, icp->cs->cpu_index,
&vmstate_icp_server, icp);
}
static void icp_unrealize(DeviceState *dev)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cb840676d3..a75cf475ad 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -143,6 +143,11 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
}
static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
+/*
+ * Hack ahead. We can't have two devices with the same name and
+ * instance id. So I rename this to pass make check.
+ * Real help from people who knows the hardware is needed.
+ */
.name = "icp/server",
.version_id = 1,
.minimum_version_id = 1,
@@ -155,16 +160,32 @@ static const VMStateDescription
pre_2_10_vmstate_dummy_icp = {
},
};
+/*
+ * See comment in hw/intc/xics.c:icp_realize()
+ *
+ * You have to remove vmstate_replace_hack_for_ppc() when you remove
+ * the machine types that need the following function.
+ */
static void pre_2_10_vmstate_register_dummy_icp(int i)
{
vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
(void *)(uintptr_t) i);
}
+/*
+ * See comment in hw/intc/xics.c:icp_realize()
+ *
+ * You have to remove vmstate_replace_hack_for_ppc() when you remove
+ * the machine types that need the following function.
+ */
static void pre_2_10_vmstate_unregister_dummy_icp(int i)
{
-vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
- (void *)(uintptr_t) i);
+/*
+ * This used to be:
+ *
+ *vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
+ * (void *)(uintptr_t) i);
+ */
}
int spapr_max_server_number(SpaprMachineState *spapr)
diff --git a/migration/savevm.c b/migration/savevm.c
index 8622f229e5..d3a30686d4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -846,6 +846,24 @@ static void vmstate_check(const VMStateDescription *vmsd)
}
}
+/*
+ * See comment in hw/intc/xics.c:icp_realize()
+ *
+ * This function can be removed when
+ * pre_2_10_vmstate_register_dummy_icp() is removed.
+ */
+int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
+ const VMStateDescription *vmsd,
+ void *opaque)
+{
+SaveS
[PATCH v2 04/13] migration: Use vmstate_register_any() for ipmi-bt*
Otherwise device-introspection-test fails.
$ ./tests/qtest/device-introspect-test
...
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
hw/ipmi/ipmi_bmc_extern.c | 2 +-
hw/ipmi/ipmi_bmc_sim.c| 2 +-
hw/ipmi/isa_ipmi_bt.c | 2 +-
hw/ipmi/isa_ipmi_kcs.c| 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index e232d35ba2..324a2c8835 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -504,7 +504,7 @@ static void ipmi_bmc_extern_init(Object *obj)
IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
-vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
+vmstate_register_any(NULL, &vmstate_ipmi_bmc_extern, ibe);
}
static void ipmi_bmc_extern_finalize(Object *obj)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 905e091094..404db5d5bc 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -2188,7 +2188,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error
**errp)
ibs->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ipmi_timeout, ibs);
-vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
+vmstate_register_any(NULL, &vmstate_ipmi_sim, ibs);
}
static Property ipmi_sim_properties[] = {
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index a83e7243d6..afb76b548a 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -125,7 +125,7 @@ static void isa_ipmi_bt_init(Object *obj)
ipmi_bmc_find_and_link(obj, (Object **) &iib->bt.bmc);
-vmstate_register(NULL, 0, &vmstate_ISAIPMIBTDevice, iib);
+vmstate_register_any(NULL, &vmstate_ISAIPMIBTDevice, iib);
}
static void *isa_ipmi_bt_get_backend_data(IPMIInterface *ii)
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index b2ed70b9da..5ab63b2fcf 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -132,7 +132,7 @@ static void isa_ipmi_kcs_init(Object *obj)
* IPMI device, so receive it, but transmit a different
* version.
*/
-vmstate_register(NULL, 0, &vmstate_ISAIPMIKCSDevice, iik);
+vmstate_register_any(NULL, &vmstate_ISAIPMIKCSDevice, iik);
}
static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
--
2.41.0
[PATCH v2 02/13] migration: Use vmstate_register_any()
This are the easiest cases, where we were already using
VMSTATE_INSTANCE_ID_ANY.
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
backends/dbus-vmstate.c | 3 +--
backends/tpm/tpm_emulator.c | 3 +--
hw/i2c/core.c | 2 +-
hw/input/adb.c | 2 +-
hw/input/ads7846.c | 2 +-
hw/input/stellaris_input.c | 3 +--
hw/net/eepro100.c | 3 +--
hw/pci/pci.c| 2 +-
hw/ppc/spapr_nvdimm.c | 3 +--
hw/timer/arm_timer.c| 2 +-
hw/virtio/virtio-mem.c | 4 ++--
11 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
index 57369ec0f2..a9d8cb0acd 100644
--- a/backends/dbus-vmstate.c
+++ b/backends/dbus-vmstate.c
@@ -426,8 +426,7 @@ dbus_vmstate_complete(UserCreatable *uc, Error **errp)
return;
}
-if (vmstate_register(VMSTATE_IF(self), VMSTATE_INSTANCE_ID_ANY,
- &dbus_vmstate, self) < 0) {
+if (vmstate_register_any(VMSTATE_IF(self), &dbus_vmstate, self) < 0) {
error_setg(errp, "Failed to register vmstate");
}
}
diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 402a2d6312..8920b75251 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -978,8 +978,7 @@ static void tpm_emulator_inst_init(Object *obj)
qemu_add_vm_change_state_handler(tpm_emulator_vm_state_change,
tpm_emu);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
- &vmstate_tpm_emulator, obj);
+vmstate_register_any(NULL, &vmstate_tpm_emulator, obj);
}
/*
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index bed594fe59..879a1d45cb 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -64,7 +64,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
bus = I2C_BUS(qbus_new(TYPE_I2C_BUS, parent, name));
QLIST_INIT(&bus->current_devs);
QSIMPLEQ_INIT(&bus->pending_masters);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_i2c_bus, bus);
+vmstate_register_any(NULL, &vmstate_i2c_bus, bus);
return bus;
}
diff --git a/hw/input/adb.c b/hw/input/adb.c
index 214ae6f42b..8aed0da2cd 100644
--- a/hw/input/adb.c
+++ b/hw/input/adb.c
@@ -247,7 +247,7 @@ static void adb_bus_realize(BusState *qbus, Error **errp)
adb_bus->autopoll_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, adb_autopoll,
adb_bus);
-vmstate_register(NULL, -1, &vmstate_adb_bus, adb_bus);
+vmstate_register_any(NULL, &vmstate_adb_bus, adb_bus);
}
static void adb_bus_unrealize(BusState *qbus)
diff --git a/hw/input/ads7846.c b/hw/input/ads7846.c
index dc0998ac79..91116c6bdb 100644
--- a/hw/input/ads7846.c
+++ b/hw/input/ads7846.c
@@ -158,7 +158,7 @@ static void ads7846_realize(SSIPeripheral *d, Error **errp)
ads7846_int_update(s);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_ads7846, s);
+vmstate_register_any(NULL, &vmstate_ads7846, s);
}
static void ads7846_class_init(ObjectClass *klass, void *data)
diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c
index e6ee5e11f1..a58721c8cd 100644
--- a/hw/input/stellaris_input.c
+++ b/hw/input/stellaris_input.c
@@ -88,6 +88,5 @@ void stellaris_gamepad_init(int n, qemu_irq *irq, const int
*keycode)
}
s->num_buttons = n;
qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
- &vmstate_stellaris_gamepad, s);
+vmstate_register_any(NULL, &vmstate_stellaris_gamepad, s);
}
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index dc07984ae9..94ce9e18ff 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1883,8 +1883,7 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error
**errp)
s->vmstate = g_memdup(&vmstate_eepro100, sizeof(vmstate_eepro100));
s->vmstate->name = qemu_get_queue(s->nic)->model;
-vmstate_register(VMSTATE_IF(&pci_dev->qdev), VMSTATE_INSTANCE_ID_ANY,
- s->vmstate, s);
+vmstate_register_any(VMSTATE_IF(&pci_dev->qdev), s->vmstate, s);
}
static void eepro100_instance_init(Object *obj)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b0d21bf43a..294c3c38ea 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -147,7 +147,7 @@ static void pci_bus_realize(BusState *qbus, Error **errp)
bus->machine_done.notify = pcibus_machine_done;
qemu_add_machine_init_done_notifier(&bus->machine_done);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_pcibus, bus);
+vmstate_register_any(NULL, &vmstate_pcibus, bus);
}
static void pcie_bus_realize(BusState *qbus, Error **errp)
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index b2f009c816..ad7afe7544 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -876,8 +876,7 @@ static void spapr_nvdimm_realize(NVDIMMDevice *dimm, Error
**errp)
s_nvdimm->hc
[PATCH v2 05/13] migration: Use VMSTATE_INSTANCE_ID_ANY for slirp
Each user network conection create a new slirp instance. We register
more than one slirp instance for number 0.
qemu-system-x86_64: -netdev user,id=hs1: savevm_state_handler_insert: Detected
duplicate SaveStateEntry: id=slirp, instance_id=0x0
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
net/slirp.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/slirp.c b/net/slirp.c
index c33b3e02e7..25b49c4526 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -46,6 +46,7 @@
#include "qapi/qmp/qdict.h"
#include "util.h"
#include "migration/register.h"
+#include "migration/vmstate.h"
#include "migration/qemu-file-types.h"
static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
@@ -659,8 +660,8 @@ static int net_slirp_init(NetClientState *peer, const char
*model,
* specific version?
*/
g_assert(slirp_state_version() == 4);
-register_savevm_live("slirp", 0, slirp_state_version(),
- &savevm_slirp_state, s->slirp);
+register_savevm_live("slirp", VMSTATE_INSTANCE_ID_ANY,
+ slirp_state_version(), &savevm_slirp_state, s->slirp);
s->poll_notifier.notify = net_slirp_poll_notify;
main_loop_poll_add_notifier(&s->poll_notifier);
--
2.41.0
[PATCH v2 13/13] migration: Use vmstate_register_any() for vmware_vga
I have no idea if we can have more than one vmware_vga device, so play it safe. Reviewed-by: Stefan Berger Signed-off-by: Juan Quintela --- hw/display/vmware_vga.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c index 09591fbd39..7490d43881 100644 --- a/hw/display/vmware_vga.c +++ b/hw/display/vmware_vga.c @@ -1264,7 +1264,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s, vga_common_init(&s->vga, OBJECT(dev), &error_fatal); vga_init(&s->vga, OBJECT(dev), address_space, io, true); -vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga); +vmstate_register_any(NULL, &vmstate_vga_common, &s->vga); s->new_depth = 32; } -- 2.41.0
[PATCH v2 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices
Just with make check I can see that we can have more than one of this
devices, so use ANY.
ok 5 /s390x/device/introspect/abstract-interfaces
...
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
hw/s390x/s390-skeys.c| 3 ++-
hw/s390x/s390-stattrib.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5024faf411..ef089e1967 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -22,6 +22,7 @@
#include "sysemu/kvm.h"
#include "migration/qemu-file-types.h"
#include "migration/register.h"
+#include "migration/vmstate.h"
#define S390_SKEYS_BUFFER_SIZE (128 * KiB) /* Room for 128k storage keys */
#define S390_SKEYS_SAVE_FLAG_EOS 0x01
@@ -457,7 +458,7 @@ static inline void s390_skeys_set_migration_enabled(Object
*obj, bool value,
ss->migration_enabled = value;
if (ss->migration_enabled) {
-register_savevm_live(TYPE_S390_SKEYS, 0, 1,
+register_savevm_live(TYPE_S390_SKEYS, VMSTATE_INSTANCE_ID_ANY, 1,
&savevm_s390_storage_keys, ss);
} else {
unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 220e845d12..055d382c3c 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -13,6 +13,7 @@
#include "qemu/units.h"
#include "migration/qemu-file.h"
#include "migration/register.h"
+#include "migration/vmstate.h"
#include "hw/s390x/storage-attributes.h"
#include "qemu/error-report.h"
#include "exec/ram_addr.h"
@@ -380,7 +381,7 @@ static void s390_stattrib_instance_init(Object *obj)
{
S390StAttribState *sas = S390_STATTRIB(obj);
-register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
+register_savevm_live(TYPE_S390_STATTRIB, VMSTATE_INSTANCE_ID_ANY, 0,
&savevm_s390_stattrib_handlers, sas);
object_property_add_bool(obj, "migration-enabled",
--
2.41.0
[PATCH v2 10/13] migration: Improve example and documentation of vmstate_register()
Signed-off-by: Juan Quintela --- docs/devel/migration.rst | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst index c3e1400c0c..bfd8710c95 100644 --- a/docs/devel/migration.rst +++ b/docs/devel/migration.rst @@ -165,13 +165,17 @@ An example (from hw/input/pckbd.c) } }; -We are declaring the state with name "pckbd". -The ``version_id`` is 3, and the fields are 4 uint8_t in a KBDState structure. -We registered this with: +We are declaring the state with name "pckbd". The ``version_id`` is +3, and there are 4 uint8_t fields in the KBDState structure. We +registered this ``VMSTATEDescription`` with one of the following +functions. The first one will generate a device ``instance_id`` +different for each registration. Use the second one if you already +have an id that is different for each instance of the device: .. code:: c -vmstate_register(NULL, 0, &vmstate_kbd, s); +vmstate_register_any(NULL, &vmstate_kbd, s); +vmstate_register(NULL, instance_id, &vmstate_kbd, s); For devices that are ``qdev`` based, we can register the device in the class init function: -- 2.41.0
[PATCH v2 08/13] migration: vmstate_register() check that instance_id is valid
Signed-off-by: Juan Quintela
---
include/migration/vmstate.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 65deaecc92..896c3f69d2 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -28,6 +28,7 @@
#define QEMU_VMSTATE_H
#include "hw/vmstate-if.h"
+#include "qemu/error-report.h"
typedef struct VMStateInfo VMStateInfo;
typedef struct VMStateField VMStateField;
@@ -1226,6 +1227,11 @@ static inline int vmstate_register(VMStateIf *obj, int
instance_id,
const VMStateDescription *vmsd,
void *opaque)
{
+if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
+error_report("vmstate_register: Invalid device: %s instance_id: %d",
+ vmsd->name, instance_id);
+return -1;
+}
return vmstate_register_with_alias_id(obj, instance_id, vmsd,
opaque, -1, 0, NULL);
}
--
2.41.0
[PATCH v2 11/13] migration: Use vmstate_register_any() for audio
We can have more than one audio card.
void audio_init_audiodevs(void)
{
AudiodevListEntry *e;
QSIMPLEQ_FOREACH(e, &audiodevs, next) {
audio_init(e->dev, &error_fatal);
}
}
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
audio/audio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/audio/audio.c b/audio/audio.c
index e9815d6812..f91e05b72c 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1781,7 +1781,7 @@ static AudioState *audio_init(Audiodev *dev, Error **errp)
QTAILQ_INSERT_TAIL(&audio_states, s, list);
QLIST_INIT (&s->card_head);
-vmstate_register (NULL, 0, &vmstate_audio, s);
+vmstate_register_any(NULL, &vmstate_audio, s);
return s;
out:
--
2.41.0
[PATCH v2 03/13] migration: Use vmstate_register_any() for isa-ide
Otherwise qom-test fails. ok 4 /i386/qom/x-remote qemu-system-i386: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=isa-ide, instance_id=0x0 Broken pipe ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0) Aborted (core dumped) $ Reviewed-by: Stefan Berger Signed-off-by: Juan Quintela --- hw/ide/isa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ide/isa.c b/hw/ide/isa.c index 95053e026f..ea60c08116 100644 --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); -vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); +vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s); ide_bus_register_restart_cb(&s->bus); } -- 2.41.0
[PATCH v2 01/13] migration: Create vmstate_register_any()
We have lots of cases where we are using an instance_id==0 when we
should be using VMSTATE_INSTANCE_ID_ANY (-1). Basically everything
that can have more than one needs to have a proper instance_id or -1
and the system will take one for it.
vmstate_register_any(): We register with -1.
Reviewed-by: Stefan Berger
Signed-off-by: Juan Quintela
---
include/migration/vmstate.h | 17 +
1 file changed, 17 insertions(+)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a31fb7293..9ca7e9cc48 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1230,6 +1230,23 @@ static inline int vmstate_register(VMStateIf *obj, int
instance_id,
opaque, -1, 0, NULL);
}
+/**
+ * vmstate_register_any() - legacy function to register state
+ * serialisation description and let the function choose the id
+ *
+ * New code shouldn't be using this function as QOM-ified devices have
+ * dc->vmsd to store the serialisation description.
+ *
+ * Returns: 0 on success, -1 on failure
+ */
+static inline int vmstate_register_any(VMStateIf *obj,
+ const VMStateDescription *vmsd,
+ void *opaque)
+{
+return vmstate_register_with_alias_id(obj, VMSTATE_INSTANCE_ID_ANY, vmsd,
+ opaque, -1, 0, NULL);
+}
+
void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
void *opaque);
--
2.41.0
[PATCH v2 00/13] migration: Check for duplicates on vmstate_register()
Hi Hi on this v2: - rebased on top of master (no conflicts) - handled reviewed by - improved documentation - Changed the ppc hack to maintain backwards compatibility. Please review. [v1] This series are based in a patch from Peter than check if a we try to register the same device with the same instance_id more than once. It was not merged when he sent it because it broke "make check". So I fixed all devices to be able to merge it. - I create vmstate_register_any(), its the same that vmstate_register(VMSTATE_INSTANCE_ID_ANY) - Later I check in vmstate_register() that they are not calling it with VMSTATE_INSTANCE_ID_ANY - After that I change vmstate_register() to make sure that we don't include a duplicate. And we get all the errors that I change in patches 3, 4, 5, 6, 7. After those patches: make check works again. And then I reviewed all the rest of vmstate_register() callers. There are the cases where they pass a device_id that is generated somehow, that ones are ok. Then we have the ones that pass always 0. This ones are only valid when there is a maximum of one device instantiated for a given machine. - audio: you can choose more than one audio output. - eeprom93xx: you can have more than one e100 card. - vmware_vga: I am not completely sure here, it appears that you could have more than one. Notice that VMSTATE_INSTANCE_ID_ANY will give us the value 0 if there is only one instance, so we are in no trouble. We can drop it if people think that we can't have more than one vmware_vga. - for the rest of the devices, I can't see any that can be instantiated more than once (testing it is easy, just starting the machine will make it fail). Notice that again, for the same reasoning, we could change all the calls to _any(). And only left the vmstate_register(... 0 ...) calls for devices that we know that we only ever want one. What needs to be done: - icp/server: We need to rename the old icp server name. Notice that I doubt that anyone is migrating this, but I need help from PPC experts. As said in the commit message, it is "abusing" the interface: - it register a new device - it realizes that it is instantiting an old beard - it unregister the new device - it registers the old device - rest of devices: * pxa2xx devices: I can't see how you can create more than one device in a machine * acpi_build: I can't see how to create more than once. * replay: neither * cpu timers: created in vl.c * global_state: only once * s390 css: not a way that I can think * spapr: looks only one * or1ktimer: I can only see one * tsc*: I see only use in pxa2xx and one by board - And now, another abuser: vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table, tcet->liobn is an uint32_t, and instance_id is an int. And it just happens that is value is < VMSTATE_INSTANCE_ID_ANY. Please, review. Juan Quintela (12): migration: Create vmstate_register_any() migration: Use vmstate_register_any() migration: Use vmstate_register_any() for isa-ide migration: Use vmstate_register_any() for ipmi-bt* migration: Use VMSTATE_INSTANCE_ID_ANY for slirp migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices migration: Hack to maintain backwards compatibility for ppc migration: vmstate_register() check that instance_id is valid migration: Improve example and documentation of vmstate_register() migration: Use vmstate_register_any() for audio migration: Use vmstate_register_any() for eeprom93xx migration: Use vmstate_register_any() for vmware_vga Peter Xu (1): migration: Check in savevm_state_handler_insert for dups docs/devel/migration.rst| 12 include/migration/vmstate.h | 34 ++ audio/audio.c | 2 +- backends/dbus-vmstate.c | 3 +-- backends/tpm/tpm_emulator.c | 3 +-- hw/display/vmware_vga.c | 2 +- hw/i2c/core.c | 2 +- hw/ide/isa.c| 2 +- hw/input/adb.c | 2 +- hw/input/ads7846.c | 2 +- hw/input/stellaris_input.c | 3 +-- hw/intc/xics.c | 17 +++-- hw/ipmi/ipmi_bmc_extern.c | 2 +- hw/ipmi/ipmi_bmc_sim.c | 2 +- hw/ipmi/isa_ipmi_bt.c | 2 +- hw/ipmi/isa_ipmi_kcs.c | 2 +- hw/net/eepro100.c | 3 +-- hw/nvram/eeprom93xx.c | 2 +- hw/pci/pci.c| 2 +- hw/ppc/spapr.c | 25 +++-- hw/ppc/spapr_nvdimm.c | 3 +-- hw/s390x/s390-skeys.c | 3 ++- hw/s390x/s390-stattrib.c| 3 ++- hw/timer/arm_timer.c| 2 +- hw/virtio/virtio-mem.c | 4 ++-- migration/savevm.c | 32 net/slirp.c | 5 +++-- 27 files changed, 139 insertions(+), 37 deletions(-) -- 2.41.0
Re: [PATCH v5 5/6] hw/virtio: add vhost-user-snd and virtio-snd-pci devices
On Fri, 20 Oct 2023 12:02, "Michael S. Tsirkin" wrote: On Fri, Oct 20, 2023 at 09:16:03AM +0100, Alex Bennée wrote: Viresh Kumar writes: > On 19-10-23, 10:56, Alex Bennée wrote: >> From: Manos Pitsidianakis >> >> Tested with rust-vmm vhost-user-sound daemon: >> >> RUST_LOG=trace cargo run --bin vhost-user-sound -- --socket /tmp/snd.sock --backend null >> >> Invocation: >> >> qemu-system-x86_64 \ >> -qmp unix:./qmp-sock,server,wait=off \ >> -m 4096 \ >> -numa node,memdev=mem \ >> -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \ >> -D qemu.log \ >> -d guest_errors,trace:\*snd\*,trace:\*sound\*,trace:\*vhost\* \ >> -chardev socket,id=vsnd,path=/tmp/snd.sock \ >> -device vhost-user-snd-pci,chardev=vsnd,id=snd \ >> /path/to/disk >> >> [AJB: imported from https://github.com/epilys/qemu-virtio-snd/commit/54ae1cdd15fef2d88e9e387a175f099a38c636f4.patch] >> >> Signed-off-by: Alex Bennée > > Missing SOB from Manos ? oops, guess I need a respin then ;-) Just ask Manos to send his S.O.B in a reply. Signed-off-by: Manos Pitsidianakis
Re: [PATCH 10/13] migration: Improve example and documentation of vmstate_register()
Stefan Berger wrote: > On 10/19/23 15:08, Juan Quintela wrote: >> Signed-off-by: Juan Quintela >> --- >> docs/devel/migration.rst | 12 >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst >> index c3e1400c0c..a9fde75862 100644 >> --- a/docs/devel/migration.rst >> +++ b/docs/devel/migration.rst >> @@ -165,13 +165,17 @@ An example (from hw/input/pckbd.c) >> } >> }; >> >> -We are declaring the state with name "pckbd". >> -The ``version_id`` is 3, and the fields are 4 uint8_t in a KBDState >> structure. >> -We registered this with: >> +We are declaring the state with name "pckbd". The ``version_id`` is >> +3, and the fields are 4 uint8_t in a KBDState structure. We > > and there are 4 uint8_t fields in the KBDState structure. Done thanks. > >> +registered this with one of those. The first one will generate a > > I am not sure what this means 'We registered this with one of > those'. What is 'one of those'? Changed to: We registered this ``VMSTATEDescription`` with one of the following functions. > Maybe you mean: We register the KBDState with one of the following > functions. > >> +device ``instance_id`` different for each registration. Use the >> +second one if you already have an id different for each instance of >> +the device: > ... have an id that is is different for each ... Done >> >> .. code:: c >> >> -vmstate_register(NULL, 0, &vmstate_kbd, s); >> +vmstate_register_any(NULL, &vmstate_kbd, s); >> +vmstate_register(NULL, instance_id, &vmstate_kbd, s); >> >> For devices that are ``qdev`` based, we can register the device in the >> class >> init function: Thanks
Re: [PATCH v5 5/6] hw/virtio: add vhost-user-snd and virtio-snd-pci devices
On Fri, Oct 20, 2023 at 09:16:03AM +0100, Alex Bennée wrote: > > Viresh Kumar writes: > > > On 19-10-23, 10:56, Alex Bennée wrote: > >> From: Manos Pitsidianakis > >> > >> Tested with rust-vmm vhost-user-sound daemon: > >> > >> RUST_LOG=trace cargo run --bin vhost-user-sound -- --socket > >> /tmp/snd.sock --backend null > >> > >> Invocation: > >> > >> qemu-system-x86_64 \ > >> -qmp unix:./qmp-sock,server,wait=off \ > >> -m 4096 \ > >> -numa node,memdev=mem \ > >> -object > >> memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \ > >> -D qemu.log \ > >> -d guest_errors,trace:\*snd\*,trace:\*sound\*,trace:\*vhost\* \ > >> -chardev socket,id=vsnd,path=/tmp/snd.sock \ > >> -device vhost-user-snd-pci,chardev=vsnd,id=snd \ > >> /path/to/disk > >> > >> [AJB: imported from > >> https://github.com/epilys/qemu-virtio-snd/commit/54ae1cdd15fef2d88e9e387a175f099a38c636f4.patch] > >> > >> Signed-off-by: Alex Bennée > > > > Missing SOB from Manos ? > > oops, guess I need a respin then ;-) Just ask Manos to send his S.O.B in a reply. > > > >> Message-Id: <20231009095937.195728-6-alex.ben...@linaro.org> > > > -- > Alex Bennée > Virtualisation Tech Lead @ Linaro
Re: [PATCH 07/13] RFC migration: icp/server is a mess
Greg Kurz wrote:
> On Fri, 20 Oct 2023 09:30:44 +0200
> Juan Quintela wrote:
>
>> Greg Kurz wrote:
>> > On Thu, 19 Oct 2023 21:08:25 +0200
>> > Juan Quintela wrote:
>> >
>> >> Current code does:
>> >> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>> >> dependinfg on cpu number
>> >> - for newer machines, it register vmstate_icp with "icp/server" name
>> >> and instance 0
>> >> - now it unregisters "icp/server" for the 1st instance.
>> >>
>> >> This is wrong at many levels:
>> >> - we shouldn't have two VMSTATEDescriptions with the same name
>> >> - In case this is the only solution that we can came with, it needs to
>> >> be:
>> >> * register pre_2_10_vmstate_dummy_icp
>> >> * unregister pre_2_10_vmstate_dummy_icp
>> >> * register real vmstate_icp
>> >>
>> >> As the initialization of this machine is already complex enough, I
>> >> need help from PPC maintainers to fix this.
>> >>
>> >> Volunteers?
>> >>
>> >> CC: Cedric Le Goater
>> >> CC: Daniel Henrique Barboza
>> >> CC: David Gibson
>> >> CC: Greg Kurz
>> >>
>> >> Signed-off-by: Juan Quintela
>> >> ---
>> >> hw/ppc/spapr.c | 7 ++-
>> >> 1 file changed, 6 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> >> index cb840676d3..8531d13492 100644
>> >> --- a/hw/ppc/spapr.c
>> >> +++ b/hw/ppc/spapr.c
>> >> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
>> >> *opaque)
>> >> }
>> >>
>> >> static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
>> >> -.name = "icp/server",
>> >> +/*
>> >> + * Hack ahead. We can't have two devices with the same name and
>> >> + * instance id. So I rename this to pass make check.
>> >> + * Real help from people who knows the hardware is needed.
>> >> + */
>> >> +.name = "pre-2.10-icp/server",
>> >> .version_id = 1,
>> >> .minimum_version_id = 1,
>> >> .needed = pre_2_10_vmstate_dummy_icp_needed,
>> >
>> > I guess this fix is acceptable as well and a lot simpler than
>> > reverting the hack actually. Outcome is the same : drop
>> > compat with pseries-2.9 and older.
>> >
>> > Reviewed-by: Greg Kurz
>>
>> I fully agree with you here.
>> The other options given on this thread is deprecate that machines, but I
>> would like to have this series sooner than 2 releases.
>
> Yeah and, especially, the deprecation of all these machine types is
> itself a massive chunk of work as it will call to identify and
> remove other related workarounds as well. Given that pretty much
> everyone working in PPC/PAPR moved away, can the community handle
> such a big change ?
>
>> And what ppc is
>> doing here is (and has always been) a hack and an abuse about how
>> vmstate registrations is supposed to work.
>>
>
> Sorry again... We should have involved migration experts at the time. :-)
I would have told you that this can't be done O:-)
Sent another version with a vmstate hack to accomodate this. You don't
have to deprecate the machines due to migration O:-)
And now that I have ppc gurus attention, could you comment in the other
question:
./hw/ppc/spapr_iommu.c
static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
{
...
vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table,
tcet);
}
./include/hw/ppc/spapr.h
struct SpaprTceTable {
...
uint32_t liobn;
};
./include/migration.h
static inline int vmstate_register(VMStateIf *obj, int instance_id,
const VMStateDescription *vmsd,
void *opaque);
liobn is an uint32_t and insntance_id is an int.
For this series, I started with this:
static inline int vmstate_register(VMStateIf *obj, int instance_id,
const VMStateDescription *vmsd,
void *opaque)
{
if (instance_id < 0) {
error_report("vmstate_register: Invalid device: %s instance_id: %d",
vmsd->name, instance_id);
return -1;
}
return vmstate_register_with_alias_id(obj, instance_id, vmsd,
opaque, -1, 0, NULL);
}
And it failed on this. So I change the test to
if (instance_id == VM_INSTANCE_ID_ANY) {
}
But we are still having troubles with signs here.
Posible actions:
- Look the other side and hope that liobn is never -1.
(if it is -1, it would become 0, so not really a big trouble).
- change vmstate type to uint32_t and make VM_INSTANCE_ID_ANY to UINT32_MAX
(exact same problem if liobn happens to be UINT32_MAX)
I have no clue what are the valid values of liobn. So I am leaning to
just look the other way and do nothing.
Advise?
Thanks, Juan.
Re: [PATCH 3/3] blockdev: mirror: avoid potential deadlock when using iothread
Am 19.10.23 um 15:19 schrieb Fiona Ebner: > The bdrv_getlength() function is a generated co-wrapper and uses > AIO_WAIT_WHILE() to wait for the spawned coroutine. AIO_WAIT_WHILE() > expects the lock to be acquired exactly once. > > This can happen when the source node is explicitly specified as the > @replaces parameter or if there is a filter on top of the source node. Correction: this should read "or if the source node is a filter node".
Re: [PATCH v2 10/22] qapi: Correct error message for 'vcpu_dirty_limit' parameter
Philippe Mathieu-Daudé writes:
> QERR_INVALID_PARAMETER_VALUE is defined as:
>
> #define QERR_INVALID_PARAMETER_VALUE \
> "Parameter '%s' expects %s"
>
> The current error is formatted as:
>
> "Parameter 'vcpu_dirty_limit' expects is invalid, it must greater then 1
> MB/s"
>
> Replace by:
>
> "Parameter 'vcpu_dirty_limit' is invalid, it must greater then 1 MB/s"
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> migration/options.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/migration/options.c b/migration/options.c
> index 1d1e1321b0..79fce0c3a9 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1163,9 +1163,8 @@ bool migrate_params_check(MigrationParameters *params,
> Error **errp)
>
> if (params->has_vcpu_dirty_limit &&
> (params->vcpu_dirty_limit < 1)) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> - "vcpu_dirty_limit",
> - "is invalid, it must greater then 1 MB/s");
> +error_setg(errp, "Parameter 'vcpu_dirty_limit' is invalid,"
> + " it must greater then 1 MB/s");
> return false;
> }
Make that "greater than", please.
Arrgh, the unit is MB/s even in QMP:
# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration.
# Defaults to 1. (Since 8.1)
Should be Bytes. Escaped review, and now it's too late to fix.
Re: [PATCH 07/13] RFC migration: icp/server is a mess
On Fri, 20 Oct 2023 17:49:38 +1000
"Nicholas Piggin" wrote:
> On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
> > On Thu, 19 Oct 2023 21:08:25 +0200
> > Juan Quintela wrote:
> >
> > > Current code does:
> > > - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> > > dependinfg on cpu number
> > > - for newer machines, it register vmstate_icp with "icp/server" name
> > > and instance 0
> > > - now it unregisters "icp/server" for the 1st instance.
> > >
> > > This is wrong at many levels:
> > > - we shouldn't have two VMSTATEDescriptions with the same name
> > > - In case this is the only solution that we can came with, it needs to
> > > be:
> > > * register pre_2_10_vmstate_dummy_icp
> > > * unregister pre_2_10_vmstate_dummy_icp
> > > * register real vmstate_icp
> > >
> > > As the initialization of this machine is already complex enough, I
> > > need help from PPC maintainers to fix this.
> > >
> > > Volunteers?
> > >
> > > CC: Cedric Le Goater
> > > CC: Daniel Henrique Barboza
> > > CC: David Gibson
> > > CC: Greg Kurz
> > >
> > > Signed-off-by: Juan Quintela
> > > ---
> > > hw/ppc/spapr.c | 7 ++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index cb840676d3..8531d13492 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
> > > *opaque)
> > > }
> > >
> > > static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > -.name = "icp/server",
> > > +/*
> > > + * Hack ahead. We can't have two devices with the same name and
> > > + * instance id. So I rename this to pass make check.
> > > + * Real help from people who knows the hardware is needed.
> > > + */
> > > +.name = "pre-2.10-icp/server",
> > > .version_id = 1,
> > > .minimum_version_id = 1,
> > > .needed = pre_2_10_vmstate_dummy_icp_needed,
> >
> > I guess this fix is acceptable as well and a lot simpler than
> > reverting the hack actually. Outcome is the same : drop
> > compat with pseries-2.9 and older.
> >
> > Reviewed-by: Greg Kurz
>
> So the reason we can't have duplicate names registered, aside from it
> surely going bad if we actually send or receive a stream at the point
> they are registered, is the duplcate check introduced in patch 9? But
> before that, this hack does seem to actually work because the duplicate
> is unregistered right away.
>
Correct.
> If I understand the workaround, there is an asymmetry in the migration
> sequence in that receiving an unexpected object would cause a failure,
> but going from newer to older would just skip some "expected" objects
> and that didn't cause a problem. So you only have to deal with ignoring
> the unexpected ones going form older to newer.
>
Correct.
> Side question, is it possible to flag the problem of *not* receiving
> an object that you did expect? That might be a source of bugs too.
>
AFAICR we try to only migrate state that differs from reset : the
destination cannot really assume it will receive anything for a
given device.
> Anyway, I wonder if we could fix this spapr problem by adding a special
> case wild card instance matcher to ignore it? It's still a bit hacky
> but maybe a bit nicer. I don't mind deprecating the machine soon if
> you want to clear the wildcard hack away soon, but it would be nice to
> separate the deprecation and removal from the fix, if possible.
>
> This patch is not tested but hopefully helps illustrate the idea.
>
I'm not sure this will fly with older QEMUs that don't know about
VMSTATE_INSTANCE_ID_WILD... but I'll let Juan comment on that.
> Thanks,
> Nick
>
Cheers,
--
Greg
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1a31fb7293..8ce03edefa 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1205,6 +1205,7 @@ int vmstate_save_state_v(QEMUFile *f, const
> VMStateDescription *vmsd,
> bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
>
> #define VMSTATE_INSTANCE_ID_ANY -1
> +#define VMSTATE_INSTANCE_ID_WILD -2
>
> /* Returns: 0 on success, -1 on failure */
> int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..2418899dd4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -155,16 +155,10 @@ static const VMStateDescription
> pre_2_10_vmstate_dummy_icp = {
> },
> };
>
> -static void pre_2_10_vmstate_register_dummy_icp(int i)
> +static void pre_2_10_vmstate_register_dummy_icp(void)
> {
> -vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
> - (void *)(uintptr_t) i);
> -}
> -
> -static void pre_2_10_vmstate_unregister_dummy_icp(int i)
> -{
> -vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
> - (void *)(uintptr_t) i);
> +
Re: [PATCH 07/13] RFC migration: icp/server is a mess
"Nicholas Piggin" wrote: > On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote: >> On Thu, 19 Oct 2023 21:08:25 +0200 >> Juan Quintela wrote: > So the reason we can't have duplicate names registered, aside from it > surely going bad if we actually send or receive a stream at the point > they are registered, is the duplcate check introduced in patch 9? But > before that, this hack does seem to actually work because the duplicate > is unregistered right away. You are creating a new general case that has only a single use that you agree it is "hacky" O:-) The problem here is that you haven't made your mind what "ipc/server" means. You want sometimes to mean pre_2_10, sometimes to mean other thing. That is not how this is supposed to work. See my proposed change, it is one line change, and just do the right thing. I know, it breaks backwards compatibility. But for one machine type that people are proposing to deprecate/remove. > If I understand the workaround, there is an asymmetry in the migration > sequence in that receiving an unexpected object would cause a failure, > but going from newer to older would just skip some "expected" objects > and that didn't cause a problem. So you only have to deal with ignoring > the unexpected ones going form older to newer. Ok, found a different workaround. Sending a new version of the series with a different hack that maintains backwards compatibility. Later, Juan.
Re: [PATCH v5 5/6] hw/virtio: add vhost-user-snd and virtio-snd-pci devices
Viresh Kumar writes: > On 19-10-23, 10:56, Alex Bennée wrote: >> From: Manos Pitsidianakis >> >> Tested with rust-vmm vhost-user-sound daemon: >> >> RUST_LOG=trace cargo run --bin vhost-user-sound -- --socket >> /tmp/snd.sock --backend null >> >> Invocation: >> >> qemu-system-x86_64 \ >> -qmp unix:./qmp-sock,server,wait=off \ >> -m 4096 \ >> -numa node,memdev=mem \ >> -object >> memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \ >> -D qemu.log \ >> -d guest_errors,trace:\*snd\*,trace:\*sound\*,trace:\*vhost\* \ >> -chardev socket,id=vsnd,path=/tmp/snd.sock \ >> -device vhost-user-snd-pci,chardev=vsnd,id=snd \ >> /path/to/disk >> >> [AJB: imported from >> https://github.com/epilys/qemu-virtio-snd/commit/54ae1cdd15fef2d88e9e387a175f099a38c636f4.patch] >> >> Signed-off-by: Alex Bennée > > Missing SOB from Manos ? oops, guess I need a respin then ;-) > >> Message-Id: <20231009095937.195728-6-alex.ben...@linaro.org> -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH 07/13] RFC migration: icp/server is a mess
On 20/10/2023 10.06, Greg Kurz wrote:
On Fri, 20 Oct 2023 09:30:44 +0200
Juan Quintela wrote:
Greg Kurz wrote:
On Thu, 19 Oct 2023 21:08:25 +0200
Juan Quintela wrote:
Current code does:
- register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
dependinfg on cpu number
- for newer machines, it register vmstate_icp with "icp/server" name
and instance 0
- now it unregisters "icp/server" for the 1st instance.
This is wrong at many levels:
- we shouldn't have two VMSTATEDescriptions with the same name
- In case this is the only solution that we can came with, it needs to
be:
* register pre_2_10_vmstate_dummy_icp
* unregister pre_2_10_vmstate_dummy_icp
* register real vmstate_icp
As the initialization of this machine is already complex enough, I
need help from PPC maintainers to fix this.
Volunteers?
CC: Cedric Le Goater
CC: Daniel Henrique Barboza
CC: David Gibson
CC: Greg Kurz
Signed-off-by: Juan Quintela
---
hw/ppc/spapr.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cb840676d3..8531d13492 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
}
static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
-.name = "icp/server",
+/*
+ * Hack ahead. We can't have two devices with the same name and
+ * instance id. So I rename this to pass make check.
+ * Real help from people who knows the hardware is needed.
+ */
+.name = "pre-2.10-icp/server",
.version_id = 1,
.minimum_version_id = 1,
.needed = pre_2_10_vmstate_dummy_icp_needed,
I guess this fix is acceptable as well and a lot simpler than
reverting the hack actually. Outcome is the same : drop
compat with pseries-2.9 and older.
Reviewed-by: Greg Kurz
I fully agree with you here.
The other options given on this thread is deprecate that machines, but I
would like to have this series sooner than 2 releases.
Yeah and, especially, the deprecation of all these machine types is
itself a massive chunk of work as it will call to identify and
remove other related workarounds as well. Given that pretty much
everyone working in PPC/PAPR moved away, can the community handle
such a big change ?
I think you could treat that as two work items. First the deprecation and
removal of old machine types. Second the (optional) cleanups. If we don't
immediately manage to find and remove each and every possible spot that
could be cleaned up after the removal of the machine types, so be it. But
better at least *start* to remove the old cruft, beginning with the machine
type, than dragging this stuff along forever.
Thomas
Re: [PATCH 07/13] RFC migration: icp/server is a mess
On Fri, 20 Oct 2023 09:30:44 +0200
Juan Quintela wrote:
> Greg Kurz wrote:
> > On Thu, 19 Oct 2023 21:08:25 +0200
> > Juan Quintela wrote:
> >
> >> Current code does:
> >> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> >> dependinfg on cpu number
> >> - for newer machines, it register vmstate_icp with "icp/server" name
> >> and instance 0
> >> - now it unregisters "icp/server" for the 1st instance.
> >>
> >> This is wrong at many levels:
> >> - we shouldn't have two VMSTATEDescriptions with the same name
> >> - In case this is the only solution that we can came with, it needs to
> >> be:
> >> * register pre_2_10_vmstate_dummy_icp
> >> * unregister pre_2_10_vmstate_dummy_icp
> >> * register real vmstate_icp
> >>
> >> As the initialization of this machine is already complex enough, I
> >> need help from PPC maintainers to fix this.
> >>
> >> Volunteers?
> >>
> >> CC: Cedric Le Goater
> >> CC: Daniel Henrique Barboza
> >> CC: David Gibson
> >> CC: Greg Kurz
> >>
> >> Signed-off-by: Juan Quintela
> >> ---
> >> hw/ppc/spapr.c | 7 ++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index cb840676d3..8531d13492 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
> >> *opaque)
> >> }
> >>
> >> static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> >> -.name = "icp/server",
> >> +/*
> >> + * Hack ahead. We can't have two devices with the same name and
> >> + * instance id. So I rename this to pass make check.
> >> + * Real help from people who knows the hardware is needed.
> >> + */
> >> +.name = "pre-2.10-icp/server",
> >> .version_id = 1,
> >> .minimum_version_id = 1,
> >> .needed = pre_2_10_vmstate_dummy_icp_needed,
> >
> > I guess this fix is acceptable as well and a lot simpler than
> > reverting the hack actually. Outcome is the same : drop
> > compat with pseries-2.9 and older.
> >
> > Reviewed-by: Greg Kurz
>
> I fully agree with you here.
> The other options given on this thread is deprecate that machines, but I
> would like to have this series sooner than 2 releases.
Yeah and, especially, the deprecation of all these machine types is
itself a massive chunk of work as it will call to identify and
remove other related workarounds as well. Given that pretty much
everyone working in PPC/PAPR moved away, can the community handle
such a big change ?
> And what ppc is
> doing here is (and has always been) a hack and an abuse about how
> vmstate registrations is supposed to work.
>
Sorry again... We should have involved migration experts at the time. :-)
> Thanks, Juan.
>
Cheers,
--
Greg
Re: [PATCH 07/13] RFC migration: icp/server is a mess
On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
> On Thu, 19 Oct 2023 21:08:25 +0200
> Juan Quintela wrote:
>
> > Current code does:
> > - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> > dependinfg on cpu number
> > - for newer machines, it register vmstate_icp with "icp/server" name
> > and instance 0
> > - now it unregisters "icp/server" for the 1st instance.
> >
> > This is wrong at many levels:
> > - we shouldn't have two VMSTATEDescriptions with the same name
> > - In case this is the only solution that we can came with, it needs to
> > be:
> > * register pre_2_10_vmstate_dummy_icp
> > * unregister pre_2_10_vmstate_dummy_icp
> > * register real vmstate_icp
> >
> > As the initialization of this machine is already complex enough, I
> > need help from PPC maintainers to fix this.
> >
> > Volunteers?
> >
> > CC: Cedric Le Goater
> > CC: Daniel Henrique Barboza
> > CC: David Gibson
> > CC: Greg Kurz
> >
> > Signed-off-by: Juan Quintela
> > ---
> > hw/ppc/spapr.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index cb840676d3..8531d13492 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
> > *opaque)
> > }
> >
> > static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > -.name = "icp/server",
> > +/*
> > + * Hack ahead. We can't have two devices with the same name and
> > + * instance id. So I rename this to pass make check.
> > + * Real help from people who knows the hardware is needed.
> > + */
> > +.name = "pre-2.10-icp/server",
> > .version_id = 1,
> > .minimum_version_id = 1,
> > .needed = pre_2_10_vmstate_dummy_icp_needed,
>
> I guess this fix is acceptable as well and a lot simpler than
> reverting the hack actually. Outcome is the same : drop
> compat with pseries-2.9 and older.
>
> Reviewed-by: Greg Kurz
So the reason we can't have duplicate names registered, aside from it
surely going bad if we actually send or receive a stream at the point
they are registered, is the duplcate check introduced in patch 9? But
before that, this hack does seem to actually work because the duplicate
is unregistered right away.
If I understand the workaround, there is an asymmetry in the migration
sequence in that receiving an unexpected object would cause a failure,
but going from newer to older would just skip some "expected" objects
and that didn't cause a problem. So you only have to deal with ignoring
the unexpected ones going form older to newer.
Side question, is it possible to flag the problem of *not* receiving
an object that you did expect? That might be a source of bugs too.
Anyway, I wonder if we could fix this spapr problem by adding a special
case wild card instance matcher to ignore it? It's still a bit hacky
but maybe a bit nicer. I don't mind deprecating the machine soon if
you want to clear the wildcard hack away soon, but it would be nice to
separate the deprecation and removal from the fix, if possible.
This patch is not tested but hopefully helps illustrate the idea.
Thanks,
Nick
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a31fb7293..8ce03edefa 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1205,6 +1205,7 @@ int vmstate_save_state_v(QEMUFile *f, const
VMStateDescription *vmsd,
bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
#define VMSTATE_INSTANCE_ID_ANY -1
+#define VMSTATE_INSTANCE_ID_WILD -2
/* Returns: 0 on success, -1 on failure */
int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cb840676d3..2418899dd4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -155,16 +155,10 @@ static const VMStateDescription
pre_2_10_vmstate_dummy_icp = {
},
};
-static void pre_2_10_vmstate_register_dummy_icp(int i)
+static void pre_2_10_vmstate_register_dummy_icp(void)
{
-vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
- (void *)(uintptr_t) i);
-}
-
-static void pre_2_10_vmstate_unregister_dummy_icp(int i)
-{
-vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
- (void *)(uintptr_t) i);
+vmstate_register(NULL, VMSTATE_INSTANCE_ID_WILD,
+ &pre_2_10_vmstate_dummy_icp, NULL);
}
int spapr_max_server_number(SpaprMachineState *spapr)
@@ -2665,12 +2659,10 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
}
if (smc->pre_2_10_has_unused_icps) {
-for (i = 0; i < spapr_max_server_number(spapr); i++) {
-/* Dummy entries get deregistered when real ICPState objects
- * are registered during CPU core hotplug.
- */
-pre_2_10_vmstate_register_dummy_icp(i);
-}
+/* Dummy entries get deregistered when
Re: [PATCH 07/13] RFC migration: icp/server is a mess
On 10/20/23 07:10, Thomas Huth wrote:
On 19/10/2023 23.15, Cédric Le Goater wrote:
On 10/19/23 22:49, Greg Kurz wrote:
Hi Juan,
On Thu, 19 Oct 2023 21:08:25 +0200
Juan Quintela wrote:
Current code does:
- register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
dependinfg on cpu number
- for newer machines, it register vmstate_icp with "icp/server" name
and instance 0
- now it unregisters "icp/server" for the 1st instance.
Heh I remember about this hack... it was caused by some rework in
the interrupt controller that broke migration.
This is wrong at many levels:
- we shouldn't have two VMSTATEDescriptions with the same name
I don't know how bad it is. The idea here is to send extra
state in the stream because older QEMU expect it (but won't use
it), so it made sense to keep the same name.
- In case this is the only solution that we can came with, it needs to
be:
* register pre_2_10_vmstate_dummy_icp
* unregister pre_2_10_vmstate_dummy_icp
* register real vmstate_icp
As the initialization of this machine is already complex enough, I
need help from PPC maintainers to fix this.
What about dropping all this code, i.e. basically reverting 46f7afa37096
("spapr:
fix migration of ICPState objects from/to older QEMU") ?
I'd vote for removing the dummy ICP states for pre-2.10 pseries machines.
Migration compatibility would be broken for these old versions but, with
a clear error report, it should be more than enough. I doubt anyone will
need such a feature now days.
In that case: Please also put the pseries-2.1 machine up to pseries-2.9 onto
the deprecation list, so that they can finally get removed after two releases.
It does not make sense to keep compat machines around if the compatibility is
not available anymore.
This would be a really good cleanup for PPC to deprecate pseries-2.1/2.9.
We did a few workarounds in that time frame which wouldn't be necessary
anymore.
Thanks,
C.
Re: [PATCH 13/13] migration: Use vmstate_register_any() for vmware_vga
Stefan Berger wrote: > On 10/19/23 15:08, Juan Quintela wrote: >> I have no idea if we can have more than one vmware_vga device, so play >> it safe. >> >> Signed-off-by: Juan Quintela > > Reviewed-by: Stefan Berger > >> --- >> hw/display/vmware_vga.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c >> index 09591fbd39..7490d43881 100644 >> --- a/hw/display/vmware_vga.c >> +++ b/hw/display/vmware_vga.c >> @@ -1264,7 +1264,7 @@ static void vmsvga_init(DeviceState *dev, struct >> vmsvga_state_s *s, >> >> vga_common_init(&s->vga, OBJECT(dev), &error_fatal); >> vga_init(&s->vga, OBJECT(dev), address_space, io, true); >> -vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga); >> +vmstate_register_any(NULL, &vmstate_vga_common, &s->vga); > > And the first one registered with 'any' will again have instance_id = > 0 assigned. So there's no side effect to be expected with any of these > device, I suppose. I will really change all the remaining registrations with 0 to any. * If there is only a registration for that device: nothing changes * If there is more than one registration for that device: It *could* work (from the migration point of view). But then there are devices that *clearly* will not be able to have more than one instance, so 0 is the best option there. On top of my head: * cpu-timers * replay * migration global_state And the rest that I put on the cover letter are basically devices that are used only once on the board initilization routine, so I feel safe leaving them with zero. Thanks for the review, Juan.
Re: [PATCH 07/13] RFC migration: icp/server is a mess
Greg Kurz wrote:
> On Thu, 19 Oct 2023 21:08:25 +0200
> Juan Quintela wrote:
>
>> Current code does:
>> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>> dependinfg on cpu number
>> - for newer machines, it register vmstate_icp with "icp/server" name
>> and instance 0
>> - now it unregisters "icp/server" for the 1st instance.
>>
>> This is wrong at many levels:
>> - we shouldn't have two VMSTATEDescriptions with the same name
>> - In case this is the only solution that we can came with, it needs to
>> be:
>> * register pre_2_10_vmstate_dummy_icp
>> * unregister pre_2_10_vmstate_dummy_icp
>> * register real vmstate_icp
>>
>> As the initialization of this machine is already complex enough, I
>> need help from PPC maintainers to fix this.
>>
>> Volunteers?
>>
>> CC: Cedric Le Goater
>> CC: Daniel Henrique Barboza
>> CC: David Gibson
>> CC: Greg Kurz
>>
>> Signed-off-by: Juan Quintela
>> ---
>> hw/ppc/spapr.c | 7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index cb840676d3..8531d13492 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
>> *opaque)
>> }
>>
>> static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
>> -.name = "icp/server",
>> +/*
>> + * Hack ahead. We can't have two devices with the same name and
>> + * instance id. So I rename this to pass make check.
>> + * Real help from people who knows the hardware is needed.
>> + */
>> +.name = "pre-2.10-icp/server",
>> .version_id = 1,
>> .minimum_version_id = 1,
>> .needed = pre_2_10_vmstate_dummy_icp_needed,
>
> I guess this fix is acceptable as well and a lot simpler than
> reverting the hack actually. Outcome is the same : drop
> compat with pseries-2.9 and older.
>
> Reviewed-by: Greg Kurz
I fully agree with you here.
The other options given on this thread is deprecate that machines, but I
would like to have this series sooner than 2 releases. And what ppc is
doing here is (and has always been) a hack and an abuse about how
vmstate registrations is supposed to work.
Thanks, Juan.
Re: [PATCH v2 07/22] qapi: Inline QERR_INVALID_PARAMETER_TYPE definition (constant param)
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using the following
> coccinelle semantic patch:
>
> @match@
> expression errp;
> constant param;
> constant value;
> @@
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, param, value);
>
> @script:python strformat depends on match@
> param << match.param;
> value << match.value;
> fixedfmt; // new var
> @@
> fixedfmt = f'"Invalid parameter type for \'{param[1:-1]}\', expected:
> {value[1:-1]}"'
> coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
> @replace@
> expression match.errp;
> constant match.param;
> constant match.value;
> identifier strformat.fixedfmt;
> @@
> -error_setg(errp, QERR_INVALID_PARAMETER_TYPE, param, value);
> +error_setg(errp, fixedfmt);
>
> Signed-off-by: Philippe Mathieu-Daudé
> Acked-by: Thomas Huth
> ---
> target/arm/arm-qmp-cmds.c| 3 ++-
> target/s390x/cpu_models_sysemu.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
> index b53d5efe13..3c99fd8222 100644
> --- a/target/arm/arm-qmp-cmds.c
> +++ b/target/arm/arm-qmp-cmds.c
> @@ -154,7 +154,8 @@ CpuModelExpansionInfo
> *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> if (model->props) {
> qdict_in = qobject_to(QDict, model->props);
> if (!qdict_in) {
> -error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
> +error_setg(errp,
> + "Invalid parameter type for 'props', expected: dict");
> return NULL;
> }
> }
> diff --git a/target/s390x/cpu_models_sysemu.c
> b/target/s390x/cpu_models_sysemu.c
> index 63981bf36b..4507714493 100644
> --- a/target/s390x/cpu_models_sysemu.c
> +++ b/target/s390x/cpu_models_sysemu.c
> @@ -111,7 +111,8 @@ static void cpu_model_from_info(S390CPUModel *model,
> const CpuModelInfo *info,
> if (info->props) {
> qdict = qobject_to(QDict, info->props);
> if (!qdict) {
> -error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
> +error_setg(errp,
> + "Invalid parameter type for 'props', expected: dict");
> return;
> }
> }
The error messages are awful. Your patch makes their awfulness more
visible. Improvement of sorts ;)
Re: [PATCH v2 06/22] qapi: Inline and remove QERR_INVALID_PARAMETER definition
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using:
>
> $ sed -i -e "s/QERR_INVALID_PARAMETER,/\"Invalid parameter '%s'\",/" \
> $(git grep -lw QERR_INVALID_PARAMETER)
>
> then manually removing the definition in include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/qapi/qmp/qerror.h | 3 ---
> monitor/hmp-cmds.c| 2 +-
> qapi/opts-visitor.c | 2 +-
> util/qemu-option.c| 8
> 4 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index e93211085a..63ab775dc5 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,9 +17,6 @@
> * add new ones!
> */
>
> -#define QERR_INVALID_PARAMETER \
> -"Invalid parameter '%s'"
> -
> #define QERR_INVALID_PARAMETER_TYPE \
> "Invalid parameter type for '%s', expected: %s"
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 6c559b48c8..9d6533643d 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -138,7 +138,7 @@ void hmp_sync_profile(Monitor *mon, const QDict *qdict)
const char *op = qdict_get_try_str(qdict, "op");
if (op == NULL) {
bool on = qsp_is_enabled();
monitor_printf(mon, "sync-profile is %s\n", on ? "on" : "off");
return;
}
if (!strcmp(op, "on")) {
qsp_enable();
} else if (!strcmp(op, "off")) {
qsp_disable();
} else if (!strcmp(op, "reset")) {
qsp_reset();
> } else {
> Error *err = NULL;
>
> -error_setg(&err, QERR_INVALID_PARAMETER, op);
The use of QERR_INVALID_PARAMETER is wrong: (1) it takes a parameter
name, but we pass a parameter value, and (2) parameter @op is valid, its
value isn't.
> +error_setg(&err, "Invalid parameter '%s'", op);
> hmp_handle_error(mon, err);
> }
> }
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 8f1efab8b9..3d1a28b419 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -184,7 +184,7 @@ opts_check_struct(Visitor *v, Error **errp)
> const QemuOpt *first;
>
> first = g_queue_peek_head(any);
> -error_setg(errp, QERR_INVALID_PARAMETER, first->name);
> +error_setg(errp, "Invalid parameter '%s'", first->name);
> return false;
> }
> return true;
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index fb391a7904..201f7a87f3 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -498,7 +498,7 @@ static bool opt_validate(QemuOpt *opt, Error **errp)
>
> desc = find_desc_by_name(list->desc, opt->name);
> if (!desc && !opts_accepts_any(list)) {
> -error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
> +error_setg(errp, "Invalid parameter '%s'", opt->name);
> return false;
> }
>
> @@ -531,7 +531,7 @@ bool qemu_opt_set_bool(QemuOpts *opts, const char *name,
> bool val,
>
> desc = find_desc_by_name(list->desc, name);
> if (!desc && !opts_accepts_any(list)) {
> -error_setg(errp, QERR_INVALID_PARAMETER, name);
> +error_setg(errp, "Invalid parameter '%s'", name);
> return false;
> }
>
> @@ -554,7 +554,7 @@ bool qemu_opt_set_number(QemuOpts *opts, const char
> *name, int64_t val,
>
> desc = find_desc_by_name(list->desc, name);
> if (!desc && !opts_accepts_any(list)) {
> -error_setg(errp, QERR_INVALID_PARAMETER, name);
> +error_setg(errp, "Invalid parameter '%s'", name);
> return false;
> }
>
> @@ -1103,7 +1103,7 @@ bool qemu_opts_validate(QemuOpts *opts, const
> QemuOptDesc *desc, Error **errp)
> QTAILQ_FOREACH(opt, &opts->head, next) {
> opt->desc = find_desc_by_name(desc, opt->name);
> if (!opt->desc) {
> -error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
> +error_setg(errp, "Invalid parameter '%s'", opt->name);
> return false;
> }
Re: [PATCH v2 05/22] qapi: Inline QERR_INVALID_PARAMETER definition (constant parameter)
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using the following
> coccinelle semantic patch:
>
> @match@
> expression errp;
> constant param;
> @@
> error_setg(errp, QERR_INVALID_PARAMETER, param);
>
> @script:python strformat depends on match@
> param << match.param;
> fixedfmt; // new var
> @@
> fixedfmt = f'"Invalid parameter \'{param[1:-1]}\'"' # Format skipping '"'.
> coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
> @replace@
> expression match.errp;
> constant match.param;
> identifier strformat.fixedfmt;
> @@
> -error_setg(errp, QERR_INVALID_PARAMETER, param);
> +error_setg(errp, fixedfmt);
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> dump/dump.c| 6 +++---
> qga/commands.c | 2 +-
> ui/ui-qmp-cmds.c | 2 +-
> util/qemu-option.c | 2 +-
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index d4ef713cd0..e173f1f14c 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1810,7 +1810,7 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>
> s->fd = fd;
> if (has_filter && !length) {
> -error_setg(errp, QERR_INVALID_PARAMETER, "length");
Incorrect use of QERR_INVALID_PARAMETER: the parameter is perfectly
valid, the problem is its invalid value. Better:
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "a non-zero size");
> +error_setg(errp, "Invalid parameter 'length'");
Applying PATCH 12's transformation then results in
error_setg(errp, "Parameter '%s' expects a non-zero size",
"length");
which you may want to contract to
error_setg(errp, "Parameter 'length' expects a non-zero size");
But not in this patch. Either before or after. I'd pick before.
> goto cleanup;
> }
> s->filter_area_begin = begin;
> @@ -1841,7 +1841,7 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>
> /* Is the filter filtering everything? */
> if (validate_start_block(s) == -1) {
> -error_setg(errp, QERR_INVALID_PARAMETER, "begin");
> +error_setg(errp, "Invalid parameter 'begin'");
> goto cleanup;
> }
>
> @@ -2145,7 +2145,7 @@ void qmp_dump_guest_memory(bool paging, const char
> *file,
#if !defined(WIN32)
if (strstart(file, "fd:", &p)) {
fd = monitor_get_fd(monitor_cur(), p, errp);
if (fd == -1) {
return;
}
}
#endif
if (strstart(file, "file:", &p)) {
fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
S_IRUSR);
if (fd < 0) {
error_setg_file_open(errp, errno, p);
return;
}
> }
>
> if (fd == -1) {
> -error_setg(errp, QERR_INVALID_PARAMETER, "protocol");
This made me go "there is no parameter protocol", but then I
double-checked the schema, and realized there is. It's just named @file
here. We should rename it to match the schema.
Again, the use of QERR_INVALID_PARAMETER is wrong: @protocol is valid,
its value isn't.
More: we should use qemu_create() instead of qemu_open_old(), to not
throw away qemu_open_internal()'s error.
> +error_setg(errp, "Invalid parameter 'protocol'");
> return;
> }
>
> diff --git a/qga/commands.c b/qga/commands.c
> index 09c683e263..871210ab0b 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -154,7 +154,7 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error
> **errp)
>
> gei = guest_exec_info_find(pid);
> if (gei == NULL) {
> -error_setg(errp, QERR_INVALID_PARAMETER, "pid");
Again, the use of QERR_INVALID_PARAMETER is wrong: @pid is valid, its
value isn't.
> +error_setg(errp, "Invalid parameter 'pid'");
> return NULL;
> }
>
> diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
> index debc07d678..41ca0100e7 100644
> --- a/ui/ui-qmp-cmds.c
> +++ b/ui/ui-qmp-cmds.c
> @@ -44,7 +44,7 @@ void qmp_set_password(SetPasswordOptions *opts, Error
> **errp)
> assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
> if (opts->connected != SET_PASSWORD_ACTION_KEEP) {
> /* vnc supports "connected=keep" only */
> -error_setg(errp, QERR_INVALID_PARAMETER, "connected");
Same misuse of QERR_INVALID_PARAMETER.
> +error_setg(errp, "Invalid parameter 'connected'");
> return;
> }
> /*
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index eedd08929b..fb391a7904 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -612,7 +612,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char
> *id,
>
> if
[PULL 14/17] tests/qtest/migration: Set q35 as the default machine for x86_86
From: Fabiano Rosas
Change the x86_64 to use the q35 machines in tests from now on. Keep
testing the pc macine on 32bit.
Signed-off-by: Fabiano Rosas
Reviewed-by: Thomas Huth
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <20231018192741.25885-10-faro...@suse.de>
---
tests/qtest/migration-test.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 9959a0dc12..03f3feac7b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -756,7 +756,12 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
got_dst_resume = false;
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
memory_size = "150M";
-machine_alias = "pc";
+
+if (g_str_equal(arch, "i386")) {
+machine_alias = "pc";
+} else {
+machine_alias = "q35";
+}
arch_opts = g_strdup_printf(
"-drive if=none,id=d0,file=%s,format=raw "
"-device ide-hd,drive=d0,secs=1,cyls=1,heads=1", bootpath);
--
2.41.0
[PULL 04/17] migration: simplify notifiers
From: Steve Sistare
Pass the callback function to add_migration_state_change_notifier so
that migration can initialize the notifier on add and clear it on
delete, which simplifies the call sites. Shorten the function names
so the extra arg can be added more legibly. Hide the global notifier
list in a new function migration_call_notifiers, and make it externally
visible so future live update code can call it.
No functional change.
Signed-off-by: Steve Sistare
Reviewed-by: Peter Xu
Tested-by: Michael Galaxy
Reviewed-by: Michael Galaxy
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <1686148954-250144-1-git-send-email-steven.sist...@oracle.com>
---
include/migration/misc.h | 6 --
hw/net/virtio-net.c | 6 +++---
hw/vfio/migration.c | 6 +++---
migration/migration.c| 22 --
net/vhost-vdpa.c | 7 ---
ui/spice-core.c | 3 +--
6 files changed, 31 insertions(+), 19 deletions(-)
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 7dcc0b5c2c..673ac490fb 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -60,8 +60,10 @@ void migration_object_init(void);
void migration_shutdown(void);
bool migration_is_idle(void);
bool migration_is_active(MigrationState *);
-void add_migration_state_change_notifier(Notifier *notify);
-void remove_migration_state_change_notifier(Notifier *notify);
+void migration_add_notifier(Notifier *notify,
+void (*func)(Notifier *notifier, void *data));
+void migration_remove_notifier(Notifier *notify);
+void migration_call_notifiers(MigrationState *s);
bool migration_in_setup(MigrationState *);
bool migration_has_finished(MigrationState *);
bool migration_has_failed(MigrationState *);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 29e33ea5ed..b85c7946a7 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3624,8 +3624,8 @@ static void virtio_net_device_realize(DeviceState *dev,
Error **errp)
n->primary_listener.hide_device = failover_hide_primary_device;
qatomic_set(&n->failover_primary_hidden, true);
device_listener_register(&n->primary_listener);
-n->migration_state.notify = virtio_net_migration_state_notifier;
-add_migration_state_change_notifier(&n->migration_state);
+migration_add_notifier(&n->migration_state,
+ virtio_net_migration_state_notifier);
n->host_features |= (1ULL << VIRTIO_NET_F_STANDBY);
}
@@ -3788,7 +3788,7 @@ static void virtio_net_device_unrealize(DeviceState *dev)
if (n->failover) {
qobject_unref(n->primary_opts);
device_listener_unregister(&n->primary_listener);
-remove_migration_state_change_notifier(&n->migration_state);
+migration_remove_notifier(&n->migration_state);
} else {
assert(n->primary_opts == NULL);
}
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 0aaad65c06..28d422b39f 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -872,8 +872,8 @@ static int vfio_migration_init(VFIODevice *vbasedev)
NULL;
migration->vm_state = qdev_add_vm_change_state_handler_full(
vbasedev->dev, vfio_vmstate_change, prepare_cb, vbasedev);
-migration->migration_state.notify = vfio_migration_state_notifier;
-add_migration_state_change_notifier(&migration->migration_state);
+migration_add_notifier(&migration->migration_state,
+ vfio_migration_state_notifier);
return 0;
}
@@ -882,7 +882,7 @@ static void vfio_migration_deinit(VFIODevice *vbasedev)
{
VFIOMigration *migration = vbasedev->migration;
-remove_migration_state_change_notifier(&migration->migration_state);
+migration_remove_notifier(&migration->migration_state);
qemu_del_vm_change_state_handler(migration->vm_state);
unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
vfio_migration_free(vbasedev);
diff --git a/migration/migration.c b/migration/migration.c
index 818b02b707..67547eb6a1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1207,7 +1207,7 @@ static void migrate_fd_cleanup(MigrationState *s)
/* It is used on info migrate. We can't free it */
error_report_err(error_copy(s->error));
}
-notifier_list_notify(&migration_state_notifiers, s);
+migration_call_notifiers(s);
block_cleanup_parameters();
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
@@ -1311,14 +1311,24 @@ static void migrate_fd_cancel(MigrationState *s)
}
}
-void add_migration_state_change_notifier(Notifier *notify)
+void migration_add_notifier(Notifier *notify,
+void (*func)(Notifier *notifier, void *data))
{
+notify->notify = func;
notifier_list_add(&migration_state_notifiers, notify);
}
-void remove_migration_state_change_notifier(Notifier *notify)
+void migrati
[PULL 15/17] tests/qtest/migration: Support more than one QEMU binary
From: Fabiano Rosas
We have strict rules around migration compatibility between different
QEMU versions but no test to validate the migration state between
different binaries.
Add infrastructure to allow running the migration tests with two
different QEMU binaries as migration source and destination.
The code now recognizes two new environment variables
QTEST_QEMU_BINARY_SRC and QTEST_QEMU_BINARY_DST. In the absence of
either of them, the test will use the QTEST_QEMU_BINARY variable. If
both are missing then the tests are run with single binary as
previously.
The machine type is selected automatically as the latest machine type
version that works with both binaries.
Usage (only one of SRC|DST is allowed):
QTEST_QEMU_BINARY_SRC=../build-8.2.0/qemu-system-x86_64 \
QTEST_QEMU_BINARY=../build-8.1.0/qemu-system-x86_64 \
./tests/qtest/migration-test
Reviewed-by: Juan Quintela
Signed-off-by: Fabiano Rosas
Reviewed-by: Thomas Huth
Signed-off-by: Juan Quintela
Message-ID: <20231018192741.25885-11-faro...@suse.de>
---
tests/qtest/migration-test.c | 28
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 03f3feac7b..4a5d37317a 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -71,6 +71,8 @@ static bool got_dst_resume;
#define QEMU_VM_FILE_MAGIC 0x5145564d
#define FILE_TEST_FILENAME "migfile"
#define FILE_TEST_OFFSET 0x1000
+#define QEMU_ENV_SRC "QTEST_QEMU_BINARY_SRC"
+#define QEMU_ENV_DST "QTEST_QEMU_BINARY_DST"
#if defined(__linux__)
#include
@@ -744,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
const char *arch = qtest_get_arch();
const char *memory_size;
const char *machine_alias, *machine_opts = "";
+g_autofree char *machine = NULL;
if (args->use_shmem) {
if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
@@ -822,6 +825,10 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
kvm_opts = ",dirty-ring-size=4096";
}
+machine = find_common_machine_version(machine_alias, QEMU_ENV_SRC,
+ QEMU_ENV_DST);
+g_test_message("Using machine type: %s", machine);
+
cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
"-machine %s,%s "
"-name source,debug-threads=on "
@@ -829,7 +836,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
"-serial file:%s/src_serial "
"%s %s %s %s %s",
kvm_opts ? kvm_opts : "",
- machine_alias, machine_opts,
+ machine, machine_opts,
memory_size, tmpfs,
arch_opts ? arch_opts : "",
arch_source ? arch_source : "",
@@ -837,7 +844,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
args->opts_source ? args->opts_source : "",
ignore_stderr);
if (!args->only_target) {
-*from = qtest_init(cmd_source);
+*from = qtest_init_with_env(QEMU_ENV_SRC, cmd_source);
qtest_qmp_set_event_callback(*from,
migrate_watch_for_stop,
&got_src_stop);
@@ -851,14 +858,14 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
"-incoming %s "
"%s %s %s %s %s",
kvm_opts ? kvm_opts : "",
- machine_alias, machine_opts,
+ machine, machine_opts,
memory_size, tmpfs, uri,
arch_opts ? arch_opts : "",
arch_target ? arch_target : "",
shmem_opts ? shmem_opts : "",
args->opts_target ? args->opts_target : "",
ignore_stderr);
-*to = qtest_init(cmd_target);
+*to = qtest_init_with_env(QEMU_ENV_DST, cmd_target);
qtest_qmp_set_event_callback(*to,
migrate_watch_for_resume,
&got_dst_resume);
@@ -2989,10 +2996,23 @@ int main(int argc, char **argv)
bool has_uffd;
const char *arch;
g_autoptr(GError) err = NULL;
+const char *qemu_src = getenv(QEMU_ENV_SRC);
+const char *qemu_dst = getenv(QEMU_ENV_DST);
int ret;
g_test_init(&argc, &argv, NULL);
+/*
+ * The default QTEST_QEMU_BINARY must always be provided because
+ * that is what helpers use to query the accel type and
+ * architecture.
+ */
+
[PULL 03/17] migration: Fix parse_ramblock() on overwritten retvals
From: Peter Xu
It's possible that some errors can be overwritten with success retval later
on, and then ignored. Always capture all errors and report.
Reported by Coverity 1522861, but actually I spot one more in the same
function.
Fixes: CID 1522861
Signed-off-by: Peter Xu
Reviewed-by: Fabiano Rosas
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <20231017203855.298260-1-pet...@redhat.com>
---
migration/ram.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 16c30a9d7a..92769902bb 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3873,6 +3873,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block,
ram_addr_t length)
ret = qemu_ram_resize(block, length, &local_err);
if (local_err) {
error_report_err(local_err);
+return ret;
}
}
/* For postcopy we need to check hugepage sizes match */
@@ -3883,7 +3884,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block,
ram_addr_t length)
error_report("Mismatched RAM page size %s "
"(local) %zd != %" PRId64, block->idstr,
block->page_size, remote_page_size);
-ret = -EINVAL;
+return -EINVAL;
}
}
if (migrate_ignore_shared()) {
@@ -3893,7 +3894,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block,
ram_addr_t length)
error_report("Mismatched GPAs for block %s "
"%" PRId64 "!= %" PRId64, block->idstr,
(uint64_t)addr, (uint64_t)block->mr->addr);
-ret = -EINVAL;
+return -EINVAL;
}
}
ret = rdma_block_notification_handle(f, block->idstr);
--
2.41.0
[PULL 10/17] tests/qtest: Introduce qtest_resolve_machine_alias
From: Fabiano Rosas
The migration tests are being enhanced to test migration between
different QEMU versions. A requirement of migration is that the
machine type between source and destination matches, including the
version.
We cannot hardcode machine types in the tests because those change
with each release. QEMU provides a machine type alias that has a fixed
name, but points to the latest machine type at each release.
Add a helper to resolve the alias into the exact machine
type. E.g. "-machine pc" resolves to "pc-i440fx-8.2"
Reviewed-by: Juan Quintela
Reviewed-by: Thomas Huth
Signed-off-by: Fabiano Rosas
Signed-off-by: Juan Quintela
Message-ID: <20231018192741.25885-6-faro...@suse.de>
---
tests/qtest/libqtest.h | 10 ++
tests/qtest/libqtest.c | 16
2 files changed, 26 insertions(+)
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index d16deb9891..6e3d3525bf 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -922,6 +922,16 @@ void qtest_qmp_fds_assert_success(QTestState *qts, int
*fds, size_t nfds,
void qtest_cb_for_every_machine(void (*cb)(const char *machine),
bool skip_old_versioned);
+/**
+ * qtest_resolve_machine_alias:
+ * @var: Environment variable from where to take the QEMU binary
+ * @alias: The alias to resolve
+ *
+ * Returns: the machine type corresponding to the alias if any,
+ * otherwise NULL.
+ */
+char *qtest_resolve_machine_alias(const char *var, const char *alias);
+
/**
* qtest_has_machine:
* @machine: The machine to look for
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 603d900e7d..c843c41188 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1565,6 +1565,22 @@ void qtest_cb_for_every_machine(void (*cb)(const char
*machine),
}
}
+char *qtest_resolve_machine_alias(const char *var, const char *alias)
+{
+struct MachInfo *machines;
+int i;
+
+machines = qtest_get_machines(var);
+
+for (i = 0; machines[i].name != NULL; i++) {
+if (machines[i].alias && g_str_equal(alias, machines[i].alias)) {
+return g_strdup(machines[i].name);
+}
+}
+
+return NULL;
+}
+
bool qtest_has_machine_with_env(const char *var, const char *machine)
{
struct MachInfo *machines;
--
2.41.0
[PULL 16/17] tests/qtest/migration: Allow user to specify a machine type
From: Fabiano Rosas
Accept the QTEST_QEMU_MACHINE_TYPE environment variable to take a
machine type to use in the tests.
The full machine type is recognized (e.g. pc-q35-8.2). Aliases
(e.g. pc) are also allowed and resolve to the latest machine version
for that alias, or, if using two QEMU binaries, to the latest common
machine version between the two.
Signed-off-by: Fabiano Rosas
Reviewed-by: Thomas Huth
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <20231018192741.25885-12-faro...@suse.de>
---
tests/qtest/migration-helpers.h | 2 ++
tests/qtest/migration-helpers.c | 26 ++
tests/qtest/migration-test.c| 5 +++--
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index d1c2351d33..e31dc85cc7 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -45,4 +45,6 @@ void wait_for_migration_fail(QTestState *from, bool
allow_active);
char *find_common_machine_version(const char *mtype, const char *var1,
const char *var2);
+char *resolve_machine_version(const char *alias, const char *var1,
+ const char *var2);
#endif /* MIGRATION_HELPERS_H */
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 13449c1fe1..24fb7b3525 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -11,6 +11,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/ctype.h"
#include "qapi/qmp/qjson.h"
#include "migration-helpers.h"
@@ -266,3 +267,28 @@ char *find_common_machine_version(const char *mtype, const
char *var1,
"binaries %s and %s", mtype, getenv(var1), getenv(var2));
g_assert_not_reached();
}
+
+char *resolve_machine_version(const char *alias, const char *var1,
+ const char *var2)
+{
+const char *mname = g_getenv("QTEST_QEMU_MACHINE_TYPE");
+g_autofree char *machine_name = NULL;
+
+if (mname) {
+const char *dash = strrchr(mname, '-');
+const char *dot = strrchr(mname, '.');
+
+machine_name = g_strdup(mname);
+
+if (dash && dot) {
+assert(qtest_has_machine(machine_name));
+return g_steal_pointer(&machine_name);
+}
+/* else: probably an alias, let it be resolved below */
+} else {
+/* use the hardcoded alias */
+machine_name = g_strdup(alias);
+}
+
+return find_common_machine_version(machine_name, var1, var2);
+}
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 4a5d37317a..bc70a14642 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -825,8 +825,9 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
kvm_opts = ",dirty-ring-size=4096";
}
-machine = find_common_machine_version(machine_alias, QEMU_ENV_SRC,
- QEMU_ENV_DST);
+machine = resolve_machine_version(machine_alias, QEMU_ENV_SRC,
+ QEMU_ENV_DST);
+
g_test_message("Using machine type: %s", machine);
cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
--
2.41.0
[PULL 12/17] tests/qtest/migration: Define a machine for all architectures
From: Fabiano Rosas
Stop relying on defaults and select a machine explicitly for every
architecture.
This is a prerequisite for being able to select machine types for
migration using different QEMU binaries for source and destination.
Signed-off-by: Fabiano Rosas
Reviewed-by: Thomas Huth
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <20231018192741.25885-8-faro...@suse.de>
---
tests/qtest/migration-test.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 241b409857..dfea75b76f 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -743,6 +743,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
const char *kvm_opts = NULL;
const char *arch = qtest_get_arch();
const char *memory_size;
+const char *machine_alias, *machine_opts = "";
if (args->use_shmem) {
if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
@@ -755,11 +756,13 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
got_dst_resume = false;
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
memory_size = "150M";
+machine_alias = "pc";
arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
start_address = X86_TEST_MEM_START;
end_address = X86_TEST_MEM_END;
} else if (g_str_equal(arch, "s390x")) {
memory_size = "128M";
+machine_alias = "s390-ccw-virtio";
arch_opts = g_strdup_printf("-bios %s", bootpath);
start_address = S390_TEST_MEM_START;
end_address = S390_TEST_MEM_END;
@@ -771,11 +774,14 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
"'nvramrc=hex .\" _\" begin %x %x "
"do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
"until'", end_address, start_address);
-arch_opts = g_strdup("-nodefaults -machine vsmt=8");
+machine_alias = "pseries";
+machine_opts = "vsmt=8";
+arch_opts = g_strdup("-nodefaults");
} else if (strcmp(arch, "aarch64") == 0) {
memory_size = "150M";
-arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
-"-kernel %s", bootpath);
+machine_alias = "virt";
+machine_opts = "gic-version=max";
+arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
start_address = ARM_TEST_MEM_START;
end_address = ARM_TEST_MEM_END;
} else {
@@ -810,11 +816,13 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
}
cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
+ "-machine %s,%s "
"-name source,debug-threads=on "
"-m %s "
"-serial file:%s/src_serial "
"%s %s %s %s %s",
kvm_opts ? kvm_opts : "",
+ machine_alias, machine_opts,
memory_size, tmpfs,
arch_opts ? arch_opts : "",
arch_source ? arch_source : "",
@@ -829,12 +837,14 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
}
cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
+ "-machine %s,%s "
"-name target,debug-threads=on "
"-m %s "
"-serial file:%s/dest_serial "
"-incoming %s "
"%s %s %s %s %s",
kvm_opts ? kvm_opts : "",
+ machine_alias, machine_opts,
memory_size, tmpfs, uri,
arch_opts ? arch_opts : "",
arch_target ? arch_target : "",
--
2.41.0
[PULL 13/17] tests/qtest/migration: Specify the geometry of the bootsector
From: Fabiano Rosas
We're about to enable the x86_64 tests to run with the q35 machine,
but that machine does not work with the program we use to dirty the
memory for the tests.
The issue is that QEMU needs to guess the geometry of the "disk" we
give to it and the guessed geometry doesn't pass the sanity checks
done by SeaBIOS. This causes SeaBIOS to interpret the geometry as if
needing a translation from LBA to CHS and SeaBIOS ends up miscomputing
the number of cylinders and aborting due to that.
The reason things work with the "pc" machine is that is uses ATA
instead of AHCI like q35 and SeaBIOS has an exception for ATA that
ends up skipping the sanity checks and ignoring translation
altogether.
Workaround this situation by specifying a geometry in the command
line.
Signed-off-by: Fabiano Rosas
Acked-by: Thomas Huth
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <20231018192741.25885-9-faro...@suse.de>
---
tests/qtest/migration-test.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index dfea75b76f..9959a0dc12 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -757,7 +757,9 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
memory_size = "150M";
machine_alias = "pc";
-arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
+arch_opts = g_strdup_printf(
+"-drive if=none,id=d0,file=%s,format=raw "
+"-device ide-hd,drive=d0,secs=1,cyls=1,heads=1", bootpath);
start_address = X86_TEST_MEM_START;
end_address = X86_TEST_MEM_END;
} else if (g_str_equal(arch, "s390x")) {
--
2.41.0
[PULL 07/17] tests/qtest: Introduce qtest_init_with_env
From: Fabiano Rosas
Add a version of qtest_init() that takes an environment variable
containing the path of the QEMU binary. This allows tests to use more
than one QEMU binary.
If no variable is provided or the environment variable does not exist,
that is not an error. Fallback to using QTEST_QEMU_BINARY.
Signed-off-by: Fabiano Rosas
Reviewed-by: Juan Quintela
Reviewed-by: Thomas Huth
Signed-off-by: Juan Quintela
Message-ID: <20231018192741.25885-3-faro...@suse.de>
---
tests/qtest/libqtest.h | 13 +
tests/qtest/libqtest.c | 26 +++---
2 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index 5fe3d13466..76fc195f1c 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -55,6 +55,19 @@ QTestState *qtest_vinitf(const char *fmt, va_list ap)
G_GNUC_PRINTF(1, 0);
*/
QTestState *qtest_init(const char *extra_args);
+/**
+ * qtest_init_with_env:
+ * @var: Environment variable from where to take the QEMU binary
+ * @extra_args: Other arguments to pass to QEMU. CAUTION: these
+ * arguments are subject to word splitting and shell evaluation.
+ *
+ * Like qtest_init(), but use a different environment variable for the
+ * QEMU binary.
+ *
+ * Returns: #QTestState instance.
+ */
+QTestState *qtest_init_with_env(const char *var, const char *extra_args);
+
/**
* qtest_init_without_qmp_handshake:
* @extra_args: other arguments to pass to QEMU. CAUTION: these
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 03fa644663..9eebba8767 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -388,7 +388,8 @@ static pid_t qtest_create_process(char *cmd)
}
#endif /* _WIN32 */
-static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
+static QTestState *G_GNUC_PRINTF(2, 3) qtest_spawn_qemu(const char *qemu_bin,
+const char *fmt, ...)
{
va_list ap;
QTestState *s = g_new0(QTestState, 1);
@@ -398,8 +399,7 @@ static QTestState *G_GNUC_PRINTF(1, 2)
qtest_spawn_qemu(const char *fmt, ...)
g_autoptr(GString) command = g_string_new("");
va_start(ap, fmt);
-g_string_append_printf(command, CMD_EXEC "%s %s",
- qtest_qemu_binary(NULL), tracearg);
+g_string_append_printf(command, CMD_EXEC "%s %s", qemu_bin, tracearg);
g_string_append_vprintf(command, fmt, ap);
va_end(ap);
@@ -438,7 +438,8 @@ static QTestState *G_GNUC_PRINTF(1, 2)
qtest_spawn_qemu(const char *fmt, ...)
return s;
}
-QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+static QTestState *qtest_init_internal(const char *qemu_bin,
+ const char *extra_args)
{
QTestState *s;
int sock, qmpsock, i;
@@ -463,7 +464,8 @@ QTestState *qtest_init_without_qmp_handshake(const char
*extra_args)
sock = init_socket(socket_path);
qmpsock = init_socket(qmp_socket_path);
-s = qtest_spawn_qemu("-qtest unix:%s "
+s = qtest_spawn_qemu(qemu_bin,
+ "-qtest unix:%s "
"-qtest-log %s "
"-chardev socket,path=%s,id=char0 "
"-mon chardev=char0,mode=control "
@@ -516,9 +518,14 @@ QTestState *qtest_init_without_qmp_handshake(const char
*extra_args)
return s;
}
-QTestState *qtest_init(const char *extra_args)
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
{
-QTestState *s = qtest_init_without_qmp_handshake(extra_args);
+return qtest_init_internal(qtest_qemu_binary(NULL), extra_args);
+}
+
+QTestState *qtest_init_with_env(const char *var, const char *extra_args)
+{
+QTestState *s = qtest_init_internal(qtest_qemu_binary(var), extra_args);
QDict *greeting;
/* Read the QMP greeting and then do the handshake */
@@ -529,6 +536,11 @@ QTestState *qtest_init(const char *extra_args)
return s;
}
+QTestState *qtest_init(const char *extra_args)
+{
+return qtest_init_with_env(NULL, extra_args);
+}
+
QTestState *qtest_vinitf(const char *fmt, va_list ap)
{
char *args = g_strdup_vprintf(fmt, ap);
--
2.41.0
[PULL 17/17] tests/qtest: Don't print messages from query instances
From: Fabiano Rosas
Now that we can query more than one binary, the "starting QEMU..."
message can get a little noisy. Mute those messages unless we're
running with --verbose.
Only affects qtest_init() calls from within libqtest. The tests
continue to output as usual.
Reviewed-by: Juan Quintela
Reviewed-by: Thomas Huth
Signed-off-by: Fabiano Rosas
Message-ID: <20231018192741.25885-13-faro...@suse.de>
Signed-off-by: Juan Quintela
---
tests/qtest/libqtest.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index c843c41188..f33a210861 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -91,6 +91,7 @@ struct QTestState
static GHookList abrt_hooks;
static void (*sighandler_old)(int);
+static bool silence_spawn_log;
static int qtest_query_target_endianness(QTestState *s);
@@ -405,7 +406,9 @@ static QTestState *G_GNUC_PRINTF(2, 3)
qtest_spawn_qemu(const char *qemu_bin,
qtest_add_abrt_handler(kill_qemu_hook_func, s);
-g_test_message("starting QEMU: %s", command->str);
+if (!silence_spawn_log) {
+g_test_message("starting QEMU: %s", command->str);
+}
#ifndef _WIN32
s->qemu_pid = fork();
@@ -1508,6 +1511,8 @@ static struct MachInfo *qtest_get_machines(const char
*var)
return machines;
}
+silence_spawn_log = !g_test_verbose();
+
qts = qtest_init_with_env(qemu_var, "-machine none");
response = qtest_qmp(qts, "{ 'execute': 'query-machines' }");
g_assert(response);
@@ -1539,6 +1544,8 @@ static struct MachInfo *qtest_get_machines(const char
*var)
qtest_quit(qts);
qobject_unref(response);
+silence_spawn_log = false;
+
memset(&machines[idx], 0, sizeof(struct MachInfo)); /* Terminating entry */
return machines;
}
--
2.41.0
[PULL 05/17] migration/multifd: Stop checking p->quit in multifd_send_thread
From: Fabiano Rosas
We don't need to check p->quit in the multifd_send_thread() because it
is shadowed by the 'exiting' flag. Ever since that flag was added
p->quit became obsolete as a way to stop the thread.
Since p->quit is set at multifd_send_terminate_threads() under the
p->mutex lock, the thread will only see it once it loops, so 'exiting'
will always be seen first.
Note that setting p->quit at multifd_send_terminate_threads() still
makes sense because we need a way to inform multifd_send_pages() that
the channel has stopped.
Signed-off-by: Fabiano Rosas
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <20231012140651.13122-3-faro...@suse.de>
---
migration/multifd.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index 1fe53d3b98..e2a45c667a 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -743,9 +743,6 @@ static void *multifd_send_thread(void *opaque)
if (flags & MULTIFD_FLAG_SYNC) {
qemu_sem_post(&p->sem_sync);
}
-} else if (p->quit) {
-qemu_mutex_unlock(&p->mutex);
-break;
} else {
qemu_mutex_unlock(&p->mutex);
/* sometimes there are spurious wakeups */
--
2.41.0
[PULL 08/17] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary
From: Fabiano Rosas
We're adding support for using more than one QEMU binary in
tests. Modify qtest_get_machines() to take an environment variable
that contains the QEMU binary path.
Since the function keeps a cache of the machines list in the form of a
static variable, refresh it any time the environment variable changes.
Reviewed-by: Juan Quintela
Reviewed-by: Thomas Huth
Signed-off-by: Fabiano Rosas
Signed-off-by: Juan Quintela
Message-ID: <20231018192741.25885-4-faro...@suse.de>
---
tests/qtest/libqtest.c | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 9eebba8767..3cc7bf3076 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1468,13 +1468,26 @@ struct MachInfo {
char *alias;
};
+static void qtest_free_machine_list(struct MachInfo *machines)
+{
+if (machines) {
+for (int i = 0; machines[i].name != NULL; i++) {
+g_free(machines[i].name);
+g_free(machines[i].alias);
+}
+
+g_free(machines);
+}
+}
+
/*
* Returns an array with pointers to the available machine names.
* The terminating entry has the name set to NULL.
*/
-static struct MachInfo *qtest_get_machines(void)
+static struct MachInfo *qtest_get_machines(const char *var)
{
static struct MachInfo *machines;
+static char *qemu_var;
QDict *response, *minfo;
QList *list;
const QListEntry *p;
@@ -1483,11 +1496,19 @@ static struct MachInfo *qtest_get_machines(void)
QTestState *qts;
int idx;
+if (g_strcmp0(qemu_var, var)) {
+qemu_var = g_strdup(var);
+
+/* new qemu, clear the cache */
+qtest_free_machine_list(machines);
+machines = NULL;
+}
+
if (machines) {
return machines;
}
-qts = qtest_init("-machine none");
+qts = qtest_init_with_env(qemu_var, "-machine none");
response = qtest_qmp(qts, "{ 'execute': 'query-machines' }");
g_assert(response);
list = qdict_get_qlist(response, "return");
@@ -1528,7 +1549,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char
*machine),
struct MachInfo *machines;
int i;
-machines = qtest_get_machines();
+machines = qtest_get_machines(NULL);
for (i = 0; machines[i].name != NULL; i++) {
/* Ignore machines that cannot be used for qtests */
@@ -1549,7 +1570,7 @@ bool qtest_has_machine(const char *machine)
struct MachInfo *machines;
int i;
-machines = qtest_get_machines();
+machines = qtest_get_machines(NULL);
for (i = 0; machines[i].name != NULL; i++) {
if (g_str_equal(machine, machines[i].name) ||
--
2.41.0
[PULL 01/17] tests/qtest/migration-test: Disable the analyze-migration.py test on s390x
From: Thomas Huth
The analyze-migration.py script fails on s390x hosts:
Traceback (most recent call last):
File "scripts/analyze-migration.py", line 662, in
dump.read(dump_memory = args.memory)
File "scripts/analyze-migration.py", line 596, in read
classdesc = self.section_classes[section_key]
KeyError: ('s390-storage_attributes', 0)
It obviously never has been adapted to s390x yet, so until this
has been done, disable this test on s390x.
Signed-off-by: Thomas Huth
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <20231018091239.164452-1-th...@redhat.com>
---
tests/qtest/migration-test.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index e1c110537b..241b409857 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -3034,7 +3034,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);
+if (!g_str_equal(arch, "s390x")) {
+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);
--
2.41.0
