On 3.09.2024 16:42, Fabiano Rosas wrote:
"Maciej S. Szmigiero" <m...@maciej.szmigiero.name> writes:
On 30.08.2024 22:22, Fabiano Rosas wrote:
"Maciej S. Szmigiero" <m...@maciej.szmigiero.name> writes:
From: "Maciej S. Szmigiero" <maciej.szmigi...@oracle.com>
Add a basic support for receiving device state via multifd channels -
channels that are shared with RAM transfers.
To differentiate between a device state and a RAM packet the packet
header is read first.
Depending whether MULTIFD_FLAG_DEVICE_STATE flag is present or not in the
packet header either device state (MultiFDPacketDeviceState_t) or RAM
data (existing MultiFDPacket_t) is then read.
The received device state data is provided to
qemu_loadvm_load_state_buffer() function for processing in the
device's load_state_buffer handler.
Signed-off-by: Maciej S. Szmigiero <maciej.szmigi...@oracle.com>
---
migration/multifd.c | 127 +++++++++++++++++++++++++++++++++++++-------
migration/multifd.h | 31 ++++++++++-
2 files changed, 138 insertions(+), 20 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index b06a9fab500e..d5a8e5a9c9b5 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
(..)
+
+ ret = qio_channel_read_all_eof(p->c, (char *)pkt_buf, pkt_len,
+ &local_err);
if (ret == 0 || ret == -1) { /* 0: EOF -1: Error */
break;
}
@@ -1181,8 +1239,33 @@ static void *multifd_recv_thread(void *opaque)
has_data = !!p->data->size;
}
- if (has_data) {
- ret = multifd_recv_state->ops->recv(p, &local_err);
+ if (!is_device_state) {
+ if (has_data) {
+ ret = multifd_recv_state->ops->recv(p, &local_err);
+ if (ret != 0) {
+ break;
+ }
+ }
+ } else {
+ g_autofree char *idstr = NULL;
+ g_autofree char *dev_state_buf = NULL;
+
+ assert(use_packets);
+
+ if (p->next_packet_size > 0) {
+ dev_state_buf = g_malloc(p->next_packet_size);
+
+ ret = qio_channel_read_all(p->c, dev_state_buf,
p->next_packet_size, &local_err);
+ if (ret != 0) {
+ break;
+ }
+ }
What's the use case for !next_packet_size and still call
load_state_buffer below? I can't see it.
Currently, next_packet_size == 0 has not usage indeed - it is
a leftover from an early version of the patch set (not public)
that had device state packet (chunk) indexing done by
the common migration code, rather than by the VFIO consumer.
And then an empty packet could be used to mark the stream
boundary - like the max chunk number to expect.
...because I would suggest to set has_data up there with
p->next_packet_size:
if (use_packets) {
...
has_data = p->next_packet_size || p->zero_num;
} else {
...
has_data = !!p->data_size;
}
and this whole block would be:
if (has_data) {
if (is_device_state) {
multifd_device_state_recv(p, &local_err);
} else {
ret = multifd_recv_state->ops->recv(p, &local_err);
}
}
The above block makes sense to me with two caveats:
I have suggestions below, but this is no big deal, so feel free to go
with what you think works best.
1) If empty device state packets (next_packet_size == 0) were
to be unsupported they need to be rejected cleanly rather
than silently skipped,
Should this be rejected on the send side? That's the most likely source
of the problem if it happens. Don't need to send something we know will
cause an error when loading.
Definitely we should send correct bit stream :), it was about the
case of bit stream corruption or simply using some future bit stream
format that the QEMU version with this patch set does not understand
yet.
And for the case of stream corruption of some sort we could hoist the
check from load_buffer into here:
else if (is_device_state) {
error_setg(errp, "empty device state packet);
break;
}
Right.
2) has_data has to have its value computed depending on whether
this is a RAM or a device state packet since looking at
p->normal_num and p->zero_num makes no sense for a device state
packet while I am not sure that looking at p->next_packet_size
for a RAM packet won't introduce some subtle regression.
It should be ok to use next_packet_size for RAM, it must always be in
sync with normal_num.
Then it should be ok, but I'll look at this deeper to be sure when
I will be preparing the next patch set version.
Thanks,
Maciej
