Re: [PATCH v4 20/33] vfio/migration: Add x-migration-load-config-after-iter VFIO property
On 2/11/25 16:57, Maciej S. Szmigiero wrote:
On 11.02.2025 16:00, Cédric Le Goater wrote:
On 2/11/25 15:37, Maciej S. Szmigiero wrote:
On 10.02.2025 18:24, Cédric Le Goater wrote:
On 1/30/25 11:08, Maciej S. Szmigiero wrote:
From: "Maciej S. Szmigiero"
This property allows configuring whether to start the config load only
after all iterables were loaded.
Such interlocking is required for ARM64 due to this platform VFIO
dependency on interrupt controller being loaded first.
The property defaults to AUTO, which means ON for ARM, OFF for other
platforms.>
Signed-off-by: Maciej S. Szmigiero
---
hw/vfio/migration.c | 25 +
hw/vfio/pci.c | 3 +++
include/hw/vfio/vfio-common.h | 1 +
3 files changed, 29 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index adfa752db527..d801c861d202 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -254,6 +254,31 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice
*vbasedev,
return ret;
}
+static bool vfio_load_config_after_iter(VFIODevice *vbasedev)
+{
+ if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
+ return true;
+ } else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
+ return false;
+ }
+
+ assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
+
+ /*
+ * Starting the config load only after all iterables were loaded is
required
+ * for ARM64 due to this platform VFIO dependency on interrupt controller
+ * being loaded first.
+ *
+ * See commit d329f5032e17 ("vfio: Move the saving of the config space to
+ * the right place in VFIO migration").
+ */
+#if defined(TARGET_ARM)
+ return true;
+#else
+ return false;
+#endif
I would rather deactivate support on ARM and avoid workarounds.
This can be done in routine vfio_multifd_transfer_supported() I believe,
at the end of this series. A warning can be added to inform the user.
The reason why this interlocking support (x-migration-load-config-after-iter)
was added because you said during the review of the previous version of
this patch set that "regarding ARM64, it would be unfortunate to deactivate
the feature since migration works correctly today [..] and this series should
improve also downtime":
https://lore.kernel.org/qemu-devel/59897119-25d7-4a8b-9616-f8ab54e03...@redhat.com/
So much happened since ... my bad. I think this patch is not well
placed in the series, it should be at the end.
The series should present first the feature in a perfect world
and introduce at the end the toggles to handle the corner cases.
It helps the reader to focus on the good side of the proposal
and better understand the more unpleasant/ugly part.
My point is that after spending time developing and testing that feature> (or
"workaround") it would be a shame to throw it away (with all the benefits
it brings) and completely disable multifd VFIO device state transfer on ARM.
Well, if you take the approach described above, this patch would
be proposed after merge as a fix/workaround for ARM or we would
fix the ARM platform.
Looks like there should be no problems moving this
x-migration-load-config-after-iter
feature to a separate patch near the end of the series - I will try
to do this.
Thanks, The patch is broken anyhow, since vfio_load_config_after_iter
is unused.
Or am I misunderstanding you right now and you only mean here to make
x-migration-load-config-after-iter forcefully enabled on ARM?
If we only need this toggle for ARM, and this seems to be the case,
let's take a more direct path and avoid a property.
The reason why we likely want a some kind of switch even on
a non-ARM platform is ability to test this functionality there.
Most VFIO setups are probably x86 so people working on this code
will benefit from easy ability to check if they haven't accidentally
broke this interlocking.
it's a valid request. We might want to find a better name for the
property.
I haven't read all your series and the comments yet.
I keep the series updated with received review comments and re-based
on top of the latest Fabiano's TLS session termination patches here:
https://gitlab.com/maciejsszmigiero/qemu/-/commits/multifd-device-state-transfer-vfio
The changes up to this point has been mostly limited to migration
core code but it would be nice to get review comments for the VFIO
parts soon too so I can post a complete new version.
yes. Please introduce vfio/migration-multifd.{c,h} files. I would like
the interface to be clear and avoid bits and pieces of multifd support
slipping into the VFIO migration component.
Also, please provide documentation on :
- the design principles and its limitations (the mlx5 kernel driver
has some outcomes)
- a guide on how to use VFIO migration with multifd
- how the properties (migration and VFIO) can be set to improve
performance.
It's not trivial, we w
Re: [PATCH v4 20/33] vfio/migration: Add x-migration-load-config-after-iter VFIO property
On 11.02.2025 16:00, Cédric Le Goater wrote:
On 2/11/25 15:37, Maciej S. Szmigiero wrote:
On 10.02.2025 18:24, Cédric Le Goater wrote:
On 1/30/25 11:08, Maciej S. Szmigiero wrote:
From: "Maciej S. Szmigiero"
This property allows configuring whether to start the config load only
after all iterables were loaded.
Such interlocking is required for ARM64 due to this platform VFIO
dependency on interrupt controller being loaded first.
The property defaults to AUTO, which means ON for ARM, OFF for other
platforms.>
Signed-off-by: Maciej S. Szmigiero
---
hw/vfio/migration.c | 25 +
hw/vfio/pci.c | 3 +++
include/hw/vfio/vfio-common.h | 1 +
3 files changed, 29 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index adfa752db527..d801c861d202 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -254,6 +254,31 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice
*vbasedev,
return ret;
}
+static bool vfio_load_config_after_iter(VFIODevice *vbasedev)
+{
+ if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
+ return true;
+ } else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
+ return false;
+ }
+
+ assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
+
+ /*
+ * Starting the config load only after all iterables were loaded is
required
+ * for ARM64 due to this platform VFIO dependency on interrupt controller
+ * being loaded first.
+ *
+ * See commit d329f5032e17 ("vfio: Move the saving of the config space to
+ * the right place in VFIO migration").
+ */
+#if defined(TARGET_ARM)
+ return true;
+#else
+ return false;
+#endif
I would rather deactivate support on ARM and avoid workarounds.
This can be done in routine vfio_multifd_transfer_supported() I believe,
at the end of this series. A warning can be added to inform the user.
The reason why this interlocking support (x-migration-load-config-after-iter)
was added because you said during the review of the previous version of
this patch set that "regarding ARM64, it would be unfortunate to deactivate
the feature since migration works correctly today [..] and this series should
improve also downtime":
https://lore.kernel.org/qemu-devel/59897119-25d7-4a8b-9616-f8ab54e03...@redhat.com/
So much happened since ... my bad. I think this patch is not well
placed in the series, it should be at the end.
The series should present first the feature in a perfect world
and introduce at the end the toggles to handle the corner cases.
It helps the reader to focus on the good side of the proposal
and better understand the more unpleasant/ugly part.
My point is that after spending time developing and testing that feature> (or
"workaround") it would be a shame to throw it away (with all the benefits
it brings) and completely disable multifd VFIO device state transfer on ARM.
Well, if you take the approach described above, this patch would
be proposed after merge as a fix/workaround for ARM or we would
fix the ARM platform.
Looks like there should be no problems moving this
x-migration-load-config-after-iter
feature to a separate patch near the end of the series - I will try
to do this.
Or am I misunderstanding you right now and you only mean here to make
x-migration-load-config-after-iter forcefully enabled on ARM?
If we only need this toggle for ARM, and this seems to be the case,
let's take a more direct path and avoid a property.
The reason why we likely want a some kind of switch even on
a non-ARM platform is ability to test this functionality there.
Most VFIO setups are probably x86 so people working on this code
will benefit from easy ability to check if they haven't accidentally
broke this interlocking.
I haven't read all your series and the comments yet.
I keep the series updated with received review comments and re-based
on top of the latest Fabiano's TLS session termination patches here:
https://gitlab.com/maciejsszmigiero/qemu/-/commits/multifd-device-state-transfer-vfio
The changes up to this point has been mostly limited to migration
core code but it would be nice to get review comments for the VFIO
parts soon too so I can post a complete new version.
Thanks,
C.
Thanks,
Maciej
Re: [PATCH v4 20/33] vfio/migration: Add x-migration-load-config-after-iter VFIO property
On 2/11/25 15:37, Maciej S. Szmigiero wrote:
On 10.02.2025 18:24, Cédric Le Goater wrote:
On 1/30/25 11:08, Maciej S. Szmigiero wrote:
From: "Maciej S. Szmigiero"
This property allows configuring whether to start the config load only
after all iterables were loaded.
Such interlocking is required for ARM64 due to this platform VFIO
dependency on interrupt controller being loaded first.
The property defaults to AUTO, which means ON for ARM, OFF for other
platforms.>
Signed-off-by: Maciej S. Szmigiero
---
hw/vfio/migration.c | 25 +
hw/vfio/pci.c | 3 +++
include/hw/vfio/vfio-common.h | 1 +
3 files changed, 29 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index adfa752db527..d801c861d202 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -254,6 +254,31 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice
*vbasedev,
return ret;
}
+static bool vfio_load_config_after_iter(VFIODevice *vbasedev)
+{
+ if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
+ return true;
+ } else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
+ return false;
+ }
+
+ assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
+
+ /*
+ * Starting the config load only after all iterables were loaded is
required
+ * for ARM64 due to this platform VFIO dependency on interrupt controller
+ * being loaded first.
+ *
+ * See commit d329f5032e17 ("vfio: Move the saving of the config space to
+ * the right place in VFIO migration").
+ */
+#if defined(TARGET_ARM)
+ return true;
+#else
+ return false;
+#endif
I would rather deactivate support on ARM and avoid workarounds.
This can be done in routine vfio_multifd_transfer_supported() I believe,
at the end of this series. A warning can be added to inform the user.
The reason why this interlocking support (x-migration-load-config-after-iter)
was added because you said during the review of the previous version of
this patch set that "regarding ARM64, it would be unfortunate to deactivate
the feature since migration works correctly today [..] and this series should
improve also downtime":
https://lore.kernel.org/qemu-devel/59897119-25d7-4a8b-9616-f8ab54e03...@redhat.com/
So much happened since ... my bad. I think this patch is not well
placed in the series, it should be at the end.
The series should present first the feature in a perfect world
and introduce at the end the toggles to handle the corner cases.
It helps the reader to focus on the good side of the proposal
and better understand the more unpleasant/ugly part.
My point is that after spending time developing and testing that feature> (or
"workaround") it would be a shame to throw it away (with all the benefits
it brings) and completely disable multifd VFIO device state transfer on ARM.
Well, if you take the approach described above, this patch would
be proposed after merge as a fix/workaround for ARM or we would
fix the ARM platform.
Or am I misunderstanding you right now and you only mean here to make
x-migration-load-config-after-iter forcefully enabled on ARM?
If we only need this toggle for ARM, and this seems to be the case,
let's take a more direct path and avoid a property.
I haven't read all your series and the comments yet.
Thanks,
C.
Thanks,
C.
Thanks,
Maciej
Re: [PATCH v4 20/33] vfio/migration: Add x-migration-load-config-after-iter VFIO property
On 10.02.2025 18:24, Cédric Le Goater wrote:
On 1/30/25 11:08, Maciej S. Szmigiero wrote:
From: "Maciej S. Szmigiero"
This property allows configuring whether to start the config load only
after all iterables were loaded.
Such interlocking is required for ARM64 due to this platform VFIO
dependency on interrupt controller being loaded first.
The property defaults to AUTO, which means ON for ARM, OFF for other
platforms.>
Signed-off-by: Maciej S. Szmigiero
---
hw/vfio/migration.c | 25 +
hw/vfio/pci.c | 3 +++
include/hw/vfio/vfio-common.h | 1 +
3 files changed, 29 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index adfa752db527..d801c861d202 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -254,6 +254,31 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice
*vbasedev,
return ret;
}
+static bool vfio_load_config_after_iter(VFIODevice *vbasedev)
+{
+ if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
+ return true;
+ } else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
+ return false;
+ }
+
+ assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
+
+ /*
+ * Starting the config load only after all iterables were loaded is
required
+ * for ARM64 due to this platform VFIO dependency on interrupt controller
+ * being loaded first.
+ *
+ * See commit d329f5032e17 ("vfio: Move the saving of the config space to
+ * the right place in VFIO migration").
+ */
+#if defined(TARGET_ARM)
+ return true;
+#else
+ return false;
+#endif
I would rather deactivate support on ARM and avoid workarounds.
This can be done in routine vfio_multifd_transfer_supported() I believe,
at the end of this series. A warning can be added to inform the user.
The reason why this interlocking support (x-migration-load-config-after-iter)
was added because you said during the review of the previous version of
this patch set that "regarding ARM64, it would be unfortunate to deactivate
the feature since migration works correctly today [..] and this series should
improve also downtime":
https://lore.kernel.org/qemu-devel/59897119-25d7-4a8b-9616-f8ab54e03...@redhat.com/
My point is that after spending time developing and testing that feature
(or "workaround") it would be a shame to throw it away (with all the benefits
it brings) and completely disable multifd VFIO device state transfer on ARM.
Or am I misunderstanding you right now and you only mean here to make
x-migration-load-config-after-iter forcefully enabled on ARM?
Thanks,
C.
Thanks,
Maciej
Re: [PATCH v4 20/33] vfio/migration: Add x-migration-load-config-after-iter VFIO property
On 1/30/25 11:08, Maciej S. Szmigiero wrote:
From: "Maciej S. Szmigiero"
This property allows configuring whether to start the config load only
after all iterables were loaded.
Such interlocking is required for ARM64 due to this platform VFIO
dependency on interrupt controller being loaded first.
The property defaults to AUTO, which means ON for ARM, OFF for other
platforms.>
Signed-off-by: Maciej S. Szmigiero
---
hw/vfio/migration.c | 25 +
hw/vfio/pci.c | 3 +++
include/hw/vfio/vfio-common.h | 1 +
3 files changed, 29 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index adfa752db527..d801c861d202 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -254,6 +254,31 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice
*vbasedev,
return ret;
}
+static bool vfio_load_config_after_iter(VFIODevice *vbasedev)
+{
+if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
+return true;
+} else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
+return false;
+}
+
+assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
+
+/*
+ * Starting the config load only after all iterables were loaded is
required
+ * for ARM64 due to this platform VFIO dependency on interrupt controller
+ * being loaded first.
+ *
+ * See commit d329f5032e17 ("vfio: Move the saving of the config space to
+ * the right place in VFIO migration").
+ */
+#if defined(TARGET_ARM)
+return true;
+#else
+return false;
+#endif
I would rather deactivate support on ARM and avoid workarounds.
This can be done in routine vfio_multifd_transfer_supported() I believe,
at the end of this series. A warning can be added to inform the user.
Thanks,
C.
+}
+
static int vfio_save_device_config_state(QEMUFile *f, void *opaque,
Error **errp)
{
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ab17a98ee5b6..83090c544d95 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3377,6 +3377,9 @@ static const Property vfio_pci_dev_properties[] = {
VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
+DEFINE_PROP_ON_OFF_AUTO("x-migration-load-config-after-iter",
VFIOPCIDevice,
+vbasedev.migration_load_config_after_iter,
+ON_OFF_AUTO_AUTO),
DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
vbasedev.migration_events, false),
DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 0c60be5b15c7..153d03745dc7 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -133,6 +133,7 @@ typedef struct VFIODevice {
bool no_mmap;
bool ram_block_discard_allowed;
OnOffAuto enable_migration;
+OnOffAuto migration_load_config_after_iter;
bool migration_events;
VFIODeviceOps *ops;
unsigned int num_irqs;
