* [PATCH v6 0/9] vfio: Improve error reporting (part 2
@ 2024-05-14 15:31 Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
` (8 more replies)
0 siblings, 9 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater
Hello,
The motivation behind these changes is to improve error reporting to
the upper management layer (libvirt) with a more detailed error, this
to let it decide, depending on the reported error, whether to try
migration again later. It would be useful in cases where migration
fails due to lack of HW resources on the host. For instance, some
adapters can only initiate a limited number of simultaneous dirty
tracking requests and this imposes a limit on the the number of VMs
that can be migrated simultaneously.
We are not quite ready for such a mechanism but what we can do first is
to cleanup the error reporting in the early save_setup sequence. This
is what the following changes propose, by adding an Error** argument to
various handlers and propagating it to the core migration subsystem.
The first part [1] of this series modifying the core migration
subsystem is now merged. This is the second part changing VFIO which
was already proposed in March. See [2].
Thanks,
C.
[1] [PATCH for-9.1 v5 00/14] migration: Improve error reporting
https://lore.kernel.org/qemu-devel/20240320064911.545001-1-clg@redhat.com/
[2] [PATCH v4 00/25] migration: Improve error reporting
https://lore.kernel.org/qemu-devel/20240306133441.2351700-1-clg@redhat.com/
Changes in v6:
- Commit log improvements (Avihai)
- Modified some titles (Avihai)
- vfio_migration_set_state() : Dropped the error_setg_errno()
change when setting device in recover state fails (Avihai)
- vfio_migration_state_notifier() : report local error (Avihai)
- vfio_save_device_config_state() : Set errp if the migration
stream is in error (Avihai)
- vfio_save_state() : Changed error prefix (Avihai)
- vfio_iommu_map_dirty_notify() : Modified goto label (Avihai)
- Fixed memory_get_xlat_addr documentation (Avihai)
- Fixed line wrapping (Avihai)
- Fixed query_dirty_bitmap documentation (Avihai)
- Dropped last patch from v5 :
vfio: Extend vfio_set_migration_error() with Error* argument
Changes in v5:
- Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
- Fixed typo in set_dirty_page_tracking documentation
- Used error_setg_errno() in vfio_devices_dma_logging_start()
- Replaced error_setg() by error_setg_errno() in vfio_migration_set_state()
- Replaced error_setg() by error_setg_errno() in
vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap()
- ':' -> '-' in vfio_iommu_map_dirty_notify()
Cédric Le Goater (9):
vfio: Add Error** argument to .set_dirty_page_tracking() handler
vfio: Add Error** argument to vfio_devices_dma_logging_start()
migration: Extend migration_file_set_error() with Error* argument
vfio/migration: Add an Error** argument to vfio_migration_set_state()
vfio/migration: Add Error** argument to .vfio_save_config() handler
vfio: Reverse test on vfio_get_xlat_addr()
memory: Add Error** argument to memory_get_xlat_addr()
vfio: Add Error** argument to .get_dirty_bitmap() handler
vfio: Also trace event failures in vfio_save_complete_precopy()
include/exec/memory.h | 15 +++-
include/hw/vfio/vfio-common.h | 29 ++++++-
include/hw/vfio/vfio-container-base.h | 39 +++++++--
include/migration/misc.h | 2 +-
hw/vfio/common.c | 112 +++++++++++++++++---------
hw/vfio/container-base.c | 11 +--
hw/vfio/container.c | 20 +++--
hw/vfio/migration.c | 105 +++++++++++++++---------
hw/vfio/pci.c | 5 +-
hw/virtio/vhost-vdpa.c | 5 +-
migration/migration.c | 6 +-
system/memory.c | 10 +--
12 files changed, 246 insertions(+), 113 deletions(-)
--
2.45.0
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
@ 2024-05-14 15:31 ` Cédric Le Goater
2024-05-15 6:40 ` Eric Auger
2024-05-14 15:31 ` [PATCH v6 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
` (7 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater
We will use the Error object to improve error reporting in the
.log_global*() handlers of VFIO. Add documentation while at it.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v5:
- Fixed typo in set_dirty_page_tracking documentation
include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
hw/vfio/common.c | 4 ++--
hw/vfio/container-base.c | 4 ++--
hw/vfio/container.c | 6 +++---
4 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
MemoryRegionSection *section);
int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
- bool start);
+ bool start, Error **errp);
int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
VFIOBitmap *vbmap,
hwaddr iova, hwaddr size);
@@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
int (*attach_device)(const char *name, VFIODevice *vbasedev,
AddressSpace *as, Error **errp);
void (*detach_device)(VFIODevice *vbasedev);
+
/* migration feature */
+
+ /**
+ * @set_dirty_page_tracking
+ *
+ * Start or stop dirty pages tracking on VFIO container
+ *
+ * @bcontainer: #VFIOContainerBase on which to de/activate dirty
+ * page tracking
+ * @start: indicates whether to start or stop dirty pages tracking
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Returns zero to indicate success and negative for error
+ */
int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
- bool start);
+ bool start, Error **errp);
int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
VFIOBitmap *vbmap,
hwaddr iova, hwaddr size);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
ret = vfio_devices_dma_logging_start(bcontainer);
} else {
- ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
+ ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
}
if (ret) {
@@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
vfio_devices_dma_logging_stop(bcontainer);
} else {
- ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
+ ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
}
if (ret) {
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
}
int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
- bool start)
+ bool start, Error **errp)
{
if (!bcontainer->dirty_pages_supported) {
return 0;
}
g_assert(bcontainer->ops->set_dirty_page_tracking);
- return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
+ return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
}
int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
static int
vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
- bool start)
+ bool start, Error **errp)
{
const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
bcontainer);
@@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
if (ret) {
ret = -errno;
- error_report("Failed to set dirty tracking flag 0x%x errno: %d",
- dirty.flags, errno);
+ error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x",
+ dirty.flags);
}
return ret;
--
2.45.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v6 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start()
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
@ 2024-05-14 15:31 ` Cédric Le Goater
2024-05-15 6:53 ` Eric Auger
2024-05-14 15:31 ` [PATCH v6 3/9] migration: Extend migration_file_set_error() with Error* argument Cédric Le Goater
` (6 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater
This allows to update the Error argument of the VFIO log_global_start()
handler. Errors for container based logging will also be propagated to
qemu_savevm_state_setup() when the ram save_setup() handler is executed.
The vfio_set_migration_error() call becomes redundant in
vfio_listener_log_global_start(). Remove it.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v6:
- Commit log improvements (Avihai)
Changes in v5:
- Used error_setg_errno() in vfio_devices_dma_logging_start()
hw/vfio/common.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 485e53916491f1164d29e739fb7106c0c77df737..b5102f54a6474a50c6366e8fbce23812d55e384e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1027,7 +1027,8 @@ static void vfio_device_feature_dma_logging_start_destroy(
g_free(feature);
}
-static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
+static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
+ Error **errp)
{
struct vfio_device_feature *feature;
VFIODirtyRanges ranges;
@@ -1038,6 +1039,7 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
feature = vfio_device_feature_dma_logging_start_create(bcontainer,
&ranges);
if (!feature) {
+ error_setg_errno(errp, errno, "Failed to prepare DMA logging");
return -errno;
}
@@ -1049,8 +1051,8 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
if (ret) {
ret = -errno;
- error_report("%s: Failed to start DMA logging, err %d (%s)",
- vbasedev->name, ret, strerror(errno));
+ error_setg_errno(errp, errno, "%s: Failed to start DMA logging",
+ vbasedev->name);
goto out;
}
vbasedev->dirty_tracking = true;
@@ -1069,20 +1071,19 @@ out:
static bool vfio_listener_log_global_start(MemoryListener *listener,
Error **errp)
{
+ ERRP_GUARD();
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
int ret;
if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
- ret = vfio_devices_dma_logging_start(bcontainer);
+ ret = vfio_devices_dma_logging_start(bcontainer, errp);
} else {
- ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
+ ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
}
if (ret) {
- error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
- ret, strerror(-ret));
- vfio_set_migration_error(ret);
+ error_prepend(errp, "vfio: Could not start dirty page tracking - ");
}
return !ret;
}
@@ -1091,17 +1092,20 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
{
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
+ Error *local_err = NULL;
int ret = 0;
if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
vfio_devices_dma_logging_stop(bcontainer);
} else {
- ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
+ ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
+ &local_err);
}
if (ret) {
- error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
- ret, strerror(-ret));
+ error_prepend(&local_err,
+ "vfio: Could not stop dirty page tracking - ");
+ error_report_err(local_err);
vfio_set_migration_error(ret);
}
}
--
2.45.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v6 3/9] migration: Extend migration_file_set_error() with Error* argument
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
@ 2024-05-14 15:31 ` Cédric Le Goater
2024-05-15 7:04 ` Eric Auger
2024-05-14 15:31 ` [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state() Cédric Le Goater
` (5 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater
Use it to update the current error of the migration stream if
available and if not, simply print out the error. Next changes will
update with an error to report.
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Acked-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v6:
- Commit log improvements (Avihai)
include/migration/misc.h | 2 +-
hw/vfio/common.c | 2 +-
hw/vfio/migration.c | 4 ++--
migration/migration.c | 6 ++++--
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/include/migration/misc.h b/include/migration/misc.h
index bf7339cc1e6430226127fb6a878d06b458170858..bfadc5613bac614a316e5aed7da95d8c7845cf42 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -97,7 +97,7 @@ void migration_add_notifier_mode(NotifierWithReturn *notify,
void migration_remove_notifier(NotifierWithReturn *notify);
bool migration_is_running(void);
-void migration_file_set_error(int err);
+void migration_file_set_error(int ret, Error *err);
/* True if incoming migration entered POSTCOPY_INCOMING_DISCARD */
bool migration_in_incoming_postcopy(void);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index b5102f54a6474a50c6366e8fbce23812d55e384e..ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -150,7 +150,7 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
static void vfio_set_migration_error(int err)
{
if (migration_is_setup_or_active()) {
- migration_file_set_error(err);
+ migration_file_set_error(err, NULL);
}
}
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 06ae40969b6c19037e190008e14f28be646278cd..bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -726,7 +726,7 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
* Migration should be aborted in this case, but vm_state_notify()
* currently does not support reporting failures.
*/
- migration_file_set_error(ret);
+ migration_file_set_error(ret, NULL);
}
trace_vfio_vmstate_change_prepare(vbasedev->name, running,
@@ -756,7 +756,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
* Migration should be aborted in this case, but vm_state_notify()
* currently does not support reporting failures.
*/
- migration_file_set_error(ret);
+ migration_file_set_error(ret, NULL);
}
trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
diff --git a/migration/migration.c b/migration/migration.c
index e88b24f1e6cbe82dad3f890c00e264d2ab6ad355..70d66a441bf04761decf91dbe57ce52c57fde58f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2994,13 +2994,15 @@ static MigThrError postcopy_pause(MigrationState *s)
}
}
-void migration_file_set_error(int err)
+void migration_file_set_error(int ret, Error *err)
{
MigrationState *s = current_migration;
WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
if (s->to_dst_file) {
- qemu_file_set_error(s->to_dst_file, err);
+ qemu_file_set_error_obj(s->to_dst_file, ret, err);
+ } else if (err) {
+ error_report_err(err);
}
}
}
--
2.45.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state()
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
` (2 preceding siblings ...)
2024-05-14 15:31 ` [PATCH v6 3/9] migration: Extend migration_file_set_error() with Error* argument Cédric Le Goater
@ 2024-05-14 15:31 ` Cédric Le Goater
2024-05-15 7:20 ` Eric Auger
2024-05-16 8:18 ` Avihai Horon
2024-05-14 15:31 ` [PATCH v6 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
` (4 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater
Add an Error** argument to vfio_migration_set_state() and adjust
callers, including vfio_save_setup(). The error will be propagated up
to qemu_savevm_state_setup() where the save_setup() handler is
executed.
Modify vfio_vmstate_change_prepare() and vfio_vmstate_change() to
store a reported error under the migration stream if a migration is in
progress.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v6:
- Commit log improvements (Avihai)
- vfio_migration_set_state() : Dropped the error_setg_errno() change
when setting device in recover state fails (Avihai)
- vfio_migration_state_notifier() : report local error (Avihai)
Changes in v5:
- Replaced error_setg() by error_setg_errno() in vfio_migration_set_state()
- Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
hw/vfio/migration.c | 77 ++++++++++++++++++++++++++++-----------------
1 file changed, 48 insertions(+), 29 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..bf11135167ebb162dd41415656bdacfa0e1ca550 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -82,7 +82,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
static int vfio_migration_set_state(VFIODevice *vbasedev,
enum vfio_device_mig_state new_state,
- enum vfio_device_mig_state recover_state)
+ enum vfio_device_mig_state recover_state,
+ Error **errp)
{
VFIOMigration *migration = vbasedev->migration;
uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
@@ -102,18 +103,19 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
ret = -errno;
if (recover_state == VFIO_DEVICE_STATE_ERROR) {
- error_report("%s: Failed setting device state to %s, err: %s. "
- "Recover state is ERROR. Resetting device",
- vbasedev->name, mig_state_to_str(new_state),
- strerror(errno));
+ error_setg_errno(errp, errno,
+ "%s: Failed setting device state to %s. "
+ "Recover state is ERROR. Resetting device",
+ vbasedev->name, mig_state_to_str(new_state));
goto reset_device;
}
- error_report(
- "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s",
- vbasedev->name, mig_state_to_str(new_state),
- strerror(errno), mig_state_to_str(recover_state));
+ error_setg_errno(errp, errno,
+ "%s: Failed setting device state to %s. "
+ "Setting device in recover state %s",
+ vbasedev->name, mig_state_to_str(new_state),
+ mig_state_to_str(recover_state));
mig_state->device_state = recover_state;
if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
@@ -137,7 +139,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
* This can happen if the device is asynchronously reset and
* terminates a data transfer.
*/
- error_report("%s: data_fd out of sync", vbasedev->name);
+ error_setg(errp, "%s: data_fd out of sync", vbasedev->name);
close(mig_state->data_fd);
return -EBADF;
@@ -168,10 +170,11 @@ reset_device:
*/
static int
vfio_migration_set_state_or_reset(VFIODevice *vbasedev,
- enum vfio_device_mig_state new_state)
+ enum vfio_device_mig_state new_state,
+ Error **errp)
{
return vfio_migration_set_state(vbasedev, new_state,
- VFIO_DEVICE_STATE_ERROR);
+ VFIO_DEVICE_STATE_ERROR, errp);
}
static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
@@ -399,10 +402,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
switch (migration->device_state) {
case VFIO_DEVICE_STATE_RUNNING:
ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
- VFIO_DEVICE_STATE_RUNNING);
+ VFIO_DEVICE_STATE_RUNNING, errp);
if (ret) {
- error_setg(errp, "%s: Failed to set new PRE_COPY state",
- vbasedev->name);
return ret;
}
@@ -435,13 +436,20 @@ static void vfio_save_cleanup(void *opaque)
{
VFIODevice *vbasedev = opaque;
VFIOMigration *migration = vbasedev->migration;
+ Error *local_err = NULL;
+ int ret;
/*
* Changing device state from STOP_COPY to STOP can take time. Do it here,
* after migration has completed, so it won't increase downtime.
*/
if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) {
- vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP);
+ ret = vfio_migration_set_state_or_reset(vbasedev,
+ VFIO_DEVICE_STATE_STOP,
+ &local_err);
+ if (ret) {
+ error_report_err(local_err);
+ }
}
g_free(migration->data_buffer);
@@ -549,11 +557,13 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
VFIODevice *vbasedev = opaque;
ssize_t data_size;
int ret;
+ Error *local_err = NULL;
/* We reach here with device state STOP or STOP_COPY only */
ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
- VFIO_DEVICE_STATE_STOP);
+ VFIO_DEVICE_STATE_STOP, &local_err);
if (ret) {
+ error_report_err(local_err);
return ret;
}
@@ -591,14 +601,9 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
{
VFIODevice *vbasedev = opaque;
- int ret;
- ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
- vbasedev->migration->device_state);
- if (ret) {
- error_setg(errp, "%s: Failed to set RESUMING state", vbasedev->name);
- }
- return ret;
+ return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
+ vbasedev->migration->device_state, errp);
}
static int vfio_load_cleanup(void *opaque)
@@ -714,19 +719,20 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
VFIODevice *vbasedev = opaque;
VFIOMigration *migration = vbasedev->migration;
enum vfio_device_mig_state new_state;
+ Error *local_err = NULL;
int ret;
new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
VFIO_DEVICE_STATE_PRE_COPY_P2P :
VFIO_DEVICE_STATE_RUNNING_P2P;
- ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
+ ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
if (ret) {
/*
* Migration should be aborted in this case, but vm_state_notify()
* currently does not support reporting failures.
*/
- migration_file_set_error(ret, NULL);
+ migration_file_set_error(ret, local_err);
}
trace_vfio_vmstate_change_prepare(vbasedev->name, running,
@@ -738,6 +744,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
{
VFIODevice *vbasedev = opaque;
enum vfio_device_mig_state new_state;
+ Error *local_err = NULL;
int ret;
if (running) {
@@ -750,13 +757,13 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
VFIO_DEVICE_STATE_STOP;
}
- ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
+ ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
if (ret) {
/*
* Migration should be aborted in this case, but vm_state_notify()
* currently does not support reporting failures.
*/
- migration_file_set_error(ret, NULL);
+ migration_file_set_error(ret, local_err);
}
trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
@@ -769,11 +776,23 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
VFIOMigration *migration = container_of(notifier, VFIOMigration,
migration_state);
VFIODevice *vbasedev = migration->vbasedev;
+ Error *local_err = NULL;
+ int ret = 0;
trace_vfio_migration_state_notifier(vbasedev->name, e->type);
if (e->type == MIG_EVENT_PRECOPY_FAILED) {
- vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
+ /*
+ * MigrationNotifyFunc may return an error code and an Error
+ * object only for MIG_EVENT_PRECOPY_SETUP. Hence, report the
+ * error locally and ignore the errp argument.
+ */
+ ret = vfio_migration_set_state_or_reset(vbasedev,
+ VFIO_DEVICE_STATE_RUNNING,
+ &local_err);
+ if (ret) {
+ error_report_err(local_err);
+ }
}
return 0;
}
--
2.45.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v6 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
` (3 preceding siblings ...)
2024-05-14 15:31 ` [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state() Cédric Le Goater
@ 2024-05-14 15:31 ` Cédric Le Goater
2024-05-15 9:20 ` Eric Auger
2024-05-16 8:22 ` Avihai Horon
2024-05-14 15:31 ` [PATCH v6 6/9] vfio: Reverse test on vfio_get_xlat_addr() Cédric Le Goater
` (3 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater
Use vmstate_save_state_with_err() to improve error reporting in the
callers and store a reported error under the migration stream. Add
documentation while at it.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v6:
- Modified title (Avihai)
- vfio_save_device_config_state() : Set errp if the migration stream
is in error (Avihai)
- vfio_save_state() : Changed error prefix (Avihai)
include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++-
hw/vfio/migration.c | 25 ++++++++++++++++++-------
hw/vfio/pci.c | 5 +++--
3 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b9da6c08ef41174610eb92726c590309a53696a3..46f88493634b5634a9c14a5caa33a463fbf2c50d 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -133,7 +133,30 @@ struct VFIODeviceOps {
int (*vfio_hot_reset_multi)(VFIODevice *vdev);
void (*vfio_eoi)(VFIODevice *vdev);
Object *(*vfio_get_object)(VFIODevice *vdev);
- void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
+
+ /**
+ * @vfio_save_config
+ *
+ * Save device config state
+ *
+ * @vdev: #VFIODevice for which to save the config
+ * @f: #QEMUFile where to send the data
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Returns zero to indicate success and negative for error
+ */
+ int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);
+
+ /**
+ * @vfio_load_config
+ *
+ * Load device config state
+ *
+ * @vdev: #VFIODevice for which to load the config
+ * @f: #QEMUFile where to get the data
+ *
+ * Returns zero to indicate success and negative for error
+ */
int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
};
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index bf11135167ebb162dd41415656bdacfa0e1ca550..d089fa9b937e725307c1a56755495d5b8fae2065 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -189,21 +189,30 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
return ret;
}
-static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
+static int vfio_save_device_config_state(QEMUFile *f, void *opaque,
+ Error **errp)
{
VFIODevice *vbasedev = opaque;
+ int ret;
qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
- vbasedev->ops->vfio_save_config(vbasedev, f);
+ ret = vbasedev->ops->vfio_save_config(vbasedev, f, errp);
+ if (ret) {
+ return ret;
+ }
}
qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
trace_vfio_save_device_config_state(vbasedev->name);
- return qemu_file_get_error(f);
+ ret = qemu_file_get_error(f);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to save state");
+ }
+ return ret;
}
static int vfio_load_device_config_state(QEMUFile *f, void *opaque)
@@ -588,13 +597,15 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
static void vfio_save_state(QEMUFile *f, void *opaque)
{
VFIODevice *vbasedev = opaque;
+ Error *local_err = NULL;
int ret;
- ret = vfio_save_device_config_state(f, opaque);
+ ret = vfio_save_device_config_state(f, opaque, &local_err);
if (ret) {
- error_report("%s: Failed to save device config space",
- vbasedev->name);
- qemu_file_set_error(f, ret);
+ error_prepend(&local_err,
+ "vfio: Failed to save device config space of %s - ",
+ vbasedev->name);
+ qemu_file_set_error_obj(f, ret, local_err);
}
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 64780d1b793345c8e8996fe6b7987059ce831c11..fc6e54e871508bb0e2a3ac9079a195c086531f21 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2586,11 +2586,12 @@ static const VMStateDescription vmstate_vfio_pci_config = {
}
};
-static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
+static int vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f, Error **errp)
{
VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
- vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL);
+ return vmstate_save_state_with_err(f, &vmstate_vfio_pci_config, vdev, NULL,
+ errp);
}
static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
--
2.45.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v6 6/9] vfio: Reverse test on vfio_get_xlat_addr()
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
` (4 preceding siblings ...)
2024-05-14 15:31 ` [PATCH v6 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
@ 2024-05-14 15:31 ` Cédric Le Goater
2024-05-15 9:22 ` Eric Auger
2024-05-14 15:31 ` [PATCH v6 7/9] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
` (2 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater
It will simplify the changes coming after.
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v6:
- Modified title (Avihai)
- vfio_iommu_map_dirty_notify() : Modified goto label (Avihai)
hw/vfio/common.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c..4e2ef3d3034e72aa6a546bcb9ea1f31a0bbd5b1b 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1224,16 +1224,20 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
}
rcu_read_lock();
- if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
- ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
- translated_addr);
- if (ret) {
- error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
- "0x%"HWADDR_PRIx") = %d (%s)",
- bcontainer, iova, iotlb->addr_mask + 1, ret,
- strerror(-ret));
- }
+ if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
+ goto out_unlock;
}
+
+ ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
+ translated_addr);
+ if (ret) {
+ error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
+ "0x%"HWADDR_PRIx") = %d (%s)",
+ bcontainer, iova, iotlb->addr_mask + 1, ret,
+ strerror(-ret));
+ }
+
+out_unlock:
rcu_read_unlock();
out:
--
2.45.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v6 7/9] memory: Add Error** argument to memory_get_xlat_addr()
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
` (5 preceding siblings ...)
2024-05-14 15:31 ` [PATCH v6 6/9] vfio: Reverse test on vfio_get_xlat_addr() Cédric Le Goater
@ 2024-05-14 15:31 ` Cédric Le Goater
2024-05-15 9:25 ` Eric Auger
2024-05-16 8:25 ` Avihai Horon
2024-05-14 15:31 ` [PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 9/9] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
8 siblings, 2 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater, Michael S . Tsirkin,
Paolo Bonzini, David Hildenbrand
Let the callers do the reporting. This will be useful in
vfio_iommu_map_dirty_notify().
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v6:
- Fixed memory_get_xlat_addr documentation (Avihai)
include/exec/memory.h | 15 ++++++++++++++-
hw/vfio/common.c | 13 +++++++++----
hw/virtio/vhost-vdpa.c | 5 ++++-
system/memory.c | 10 +++++-----
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index d417d7f363dbbca6553c449582a93d5da73cca40..9cdd64e9c69b63f9d27cebc2e8cb366e22ed7577 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -774,9 +774,22 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
RamDiscardListener *rdl);
+/**
+ * memory_get_xlat_addr: Extract addresses from a TLB entry
+ *
+ * @iotlb: pointer to an #IOMMUTLBEntry
+ * @vaddr: virtual address
+ * @ram_addr: RAM address
+ * @read_only: indicates if writes are allowed
+ * @mr_has_discard_manager: indicates memory is controlled by a
+ * RamDiscardManager
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
+ */
bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
ram_addr_t *ram_addr, bool *read_only,
- bool *mr_has_discard_manager);
+ bool *mr_has_discard_manager, Error **errp);
typedef struct CoalescedMemoryRange CoalescedMemoryRange;
typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 4e2ef3d3034e72aa6a546bcb9ea1f31a0bbd5b1b..919c4c52bc1590fd6c0bda37ba5881f58ff2ffff 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -253,12 +253,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
/* Called with rcu_read_lock held. */
static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
- ram_addr_t *ram_addr, bool *read_only)
+ ram_addr_t *ram_addr, bool *read_only,
+ Error **errp)
{
bool ret, mr_has_discard_manager;
ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
- &mr_has_discard_manager);
+ &mr_has_discard_manager, errp);
if (ret && mr_has_discard_manager) {
/*
* Malicious VMs might trigger discarding of IOMMU-mapped memory. The
@@ -288,6 +289,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
hwaddr iova = iotlb->iova + giommu->iommu_offset;
void *vaddr;
int ret;
+ Error *local_err = NULL;
trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
iova, iova + iotlb->addr_mask);
@@ -304,7 +306,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
bool read_only;
- if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
+ if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, &local_err)) {
+ error_report_err(local_err);
goto out;
}
/*
@@ -1213,6 +1216,7 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
VFIOContainerBase *bcontainer = giommu->bcontainer;
hwaddr iova = iotlb->iova + giommu->iommu_offset;
ram_addr_t translated_addr;
+ Error *local_err = NULL;
int ret = -EINVAL;
trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
@@ -1224,7 +1228,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
}
rcu_read_lock();
- if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
+ if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
+ error_report_err(local_err);
goto out_unlock;
}
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index e827b9175fc61f1ef419e48d90a440b00449312a..ed99ab87457d8f31b98ace960713f48d47b27102 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -208,6 +208,7 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
void *vaddr;
int ret;
Int128 llend;
+ Error *local_err = NULL;
if (iotlb->target_as != &address_space_memory) {
error_report("Wrong target AS \"%s\", only system memory is allowed",
@@ -227,7 +228,9 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
bool read_only;
- if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL)) {
+ if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL,
+ &local_err)) {
+ error_report_err(local_err);
return;
}
ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova,
diff --git a/system/memory.c b/system/memory.c
index 642a449f8c867d38c62a748a4dfd5c055637c205..9540caa8a1f4da8501bf5ae9d7eedde8b775e1dc 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2179,7 +2179,7 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
/* Called with rcu_read_lock held. */
bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
ram_addr_t *ram_addr, bool *read_only,
- bool *mr_has_discard_manager)
+ bool *mr_has_discard_manager, Error **errp)
{
MemoryRegion *mr;
hwaddr xlat;
@@ -2197,7 +2197,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
mr = address_space_translate(&address_space_memory, iotlb->translated_addr,
&xlat, &len, writable, MEMTXATTRS_UNSPECIFIED);
if (!memory_region_is_ram(mr)) {
- error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
+ error_setg(errp, "iommu map to non memory area %" HWADDR_PRIx "", xlat);
return false;
} else if (memory_region_has_ram_discard_manager(mr)) {
RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
@@ -2216,8 +2216,8 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
* were already restored before IOMMUs are restored.
*/
if (!ram_discard_manager_is_populated(rdm, &tmp)) {
- error_report("iommu map to discarded memory (e.g., unplugged via"
- " virtio-mem): %" HWADDR_PRIx "",
+ error_setg(errp, "iommu map to discarded memory (e.g., unplugged"
+ " via virtio-mem): %" HWADDR_PRIx "",
iotlb->translated_addr);
return false;
}
@@ -2228,7 +2228,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
* check that it did not truncate too much.
*/
if (len & iotlb->addr_mask) {
- error_report("iommu has granularity incompatible with target AS");
+ error_setg(errp, "iommu has granularity incompatible with target AS");
return false;
}
--
2.45.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
` (6 preceding siblings ...)
2024-05-14 15:31 ` [PATCH v6 7/9] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
@ 2024-05-14 15:31 ` Cédric Le Goater
2024-05-15 9:34 ` Eric Auger
2024-05-16 8:42 ` Avihai Horon
2024-05-14 15:31 ` [PATCH v6 9/9] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
8 siblings, 2 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater
Let the callers do the error reporting. Add documentation while at it.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v6:
- Fixed the line wrapping (Avihai)
- Fixed query_dirty_bitmap documentation (Avihai)
Changes in v5:
- Replaced error_setg() by error_setg_errno() in
vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap()
- ':' -> '-' in vfio_iommu_map_dirty_notify()
include/hw/vfio/vfio-common.h | 4 +-
include/hw/vfio/vfio-container-base.h | 21 +++++++--
hw/vfio/common.c | 61 ++++++++++++++++++---------
hw/vfio/container-base.c | 7 +--
hw/vfio/container.c | 14 +++---
5 files changed, 72 insertions(+), 35 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 46f88493634b5634a9c14a5caa33a463fbf2c50d..68911d36676667352e94a97895828aff4b194b57 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -274,9 +274,9 @@ bool
vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer);
int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
VFIOBitmap *vbmap, hwaddr iova,
- hwaddr size);
+ hwaddr size, Error **errp);
int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
- uint64_t size, ram_addr_t ram_addr);
+ uint64_t size, ram_addr_t ram_addr, Error **errp);
/* Returns 0 on success, or a negative errno. */
int vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index 326ceea52a2030eec9dad289a9845866c4a8c090..42cfbf32dc737606c3f620d126f35d85d4833534 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -84,8 +84,8 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
bool start, Error **errp);
int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
- VFIOBitmap *vbmap,
- hwaddr iova, hwaddr size);
+ VFIOBitmap *vbmap, hwaddr iova,
+ hwaddr size, Error **errp);
void vfio_container_init(VFIOContainerBase *bcontainer,
VFIOAddressSpace *space,
@@ -138,9 +138,22 @@ struct VFIOIOMMUClass {
*/
int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
bool start, Error **errp);
+ /**
+ * @query_dirty_bitmap
+ *
+ * Get bitmap of dirty pages from container
+ *
+ * @bcontainer: #VFIOContainerBase from which to get dirty pages
+ * @vbmap: #VFIOBitmap internal bitmap structure
+ * @iova: iova base address
+ * @size: size of iova range
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Returns zero to indicate success and negative for error
+ */
int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
- VFIOBitmap *vbmap,
- hwaddr iova, hwaddr size);
+ VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
+ Error **errp);
/* PCI specific */
int (*pci_hot_reset)(VFIODevice *vbasedev, bool single);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 919c4c52bc1590fd6c0bda37ba5881f58ff2ffff..9b5123d45fc1f9d5be4bbbf92898f89cd11e1363 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1140,8 +1140,8 @@ static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova,
}
int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
- VFIOBitmap *vbmap, hwaddr iova,
- hwaddr size)
+ VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
+ Error **errp)
{
VFIODevice *vbasedev;
int ret;
@@ -1150,10 +1150,10 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
ret = vfio_device_dma_logging_report(vbasedev, iova, size,
vbmap->bitmap);
if (ret) {
- error_report("%s: Failed to get DMA logging report, iova: "
- "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx
- ", err: %d (%s)",
- vbasedev->name, iova, size, ret, strerror(-ret));
+ error_setg_errno(errp, -ret,
+ "%s: Failed to get DMA logging report, iova: "
+ "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx,
+ vbasedev->name, iova, size);
return ret;
}
@@ -1163,7 +1163,7 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
}
int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
- uint64_t size, ram_addr_t ram_addr)
+ uint64_t size, ram_addr_t ram_addr, Error **errp)
{
bool all_device_dirty_tracking =
vfio_devices_all_device_dirty_tracking(bcontainer);
@@ -1180,13 +1180,17 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
ret = vfio_bitmap_alloc(&vbmap, size);
if (ret) {
+ error_setg_errno(errp, -ret,
+ "Failed to allocate dirty tracking bitmap");
return ret;
}
if (all_device_dirty_tracking) {
- ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size);
+ ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size,
+ errp);
} else {
- ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size);
+ ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size,
+ errp);
}
if (ret) {
@@ -1234,12 +1238,13 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
}
ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
- translated_addr);
+ translated_addr, &local_err);
if (ret) {
- error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
- "0x%"HWADDR_PRIx") = %d (%s)",
- bcontainer, iova, iotlb->addr_mask + 1, ret,
- strerror(-ret));
+ error_prepend(&local_err,
+ "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
+ "0x%"HWADDR_PRIx") failed - ", bcontainer, iova,
+ iotlb->addr_mask + 1);
+ error_report_err(local_err);
}
out_unlock:
@@ -1259,12 +1264,19 @@ static int vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section,
const ram_addr_t ram_addr = memory_region_get_ram_addr(section->mr) +
section->offset_within_region;
VFIORamDiscardListener *vrdl = opaque;
+ Error *local_err = NULL;
+ int ret;
/*
* Sync the whole mapped region (spanning multiple individual mappings)
* in one go.
*/
- return vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr);
+ ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr,
+ &local_err);
+ if (ret) {
+ error_report_err(local_err);
+ }
+ return ret;
}
static int
@@ -1296,7 +1308,7 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
}
static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
- MemoryRegionSection *section)
+ MemoryRegionSection *section, Error **errp)
{
ram_addr_t ram_addr;
@@ -1327,7 +1339,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
}
return 0;
} else if (memory_region_has_ram_discard_manager(section->mr)) {
- return vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section);
+ int ret;
+
+ ret = vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section);
+ if (ret) {
+ error_setg(errp,
+ "Failed to sync dirty bitmap with RAM discard listener");
+ return ret;
+ }
}
ram_addr = memory_region_get_ram_addr(section->mr) +
@@ -1335,7 +1354,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
return vfio_get_dirty_bitmap(bcontainer,
REAL_HOST_PAGE_ALIGN(section->offset_within_address_space),
- int128_get64(section->size), ram_addr);
+ int128_get64(section->size), ram_addr, errp);
}
static void vfio_listener_log_sync(MemoryListener *listener,
@@ -1344,16 +1363,16 @@ static void vfio_listener_log_sync(MemoryListener *listener,
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
int ret;
+ Error *local_err = NULL;
if (vfio_listener_skipped_section(section)) {
return;
}
if (vfio_devices_all_dirty_tracking(bcontainer)) {
- ret = vfio_sync_dirty_bitmap(bcontainer, section);
+ ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
if (ret) {
- error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
- strerror(-ret));
+ error_report_err(local_err);
vfio_set_migration_error(ret);
}
}
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 7c0764121d24b02b6c4e66e368d7dff78a6d65aa..4fa250c46fd16333d2e2358ede8b0a0afdb185b3 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -64,11 +64,12 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
}
int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
- VFIOBitmap *vbmap,
- hwaddr iova, hwaddr size)
+ VFIOBitmap *vbmap, hwaddr iova,
+ hwaddr size, Error **errp)
{
g_assert(bcontainer->ops->query_dirty_bitmap);
- return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size);
+ return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size,
+ errp);
}
void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space,
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index c35221fbe7dc5453050f97cd186fc958e24f28f7..e00db6546c652c61a352f8e4cd65b212ecfb65bc 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -130,6 +130,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
};
bool need_dirty_sync = false;
int ret;
+ Error *local_err = NULL;
if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) {
if (!vfio_devices_all_device_dirty_tracking(bcontainer) &&
@@ -165,8 +166,9 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
if (need_dirty_sync) {
ret = vfio_get_dirty_bitmap(bcontainer, iova, size,
- iotlb->translated_addr);
+ iotlb->translated_addr, &local_err);
if (ret) {
+ error_report_err(local_err);
return ret;
}
}
@@ -236,7 +238,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
VFIOBitmap *vbmap,
- hwaddr iova, hwaddr size)
+ hwaddr iova, hwaddr size,
+ Error **errp)
{
const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
bcontainer);
@@ -264,9 +267,10 @@ static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap);
if (ret) {
ret = -errno;
- error_report("Failed to get dirty bitmap for iova: 0x%"PRIx64
- " size: 0x%"PRIx64" err: %d", (uint64_t)range->iova,
- (uint64_t)range->size, errno);
+ error_setg_errno(errp, errno,
+ "Failed to get dirty bitmap for iova: 0x%"PRIx64
+ " size: 0x%"PRIx64, (uint64_t)range->iova,
+ (uint64_t)range->size);
}
g_free(dbitmap);
--
2.45.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v6 9/9] vfio: Also trace event failures in vfio_save_complete_precopy()
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
` (7 preceding siblings ...)
2024-05-14 15:31 ` [PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
@ 2024-05-14 15:31 ` Cédric Le Goater
2024-05-15 9:34 ` Eric Auger
8 siblings, 1 reply; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Cédric Le Goater
vfio_save_complete_precopy() currently returns before doing the trace
event. Change that.
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/migration.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index d089fa9b937e725307c1a56755495d5b8fae2065..b06d887df53155e676bf13fa03adaf0627841994 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -585,9 +585,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
ret = qemu_file_get_error(f);
- if (ret) {
- return ret;
- }
trace_vfio_save_complete_precopy(vbasedev->name, ret);
--
2.45.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-14 15:31 ` [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
@ 2024-05-15 6:40 ` Eric Auger
2024-05-15 16:55 ` Cédric Le Goater
0 siblings, 1 reply; 29+ messages in thread
From: Eric Auger @ 2024-05-15 6:40 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
Hi Cédric,
On 5/14/24 17:31, Cédric Le Goater wrote:
> We will use the Error object to improve error reporting in the
> .log_global*() handlers of VFIO. Add documentation while at it.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> Changes in v5:
>
> - Fixed typo in set_dirty_page_tracking documentation
>
> include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
> hw/vfio/common.c | 4 ++--
> hw/vfio/container-base.c | 4 ++--
> hw/vfio/container.c | 6 +++---
> 4 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
> void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
> MemoryRegionSection *section);
> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> - bool start);
> + bool start, Error **errp);
I am a bit confused now wrt [PATCH v2 03/11] vfio: Make
VFIOIOMMUClass::attach_device() and its wrapper return bool & co
Shall we return a bool or a int?
Looking at ./include/qapi/error.h I have not found any stringent requirement
* - Whenever practical, also return a value that indicates success /
* failure. This can make the error checking more concise, and can
* avoid useless error object creation and destruction. Note that
* we still have many functions returning void. We recommend
* • bool-valued functions return true on success / false on failure,
* • pointer-valued functions return non-null / null pointer, and
* • integer-valued functions return non-negative / negative.
> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> VFIOBitmap *vbmap,
> hwaddr iova, hwaddr size);
> @@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
> int (*attach_device)(const char *name, VFIODevice *vbasedev,
> AddressSpace *as, Error **errp);
> void (*detach_device)(VFIODevice *vbasedev);
> +
> /* migration feature */
> +
> + /**
> + * @set_dirty_page_tracking
> + *
> + * Start or stop dirty pages tracking on VFIO container
> + *
> + * @bcontainer: #VFIOContainerBase on which to de/activate dirty
> + * page tracking
> + * @start: indicates whether to start or stop dirty pages tracking
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Returns zero to indicate success and negative for error
> + */
> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
> - bool start);
> + bool start, Error **errp);
> int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
> VFIOBitmap *vbmap,
> hwaddr iova, hwaddr size);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
> if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
> ret = vfio_devices_dma_logging_start(bcontainer);
> } else {
> - ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
> + ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
> }
>
> if (ret) {
> @@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
> if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
> vfio_devices_dma_logging_stop(bcontainer);
> } else {
> - ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
> + ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
> }
>
> if (ret) {
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
> }
>
> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> - bool start)
> + bool start, Error **errp)
> {
> if (!bcontainer->dirty_pages_supported) {
> return 0;
> }
>
> g_assert(bcontainer->ops->set_dirty_page_tracking);
> - return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
> + return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
> }
>
> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>
> static int
> vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
> - bool start)
> + bool start, Error **errp)
> {
> const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
> bcontainer);
> @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
> ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
> if (ret) {
> ret = -errno;
> - error_report("Failed to set dirty tracking flag 0x%x errno: %d",
> - dirty.flags, errno);
> + error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x",
> + dirty.flags);
> }
>
> return ret;
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start()
2024-05-14 15:31 ` [PATCH v6 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
@ 2024-05-15 6:53 ` Eric Auger
2024-05-15 17:09 ` Cédric Le Goater
0 siblings, 1 reply; 29+ messages in thread
From: Eric Auger @ 2024-05-15 6:53 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
Hi Cédric,
On 5/14/24 17:31, Cédric Le Goater wrote:
> This allows to update the Error argument of the VFIO log_global_start()
> handler. Errors for container based logging will also be propagated to
> qemu_savevm_state_setup() when the ram save_setup() handler is executed.
nit: also now collect & print errors from
vfio_container_set_dirty_page_tracking()
>
> The vfio_set_migration_error() call becomes redundant in
> vfio_listener_log_global_start(). Remove it.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v6:
>
> - Commit log improvements (Avihai)
>
> Changes in v5:
>
> - Used error_setg_errno() in vfio_devices_dma_logging_start()
>
> hw/vfio/common.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 485e53916491f1164d29e739fb7106c0c77df737..b5102f54a6474a50c6366e8fbce23812d55e384e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1027,7 +1027,8 @@ static void vfio_device_feature_dma_logging_start_destroy(
> g_free(feature);
> }
>
> -static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
> +static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
> + Error **errp)
> {
> struct vfio_device_feature *feature;
> VFIODirtyRanges ranges;
> @@ -1038,6 +1039,7 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
> feature = vfio_device_feature_dma_logging_start_create(bcontainer,
> &ranges);
> if (!feature) {
> + error_setg_errno(errp, errno, "Failed to prepare DMA logging");
> return -errno;
> }
>
> @@ -1049,8 +1051,8 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
> ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
> if (ret) {
> ret = -errno;
> - error_report("%s: Failed to start DMA logging, err %d (%s)",
> - vbasedev->name, ret, strerror(errno));
> + error_setg_errno(errp, errno, "%s: Failed to start DMA logging",
> + vbasedev->name);
> goto out;
> }
> vbasedev->dirty_tracking = true;
> @@ -1069,20 +1071,19 @@ out:
> static bool vfio_listener_log_global_start(MemoryListener *listener,
> Error **errp)
> {
> + ERRP_GUARD();
> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> listener);
> int ret;
>
> if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
> - ret = vfio_devices_dma_logging_start(bcontainer);
> + ret = vfio_devices_dma_logging_start(bcontainer, errp);
> } else {
> - ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
> + ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
> }
>
> if (ret) {
> - error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
> - ret, strerror(-ret));
> - vfio_set_migration_error(ret);
> + error_prepend(errp, "vfio: Could not start dirty page tracking - ");
> }
> return !ret;
> }
> @@ -1091,17 +1092,20 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
> {
> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> listener);
> + Error *local_err = NULL;
> int ret = 0;
>
> if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
> vfio_devices_dma_logging_stop(bcontainer);
> } else {
> - ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
> + ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
> + &local_err);
> }
>
> if (ret) {
> - error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
> - ret, strerror(-ret));
> + error_prepend(&local_err,
> + "vfio: Could not stop dirty page tracking - ");
> + error_report_err(local_err);
> vfio_set_migration_error(ret);
> }
> }
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 3/9] migration: Extend migration_file_set_error() with Error* argument
2024-05-14 15:31 ` [PATCH v6 3/9] migration: Extend migration_file_set_error() with Error* argument Cédric Le Goater
@ 2024-05-15 7:04 ` Eric Auger
2024-05-15 17:15 ` Cédric Le Goater
0 siblings, 1 reply; 29+ messages in thread
From: Eric Auger @ 2024-05-15 7:04 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
Hi Cédric,
On 5/14/24 17:31, Cédric Le Goater wrote:
> Use it to update the current error of the migration stream if
> available and if not, simply print out the error. Next changes will
> update with an error to report.
>
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> Acked-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v6:
>
> - Commit log improvements (Avihai)
>
> include/migration/misc.h | 2 +-
> hw/vfio/common.c | 2 +-
> hw/vfio/migration.c | 4 ++--
> migration/migration.c | 6 ++++--
> 4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index bf7339cc1e6430226127fb6a878d06b458170858..bfadc5613bac614a316e5aed7da95d8c7845cf42 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -97,7 +97,7 @@ void migration_add_notifier_mode(NotifierWithReturn *notify,
>
> void migration_remove_notifier(NotifierWithReturn *notify);
> bool migration_is_running(void);
> -void migration_file_set_error(int err);
> +void migration_file_set_error(int ret, Error *err);
>
> /* True if incoming migration entered POSTCOPY_INCOMING_DISCARD */
> bool migration_in_incoming_postcopy(void);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index b5102f54a6474a50c6366e8fbce23812d55e384e..ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -150,7 +150,7 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
> static void vfio_set_migration_error(int err)
nit: I would have renamed err into ret here to avoid any further confusion.
> {
> if (migration_is_setup_or_active()) {
> - migration_file_set_error(err);
> + migration_file_set_error(err, NULL);
> }
> }
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 06ae40969b6c19037e190008e14f28be646278cd..bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -726,7 +726,7 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
> * Migration should be aborted in this case, but vm_state_notify()
> * currently does not support reporting failures.
> */
> - migration_file_set_error(ret);
> + migration_file_set_error(ret, NULL);
> }
>
> trace_vfio_vmstate_change_prepare(vbasedev->name, running,
> @@ -756,7 +756,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> * Migration should be aborted in this case, but vm_state_notify()
> * currently does not support reporting failures.
> */
> - migration_file_set_error(ret);
> + migration_file_set_error(ret, NULL);
> }
>
> trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
> diff --git a/migration/migration.c b/migration/migration.c
> index e88b24f1e6cbe82dad3f890c00e264d2ab6ad355..70d66a441bf04761decf91dbe57ce52c57fde58f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2994,13 +2994,15 @@ static MigThrError postcopy_pause(MigrationState *s)
> }
> }
>
> -void migration_file_set_error(int err)
> +void migration_file_set_error(int ret, Error *err)
> {
> MigrationState *s = current_migration;
>
> WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
> if (s->to_dst_file) {
> - qemu_file_set_error(s->to_dst_file, err);
> + qemu_file_set_error_obj(s->to_dst_file, ret, err);
> + } else if (err) {
> + error_report_err(err);
> }
> }
> }
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state()
2024-05-14 15:31 ` [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state() Cédric Le Goater
@ 2024-05-15 7:20 ` Eric Auger
2024-05-16 7:17 ` Cédric Le Goater
2024-05-16 8:18 ` Avihai Horon
1 sibling, 1 reply; 29+ messages in thread
From: Eric Auger @ 2024-05-15 7:20 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
Hi Cédric,
On 5/14/24 17:31, Cédric Le Goater wrote:
> Add an Error** argument to vfio_migration_set_state() and adjust
> callers, including vfio_save_setup(). The error will be propagated up
> to qemu_savevm_state_setup() where the save_setup() handler is
> executed.
>
> Modify vfio_vmstate_change_prepare() and vfio_vmstate_change() to
> store a reported error under the migration stream if a migration is in
> progress.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v6:
>
> - Commit log improvements (Avihai)
> - vfio_migration_set_state() : Dropped the error_setg_errno() change
> when setting device in recover state fails (Avihai)
> - vfio_migration_state_notifier() : report local error (Avihai)
>
> Changes in v5:
>
> - Replaced error_setg() by error_setg_errno() in vfio_migration_set_state()
> - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>
> hw/vfio/migration.c | 77 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 48 insertions(+), 29 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..bf11135167ebb162dd41415656bdacfa0e1ca550 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -82,7 +82,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>
> static int vfio_migration_set_state(VFIODevice *vbasedev,
> enum vfio_device_mig_state new_state,
> - enum vfio_device_mig_state recover_state)
> + enum vfio_device_mig_state recover_state,
> + Error **errp)
> {
> VFIOMigration *migration = vbasedev->migration;
> uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
> @@ -102,18 +103,19 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
> ret = -errno;
>
> if (recover_state == VFIO_DEVICE_STATE_ERROR) {
> - error_report("%s: Failed setting device state to %s, err: %s. "
> - "Recover state is ERROR. Resetting device",
> - vbasedev->name, mig_state_to_str(new_state),
> - strerror(errno));
> + error_setg_errno(errp, errno,
> + "%s: Failed setting device state to %s. "
> + "Recover state is ERROR. Resetting device",
> + vbasedev->name, mig_state_to_str(new_state));
nit: this may be simplified
with a first unconditional msg saying Failed setting device state to %s.
Recover state is %s.
and not duplicating the msg.
In the reset label you can prepend "resetting the device ..."
>
> goto reset_device;
> }
>
> - error_report(
> - "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s",
> - vbasedev->name, mig_state_to_str(new_state),
> - strerror(errno), mig_state_to_str(recover_state));
> + error_setg_errno(errp, errno,
> + "%s: Failed setting device state to %s. "
> + "Setting device in recover state %s",
> + vbasedev->name, mig_state_to_str(new_state),
> + mig_state_to_str(recover_state));>
> mig_state->device_state = recover_state;
> if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
in case you fail setting the device in recover state, your simply
error_report() and do not prepend any other msg in the error handle. Is
it what you want? In the positive, maybe worth a comment.
> @@ -137,7 +139,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
> * This can happen if the device is asynchronously reset and
> * terminates a data transfer.
> */
> - error_report("%s: data_fd out of sync", vbasedev->name);
> + error_setg(errp, "%s: data_fd out of sync", vbasedev->name);
> close(mig_state->data_fd);
>
> return -EBADF;
> @@ -168,10 +170,11 @@ reset_device:
> */
> static int
> vfio_migration_set_state_or_reset(VFIODevice *vbasedev,
> - enum vfio_device_mig_state new_state)
> + enum vfio_device_mig_state new_state,
> + Error **errp)
> {
> return vfio_migration_set_state(vbasedev, new_state,
> - VFIO_DEVICE_STATE_ERROR);
> + VFIO_DEVICE_STATE_ERROR, errp);
> }
>
> static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
> @@ -399,10 +402,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
> switch (migration->device_state) {
> case VFIO_DEVICE_STATE_RUNNING:
> ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
> - VFIO_DEVICE_STATE_RUNNING);
> + VFIO_DEVICE_STATE_RUNNING, errp);
> if (ret) {
> - error_setg(errp, "%s: Failed to set new PRE_COPY state",
> - vbasedev->name);
> return ret;
> }
>
> @@ -435,13 +436,20 @@ static void vfio_save_cleanup(void *opaque)
> {
> VFIODevice *vbasedev = opaque;
> VFIOMigration *migration = vbasedev->migration;
> + Error *local_err = NULL;
> + int ret;
>
> /*
> * Changing device state from STOP_COPY to STOP can take time. Do it here,
> * after migration has completed, so it won't increase downtime.
> */
> if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) {
> - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP);
> + ret = vfio_migration_set_state_or_reset(vbasedev,
> + VFIO_DEVICE_STATE_STOP,
> + &local_err);
> + if (ret) {
> + error_report_err(local_err);
> + }
> }
>
> g_free(migration->data_buffer);
> @@ -549,11 +557,13 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
> VFIODevice *vbasedev = opaque;
> ssize_t data_size;
> int ret;
> + Error *local_err = NULL;
>
> /* We reach here with device state STOP or STOP_COPY only */
> ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
> - VFIO_DEVICE_STATE_STOP);
> + VFIO_DEVICE_STATE_STOP, &local_err);
> if (ret) {
> + error_report_err(local_err);
> return ret;
> }
>
> @@ -591,14 +601,9 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
> static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
> {
> VFIODevice *vbasedev = opaque;
> - int ret;
>
> - ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
> - vbasedev->migration->device_state);
> - if (ret) {
> - error_setg(errp, "%s: Failed to set RESUMING state", vbasedev->name);
> - }
> - return ret;
> + return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
> + vbasedev->migration->device_state, errp);
> }
>
> static int vfio_load_cleanup(void *opaque)
> @@ -714,19 +719,20 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
> VFIODevice *vbasedev = opaque;
> VFIOMigration *migration = vbasedev->migration;
> enum vfio_device_mig_state new_state;
> + Error *local_err = NULL;
> int ret;
>
> new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
> VFIO_DEVICE_STATE_PRE_COPY_P2P :
> VFIO_DEVICE_STATE_RUNNING_P2P;
>
> - ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
> + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
> if (ret) {
> /*
> * Migration should be aborted in this case, but vm_state_notify()
> * currently does not support reporting failures.
> */
> - migration_file_set_error(ret, NULL);
> + migration_file_set_error(ret, local_err);
> }
>
> trace_vfio_vmstate_change_prepare(vbasedev->name, running,
> @@ -738,6 +744,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> {
> VFIODevice *vbasedev = opaque;
> enum vfio_device_mig_state new_state;
> + Error *local_err = NULL;
> int ret;
>
> if (running) {
> @@ -750,13 +757,13 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> VFIO_DEVICE_STATE_STOP;
> }
>
> - ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
> + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
> if (ret) {
> /*
> * Migration should be aborted in this case, but vm_state_notify()
> * currently does not support reporting failures.
> */
> - migration_file_set_error(ret, NULL);
> + migration_file_set_error(ret, local_err);
> }
>
> trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
> @@ -769,11 +776,23 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
> VFIOMigration *migration = container_of(notifier, VFIOMigration,
> migration_state);
> VFIODevice *vbasedev = migration->vbasedev;
> + Error *local_err = NULL;
> + int ret = 0;
>
> trace_vfio_migration_state_notifier(vbasedev->name, e->type);
>
> if (e->type == MIG_EVENT_PRECOPY_FAILED) {
> - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
> + /*
> + * MigrationNotifyFunc may return an error code and an Error
> + * object only for MIG_EVENT_PRECOPY_SETUP. Hence, report the
> + * error locally and ignore the errp argument.
> + */
> + ret = vfio_migration_set_state_or_reset(vbasedev,
> + VFIO_DEVICE_STATE_RUNNING,
> + &local_err);
> + if (ret) {
> + error_report_err(local_err);
> + }
> }
> return 0;
> }
Thanks
Eric
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler
2024-05-14 15:31 ` [PATCH v6 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
@ 2024-05-15 9:20 ` Eric Auger
2024-05-16 8:22 ` Avihai Horon
1 sibling, 0 replies; 29+ messages in thread
From: Eric Auger @ 2024-05-15 9:20 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
On 5/14/24 17:31, Cédric Le Goater wrote:
> Use vmstate_save_state_with_err() to improve error reporting in the
> callers and store a reported error under the migration stream. Add
> documentation while at it.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v6:
>
> - Modified title (Avihai)
> - vfio_save_device_config_state() : Set errp if the migration stream
> is in error (Avihai)
> - vfio_save_state() : Changed error prefix (Avihai)
>
> include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++-
> hw/vfio/migration.c | 25 ++++++++++++++++++-------
> hw/vfio/pci.c | 5 +++--
> 3 files changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b9da6c08ef41174610eb92726c590309a53696a3..46f88493634b5634a9c14a5caa33a463fbf2c50d 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -133,7 +133,30 @@ struct VFIODeviceOps {
> int (*vfio_hot_reset_multi)(VFIODevice *vdev);
> void (*vfio_eoi)(VFIODevice *vdev);
> Object *(*vfio_get_object)(VFIODevice *vdev);
> - void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
> +
> + /**
> + * @vfio_save_config
> + *
> + * Save device config state
> + *
> + * @vdev: #VFIODevice for which to save the config
> + * @f: #QEMUFile where to send the data
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Returns zero to indicate success and negative for error
> + */
> + int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);
> +
> + /**
> + * @vfio_load_config
> + *
> + * Load device config state
> + *
> + * @vdev: #VFIODevice for which to load the config
> + * @f: #QEMUFile where to get the data
> + *
> + * Returns zero to indicate success and negative for error
> + */
> int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
> };
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index bf11135167ebb162dd41415656bdacfa0e1ca550..d089fa9b937e725307c1a56755495d5b8fae2065 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -189,21 +189,30 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
> return ret;
> }
>
> -static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
> +static int vfio_save_device_config_state(QEMUFile *f, void *opaque,
> + Error **errp)
> {
> VFIODevice *vbasedev = opaque;
> + int ret;
>
> qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
>
> if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
> - vbasedev->ops->vfio_save_config(vbasedev, f);
> + ret = vbasedev->ops->vfio_save_config(vbasedev, f, errp);
> + if (ret) {
> + return ret;
> + }
> }
>
> qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>
> trace_vfio_save_device_config_state(vbasedev->name);
>
> - return qemu_file_get_error(f);
> + ret = qemu_file_get_error(f);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Failed to save state");
> + }
> + return ret;
> }
>
> static int vfio_load_device_config_state(QEMUFile *f, void *opaque)
> @@ -588,13 +597,15 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
> static void vfio_save_state(QEMUFile *f, void *opaque)
> {
> VFIODevice *vbasedev = opaque;
> + Error *local_err = NULL;
> int ret;
>
> - ret = vfio_save_device_config_state(f, opaque);
> + ret = vfio_save_device_config_state(f, opaque, &local_err);
> if (ret) {
> - error_report("%s: Failed to save device config space",
> - vbasedev->name);
> - qemu_file_set_error(f, ret);
> + error_prepend(&local_err,
> + "vfio: Failed to save device config space of %s - ",
> + vbasedev->name);
> + qemu_file_set_error_obj(f, ret, local_err);> }
> }
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 64780d1b793345c8e8996fe6b7987059ce831c11..fc6e54e871508bb0e2a3ac9079a195c086531f21 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2586,11 +2586,12 @@ static const VMStateDescription vmstate_vfio_pci_config = {
> }
> };
>
> -static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
> +static int vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f, Error **errp)
> {
> VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>
> - vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL);
> + return vmstate_save_state_with_err(f, &vmstate_vfio_pci_config, vdev, NULL,
> + errp);
> }
>
> static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
Looks good to me
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 6/9] vfio: Reverse test on vfio_get_xlat_addr()
2024-05-14 15:31 ` [PATCH v6 6/9] vfio: Reverse test on vfio_get_xlat_addr() Cédric Le Goater
@ 2024-05-15 9:22 ` Eric Auger
0 siblings, 0 replies; 29+ messages in thread
From: Eric Auger @ 2024-05-15 9:22 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
On 5/14/24 17:31, Cédric Le Goater wrote:
> It will simplify the changes coming after.
>
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v6:
>
> - Modified title (Avihai)
> - vfio_iommu_map_dirty_notify() : Modified goto label (Avihai)
>
> hw/vfio/common.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c..4e2ef3d3034e72aa6a546bcb9ea1f31a0bbd5b1b 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1224,16 +1224,20 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> }
>
> rcu_read_lock();
> - if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> - ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
> - translated_addr);
> - if (ret) {
> - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> - "0x%"HWADDR_PRIx") = %d (%s)",
> - bcontainer, iova, iotlb->addr_mask + 1, ret,
> - strerror(-ret));
> - }
> + if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> + goto out_unlock;
> }
> +
> + ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
> + translated_addr);
> + if (ret) {
> + error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> + "0x%"HWADDR_PRIx") = %d (%s)",
> + bcontainer, iova, iotlb->addr_mask + 1, ret,
> + strerror(-ret));
> + }
> +
> +out_unlock:
> rcu_read_unlock();
>
> out:
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 7/9] memory: Add Error** argument to memory_get_xlat_addr()
2024-05-14 15:31 ` [PATCH v6 7/9] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
@ 2024-05-15 9:25 ` Eric Auger
2024-05-16 8:25 ` Avihai Horon
1 sibling, 0 replies; 29+ messages in thread
From: Eric Auger @ 2024-05-15 9:25 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Michael S . Tsirkin, Paolo Bonzini,
David Hildenbrand
On 5/14/24 17:31, Cédric Le Goater wrote:
> Let the callers do the reporting. This will be useful in
> vfio_iommu_map_dirty_notify().
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v6:
>
> - Fixed memory_get_xlat_addr documentation (Avihai)
>
> include/exec/memory.h | 15 ++++++++++++++-
> hw/vfio/common.c | 13 +++++++++----
> hw/virtio/vhost-vdpa.c | 5 ++++-
> system/memory.c | 10 +++++-----
> 4 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index d417d7f363dbbca6553c449582a93d5da73cca40..9cdd64e9c69b63f9d27cebc2e8cb366e22ed7577 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -774,9 +774,22 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
> void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> RamDiscardListener *rdl);
>
> +/**
> + * memory_get_xlat_addr: Extract addresses from a TLB entry
> + *
> + * @iotlb: pointer to an #IOMMUTLBEntry
> + * @vaddr: virtual address
> + * @ram_addr: RAM address
> + * @read_only: indicates if writes are allowed
> + * @mr_has_discard_manager: indicates memory is controlled by a
> + * RamDiscardManager
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Return: true on success, else false setting @errp with error.
> + */
> bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> ram_addr_t *ram_addr, bool *read_only,
> - bool *mr_has_discard_manager);
> + bool *mr_has_discard_manager, Error **errp);
>
> typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4e2ef3d3034e72aa6a546bcb9ea1f31a0bbd5b1b..919c4c52bc1590fd6c0bda37ba5881f58ff2ffff 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -253,12 +253,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>
> /* Called with rcu_read_lock held. */
> static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> - ram_addr_t *ram_addr, bool *read_only)
> + ram_addr_t *ram_addr, bool *read_only,
> + Error **errp)
> {
> bool ret, mr_has_discard_manager;
>
> ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
> - &mr_has_discard_manager);
> + &mr_has_discard_manager, errp);
> if (ret && mr_has_discard_manager) {
> /*
> * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> @@ -288,6 +289,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> void *vaddr;
> int ret;
> + Error *local_err = NULL;
>
> trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
> iova, iova + iotlb->addr_mask);
> @@ -304,7 +306,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> bool read_only;
>
> - if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> + if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, &local_err)) {
> + error_report_err(local_err);
> goto out;
> }
> /*
> @@ -1213,6 +1216,7 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> VFIOContainerBase *bcontainer = giommu->bcontainer;
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> ram_addr_t translated_addr;
> + Error *local_err = NULL;
> int ret = -EINVAL;
>
> trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
> @@ -1224,7 +1228,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> }
>
> rcu_read_lock();
> - if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> + if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
> + error_report_err(local_err);
> goto out_unlock;
> }
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index e827b9175fc61f1ef419e48d90a440b00449312a..ed99ab87457d8f31b98ace960713f48d47b27102 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -208,6 +208,7 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> void *vaddr;
> int ret;
> Int128 llend;
> + Error *local_err = NULL;
>
> if (iotlb->target_as != &address_space_memory) {
> error_report("Wrong target AS \"%s\", only system memory is allowed",
> @@ -227,7 +228,9 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> bool read_only;
>
> - if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL)) {
> + if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL,
> + &local_err)) {
> + error_report_err(local_err);
> return;
> }
> ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova,
> diff --git a/system/memory.c b/system/memory.c
> index 642a449f8c867d38c62a748a4dfd5c055637c205..9540caa8a1f4da8501bf5ae9d7eedde8b775e1dc 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2179,7 +2179,7 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> /* Called with rcu_read_lock held. */
> bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> ram_addr_t *ram_addr, bool *read_only,
> - bool *mr_has_discard_manager)
> + bool *mr_has_discard_manager, Error **errp)
> {
> MemoryRegion *mr;
> hwaddr xlat;
> @@ -2197,7 +2197,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> mr = address_space_translate(&address_space_memory, iotlb->translated_addr,
> &xlat, &len, writable, MEMTXATTRS_UNSPECIFIED);
> if (!memory_region_is_ram(mr)) {
> - error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
> + error_setg(errp, "iommu map to non memory area %" HWADDR_PRIx "", xlat);
> return false;
> } else if (memory_region_has_ram_discard_manager(mr)) {
> RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> @@ -2216,8 +2216,8 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> * were already restored before IOMMUs are restored.
> */
> if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> - error_report("iommu map to discarded memory (e.g., unplugged via"
> - " virtio-mem): %" HWADDR_PRIx "",
> + error_setg(errp, "iommu map to discarded memory (e.g., unplugged"
> + " via virtio-mem): %" HWADDR_PRIx "",
> iotlb->translated_addr);
> return false;
> }
> @@ -2228,7 +2228,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> * check that it did not truncate too much.
> */
> if (len & iotlb->addr_mask) {
> - error_report("iommu has granularity incompatible with target AS");
> + error_setg(errp, "iommu has granularity incompatible with target AS");
> return false;
> }
>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler
2024-05-14 15:31 ` [PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
@ 2024-05-15 9:34 ` Eric Auger
2024-05-16 8:42 ` Avihai Horon
1 sibling, 0 replies; 29+ messages in thread
From: Eric Auger @ 2024-05-15 9:34 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
On 5/14/24 17:31, Cédric Le Goater wrote:
> Let the callers do the error reporting. Add documentation while at it.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v6:
>
> - Fixed the line wrapping (Avihai)
> - Fixed query_dirty_bitmap documentation (Avihai)
>
> Changes in v5:
>
> - Replaced error_setg() by error_setg_errno() in
> vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap()
> - ':' -> '-' in vfio_iommu_map_dirty_notify()
>
> include/hw/vfio/vfio-common.h | 4 +-
> include/hw/vfio/vfio-container-base.h | 21 +++++++--
> hw/vfio/common.c | 61 ++++++++++++++++++---------
> hw/vfio/container-base.c | 7 +--
> hw/vfio/container.c | 14 +++---
> 5 files changed, 72 insertions(+), 35 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 46f88493634b5634a9c14a5caa33a463fbf2c50d..68911d36676667352e94a97895828aff4b194b57 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -274,9 +274,9 @@ bool
> vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer);
> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> VFIOBitmap *vbmap, hwaddr iova,
> - hwaddr size);
> + hwaddr size, Error **errp);
> int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
> - uint64_t size, ram_addr_t ram_addr);
> + uint64_t size, ram_addr_t ram_addr, Error **errp);
>
> /* Returns 0 on success, or a negative errno. */
> int vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 326ceea52a2030eec9dad289a9845866c4a8c090..42cfbf32dc737606c3f620d126f35d85d4833534 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -84,8 +84,8 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> bool start, Error **errp);
> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> - VFIOBitmap *vbmap,
> - hwaddr iova, hwaddr size);
> + VFIOBitmap *vbmap, hwaddr iova,
> + hwaddr size, Error **errp);
>
> void vfio_container_init(VFIOContainerBase *bcontainer,
> VFIOAddressSpace *space,
> @@ -138,9 +138,22 @@ struct VFIOIOMMUClass {
> */
> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
> bool start, Error **errp);
> + /**
> + * @query_dirty_bitmap
> + *
> + * Get bitmap of dirty pages from container
> + *
> + * @bcontainer: #VFIOContainerBase from which to get dirty pages
> + * @vbmap: #VFIOBitmap internal bitmap structure
> + * @iova: iova base address
> + * @size: size of iova range
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Returns zero to indicate success and negative for error
> + */
> int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
> - VFIOBitmap *vbmap,
> - hwaddr iova, hwaddr size);
> + VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
> + Error **errp);
> /* PCI specific */
> int (*pci_hot_reset)(VFIODevice *vbasedev, bool single);
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 919c4c52bc1590fd6c0bda37ba5881f58ff2ffff..9b5123d45fc1f9d5be4bbbf92898f89cd11e1363 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1140,8 +1140,8 @@ static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova,
> }
>
> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> - VFIOBitmap *vbmap, hwaddr iova,
> - hwaddr size)
> + VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
> + Error **errp)
> {
> VFIODevice *vbasedev;
> int ret;
> @@ -1150,10 +1150,10 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> ret = vfio_device_dma_logging_report(vbasedev, iova, size,
> vbmap->bitmap);
> if (ret) {
> - error_report("%s: Failed to get DMA logging report, iova: "
> - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx
> - ", err: %d (%s)",
> - vbasedev->name, iova, size, ret, strerror(-ret));
> + error_setg_errno(errp, -ret,
> + "%s: Failed to get DMA logging report, iova: "
> + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx,
> + vbasedev->name, iova, size);
>
> return ret;
> }
> @@ -1163,7 +1163,7 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> }
>
> int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
> - uint64_t size, ram_addr_t ram_addr)
> + uint64_t size, ram_addr_t ram_addr, Error **errp)
> {
> bool all_device_dirty_tracking =
> vfio_devices_all_device_dirty_tracking(bcontainer);
> @@ -1180,13 +1180,17 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
>
> ret = vfio_bitmap_alloc(&vbmap, size);
> if (ret) {
> + error_setg_errno(errp, -ret,
> + "Failed to allocate dirty tracking bitmap");
> return ret;
> }
>
> if (all_device_dirty_tracking) {
> - ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size);
> + ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size,
> + errp);
> } else {
> - ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size);
> + ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size,
> + errp);
> }
>
> if (ret) {
> @@ -1234,12 +1238,13 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> }
>
> ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
> - translated_addr);
> + translated_addr, &local_err);
> if (ret) {
> - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> - "0x%"HWADDR_PRIx") = %d (%s)",
> - bcontainer, iova, iotlb->addr_mask + 1, ret,
> - strerror(-ret));
> + error_prepend(&local_err,
> + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> + "0x%"HWADDR_PRIx") failed - ", bcontainer, iova,
> + iotlb->addr_mask + 1);
> + error_report_err(local_err);
> }
>
> out_unlock:
> @@ -1259,12 +1264,19 @@ static int vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section,
> const ram_addr_t ram_addr = memory_region_get_ram_addr(section->mr) +
> section->offset_within_region;
> VFIORamDiscardListener *vrdl = opaque;
> + Error *local_err = NULL;
> + int ret;
>
> /*
> * Sync the whole mapped region (spanning multiple individual mappings)
> * in one go.
> */
> - return vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr);
> + ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr,
> + &local_err);
> + if (ret) {
> + error_report_err(local_err);
> + }
> + return ret;
> }
>
> static int
> @@ -1296,7 +1308,7 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
> }
>
> static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
> - MemoryRegionSection *section)
> + MemoryRegionSection *section, Error **errp)
> {
> ram_addr_t ram_addr;
>
> @@ -1327,7 +1339,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
> }
> return 0;
> } else if (memory_region_has_ram_discard_manager(section->mr)) {
> - return vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section);
> + int ret;
> +
> + ret = vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section);
> + if (ret) {
> + error_setg(errp,
> + "Failed to sync dirty bitmap with RAM discard listener");
> + return ret;
> + }
> }
>
> ram_addr = memory_region_get_ram_addr(section->mr) +
> @@ -1335,7 +1354,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>
> return vfio_get_dirty_bitmap(bcontainer,
> REAL_HOST_PAGE_ALIGN(section->offset_within_address_space),
> - int128_get64(section->size), ram_addr);
> + int128_get64(section->size), ram_addr, errp);
> }
>
> static void vfio_listener_log_sync(MemoryListener *listener,
> @@ -1344,16 +1363,16 @@ static void vfio_listener_log_sync(MemoryListener *listener,
> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> listener);
> int ret;
> + Error *local_err = NULL;
>
> if (vfio_listener_skipped_section(section)) {
> return;
> }
>
> if (vfio_devices_all_dirty_tracking(bcontainer)) {
> - ret = vfio_sync_dirty_bitmap(bcontainer, section);
> + ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
> if (ret) {
> - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
> - strerror(-ret));
> + error_report_err(local_err);
> vfio_set_migration_error(ret);
> }
> }
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 7c0764121d24b02b6c4e66e368d7dff78a6d65aa..4fa250c46fd16333d2e2358ede8b0a0afdb185b3 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -64,11 +64,12 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> }
>
> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> - VFIOBitmap *vbmap,
> - hwaddr iova, hwaddr size)
> + VFIOBitmap *vbmap, hwaddr iova,
> + hwaddr size, Error **errp)
> {
> g_assert(bcontainer->ops->query_dirty_bitmap);
> - return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size);
> + return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size,
> + errp);
> }
>
> void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space,
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index c35221fbe7dc5453050f97cd186fc958e24f28f7..e00db6546c652c61a352f8e4cd65b212ecfb65bc 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -130,6 +130,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
> };
> bool need_dirty_sync = false;
> int ret;
> + Error *local_err = NULL;
>
> if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) {
> if (!vfio_devices_all_device_dirty_tracking(bcontainer) &&
> @@ -165,8 +166,9 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
>
> if (need_dirty_sync) {
> ret = vfio_get_dirty_bitmap(bcontainer, iova, size,
> - iotlb->translated_addr);
> + iotlb->translated_addr, &local_err);
> if (ret) {
> + error_report_err(local_err);
> return ret;
> }
> }
> @@ -236,7 +238,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>
> static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> VFIOBitmap *vbmap,
> - hwaddr iova, hwaddr size)
> + hwaddr iova, hwaddr size,
> + Error **errp)
> {
> const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
> bcontainer);
> @@ -264,9 +267,10 @@ static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap);
> if (ret) {
> ret = -errno;
> - error_report("Failed to get dirty bitmap for iova: 0x%"PRIx64
> - " size: 0x%"PRIx64" err: %d", (uint64_t)range->iova,
> - (uint64_t)range->size, errno);
> + error_setg_errno(errp, errno,
> + "Failed to get dirty bitmap for iova: 0x%"PRIx64
> + " size: 0x%"PRIx64, (uint64_t)range->iova,
> + (uint64_t)range->size);
> }
>
> g_free(dbitmap);
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 9/9] vfio: Also trace event failures in vfio_save_complete_precopy()
2024-05-14 15:31 ` [PATCH v6 9/9] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
@ 2024-05-15 9:34 ` Eric Auger
0 siblings, 0 replies; 29+ messages in thread
From: Eric Auger @ 2024-05-15 9:34 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
On 5/14/24 17:31, Cédric Le Goater wrote:
> vfio_save_complete_precopy() currently returns before doing the trace
> event. Change that.
>
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> hw/vfio/migration.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index d089fa9b937e725307c1a56755495d5b8fae2065..b06d887df53155e676bf13fa03adaf0627841994 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -585,9 +585,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>
> qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> ret = qemu_file_get_error(f);
> - if (ret) {
> - return ret;
> - }
>
> trace_vfio_save_complete_precopy(vbasedev->name, ret);
>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-15 6:40 ` Eric Auger
@ 2024-05-15 16:55 ` Cédric Le Goater
0 siblings, 0 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-15 16:55 UTC (permalink / raw)
To: Eric Auger, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster, Duan, Zhenzhong
On 5/15/24 08:40, Eric Auger wrote:
> Hi Cédric,
>
> On 5/14/24 17:31, Cédric Le Goater wrote:
>> We will use the Error object to improve error reporting in the
>> .log_global*() handlers of VFIO. Add documentation while at it.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>> Changes in v5:
>>
>> - Fixed typo in set_dirty_page_tracking documentation
>>
>> include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
>> hw/vfio/common.c | 4 ++--
>> hw/vfio/container-base.c | 4 ++--
>> hw/vfio/container.c | 6 +++---
>> 4 files changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
>> index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>> void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>> MemoryRegionSection *section);
>> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>> - bool start);
>> + bool start, Error **errp);
> I am a bit confused now wrt [PATCH v2 03/11] vfio: Make
> VFIOIOMMUClass::attach_device() and its wrapper return bool & co
>
> Shall we return a bool or a int?
It would be better to follow the best practices described in qapi/error.h.
Zhenzhong excluded some files from his cleanup series to avoid conflicts
with this series of mine. And indeed, I would prefer to merge this one
first. It should be addressed later.
>
> Looking at ./include/qapi/error.h I have not found any stringent requirement
>
> * - Whenever practical, also return a value that indicates success /
> * failure. This can make the error checking more concise, and can
> * avoid useless error object creation and destruction. Note that
> * we still have many functions returning void. We recommend
> * • bool-valued functions return true on success / false on failure,
> * • pointer-valued functions return non-null / null pointer, and
> * • integer-valued functions return non-negative / negative.
>
>
>
>> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> VFIOBitmap *vbmap,
>> hwaddr iova, hwaddr size);
>> @@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
>> int (*attach_device)(const char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp);
>> void (*detach_device)(VFIODevice *vbasedev);
>> +
>> /* migration feature */
>> +
>> + /**
>> + * @set_dirty_page_tracking
>> + *
>> + * Start or stop dirty pages tracking on VFIO container
>> + *
>> + * @bcontainer: #VFIOContainerBase on which to de/activate dirty
>> + * page tracking
>> + * @start: indicates whether to start or stop dirty pages tracking
>> + * @errp: pointer to Error*, to store an error if it happens.
>> + *
>> + * Returns zero to indicate success and negative for error
>> + */
>> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>> - bool start);
>> + bool start, Error **errp);
>> int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
>> VFIOBitmap *vbmap,
>> hwaddr iova, hwaddr size);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>> if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>> ret = vfio_devices_dma_logging_start(bcontainer);
>> } else {
>> - ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
>> + ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
>> }
>>
>> if (ret) {
>> @@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>> if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>> vfio_devices_dma_logging_stop(bcontainer);
>> } else {
>> - ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
>> + ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
>> }
>>
>> if (ret) {
>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>> index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644
>> --- a/hw/vfio/container-base.c
>> +++ b/hw/vfio/container-base.c
>> @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>> }
>>
>> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>> - bool start)
>> + bool start, Error **errp)
>> {
>> if (!bcontainer->dirty_pages_supported) {
>> return 0;
>> }
>>
>> g_assert(bcontainer->ops->set_dirty_page_tracking);
>> - return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
>> + return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
>> }
>>
>> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>>
>> static int
>> vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>> - bool start)
>> + bool start, Error **errp)
>> {
>> const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>> bcontainer);
>> @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>> ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
>> if (ret) {
>> ret = -errno;
>> - error_report("Failed to set dirty tracking flag 0x%x errno: %d",
>> - dirty.flags, errno);
>> + error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x",
>> + dirty.flags);
>> }
>>
>> return ret;
>
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>
> Eric
Thanks,
C.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start()
2024-05-15 6:53 ` Eric Auger
@ 2024-05-15 17:09 ` Cédric Le Goater
0 siblings, 0 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-15 17:09 UTC (permalink / raw)
To: Eric Auger, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
On 5/15/24 08:53, Eric Auger wrote:
> Hi Cédric,
> On 5/14/24 17:31, Cédric Le Goater wrote:
>> This allows to update the Error argument of the VFIO log_global_start()
>> handler. Errors for container based logging will also be propagated to
>> qemu_savevm_state_setup() when the ram save_setup() handler is executed.
> nit: also now collect & print errors from
> vfio_container_set_dirty_page_tracking()
OK. To avoid resending, I amended the commit log with :
"Also, errors from vfio_container_set_dirty_page_tracking() are now
collected and reported."
Thanks,
C.
>>
>> The vfio_set_migration_error() call becomes redundant in
>> vfio_listener_log_global_start(). Remove it.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>> Changes in v6:
>>
>> - Commit log improvements (Avihai)
>>
>> Changes in v5:
>>
>> - Used error_setg_errno() in vfio_devices_dma_logging_start()
>>
>> hw/vfio/common.c | 26 +++++++++++++++-----------
>> 1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 485e53916491f1164d29e739fb7106c0c77df737..b5102f54a6474a50c6366e8fbce23812d55e384e 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1027,7 +1027,8 @@ static void vfio_device_feature_dma_logging_start_destroy(
>> g_free(feature);
>> }
>>
>> -static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
>> +static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>> + Error **errp)
>> {
>> struct vfio_device_feature *feature;
>> VFIODirtyRanges ranges;
>> @@ -1038,6 +1039,7 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
>> feature = vfio_device_feature_dma_logging_start_create(bcontainer,
>> &ranges);
>> if (!feature) {
>> + error_setg_errno(errp, errno, "Failed to prepare DMA logging");
>> return -errno;
>> }
>>
>> @@ -1049,8 +1051,8 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
>> ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
>> if (ret) {
>> ret = -errno;
>> - error_report("%s: Failed to start DMA logging, err %d (%s)",
>> - vbasedev->name, ret, strerror(errno));
>> + error_setg_errno(errp, errno, "%s: Failed to start DMA logging",
>> + vbasedev->name);
>> goto out;
>> }
>> vbasedev->dirty_tracking = true;
>> @@ -1069,20 +1071,19 @@ out:
>> static bool vfio_listener_log_global_start(MemoryListener *listener,
>> Error **errp)
>> {
>> + ERRP_GUARD();
>> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>> listener);
>> int ret;
>>
>> if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>> - ret = vfio_devices_dma_logging_start(bcontainer);
>> + ret = vfio_devices_dma_logging_start(bcontainer, errp);
>> } else {
>> - ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
>> + ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
>> }
>>
>> if (ret) {
>> - error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
>> - ret, strerror(-ret));
>> - vfio_set_migration_error(ret);
>> + error_prepend(errp, "vfio: Could not start dirty page tracking - ");
>> }
>> return !ret;
>> }
>> @@ -1091,17 +1092,20 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>> {
>> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>> listener);
>> + Error *local_err = NULL;
>> int ret = 0;
>>
>> if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>> vfio_devices_dma_logging_stop(bcontainer);
>> } else {
>> - ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
>> + ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
>> + &local_err);
>> }
>>
>> if (ret) {
>> - error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
>> - ret, strerror(-ret));
>> + error_prepend(&local_err,
>> + "vfio: Could not stop dirty page tracking - ");
>> + error_report_err(local_err);
>> vfio_set_migration_error(ret);
>> }
>> }
>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>
> Eric
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 3/9] migration: Extend migration_file_set_error() with Error* argument
2024-05-15 7:04 ` Eric Auger
@ 2024-05-15 17:15 ` Cédric Le Goater
0 siblings, 0 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-15 17:15 UTC (permalink / raw)
To: Eric Auger, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
On 5/15/24 09:04, Eric Auger wrote:
> Hi Cédric,
>
> On 5/14/24 17:31, Cédric Le Goater wrote:
>> Use it to update the current error of the migration stream if
>> available and if not, simply print out the error. Next changes will
>> update with an error to report.
>>
>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>> Acked-by: Fabiano Rosas <farosas@suse.de>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>> Changes in v6:
>>
>> - Commit log improvements (Avihai)
>>
>> include/migration/misc.h | 2 +-
>> hw/vfio/common.c | 2 +-
>> hw/vfio/migration.c | 4 ++--
>> migration/migration.c | 6 ++++--
>> 4 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>> index bf7339cc1e6430226127fb6a878d06b458170858..bfadc5613bac614a316e5aed7da95d8c7845cf42 100644
>> --- a/include/migration/misc.h
>> +++ b/include/migration/misc.h
>> @@ -97,7 +97,7 @@ void migration_add_notifier_mode(NotifierWithReturn *notify,
>>
>> void migration_remove_notifier(NotifierWithReturn *notify);
>> bool migration_is_running(void);
>> -void migration_file_set_error(int err);
>> +void migration_file_set_error(int ret, Error *err);
>>
>> /* True if incoming migration entered POSTCOPY_INCOMING_DISCARD */
>> bool migration_in_incoming_postcopy(void);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index b5102f54a6474a50c6366e8fbce23812d55e384e..ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -150,7 +150,7 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
>> static void vfio_set_migration_error(int err)
> nit: I would have renamed err into ret here to avoid any further confusion.
That was done in v5 :
https://lore.kernel.org/qemu-devel/20240506092053.388578-11-clg@redhat.com/
in the last patch, that I dropped in v6 because I believe it needs more
work. I will address these last changes, including the err->ret rename,
in a followup series if that's ok with you.
Thanks,
C.
>> {
>> if (migration_is_setup_or_active()) {
>> - migration_file_set_error(err);
>> + migration_file_set_error(err, NULL);
>> }
>> }
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 06ae40969b6c19037e190008e14f28be646278cd..bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -726,7 +726,7 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>> * Migration should be aborted in this case, but vm_state_notify()
>> * currently does not support reporting failures.
>> */
>> - migration_file_set_error(ret);
>> + migration_file_set_error(ret, NULL);
>> }
>>
>> trace_vfio_vmstate_change_prepare(vbasedev->name, running,
>> @@ -756,7 +756,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>> * Migration should be aborted in this case, but vm_state_notify()
>> * currently does not support reporting failures.
>> */
>> - migration_file_set_error(ret);
>> + migration_file_set_error(ret, NULL);
>> }
>>
>> trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
>> diff --git a/migration/migration.c b/migration/migration.c
>> index e88b24f1e6cbe82dad3f890c00e264d2ab6ad355..70d66a441bf04761decf91dbe57ce52c57fde58f 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2994,13 +2994,15 @@ static MigThrError postcopy_pause(MigrationState *s)
>> }
>> }
>>
>> -void migration_file_set_error(int err)
>> +void migration_file_set_error(int ret, Error *err)
>> {
>> MigrationState *s = current_migration;
>>
>> WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
>> if (s->to_dst_file) {
>> - qemu_file_set_error(s->to_dst_file, err);
>> + qemu_file_set_error_obj(s->to_dst_file, ret, err);
>> + } else if (err) {
>> + error_report_err(err);
>> }
>> }
>> }
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>
> Eric
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state()
2024-05-15 7:20 ` Eric Auger
@ 2024-05-16 7:17 ` Cédric Le Goater
0 siblings, 0 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-16 7:17 UTC (permalink / raw)
To: Eric Auger, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé,
Markus Armbruster
On 5/15/24 09:20, Eric Auger wrote:
> Hi Cédric,
>
> On 5/14/24 17:31, Cédric Le Goater wrote:
>> Add an Error** argument to vfio_migration_set_state() and adjust
>> callers, including vfio_save_setup(). The error will be propagated up
>> to qemu_savevm_state_setup() where the save_setup() handler is
>> executed.
>>
>> Modify vfio_vmstate_change_prepare() and vfio_vmstate_change() to
>> store a reported error under the migration stream if a migration is in
>> progress.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>> Changes in v6:
>>
>> - Commit log improvements (Avihai)
>> - vfio_migration_set_state() : Dropped the error_setg_errno() change
>> when setting device in recover state fails (Avihai)
>> - vfio_migration_state_notifier() : report local error (Avihai)
>>
>> Changes in v5:
>>
>> - Replaced error_setg() by error_setg_errno() in vfio_migration_set_state()
>> - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>>
>> hw/vfio/migration.c | 77 ++++++++++++++++++++++++++++-----------------
>> 1 file changed, 48 insertions(+), 29 deletions(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..bf11135167ebb162dd41415656bdacfa0e1ca550 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -82,7 +82,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>>
>> static int vfio_migration_set_state(VFIODevice *vbasedev,
>> enum vfio_device_mig_state new_state,
>> - enum vfio_device_mig_state recover_state)
>> + enum vfio_device_mig_state recover_state,
>> + Error **errp)
>> {
>> VFIOMigration *migration = vbasedev->migration;
>> uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
>> @@ -102,18 +103,19 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>> ret = -errno;
>>
>> if (recover_state == VFIO_DEVICE_STATE_ERROR) {
>> - error_report("%s: Failed setting device state to %s, err: %s. "
>> - "Recover state is ERROR. Resetting device",
>> - vbasedev->name, mig_state_to_str(new_state),
>> - strerror(errno));
>> + error_setg_errno(errp, errno,
>> + "%s: Failed setting device state to %s. "
>> + "Recover state is ERROR. Resetting device",
>> + vbasedev->name, mig_state_to_str(new_state));
> nit: this may be simplified
> with a first unconditional msg saying Failed setting device state to %s.
> Recover state is %s.
> and not duplicating the msg.
>
> In the reset label you can prepend "resetting the device ..."
I agree.
I'd rather do that in a followup patch after ther error_setg_* changes.
Adding to the TODO list.
>>
>> goto reset_device;
>> }
>>
>> - error_report(
>> - "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s",
>> - vbasedev->name, mig_state_to_str(new_state),
>> - strerror(errno), mig_state_to_str(recover_state));
>> + error_setg_errno(errp, errno,
>> + "%s: Failed setting device state to %s. "
>> + "Setting device in recover state %s",
>> + vbasedev->name, mig_state_to_str(new_state),
>> + mig_state_to_str(recover_state));>
>> mig_state->device_state = recover_state;
>> if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
> in case you fail setting the device in recover state, your simply
> error_report() and do not prepend any other msg in the error handle. Is
> it what you want?
yes. Otherwise, we loose the previous error message, which I think is more
important to report (first error). The following error_report handles an
error of an error, which can go on stderr.
> In the positive, maybe worth a comment.
OK, to avoid a resend, I will amend the patch with:
mig_state->device_state = recover_state;
if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
ret = -errno;
+ /*
+ * If setting the device in recover state fails, report
+ * the error here and propagate the first error.
+ */
error_report(
"%s: Failed setting device in recover state, err: %s. Resetting device",
vbasedev->name, strerror(errno));
Thanks,
C.
>> @@ -137,7 +139,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>> * This can happen if the device is asynchronously reset and
>> * terminates a data transfer.
>> */
>> - error_report("%s: data_fd out of sync", vbasedev->name);
>> + error_setg(errp, "%s: data_fd out of sync", vbasedev->name);
>> close(mig_state->data_fd);
>>
>> return -EBADF;
>> @@ -168,10 +170,11 @@ reset_device:
>> */
>> static int
>> vfio_migration_set_state_or_reset(VFIODevice *vbasedev,
>> - enum vfio_device_mig_state new_state)
>> + enum vfio_device_mig_state new_state,
>> + Error **errp)
>> {
>> return vfio_migration_set_state(vbasedev, new_state,
>> - VFIO_DEVICE_STATE_ERROR);
>> + VFIO_DEVICE_STATE_ERROR, errp);
>> }
>>
>> static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
>> @@ -399,10 +402,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
>> switch (migration->device_state) {
>> case VFIO_DEVICE_STATE_RUNNING:
>> ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
>> - VFIO_DEVICE_STATE_RUNNING);
>> + VFIO_DEVICE_STATE_RUNNING, errp);
>> if (ret) {
>> - error_setg(errp, "%s: Failed to set new PRE_COPY state",
>> - vbasedev->name);
>> return ret;
>> }
>>
>> @@ -435,13 +436,20 @@ static void vfio_save_cleanup(void *opaque)
>> {
>> VFIODevice *vbasedev = opaque;
>> VFIOMigration *migration = vbasedev->migration;
>> + Error *local_err = NULL;
>> + int ret;
>>
>> /*
>> * Changing device state from STOP_COPY to STOP can take time. Do it here,
>> * after migration has completed, so it won't increase downtime.
>> */
>> if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) {
>> - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP);
>> + ret = vfio_migration_set_state_or_reset(vbasedev,
>> + VFIO_DEVICE_STATE_STOP,
>> + &local_err);
>> + if (ret) {
>> + error_report_err(local_err);
>> + }
>> }
>>
>> g_free(migration->data_buffer);
>> @@ -549,11 +557,13 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>> VFIODevice *vbasedev = opaque;
>> ssize_t data_size;
>> int ret;
>> + Error *local_err = NULL;
>>
>> /* We reach here with device state STOP or STOP_COPY only */
>> ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
>> - VFIO_DEVICE_STATE_STOP);
>> + VFIO_DEVICE_STATE_STOP, &local_err);
>> if (ret) {
>> + error_report_err(local_err);
>> return ret;
>> }
>>
>> @@ -591,14 +601,9 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>> static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
>> {
>> VFIODevice *vbasedev = opaque;
>> - int ret;
>>
>> - ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
>> - vbasedev->migration->device_state);
>> - if (ret) {
>> - error_setg(errp, "%s: Failed to set RESUMING state", vbasedev->name);
>> - }
>> - return ret;
>> + return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
>> + vbasedev->migration->device_state, errp);
>> }
>>
>> static int vfio_load_cleanup(void *opaque)
>> @@ -714,19 +719,20 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>> VFIODevice *vbasedev = opaque;
>> VFIOMigration *migration = vbasedev->migration;
>> enum vfio_device_mig_state new_state;
>> + Error *local_err = NULL;
>> int ret;
>>
>> new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
>> VFIO_DEVICE_STATE_PRE_COPY_P2P :
>> VFIO_DEVICE_STATE_RUNNING_P2P;
>>
>> - ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
>> + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
>> if (ret) {
>> /*
>> * Migration should be aborted in this case, but vm_state_notify()
>> * currently does not support reporting failures.
>> */
>> - migration_file_set_error(ret, NULL);
>> + migration_file_set_error(ret, local_err);
>> }
>>
>> trace_vfio_vmstate_change_prepare(vbasedev->name, running,
>> @@ -738,6 +744,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>> {
>> VFIODevice *vbasedev = opaque;
>> enum vfio_device_mig_state new_state;
>> + Error *local_err = NULL;
>> int ret;
>>
>> if (running) {
>> @@ -750,13 +757,13 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>> VFIO_DEVICE_STATE_STOP;
>> }
>>
>> - ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
>> + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
>> if (ret) {
>> /*
>> * Migration should be aborted in this case, but vm_state_notify()
>> * currently does not support reporting failures.
>> */
>> - migration_file_set_error(ret, NULL);
>> + migration_file_set_error(ret, local_err);
>> }
>>
>> trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
>> @@ -769,11 +776,23 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
>> VFIOMigration *migration = container_of(notifier, VFIOMigration,
>> migration_state);
>> VFIODevice *vbasedev = migration->vbasedev;
>> + Error *local_err = NULL;
>> + int ret = 0;
>>
>> trace_vfio_migration_state_notifier(vbasedev->name, e->type);
>>
>> if (e->type == MIG_EVENT_PRECOPY_FAILED) {
>> - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
>> + /*
>> + * MigrationNotifyFunc may return an error code and an Error
>> + * object only for MIG_EVENT_PRECOPY_SETUP. Hence, report the
>> + * error locally and ignore the errp argument.
>> + */
>> + ret = vfio_migration_set_state_or_reset(vbasedev,
>> + VFIO_DEVICE_STATE_RUNNING,
>> + &local_err);
>> + if (ret) {
>> + error_report_err(local_err);
>> + }
>> }
>> return 0;
>> }
> Thanks
>
> Eric
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state()
2024-05-14 15:31 ` [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state() Cédric Le Goater
2024-05-15 7:20 ` Eric Auger
@ 2024-05-16 8:18 ` Avihai Horon
2024-05-16 12:07 ` Cédric Le Goater
1 sibling, 1 reply; 29+ messages in thread
From: Avihai Horon @ 2024-05-16 8:18 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé,
Markus Armbruster
On 14/05/2024 18:31, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> Add an Error** argument to vfio_migration_set_state() and adjust
> callers, including vfio_save_setup(). The error will be propagated up
> to qemu_savevm_state_setup() where the save_setup() handler is
> executed.
>
> Modify vfio_vmstate_change_prepare() and vfio_vmstate_change() to
> store a reported error under the migration stream if a migration is in
> progress.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
With/without Eric's suggestion:
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Two nits below though (if you respin or want to amend inline).
> ---
>
> Changes in v6:
>
> - Commit log improvements (Avihai)
> - vfio_migration_set_state() : Dropped the error_setg_errno() change
> when setting device in recover state fails (Avihai)
> - vfio_migration_state_notifier() : report local error (Avihai)
>
> Changes in v5:
>
> - Replaced error_setg() by error_setg_errno() in vfio_migration_set_state()
> - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>
> hw/vfio/migration.c | 77 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 48 insertions(+), 29 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..bf11135167ebb162dd41415656bdacfa0e1ca550 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -82,7 +82,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>
> static int vfio_migration_set_state(VFIODevice *vbasedev,
> enum vfio_device_mig_state new_state,
> - enum vfio_device_mig_state recover_state)
> + enum vfio_device_mig_state recover_state,
> + Error **errp)
> {
> VFIOMigration *migration = vbasedev->migration;
> uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
> @@ -102,18 +103,19 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
> ret = -errno;
>
> if (recover_state == VFIO_DEVICE_STATE_ERROR) {
> - error_report("%s: Failed setting device state to %s, err: %s. "
> - "Recover state is ERROR. Resetting device",
> - vbasedev->name, mig_state_to_str(new_state),
> - strerror(errno));
> + error_setg_errno(errp, errno,
> + "%s: Failed setting device state to %s. "
> + "Recover state is ERROR. Resetting device",
> + vbasedev->name, mig_state_to_str(new_state));
>
> goto reset_device;
> }
>
> - error_report(
> - "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s",
> - vbasedev->name, mig_state_to_str(new_state),
> - strerror(errno), mig_state_to_str(recover_state));
> + error_setg_errno(errp, errno,
> + "%s: Failed setting device state to %s. "
> + "Setting device in recover state %s",
> + vbasedev->name, mig_state_to_str(new_state),
> + mig_state_to_str(recover_state));
>
> mig_state->device_state = recover_state;
> if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
> @@ -137,7 +139,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
> * This can happen if the device is asynchronously reset and
> * terminates a data transfer.
> */
> - error_report("%s: data_fd out of sync", vbasedev->name);
> + error_setg(errp, "%s: data_fd out of sync", vbasedev->name);
> close(mig_state->data_fd);
>
> return -EBADF;
> @@ -168,10 +170,11 @@ reset_device:
> */
> static int
> vfio_migration_set_state_or_reset(VFIODevice *vbasedev,
> - enum vfio_device_mig_state new_state)
> + enum vfio_device_mig_state new_state,
> + Error **errp)
> {
> return vfio_migration_set_state(vbasedev, new_state,
> - VFIO_DEVICE_STATE_ERROR);
> + VFIO_DEVICE_STATE_ERROR, errp);
> }
>
> static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
> @@ -399,10 +402,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
> switch (migration->device_state) {
> case VFIO_DEVICE_STATE_RUNNING:
> ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
> - VFIO_DEVICE_STATE_RUNNING);
> + VFIO_DEVICE_STATE_RUNNING, errp);
> if (ret) {
> - error_setg(errp, "%s: Failed to set new PRE_COPY state",
> - vbasedev->name);
> return ret;
> }
>
> @@ -435,13 +436,20 @@ static void vfio_save_cleanup(void *opaque)
> {
> VFIODevice *vbasedev = opaque;
> VFIOMigration *migration = vbasedev->migration;
> + Error *local_err = NULL;
> + int ret;
>
> /*
> * Changing device state from STOP_COPY to STOP can take time. Do it here,
> * after migration has completed, so it won't increase downtime.
> */
> if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) {
> - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP);
> + ret = vfio_migration_set_state_or_reset(vbasedev,
> + VFIO_DEVICE_STATE_STOP,
> + &local_err);
> + if (ret) {
> + error_report_err(local_err);
> + }
> }
>
> g_free(migration->data_buffer);
> @@ -549,11 +557,13 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
> VFIODevice *vbasedev = opaque;
> ssize_t data_size;
> int ret;
> + Error *local_err = NULL;
>
> /* We reach here with device state STOP or STOP_COPY only */
> ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
> - VFIO_DEVICE_STATE_STOP);
> + VFIO_DEVICE_STATE_STOP, &local_err);
> if (ret) {
> + error_report_err(local_err);
> return ret;
> }
>
> @@ -591,14 +601,9 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
> static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
> {
> VFIODevice *vbasedev = opaque;
> - int ret;
>
> - ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
> - vbasedev->migration->device_state);
> - if (ret) {
> - error_setg(errp, "%s: Failed to set RESUMING state", vbasedev->name);
> - }
> - return ret;
> + return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
> + vbasedev->migration->device_state, errp);
> }
>
> static int vfio_load_cleanup(void *opaque)
> @@ -714,19 +719,20 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
> VFIODevice *vbasedev = opaque;
> VFIOMigration *migration = vbasedev->migration;
> enum vfio_device_mig_state new_state;
> + Error *local_err = NULL;
> int ret;
>
> new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
> VFIO_DEVICE_STATE_PRE_COPY_P2P :
> VFIO_DEVICE_STATE_RUNNING_P2P;
>
> - ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
> + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
> if (ret) {
> /*
> * Migration should be aborted in this case, but vm_state_notify()
> * currently does not support reporting failures.
> */
> - migration_file_set_error(ret, NULL);
> + migration_file_set_error(ret, local_err);
> }
>
> trace_vfio_vmstate_change_prepare(vbasedev->name, running,
> @@ -738,6 +744,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> {
> VFIODevice *vbasedev = opaque;
> enum vfio_device_mig_state new_state;
> + Error *local_err = NULL;
> int ret;
>
> if (running) {
> @@ -750,13 +757,13 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> VFIO_DEVICE_STATE_STOP;
> }
>
> - ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
> + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
> if (ret) {
> /*
> * Migration should be aborted in this case, but vm_state_notify()
> * currently does not support reporting failures.
> */
> - migration_file_set_error(ret, NULL);
> + migration_file_set_error(ret, local_err);
> }
>
> trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
> @@ -769,11 +776,23 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
> VFIOMigration *migration = container_of(notifier, VFIOMigration,
> migration_state);
> VFIODevice *vbasedev = migration->vbasedev;
> + Error *local_err = NULL;
> + int ret = 0;
Nit: No need to set ret to 0.
>
> trace_vfio_migration_state_notifier(vbasedev->name, e->type);
>
> if (e->type == MIG_EVENT_PRECOPY_FAILED) {
> - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
> + /*
> + * MigrationNotifyFunc may return an error code and an Error
> + * object only for MIG_EVENT_PRECOPY_SETUP. Hence, report the
> + * error locally and ignore the errp argument.
> + */
Nit: maybe rephrase to the following:
"MigrationNotifyFunc may not return an error code and an Error object
for MIG_EVENT_PRECOPY_FAILED. Hence ..."
(This way, this comment will not become stale if new MIG_EVENTs that are
allowed to return error are added)
Thanks.
> + ret = vfio_migration_set_state_or_reset(vbasedev,
> + VFIO_DEVICE_STATE_RUNNING,
> + &local_err);
> + if (ret) {
> + error_report_err(local_err);
> + }
> }
> return 0;
> }
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler
2024-05-14 15:31 ` [PATCH v6 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
2024-05-15 9:20 ` Eric Auger
@ 2024-05-16 8:22 ` Avihai Horon
1 sibling, 0 replies; 29+ messages in thread
From: Avihai Horon @ 2024-05-16 8:22 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé,
Markus Armbruster
On 14/05/2024 18:31, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> Use vmstate_save_state_with_err() to improve error reporting in the
> callers and store a reported error under the migration stream. Add
> documentation while at it.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> ---
>
> Changes in v6:
>
> - Modified title (Avihai)
> - vfio_save_device_config_state() : Set errp if the migration stream
> is in error (Avihai)
> - vfio_save_state() : Changed error prefix (Avihai)
>
> include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++-
> hw/vfio/migration.c | 25 ++++++++++++++++++-------
> hw/vfio/pci.c | 5 +++--
> 3 files changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b9da6c08ef41174610eb92726c590309a53696a3..46f88493634b5634a9c14a5caa33a463fbf2c50d 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -133,7 +133,30 @@ struct VFIODeviceOps {
> int (*vfio_hot_reset_multi)(VFIODevice *vdev);
> void (*vfio_eoi)(VFIODevice *vdev);
> Object *(*vfio_get_object)(VFIODevice *vdev);
> - void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
> +
> + /**
> + * @vfio_save_config
> + *
> + * Save device config state
> + *
> + * @vdev: #VFIODevice for which to save the config
> + * @f: #QEMUFile where to send the data
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Returns zero to indicate success and negative for error
> + */
> + int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);
> +
> + /**
> + * @vfio_load_config
> + *
> + * Load device config state
> + *
> + * @vdev: #VFIODevice for which to load the config
> + * @f: #QEMUFile where to get the data
> + *
> + * Returns zero to indicate success and negative for error
> + */
> int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
> };
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index bf11135167ebb162dd41415656bdacfa0e1ca550..d089fa9b937e725307c1a56755495d5b8fae2065 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -189,21 +189,30 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
> return ret;
> }
>
> -static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
> +static int vfio_save_device_config_state(QEMUFile *f, void *opaque,
> + Error **errp)
> {
> VFIODevice *vbasedev = opaque;
> + int ret;
>
> qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
>
> if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
> - vbasedev->ops->vfio_save_config(vbasedev, f);
> + ret = vbasedev->ops->vfio_save_config(vbasedev, f, errp);
> + if (ret) {
> + return ret;
> + }
> }
>
> qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>
> trace_vfio_save_device_config_state(vbasedev->name);
>
> - return qemu_file_get_error(f);
> + ret = qemu_file_get_error(f);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Failed to save state");
> + }
> + return ret;
> }
>
> static int vfio_load_device_config_state(QEMUFile *f, void *opaque)
> @@ -588,13 +597,15 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
> static void vfio_save_state(QEMUFile *f, void *opaque)
> {
> VFIODevice *vbasedev = opaque;
> + Error *local_err = NULL;
> int ret;
>
> - ret = vfio_save_device_config_state(f, opaque);
> + ret = vfio_save_device_config_state(f, opaque, &local_err);
> if (ret) {
> - error_report("%s: Failed to save device config space",
> - vbasedev->name);
> - qemu_file_set_error(f, ret);
> + error_prepend(&local_err,
> + "vfio: Failed to save device config space of %s - ",
> + vbasedev->name);
> + qemu_file_set_error_obj(f, ret, local_err);
> }
> }
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 64780d1b793345c8e8996fe6b7987059ce831c11..fc6e54e871508bb0e2a3ac9079a195c086531f21 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2586,11 +2586,12 @@ static const VMStateDescription vmstate_vfio_pci_config = {
> }
> };
>
> -static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
> +static int vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f, Error **errp)
> {
> VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>
> - vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL);
> + return vmstate_save_state_with_err(f, &vmstate_vfio_pci_config, vdev, NULL,
> + errp);
> }
>
> static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 7/9] memory: Add Error** argument to memory_get_xlat_addr()
2024-05-14 15:31 ` [PATCH v6 7/9] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
2024-05-15 9:25 ` Eric Auger
@ 2024-05-16 8:25 ` Avihai Horon
1 sibling, 0 replies; 29+ messages in thread
From: Avihai Horon @ 2024-05-16 8:25 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé,
Markus Armbruster, Michael S . Tsirkin, Paolo Bonzini,
David Hildenbrand
On 14/05/2024 18:31, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> Let the callers do the reporting. This will be useful in
> vfio_iommu_map_dirty_notify().
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
FWIW,
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> ---
>
> Changes in v6:
>
> - Fixed memory_get_xlat_addr documentation (Avihai)
>
> include/exec/memory.h | 15 ++++++++++++++-
> hw/vfio/common.c | 13 +++++++++----
> hw/virtio/vhost-vdpa.c | 5 ++++-
> system/memory.c | 10 +++++-----
> 4 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index d417d7f363dbbca6553c449582a93d5da73cca40..9cdd64e9c69b63f9d27cebc2e8cb366e22ed7577 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -774,9 +774,22 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
> void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> RamDiscardListener *rdl);
>
> +/**
> + * memory_get_xlat_addr: Extract addresses from a TLB entry
> + *
> + * @iotlb: pointer to an #IOMMUTLBEntry
> + * @vaddr: virtual address
> + * @ram_addr: RAM address
> + * @read_only: indicates if writes are allowed
> + * @mr_has_discard_manager: indicates memory is controlled by a
> + * RamDiscardManager
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Return: true on success, else false setting @errp with error.
> + */
> bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> ram_addr_t *ram_addr, bool *read_only,
> - bool *mr_has_discard_manager);
> + bool *mr_has_discard_manager, Error **errp);
>
> typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4e2ef3d3034e72aa6a546bcb9ea1f31a0bbd5b1b..919c4c52bc1590fd6c0bda37ba5881f58ff2ffff 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -253,12 +253,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>
> /* Called with rcu_read_lock held. */
> static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> - ram_addr_t *ram_addr, bool *read_only)
> + ram_addr_t *ram_addr, bool *read_only,
> + Error **errp)
> {
> bool ret, mr_has_discard_manager;
>
> ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
> - &mr_has_discard_manager);
> + &mr_has_discard_manager, errp);
> if (ret && mr_has_discard_manager) {
> /*
> * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> @@ -288,6 +289,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> void *vaddr;
> int ret;
> + Error *local_err = NULL;
>
> trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
> iova, iova + iotlb->addr_mask);
> @@ -304,7 +306,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> bool read_only;
>
> - if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> + if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, &local_err)) {
> + error_report_err(local_err);
> goto out;
> }
> /*
> @@ -1213,6 +1216,7 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> VFIOContainerBase *bcontainer = giommu->bcontainer;
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> ram_addr_t translated_addr;
> + Error *local_err = NULL;
> int ret = -EINVAL;
>
> trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
> @@ -1224,7 +1228,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> }
>
> rcu_read_lock();
> - if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> + if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
> + error_report_err(local_err);
> goto out_unlock;
> }
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index e827b9175fc61f1ef419e48d90a440b00449312a..ed99ab87457d8f31b98ace960713f48d47b27102 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -208,6 +208,7 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> void *vaddr;
> int ret;
> Int128 llend;
> + Error *local_err = NULL;
>
> if (iotlb->target_as != &address_space_memory) {
> error_report("Wrong target AS \"%s\", only system memory is allowed",
> @@ -227,7 +228,9 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> bool read_only;
>
> - if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL)) {
> + if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL,
> + &local_err)) {
> + error_report_err(local_err);
> return;
> }
> ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova,
> diff --git a/system/memory.c b/system/memory.c
> index 642a449f8c867d38c62a748a4dfd5c055637c205..9540caa8a1f4da8501bf5ae9d7eedde8b775e1dc 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2179,7 +2179,7 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> /* Called with rcu_read_lock held. */
> bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> ram_addr_t *ram_addr, bool *read_only,
> - bool *mr_has_discard_manager)
> + bool *mr_has_discard_manager, Error **errp)
> {
> MemoryRegion *mr;
> hwaddr xlat;
> @@ -2197,7 +2197,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> mr = address_space_translate(&address_space_memory, iotlb->translated_addr,
> &xlat, &len, writable, MEMTXATTRS_UNSPECIFIED);
> if (!memory_region_is_ram(mr)) {
> - error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
> + error_setg(errp, "iommu map to non memory area %" HWADDR_PRIx "", xlat);
> return false;
> } else if (memory_region_has_ram_discard_manager(mr)) {
> RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> @@ -2216,8 +2216,8 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> * were already restored before IOMMUs are restored.
> */
> if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> - error_report("iommu map to discarded memory (e.g., unplugged via"
> - " virtio-mem): %" HWADDR_PRIx "",
> + error_setg(errp, "iommu map to discarded memory (e.g., unplugged"
> + " via virtio-mem): %" HWADDR_PRIx "",
> iotlb->translated_addr);
> return false;
> }
> @@ -2228,7 +2228,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> * check that it did not truncate too much.
> */
> if (len & iotlb->addr_mask) {
> - error_report("iommu has granularity incompatible with target AS");
> + error_setg(errp, "iommu has granularity incompatible with target AS");
> return false;
> }
>
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler
2024-05-14 15:31 ` [PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
2024-05-15 9:34 ` Eric Auger
@ 2024-05-16 8:42 ` Avihai Horon
2024-05-16 12:22 ` Cédric Le Goater
1 sibling, 1 reply; 29+ messages in thread
From: Avihai Horon @ 2024-05-16 8:42 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé,
Markus Armbruster
On 14/05/2024 18:31, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> Let the callers do the error reporting. Add documentation while at it.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v6:
>
> - Fixed the line wrapping (Avihai)
> - Fixed query_dirty_bitmap documentation (Avihai)
>
> Changes in v5:
>
> - Replaced error_setg() by error_setg_errno() in
> vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap()
> - ':' -> '-' in vfio_iommu_map_dirty_notify()
>
> include/hw/vfio/vfio-common.h | 4 +-
> include/hw/vfio/vfio-container-base.h | 21 +++++++--
> hw/vfio/common.c | 61 ++++++++++++++++++---------
> hw/vfio/container-base.c | 7 +--
> hw/vfio/container.c | 14 +++---
> 5 files changed, 72 insertions(+), 35 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 46f88493634b5634a9c14a5caa33a463fbf2c50d..68911d36676667352e94a97895828aff4b194b57 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -274,9 +274,9 @@ bool
> vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer);
> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> VFIOBitmap *vbmap, hwaddr iova,
> - hwaddr size);
> + hwaddr size, Error **errp);
[*] Can be wrapped like:
int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
Error **errp);
> int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
> - uint64_t size, ram_addr_t ram_addr);
> + uint64_t size, ram_addr_t ram_addr, Error **errp);
>
> /* Returns 0 on success, or a negative errno. */
> int vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 326ceea52a2030eec9dad289a9845866c4a8c090..42cfbf32dc737606c3f620d126f35d85d4833534 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -84,8 +84,8 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> bool start, Error **errp);
> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> - VFIOBitmap *vbmap,
> - hwaddr iova, hwaddr size);
> + VFIOBitmap *vbmap, hwaddr iova,
> + hwaddr size, Error **errp);
>
> void vfio_container_init(VFIOContainerBase *bcontainer,
> VFIOAddressSpace *space,
> @@ -138,9 +138,22 @@ struct VFIOIOMMUClass {
> */
> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
> bool start, Error **errp);
> + /**
> + * @query_dirty_bitmap
> + *
> + * Get bitmap of dirty pages from container
> + *
> + * @bcontainer: #VFIOContainerBase from which to get dirty pages
> + * @vbmap: #VFIOBitmap internal bitmap structure
> + * @iova: iova base address
> + * @size: size of iova range
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Returns zero to indicate success and negative for error
> + */
> int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
> - VFIOBitmap *vbmap,
> - hwaddr iova, hwaddr size);
> + VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
> + Error **errp);
> /* PCI specific */
> int (*pci_hot_reset)(VFIODevice *vbasedev, bool single);
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 919c4c52bc1590fd6c0bda37ba5881f58ff2ffff..9b5123d45fc1f9d5be4bbbf92898f89cd11e1363 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1140,8 +1140,8 @@ static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova,
> }
>
> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> - VFIOBitmap *vbmap, hwaddr iova,
> - hwaddr size)
> + VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
> + Error **errp)
> {
> VFIODevice *vbasedev;
> int ret;
> @@ -1150,10 +1150,10 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> ret = vfio_device_dma_logging_report(vbasedev, iova, size,
> vbmap->bitmap);
> if (ret) {
> - error_report("%s: Failed to get DMA logging report, iova: "
> - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx
> - ", err: %d (%s)",
> - vbasedev->name, iova, size, ret, strerror(-ret));
> + error_setg_errno(errp, -ret,
> + "%s: Failed to get DMA logging report, iova: "
> + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx,
> + vbasedev->name, iova, size);
>
> return ret;
> }
> @@ -1163,7 +1163,7 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> }
>
> int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
> - uint64_t size, ram_addr_t ram_addr)
> + uint64_t size, ram_addr_t ram_addr, Error **errp)
> {
> bool all_device_dirty_tracking =
> vfio_devices_all_device_dirty_tracking(bcontainer);
> @@ -1180,13 +1180,17 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
>
> ret = vfio_bitmap_alloc(&vbmap, size);
> if (ret) {
> + error_setg_errno(errp, -ret,
> + "Failed to allocate dirty tracking bitmap");
> return ret;
> }
>
> if (all_device_dirty_tracking) {
> - ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size);
> + ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size,
> + errp);
> } else {
> - ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size);
> + ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size,
> + errp);
> }
>
> if (ret) {
> @@ -1234,12 +1238,13 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> }
>
> ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
> - translated_addr);
> + translated_addr, &local_err);
> if (ret) {
> - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> - "0x%"HWADDR_PRIx") = %d (%s)",
> - bcontainer, iova, iotlb->addr_mask + 1, ret,
> - strerror(-ret));
> + error_prepend(&local_err,
> + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> + "0x%"HWADDR_PRIx") failed - ", bcontainer, iova,
> + iotlb->addr_mask + 1);
> + error_report_err(local_err);
> }
>
> out_unlock:
> @@ -1259,12 +1264,19 @@ static int vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section,
> const ram_addr_t ram_addr = memory_region_get_ram_addr(section->mr) +
> section->offset_within_region;
> VFIORamDiscardListener *vrdl = opaque;
> + Error *local_err = NULL;
> + int ret;
>
> /*
> * Sync the whole mapped region (spanning multiple individual mappings)
> * in one go.
> */
> - return vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr);
> + ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr,
> + &local_err);
> + if (ret) {
> + error_report_err(local_err);
> + }
> + return ret;
> }
>
> static int
> @@ -1296,7 +1308,7 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
> }
>
> static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
> - MemoryRegionSection *section)
> + MemoryRegionSection *section, Error **errp)
> {
> ram_addr_t ram_addr;
>
> @@ -1327,7 +1339,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
> }
> return 0;
> } else if (memory_region_has_ram_discard_manager(section->mr)) {
> - return vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section);
> + int ret;
> +
> + ret = vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section);
> + if (ret) {
> + error_setg(errp,
> + "Failed to sync dirty bitmap with RAM discard listener");
> + return ret;
> + }
Need to move "return ret" out of the "if (ret)" branch -- we need to
immediately return from the else if branch also on success.
With that,
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
(Nit: there are 2 more places where line wrapping can be improved, if
you want. Marked them with [*])
> }
>
> ram_addr = memory_region_get_ram_addr(section->mr) +
> @@ -1335,7 +1354,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>
> return vfio_get_dirty_bitmap(bcontainer,
> REAL_HOST_PAGE_ALIGN(section->offset_within_address_space),
> - int128_get64(section->size), ram_addr);
> + int128_get64(section->size), ram_addr, errp);
> }
>
> static void vfio_listener_log_sync(MemoryListener *listener,
> @@ -1344,16 +1363,16 @@ static void vfio_listener_log_sync(MemoryListener *listener,
> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> listener);
> int ret;
> + Error *local_err = NULL;
>
> if (vfio_listener_skipped_section(section)) {
> return;
> }
>
> if (vfio_devices_all_dirty_tracking(bcontainer)) {
> - ret = vfio_sync_dirty_bitmap(bcontainer, section);
> + ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
> if (ret) {
> - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
> - strerror(-ret));
> + error_report_err(local_err);
> vfio_set_migration_error(ret);
> }
> }
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 7c0764121d24b02b6c4e66e368d7dff78a6d65aa..4fa250c46fd16333d2e2358ede8b0a0afdb185b3 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -64,11 +64,12 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> }
>
> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> - VFIOBitmap *vbmap,
> - hwaddr iova, hwaddr size)
> + VFIOBitmap *vbmap, hwaddr iova,
> + hwaddr size, Error **errp)
> {
> g_assert(bcontainer->ops->query_dirty_bitmap);
> - return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size);
> + return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size,
> + errp);
> }
>
> void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space,
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index c35221fbe7dc5453050f97cd186fc958e24f28f7..e00db6546c652c61a352f8e4cd65b212ecfb65bc 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -130,6 +130,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
> };
> bool need_dirty_sync = false;
> int ret;
> + Error *local_err = NULL;
>
> if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) {
> if (!vfio_devices_all_device_dirty_tracking(bcontainer) &&
> @@ -165,8 +166,9 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
>
> if (need_dirty_sync) {
> ret = vfio_get_dirty_bitmap(bcontainer, iova, size,
> - iotlb->translated_addr);
> + iotlb->translated_addr, &local_err);
> if (ret) {
> + error_report_err(local_err);
> return ret;
> }
> }
> @@ -236,7 +238,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>
> static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> VFIOBitmap *vbmap,
> - hwaddr iova, hwaddr size)
> + hwaddr iova, hwaddr size,
> + Error **errp)
[*] Can be wrapped like:
static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase
*bcontainer,
VFIOBitmap *vbmap, hwaddr iova,
hwaddr size, Error **errp)
Thanks.
> {
> const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
> bcontainer);
> @@ -264,9 +267,10 @@ static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap);
> if (ret) {
> ret = -errno;
> - error_report("Failed to get dirty bitmap for iova: 0x%"PRIx64
> - " size: 0x%"PRIx64" err: %d", (uint64_t)range->iova,
> - (uint64_t)range->size, errno);
> + error_setg_errno(errp, errno,
> + "Failed to get dirty bitmap for iova: 0x%"PRIx64
> + " size: 0x%"PRIx64, (uint64_t)range->iova,
> + (uint64_t)range->size);
> }
>
> g_free(dbitmap);
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state()
2024-05-16 8:18 ` Avihai Horon
@ 2024-05-16 12:07 ` Cédric Le Goater
0 siblings, 0 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:07 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé,
Markus Armbruster
On 5/16/24 10:18, Avihai Horon wrote:
>
> On 14/05/2024 18:31, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Add an Error** argument to vfio_migration_set_state() and adjust
>> callers, including vfio_save_setup(). The error will be propagated up
>> to qemu_savevm_state_setup() where the save_setup() handler is
>> executed.
>>
>> Modify vfio_vmstate_change_prepare() and vfio_vmstate_change() to
>> store a reported error under the migration stream if a migration is in
>> progress.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>
> With/without Eric's suggestion:
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>
> Two nits below though (if you respin or want to amend inline).
>
>> ---
>>
>> Changes in v6:
>>
>> - Commit log improvements (Avihai)
>> - vfio_migration_set_state() : Dropped the error_setg_errno() change
>> when setting device in recover state fails (Avihai)
>> - vfio_migration_state_notifier() : report local error (Avihai)
>>
>> Changes in v5:
>>
>> - Replaced error_setg() by error_setg_errno() in vfio_migration_set_state()
>> - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>>
>> hw/vfio/migration.c | 77 ++++++++++++++++++++++++++++-----------------
>> 1 file changed, 48 insertions(+), 29 deletions(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..bf11135167ebb162dd41415656bdacfa0e1ca550 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -82,7 +82,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>>
>> static int vfio_migration_set_state(VFIODevice *vbasedev,
>> enum vfio_device_mig_state new_state,
>> - enum vfio_device_mig_state recover_state)
>> + enum vfio_device_mig_state recover_state,
>> + Error **errp)
>> {
>> VFIOMigration *migration = vbasedev->migration;
>> uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
>> @@ -102,18 +103,19 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>> ret = -errno;
>>
>> if (recover_state == VFIO_DEVICE_STATE_ERROR) {
>> - error_report("%s: Failed setting device state to %s, err: %s. "
>> - "Recover state is ERROR. Resetting device",
>> - vbasedev->name, mig_state_to_str(new_state),
>> - strerror(errno));
>> + error_setg_errno(errp, errno,
>> + "%s: Failed setting device state to %s. "
>> + "Recover state is ERROR. Resetting device",
>> + vbasedev->name, mig_state_to_str(new_state));
>>
>> goto reset_device;
>> }
>>
>> - error_report(
>> - "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s",
>> - vbasedev->name, mig_state_to_str(new_state),
>> - strerror(errno), mig_state_to_str(recover_state));
>> + error_setg_errno(errp, errno,
>> + "%s: Failed setting device state to %s. "
>> + "Setting device in recover state %s",
>> + vbasedev->name, mig_state_to_str(new_state),
>> + mig_state_to_str(recover_state));
>>
>> mig_state->device_state = recover_state;
>> if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
>> @@ -137,7 +139,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>> * This can happen if the device is asynchronously reset and
>> * terminates a data transfer.
>> */
>> - error_report("%s: data_fd out of sync", vbasedev->name);
>> + error_setg(errp, "%s: data_fd out of sync", vbasedev->name);
>> close(mig_state->data_fd);
>>
>> return -EBADF;
>> @@ -168,10 +170,11 @@ reset_device:
>> */
>> static int
>> vfio_migration_set_state_or_reset(VFIODevice *vbasedev,
>> - enum vfio_device_mig_state new_state)
>> + enum vfio_device_mig_state new_state,
>> + Error **errp)
>> {
>> return vfio_migration_set_state(vbasedev, new_state,
>> - VFIO_DEVICE_STATE_ERROR);
>> + VFIO_DEVICE_STATE_ERROR, errp);
>> }
>>
>> static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
>> @@ -399,10 +402,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
>> switch (migration->device_state) {
>> case VFIO_DEVICE_STATE_RUNNING:
>> ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
>> - VFIO_DEVICE_STATE_RUNNING);
>> + VFIO_DEVICE_STATE_RUNNING, errp);
>> if (ret) {
>> - error_setg(errp, "%s: Failed to set new PRE_COPY state",
>> - vbasedev->name);
>> return ret;
>> }
>>
>> @@ -435,13 +436,20 @@ static void vfio_save_cleanup(void *opaque)
>> {
>> VFIODevice *vbasedev = opaque;
>> VFIOMigration *migration = vbasedev->migration;
>> + Error *local_err = NULL;
>> + int ret;
>>
>> /*
>> * Changing device state from STOP_COPY to STOP can take time. Do it here,
>> * after migration has completed, so it won't increase downtime.
>> */
>> if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) {
>> - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP);
>> + ret = vfio_migration_set_state_or_reset(vbasedev,
>> + VFIO_DEVICE_STATE_STOP,
>> + &local_err);
>> + if (ret) {
>> + error_report_err(local_err);
>> + }
>> }
>>
>> g_free(migration->data_buffer);
>> @@ -549,11 +557,13 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>> VFIODevice *vbasedev = opaque;
>> ssize_t data_size;
>> int ret;
>> + Error *local_err = NULL;
>>
>> /* We reach here with device state STOP or STOP_COPY only */
>> ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
>> - VFIO_DEVICE_STATE_STOP);
>> + VFIO_DEVICE_STATE_STOP, &local_err);
>> if (ret) {
>> + error_report_err(local_err);
>> return ret;
>> }
>>
>> @@ -591,14 +601,9 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>> static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
>> {
>> VFIODevice *vbasedev = opaque;
>> - int ret;
>>
>> - ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
>> - vbasedev->migration->device_state);
>> - if (ret) {
>> - error_setg(errp, "%s: Failed to set RESUMING state", vbasedev->name);
>> - }
>> - return ret;
>> + return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
>> + vbasedev->migration->device_state, errp);
>> }
>>
>> static int vfio_load_cleanup(void *opaque)
>> @@ -714,19 +719,20 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>> VFIODevice *vbasedev = opaque;
>> VFIOMigration *migration = vbasedev->migration;
>> enum vfio_device_mig_state new_state;
>> + Error *local_err = NULL;
>> int ret;
>>
>> new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
>> VFIO_DEVICE_STATE_PRE_COPY_P2P :
>> VFIO_DEVICE_STATE_RUNNING_P2P;
>>
>> - ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
>> + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
>> if (ret) {
>> /*
>> * Migration should be aborted in this case, but vm_state_notify()
>> * currently does not support reporting failures.
>> */
>> - migration_file_set_error(ret, NULL);
>> + migration_file_set_error(ret, local_err);
>> }
>>
>> trace_vfio_vmstate_change_prepare(vbasedev->name, running,
>> @@ -738,6 +744,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>> {
>> VFIODevice *vbasedev = opaque;
>> enum vfio_device_mig_state new_state;
>> + Error *local_err = NULL;
>> int ret;
>>
>> if (running) {
>> @@ -750,13 +757,13 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>> VFIO_DEVICE_STATE_STOP;
>> }
>>
>> - ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
>> + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
>> if (ret) {
>> /*
>> * Migration should be aborted in this case, but vm_state_notify()
>> * currently does not support reporting failures.
>> */
>> - migration_file_set_error(ret, NULL);
>> + migration_file_set_error(ret, local_err);
>> }
>>
>> trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
>> @@ -769,11 +776,23 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
>> VFIOMigration *migration = container_of(notifier, VFIOMigration,
>> migration_state);
>> VFIODevice *vbasedev = migration->vbasedev;
>> + Error *local_err = NULL;
>> + int ret = 0;
>
> Nit: No need to set ret to 0.
>
>>
>> trace_vfio_migration_state_notifier(vbasedev->name, e->type);
>>
>> if (e->type == MIG_EVENT_PRECOPY_FAILED) {
>> - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
>> + /*
>> + * MigrationNotifyFunc may return an error code and an Error
>> + * object only for MIG_EVENT_PRECOPY_SETUP. Hence, report the
>> + * error locally and ignore the errp argument.
>> + */
>
> Nit: maybe rephrase to the following:
>
> "MigrationNotifyFunc may not return an error code and an Error object for MIG_EVENT_PRECOPY_FAILED. Hence ..."
>
> (This way, this comment will not become stale if new MIG_EVENTs that are allowed to return error are added)
ok. done. There will be a (final) v7.
Thanks,
C.
> Thanks.
>
>> + ret = vfio_migration_set_state_or_reset(vbasedev,
>> + VFIO_DEVICE_STATE_RUNNING,
>> + &local_err);
>> + if (ret) {
>> + error_report_err(local_err);
>> + }
>> }
>> return 0;
>> }
>> --
>> 2.45.0
>>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler
2024-05-16 8:42 ` Avihai Horon
@ 2024-05-16 12:22 ` Cédric Le Goater
0 siblings, 0 replies; 29+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:22 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé,
Markus Armbruster
On 5/16/24 10:42, Avihai Horon wrote:
>
> On 14/05/2024 18:31, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Let the callers do the error reporting. Add documentation while at it.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>> Changes in v6:
>>
>> - Fixed the line wrapping (Avihai)
>> - Fixed query_dirty_bitmap documentation (Avihai)
>>
>> Changes in v5:
>>
>> - Replaced error_setg() by error_setg_errno() in
>> vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap()
>> - ':' -> '-' in vfio_iommu_map_dirty_notify()
>>
>> include/hw/vfio/vfio-common.h | 4 +-
>> include/hw/vfio/vfio-container-base.h | 21 +++++++--
>> hw/vfio/common.c | 61 ++++++++++++++++++---------
>> hw/vfio/container-base.c | 7 +--
>> hw/vfio/container.c | 14 +++---
>> 5 files changed, 72 insertions(+), 35 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 46f88493634b5634a9c14a5caa33a463fbf2c50d..68911d36676667352e94a97895828aff4b194b57 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -274,9 +274,9 @@ bool
>> vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer);
>> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> VFIOBitmap *vbmap, hwaddr iova,
>> - hwaddr size);
>> + hwaddr size, Error **errp);
>
> [*] Can be wrapped like:
>
> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
> Error **errp);
OK. So I changed all *_dirty_bitmap() prototypes.
>> int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
>> - uint64_t size, ram_addr_t ram_addr);
>> + uint64_t size, ram_addr_t ram_addr, Error **errp);
>>
>> /* Returns 0 on success, or a negative errno. */
>> int vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
>> index 326ceea52a2030eec9dad289a9845866c4a8c090..42cfbf32dc737606c3f620d126f35d85d4833534 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -84,8 +84,8 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>> bool start, Error **errp);
>> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> - VFIOBitmap *vbmap,
>> - hwaddr iova, hwaddr size);
>> + VFIOBitmap *vbmap, hwaddr iova,
>> + hwaddr size, Error **errp);
>>
>> void vfio_container_init(VFIOContainerBase *bcontainer,
>> VFIOAddressSpace *space,
>> @@ -138,9 +138,22 @@ struct VFIOIOMMUClass {
>> */
>> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>> bool start, Error **errp);
>> + /**
>> + * @query_dirty_bitmap
>> + *
>> + * Get bitmap of dirty pages from container
>> + *
>> + * @bcontainer: #VFIOContainerBase from which to get dirty pages
>> + * @vbmap: #VFIOBitmap internal bitmap structure
>> + * @iova: iova base address
>> + * @size: size of iova range
>> + * @errp: pointer to Error*, to store an error if it happens.
>> + *
>> + * Returns zero to indicate success and negative for error
>> + */
>> int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
>> - VFIOBitmap *vbmap,
>> - hwaddr iova, hwaddr size);
>> + VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
>> + Error **errp);
>> /* PCI specific */
>> int (*pci_hot_reset)(VFIODevice *vbasedev, bool single);
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 919c4c52bc1590fd6c0bda37ba5881f58ff2ffff..9b5123d45fc1f9d5be4bbbf92898f89cd11e1363 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1140,8 +1140,8 @@ static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova,
>> }
>>
>> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> - VFIOBitmap *vbmap, hwaddr iova,
>> - hwaddr size)
>> + VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
>> + Error **errp)
>> {
>> VFIODevice *vbasedev;
>> int ret;
>> @@ -1150,10 +1150,10 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> ret = vfio_device_dma_logging_report(vbasedev, iova, size,
>> vbmap->bitmap);
>> if (ret) {
>> - error_report("%s: Failed to get DMA logging report, iova: "
>> - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx
>> - ", err: %d (%s)",
>> - vbasedev->name, iova, size, ret, strerror(-ret));
>> + error_setg_errno(errp, -ret,
>> + "%s: Failed to get DMA logging report, iova: "
>> + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx,
>> + vbasedev->name, iova, size);
>>
>> return ret;
>> }
>> @@ -1163,7 +1163,7 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> }
>>
>> int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
>> - uint64_t size, ram_addr_t ram_addr)
>> + uint64_t size, ram_addr_t ram_addr, Error **errp)
>> {
>> bool all_device_dirty_tracking =
>> vfio_devices_all_device_dirty_tracking(bcontainer);
>> @@ -1180,13 +1180,17 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
>>
>> ret = vfio_bitmap_alloc(&vbmap, size);
>> if (ret) {
>> + error_setg_errno(errp, -ret,
>> + "Failed to allocate dirty tracking bitmap");
>> return ret;
>> }
>>
>> if (all_device_dirty_tracking) {
>> - ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size);
>> + ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size,
>> + errp);
>> } else {
>> - ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size);
>> + ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size,
>> + errp);
>> }
>>
>> if (ret) {
>> @@ -1234,12 +1238,13 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> }
>>
>> ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
>> - translated_addr);
>> + translated_addr, &local_err);
>> if (ret) {
>> - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>> - "0x%"HWADDR_PRIx") = %d (%s)",
>> - bcontainer, iova, iotlb->addr_mask + 1, ret,
>> - strerror(-ret));
>> + error_prepend(&local_err,
>> + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>> + "0x%"HWADDR_PRIx") failed - ", bcontainer, iova,
>> + iotlb->addr_mask + 1);
>> + error_report_err(local_err);
>> }
>>
>> out_unlock:
>> @@ -1259,12 +1264,19 @@ static int vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section,
>> const ram_addr_t ram_addr = memory_region_get_ram_addr(section->mr) +
>> section->offset_within_region;
>> VFIORamDiscardListener *vrdl = opaque;
>> + Error *local_err = NULL;
>> + int ret;
>>
>> /*
>> * Sync the whole mapped region (spanning multiple individual mappings)
>> * in one go.
>> */
>> - return vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr);
>> + ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr,
>> + &local_err);
>> + if (ret) {
>> + error_report_err(local_err);
>> + }
>> + return ret;
>> }
>>
>> static int
>> @@ -1296,7 +1308,7 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
>> }
>>
>> static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>> - MemoryRegionSection *section)
>> + MemoryRegionSection *section, Error **errp)
>> {
>> ram_addr_t ram_addr;
>>
>> @@ -1327,7 +1339,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>> }
>> return 0;
>> } else if (memory_region_has_ram_discard_manager(section->mr)) {
>> - return vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section);
>> + int ret;
>> +
>> + ret = vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section);
>> + if (ret) {
>> + error_setg(errp,
>> + "Failed to sync dirty bitmap with RAM discard listener");
>> + return ret;
>> + }
>
> Need to move "return ret" out of the "if (ret)" branch -- we need to immediately return from the else if branch also on success.
drat. Good catch. This deserves a v7.
Thanks,
C.
> With that,
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>
> (Nit: there are 2 more places where line wrapping can be improved, if you want. Marked them with [*])
>
>> }
>>
>> ram_addr = memory_region_get_ram_addr(section->mr) +
>> @@ -1335,7 +1354,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>>
>> return vfio_get_dirty_bitmap(bcontainer,
>> REAL_HOST_PAGE_ALIGN(section->offset_within_address_space),
>> - int128_get64(section->size), ram_addr);
>> + int128_get64(section->size), ram_addr, errp);
>> }
>>
>> static void vfio_listener_log_sync(MemoryListener *listener,
>> @@ -1344,16 +1363,16 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>> listener);
>> int ret;
>> + Error *local_err = NULL;
>>
>> if (vfio_listener_skipped_section(section)) {
>> return;
>> }
>>
>> if (vfio_devices_all_dirty_tracking(bcontainer)) {
>> - ret = vfio_sync_dirty_bitmap(bcontainer, section);
>> + ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
>> if (ret) {
>> - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
>> - strerror(-ret));
>> + error_report_err(local_err);
>> vfio_set_migration_error(ret);
>> }
>> }
>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>> index 7c0764121d24b02b6c4e66e368d7dff78a6d65aa..4fa250c46fd16333d2e2358ede8b0a0afdb185b3 100644
>> --- a/hw/vfio/container-base.c
>> +++ b/hw/vfio/container-base.c
>> @@ -64,11 +64,12 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>> }
>>
>> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> - VFIOBitmap *vbmap,
>> - hwaddr iova, hwaddr size)
>> + VFIOBitmap *vbmap, hwaddr iova,
>> + hwaddr size, Error **errp)
>> {
>> g_assert(bcontainer->ops->query_dirty_bitmap);
>> - return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size);
>> + return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size,
>> + errp);
>> }
>>
>> void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space,
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index c35221fbe7dc5453050f97cd186fc958e24f28f7..e00db6546c652c61a352f8e4cd65b212ecfb65bc 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -130,6 +130,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
>> };
>> bool need_dirty_sync = false;
>> int ret;
>> + Error *local_err = NULL;
>>
>> if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) {
>> if (!vfio_devices_all_device_dirty_tracking(bcontainer) &&
>> @@ -165,8 +166,9 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
>>
>> if (need_dirty_sync) {
>> ret = vfio_get_dirty_bitmap(bcontainer, iova, size,
>> - iotlb->translated_addr);
>> + iotlb->translated_addr, &local_err);
>> if (ret) {
>> + error_report_err(local_err);
>> return ret;
>> }
>> }
>> @@ -236,7 +238,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>>
>> static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> VFIOBitmap *vbmap,
>> - hwaddr iova, hwaddr size)
>> + hwaddr iova, hwaddr size,
>> + Error **errp)
>
> [*] Can be wrapped like:
>
> static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> VFIOBitmap *vbmap, hwaddr iova,
> hwaddr size, Error **errp)
>
> Thanks.
>
>> {
>> const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>> bcontainer);
>> @@ -264,9 +267,10 @@ static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap);
>> if (ret) {
>> ret = -errno;
>> - error_report("Failed to get dirty bitmap for iova: 0x%"PRIx64
>> - " size: 0x%"PRIx64" err: %d", (uint64_t)range->iova,
>> - (uint64_t)range->size, errno);
>> + error_setg_errno(errp, errno,
>> + "Failed to get dirty bitmap for iova: 0x%"PRIx64
>> + " size: 0x%"PRIx64, (uint64_t)range->iova,
>> + (uint64_t)range->size);
>> }
>>
>> g_free(dbitmap);
>> --
>> 2.45.0
>>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2024-05-16 12:24 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-14 15:31 [PATCH v6 0/9] vfio: Improve error reporting (part 2 Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
2024-05-15 6:40 ` Eric Auger
2024-05-15 16:55 ` Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
2024-05-15 6:53 ` Eric Auger
2024-05-15 17:09 ` Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 3/9] migration: Extend migration_file_set_error() with Error* argument Cédric Le Goater
2024-05-15 7:04 ` Eric Auger
2024-05-15 17:15 ` Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state() Cédric Le Goater
2024-05-15 7:20 ` Eric Auger
2024-05-16 7:17 ` Cédric Le Goater
2024-05-16 8:18 ` Avihai Horon
2024-05-16 12:07 ` Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
2024-05-15 9:20 ` Eric Auger
2024-05-16 8:22 ` Avihai Horon
2024-05-14 15:31 ` [PATCH v6 6/9] vfio: Reverse test on vfio_get_xlat_addr() Cédric Le Goater
2024-05-15 9:22 ` Eric Auger
2024-05-14 15:31 ` [PATCH v6 7/9] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
2024-05-15 9:25 ` Eric Auger
2024-05-16 8:25 ` Avihai Horon
2024-05-14 15:31 ` [PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
2024-05-15 9:34 ` Eric Auger
2024-05-16 8:42 ` Avihai Horon
2024-05-16 12:22 ` Cédric Le Goater
2024-05-14 15:31 ` [PATCH v6 9/9] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
2024-05-15 9:34 ` Eric Auger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).