| Message ID | f0bf02377f18f3cf6b8942528b3b5bce97fb6ab7.1741344976.git.maciej.szmigiero@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Avihai, Could you send a Ack on this patch ? Thanks, C. On 3/7/25 11:57, Maciej S. Szmigiero wrote: > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > Wire data commonly use BE byte order (including in the existing migration > protocol), use it also for for VFIO device state packets. > > Fixes: 3228d311ab18 ("vfio/migration: Multifd device state transfer support - received buffers queuing") > Fixes: 6d644baef203 ("vfio/migration: Multifd device state transfer support - send side") > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > --- > hw/vfio/migration-multifd.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c > index a9d41b9f1cb1..e816461e1652 100644 > --- a/hw/vfio/migration-multifd.c > +++ b/hw/vfio/migration-multifd.c > @@ -13,6 +13,7 @@ > #include "hw/vfio/vfio-common.h" > #include "migration/misc.h" > #include "qapi/error.h" > +#include "qemu/bswap.h" > #include "qemu/error-report.h" > #include "qemu/lockable.h" > #include "qemu/main-loop.h" > @@ -208,12 +209,16 @@ bool vfio_multifd_load_state_buffer(void *opaque, char *data, size_t data_size, > return false; > } > > + packet->version = be32_to_cpu(packet->version); > if (packet->version != VFIO_DEVICE_STATE_PACKET_VER_CURRENT) { > error_setg(errp, "%s: packet has unknown version %" PRIu32, > vbasedev->name, packet->version); > return false; > } > > + packet->idx = be32_to_cpu(packet->idx); > + packet->flags = be32_to_cpu(packet->flags); > + > if (packet->idx == UINT32_MAX) { > error_setg(errp, "%s: packet index is invalid", vbasedev->name); > return false; > @@ -682,9 +687,9 @@ vfio_save_complete_precopy_thread_config_state(VFIODevice *vbasedev, > > packet_len = sizeof(*packet) + bioc->usage; > packet = g_malloc0(packet_len); > - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; > - packet->idx = idx; > - packet->flags = VFIO_DEVICE_STATE_CONFIG_STATE; > + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); > + packet->idx = cpu_to_be32(idx); > + packet->flags = cpu_to_be32(VFIO_DEVICE_STATE_CONFIG_STATE); > memcpy(&packet->data, bioc->data, bioc->usage); > > if (!multifd_queue_device_state(idstr, instance_id, > @@ -734,7 +739,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, > } > > packet = g_malloc0(sizeof(*packet) + migration->data_buffer_size); > - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; > + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); > > for (idx = 0; ; idx++) { > ssize_t data_size; > @@ -755,7 +760,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, > break; > } > > - packet->idx = idx; > + packet->idx = cpu_to_be32(idx); > packet_size = sizeof(*packet) + data_size; > > if (!multifd_queue_device_state(d->idstr, d->instance_id, >
On 3/7/25 11:57, Maciej S. Szmigiero wrote: > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > Wire data commonly use BE byte order (including in the existing migration > protocol), use it also for for VFIO device state packets. > > Fixes: 3228d311ab18 ("vfio/migration: Multifd device state transfer support - received buffers queuing") > Fixes: 6d644baef203 ("vfio/migration: Multifd device state transfer support - send side") > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> Maciej, Could you please resend this patch as a standalone patch ? and not as a reply on : https://lore.kernel.org/qemu-devel/cover.1741124640.git.maciej.szmigiero@oracle.com/ This is confusing b4. Thanks, C. > --- > hw/vfio/migration-multifd.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c > index a9d41b9f1cb1..e816461e1652 100644 > --- a/hw/vfio/migration-multifd.c > +++ b/hw/vfio/migration-multifd.c > @@ -13,6 +13,7 @@ > #include "hw/vfio/vfio-common.h" > #include "migration/misc.h" > #include "qapi/error.h" > +#include "qemu/bswap.h" > #include "qemu/error-report.h" > #include "qemu/lockable.h" > #include "qemu/main-loop.h" > @@ -208,12 +209,16 @@ bool vfio_multifd_load_state_buffer(void *opaque, char *data, size_t data_size, > return false; > } > > + packet->version = be32_to_cpu(packet->version); > if (packet->version != VFIO_DEVICE_STATE_PACKET_VER_CURRENT) { > error_setg(errp, "%s: packet has unknown version %" PRIu32, > vbasedev->name, packet->version); > return false; > } > > + packet->idx = be32_to_cpu(packet->idx); > + packet->flags = be32_to_cpu(packet->flags); > + > if (packet->idx == UINT32_MAX) { > error_setg(errp, "%s: packet index is invalid", vbasedev->name); > return false; > @@ -682,9 +687,9 @@ vfio_save_complete_precopy_thread_config_state(VFIODevice *vbasedev, > > packet_len = sizeof(*packet) + bioc->usage; > packet = g_malloc0(packet_len); > - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; > - packet->idx = idx; > - packet->flags = VFIO_DEVICE_STATE_CONFIG_STATE; > + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); > + packet->idx = cpu_to_be32(idx); > + packet->flags = cpu_to_be32(VFIO_DEVICE_STATE_CONFIG_STATE); > memcpy(&packet->data, bioc->data, bioc->usage); > > if (!multifd_queue_device_state(idstr, instance_id, > @@ -734,7 +739,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, > } > > packet = g_malloc0(sizeof(*packet) + migration->data_buffer_size); > - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; > + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); > > for (idx = 0; ; idx++) { > ssize_t data_size; > @@ -755,7 +760,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, > break; > } > > - packet->idx = idx; > + packet->idx = cpu_to_be32(idx); > packet_size = sizeof(*packet) + data_size; > > if (!multifd_queue_device_state(d->idstr, d->instance_id, >
On 07/03/2025 12:57, Maciej S. Szmigiero wrote: > External email: Use caution opening links or attachments > > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > Wire data commonly use BE byte order (including in the existing migration > protocol), use it also for for VFIO device state packets. Nit: should we add a sentence about the motivation? Something like: This will allow VFIO multifd device state transfer between hosts with different endianness. Although currently there is no such use case, it's good to have it now for completeness. > > Fixes: 3228d311ab18 ("vfio/migration: Multifd device state transfer support - received buffers queuing") > Fixes: 6d644baef203 ("vfio/migration: Multifd device state transfer support - send side") > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> Reviewed-by: Avihai Horon <avihaih@nvidia.com> Thanks. > --- > hw/vfio/migration-multifd.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c > index a9d41b9f1cb1..e816461e1652 100644 > --- a/hw/vfio/migration-multifd.c > +++ b/hw/vfio/migration-multifd.c > @@ -13,6 +13,7 @@ > #include "hw/vfio/vfio-common.h" > #include "migration/misc.h" > #include "qapi/error.h" > +#include "qemu/bswap.h" > #include "qemu/error-report.h" > #include "qemu/lockable.h" > #include "qemu/main-loop.h" > @@ -208,12 +209,16 @@ bool vfio_multifd_load_state_buffer(void *opaque, char *data, size_t data_size, > return false; > } > > + packet->version = be32_to_cpu(packet->version); > if (packet->version != VFIO_DEVICE_STATE_PACKET_VER_CURRENT) { > error_setg(errp, "%s: packet has unknown version %" PRIu32, > vbasedev->name, packet->version); > return false; > } > > + packet->idx = be32_to_cpu(packet->idx); > + packet->flags = be32_to_cpu(packet->flags); > + > if (packet->idx == UINT32_MAX) { > error_setg(errp, "%s: packet index is invalid", vbasedev->name); > return false; > @@ -682,9 +687,9 @@ vfio_save_complete_precopy_thread_config_state(VFIODevice *vbasedev, > > packet_len = sizeof(*packet) + bioc->usage; > packet = g_malloc0(packet_len); > - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; > - packet->idx = idx; > - packet->flags = VFIO_DEVICE_STATE_CONFIG_STATE; > + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); > + packet->idx = cpu_to_be32(idx); > + packet->flags = cpu_to_be32(VFIO_DEVICE_STATE_CONFIG_STATE); > memcpy(&packet->data, bioc->data, bioc->usage); > > if (!multifd_queue_device_state(idstr, instance_id, > @@ -734,7 +739,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, > } > > packet = g_malloc0(sizeof(*packet) + migration->data_buffer_size); > - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; > + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); > > for (idx = 0; ; idx++) { > ssize_t data_size; > @@ -755,7 +760,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, > break; > } > > - packet->idx = idx; > + packet->idx = cpu_to_be32(idx); > packet_size = sizeof(*packet) + data_size; > > if (!multifd_queue_device_state(d->idstr, d->instance_id,
On 3/10/25 09:17, Avihai Horon wrote: > > On 07/03/2025 12:57, Maciej S. Szmigiero wrote: >> External email: Use caution opening links or attachments >> >> >> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> >> >> Wire data commonly use BE byte order (including in the existing migration >> protocol), use it also for for VFIO device state packets. > > Nit: should we add a sentence about the motivation? Something like: > > This will allow VFIO multifd device state transfer between hosts with different endianness. > Although currently there is no such use case, it's good to have it now for completeness. Maciej, Could you please send a v2 with this change ? and >> >> Fixes: 3228d311ab18 ("vfio/migration: Multifd device state transfer support - received buffers queuing") >> Fixes: 6d644baef203 ("vfio/migration: Multifd device state transfer support - send side") we don't need these Fixes trailers because the feature is not part of a released QEMU version yet. >> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > > Reviewed-by: Avihai Horon <avihaih@nvidia.com> > > Thanks. Thanks Avihai, C. >> --- >> hw/vfio/migration-multifd.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c >> index a9d41b9f1cb1..e816461e1652 100644 >> --- a/hw/vfio/migration-multifd.c >> +++ b/hw/vfio/migration-multifd.c >> @@ -13,6 +13,7 @@ >> #include "hw/vfio/vfio-common.h" >> #include "migration/misc.h" >> #include "qapi/error.h" >> +#include "qemu/bswap.h" >> #include "qemu/error-report.h" >> #include "qemu/lockable.h" >> #include "qemu/main-loop.h" >> @@ -208,12 +209,16 @@ bool vfio_multifd_load_state_buffer(void *opaque, char *data, size_t data_size, >> return false; >> } >> >> + packet->version = be32_to_cpu(packet->version); >> if (packet->version != VFIO_DEVICE_STATE_PACKET_VER_CURRENT) { >> error_setg(errp, "%s: packet has unknown version %" PRIu32, >> vbasedev->name, packet->version); >> return false; >> } >> >> + packet->idx = be32_to_cpu(packet->idx); >> + packet->flags = be32_to_cpu(packet->flags); >> + >> if (packet->idx == UINT32_MAX) { >> error_setg(errp, "%s: packet index is invalid", vbasedev->name); >> return false; >> @@ -682,9 +687,9 @@ vfio_save_complete_precopy_thread_config_state(VFIODevice *vbasedev, >> >> packet_len = sizeof(*packet) + bioc->usage; >> packet = g_malloc0(packet_len); >> - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; >> - packet->idx = idx; >> - packet->flags = VFIO_DEVICE_STATE_CONFIG_STATE; >> + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); >> + packet->idx = cpu_to_be32(idx); >> + packet->flags = cpu_to_be32(VFIO_DEVICE_STATE_CONFIG_STATE); >> memcpy(&packet->data, bioc->data, bioc->usage); >> >> if (!multifd_queue_device_state(idstr, instance_id, >> @@ -734,7 +739,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, >> } >> >> packet = g_malloc0(sizeof(*packet) + migration->data_buffer_size); >> - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; >> + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); >> >> for (idx = 0; ; idx++) { >> ssize_t data_size; >> @@ -755,7 +760,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, >> break; >> } >> >> - packet->idx = idx; >> + packet->idx = cpu_to_be32(idx); >> packet_size = sizeof(*packet) + data_size; >> >> if (!multifd_queue_device_state(d->idstr, d->instance_id, >
On 10.03.2025 09:17, Avihai Horon wrote: > > On 07/03/2025 12:57, Maciej S. Szmigiero wrote: >> External email: Use caution opening links or attachments >> >> >> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> >> >> Wire data commonly use BE byte order (including in the existing migration >> protocol), use it also for for VFIO device state packets. > > Nit: should we add a sentence about the motivation? Something like: > > This will allow VFIO multifd device state transfer between hosts with different endianness. > Although currently there is no such use case, it's good to have it now for completeness. Added this paragraph to the commit message in v2. >> >> Fixes: 3228d311ab18 ("vfio/migration: Multifd device state transfer support - received buffers queuing") >> Fixes: 6d644baef203 ("vfio/migration: Multifd device state transfer support - send side") >> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > > Reviewed-by: Avihai Horon <avihaih@nvidia.com> > > Thanks. Thanks, Maciej
On 10.03.2025 10:23, Cédric Le Goater wrote: > On 3/10/25 09:17, Avihai Horon wrote: >> >> On 07/03/2025 12:57, Maciej S. Szmigiero wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> >>> >>> Wire data commonly use BE byte order (including in the existing migration >>> protocol), use it also for for VFIO device state packets. >> >> Nit: should we add a sentence about the motivation? Something like: >> >> This will allow VFIO multifd device state transfer between hosts with different endianness. >> Although currently there is no such use case, it's good to have it now for completeness. > > Maciej, > > Could you please send a v2 with this change ? and I've sent v2 now as a standalone patch. >>> >>> Fixes: 3228d311ab18 ("vfio/migration: Multifd device state transfer support - received buffers queuing") >>> Fixes: 6d644baef203 ("vfio/migration: Multifd device state transfer support - send side") > > we don't need these Fixes trailers because the feature is not part of > a released QEMU version yet. Removed these tags now. I've originally added them because you said last week it's going to be a "Fixes" patch: https://lore.kernel.org/qemu-devel/7eb4dd80-0ae3-4522-bd1d-b004c19c4bf2@redhat.com/ >>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> >> >> Reviewed-by: Avihai Horon <avihaih@nvidia.com> >> >> Thanks. > > Thanks Avihai, > > C. Thanks, Maciej
On 3/10/25 13:53, Maciej S. Szmigiero wrote: > On 10.03.2025 10:23, Cédric Le Goater wrote: >> On 3/10/25 09:17, Avihai Horon wrote: >>> >>> On 07/03/2025 12:57, Maciej S. Szmigiero wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> >>>> >>>> Wire data commonly use BE byte order (including in the existing migration >>>> protocol), use it also for for VFIO device state packets. >>> >>> Nit: should we add a sentence about the motivation? Something like: >>> >>> This will allow VFIO multifd device state transfer between hosts with different endianness. >>> Although currently there is no such use case, it's good to have it now for completeness. >> >> Maciej, >> >> Could you please send a v2 with this change ? and > > I've sent v2 now as a standalone patch. > >>>> >>>> Fixes: 3228d311ab18 ("vfio/migration: Multifd device state transfer support - received buffers queuing") >>>> Fixes: 6d644baef203 ("vfio/migration: Multifd device state transfer support - send side") >> >> we don't need these Fixes trailers because the feature is not part of >> a released QEMU version yet. > > Removed these tags now. > > I've originally added them because you said last week it's going to be a "Fixes" patch: > https://lore.kernel.org/qemu-devel/7eb4dd80-0ae3-4522-bd1d-b004c19c4bf2@redhat.com/ yes. We are still in time for hard freeze, even soft freeze. So, it's less important. These tags are really useful for distro backports and for stable trees. On the TODO list, we still have to discuss : "vfio/migration: Add max in-flight VFIO device state buffer * limit" and we should be done for QEMU 10.0. Thanks, C.
diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c index a9d41b9f1cb1..e816461e1652 100644 --- a/hw/vfio/migration-multifd.c +++ b/hw/vfio/migration-multifd.c @@ -13,6 +13,7 @@ #include "hw/vfio/vfio-common.h" #include "migration/misc.h" #include "qapi/error.h" +#include "qemu/bswap.h" #include "qemu/error-report.h" #include "qemu/lockable.h" #include "qemu/main-loop.h" @@ -208,12 +209,16 @@ bool vfio_multifd_load_state_buffer(void *opaque, char *data, size_t data_size, return false; } + packet->version = be32_to_cpu(packet->version); if (packet->version != VFIO_DEVICE_STATE_PACKET_VER_CURRENT) { error_setg(errp, "%s: packet has unknown version %" PRIu32, vbasedev->name, packet->version); return false; } + packet->idx = be32_to_cpu(packet->idx); + packet->flags = be32_to_cpu(packet->flags); + if (packet->idx == UINT32_MAX) { error_setg(errp, "%s: packet index is invalid", vbasedev->name); return false; @@ -682,9 +687,9 @@ vfio_save_complete_precopy_thread_config_state(VFIODevice *vbasedev, packet_len = sizeof(*packet) + bioc->usage; packet = g_malloc0(packet_len); - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; - packet->idx = idx; - packet->flags = VFIO_DEVICE_STATE_CONFIG_STATE; + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); + packet->idx = cpu_to_be32(idx); + packet->flags = cpu_to_be32(VFIO_DEVICE_STATE_CONFIG_STATE); memcpy(&packet->data, bioc->data, bioc->usage); if (!multifd_queue_device_state(idstr, instance_id, @@ -734,7 +739,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, } packet = g_malloc0(sizeof(*packet) + migration->data_buffer_size); - packet->version = VFIO_DEVICE_STATE_PACKET_VER_CURRENT; + packet->version = cpu_to_be32(VFIO_DEVICE_STATE_PACKET_VER_CURRENT); for (idx = 0; ; idx++) { ssize_t data_size; @@ -755,7 +760,7 @@ vfio_multifd_save_complete_precopy_thread(SaveLiveCompletePrecopyThreadData *d, break; } - packet->idx = idx; + packet->idx = cpu_to_be32(idx); packet_size = sizeof(*packet) + data_size; if (!multifd_queue_device_state(d->idstr, d->instance_id,