* [PATCH v5 00/10] vfio: Improve error reporting (part 2)
@ 2024-05-06 9:20 Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
` (9 more replies)
0 siblings, 10 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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 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 (10):
vfio: Add Error** argument to .set_dirty_page_tracking() handler
vfio: Add Error** argument to vfio_devices_dma_logging_start()
vfio: Extend migration_file_set_error() with Error** argument
vfio: Use new Error** argument in vfio_save_setup()
vfio: Add Error** argument to .vfio_save_config() handler
vfio: Reverse test on vfio_get_dirty_bitmap()
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()
vfio: Extend vfio_set_migration_error() with Error* argument
include/exec/memory.h | 15 ++-
include/hw/vfio/vfio-common.h | 29 +++++-
include/hw/vfio/vfio-container-base.h | 35 ++++++-
include/migration/misc.h | 2 +-
hw/vfio/common.c | 137 ++++++++++++++++----------
hw/vfio/container-base.c | 9 +-
hw/vfio/container.c | 20 ++--
hw/vfio/migration.c | 97 ++++++++++--------
hw/vfio/pci.c | 5 +-
hw/virtio/vhost-vdpa.c | 5 +-
migration/migration.c | 6 +-
system/memory.c | 10 +-
12 files changed, 244 insertions(+), 126 deletions(-)
--
2.45.0
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 13:03 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 02/10] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
` (8 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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>
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] 37+ messages in thread
* [PATCH v5 02/10] vfio: Add Error** argument to vfio_devices_dma_logging_start()
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 13:08 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 03/10] vfio: Extend migration_file_set_error() with Error** argument Cédric Le Goater
` (7 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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 detected when device level logging is started will be
propagated up to qemu_savevm_state_setup() when the ram save_setup()
handler is executed.
The vfio_set_migration_error() call becomes redundant in
vfio_devices_dma_logging_start(). Remove it.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
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] 37+ messages in thread
* [PATCH v5 03/10] vfio: Extend migration_file_set_error() with Error** argument
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 02/10] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 13:14 ` Avihai Horon
2024-05-13 22:19 ` Fabiano Rosas
2024-05-06 9:20 ` [PATCH v5 04/10] vfio: Use new Error** argument in vfio_save_setup() Cédric Le Goater
` (6 subsequent siblings)
9 siblings, 2 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
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 c9e200f4eb8f8a8ab2c8b8d0e0dbf871817b94fc..8da2f6454d82046c449f034eb978e1247a9be682 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -103,7 +103,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 b5af6b5105d58f358f6d4d31694e21debd8eb81d..9c648f5ba1c0104088e37baf90d9f94fbdc21570 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3033,13 +3033,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] 37+ messages in thread
* [PATCH v5 04/10] vfio: Use new Error** argument in vfio_save_setup()
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
` (2 preceding siblings ...)
2024-05-06 9:20 ` [PATCH v5 03/10] vfio: Extend migration_file_set_error() with Error** argument Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 13:21 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
` (5 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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 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 | 76 +++++++++++++++++++++++++--------------------
1 file changed, 43 insertions(+), 33 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..9b6375c949f7a8dca857ead2506855f63fa051e4 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,25 +103,26 @@ 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)) {
ret = -errno;
- error_report(
- "%s: Failed setting device in recover state, err: %s. Resetting device",
- vbasedev->name, strerror(errno));
+ error_setg_errno(errp, errno,
+ "%s: Failed setting device in recover state. "
+ "Resetting device", vbasedev->name);
goto reset_device;
}
@@ -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,13 +776,16 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
VFIOMigration *migration = container_of(notifier, VFIOMigration,
migration_state);
VFIODevice *vbasedev = migration->vbasedev;
+ 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);
+ ret = vfio_migration_set_state_or_reset(vbasedev,
+ VFIO_DEVICE_STATE_RUNNING,
+ errp);
}
- return 0;
+ return ret;
}
static void vfio_migration_free(VFIODevice *vbasedev)
--
2.45.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
` (3 preceding siblings ...)
2024-05-06 9:20 ` [PATCH v5 04/10] vfio: Use new Error** argument in vfio_save_setup() Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 13:30 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 06/10] vfio: Reverse test on vfio_get_dirty_bitmap() Cédric Le Goater
` (4 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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>
---
include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++-
hw/vfio/migration.c | 18 ++++++++++++------
hw/vfio/pci.c | 5 +++--
3 files changed, 39 insertions(+), 9 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 9b6375c949f7a8dca857ead2506855f63fa051e4..87437490bd50321b3eb27770c932078597053746 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -189,14 +189,19 @@ 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);
@@ -588,13 +593,14 @@ 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, "%s: Failed to save device config space",
+ 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] 37+ messages in thread
* [PATCH v5 06/10] vfio: Reverse test on vfio_get_dirty_bitmap()
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
` (4 preceding siblings ...)
2024-05-06 9:20 ` [PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 13:42 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 07/10] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
` (3 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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.
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 ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c..b929bb0b7ac60dcef34c0d5a098d5d91f75501dd 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_lock;
}
+
+ 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_lock:
rcu_read_unlock();
out:
--
2.45.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v5 07/10] memory: Add Error** argument to memory_get_xlat_addr()
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
` (5 preceding siblings ...)
2024-05-06 9:20 ` [PATCH v5 06/10] vfio: Reverse test on vfio_get_dirty_bitmap() Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 13:44 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 08/10] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
` (2 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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>
---
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 dadb5cd65ab58b4868fcae06b4e301f0ecb0c1d2..2c45051b7b419c48b4e14c25f4d16a99ccd23996 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 addressf
+ * @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 b929bb0b7ac60dcef34c0d5a098d5d91f75501dd..da748563eb33843e93631a5240759964f33162f2 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_lock;
}
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 49f1cb2c3835f78f5f948531cbbf13adc706f1ad..e2b9f0aa9ea7326a5bedde28b8c777cf86bc253e 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] 37+ messages in thread
* [PATCH v5 08/10] vfio: Add Error** argument to .get_dirty_bitmap() handler
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
` (6 preceding siblings ...)
2024-05-06 9:20 ` [PATCH v5 07/10] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 13:51 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 09/10] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument Cédric Le Goater
9 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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 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 | 17 +++++++-
hw/vfio/common.c | 59 ++++++++++++++++++---------
hw/vfio/container-base.c | 5 ++-
hw/vfio/container.c | 14 ++++---
5 files changed, 68 insertions(+), 31 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..48c92e186231c2c2b548abed08800faff3f430a7 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -85,7 +85,7 @@ 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);
+ 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 list 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);
+ 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 da748563eb33843e93631a5240759964f33162f2..c3d82a9d6e434e33f361e4b96157bf912d5c3a2f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1141,7 +1141,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)
+ 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_lock:
@@ -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..8db59881873c3b1edee81104b966af737e5fa6f6 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -65,10 +65,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)
+ 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] 37+ messages in thread
* [PATCH v5 09/10] vfio: Also trace event failures in vfio_save_complete_precopy()
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
` (7 preceding siblings ...)
2024-05-06 9:20 ` [PATCH v5 08/10] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 13:54 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument Cédric Le Goater
9 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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.
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 87437490bd50321b3eb27770c932078597053746..88591695a7b61c1c623c707334c5c57f5e54c58a 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -581,9 +581,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] 37+ messages in thread
* [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
` (8 preceding siblings ...)
2024-05-06 9:20 ` [PATCH v5 09/10] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
@ 2024-05-06 9:20 ` Cédric Le Goater
2024-05-13 14:26 ` Avihai Horon
9 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-06 9:20 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_set_migration_error() sets the 'return' error on the migration
stream if a migration is in progress. To improve error reporting, add
a new Error* argument to also set the Error object on the migration
stream, if a migration is progress.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v5:
- Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
hw/vfio/common.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 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, Error *err)
{
if (migration_is_setup_or_active()) {
- migration_file_set_error(err, NULL);
+ migration_file_set_error(ret, err);
}
}
@@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
iova, iova + iotlb->addr_mask);
if (iotlb->target_as != &address_space_memory) {
- error_report("Wrong target AS \"%s\", only system memory is allowed",
- iotlb->target_as->name ? iotlb->target_as->name : "none");
- vfio_set_migration_error(-EINVAL);
+ error_setg(&local_err,
+ "Wrong target AS \"%s\", only system memory is allowed",
+ iotlb->target_as->name ? iotlb->target_as->name : "none");
+ vfio_set_migration_error(-EINVAL, local_err);
return;
}
@@ -330,11 +331,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
ret = vfio_container_dma_unmap(bcontainer, iova,
iotlb->addr_mask + 1, iotlb);
if (ret) {
- error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
- "0x%"HWADDR_PRIx") = %d (%s)",
- bcontainer, iova,
- iotlb->addr_mask + 1, ret, strerror(-ret));
- vfio_set_migration_error(ret);
+ error_setg(&local_err,
+ "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
+ "0x%"HWADDR_PRIx") = %d (%s)",
+ bcontainer, iova,
+ iotlb->addr_mask + 1, ret, strerror(-ret));
+ vfio_set_migration_error(ret, local_err);
}
}
out:
@@ -1108,8 +1110,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
if (ret) {
error_prepend(&local_err,
"vfio: Could not stop dirty page tracking - ");
- error_report_err(local_err);
- vfio_set_migration_error(ret);
+ vfio_set_migration_error(ret, local_err);
}
}
@@ -1226,14 +1227,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
if (iotlb->target_as != &address_space_memory) {
- error_report("Wrong target AS \"%s\", only system memory is allowed",
- iotlb->target_as->name ? iotlb->target_as->name : "none");
+ error_setg(&local_err,
+ "Wrong target AS \"%s\", only system memory is allowed",
+ iotlb->target_as->name ? iotlb->target_as->name : "none");
goto out;
}
rcu_read_lock();
if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
- error_report_err(local_err);
goto out_lock;
}
@@ -1244,7 +1245,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
"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_lock:
@@ -1252,7 +1252,7 @@ out_lock:
out:
if (ret) {
- vfio_set_migration_error(ret);
+ vfio_set_migration_error(ret, local_err);
}
}
@@ -1372,8 +1372,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
if (vfio_devices_all_dirty_tracking(bcontainer)) {
ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
if (ret) {
- error_report_err(local_err);
- vfio_set_migration_error(ret);
+ vfio_set_migration_error(ret, local_err);
}
}
}
--
2.45.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-06 9:20 ` [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
@ 2024-05-13 13:03 ` Avihai Horon
2024-05-13 16:27 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 13:03 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
Hi Cedric,
On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> We will use the Error object to improve error reporting in the
> .log_global*() handlers of VFIO. Add documentation while at it.
First of all, I think commit 3688fec8923 ("memory: Add Error** argument
to .log_global_start() handler") forgot to set errp in
vfio_listener_log_global_start() in case of error.
This causes a null pointer de-reference if DPT start fails.
Maybe add a fix for that in the beginning of this series, or as a
stand-alone fix?
Back to this patch, personally, I found the split of patch #1 and #2 a
bit confusing.
Maybe consider squashing patch #1 and #2 so container based and device
based DPT start/stop are changed in the same patch? Like you did in
patch #8?
Whatever you think is better.
In any case:
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 02/10] vfio: Add Error** argument to vfio_devices_dma_logging_start()
2024-05-06 9:20 ` [PATCH v5 02/10] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
@ 2024-05-13 13:08 ` Avihai Horon
2024-05-13 16:49 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 13:08 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> This allows to update the Error argument of the VFIO log_global_start()
> handler. Errors detected when device level logging is started will be
> propagated up to qemu_savevm_state_setup() when the ram save_setup()
> handler is executed.
Errors for container based logging will also be propagated now.
>
> The vfio_set_migration_error() call becomes redundant in
> vfio_devices_dma_logging_start(). Remove it.
Becomes redundant in vfio_listener_log_global_start()?
Other than that,
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 03/10] vfio: Extend migration_file_set_error() with Error** argument
2024-05-06 9:20 ` [PATCH v5 03/10] vfio: Extend migration_file_set_error() with Error** argument Cédric Le Goater
@ 2024-05-13 13:14 ` Avihai Horon
2024-05-13 16:51 ` Cédric Le Goater
2024-05-13 22:19 ` Fabiano Rosas
1 sibling, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 13:14 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
Change commit title:
vfio: Extend migration_file_set_error() with Error** argument
to:
migration: Extend migration_file_set_error() with Error* argument
?
Other than that,
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>
> 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.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> 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 c9e200f4eb8f8a8ab2c8b8d0e0dbf871817b94fc..8da2f6454d82046c449f034eb978e1247a9be682 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -103,7 +103,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 b5af6b5105d58f358f6d4d31694e21debd8eb81d..9c648f5ba1c0104088e37baf90d9f94fbdc21570 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3033,13 +3033,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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 04/10] vfio: Use new Error** argument in vfio_save_setup()
2024-05-06 9:20 ` [PATCH v5 04/10] vfio: Use new Error** argument in vfio_save_setup() Cédric Le Goater
@ 2024-05-13 13:21 ` Avihai Horon
2024-05-14 7:57 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 13:21 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
Nit: change commit title prefix to vfio/migration (also in other patches
that are closely related to vfio migration)
Plus, maybe change subject to "Add an Error** argument to
vfio_migration_set_state() and adjust callers" as it's the main subject
of the patch?
>
> 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 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 | 76 +++++++++++++++++++++++++--------------------
> 1 file changed, 43 insertions(+), 33 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..9b6375c949f7a8dca857ead2506855f63fa051e4 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,25 +103,26 @@ 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)) {
> ret = -errno;
> - error_report(
> - "%s: Failed setting device in recover state, err: %s. Resetting device",
> - vbasedev->name, strerror(errno));
> + error_setg_errno(errp, errno,
> + "%s: Failed setting device in recover state. "
> + "Resetting device", vbasedev->name);
Here we set errp again when it's already set.
Maybe in this case just:
error_report_err(*errp);
*errp = NULL;
error_setg_errno(errp, errno,
"%s: Failed setting device in recover state. "
"Resetting device", vbasedev->name);
?
>
> goto reset_device;
> }
> @@ -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,13 +776,16 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
> VFIOMigration *migration = container_of(notifier, VFIOMigration,
> migration_state);
> VFIODevice *vbasedev = migration->vbasedev;
> + 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);
> + ret = vfio_migration_set_state_or_reset(vbasedev,
> + VFIO_DEVICE_STATE_RUNNING,
> + errp);
> }
> - return 0;
> + return ret;
In MigrationNotifyFunc declaration it's stated that:
/*
* A MigrationNotifyFunc may return an error code and an Error object,
* but only when @e->type is MIG_EVENT_PRECOPY_SETUP. The code is an int
* to allow for different failure modes and recovery actions.
*/
And indeed migration_call_notifiers() will assert if this notifier fails
on MIG_EVENT_PRECOPY_FAILED.
So I guess we should simply report the error locally and return 0.
Thanks.
> }
>
> static void vfio_migration_free(VFIODevice *vbasedev)
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler
2024-05-06 9:20 ` [PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
@ 2024-05-13 13:30 ` Avihai Horon
2024-05-14 8:50 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 13:30 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 06/05/2024 12:20, 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>
> ---
> include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++-
> hw/vfio/migration.c | 18 ++++++++++++------
> hw/vfio/pci.c | 5 +++--
> 3 files changed, 39 insertions(+), 9 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 9b6375c949f7a8dca857ead2506855f63fa051e4..87437490bd50321b3eb27770c932078597053746 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -189,14 +189,19 @@ 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);
Below we have:
return qemu_file_get_error(f);
Need to set errp in case of error.
> @@ -588,13 +593,14 @@ 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, "%s: Failed to save device config space",
Add " - " ("... device config space - "), like in the other patches?
Thanks.
> + 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] 37+ messages in thread
* Re: [PATCH v5 06/10] vfio: Reverse test on vfio_get_dirty_bitmap()
2024-05-06 9:20 ` [PATCH v5 06/10] vfio: Reverse test on vfio_get_dirty_bitmap() Cédric Le Goater
@ 2024-05-13 13:42 ` Avihai Horon
2024-05-14 8:55 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 13:42 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
Title should be: Reverse test on vfio_get_xlat_addr()?
> It will simplify the changes coming after.
>
> 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 ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c..b929bb0b7ac60dcef34c0d5a098d5d91f75501dd 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_lock;
> }
> +
> + 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_lock:
s/out_lock/out_unlock?
With the above,
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> rcu_read_unlock();
>
> out:
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5 07/10] memory: Add Error** argument to memory_get_xlat_addr()
2024-05-06 9:20 ` [PATCH v5 07/10] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
@ 2024-05-13 13:44 ` Avihai Horon
2024-05-14 8:57 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 13:44 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 06/05/2024 12:20, 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>
> ---
> 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 dadb5cd65ab58b4868fcae06b4e301f0ecb0c1d2..2c45051b7b419c48b4e14c25f4d16a99ccd23996 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 addressf
Nit: s/addressf/address
Thanks.
> + * @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 b929bb0b7ac60dcef34c0d5a098d5d91f75501dd..da748563eb33843e93631a5240759964f33162f2 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_lock;
> }
>
> 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 49f1cb2c3835f78f5f948531cbbf13adc706f1ad..e2b9f0aa9ea7326a5bedde28b8c777cf86bc253e 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] 37+ messages in thread
* Re: [PATCH v5 08/10] vfio: Add Error** argument to .get_dirty_bitmap() handler
2024-05-06 9:20 ` [PATCH v5 08/10] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
@ 2024-05-13 13:51 ` Avihai Horon
2024-05-14 9:05 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 13:51 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 06/05/2024 12:20, 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 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 | 17 +++++++-
> hw/vfio/common.c | 59 ++++++++++++++++++---------
> hw/vfio/container-base.c | 5 ++-
> hw/vfio/container.c | 14 ++++---
> 5 files changed, 68 insertions(+), 31 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);
Nit: while at it, can we fix the line wrap here?
> 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..48c92e186231c2c2b548abed08800faff3f430a7 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -85,7 +85,7 @@ 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);
> + hwaddr iova, hwaddr size, Error **errp);
Nit: while at it, can we fix the line wrap here?
>
> 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 list of dirty pages from container
s/list/bitmap?
> + *
> + * @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);
> + 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 da748563eb33843e93631a5240759964f33162f2..c3d82a9d6e434e33f361e4b96157bf912d5c3a2f 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1141,7 +1141,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)
> + hwaddr size, Error **errp)
Nit: while at it, can we fix the line wrap here?
> {
> 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_lock:
> @@ -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..8db59881873c3b1edee81104b966af737e5fa6f6 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -65,10 +65,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)
> + hwaddr iova, hwaddr size, Error **errp)
Nit: while at it, can we fix the line wrap here?
Thanks.
> {
> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 09/10] vfio: Also trace event failures in vfio_save_complete_precopy()
2024-05-06 9:20 ` [PATCH v5 09/10] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
@ 2024-05-13 13:54 ` Avihai Horon
0 siblings, 0 replies; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 13:54 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> vfio_save_complete_precopy() currently returns before doing the trace
> event. Change that.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> ---
> hw/vfio/migration.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 87437490bd50321b3eb27770c932078597053746..88591695a7b61c1c623c707334c5c57f5e54c58a 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -581,9 +581,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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument
2024-05-06 9:20 ` [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument Cédric Le Goater
@ 2024-05-13 14:26 ` Avihai Horon
2024-05-14 11:20 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-13 14:26 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> vfio_set_migration_error() sets the 'return' error on the migration
> stream if a migration is in progress. To improve error reporting, add
> a new Error* argument to also set the Error object on the migration
> stream, if a migration is progress.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v5:
>
> - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>
> hw/vfio/common.c | 37 ++++++++++++++++++-------------------
> 1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 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, Error *err)
> {
> if (migration_is_setup_or_active()) {
> - migration_file_set_error(err, NULL);
> + migration_file_set_error(ret, err);
> }
> }
>
> @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> iova, iova + iotlb->addr_mask);
>
> if (iotlb->target_as != &address_space_memory) {
> - error_report("Wrong target AS \"%s\", only system memory is allowed",
> - iotlb->target_as->name ? iotlb->target_as->name : "none");
> - vfio_set_migration_error(-EINVAL);
> + error_setg(&local_err,
> + "Wrong target AS \"%s\", only system memory is allowed",
> + iotlb->target_as->name ? iotlb->target_as->name : "none");
> + vfio_set_migration_error(-EINVAL, local_err);
> return;
> }
>
> @@ -330,11 +331,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> ret = vfio_container_dma_unmap(bcontainer, iova,
> iotlb->addr_mask + 1, iotlb);
> if (ret) {
> - error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
> - "0x%"HWADDR_PRIx") = %d (%s)",
> - bcontainer, iova,
> - iotlb->addr_mask + 1, ret, strerror(-ret));
> - vfio_set_migration_error(ret);
> + error_setg(&local_err,
> + "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
> + "0x%"HWADDR_PRIx") = %d (%s)",
> + bcontainer, iova,
> + iotlb->addr_mask + 1, ret, strerror(-ret));
Use error_setg_errno()?
> + vfio_set_migration_error(ret, local_err);
Now dma unmap errors (and also the error before it) are not reported if
they happen not during migration.
This makes me think, maybe vfio_set_migration_error() is redundant and
can be replaced by migration_file_set_error()?
Thanks.
> }
> }
> out:
> @@ -1108,8 +1110,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
> if (ret) {
> error_prepend(&local_err,
> "vfio: Could not stop dirty page tracking - ");
> - error_report_err(local_err);
> - vfio_set_migration_error(ret);
> + vfio_set_migration_error(ret, local_err);
> }
> }
>
> @@ -1226,14 +1227,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
>
> if (iotlb->target_as != &address_space_memory) {
> - error_report("Wrong target AS \"%s\", only system memory is allowed",
> - iotlb->target_as->name ? iotlb->target_as->name : "none");
> + error_setg(&local_err,
> + "Wrong target AS \"%s\", only system memory is allowed",
> + iotlb->target_as->name ? iotlb->target_as->name : "none");
> goto out;
> }
>
> rcu_read_lock();
> if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
> - error_report_err(local_err);
> goto out_lock;
> }
>
> @@ -1244,7 +1245,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> "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_lock:
> @@ -1252,7 +1252,7 @@ out_lock:
>
> out:
> if (ret) {
> - vfio_set_migration_error(ret);
> + vfio_set_migration_error(ret, local_err);
> }
> }
>
> @@ -1372,8 +1372,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
> if (vfio_devices_all_dirty_tracking(bcontainer)) {
> ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
> if (ret) {
> - error_report_err(local_err);
> - vfio_set_migration_error(ret);
> + vfio_set_migration_error(ret, local_err);
> }
> }
> }
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-13 13:03 ` Avihai Horon
@ 2024-05-13 16:27 ` Cédric Le Goater
2024-05-15 12:17 ` Avihai Horon
0 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-13 16:27 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/13/24 15:03, Avihai Horon wrote:
> Hi Cedric,
>
> On 06/05/2024 12:20, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> We will use the Error object to improve error reporting in the
>> .log_global*() handlers of VFIO. Add documentation while at it.
>
> First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error.
yes. This is unfortunate. There has been a few respins, the series
was split and I was hoping to upstream this part sooner. My bad.
> This causes a null pointer de-reference if DPT start fails.
> Maybe add a fix for that in the beginning of this series, or as a stand-alone fix?
Since it is fixed by patch 1+2 of this series, we should be fine ?
> Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing.
> Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8?
> Whatever you think is better.
ok. Let's see how v5 goes. I might just send a PR with it if
no major changes are requested.
>
> In any case:
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Thanks,
C.
>
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 02/10] vfio: Add Error** argument to vfio_devices_dma_logging_start()
2024-05-13 13:08 ` Avihai Horon
@ 2024-05-13 16:49 ` Cédric Le Goater
0 siblings, 0 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-13 16:49 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/13/24 15:08, Avihai Horon wrote:
>
> On 06/05/2024 12:20, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> This allows to update the Error argument of the VFIO log_global_start()
>> handler. Errors detected when device level logging is started will be
>> propagated up to qemu_savevm_state_setup() when the ram save_setup()
>> handler is executed.
>
> Errors for container based logging will also be propagated now.
>
>>
>> The vfio_set_migration_error() call becomes redundant in
>> vfio_devices_dma_logging_start(). Remove it.
>
> Becomes redundant in vfio_listener_log_global_start()?
Both sentences updated.
> Other than that,
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Thanks,
C.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 03/10] vfio: Extend migration_file_set_error() with Error** argument
2024-05-13 13:14 ` Avihai Horon
@ 2024-05-13 16:51 ` Cédric Le Goater
0 siblings, 0 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-13 16:51 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/13/24 15:14, Avihai Horon wrote:
>
> On 06/05/2024 12:20, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>
> Change commit title:
>
> vfio: Extend migration_file_set_error() with Error** argument
>
> to:
>
> migration: Extend migration_file_set_error() with Error* argument
>
> ?
yes.
> Other than that,
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
Thanks,
C.
>
>>
>> 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.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>> 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 c9e200f4eb8f8a8ab2c8b8d0e0dbf871817b94fc..8da2f6454d82046c449f034eb978e1247a9be682 100644
>> --- a/include/migration/misc.h
>> +++ b/include/migration/misc.h
>> @@ -103,7 +103,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 b5af6b5105d58f358f6d4d31694e21debd8eb81d..9c648f5ba1c0104088e37baf90d9f94fbdc21570 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3033,13 +3033,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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 03/10] vfio: Extend migration_file_set_error() with Error** argument
2024-05-06 9:20 ` [PATCH v5 03/10] vfio: Extend migration_file_set_error() with Error** argument Cédric Le Goater
2024-05-13 13:14 ` Avihai Horon
@ 2024-05-13 22:19 ` Fabiano Rosas
1 sibling, 0 replies; 37+ messages in thread
From: Fabiano Rosas @ 2024-05-13 22:19 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Alex Williamson, Avihai Horon,
Philippe Mathieu-Daudé, Markus Armbruster,
Cédric Le Goater
Cédric Le Goater <clg@redhat.com> writes:
> 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.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
Acked-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5 04/10] vfio: Use new Error** argument in vfio_save_setup()
2024-05-13 13:21 ` Avihai Horon
@ 2024-05-14 7:57 ` Cédric Le Goater
0 siblings, 0 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-14 7:57 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/13/24 15:21, Avihai Horon wrote:
>
> On 06/05/2024 12:20, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>
> Nit: change commit title prefix to vfio/migration (also in other patches that are closely related to vfio migration)
>
> Plus, maybe change subject to "Add an Error** argument to vfio_migration_set_state() and adjust callers" as it's the main subject of the patch?
>
>>
>> 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 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 | 76 +++++++++++++++++++++++++--------------------
>> 1 file changed, 43 insertions(+), 33 deletions(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..9b6375c949f7a8dca857ead2506855f63fa051e4 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,25 +103,26 @@ 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)) {
>> ret = -errno;
>> - error_report(
>> - "%s: Failed setting device in recover state, err: %s. Resetting device",
>> - vbasedev->name, strerror(errno));
>> + error_setg_errno(errp, errno,
>> + "%s: Failed setting device in recover state. "
>> + "Resetting device", vbasedev->name);
>
> Here we set errp again when it's already set.
yes.
> Maybe in this case just:
>
> error_report_err(*errp);
> *errp = NULL;
> error_setg_errno(errp, errno,
> "%s: Failed setting device in recover state. "
> "Resetting device", vbasedev->name);
> ?
I see two alternatives for the recover state error :
1. append to the existing one, but we lack an error_append() helper
2. report as done today.
I tend to prefer 2. and we cab come back to 1. later on.
>
>>
>> goto reset_device;
>> }
>> @@ -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,13 +776,16 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
>> VFIOMigration *migration = container_of(notifier, VFIOMigration,
>> migration_state);
>> VFIODevice *vbasedev = migration->vbasedev;
>> + 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);
>> + ret = vfio_migration_set_state_or_reset(vbasedev,
>> + VFIO_DEVICE_STATE_RUNNING,
>> + errp);
>> }
>> - return 0;
>> + return ret;
>
> In MigrationNotifyFunc declaration it's stated that:
>
> /*
> * A MigrationNotifyFunc may return an error code and an Error object,
> * but only when @e->type is MIG_EVENT_PRECOPY_SETUP. The code is an int
> * to allow for different failure modes and recovery actions.
> */
>
> And indeed migration_call_notifiers() will assert if this notifier fails on MIG_EVENT_PRECOPY_FAILED.
My series has been out of tree for too long :/
> So I guess we should simply report the error locally and return 0.
yes. That said, I don't see why migrate_fd_cleanup() or postcopy_start()
could not use a local_err and report the error themselves. For later.
I will add a comment though.
Thanks,
C.
>
> Thanks.
>
>> }
>>
>> static void vfio_migration_free(VFIODevice *vbasedev)
>> --
>> 2.45.0
>>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler
2024-05-13 13:30 ` Avihai Horon
@ 2024-05-14 8:50 ` Cédric Le Goater
0 siblings, 0 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-14 8:50 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/13/24 15:30, Avihai Horon wrote:
>
> On 06/05/2024 12:20, 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>
>> ---
>> include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++-
>> hw/vfio/migration.c | 18 ++++++++++++------
>> hw/vfio/pci.c | 5 +++--
>> 3 files changed, 39 insertions(+), 9 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 9b6375c949f7a8dca857ead2506855f63fa051e4..87437490bd50321b3eb27770c932078597053746 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -189,14 +189,19 @@ 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);
>
> Below we have:
>
> return qemu_file_get_error(f);
>
> Need to set errp in case of error.
yep. I will change the returned type to bool while at it.
>
>> @@ -588,13 +593,14 @@ 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, "%s: Failed to save device config space",
>
> Add " - " ("... device config space - "), like in the other patches?
ok. I will check how (in)consistent I was when doing these
error_prepend changes.
Thanks,
C.
>
> Thanks.
>
>> + 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] 37+ messages in thread
* Re: [PATCH v5 06/10] vfio: Reverse test on vfio_get_dirty_bitmap()
2024-05-13 13:42 ` Avihai Horon
@ 2024-05-14 8:55 ` Cédric Le Goater
0 siblings, 0 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-14 8:55 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/13/24 15:42, Avihai Horon wrote:
>
> On 06/05/2024 12:20, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
> Title should be: Reverse test on vfio_get_xlat_addr()?
It could.
>
>> It will simplify the changes coming after.
>>
>> 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 ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c..b929bb0b7ac60dcef34c0d5a098d5d91f75501dd 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_lock;
>> }
>> +
>> + 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_lock:
>
> s/out_lock/out_unlock?
yes. better naming.
>
> With the above,
both done.
Thanks,
C.
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>
>> rcu_read_unlock();
>>
>> out:
>> --
>> 2.45.0
>>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5 07/10] memory: Add Error** argument to memory_get_xlat_addr()
2024-05-13 13:44 ` Avihai Horon
@ 2024-05-14 8:57 ` Cédric Le Goater
0 siblings, 0 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-14 8:57 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster,
Michael S. Tsirkin, Paolo Bonzini, David Hildenbrand
On 5/13/24 15:44, Avihai Horon wrote:
>
> On 06/05/2024 12:20, 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>
>> ---
>> 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 dadb5cd65ab58b4868fcae06b4e301f0ecb0c1d2..2c45051b7b419c48b4e14c25f4d16a99ccd23996 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 addressf
>
> Nit: s/addressf/address
Fixed.
Thanks,
C.
>
> Thanks.
>
>> + * @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 b929bb0b7ac60dcef34c0d5a098d5d91f75501dd..da748563eb33843e93631a5240759964f33162f2 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_lock;
>> }
>>
>> 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 49f1cb2c3835f78f5f948531cbbf13adc706f1ad..e2b9f0aa9ea7326a5bedde28b8c777cf86bc253e 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] 37+ messages in thread
* Re: [PATCH v5 08/10] vfio: Add Error** argument to .get_dirty_bitmap() handler
2024-05-13 13:51 ` Avihai Horon
@ 2024-05-14 9:05 ` Cédric Le Goater
0 siblings, 0 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-14 9:05 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/13/24 15:51, Avihai Horon wrote:
>
> On 06/05/2024 12:20, 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 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 | 17 +++++++-
>> hw/vfio/common.c | 59 ++++++++++++++++++---------
>> hw/vfio/container-base.c | 5 ++-
>> hw/vfio/container.c | 14 ++++---
>> 5 files changed, 68 insertions(+), 31 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);
>
> Nit: while at it, can we fix the line wrap here?
>
>> 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..48c92e186231c2c2b548abed08800faff3f430a7 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -85,7 +85,7 @@ 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);
>> + hwaddr iova, hwaddr size, Error **errp);
>
> Nit: while at it, can we fix the line wrap here?
do you mean :
int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
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 list of dirty pages from container
>
> s/list/bitmap?
yep
Thanks,
C.
>
>> + *
>> + * @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);
>> + 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 da748563eb33843e93631a5240759964f33162f2..c3d82a9d6e434e33f361e4b96157bf912d5c3a2f 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1141,7 +1141,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)
>> + hwaddr size, Error **errp)
>
> Nit: while at it, can we fix the line wrap here?
>
>> {
>> 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_lock:
>> @@ -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..8db59881873c3b1edee81104b966af737e5fa6f6 100644
>> --- a/hw/vfio/container-base.c
>> +++ b/hw/vfio/container-base.c
>> @@ -65,10 +65,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)
>> + hwaddr iova, hwaddr size, Error **errp)
>
> Nit: while at it, can we fix the line wrap here?
>
> Thanks.
>
>> {
>> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument
2024-05-13 14:26 ` Avihai Horon
@ 2024-05-14 11:20 ` Cédric Le Goater
2024-05-14 14:35 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-14 11:20 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/13/24 16:26, Avihai Horon wrote:
>
> On 06/05/2024 12:20, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> vfio_set_migration_error() sets the 'return' error on the migration
>> stream if a migration is in progress. To improve error reporting, add
>> a new Error* argument to also set the Error object on the migration
>> stream, if a migration is progress.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>> Changes in v5:
>>
>> - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>>
>> hw/vfio/common.c | 37 ++++++++++++++++++-------------------
>> 1 file changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 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, Error *err)
>> {
>> if (migration_is_setup_or_active()) {
>> - migration_file_set_error(err, NULL);
>> + migration_file_set_error(ret, err);
>> }
>> }
>>
>> @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> iova, iova + iotlb->addr_mask);
>>
>> if (iotlb->target_as != &address_space_memory) {
>> - error_report("Wrong target AS \"%s\", only system memory is allowed",
>> - iotlb->target_as->name ? iotlb->target_as->name : "none");
>> - vfio_set_migration_error(-EINVAL);
>> + error_setg(&local_err,
>> + "Wrong target AS \"%s\", only system memory is allowed",
>> + iotlb->target_as->name ? iotlb->target_as->name : "none");
>> + vfio_set_migration_error(-EINVAL, local_err);
>> return;
>> }
>>
>> @@ -330,11 +331,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> ret = vfio_container_dma_unmap(bcontainer, iova,
>> iotlb->addr_mask + 1, iotlb);
>> if (ret) {
>> - error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
>> - "0x%"HWADDR_PRIx") = %d (%s)",
>> - bcontainer, iova,
>> - iotlb->addr_mask + 1, ret, strerror(-ret));
>> - vfio_set_migration_error(ret);
>> + error_setg(&local_err,
>> + "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
>> + "0x%"HWADDR_PRIx") = %d (%s)",
>> + bcontainer, iova,
>> + iotlb->addr_mask + 1, ret, strerror(-ret));
>
> Use error_setg_errno()?
sure.
>
>> + vfio_set_migration_error(ret, local_err);
>
> Now dma unmap errors (and also the error before it) are not reported if they happen not during migration.
>
> This makes me think, maybe vfio_set_migration_error() is redundant and can be replaced by migration_file_set_error()?
yes. Good suggestion. I would like to get rid of vfio_set_migration_error(),
so that's a good start.
Thanks,
C.
> Thanks.
>
>> }
>> }
>> out:
>> @@ -1108,8 +1110,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>> if (ret) {
>> error_prepend(&local_err,
>> "vfio: Could not stop dirty page tracking - ");
>> - error_report_err(local_err);
>> - vfio_set_migration_error(ret);
>> + vfio_set_migration_error(ret, local_err);
>> }
>> }
>>
>> @@ -1226,14 +1227,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
>>
>> if (iotlb->target_as != &address_space_memory) {
>> - error_report("Wrong target AS \"%s\", only system memory is allowed",
>> - iotlb->target_as->name ? iotlb->target_as->name : "none");
>> + error_setg(&local_err,
>> + "Wrong target AS \"%s\", only system memory is allowed",
>> + iotlb->target_as->name ? iotlb->target_as->name : "none");
>> goto out;
>> }
>>
>> rcu_read_lock();
>> if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
>> - error_report_err(local_err);
>> goto out_lock;
>> }
>>
>> @@ -1244,7 +1245,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> "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_lock:
>> @@ -1252,7 +1252,7 @@ out_lock:
>>
>> out:
>> if (ret) {
>> - vfio_set_migration_error(ret);
>> + vfio_set_migration_error(ret, local_err);
>> }
>> }
>>
>> @@ -1372,8 +1372,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>> if (vfio_devices_all_dirty_tracking(bcontainer)) {
>> ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
>> if (ret) {
>> - error_report_err(local_err);
>> - vfio_set_migration_error(ret);
>> + vfio_set_migration_error(ret, local_err);
>> }
>> }
>> }
>> --
>> 2.45.0
>>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument
2024-05-14 11:20 ` Cédric Le Goater
@ 2024-05-14 14:35 ` Cédric Le Goater
0 siblings, 0 replies; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-14 14:35 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/14/24 13:20, Cédric Le Goater wrote:
> On 5/13/24 16:26, Avihai Horon wrote:
>>
>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> vfio_set_migration_error() sets the 'return' error on the migration
>>> stream if a migration is in progress. To improve error reporting, add
>>> a new Error* argument to also set the Error object on the migration
>>> stream, if a migration is progress.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>
>>> Changes in v5:
>>>
>>> - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>>>
>>> hw/vfio/common.c | 37 ++++++++++++++++++-------------------
>>> 1 file changed, 18 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 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, Error *err)
>>> {
>>> if (migration_is_setup_or_active()) {
>>> - migration_file_set_error(err, NULL);
>>> + migration_file_set_error(ret, err);
>>> }
>>> }
>>>
>>> @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>> iova, iova + iotlb->addr_mask);
>>>
>>> if (iotlb->target_as != &address_space_memory) {
>>> - error_report("Wrong target AS \"%s\", only system memory is allowed",
>>> - iotlb->target_as->name ? iotlb->target_as->name : "none");
>>> - vfio_set_migration_error(-EINVAL);
>>> + error_setg(&local_err,
>>> + "Wrong target AS \"%s\", only system memory is allowed",
>>> + iotlb->target_as->name ? iotlb->target_as->name : "none");
>>> + vfio_set_migration_error(-EINVAL, local_err);
>>> return;
>>> }
>>>
>>> @@ -330,11 +331,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>> ret = vfio_container_dma_unmap(bcontainer, iova,
>>> iotlb->addr_mask + 1, iotlb);
>>> if (ret) {
>>> - error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
>>> - "0x%"HWADDR_PRIx") = %d (%s)",
>>> - bcontainer, iova,
>>> - iotlb->addr_mask + 1, ret, strerror(-ret));
>>> - vfio_set_migration_error(ret);
>>> + error_setg(&local_err,
>>> + "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
>>> + "0x%"HWADDR_PRIx") = %d (%s)",
>>> + bcontainer, iova,
>>> + iotlb->addr_mask + 1, ret, strerror(-ret));
>>
>> Use error_setg_errno()?
>
> sure.
>
>>
>>> + vfio_set_migration_error(ret, local_err);
>>
>> Now dma unmap errors (and also the error before it) are not reported if they happen not during migration.
>>
>> This makes me think, maybe vfio_set_migration_error() is redundant and can be replaced by migration_file_set_error()?
>
>
> yes. Good suggestion. I would like to get rid of vfio_set_migration_error(),
> so that's a good start.
After taking a closer look, I will drop this patch from v6 because
it needs further splitting and some rework.
Thanks,
C.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-13 16:27 ` Cédric Le Goater
@ 2024-05-15 12:17 ` Avihai Horon
2024-05-15 12:25 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-15 12:17 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 13/05/2024 19:27, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/13/24 15:03, Avihai Horon wrote:
>> Hi Cedric,
>>
>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> We will use the Error object to improve error reporting in the
>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>
>> First of all, I think commit 3688fec8923 ("memory: Add Error**
>> argument to .log_global_start() handler") forgot to set errp in
>> vfio_listener_log_global_start() in case of error.
>
> yes. This is unfortunate. There has been a few respins, the series
> was split and I was hoping to upstream this part sooner. My bad.
>
>> This causes a null pointer de-reference if DPT start fails.
>> Maybe add a fix for that in the beginning of this series, or as a
>> stand-alone fix?
>
> Since it is fixed by patch 1+2 of this series, we should be fine ?
A fix could be useful to be backported to QEMU 9.0, no?
>
>> Back to this patch, personally, I found the split of patch #1 and #2
>> a bit confusing.
>> Maybe consider squashing patch #1 and #2 so container based and
>> device based DPT start/stop are changed in the same patch? Like you
>> did in patch #8?
>> Whatever you think is better.
>
> ok. Let's see how v5 goes. I might just send a PR with it if
> no major changes are requested.
>
>>
>> In any case:
>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>
>
> Thanks,
>
> C.
>
>
>>
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-15 12:17 ` Avihai Horon
@ 2024-05-15 12:25 ` Cédric Le Goater
2024-05-15 12:29 ` Avihai Horon
0 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-15 12:25 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/15/24 14:17, Avihai Horon wrote:
>
> On 13/05/2024 19:27, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 5/13/24 15:03, Avihai Horon wrote:
>>> Hi Cedric,
>>>
>>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> We will use the Error object to improve error reporting in the
>>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>
>>> First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error.
>>
>> yes. This is unfortunate. There has been a few respins, the series
>> was split and I was hoping to upstream this part sooner. My bad.
>>
>>> This causes a null pointer de-reference if DPT start fails.
>>> Maybe add a fix for that in the beginning of this series, or as a stand-alone fix?
>>
>> Since it is fixed by patch 1+2 of this series, we should be fine ?
>
> A fix could be useful to be backported to QEMU 9.0, no?
These changes were only merged in 9.1 with the migration PR.
Am I missing something ?
Thanks,
C.
>
>>
>>> Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing.
>>> Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8?
>>> Whatever you think is better.
>>
>> ok. Let's see how v5 goes. I might just send a PR with it if
>> no major changes are requested.
>>
>>>
>>> In any case:
>>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>>
>>>>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-15 12:25 ` Cédric Le Goater
@ 2024-05-15 12:29 ` Avihai Horon
2024-05-15 12:36 ` Cédric Le Goater
0 siblings, 1 reply; 37+ messages in thread
From: Avihai Horon @ 2024-05-15 12:29 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 15/05/2024 15:25, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/15/24 14:17, Avihai Horon wrote:
>>
>> On 13/05/2024 19:27, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 5/13/24 15:03, Avihai Horon wrote:
>>>> Hi Cedric,
>>>>
>>>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> We will use the Error object to improve error reporting in the
>>>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>>
>>>> First of all, I think commit 3688fec8923 ("memory: Add Error**
>>>> argument to .log_global_start() handler") forgot to set errp in
>>>> vfio_listener_log_global_start() in case of error.
>>>
>>> yes. This is unfortunate. There has been a few respins, the series
>>> was split and I was hoping to upstream this part sooner. My bad.
>>>
>>>> This causes a null pointer de-reference if DPT start fails.
>>>> Maybe add a fix for that in the beginning of this series, or as a
>>>> stand-alone fix?
>>>
>>> Since it is fixed by patch 1+2 of this series, we should be fine ?
>>
>> A fix could be useful to be backported to QEMU 9.0, no?
>
> These changes were only merged in 9.1 with the migration PR.
> Am I missing something ?
Oh, my bad. I thought they were merged in 9.0.
So patches 1+2 should cover it.
Thanks.
>
> Thanks,
>
> C.
>
>
>>
>>>
>>>> Back to this patch, personally, I found the split of patch #1 and
>>>> #2 a bit confusing.
>>>> Maybe consider squashing patch #1 and #2 so container based and
>>>> device based DPT start/stop are changed in the same patch? Like you
>>>> did in patch #8?
>>>> Whatever you think is better.
>>>
>>> ok. Let's see how v5 goes. I might just send a PR with it if
>>> no major changes are requested.
>>>
>>>>
>>>> In any case:
>>>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>>>
>>>>>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-15 12:29 ` Avihai Horon
@ 2024-05-15 12:36 ` Cédric Le Goater
2024-05-15 12:49 ` Avihai Horon
0 siblings, 1 reply; 37+ messages in thread
From: Cédric Le Goater @ 2024-05-15 12:36 UTC (permalink / raw)
To: Avihai Horon, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 5/15/24 14:29, Avihai Horon wrote:
>
> On 15/05/2024 15:25, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 5/15/24 14:17, Avihai Horon wrote:
>>>
>>> On 13/05/2024 19:27, Cédric Le Goater wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 5/13/24 15:03, Avihai Horon wrote:
>>>>> Hi Cedric,
>>>>>
>>>>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> We will use the Error object to improve error reporting in the
>>>>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>>>
>>>>> First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error.
>>>>
>>>> yes. This is unfortunate. There has been a few respins, the series
>>>> was split and I was hoping to upstream this part sooner. My bad.
>>>>
>>>>> This causes a null pointer de-reference if DPT start fails.
>>>>> Maybe add a fix for that in the beginning of this series, or as a stand-alone fix?
>>>>
>>>> Since it is fixed by patch 1+2 of this series, we should be fine ?
>>>
>>> A fix could be useful to be backported to QEMU 9.0, no?
>>
>> These changes were only merged in 9.1 with the migration PR.
>> Am I missing something ?
>
> Oh, my bad. I thought they were merged in 9.0.
> So patches 1+2 should cover it.
yeah. I still would like to merge them asap, with your little series
possibly, the one adding the event, plus the 2 cleanup series from
ZhenZhong. I will update vfio-next when they are sufficiently reviewed.
Thanks,
C.
>
> Thanks.
>
>>
>> Thanks,
>>
>> C.
>>
>>
>>>
>>>>
>>>>> Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing.
>>>>> Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8?
>>>>> Whatever you think is better.
>>>>
>>>> ok. Let's see how v5 goes. I might just send a PR with it if
>>>> no major changes are requested.
>>>>
>>>>>
>>>>> In any case:
>>>>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> 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 [flat|nested] 37+ messages in thread
* Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
2024-05-15 12:36 ` Cédric Le Goater
@ 2024-05-15 12:49 ` Avihai Horon
0 siblings, 0 replies; 37+ messages in thread
From: Avihai Horon @ 2024-05-15 12:49 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Peter Xu, Fabiano Rosas, Alex Williamson,
Philippe Mathieu-Daudé, Markus Armbruster
On 15/05/2024 15:36, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/15/24 14:29, Avihai Horon wrote:
>>
>> On 15/05/2024 15:25, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 5/15/24 14:17, Avihai Horon wrote:
>>>>
>>>> On 13/05/2024 19:27, Cédric Le Goater wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On 5/13/24 15:03, Avihai Horon wrote:
>>>>>> Hi Cedric,
>>>>>>
>>>>>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>>>>>> External email: Use caution opening links or attachments
>>>>>>>
>>>>>>>
>>>>>>> We will use the Error object to improve error reporting in the
>>>>>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>>>>
>>>>>> First of all, I think commit 3688fec8923 ("memory: Add Error**
>>>>>> argument to .log_global_start() handler") forgot to set errp in
>>>>>> vfio_listener_log_global_start() in case of error.
>>>>>
>>>>> yes. This is unfortunate. There has been a few respins, the series
>>>>> was split and I was hoping to upstream this part sooner. My bad.
>>>>>
>>>>>> This causes a null pointer de-reference if DPT start fails.
>>>>>> Maybe add a fix for that in the beginning of this series, or as a
>>>>>> stand-alone fix?
>>>>>
>>>>> Since it is fixed by patch 1+2 of this series, we should be fine ?
>>>>
>>>> A fix could be useful to be backported to QEMU 9.0, no?
>>>
>>> These changes were only merged in 9.1 with the migration PR.
>>> Am I missing something ?
>>
>> Oh, my bad. I thought they were merged in 9.0.
>> So patches 1+2 should cover it.
>
> yeah. I still would like to merge them asap, with your little series
> possibly, the one adding the event, plus the 2 cleanup series from
> ZhenZhong. I will update vfio-next when they are sufficiently reviewed.
>
Sure, I am going to post v3 of my series shortly and then review your v6.
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2024-05-15 12:50 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-06 9:20 [PATCH v5 00/10] vfio: Improve error reporting (part 2) Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler Cédric Le Goater
2024-05-13 13:03 ` Avihai Horon
2024-05-13 16:27 ` Cédric Le Goater
2024-05-15 12:17 ` Avihai Horon
2024-05-15 12:25 ` Cédric Le Goater
2024-05-15 12:29 ` Avihai Horon
2024-05-15 12:36 ` Cédric Le Goater
2024-05-15 12:49 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 02/10] vfio: Add Error** argument to vfio_devices_dma_logging_start() Cédric Le Goater
2024-05-13 13:08 ` Avihai Horon
2024-05-13 16:49 ` Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 03/10] vfio: Extend migration_file_set_error() with Error** argument Cédric Le Goater
2024-05-13 13:14 ` Avihai Horon
2024-05-13 16:51 ` Cédric Le Goater
2024-05-13 22:19 ` Fabiano Rosas
2024-05-06 9:20 ` [PATCH v5 04/10] vfio: Use new Error** argument in vfio_save_setup() Cédric Le Goater
2024-05-13 13:21 ` Avihai Horon
2024-05-14 7:57 ` Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler Cédric Le Goater
2024-05-13 13:30 ` Avihai Horon
2024-05-14 8:50 ` Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 06/10] vfio: Reverse test on vfio_get_dirty_bitmap() Cédric Le Goater
2024-05-13 13:42 ` Avihai Horon
2024-05-14 8:55 ` Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 07/10] memory: Add Error** argument to memory_get_xlat_addr() Cédric Le Goater
2024-05-13 13:44 ` Avihai Horon
2024-05-14 8:57 ` Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 08/10] vfio: Add Error** argument to .get_dirty_bitmap() handler Cédric Le Goater
2024-05-13 13:51 ` Avihai Horon
2024-05-14 9:05 ` Cédric Le Goater
2024-05-06 9:20 ` [PATCH v5 09/10] vfio: Also trace event failures in vfio_save_complete_precopy() Cédric Le Goater
2024-05-13 13:54 ` Avihai Horon
2024-05-06 9:20 ` [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument Cédric Le Goater
2024-05-13 14:26 ` Avihai Horon
2024-05-14 11:20 ` Cédric Le Goater
2024-05-14 14:35 ` 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).