Avihai Horon <avih...@nvidia.com> writes:
> On 09/09/2024 21:05, Maciej S. Szmigiero wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 5.09.2024 18:47, Avihai Horon wrote:
>>>
>>> On 27/08/2024 20:54, Maciej S. Szmigiero wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 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
>>>> @@ -21,6 +21,7 @@
>>>> #include "file.h"
>>>> #include "migration.h"
>>>> #include "migration-stats.h"
>>>> +#include "savevm.h"
>>>> #include "socket.h"
>>>> #include "tls.h"
>>>> #include "qemu-file.h"
>>>> @@ -209,10 +210,10 @@ void
>>>> multifd_send_fill_packet(MultiFDSendParams *p)
>>>>
>>>> memset(packet, 0, p->packet_len);
>>>>
>>>> - packet->magic = cpu_to_be32(MULTIFD_MAGIC);
>>>> - packet->version = cpu_to_be32(MULTIFD_VERSION);
>>>> + packet->hdr.magic = cpu_to_be32(MULTIFD_MAGIC);
>>>> + packet->hdr.version = cpu_to_be32(MULTIFD_VERSION);
>>>>
>>>> - packet->flags = cpu_to_be32(p->flags);
>>>> + packet->hdr.flags = cpu_to_be32(p->flags);
>>>> packet->next_packet_size = cpu_to_be32(p->next_packet_size);
>>>>
>>>> packet_num = qatomic_fetch_inc(&multifd_send_state->packet_num);
>>>> @@ -228,31 +229,49 @@ void
>>>> multifd_send_fill_packet(MultiFDSendParams *p)
>>>> p->flags, p->next_packet_size);
>>>> }
>>>>
>>>> -static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error
>>>> **errp)
>>>> +static int multifd_recv_unfill_packet_header(MultiFDRecvParams *p,
>>>> + MultiFDPacketHdr_t *hdr,
>>>> + Error **errp)
>>>> {
>>>> - MultiFDPacket_t *packet = p->packet;
>>>> - int ret = 0;
>>>> -
>>>> - packet->magic = be32_to_cpu(packet->magic);
>>>> - if (packet->magic != MULTIFD_MAGIC) {
>>>> + hdr->magic = be32_to_cpu(hdr->magic);
>>>> + if (hdr->magic != MULTIFD_MAGIC) {
>>>> error_setg(errp, "multifd: received packet "
>>>> "magic %x and expected magic %x",
>>>> - packet->magic, MULTIFD_MAGIC);
>>>> + hdr->magic, MULTIFD_MAGIC);
>>>> return -1;
>>>> }
>>>>
>>>> - packet->version = be32_to_cpu(packet->version);
>>>> - if (packet->version != MULTIFD_VERSION) {
>>>> + hdr->version = be32_to_cpu(hdr->version);
>>>> + if (hdr->version != MULTIFD_VERSION) {
>>>> error_setg(errp, "multifd: received packet "
>>>> "version %u and expected version %u",
>>>> - packet->version, MULTIFD_VERSION);
>>>> + hdr->version, MULTIFD_VERSION);
>>>> return -1;
>>>> }
>>>>
>>>> - p->flags = be32_to_cpu(packet->flags);
>>>> + p->flags = be32_to_cpu(hdr->flags);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int
>>>> multifd_recv_unfill_packet_device_state(MultiFDRecvParams *p,
>>>> + Error **errp)
>>>> +{
>>>> + MultiFDPacketDeviceState_t *packet = p->packet_dev_state;
>>>> +
>>>> + packet->instance_id = be32_to_cpu(packet->instance_id);
>>>> + p->next_packet_size = be32_to_cpu(packet->next_packet_size);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int multifd_recv_unfill_packet_ram(MultiFDRecvParams *p,
>>>> Error **errp)
>>>> +{
>>>> + MultiFDPacket_t *packet = p->packet;
>>>> + int ret = 0;
>>>> +
>>>> p->next_packet_size = be32_to_cpu(packet->next_packet_size);
>>>> p->packet_num = be64_to_cpu(packet->packet_num);
>>>> - p->packets_recved++;
>>>>
>>>> if (!(p->flags & MULTIFD_FLAG_SYNC)) {
>>>> ret = multifd_ram_unfill_packet(p, errp);
>>>> @@ -264,6 +283,19 @@ static int
>>>> multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>>>> return ret;
>>>> }
>>>>
>>>> +static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error
>>>> **errp)
>>>> +{
>>>> + p->packets_recved++;
>>>> +
>>>> + if (p->flags & MULTIFD_FLAG_DEVICE_STATE) {
>>>> + return multifd_recv_unfill_packet_device_state(p, errp);
>>>> + } else {
>>>> + return multifd_recv_unfill_packet_ram(p, errp);
>>>> + }
>>>> +
>>>> + g_assert_not_reached();
>>>
>>> We can drop the assert and the "else":
>>> if (p->flags & MULTIFD_FLAG_DEVICE_STATE) {
>>> return multifd_recv_unfill_packet_device_state(p, errp);
>>> }
>>>
>>> return multifd_recv_unfill_packet_ram(p, errp);
>>
>> Ack.
>>
>>>> +}
>>>> +
>>>> static bool multifd_send_should_exit(void)
>>>> {
>>>> return qatomic_read(&multifd_send_state->exiting);
>>>> diff --git a/migration/multifd.h b/migration/multifd.h
>>>> index a3e35196d179..a8f3e4838c01 100644
>>>> --- a/migration/multifd.h
>>>> +++ b/migration/multifd.h
>>>> @@ -45,6 +45,12 @@ MultiFDRecvData *multifd_get_recv_data(void);
>>>> #define MULTIFD_FLAG_QPL (4 << 1)
>>>> #define MULTIFD_FLAG_UADK (8 << 1)
>>>>
>>>> +/*
>>>> + * If set it means that this packet contains device state
>>>> + * (MultiFDPacketDeviceState_t), not RAM data (MultiFDPacket_t).
>>>> + */
>>>> +#define MULTIFD_FLAG_DEVICE_STATE (1 << 4)
>>>> +
>>>> /* This value needs to be a multiple of qemu_target_page_size() */
>>>> #define MULTIFD_PACKET_SIZE (512 * 1024)
>>>>
>>>> @@ -52,6 +58,11 @@ typedef struct {
>>>> uint32_t magic;
>>>> uint32_t version;
>>>> uint32_t flags;
>>>> +} __attribute__((packed)) MultiFDPacketHdr_t;
>>>
>>> Maybe split this patch into two: one that adds the packet header
>>> concept and another that adds the new device packet?
>>
>> Can do.
>>
>>>> +
>>>> +typedef struct {
>>>> + MultiFDPacketHdr_t hdr;
>>>> +
>>>> /* maximum number of allocated pages */
>>>> uint32_t pages_alloc;
>>>> /* non zero pages */
>>>> @@ -72,6 +83,16 @@ typedef struct {
>>>> uint64_t offset[];
>>>> } __attribute__((packed)) MultiFDPacket_t;
>>>>
>>>> +typedef struct {
>>>> + MultiFDPacketHdr_t hdr;
>>>> +
>>>> + char idstr[256] QEMU_NONSTRING;
>>>
>>> idstr should be null terminated, or am I missing something?
>>
>> There's no need to always NULL-terminate a constant-size field,
>> since the strncpy() already stops at the field size, so we can
>> gain another byte for actual string use this way.
>>
>> RAM block idstr also uses the same "trick":
>>> void multifd_ram_fill_packet(MultiFDSendParams *p):
>>> strncpy(packet->ramblock, pages->block->idstr, 256);
>>
> But can idstr actually be 256 bytes long without null byte?
> There are a lot of places where idstr is a parameter for functions that
> expect null terminated string and it is also printed as such.
Yeah, and I actually don't see the "trick" being used in
RAMBlock. Anyway, it's best to null terminate to be more predictable. We
also had Coverity reports about similar things:
https://lore.kernel.org/r/CAFEAcA_F2qrSAacY=V5Hez1qFGuNW0-XqL2LQ=y_ukyuhej...@mail.gmail.com
I haven't got the time to send that patch yet.
>
> Thanks.
