* [PATCH v7 0/9] vfio: Improve error reporting (part 2)
@ 2024-05-16 12:46 Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
` (9 more replies)
0 siblings, 10 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, 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 v7:
- Commit log improvements (Eric)
- vfio_set_migration_error() : err -> ret rename (Eric)
- vfio_migration_set_state() :
Introduced an error prefix to remove redundancy in error messages (Eric)
Commented error_report when setting the device in recover state fails (Eric)
- vfio_migration_state_notifier() :
Remove useless assignment of local ret variable (Avihai)
Rephrased comment regarding MigrationNotifyFunc API (Avihai)
- Fixed even more line wrapping of *dirty_bitmap() routines (Avihai)
- vfio_sync_dirty_bitmap()
Fixed return when vfio_sync_ram_discard_listener_dirty_bitmap() is called (Avihai)
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 | 30 ++++++-
include/hw/vfio/vfio-container-base.h | 37 +++++++--
include/migration/misc.h | 2 +-
hw/vfio/common.c | 113 ++++++++++++++++----------
hw/vfio/container-base.c | 10 +--
hw/vfio/container.c | 20 +++--
hw/vfio/migration.c | 109 ++++++++++++++++---------
hw/vfio/pci.c | 5 +-
hw/virtio/vhost-vdpa.c | 5 +-
migration/migration.c | 6 +-
system/memory.c | 10 +--
12 files changed, 246 insertions(+), 116 deletions(-)
--
2.45.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v7 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
@ 2024-05-16 12:46 ` Cédric Le Goater
2024-05-29 6:26 ` Markus Armbruster
2024-05-16 12:46 ` [PATCH v7 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
` (8 subsequent siblings)
9 siblings, 1 reply; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
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] 14+ messages in thread
* [PATCH v7 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start()
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
@ 2024-05-16 12:46 ` Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 3/9] migration: Extend migration_file_set_error() with Error* argument Cédric Le Goater
` (7 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, 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.
Also, errors from vfio_container_set_dirty_page_tracking() are now
collected and reported.
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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v7:
- Commit log improvements (Eric)
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] 14+ messages in thread
* [PATCH v7 3/9] migration: Extend migration_file_set_error() with Error* argument
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
@ 2024-05-16 12:46 ` Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state() Cédric Le Goater
` (6 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v7:
- vfio_set_migration_error() err -> ret rename (Eric)
include/migration/misc.h | 2 +-
hw/vfio/common.c | 4 ++--
hw/vfio/migration.c | 4 ++--
migration/migration.c | 6 ++++--
4 files changed, 9 insertions(+), 7 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..2c97de6c730d963d961bf81c0831326c0e25afa7 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -147,10 +147,10 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
return vbasedev->bcontainer->space->as != &address_space_memory;
}
-static void vfio_set_migration_error(int err)
+static void vfio_set_migration_error(int ret)
{
if (migration_is_setup_or_active()) {
- migration_file_set_error(err);
+ migration_file_set_error(ret, 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] 14+ messages in thread
* [PATCH v7 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state()
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
` (2 preceding siblings ...)
2024-05-16 12:46 ` [PATCH v7 3/9] migration: Extend migration_file_set_error() with Error* argument Cédric Le Goater
@ 2024-05-16 12:46 ` Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
` (5 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, 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.
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v7:
- vfio_migration_set_state() :
Introduced an error prefix to remove redundancy in error messages (Eric)
Commented error_report when setting the device in recover state fails (Eric)
- vfio_migration_state_notifier() :
Remove useless assignment of local ret variable (Avihai)
Rephrased comment regarding MigrationNotifyFunc API (Avihai)
hw/vfio/migration.c | 81 +++++++++++++++++++++++++++++----------------
1 file changed, 52 insertions(+), 29 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..43fed0dbdbe3415ae2dd68fbe45b302b85a80fa4 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) +
@@ -92,6 +93,9 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
struct vfio_device_feature_mig_state *mig_state =
(struct vfio_device_feature_mig_state *)feature->data;
int ret;
+ g_autofree char *error_prefix =
+ g_strdup_printf("%s: Failed setting device state to %s.",
+ vbasedev->name, mig_state_to_str(new_state));
feature->argsz = sizeof(buf);
feature->flags =
@@ -102,22 +106,24 @@ 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 Recover state is ERROR. Resetting device",
+ error_prefix);
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 Setting device in recover state %s",
+ error_prefix, mig_state_to_str(recover_state));
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));
@@ -137,7 +143,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 +174,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 +406,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 +440,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 +561,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 +605,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 +723,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 +748,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 +761,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 +780,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;
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 not return an error code and an Error
+ * object for MIG_EVENT_PRECOPY_FAILED. 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] 14+ messages in thread
* [PATCH v7 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
` (3 preceding siblings ...)
2024-05-16 12:46 ` [PATCH v7 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state() Cédric Le Goater
@ 2024-05-16 12:46 ` Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 6/9] vfio: Reverse test on vfio_get_xlat_addr() Cédric Le Goater
` (4 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
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 d66e27db02a4db8329204f88d02a204eedf1caa1..3ff633ad3b395e953a55683f5f0308bca50af3dd 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 43fed0dbdbe3415ae2dd68fbe45b302b85a80fa4..5d91364f3bbc34060d84b4b4b1823eadbc7b12bf 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -193,21 +193,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)
@@ -592,13 +601,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] 14+ messages in thread
* [PATCH v7 6/9] vfio: Reverse test on vfio_get_xlat_addr()
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
` (4 preceding siblings ...)
2024-05-16 12:46 ` [PATCH v7 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
@ 2024-05-16 12:46 ` Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 7/9] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
` (3 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, Philippe Mathieu-Daudé, Markus Armbruster,
Cédric Le Goater
It will simplify the changes coming after.
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
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 2c97de6c730d963d961bf81c0831326c0e25afa7..c7f274fb5c851e4c44498552891265018d2c5313 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] 14+ messages in thread
* [PATCH v7 7/9] memory: Add Error** argument to memory_get_xlat_addr()
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
` (5 preceding siblings ...)
2024-05-16 12:46 ` [PATCH v7 6/9] vfio: Reverse test on vfio_get_xlat_addr() Cédric Le Goater
@ 2024-05-16 12:46 ` Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
` (2 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
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 c7f274fb5c851e4c44498552891265018d2c5313..7313043f1d161ed0326b5ba3fa1085608eaf6740 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] 14+ messages in thread
* [PATCH v7 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
` (6 preceding siblings ...)
2024-05-16 12:46 ` [PATCH v7 7/9] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
@ 2024-05-16 12:46 ` Cédric Le Goater
2024-05-16 14:56 ` Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 9/9] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
2024-05-16 16:22 ` [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
9 siblings, 1 reply; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, Philippe Mathieu-Daudé, Markus Armbruster,
Cédric Le Goater
Let the callers do the error reporting. Add documentation while at it.
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v7:
- Fixed even more line wrapping of *dirty_bitmap() routines (Avihai)
- vfio_sync_dirty_bitmap()
Fixed return when vfio_sync_ram_discard_listener_dirty_bitmap() is called (Avihai)
include/hw/vfio/vfio-common.h | 5 +--
include/hw/vfio/vfio-container-base.h | 19 +++++++--
hw/vfio/common.c | 60 +++++++++++++++++----------
hw/vfio/container-base.c | 6 +--
hw/vfio/container.c | 14 ++++---
5 files changed, 67 insertions(+), 37 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 3ff633ad3b395e953a55683f5f0308bca50af3dd..b6ac24953667bc5f72f28480a6bf0f4722069cb9 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -273,10 +273,9 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer);
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);
+ 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..b04057ad1aff73d974ecec718d0fe45f7a930b59 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -84,8 +84,7 @@ 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 +137,21 @@ 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 7313043f1d161ed0326b5ba3fa1085608eaf6740..21910802c0c58a0efdb07d31c5a709660e89e328 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1140,8 +1140,7 @@ 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 +1149,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 +1162,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 +1179,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 +1237,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 +1263,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 +1307,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 +1338,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 +1353,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 +1362,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..26f4bb464a720c9895b35c7c9e01c84d6322c3c9 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -64,11 +64,11 @@ 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..9534120d4ac835bb58e37667dad8d39205404c08 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;
}
}
@@ -235,8 +237,7 @@ 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)
+ VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp)
{
const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
bcontainer);
@@ -264,9 +265,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] 14+ messages in thread
* [PATCH v7 9/9] vfio: Also trace event failures in vfio_save_complete_precopy()
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
` (7 preceding siblings ...)
2024-05-16 12:46 ` [PATCH v7 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
@ 2024-05-16 12:46 ` Cédric Le Goater
2024-05-16 16:22 ` [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
9 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 12:46 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, 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>
Reviewed-by: Eric Auger <eric.auger@redhat.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 5d91364f3bbc34060d84b4b4b1823eadbc7b12bf..c4403a38ddb5e7e09fbcde0ad4132653ecaf0d24 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -589,9 +589,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] 14+ messages in thread
* Re: [PATCH v7 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler
2024-05-16 12:46 ` [PATCH v7 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
@ 2024-05-16 14:56 ` Cédric Le Goater
0 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 14:56 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, Philippe Mathieu-Daudé, Markus Armbruster
On 5/16/24 14:46, Cédric Le Goater wrote:
> Let the callers do the error reporting. Add documentation while at it.
>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v7:
>
> - Fixed even more line wrapping of *dirty_bitmap() routines (Avihai)
> - vfio_sync_dirty_bitmap()
> Fixed return when vfio_sync_ram_discard_listener_dirty_bitmap() is called (Avihai)
>
> include/hw/vfio/vfio-common.h | 5 +--
> include/hw/vfio/vfio-container-base.h | 19 +++++++--
> hw/vfio/common.c | 60 +++++++++++++++++----------
> hw/vfio/container-base.c | 6 +--
> hw/vfio/container.c | 14 ++++---
> 5 files changed, 67 insertions(+), 37 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 3ff633ad3b395e953a55683f5f0308bca50af3dd..b6ac24953667bc5f72f28480a6bf0f4722069cb9 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -273,10 +273,9 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer);
> 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);
> + 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..b04057ad1aff73d974ecec718d0fe45f7a930b59 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -84,8 +84,7 @@ 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 +137,21 @@ 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 7313043f1d161ed0326b5ba3fa1085608eaf6740..21910802c0c58a0efdb07d31c5a709660e89e328 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1140,8 +1140,7 @@ 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 +1149,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 +1162,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 +1179,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 +1237,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 +1263,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 +1307,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 +1338,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;
sigh. I missed that change, although I said it was done in the commit log :/
Hopefully I caught it and will fix inline. Sorry about that.
Thanks,
C.
> + }
> }
>
> ram_addr = memory_region_get_ram_addr(section->mr) +
> @@ -1335,7 +1353,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 +1362,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..26f4bb464a720c9895b35c7c9e01c84d6322c3c9 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -64,11 +64,11 @@ 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..9534120d4ac835bb58e37667dad8d39205404c08 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;
> }
> }
> @@ -235,8 +237,7 @@ 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)
> + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp)
> {
> const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
> bcontainer);
> @@ -264,9 +265,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);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 0/9] vfio: Improve error reporting (part 2)
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
` (8 preceding siblings ...)
2024-05-16 12:46 ` [PATCH v7 9/9] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
@ 2024-05-16 16:22 ` Cédric Le Goater
9 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-16 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson, Avihai Horon,
Eric Auger, Philippe Mathieu-Daudé, Markus Armbruster
On 5/16/24 14:46, Cédric Le Goater wrote:
> 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 v7:
>
> - Commit log improvements (Eric)
> - vfio_set_migration_error() : err -> ret rename (Eric)
> - vfio_migration_set_state() :
> Introduced an error prefix to remove redundancy in error messages (Eric)
> Commented error_report when setting the device in recover state fails (Eric)
> - vfio_migration_state_notifier() :
> Remove useless assignment of local ret variable (Avihai)
> Rephrased comment regarding MigrationNotifyFunc API (Avihai)
> - Fixed even more line wrapping of *dirty_bitmap() routines (Avihai)
> - vfio_sync_dirty_bitmap()
> Fixed return when vfio_sync_ram_discard_listener_dirty_bitmap() is called (Avihai)
I fixed this last issue as commented in patch 8. Let's address other
issues, if minor, with followup patches.
Applied to vfio-next.
Thanks,
C.
>
> 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 | 30 ++++++-
> include/hw/vfio/vfio-container-base.h | 37 +++++++--
> include/migration/misc.h | 2 +-
> hw/vfio/common.c | 113 ++++++++++++++++----------
> hw/vfio/container-base.c | 10 +--
> hw/vfio/container.c | 20 +++--
> hw/vfio/migration.c | 109 ++++++++++++++++---------
> hw/vfio/pci.c | 5 +-
> hw/virtio/vhost-vdpa.c | 5 +-
> migration/migration.c | 6 +-
> system/memory.c | 10 +--
> 12 files changed, 246 insertions(+), 116 deletions(-)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-16 12:46 ` [PATCH v7 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
@ 2024-05-29 6:26 ` Markus Armbruster
2024-05-29 9:45 ` Cédric Le Goater
0 siblings, 1 reply; 14+ messages in thread
From: Markus Armbruster @ 2024-05-29 6:26 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-devel, Peter Xu, Fabiano Rosas, Alex Williamson,
Avihai Horon, Eric Auger, Philippe Mathieu-Daudé
I had a look at this before I realized it's already in. I'm sending
this out not to demand any change, but only to point out an issue to be
avoided in future work.
Cédric Le Goater <clg@redhat.com> writes:
> 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>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> 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);
Note for later: this is always called via
vfio_container_set_dirty_page_tracking().
> 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) {
Note for later: all callers pass NULL to ignore the new Error.
> 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);
Silent improvement: errno is now reported symbolically (like "Operation
not permitted") instead of numerically (like "1"). Worth mentioning in
the commit message.
Since the callers pass NULL to ignore the Error, this error condition is
now silent, I believe.
I figure you correct this in later patches. If we accept temporary loss
of error reporting, the commit message should mention it. Loss of error
reporting is easy enough to avoid, though: have the callers pass a
pointer to a local @err, and on failure report it with
error_report_err().
> }
>
> return ret;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-29 6:26 ` Markus Armbruster
@ 2024-05-29 9:45 ` Cédric Le Goater
0 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2024-05-29 9:45 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Peter Xu, Fabiano Rosas, Alex Williamson,
Avihai Horon, Eric Auger, Philippe Mathieu-Daudé
On 5/29/24 08:26, Markus Armbruster wrote:
> I had a look at this before I realized it's already in. I'm sending
> this out not to demand any change, but only to point out an issue to be
> avoided in future work.
>
> Cédric Le Goater <clg@redhat.com> writes:
>
>> 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>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>> 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);
>
> Note for later: this is always called via
> vfio_container_set_dirty_page_tracking().
>
>> 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) {
>
> Note for later: all callers pass NULL to ignore the new Error.
>
>> 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);
>
> Silent improvement: errno is now reported symbolically (like "Operation
> not permitted") instead of numerically (like "1"). Worth mentioning in
> the commit message.
ok.
> Since the callers pass NULL to ignore the Error, this error condition is
> now silent, I believe.
>
> I figure you correct this in later patches. If we accept temporary loss
> of error reporting, the commit message should mention it. Loss of error
> reporting is easy enough to avoid, though: have the callers pass a
> pointer to a local @err, and on failure report it with
> error_report_err().
Yes. The following patch is changing the NULL to an Error pointer. Indeed,
I could have merged both patches and resend. Seemed overkill at the time
and I preferred the changes to get more exposure too.
I did it the way you propose in some of the other patches. I think the
code of this series was reshuffled a few times and it fell through the
cracks. Let's add some laziness on top of that to be honest.
Thanks,
C.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-05-29 9:46 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-16 12:46 [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
2024-05-29 6:26 ` Markus Armbruster
2024-05-29 9:45 ` Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 3/9] migration: Extend migration_file_set_error() with Error* argument Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state() Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 6/9] vfio: Reverse test on vfio_get_xlat_addr() Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 7/9] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
2024-05-16 14:56 ` Cédric Le Goater
2024-05-16 12:46 ` [PATCH v7 9/9] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
2024-05-16 16:22 ` [PATCH v7 0/9] vfio: Improve error reporting (part 2) Cédric Le Goater
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).