From 836bb308685007ca7cccb891ec213a33f478af18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 16 May 2024 14:46:50 +0200 Subject: [PATCH 01/47] vfio: Add Error** argument to .set_dirty_page_tracking() handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é Reviewed-by: Avihai Horon Reviewed-by: Eric Auger Signed-off-by: Cédric Le Goater --- hw/vfio/common.c | 4 ++-- hw/vfio/container-base.c | 4 ++-- hw/vfio/container.c | 6 +++--- include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++-- 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 8f9cbdc026..485e539164 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 913ae49077..7c0764121d 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 77bdec276e..c35221fbe7 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; diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 3582d5f97a..326ceea52a 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); From 0f21358f33f0b9aa1b8dd5e33de92118186999db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 16 May 2024 14:46:51 +0200 Subject: [PATCH 02/47] vfio: Add Error** argument to vfio_devices_dma_logging_start() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows to update the Error argument of the VFIO log_global_start() handler. Errors for container based logging will also be propagated to qemu_savevm_state_setup() when the ram save_setup() handler is executed. Also, errors from vfio_container_set_dirty_page_tracking() are now collected and reported. The vfio_set_migration_error() call becomes redundant in vfio_listener_log_global_start(). Remove it. Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Avihai Horon Reviewed-by: Eric Auger Signed-off-by: Cédric Le Goater --- 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 485e539164..b5102f54a6 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); } } From 019d9e6cc408f402dd27d6884ac7b742e0e4f99e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 16 May 2024 14:46:52 +0200 Subject: [PATCH 03/47] migration: Extend migration_file_set_error() with Error* argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use it to update the current error of the migration stream if available and if not, simply print out the error. Next changes will update with an error to report. Reviewed-by: Avihai Horon Acked-by: Fabiano Rosas Reviewed-by: Eric Auger Signed-off-by: Cédric Le Goater --- hw/vfio/common.c | 4 ++-- hw/vfio/migration.c | 4 ++-- include/migration/misc.h | 2 +- migration/migration.c | 6 ++++-- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index b5102f54a6..2c97de6c73 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -147,10 +147,10 @@ bool vfio_viommu_preset(VFIODevice *vbasedev) return vbasedev->bcontainer->space->as != &address_space_memory; } -static void vfio_set_migration_error(int err) +static void vfio_set_migration_error(int ret) { if (migration_is_setup_or_active()) { - migration_file_set_error(err); + migration_file_set_error(ret, NULL); } } diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 06ae40969b..bf2fd0759b 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/include/migration/misc.h b/include/migration/misc.h index bf7339cc1e..bfadc5613b 100644 --- a/include/migration/misc.h +++ b/include/migration/misc.h @@ -97,7 +97,7 @@ void migration_add_notifier_mode(NotifierWithReturn *notify, void migration_remove_notifier(NotifierWithReturn *notify); bool migration_is_running(void); -void migration_file_set_error(int err); +void migration_file_set_error(int ret, Error *err); /* True if incoming migration entered POSTCOPY_INCOMING_DISCARD */ bool migration_in_incoming_postcopy(void); diff --git a/migration/migration.c b/migration/migration.c index e88b24f1e6..70d66a441b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2994,13 +2994,15 @@ static MigThrError postcopy_pause(MigrationState *s) } } -void migration_file_set_error(int err) +void migration_file_set_error(int ret, Error *err) { MigrationState *s = current_migration; WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) { if (s->to_dst_file) { - qemu_file_set_error(s->to_dst_file, err); + qemu_file_set_error_obj(s->to_dst_file, ret, err); + } else if (err) { + error_report_err(err); } } } From fbd2469a66e3fd3f3457170928daf8098f207427 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 16 May 2024 14:46:53 +0200 Subject: [PATCH 04/47] vfio/migration: Add an Error** argument to vfio_migration_set_state() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an Error** argument to vfio_migration_set_state() and adjust callers, including vfio_save_setup(). The error will be propagated up to qemu_savevm_state_setup() where the save_setup() handler is executed. Modify vfio_vmstate_change_prepare() and vfio_vmstate_change() to store a reported error under the migration stream if a migration is in progress. Reviewed-by: Avihai Horon Signed-off-by: Cédric Le Goater --- hw/vfio/migration.c | 81 +++++++++++++++++++++++++++++---------------- 1 file changed, 52 insertions(+), 29 deletions(-) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index bf2fd0759b..43fed0dbdb 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -82,7 +82,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state) static int vfio_migration_set_state(VFIODevice *vbasedev, enum vfio_device_mig_state new_state, - enum vfio_device_mig_state recover_state) + enum vfio_device_mig_state recover_state, + Error **errp) { VFIOMigration *migration = vbasedev->migration; uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) + @@ -92,6 +93,9 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, struct vfio_device_feature_mig_state *mig_state = (struct vfio_device_feature_mig_state *)feature->data; int ret; + g_autofree char *error_prefix = + g_strdup_printf("%s: Failed setting device state to %s.", + vbasedev->name, mig_state_to_str(new_state)); feature->argsz = sizeof(buf); feature->flags = @@ -102,22 +106,24 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, ret = -errno; if (recover_state == VFIO_DEVICE_STATE_ERROR) { - error_report("%s: Failed setting device state to %s, err: %s. " - "Recover state is ERROR. Resetting device", - vbasedev->name, mig_state_to_str(new_state), - strerror(errno)); + error_setg_errno(errp, errno, + "%s Recover state is ERROR. Resetting device", + error_prefix); goto reset_device; } - error_report( - "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s", - vbasedev->name, mig_state_to_str(new_state), - strerror(errno), mig_state_to_str(recover_state)); + error_setg_errno(errp, errno, + "%s Setting device in recover state %s", + error_prefix, mig_state_to_str(recover_state)); mig_state->device_state = recover_state; if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) { ret = -errno; + /* + * If setting the device in recover state fails, report + * the error here and propagate the first error. + */ error_report( "%s: Failed setting device in recover state, err: %s. Resetting device", vbasedev->name, strerror(errno)); @@ -137,7 +143,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, * This can happen if the device is asynchronously reset and * terminates a data transfer. */ - error_report("%s: data_fd out of sync", vbasedev->name); + error_setg(errp, "%s: data_fd out of sync", vbasedev->name); close(mig_state->data_fd); return -EBADF; @@ -168,10 +174,11 @@ reset_device: */ static int vfio_migration_set_state_or_reset(VFIODevice *vbasedev, - enum vfio_device_mig_state new_state) + enum vfio_device_mig_state new_state, + Error **errp) { return vfio_migration_set_state(vbasedev, new_state, - VFIO_DEVICE_STATE_ERROR); + VFIO_DEVICE_STATE_ERROR, errp); } static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev, @@ -399,10 +406,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp) switch (migration->device_state) { case VFIO_DEVICE_STATE_RUNNING: ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY, - VFIO_DEVICE_STATE_RUNNING); + VFIO_DEVICE_STATE_RUNNING, errp); if (ret) { - error_setg(errp, "%s: Failed to set new PRE_COPY state", - vbasedev->name); return ret; } @@ -435,13 +440,20 @@ static void vfio_save_cleanup(void *opaque) { VFIODevice *vbasedev = opaque; VFIOMigration *migration = vbasedev->migration; + Error *local_err = NULL; + int ret; /* * Changing device state from STOP_COPY to STOP can take time. Do it here, * after migration has completed, so it won't increase downtime. */ if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) { - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP); + ret = vfio_migration_set_state_or_reset(vbasedev, + VFIO_DEVICE_STATE_STOP, + &local_err); + if (ret) { + error_report_err(local_err); + } } g_free(migration->data_buffer); @@ -549,11 +561,13 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) VFIODevice *vbasedev = opaque; ssize_t data_size; int ret; + Error *local_err = NULL; /* We reach here with device state STOP or STOP_COPY only */ ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY, - VFIO_DEVICE_STATE_STOP); + VFIO_DEVICE_STATE_STOP, &local_err); if (ret) { + error_report_err(local_err); return ret; } @@ -591,14 +605,9 @@ static void vfio_save_state(QEMUFile *f, void *opaque) static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp) { VFIODevice *vbasedev = opaque; - int ret; - ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING, - vbasedev->migration->device_state); - if (ret) { - error_setg(errp, "%s: Failed to set RESUMING state", vbasedev->name); - } - return ret; + return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING, + vbasedev->migration->device_state, errp); } static int vfio_load_cleanup(void *opaque) @@ -714,19 +723,20 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running, VFIODevice *vbasedev = opaque; VFIOMigration *migration = vbasedev->migration; enum vfio_device_mig_state new_state; + Error *local_err = NULL; int ret; new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ? VFIO_DEVICE_STATE_PRE_COPY_P2P : VFIO_DEVICE_STATE_RUNNING_P2P; - ret = vfio_migration_set_state_or_reset(vbasedev, new_state); + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err); if (ret) { /* * Migration should be aborted in this case, but vm_state_notify() * currently does not support reporting failures. */ - migration_file_set_error(ret, NULL); + migration_file_set_error(ret, local_err); } trace_vfio_vmstate_change_prepare(vbasedev->name, running, @@ -738,6 +748,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state) { VFIODevice *vbasedev = opaque; enum vfio_device_mig_state new_state; + Error *local_err = NULL; int ret; if (running) { @@ -750,13 +761,13 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state) VFIO_DEVICE_STATE_STOP; } - ret = vfio_migration_set_state_or_reset(vbasedev, new_state); + ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err); if (ret) { /* * Migration should be aborted in this case, but vm_state_notify() * currently does not support reporting failures. */ - migration_file_set_error(ret, NULL); + migration_file_set_error(ret, local_err); } trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state), @@ -769,11 +780,23 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier, VFIOMigration *migration = container_of(notifier, VFIOMigration, migration_state); VFIODevice *vbasedev = migration->vbasedev; + Error *local_err = NULL; + int ret; trace_vfio_migration_state_notifier(vbasedev->name, e->type); if (e->type == MIG_EVENT_PRECOPY_FAILED) { - vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING); + /* + * MigrationNotifyFunc may not return an error code and an Error + * object for MIG_EVENT_PRECOPY_FAILED. Hence, report the error + * locally and ignore the errp argument. + */ + ret = vfio_migration_set_state_or_reset(vbasedev, + VFIO_DEVICE_STATE_RUNNING, + &local_err); + if (ret) { + error_report_err(local_err); + } } return 0; } From 3783f814e7e2e11cbd4e18e51e710db37900a1b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 16 May 2024 14:46:54 +0200 Subject: [PATCH 05/47] vfio/migration: Add Error** argument to .vfio_save_config() handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é Reviewed-by: Eric Auger Reviewed-by: Avihai Horon Signed-off-by: Cédric Le Goater --- hw/vfio/migration.c | 25 ++++++++++++++++++------- hw/vfio/pci.c | 5 +++-- include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 43fed0dbdb..5d91364f3b 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -193,21 +193,30 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev, return ret; } -static int vfio_save_device_config_state(QEMUFile *f, void *opaque) +static int vfio_save_device_config_state(QEMUFile *f, void *opaque, + Error **errp) { VFIODevice *vbasedev = opaque; + int ret; qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE); if (vbasedev->ops && vbasedev->ops->vfio_save_config) { - vbasedev->ops->vfio_save_config(vbasedev, f); + ret = vbasedev->ops->vfio_save_config(vbasedev, f, errp); + if (ret) { + return ret; + } } qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); trace_vfio_save_device_config_state(vbasedev->name); - return qemu_file_get_error(f); + ret = qemu_file_get_error(f); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to save state"); + } + return ret; } static int vfio_load_device_config_state(QEMUFile *f, void *opaque) @@ -592,13 +601,15 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) static void vfio_save_state(QEMUFile *f, void *opaque) { VFIODevice *vbasedev = opaque; + Error *local_err = NULL; int ret; - ret = vfio_save_device_config_state(f, opaque); + ret = vfio_save_device_config_state(f, opaque, &local_err); if (ret) { - error_report("%s: Failed to save device config space", - vbasedev->name); - qemu_file_set_error(f, ret); + error_prepend(&local_err, + "vfio: Failed to save device config space of %s - ", + vbasedev->name); + qemu_file_set_error_obj(f, ret, local_err); } } diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 64780d1b79..fc6e54e871 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) diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index d66e27db02..3ff633ad3b 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); }; From 94d12088409b9544dd4e191e2971c47ecec4c315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 16 May 2024 14:46:55 +0200 Subject: [PATCH 06/47] vfio: Reverse test on vfio_get_xlat_addr() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It will simplify the changes coming after. Reviewed-by: Avihai Horon Reviewed-by: Eric Auger Signed-off-by: Cédric Le Goater --- 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 2c97de6c73..c7f274fb5c 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1224,16 +1224,20 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) } rcu_read_lock(); - if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) { - ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, - translated_addr); - if (ret) { - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " - "0x%"HWADDR_PRIx") = %d (%s)", - bcontainer, iova, iotlb->addr_mask + 1, ret, - strerror(-ret)); - } + if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) { + goto out_unlock; } + + ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, + translated_addr); + if (ret) { + error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " + "0x%"HWADDR_PRIx") = %d (%s)", + bcontainer, iova, iotlb->addr_mask + 1, ret, + strerror(-ret)); + } + +out_unlock: rcu_read_unlock(); out: From ebb481c03c22eaf052b02c37ddd53989a078b771 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 16 May 2024 14:46:56 +0200 Subject: [PATCH 07/47] memory: Add Error** argument to memory_get_xlat_addr() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let the callers do the reporting. This will be useful in vfio_iommu_map_dirty_notify(). Cc: Michael S. Tsirkin Cc: Paolo Bonzini Cc: David Hildenbrand Reviewed-by: Peter Xu Reviewed-by: Eric Auger Reviewed-by: Avihai Horon Signed-off-by: Cédric Le Goater --- hw/vfio/common.c | 13 +++++++++---- hw/virtio/vhost-vdpa.c | 5 ++++- include/exec/memory.h | 15 ++++++++++++++- system/memory.c | 10 +++++----- 4 files changed, 32 insertions(+), 11 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index c7f274fb5c..7313043f1d 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -253,12 +253,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section) /* Called with rcu_read_lock held. */ static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, - ram_addr_t *ram_addr, bool *read_only) + ram_addr_t *ram_addr, bool *read_only, + Error **errp) { bool ret, mr_has_discard_manager; ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only, - &mr_has_discard_manager); + &mr_has_discard_manager, errp); if (ret && mr_has_discard_manager) { /* * Malicious VMs might trigger discarding of IOMMU-mapped memory. The @@ -288,6 +289,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) hwaddr iova = iotlb->iova + giommu->iommu_offset; void *vaddr; int ret; + Error *local_err = NULL; trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP", iova, iova + iotlb->addr_mask); @@ -304,7 +306,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { bool read_only; - if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { + if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, &local_err)) { + error_report_err(local_err); goto out; } /* @@ -1213,6 +1216,7 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) VFIOContainerBase *bcontainer = giommu->bcontainer; hwaddr iova = iotlb->iova + giommu->iommu_offset; ram_addr_t translated_addr; + Error *local_err = NULL; int ret = -EINVAL; trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask); @@ -1224,7 +1228,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) } rcu_read_lock(); - if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) { + if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) { + error_report_err(local_err); goto out_unlock; } diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index e827b9175f..ed99ab8745 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/include/exec/memory.h b/include/exec/memory.h index d417d7f363..9cdd64e9c6 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -774,9 +774,22 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm, void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, RamDiscardListener *rdl); +/** + * memory_get_xlat_addr: Extract addresses from a TLB entry + * + * @iotlb: pointer to an #IOMMUTLBEntry + * @vaddr: virtual address + * @ram_addr: RAM address + * @read_only: indicates if writes are allowed + * @mr_has_discard_manager: indicates memory is controlled by a + * RamDiscardManager + * @errp: pointer to Error*, to store an error if it happens. + * + * Return: true on success, else false setting @errp with error. + */ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, ram_addr_t *ram_addr, bool *read_only, - bool *mr_has_discard_manager); + bool *mr_has_discard_manager, Error **errp); typedef struct CoalescedMemoryRange CoalescedMemoryRange; typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd; diff --git a/system/memory.c b/system/memory.c index 642a449f8c..9540caa8a1 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; } From 2da5f9e4d86c1a6cf5ff35cdb7b87e993e947fe0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 16 May 2024 14:46:57 +0200 Subject: [PATCH 08/47] vfio: Add Error** argument to .get_dirty_bitmap() handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let the callers do the error reporting. Add documentation while at it. Reviewed-by: Eric Auger Reviewed-by: Avihai Horon Signed-off-by: Cédric Le Goater --- hw/vfio/common.c | 60 +++++++++++++++++---------- hw/vfio/container-base.c | 6 +-- hw/vfio/container.c | 14 ++++--- include/hw/vfio/vfio-common.h | 5 +-- include/hw/vfio/vfio-container-base.h | 19 +++++++-- 5 files changed, 67 insertions(+), 37 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 7313043f1d..1fbd10801d 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1140,8 +1140,7 @@ static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova, } int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, hwaddr iova, - hwaddr size) + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp) { VFIODevice *vbasedev; int ret; @@ -1150,10 +1149,10 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, ret = vfio_device_dma_logging_report(vbasedev, iova, size, vbmap->bitmap); if (ret) { - error_report("%s: Failed to get DMA logging report, iova: " - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx - ", err: %d (%s)", - vbasedev->name, iova, size, ret, strerror(-ret)); + error_setg_errno(errp, -ret, + "%s: Failed to get DMA logging report, iova: " + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx, + vbasedev->name, iova, size); return ret; } @@ -1163,7 +1162,7 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, } int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, - uint64_t size, ram_addr_t ram_addr) + uint64_t size, ram_addr_t ram_addr, Error **errp) { bool all_device_dirty_tracking = vfio_devices_all_device_dirty_tracking(bcontainer); @@ -1180,13 +1179,17 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, ret = vfio_bitmap_alloc(&vbmap, size); if (ret) { + error_setg_errno(errp, -ret, + "Failed to allocate dirty tracking bitmap"); return ret; } if (all_device_dirty_tracking) { - ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size); + ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size, + errp); } else { - ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size); + ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size, + errp); } if (ret) { @@ -1234,12 +1237,13 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) } ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, - translated_addr); + translated_addr, &local_err); if (ret) { - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " - "0x%"HWADDR_PRIx") = %d (%s)", - bcontainer, iova, iotlb->addr_mask + 1, ret, - strerror(-ret)); + error_prepend(&local_err, + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " + "0x%"HWADDR_PRIx") failed - ", bcontainer, iova, + iotlb->addr_mask + 1); + error_report_err(local_err); } out_unlock: @@ -1259,12 +1263,19 @@ static int vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section, const ram_addr_t ram_addr = memory_region_get_ram_addr(section->mr) + section->offset_within_region; VFIORamDiscardListener *vrdl = opaque; + Error *local_err = NULL; + int ret; /* * Sync the whole mapped region (spanning multiple individual mappings) * in one go. */ - return vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr); + ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr, + &local_err); + if (ret) { + error_report_err(local_err); + } + return ret; } static int @@ -1296,7 +1307,7 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer, } static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, - MemoryRegionSection *section) + MemoryRegionSection *section, Error **errp) { ram_addr_t ram_addr; @@ -1327,7 +1338,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, } return 0; } else if (memory_region_has_ram_discard_manager(section->mr)) { - return vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); + int ret; + + ret = vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); + if (ret) { + error_setg(errp, + "Failed to sync dirty bitmap with RAM discard listener"); + } + return ret; } ram_addr = memory_region_get_ram_addr(section->mr) + @@ -1335,7 +1353,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, return vfio_get_dirty_bitmap(bcontainer, REAL_HOST_PAGE_ALIGN(section->offset_within_address_space), - int128_get64(section->size), ram_addr); + int128_get64(section->size), ram_addr, errp); } static void vfio_listener_log_sync(MemoryListener *listener, @@ -1344,16 +1362,16 @@ static void vfio_listener_log_sync(MemoryListener *listener, VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase, listener); int ret; + Error *local_err = NULL; if (vfio_listener_skipped_section(section)) { return; } if (vfio_devices_all_dirty_tracking(bcontainer)) { - ret = vfio_sync_dirty_bitmap(bcontainer, section); + ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err); if (ret) { - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret, - strerror(-ret)); + error_report_err(local_err); vfio_set_migration_error(ret); } } diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c index 7c0764121d..26f4bb464a 100644 --- a/hw/vfio/container-base.c +++ b/hw/vfio/container-base.c @@ -64,11 +64,11 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, } int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, - hwaddr iova, hwaddr size) + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp) { g_assert(bcontainer->ops->query_dirty_bitmap); - return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size); + return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size, + errp); } void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space, diff --git a/hw/vfio/container.c b/hw/vfio/container.c index c35221fbe7..9534120d4a 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -130,6 +130,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, }; bool need_dirty_sync = false; int ret; + Error *local_err = NULL; if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) { if (!vfio_devices_all_device_dirty_tracking(bcontainer) && @@ -165,8 +166,9 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, if (need_dirty_sync) { ret = vfio_get_dirty_bitmap(bcontainer, iova, size, - iotlb->translated_addr); + iotlb->translated_addr, &local_err); if (ret) { + error_report_err(local_err); return ret; } } @@ -235,8 +237,7 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, } static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, - hwaddr iova, hwaddr size) + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp) { const VFIOContainer *container = container_of(bcontainer, VFIOContainer, bcontainer); @@ -264,9 +265,10 @@ static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap); if (ret) { ret = -errno; - error_report("Failed to get dirty bitmap for iova: 0x%"PRIx64 - " size: 0x%"PRIx64" err: %d", (uint64_t)range->iova, - (uint64_t)range->size, errno); + error_setg_errno(errp, errno, + "Failed to get dirty bitmap for iova: 0x%"PRIx64 + " size: 0x%"PRIx64, (uint64_t)range->iova, + (uint64_t)range->size); } g_free(dbitmap); diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 3ff633ad3b..b6ac249536 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -273,10 +273,9 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer); bool vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer); int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, hwaddr iova, - hwaddr size); + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp); int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, - uint64_t size, ram_addr_t ram_addr); + uint64_t size, ram_addr_t ram_addr, Error **errp); /* Returns 0 on success, or a negative errno. */ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp); diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 326ceea52a..b04057ad1a 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -84,8 +84,7 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer, int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, bool start, Error **errp); int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, - hwaddr iova, hwaddr size); + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp); void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space, @@ -138,9 +137,21 @@ struct VFIOIOMMUClass { */ int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, bool start, Error **errp); + /** + * @query_dirty_bitmap + * + * Get bitmap of dirty pages from container + * + * @bcontainer: #VFIOContainerBase from which to get dirty pages + * @vbmap: #VFIOBitmap internal bitmap structure + * @iova: iova base address + * @size: size of iova range + * @errp: pointer to Error*, to store an error if it happens. + * + * Returns zero to indicate success and negative for error + */ int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, - hwaddr iova, hwaddr size); + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp); /* PCI specific */ int (*pci_hot_reset)(VFIODevice *vbasedev, bool single); From 33dc04b0722974ae140527d9c44eb442f8ac463e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 16 May 2024 14:46:58 +0200 Subject: [PATCH 09/47] vfio: Also trace event failures in vfio_save_complete_precopy() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vfio_save_complete_precopy() currently returns before doing the trace event. Change that. Reviewed-by: Avihai Horon Reviewed-by: Eric Auger Signed-off-by: Cédric Le Goater --- hw/vfio/migration.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 5d91364f3b..c4403a38dd 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -589,9 +589,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); ret = qemu_file_get_error(f); - if (ret) { - return ret; - } trace_vfio_save_complete_precopy(vbasedev->name, ret); From ae7aca14bd531922e27767e90902b673f2cf786a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 25 Apr 2024 11:02:11 +0200 Subject: [PATCH 10/47] vfio/ap: Use g_autofree variable in vfio_ap_register_irq_notifier() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewed-by: Markus Armbruster Reviewed-by: Anthony Krowiak Signed-off-by: Cédric Le Goater --- hw/vfio/ap.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index 7c4caa5938..03f8ffaa5e 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -77,7 +77,7 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev, size_t argsz; IOHandler *fd_read; EventNotifier *notifier; - struct vfio_irq_info *irq_info; + g_autofree struct vfio_irq_info *irq_info = NULL; VFIODevice *vdev = &vapdev->vdev; switch (irq) { @@ -104,14 +104,14 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev, if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, irq_info) < 0 || irq_info->count < 1) { error_setg_errno(errp, errno, "vfio: Error getting irq info"); - goto out_free_info; + return; } if (event_notifier_init(notifier, 0)) { error_setg_errno(errp, errno, "vfio: Unable to init event notifier for irq (%d)", irq); - goto out_free_info; + return; } fd = event_notifier_get_fd(notifier); @@ -122,10 +122,6 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev, qemu_set_fd_handler(fd, NULL, NULL, vapdev); event_notifier_cleanup(notifier); } - -out_free_info: - g_free(irq_info); - } static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, From cbd470f0aac51103e010e45e1b10cb3bfedb4f99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 25 Apr 2024 11:02:12 +0200 Subject: [PATCH 11/47] vfio/ap: Make vfio_ap_register_irq_notifier() return a bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since vfio_ap_register_irq_notifier() takes and 'Error **' argument, best practices suggest to return a bool. See the qapi/error.h Rules section. Reviewed-by: Markus Armbruster Signed-off-by: Cédric Le Goater --- hw/vfio/ap.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index 03f8ffaa5e..8bb024e2fd 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -70,7 +70,7 @@ static void vfio_ap_req_notifier_handler(void *opaque) } } -static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev, +static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev, unsigned int irq, Error **errp) { int fd; @@ -87,13 +87,13 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev, break; default: error_setg(errp, "vfio: Unsupported device irq(%d)", irq); - return; + return false; } if (vdev->num_irqs < irq + 1) { error_setg(errp, "vfio: IRQ %u not available (number of irqs %u)", irq, vdev->num_irqs); - return; + return false; } argsz = sizeof(*irq_info); @@ -104,14 +104,14 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev, if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, irq_info) < 0 || irq_info->count < 1) { error_setg_errno(errp, errno, "vfio: Error getting irq info"); - return; + return false; } if (event_notifier_init(notifier, 0)) { error_setg_errno(errp, errno, "vfio: Unable to init event notifier for irq (%d)", irq); - return; + return false; } fd = event_notifier_get_fd(notifier); @@ -122,6 +122,8 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev, qemu_set_fd_handler(fd, NULL, NULL, vapdev); event_notifier_cleanup(notifier); } + + return true; } static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, @@ -167,8 +169,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) goto error; } - vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err); - if (err) { + if (!vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err)) { /* * Report this error, but do not make it a failing condition. * Lack of this IRQ in the host does not prevent normal operation. From 04f8e4f29b5264f855be015852497b3c1dcdb3c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 25 Apr 2024 11:02:13 +0200 Subject: [PATCH 12/47] vfio/ccw: Use g_autofree variable in vfio_ccw_register_irq_notifier() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewed-by: Markus Armbruster Reviewed-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/ccw.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 90e4a53437..6764388bc4 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -384,7 +384,7 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, Error **errp) { VFIODevice *vdev = &vcdev->vdev; - struct vfio_irq_info *irq_info; + g_autofree struct vfio_irq_info *irq_info = NULL; size_t argsz; int fd; EventNotifier *notifier; @@ -421,14 +421,14 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, irq_info) < 0 || irq_info->count < 1) { error_setg_errno(errp, errno, "vfio: Error getting irq info"); - goto out_free_info; + return; } if (event_notifier_init(notifier, 0)) { error_setg_errno(errp, errno, "vfio: Unable to init event notifier for irq (%d)", irq); - goto out_free_info; + return; } fd = event_notifier_get_fd(notifier); @@ -439,9 +439,6 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, qemu_set_fd_handler(fd, NULL, NULL, vcdev); event_notifier_cleanup(notifier); } - -out_free_info: - g_free(irq_info); } static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, From 8aaeff97acee1ad0c96b6c229cd02c8f3e96dcda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 25 Apr 2024 11:02:14 +0200 Subject: [PATCH 13/47] vfio/ccw: Make vfio_ccw_register_irq_notifier() return a bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since vfio_ccw_register_irq_notifier() takes an 'Error **' argument, best practices suggest to return a bool. See the qapi/error.h Rules section. Reviewed-by: Markus Armbruster Reviewed-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/ccw.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 6764388bc4..1c630f6e9a 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -379,7 +379,7 @@ read_err: css_inject_io_interrupt(sch); } -static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, +static bool vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, unsigned int irq, Error **errp) { @@ -405,13 +405,13 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, break; default: error_setg(errp, "vfio: Unsupported device irq(%d)", irq); - return; + return false; } if (vdev->num_irqs < irq + 1) { error_setg(errp, "vfio: IRQ %u not available (number of irqs %u)", irq, vdev->num_irqs); - return; + return false; } argsz = sizeof(*irq_info); @@ -421,14 +421,14 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, irq_info) < 0 || irq_info->count < 1) { error_setg_errno(errp, errno, "vfio: Error getting irq info"); - return; + return false; } if (event_notifier_init(notifier, 0)) { error_setg_errno(errp, errno, "vfio: Unable to init event notifier for irq (%d)", irq); - return; + return false; } fd = event_notifier_get_fd(notifier); @@ -439,6 +439,8 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, qemu_set_fd_handler(fd, NULL, NULL, vcdev); event_notifier_cleanup(notifier); } + + return true; } static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, @@ -602,20 +604,18 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) goto out_region_err; } - vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err); - if (err) { + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err)) { goto out_io_notifier_err; } if (vcdev->crw_region) { - vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX, &err); - if (err) { + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX, + &err)) { goto out_irq_notifier_err; } } - vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err); - if (err) { + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err)) { /* * Report this error, but do not make it a failing condition. * Lack of this IRQ in the host does not prevent normal operation. From 187716feeba406b5a3879db66a7bafd687472a1f Mon Sep 17 00:00:00 2001 From: Vinayak Kale Date: Fri, 3 May 2024 20:21:42 +0530 Subject: [PATCH 14/47] vfio/pci: migration: Skip config space check for Vendor Specific Information in VSC during restore/load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In case of migration, during restore operation, qemu checks config space of the pci device with the config space in the migration stream captured during save operation. In case of config space data mismatch, restore operation is failed. config space check is done in function get_pci_config_device(). By default VSC (vendor-specific-capability) in config space is checked. Due to qemu's config space check for VSC, live migration is broken across NVIDIA vGPU devices in situation where source and destination host driver is different. In this situation, Vendor Specific Information in VSC varies on the destination to ensure vGPU feature capabilities exposed to the guest driver are compatible with destination host. If a vfio-pci device is migration capable and vfio-pci vendor driver is OK with volatile Vendor Specific Info in VSC then qemu should exempt config space check for Vendor Specific Info. It is vendor driver's responsibility to ensure that VSC is consistent across migration. Here consistency could mean that VSC format should be same on source and destination, however actual Vendor Specific Info may not be byte-to-byte identical. This patch skips the check for Vendor Specific Information in VSC for VFIO-PCI device by clearing pdev->cmask[] offsets. Config space check is still enforced for 3 byte VSC header. If cmask[] is not set for an offset, then qemu skips config space check for that offset. VSC check is skipped for machine types >= 9.1. The check would be enforced on older machine types (<= 9.0). Cc: Alex Williamson Cc: Michael S. Tsirkin Cc: Cédric Le Goater Signed-off-by: Vinayak Kale Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/core/machine.c | 1 + hw/vfio/pci.c | 26 ++++++++++++++++++++++++++ hw/vfio/pci.h | 1 + 3 files changed, 28 insertions(+) diff --git a/hw/core/machine.c b/hw/core/machine.c index c7ceb11501..3442f31f94 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -35,6 +35,7 @@ GlobalProperty hw_compat_9_0[] = { {"arm-cpu", "backcompat-cntfrq", "true" }, + {"vfio-pci", "skip-vsc-check", "false" }, }; const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0); diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index fc6e54e871..4789d43c0f 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2134,6 +2134,28 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos) } } +static int vfio_add_vendor_specific_cap(VFIOPCIDevice *vdev, int pos, + uint8_t size, Error **errp) +{ + PCIDevice *pdev = &vdev->pdev; + + pos = pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, size, errp); + if (pos < 0) { + return pos; + } + + /* + * Exempt config space check for Vendor Specific Information during + * restore/load. + * Config space check is still enforced for 3 byte VSC header. + */ + if (vdev->skip_vsc_check && size > 3) { + memset(pdev->cmask + pos + 3, 0, size - 3); + } + + return pos; +} + static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) { ERRP_GUARD(); @@ -2202,6 +2224,9 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) vfio_check_af_flr(vdev, pos); ret = pci_add_capability(pdev, cap_id, pos, size, errp); break; + case PCI_CAP_ID_VNDR: + ret = vfio_add_vendor_specific_cap(vdev, pos, size, errp); + break; default: ret = pci_add_capability(pdev, cap_id, pos, size, errp); break; @@ -3391,6 +3416,7 @@ static Property vfio_pci_dev_properties[] = { DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd, TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *), #endif + DEFINE_PROP_BOOL("skip-vsc-check", VFIOPCIDevice, skip_vsc_check, true), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index 6e64a2654e..92cd62d115 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -177,6 +177,7 @@ struct VFIOPCIDevice { OnOffAuto ramfb_migrate; bool defer_kvm_irq_routing; bool clear_parent_atomics_on_exit; + bool skip_vsc_check; VFIODisplay *dpy; Notifier irqchip_change_notifier; }; From a0359b56ecb2002fbecd249b1ea71df618dbe4a2 Mon Sep 17 00:00:00 2001 From: Avihai Horon Date: Wed, 15 May 2024 16:21:35 +0300 Subject: [PATCH 15/47] qapi/vfio: Add VFIO migration QAPI event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a new QAPI event for VFIO migration. This event will be emitted when a VFIO device changes its migration state, for example, during migration or when stopping/starting the guest. This event can be used by management applications to get updates on the current state of the VFIO device for their own purposes. Note that this new event is introduced since VFIO devices have a unique set of migration states which cannot be described as accurately by other existing events such as run state or migration status. Signed-off-by: Avihai Horon Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- MAINTAINERS | 1 + qapi/meson.build | 1 + qapi/qapi-schema.json | 1 + qapi/vfio.json | 67 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+) create mode 100644 qapi/vfio.json diff --git a/MAINTAINERS b/MAINTAINERS index 1b79767d61..448dc951c5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2164,6 +2164,7 @@ F: hw/vfio/* F: include/hw/vfio/ F: docs/igd-assign.txt F: docs/devel/migration/vfio.rst +F: qapi/vfio.json vfio-ccw M: Eric Farman diff --git a/qapi/meson.build b/qapi/meson.build index c92af6e063..e7bc54e5d0 100644 --- a/qapi/meson.build +++ b/qapi/meson.build @@ -52,6 +52,7 @@ qapi_all_modules = [ 'stats', 'trace', 'transaction', + 'vfio', 'virtio', 'yank', ] diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json index 5e33da7228..b1581988e4 100644 --- a/qapi/qapi-schema.json +++ b/qapi/qapi-schema.json @@ -78,5 +78,6 @@ { 'include': 'pci.json' } { 'include': 'stats.json' } { 'include': 'virtio.json' } +{ 'include': 'vfio.json' } { 'include': 'cryptodev.json' } { 'include': 'cxl.json' } diff --git a/qapi/vfio.json b/qapi/vfio.json new file mode 100644 index 0000000000..a0e5013188 --- /dev/null +++ b/qapi/vfio.json @@ -0,0 +1,67 @@ +# -*- Mode: Python -*- +# vim: filetype=python +# + +## +# = VFIO devices +## + +## +# @VfioMigrationState: +# +# An enumeration of the VFIO device migration states. +# +# @stop: The device is stopped. +# +# @running: The device is running. +# +# @stop-copy: The device is stopped and its internal state is available +# for reading. +# +# @resuming: The device is stopped and its internal state is available +# for writing. +# +# @running-p2p: The device is running in the P2P quiescent state. +# +# @pre-copy: The device is running, tracking its internal state and its +# internal state is available for reading. +# +# @pre-copy-p2p: The device is running in the P2P quiescent state, +# tracking its internal state and its internal state is available +# for reading. +# +# Since: 9.1 +## +{ 'enum': 'VfioMigrationState', + 'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p', + 'pre-copy', 'pre-copy-p2p' ], + 'prefix': 'QAPI_VFIO_MIGRATION_STATE' } + +## +# @VFIO_MIGRATION: +# +# This event is emitted when a VFIO device migration state is changed. +# +# @device-id: The device's id, if it has one. +# +# @qom-path: The device's QOM path. +# +# @device-state: The new changed device migration state. +# +# Since: 9.1 +# +# Example: +# +# <- { "timestamp": { "seconds": 1713771323, "microseconds": 212268 }, +# "event": "VFIO_MIGRATION", +# "data": { +# "device-id": "vfio_dev1", +# "qom-path": "/machine/peripheral/vfio_dev1", +# "device-state": "stop" } } +## +{ 'event': 'VFIO_MIGRATION', + 'data': { + 'device-id': 'str', + 'qom-path': 'str', + 'device-state': 'VfioMigrationState' + } } From 5e1f8905ca72775452f714008c3906c9d86158ae Mon Sep 17 00:00:00 2001 From: Avihai Horon Date: Wed, 15 May 2024 16:21:36 +0300 Subject: [PATCH 16/47] vfio/migration: Emit VFIO migration QAPI event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Emit VFIO migration QAPI event when a VFIO device changes its migration state. This can be used by management applications to get updates on the current state of the VFIO device for their own purposes. A new per VFIO device capability, "migration-events", is added so events can be enabled only for the required devices. It is disabled by default. Signed-off-by: Avihai Horon Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/migration.c | 59 +++++++++++++++++++++++++++++++++-- hw/vfio/pci.c | 2 ++ include/hw/vfio/vfio-common.h | 1 + 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index c4403a38dd..af579b868d 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -24,6 +24,7 @@ #include "migration/register.h" #include "migration/blocker.h" #include "qapi/error.h" +#include "qapi/qapi-events-vfio.h" #include "exec/ramlist.h" #include "exec/ram_addr.h" #include "pci.h" @@ -80,6 +81,58 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state) } } +static VfioMigrationState +mig_state_to_qapi_state(enum vfio_device_mig_state state) +{ + switch (state) { + case VFIO_DEVICE_STATE_STOP: + return QAPI_VFIO_MIGRATION_STATE_STOP; + case VFIO_DEVICE_STATE_RUNNING: + return QAPI_VFIO_MIGRATION_STATE_RUNNING; + case VFIO_DEVICE_STATE_STOP_COPY: + return QAPI_VFIO_MIGRATION_STATE_STOP_COPY; + case VFIO_DEVICE_STATE_RESUMING: + return QAPI_VFIO_MIGRATION_STATE_RESUMING; + case VFIO_DEVICE_STATE_RUNNING_P2P: + return QAPI_VFIO_MIGRATION_STATE_RUNNING_P2P; + case VFIO_DEVICE_STATE_PRE_COPY: + return QAPI_VFIO_MIGRATION_STATE_PRE_COPY; + case VFIO_DEVICE_STATE_PRE_COPY_P2P: + return QAPI_VFIO_MIGRATION_STATE_PRE_COPY_P2P; + default: + g_assert_not_reached(); + } +} + +static void vfio_migration_send_event(VFIODevice *vbasedev) +{ + VFIOMigration *migration = vbasedev->migration; + DeviceState *dev = vbasedev->dev; + g_autofree char *qom_path = NULL; + Object *obj; + + if (!vbasedev->migration_events) { + return; + } + + g_assert(vbasedev->ops->vfio_get_object); + obj = vbasedev->ops->vfio_get_object(vbasedev); + g_assert(obj); + qom_path = object_get_canonical_path(obj); + + qapi_event_send_vfio_migration( + dev->id, qom_path, mig_state_to_qapi_state(migration->device_state)); +} + +static void vfio_migration_set_device_state(VFIODevice *vbasedev, + enum vfio_device_mig_state state) +{ + VFIOMigration *migration = vbasedev->migration; + + migration->device_state = state; + vfio_migration_send_event(vbasedev); +} + static int vfio_migration_set_state(VFIODevice *vbasedev, enum vfio_device_mig_state new_state, enum vfio_device_mig_state recover_state, @@ -131,12 +184,12 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, goto reset_device; } - migration->device_state = recover_state; + vfio_migration_set_device_state(vbasedev, recover_state); return ret; } - migration->device_state = new_state; + vfio_migration_set_device_state(vbasedev, new_state); if (mig_state->data_fd != -1) { if (migration->data_fd != -1) { /* @@ -162,7 +215,7 @@ reset_device: strerror(errno)); } - migration->device_state = VFIO_DEVICE_STATE_RUNNING; + vfio_migration_set_device_state(vbasedev, VFIO_DEVICE_STATE_RUNNING); return ret; } diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 4789d43c0f..b5d1d398b1 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -3388,6 +3388,8 @@ static Property vfio_pci_dev_properties[] = { VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false), DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice, vbasedev.enable_migration, ON_OFF_AUTO_AUTO), + DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice, + vbasedev.migration_events, false), DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false), DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice, vbasedev.ram_block_discard_allowed, false), diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index b6ac249536..878e34a128 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -115,6 +115,7 @@ typedef struct VFIODevice { bool no_mmap; bool ram_block_discard_allowed; OnOffAuto enable_migration; + bool migration_events; VFIODeviceOps *ops; unsigned int num_irqs; unsigned int num_regions; From 64366eddf15a6224263c1152f9b0a1a97965e932 Mon Sep 17 00:00:00 2001 From: Avihai Horon Date: Wed, 15 May 2024 16:21:37 +0300 Subject: [PATCH 17/47] vfio/migration: Don't emit STOP_COPY VFIO migration QAPI event twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When migrating a VFIO device that supports pre-copy, it is transitioned to STOP_COPY twice: once in vfio_vmstate_change() and second time in vfio_save_complete_precopy(). The second transition is harmless, as it's a STOP_COPY->STOP_COPY no-op transition. However, with the newly added VFIO migration QAPI event, the STOP_COPY event is undesirably emitted twice. Prevent this by returning early in vfio_migration_set_state() if new_state is the same as current device state. Note that the STOP_COPY transition in vfio_save_complete_precopy() is essential for VFIO devices that don't support pre-copy, for migrating an already stopped guest and for snapshots. Signed-off-by: Avihai Horon Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/migration.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index af579b868d..56edffaf62 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -150,6 +150,10 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, g_strdup_printf("%s: Failed setting device state to %s.", vbasedev->name, mig_state_to_str(new_state)); + if (new_state == migration->device_state) { + return 0; + } + feature->argsz = sizeof(buf); feature->flags = VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE; From 11ebce2a5544cacab31e8ff169c6f0ceef37f4e8 Mon Sep 17 00:00:00 2001 From: Avihai Horon Date: Wed, 15 May 2024 16:21:38 +0300 Subject: [PATCH 18/47] vfio/migration: Enhance VFIO migration state tracing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move trace_vfio_migration_set_state() to the top of the function, add recover_state to it, and add a new trace event to vfio_migration_set_device_state(). This improves tracing of device state changes as state changes are now also logged when vfio_migration_set_state() fails (covering recover state and device reset transitions) and in no-op state transitions to the same state. Suggested-by: Cédric Le Goater Signed-off-by: Avihai Horon Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/migration.c | 8 ++++++-- hw/vfio/trace-events | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 56edffaf62..34d4be2ce1 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -129,6 +129,9 @@ static void vfio_migration_set_device_state(VFIODevice *vbasedev, { VFIOMigration *migration = vbasedev->migration; + trace_vfio_migration_set_device_state(vbasedev->name, + mig_state_to_str(state)); + migration->device_state = state; vfio_migration_send_event(vbasedev); } @@ -150,6 +153,9 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, g_strdup_printf("%s: Failed setting device state to %s.", vbasedev->name, mig_state_to_str(new_state)); + trace_vfio_migration_set_state(vbasedev->name, mig_state_to_str(new_state), + mig_state_to_str(recover_state)); + if (new_state == migration->device_state) { return 0; } @@ -209,8 +215,6 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, migration->data_fd = mig_state->data_fd; } - trace_vfio_migration_set_state(vbasedev->name, mig_state_to_str(new_state)); - return 0; reset_device: diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index f0474b244b..64161bf6f4 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -152,7 +152,8 @@ vfio_load_device_config_state(const char *name) " (%s)" vfio_load_state(const char *name, uint64_t data) " (%s) data 0x%"PRIx64 vfio_load_state_device_data(const char *name, uint64_t data_size, int ret) " (%s) size 0x%"PRIx64" ret %d" vfio_migration_realize(const char *name) " (%s)" -vfio_migration_set_state(const char *name, const char *state) " (%s) state %s" +vfio_migration_set_device_state(const char *name, const char *state) " (%s) state %s" +vfio_migration_set_state(const char *name, const char *new_state, const char *recover_state) " (%s) new state %s, recover state %s" vfio_migration_state_notifier(const char *name, int state) " (%s) state %d" vfio_save_block(const char *name, int data_size) " (%s) data_size %d" vfio_save_cleanup(const char *name) " (%s)" From 81987bd58b558be71dc4d8adf163f8c787a1c3e9 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:42 +0800 Subject: [PATCH 19/47] vfio/pci: Use g_autofree in vfio_realize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local pointer name is allocated before vfio_attach_device() call and freed after the call. Same for tmp when calling realpath(). Use 'g_autofree' to avoid the g_free() calls. Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/pci.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index b5d1d398b1..84f7bff664 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2972,12 +2972,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) ERRP_GUARD(); VFIOPCIDevice *vdev = VFIO_PCI(pdev); VFIODevice *vbasedev = &vdev->vbasedev; - char *tmp, *subsys; + char *subsys; Error *err = NULL; int i, ret; bool is_mdev; char uuid[UUID_STR_LEN]; - char *name; + g_autofree char *name = NULL; + g_autofree char *tmp = NULL; if (vbasedev->fd < 0 && !vbasedev->sysfsdev) { if (!(~vdev->host.domain || ~vdev->host.bus || @@ -3008,7 +3009,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) */ tmp = g_strdup_printf("%s/subsystem", vbasedev->sysfsdev); subsys = realpath(tmp, NULL); - g_free(tmp); is_mdev = subsys && (strcmp(subsys, "/sys/bus/mdev") == 0); free(subsys); @@ -3029,7 +3029,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) ret = vfio_attach_device(name, vbasedev, pci_device_iommu_address_space(pdev), errp); - g_free(name); if (ret) { goto error; } From f3758413b77d396dd4db68643f7068fb49ec77d9 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:43 +0800 Subject: [PATCH 20/47] vfio/pci: Use g_autofree in iommufd_cdev_get_info_iova_range() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local pointer info is freed before return from iommufd_cdev_get_info_iova_range(). Use 'g_autofree' to avoid the g_free() calls. Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/iommufd.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 8827ffe636..c644127972 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -258,7 +258,7 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container, uint32_t ioas_id, Error **errp) { VFIOContainerBase *bcontainer = &container->bcontainer; - struct iommu_ioas_iova_ranges *info; + g_autofree struct iommu_ioas_iova_ranges *info = NULL; struct iommu_iova_range *iova_ranges; int ret, sz, fd = container->be->fd; @@ -291,12 +291,10 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container, } bcontainer->pgsizes = info->out_iova_alignment; - g_free(info); return 0; error: ret = -errno; - g_free(info); error_setg_errno(errp, errno, "Cannot get IOVA ranges"); return ret; } From b77548355ab5d8c8377ba8f981c7c597507de37a Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:44 +0800 Subject: [PATCH 21/47] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make VFIOIOMMUClass::attach_device() and its wrapper function vfio_attach_device() return bool. This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/ap.c | 6 ++---- hw/vfio/ccw.c | 6 ++---- hw/vfio/common.c | 4 ++-- hw/vfio/container.c | 14 +++++++------- hw/vfio/iommufd.c | 11 +++++------ hw/vfio/pci.c | 5 ++--- hw/vfio/platform.c | 7 +++---- include/hw/vfio/vfio-common.h | 4 ++-- include/hw/vfio/vfio-container-base.h | 4 ++-- 9 files changed, 27 insertions(+), 34 deletions(-) diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index 8bb024e2fd..ba653ef70f 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -154,7 +154,6 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, static void vfio_ap_realize(DeviceState *dev, Error **errp) { ERRP_GUARD(); - int ret; Error *err = NULL; VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev); VFIODevice *vbasedev = &vapdev->vdev; @@ -163,9 +162,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) return; } - ret = vfio_attach_device(vbasedev->name, vbasedev, - &address_space_memory, errp); - if (ret) { + if (!vfio_attach_device(vbasedev->name, vbasedev, + &address_space_memory, errp)) { goto error; } diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 1c630f6e9a..89bb980167 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -579,7 +579,6 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); VFIODevice *vbasedev = &vcdev->vdev; Error *err = NULL; - int ret; /* Call the class init function for subchannel. */ if (cdc->realize) { @@ -593,9 +592,8 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) return; } - ret = vfio_attach_device(cdev->mdevid, vbasedev, - &address_space_memory, errp); - if (ret) { + if (!vfio_attach_device(cdev->mdevid, vbasedev, + &address_space_memory, errp)) { goto out_attach_dev_err; } diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 1fbd10801d..c04a259ffd 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1523,8 +1523,8 @@ retry: return info; } -int vfio_attach_device(char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp) +bool vfio_attach_device(char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp) { const VFIOIOMMUClass *ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 9534120d4a..e7c4167747 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -910,8 +910,8 @@ static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) * @name and @vbasedev->name are likely to be different depending * on the type of the device, hence the need for passing @name */ -static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp) +static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp) { int groupid = vfio_device_groupid(vbasedev, errp); VFIODevice *vbasedev_iter; @@ -920,27 +920,27 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, int ret; if (groupid < 0) { - return groupid; + return false; } trace_vfio_attach_device(vbasedev->name, groupid); group = vfio_get_group(groupid, as, errp); if (!group) { - return -ENOENT; + return false; } QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { error_setg(errp, "device is already attached"); vfio_put_group(group); - return -EBUSY; + return false; } } ret = vfio_get_device(group, name, vbasedev, errp); if (ret) { vfio_put_group(group); - return ret; + return false; } bcontainer = &group->container->bcontainer; @@ -948,7 +948,7 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, container_next); QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next); - return ret; + return true; } static void vfio_legacy_detach_device(VFIODevice *vbasedev) diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index c644127972..4c6992fca1 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -299,8 +299,8 @@ error: return ret; } -static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp) +static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp) { VFIOContainerBase *bcontainer; VFIOIOMMUFDContainer *container; @@ -315,7 +315,7 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, if (vbasedev->fd < 0) { devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); if (devfd < 0) { - return devfd; + return false; } vbasedev->fd = devfd; } else { @@ -392,7 +392,6 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, memory_listener_register(&bcontainer->listener, bcontainer->space->as); if (bcontainer->error) { - ret = -1; error_propagate_prepend(errp, bcontainer->error, "memory listener initialization failed: "); goto err_listener_register; @@ -431,7 +430,7 @@ found_container: trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev->num_irqs, vbasedev->num_regions, vbasedev->flags); - return 0; + return true; err_listener_register: iommufd_cdev_ram_block_discard_disable(false); @@ -444,7 +443,7 @@ err_alloc_ioas: iommufd_cdev_unbind_and_disconnect(vbasedev); err_connect_bind: close(vbasedev->fd); - return ret; + return false; } static void iommufd_cdev_detach(VFIODevice *vbasedev) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 84f7bff664..c1adef5cf8 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -3027,9 +3027,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) name = g_strdup(vbasedev->name); } - ret = vfio_attach_device(name, vbasedev, - pci_device_iommu_address_space(pdev), errp); - if (ret) { + if (!vfio_attach_device(name, vbasedev, + pci_device_iommu_address_space(pdev), errp)) { goto error; } diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index dcd2365fb3..2bd16096bb 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -552,10 +552,9 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) return ret; } - ret = vfio_attach_device(vbasedev->name, vbasedev, - &address_space_memory, errp); - if (ret) { - return ret; + if (!vfio_attach_device(vbasedev->name, vbasedev, + &address_space_memory, errp)) { + return -EINVAL; } ret = vfio_populate_device(vbasedev, errp); diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 878e34a128..e85817e65e 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -222,8 +222,8 @@ void vfio_region_exit(VFIORegion *region); void vfio_region_finalize(VFIORegion *region); void vfio_reset_handler(void *opaque); struct vfio_device_info *vfio_get_device_info(int fd); -int vfio_attach_device(char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp); +bool vfio_attach_device(char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp); void vfio_detach_device(VFIODevice *vbasedev); int vfio_kvm_device_add_fd(int fd, Error **errp); diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index b04057ad1a..44927ca8c3 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -117,8 +117,8 @@ struct VFIOIOMMUClass { int (*dma_unmap)(const VFIOContainerBase *bcontainer, hwaddr iova, ram_addr_t size, IOMMUTLBEntry *iotlb); - int (*attach_device)(const char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp); + bool (*attach_device)(const char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp); void (*detach_device)(VFIODevice *vbasedev); /* migration feature */ From 35b25cf40e4228e922cc9830bd61a7341623023c Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:45 +0800 Subject: [PATCH 22/47] vfio: Make VFIOIOMMUClass::setup() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 10 +++++----- hw/vfio/spapr.c | 12 +++++------- include/hw/vfio/vfio-container-base.h | 2 +- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index e7c4167747..f2e9560a19 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -507,7 +507,7 @@ static void vfio_get_iommu_info_migration(VFIOContainer *container, } } -static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) +static bool vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) { VFIOContainer *container = container_of(bcontainer, VFIOContainer, bcontainer); @@ -517,7 +517,7 @@ static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) ret = vfio_get_iommu_info(container, &info); if (ret) { error_setg_errno(errp, -ret, "Failed to get VFIO IOMMU info"); - return ret; + return false; } if (info->flags & VFIO_IOMMU_INFO_PGSIZES) { @@ -533,7 +533,7 @@ static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) vfio_get_info_iova_range(info, bcontainer); vfio_get_iommu_info_migration(container, info); - return 0; + return true; } static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, @@ -635,8 +635,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, assert(bcontainer->ops->setup); - ret = bcontainer->ops->setup(bcontainer, errp); - if (ret) { + if (!bcontainer->ops->setup(bcontainer, errp)) { + ret = -EINVAL; goto enable_discards_exit; } diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c index 0d949bb728..148b257c9c 100644 --- a/hw/vfio/spapr.c +++ b/hw/vfio/spapr.c @@ -458,8 +458,8 @@ static void vfio_spapr_container_release(VFIOContainerBase *bcontainer) } } -static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, - Error **errp) +static bool vfio_spapr_container_setup(VFIOContainerBase *bcontainer, + Error **errp) { VFIOContainer *container = container_of(bcontainer, VFIOContainer, bcontainer); @@ -480,7 +480,7 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, ret = ioctl(fd, VFIO_IOMMU_ENABLE); if (ret) { error_setg_errno(errp, errno, "failed to enable container"); - return -errno; + return false; } } else { scontainer->prereg_listener = vfio_prereg_listener; @@ -488,7 +488,6 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, memory_listener_register(&scontainer->prereg_listener, &address_space_memory); if (bcontainer->error) { - ret = -1; error_propagate_prepend(errp, bcontainer->error, "RAM memory listener initialization failed: "); goto listener_unregister_exit; @@ -500,7 +499,6 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, if (ret) { error_setg_errno(errp, errno, "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed"); - ret = -errno; goto listener_unregister_exit; } @@ -527,13 +525,13 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, 0x1000); } - return 0; + return true; listener_unregister_exit: if (v2) { memory_listener_unregister(&scontainer->prereg_listener); } - return ret; + return false; } static void vfio_iommu_spapr_class_init(ObjectClass *klass, void *data) diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 44927ca8c3..202e23cb6b 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -110,7 +110,7 @@ struct VFIOIOMMUClass { InterfaceClass parent_class; /* basic feature */ - int (*setup)(VFIOContainerBase *bcontainer, Error **errp); + bool (*setup)(VFIOContainerBase *bcontainer, Error **errp); int (*dma_map)(const VFIOContainerBase *bcontainer, hwaddr iova, ram_addr_t size, void *vaddr, bool readonly); From 33e4c22fd1dd2aa1e1e6f45e8e89c9b961fb72d1 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:46 +0800 Subject: [PATCH 23/47] vfio: Make VFIOIOMMUClass::add_window() and its wrapper return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make VFIOIOMMUClass::add_window() and its wrapper function vfio_container_add_section_window() return bool. This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/common.c | 2 +- hw/vfio/container-base.c | 8 ++++---- hw/vfio/spapr.c | 16 ++++++++-------- include/hw/vfio/vfio-container-base.h | 12 ++++++------ 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index c04a259ffd..f9619a1dfb 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -588,7 +588,7 @@ static void vfio_listener_region_add(MemoryListener *listener, return; } - if (vfio_container_add_section_window(bcontainer, section, &err)) { + if (!vfio_container_add_section_window(bcontainer, section, &err)) { goto fail; } diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c index 26f4bb464a..760d9d0622 100644 --- a/hw/vfio/container-base.c +++ b/hw/vfio/container-base.c @@ -31,12 +31,12 @@ int vfio_container_dma_unmap(VFIOContainerBase *bcontainer, return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb); } -int vfio_container_add_section_window(VFIOContainerBase *bcontainer, - MemoryRegionSection *section, - Error **errp) +bool vfio_container_add_section_window(VFIOContainerBase *bcontainer, + MemoryRegionSection *section, + Error **errp) { if (!bcontainer->ops->add_window) { - return 0; + return true; } return bcontainer->ops->add_window(bcontainer, section, errp); diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c index 148b257c9c..47b040f1bc 100644 --- a/hw/vfio/spapr.c +++ b/hw/vfio/spapr.c @@ -323,7 +323,7 @@ static int vfio_spapr_create_window(VFIOContainer *container, return 0; } -static int +static bool vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, MemoryRegionSection *section, Error **errp) @@ -351,13 +351,13 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, error_setg(errp, "Container %p can't map guest IOVA region" " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova, end); - return -EINVAL; + return false; } - return 0; + return true; } if (container->iommu_type != VFIO_SPAPR_TCE_v2_IOMMU) { - return 0; + return true; } /* For now intersections are not allowed, we may relax this later */ @@ -373,14 +373,14 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, section->offset_within_address_space + int128_get64(section->size) - 1, hostwin->min_iova, hostwin->max_iova); - return -EINVAL; + return false; } } ret = vfio_spapr_create_window(container, section, &pgsize); if (ret) { error_setg_errno(errp, -ret, "Failed to create SPAPR window"); - return ret; + return false; } vfio_host_win_add(scontainer, section->offset_within_address_space, @@ -406,14 +406,14 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, "vfio: failed GROUP_SET_SPAPR_TCE for " "KVM VFIO device %d and group fd %d", param.tablefd, param.groupfd); - return -errno; + return false; } trace_vfio_spapr_group_attach(param.groupfd, param.tablefd); } } } #endif - return 0; + return true; } static void diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 202e23cb6b..2776481fc9 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -76,9 +76,9 @@ int vfio_container_dma_map(VFIOContainerBase *bcontainer, int vfio_container_dma_unmap(VFIOContainerBase *bcontainer, hwaddr iova, ram_addr_t size, IOMMUTLBEntry *iotlb); -int vfio_container_add_section_window(VFIOContainerBase *bcontainer, - MemoryRegionSection *section, - Error **errp); +bool vfio_container_add_section_window(VFIOContainerBase *bcontainer, + MemoryRegionSection *section, + Error **errp); void vfio_container_del_section_window(VFIOContainerBase *bcontainer, MemoryRegionSection *section); int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, @@ -156,9 +156,9 @@ struct VFIOIOMMUClass { int (*pci_hot_reset)(VFIODevice *vbasedev, bool single); /* SPAPR specific */ - int (*add_window)(VFIOContainerBase *bcontainer, - MemoryRegionSection *section, - Error **errp); + bool (*add_window)(VFIOContainerBase *bcontainer, + MemoryRegionSection *section, + Error **errp); void (*del_window)(VFIOContainerBase *bcontainer, MemoryRegionSection *section); void (*release)(VFIOContainerBase *bcontainer); From f6c12eaca52ca014ae1ec57ecee51c57187c2ef6 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:47 +0800 Subject: [PATCH 24/47] vfio/container: Make vfio_connect_container() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index f2e9560a19..802828ddff 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -536,8 +536,8 @@ static bool vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) return true; } -static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, - Error **errp) +static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as, + Error **errp) { VFIOContainer *container; VFIOContainerBase *bcontainer; @@ -589,19 +589,18 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, error_report("vfio: error disconnecting group %d from" " container", group->groupid); } - return ret; + return false; } group->container = container; QLIST_INSERT_HEAD(&container->group_list, group, container_next); vfio_kvm_device_add_group(group); - return 0; + return true; } } fd = qemu_open_old("/dev/vfio/vfio", O_RDWR); if (fd < 0) { error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio"); - ret = -errno; goto put_space_exit; } @@ -609,7 +608,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, if (ret != VFIO_API_VERSION) { error_setg(errp, "supported vfio version: %d, " "reported version: %d", VFIO_API_VERSION, ret); - ret = -EINVAL; goto close_fd_exit; } @@ -636,7 +634,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, assert(bcontainer->ops->setup); if (!bcontainer->ops->setup(bcontainer, errp)) { - ret = -EINVAL; goto enable_discards_exit; } @@ -652,7 +649,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, memory_listener_register(&bcontainer->listener, bcontainer->space->as); if (bcontainer->error) { - ret = -1; error_propagate_prepend(errp, bcontainer->error, "memory listener initialization failed: "); goto listener_release_exit; @@ -660,7 +656,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, bcontainer->initialized = true; - return 0; + return true; listener_release_exit: QLIST_REMOVE(group, container_next); QLIST_REMOVE(bcontainer, next); @@ -685,7 +681,7 @@ close_fd_exit: put_space_exit: vfio_put_address_space(space); - return ret; + return false; } static void vfio_disconnect_container(VFIOGroup *group) @@ -772,7 +768,7 @@ static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp) group->groupid = groupid; QLIST_INIT(&group->device_list); - if (vfio_connect_container(group, as, errp)) { + if (!vfio_connect_container(group, as, errp)) { error_prepend(errp, "failed to setup container for group %d: ", groupid); goto close_fd_exit; From 534ed2e4725225e522bc5d69c219b574c9f164de Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:48 +0800 Subject: [PATCH 25/47] vfio/container: Make vfio_set_iommu() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 802828ddff..e330b28974 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -393,21 +393,20 @@ static const VFIOIOMMUClass *vfio_get_iommu_class(int iommu_type, Error **errp) return VFIO_IOMMU_CLASS(klass); } -static int vfio_set_iommu(VFIOContainer *container, int group_fd, - VFIOAddressSpace *space, Error **errp) +static bool vfio_set_iommu(VFIOContainer *container, int group_fd, + VFIOAddressSpace *space, Error **errp) { - int iommu_type, ret; + int iommu_type; const VFIOIOMMUClass *vioc; iommu_type = vfio_get_iommu_type(container, errp); if (iommu_type < 0) { - return iommu_type; + return false; } - ret = ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd); - if (ret) { + if (ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) { error_setg_errno(errp, errno, "Failed to set group container"); - return -errno; + return false; } while (ioctl(container->fd, VFIO_SET_IOMMU, iommu_type)) { @@ -422,7 +421,7 @@ static int vfio_set_iommu(VFIOContainer *container, int group_fd, continue; } error_setg_errno(errp, errno, "Failed to set iommu for container"); - return -errno; + return false; } container->iommu_type = iommu_type; @@ -430,11 +429,11 @@ static int vfio_set_iommu(VFIOContainer *container, int group_fd, vioc = vfio_get_iommu_class(iommu_type, errp); if (!vioc) { error_setg(errp, "No available IOMMU models"); - return -EINVAL; + return false; } vfio_container_init(&container->bcontainer, space, vioc); - return 0; + return true; } static int vfio_get_iommu_info(VFIOContainer *container, @@ -615,8 +614,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as, container->fd = fd; bcontainer = &container->bcontainer; - ret = vfio_set_iommu(container, group->fd, space, errp); - if (ret) { + if (!vfio_set_iommu(container, group->fd, space, errp)) { goto free_container_exit; } From be1ff306bb31fbecfe3593a2a37493407cee87d8 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:49 +0800 Subject: [PATCH 26/47] vfio/container: Make vfio_get_device() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index e330b28974..53649e397c 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -802,8 +802,8 @@ static void vfio_put_group(VFIOGroup *group) g_free(group); } -static int vfio_get_device(VFIOGroup *group, const char *name, - VFIODevice *vbasedev, Error **errp) +static bool vfio_get_device(VFIOGroup *group, const char *name, + VFIODevice *vbasedev, Error **errp) { g_autofree struct vfio_device_info *info = NULL; int fd; @@ -815,14 +815,14 @@ static int vfio_get_device(VFIOGroup *group, const char *name, error_append_hint(errp, "Verify all devices in group %d are bound to vfio- " "or pci-stub and not already in use\n", group->groupid); - return fd; + return false; } info = vfio_get_device_info(fd); if (!info) { error_setg_errno(errp, errno, "error getting device info"); close(fd); - return -1; + return false; } /* @@ -837,7 +837,7 @@ static int vfio_get_device(VFIOGroup *group, const char *name, error_setg(errp, "Inconsistent setting of support for discarding " "RAM (e.g., balloon) within group"); close(fd); - return -1; + return false; } if (!group->ram_block_discard_allowed) { @@ -858,7 +858,7 @@ static int vfio_get_device(VFIOGroup *group, const char *name, vbasedev->reset_works = !!(info->flags & VFIO_DEVICE_FLAGS_RESET); - return 0; + return true; } static void vfio_put_base_device(VFIODevice *vbasedev) @@ -911,7 +911,6 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, VFIODevice *vbasedev_iter; VFIOGroup *group; VFIOContainerBase *bcontainer; - int ret; if (groupid < 0) { return false; @@ -931,8 +930,7 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, return false; } } - ret = vfio_get_device(group, name, vbasedev, errp); - if (ret) { + if (!vfio_get_device(group, name, vbasedev, errp)) { vfio_put_group(group); return false; } From 45d0d8c4044d330225ef684894e2cdcc05ec0823 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:50 +0800 Subject: [PATCH 27/47] vfio/iommufd: Make iommufd_cdev_*() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand to return bool if 'Error **' is used to pass error. The changed functions include: iommufd_cdev_kvm_device_add iommufd_cdev_connect_and_bind iommufd_cdev_attach_ioas_hwpt iommufd_cdev_detach_ioas_hwpt iommufd_cdev_attach_container iommufd_cdev_get_info_iova_range After the change, all functions in hw/vfio/iommufd.c follows the standand. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/iommufd.c | 88 +++++++++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 49 deletions(-) diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 4c6992fca1..84c86b970e 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -49,9 +49,9 @@ static int iommufd_cdev_unmap(const VFIOContainerBase *bcontainer, container->ioas_id, iova, size); } -static int iommufd_cdev_kvm_device_add(VFIODevice *vbasedev, Error **errp) +static bool iommufd_cdev_kvm_device_add(VFIODevice *vbasedev, Error **errp) { - return vfio_kvm_device_add_fd(vbasedev->fd, errp); + return !vfio_kvm_device_add_fd(vbasedev->fd, errp); } static void iommufd_cdev_kvm_device_del(VFIODevice *vbasedev) @@ -63,18 +63,16 @@ static void iommufd_cdev_kvm_device_del(VFIODevice *vbasedev) } } -static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp) +static bool iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp) { IOMMUFDBackend *iommufd = vbasedev->iommufd; struct vfio_device_bind_iommufd bind = { .argsz = sizeof(bind), .flags = 0, }; - int ret; - ret = iommufd_backend_connect(iommufd, errp); - if (ret) { - return ret; + if (iommufd_backend_connect(iommufd, errp)) { + return false; } /* @@ -82,15 +80,13 @@ static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp) * in KVM. Especially for some emulated devices, it requires * to have kvm information in the device open. */ - ret = iommufd_cdev_kvm_device_add(vbasedev, errp); - if (ret) { + if (!iommufd_cdev_kvm_device_add(vbasedev, errp)) { goto err_kvm_device_add; } /* Bind device to iommufd */ bind.iommufd = iommufd->fd; - ret = ioctl(vbasedev->fd, VFIO_DEVICE_BIND_IOMMUFD, &bind); - if (ret) { + if (ioctl(vbasedev->fd, VFIO_DEVICE_BIND_IOMMUFD, &bind)) { error_setg_errno(errp, errno, "error bind device fd=%d to iommufd=%d", vbasedev->fd, bind.iommufd); goto err_bind; @@ -99,12 +95,12 @@ static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp) vbasedev->devid = bind.out_devid; trace_iommufd_cdev_connect_and_bind(bind.iommufd, vbasedev->name, vbasedev->fd, vbasedev->devid); - return ret; + return true; err_bind: iommufd_cdev_kvm_device_del(vbasedev); err_kvm_device_add: iommufd_backend_disconnect(iommufd); - return ret; + return false; } static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev) @@ -176,10 +172,10 @@ out: return ret; } -static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id, +static bool iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id, Error **errp) { - int ret, iommufd = vbasedev->iommufd->fd; + int iommufd = vbasedev->iommufd->fd; struct vfio_device_attach_iommufd_pt attach_data = { .argsz = sizeof(attach_data), .flags = 0, @@ -187,38 +183,38 @@ static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id, }; /* Attach device to an IOAS or hwpt within iommufd */ - ret = ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data); - if (ret) { + if (ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data)) { error_setg_errno(errp, errno, "[iommufd=%d] error attach %s (%d) to id=%d", iommufd, vbasedev->name, vbasedev->fd, id); - } else { - trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name, - vbasedev->fd, id); + return false; } - return ret; + + trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name, + vbasedev->fd, id); + return true; } -static int iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp) +static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp) { - int ret, iommufd = vbasedev->iommufd->fd; + int iommufd = vbasedev->iommufd->fd; struct vfio_device_detach_iommufd_pt detach_data = { .argsz = sizeof(detach_data), .flags = 0, }; - ret = ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data); - if (ret) { + if (ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data)) { error_setg_errno(errp, errno, "detach %s failed", vbasedev->name); - } else { - trace_iommufd_cdev_detach_ioas_hwpt(iommufd, vbasedev->name); + return false; } - return ret; + + trace_iommufd_cdev_detach_ioas_hwpt(iommufd, vbasedev->name); + return true; } -static int iommufd_cdev_attach_container(VFIODevice *vbasedev, - VFIOIOMMUFDContainer *container, - Error **errp) +static bool iommufd_cdev_attach_container(VFIODevice *vbasedev, + VFIOIOMMUFDContainer *container, + Error **errp) { return iommufd_cdev_attach_ioas_hwpt(vbasedev, container->ioas_id, errp); } @@ -228,7 +224,7 @@ static void iommufd_cdev_detach_container(VFIODevice *vbasedev, { Error *err = NULL; - if (iommufd_cdev_detach_ioas_hwpt(vbasedev, &err)) { + if (!iommufd_cdev_detach_ioas_hwpt(vbasedev, &err)) { error_report_err(err); } } @@ -254,20 +250,19 @@ static int iommufd_cdev_ram_block_discard_disable(bool state) return ram_block_uncoordinated_discard_disable(state); } -static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container, - uint32_t ioas_id, Error **errp) +static bool iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container, + uint32_t ioas_id, Error **errp) { VFIOContainerBase *bcontainer = &container->bcontainer; g_autofree struct iommu_ioas_iova_ranges *info = NULL; struct iommu_iova_range *iova_ranges; - int ret, sz, fd = container->be->fd; + int sz, fd = container->be->fd; info = g_malloc0(sizeof(*info)); info->size = sizeof(*info); info->ioas_id = ioas_id; - ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info); - if (ret && errno != EMSGSIZE) { + if (ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info) && errno != EMSGSIZE) { goto error; } @@ -275,8 +270,7 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container, info = g_realloc(info, sizeof(*info) + sz); info->allowed_iovas = (uintptr_t)(info + 1); - ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info); - if (ret) { + if (ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info)) { goto error; } @@ -291,12 +285,11 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container, } bcontainer->pgsizes = info->out_iova_alignment; - return 0; + return true; error: - ret = -errno; error_setg_errno(errp, errno, "Cannot get IOVA ranges"); - return ret; + return false; } static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, @@ -322,8 +315,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, devfd = vbasedev->fd; } - ret = iommufd_cdev_connect_and_bind(vbasedev, errp); - if (ret) { + if (!iommufd_cdev_connect_and_bind(vbasedev, errp)) { goto err_connect_bind; } @@ -336,7 +328,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, vbasedev->iommufd != container->be) { continue; } - if (iommufd_cdev_attach_container(vbasedev, container, &err)) { + if (!iommufd_cdev_attach_container(vbasedev, container, &err)) { const char *msg = error_get_pretty(err); trace_iommufd_cdev_fail_attach_existing_container(msg); @@ -369,8 +361,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, vfio_container_init(bcontainer, space, iommufd_vioc); QLIST_INSERT_HEAD(&space->containers, bcontainer, next); - ret = iommufd_cdev_attach_container(vbasedev, container, errp); - if (ret) { + if (!iommufd_cdev_attach_container(vbasedev, container, errp)) { goto err_attach_container; } @@ -379,8 +370,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, goto err_discard_disable; } - ret = iommufd_cdev_get_info_iova_range(container, ioas_id, &err); - if (ret) { + if (!iommufd_cdev_get_info_iova_range(container, ioas_id, &err)) { error_append_hint(&err, "Fallback to default 64bit IOVA range and 4K page size\n"); warn_report_err(err); From f38f5dd1d454e41def9cdba899d0b059dd8855d9 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:51 +0800 Subject: [PATCH 28/47] vfio/cpr: Make vfio_cpr_register_container() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 3 +-- hw/vfio/cpr.c | 4 ++-- hw/vfio/iommufd.c | 3 +-- include/hw/vfio/vfio-common.h | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 53649e397c..096cc97258 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -618,8 +618,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as, goto free_container_exit; } - ret = vfio_cpr_register_container(bcontainer, errp); - if (ret) { + if (!vfio_cpr_register_container(bcontainer, errp)) { goto free_container_exit; } diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c index 392c2dd95d..87e51fcee1 100644 --- a/hw/vfio/cpr.c +++ b/hw/vfio/cpr.c @@ -25,12 +25,12 @@ static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier, return 0; } -int vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp) +bool vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp) { migration_add_notifier_mode(&bcontainer->cpr_reboot_notifier, vfio_cpr_reboot_notifier, MIG_MODE_CPR_REBOOT); - return 0; + return true; } void vfio_cpr_unregister_container(VFIOContainerBase *bcontainer) diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 84c86b970e..6a446b16dc 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -396,8 +396,7 @@ found_container: goto err_listener_register; } - ret = vfio_cpr_register_container(bcontainer, errp); - if (ret) { + if (!vfio_cpr_register_container(bcontainer, errp)) { goto err_listener_register; } diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index e85817e65e..b7bb4f5304 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -229,7 +229,7 @@ void vfio_detach_device(VFIODevice *vbasedev); int vfio_kvm_device_add_fd(int fd, Error **errp); int vfio_kvm_device_del_fd(int fd, Error **errp); -int vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp); +bool vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp); void vfio_cpr_unregister_container(VFIOContainerBase *bcontainer); extern const MemoryRegionOps vfio_region_ops; From 9067d50dff29b1b96ef0d4ab7448dbd7b636e55c Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Tue, 7 May 2024 14:42:52 +0800 Subject: [PATCH 29/47] backends/iommufd: Make iommufd_backend_*() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand to return bool if 'Error **' is used to pass error. The changed functions include: iommufd_backend_connect iommufd_backend_alloc_ioas By this chance, simplify the functions a bit by avoiding duplicate recordings, e.g., log through either error interface or trace, not both. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- backends/iommufd.c | 29 +++++++++++++---------------- backends/trace-events | 4 ++-- hw/vfio/iommufd.c | 5 ++--- include/sysemu/iommufd.h | 6 +++--- 4 files changed, 20 insertions(+), 24 deletions(-) diff --git a/backends/iommufd.c b/backends/iommufd.c index 76a0204852..c506afbdac 100644 --- a/backends/iommufd.c +++ b/backends/iommufd.c @@ -72,24 +72,22 @@ static void iommufd_backend_class_init(ObjectClass *oc, void *data) object_class_property_add_str(oc, "fd", NULL, iommufd_backend_set_fd); } -int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp) +bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp) { - int fd, ret = 0; + int fd; if (be->owned && !be->users) { fd = qemu_open_old("/dev/iommu", O_RDWR); if (fd < 0) { error_setg_errno(errp, errno, "/dev/iommu opening failed"); - ret = fd; - goto out; + return false; } be->fd = fd; } be->users++; -out: - trace_iommufd_backend_connect(be->fd, be->owned, - be->users, ret); - return ret; + + trace_iommufd_backend_connect(be->fd, be->owned, be->users); + return true; } void iommufd_backend_disconnect(IOMMUFDBackend *be) @@ -106,25 +104,24 @@ out: trace_iommufd_backend_disconnect(be->fd, be->users); } -int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id, - Error **errp) +bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id, + Error **errp) { - int ret, fd = be->fd; + int fd = be->fd; struct iommu_ioas_alloc alloc_data = { .size = sizeof(alloc_data), .flags = 0, }; - ret = ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data); - if (ret) { + if (ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data)) { error_setg_errno(errp, errno, "Failed to allocate ioas"); - return ret; + return false; } *ioas_id = alloc_data.out_ioas_id; - trace_iommufd_backend_alloc_ioas(fd, *ioas_id, ret); + trace_iommufd_backend_alloc_ioas(fd, *ioas_id); - return ret; + return true; } void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id) diff --git a/backends/trace-events b/backends/trace-events index d45c6e31a6..211e6f374a 100644 --- a/backends/trace-events +++ b/backends/trace-events @@ -7,11 +7,11 @@ dbus_vmstate_loading(const char *id) "id: %s" dbus_vmstate_saving(const char *id) "id: %s" # iommufd.c -iommufd_backend_connect(int fd, bool owned, uint32_t users, int ret) "fd=%d owned=%d users=%d (%d)" +iommufd_backend_connect(int fd, bool owned, uint32_t users) "fd=%d owned=%d users=%d" iommufd_backend_disconnect(int fd, uint32_t users) "fd=%d users=%d" iommu_backend_set_fd(int fd) "pre-opened /dev/iommu fd=%d" iommufd_backend_map_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, void *vaddr, bool readonly, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" addr=%p readonly=%d (%d)" iommufd_backend_unmap_dma_non_exist(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " Unmap nonexistent mapping: iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)" iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)" -iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas, int ret) " iommufd=%d ioas=%d (%d)" +iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d" iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)" diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 6a446b16dc..554f9a6292 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -71,7 +71,7 @@ static bool iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp) .flags = 0, }; - if (iommufd_backend_connect(iommufd, errp)) { + if (!iommufd_backend_connect(iommufd, errp)) { return false; } @@ -346,8 +346,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, } /* Need to allocate a new dedicated container */ - ret = iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp); - if (ret < 0) { + if (!iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp)) { goto err_alloc_ioas; } diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h index 9af27ebd6c..293bfbe967 100644 --- a/include/sysemu/iommufd.h +++ b/include/sysemu/iommufd.h @@ -23,11 +23,11 @@ struct IOMMUFDBackend { /*< public >*/ }; -int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp); +bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp); void iommufd_backend_disconnect(IOMMUFDBackend *be); -int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id, - Error **errp); +bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id, + Error **errp); void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id); int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova, ram_addr_t size, void *vaddr, bool readonly); From 9442d8af674c80a2f8a7358977e1fc7ed43d2776 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:39:56 +0800 Subject: [PATCH 30/47] vfio/display: Fix error path in call site of ramfb_setup() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vfio_display_dmabuf_init() and vfio_display_region_init() calls ramfb_setup() without checking its return value. So we may run into a situation that vfio_display_probe() succeed but errp is set. This is risky and may lead to assert failure in error_setv(). Cc: Gerd Hoffmann Fixes: b290659fc3d ("hw/vfio/display: add ramfb support") Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/display.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hw/vfio/display.c b/hw/vfio/display.c index fe624a6c9b..d28b724102 100644 --- a/hw/vfio/display.c +++ b/hw/vfio/display.c @@ -361,6 +361,9 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp) vdev); if (vdev->enable_ramfb) { vdev->dpy->ramfb = ramfb_setup(errp); + if (!vdev->dpy->ramfb) { + return -EINVAL; + } } vfio_display_edid_init(vdev); return 0; @@ -488,6 +491,9 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp) vdev); if (vdev->enable_ramfb) { vdev->dpy->ramfb = ramfb_setup(errp); + if (!vdev->dpy->ramfb) { + return -EINVAL; + } } return 0; } From 455c009dc4ec13dab51c3764332433013b5cf3cb Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:39:57 +0800 Subject: [PATCH 31/47] vfio/display: Make vfio_display_*() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand in qapi/error.h to return bool for bool-valued functions. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/display.c | 20 ++++++++++---------- hw/vfio/pci.c | 3 +-- hw/vfio/pci.h | 2 +- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/hw/vfio/display.c b/hw/vfio/display.c index d28b724102..661e921616 100644 --- a/hw/vfio/display.c +++ b/hw/vfio/display.c @@ -348,11 +348,11 @@ static const GraphicHwOps vfio_display_dmabuf_ops = { .ui_info = vfio_display_edid_ui_info, }; -static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp) { if (!display_opengl) { error_setg(errp, "vfio-display-dmabuf: opengl not available"); - return -1; + return false; } vdev->dpy = g_new0(VFIODisplay, 1); @@ -362,11 +362,11 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp) if (vdev->enable_ramfb) { vdev->dpy->ramfb = ramfb_setup(errp); if (!vdev->dpy->ramfb) { - return -EINVAL; + return false; } } vfio_display_edid_init(vdev); - return 0; + return true; } static void vfio_display_dmabuf_exit(VFIODisplay *dpy) @@ -483,7 +483,7 @@ static const GraphicHwOps vfio_display_region_ops = { .gfx_update = vfio_display_region_update, }; -static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp) { vdev->dpy = g_new0(VFIODisplay, 1); vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0, @@ -492,10 +492,10 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp) if (vdev->enable_ramfb) { vdev->dpy->ramfb = ramfb_setup(errp); if (!vdev->dpy->ramfb) { - return -EINVAL; + return false; } } - return 0; + return true; } static void vfio_display_region_exit(VFIODisplay *dpy) @@ -510,7 +510,7 @@ static void vfio_display_region_exit(VFIODisplay *dpy) /* ---------------------------------------------------------------------- */ -int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp) +bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp) { struct vfio_device_gfx_plane_info probe; int ret; @@ -533,11 +533,11 @@ int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp) if (vdev->display == ON_OFF_AUTO_AUTO) { /* not an error in automatic mode */ - return 0; + return true; } error_setg(errp, "vfio: device doesn't support any (known) display method"); - return -1; + return false; } void vfio_display_finalize(VFIOPCIDevice *vdev) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index c1adef5cf8..a447013a1d 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -3200,8 +3200,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) } if (vdev->display != ON_OFF_AUTO_OFF) { - ret = vfio_display_probe(vdev, errp); - if (ret) { + if (!vfio_display_probe(vdev, errp)) { goto out_deregister; } } diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index 92cd62d115..a5ac9efd4b 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -232,7 +232,7 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev, Error **errp); void vfio_display_reset(VFIOPCIDevice *vdev); -int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp); +bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp); void vfio_display_finalize(VFIOPCIDevice *vdev); extern const VMStateDescription vfio_display_vmstate; From 50b632b64ce460844089ddd5061460d8b119df5d Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:39:58 +0800 Subject: [PATCH 32/47] vfio/helpers: Use g_autofree in vfio_set_irq_signaling() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local pointer irq_set is freed before return from vfio_set_irq_signaling(). Use 'g_autofree' to avoid the g_free() calls. Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/helpers.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c index 47b4096c05..1f3bdd9bf0 100644 --- a/hw/vfio/helpers.c +++ b/hw/vfio/helpers.c @@ -111,7 +111,7 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, int action, int fd, Error **errp) { ERRP_GUARD(); - struct vfio_irq_set *irq_set; + g_autofree struct vfio_irq_set *irq_set = NULL; int argsz, ret = 0; const char *name; int32_t *pfd; @@ -130,7 +130,6 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) { ret = -errno; } - g_free(irq_set); if (!ret) { return 0; From 84e37d02969ca1c7a6a8670e7d1da8e4ca5d56b9 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:39:59 +0800 Subject: [PATCH 33/47] vfio/helpers: Make vfio_set_irq_signaling() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand in qapi/error.h to return bool for bool-valued functions. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/ap.c | 8 +++---- hw/vfio/ccw.c | 8 +++---- hw/vfio/helpers.c | 18 ++++++---------- hw/vfio/pci.c | 40 ++++++++++++++++++----------------- hw/vfio/platform.c | 18 +++++++--------- include/hw/vfio/vfio-common.h | 4 ++-- 6 files changed, 46 insertions(+), 50 deletions(-) diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index ba653ef70f..d8a9615fee 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -117,8 +117,8 @@ static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev, fd = event_notifier_get_fd(notifier); qemu_set_fd_handler(fd, fd_read, NULL, vapdev); - if (vfio_set_irq_signaling(vdev, irq, 0, VFIO_IRQ_SET_ACTION_TRIGGER, fd, - errp)) { + if (!vfio_set_irq_signaling(vdev, irq, 0, VFIO_IRQ_SET_ACTION_TRIGGER, fd, + errp)) { qemu_set_fd_handler(fd, NULL, NULL, vapdev); event_notifier_cleanup(notifier); } @@ -141,8 +141,8 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, return; } - if (vfio_set_irq_signaling(&vapdev->vdev, irq, 0, - VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { + if (!vfio_set_irq_signaling(&vapdev->vdev, irq, 0, + VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { warn_reportf_err(err, VFIO_MSG_PREFIX, vapdev->vdev.name); } diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 89bb980167..1f578a3c75 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -434,8 +434,8 @@ static bool vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, fd = event_notifier_get_fd(notifier); qemu_set_fd_handler(fd, fd_read, NULL, vcdev); - if (vfio_set_irq_signaling(vdev, irq, 0, - VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) { + if (!vfio_set_irq_signaling(vdev, irq, 0, + VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) { qemu_set_fd_handler(fd, NULL, NULL, vcdev); event_notifier_cleanup(notifier); } @@ -464,8 +464,8 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, return; } - if (vfio_set_irq_signaling(&vcdev->vdev, irq, 0, - VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { + if (!vfio_set_irq_signaling(&vcdev->vdev, irq, 0, + VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { warn_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name); } diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c index 1f3bdd9bf0..9edbc96688 100644 --- a/hw/vfio/helpers.c +++ b/hw/vfio/helpers.c @@ -107,12 +107,12 @@ static const char *index_to_str(VFIODevice *vbasedev, int index) } } -int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, - int action, int fd, Error **errp) +bool vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, + int action, int fd, Error **errp) { ERRP_GUARD(); g_autofree struct vfio_irq_set *irq_set = NULL; - int argsz, ret = 0; + int argsz; const char *name; int32_t *pfd; @@ -127,15 +127,11 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, pfd = (int32_t *)&irq_set->data; *pfd = fd; - if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) { - ret = -errno; + if (!ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) { + return true; } - if (!ret) { - return 0; - } - - error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure"); + error_setg_errno(errp, errno, "VFIO_DEVICE_SET_IRQS failure"); name = index_to_str(vbasedev, index); if (name) { @@ -146,7 +142,7 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, error_prepend(errp, "Failed to %s %s eventfd signaling for interrupt ", fd < 0 ? "tear down" : "set up", action_to_str(action)); - return ret; + return false; } /* diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index a447013a1d..358da4497b 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -147,10 +147,10 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp) goto fail_irqfd; } - if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0, - VFIO_IRQ_SET_ACTION_UNMASK, - event_notifier_get_fd(&vdev->intx.unmask), - errp)) { + if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0, + VFIO_IRQ_SET_ACTION_UNMASK, + event_notifier_get_fd(&vdev->intx.unmask), + errp)) { goto fail_vfio; } @@ -295,8 +295,8 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp) fd = event_notifier_get_fd(&vdev->intx.interrupt); qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev); - if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0, - VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) { + if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0, + VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) { qemu_set_fd_handler(fd, NULL, NULL, vdev); event_notifier_cleanup(&vdev->intx.interrupt); return -errno; @@ -590,9 +590,10 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr, fd = event_notifier_get_fd(&vector->interrupt); } - if (vfio_set_irq_signaling(&vdev->vbasedev, - VFIO_PCI_MSIX_IRQ_INDEX, nr, - VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) { + if (!vfio_set_irq_signaling(&vdev->vbasedev, + VFIO_PCI_MSIX_IRQ_INDEX, nr, + VFIO_IRQ_SET_ACTION_TRIGGER, fd, + &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } } @@ -634,8 +635,9 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr) int32_t fd = event_notifier_get_fd(&vector->interrupt); Error *err = NULL; - if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr, - VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) { + if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, + nr, VFIO_IRQ_SET_ACTION_TRIGGER, fd, + &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } } @@ -2873,8 +2875,8 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev) fd = event_notifier_get_fd(&vdev->err_notifier); qemu_set_fd_handler(fd, vfio_err_notifier_handler, NULL, vdev); - if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0, - VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) { + if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0, + VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); qemu_set_fd_handler(fd, NULL, NULL, vdev); event_notifier_cleanup(&vdev->err_notifier); @@ -2890,8 +2892,8 @@ static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev) return; } - if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0, - VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { + if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0, + VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier), @@ -2938,8 +2940,8 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev) fd = event_notifier_get_fd(&vdev->req_notifier); qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev); - if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0, - VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) { + if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0, + VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); qemu_set_fd_handler(fd, NULL, NULL, vdev); event_notifier_cleanup(&vdev->req_notifier); @@ -2956,8 +2958,8 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) return; } - if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0, - VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { + if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0, + VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier), diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index 2bd16096bb..3233ca8691 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -115,18 +115,17 @@ static int vfio_set_trigger_eventfd(VFIOINTp *intp, VFIODevice *vbasedev = &intp->vdev->vbasedev; int32_t fd = event_notifier_get_fd(intp->interrupt); Error *err = NULL; - int ret; qemu_set_fd_handler(fd, (IOHandler *)handler, NULL, intp); - ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0, - VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err); - if (ret) { + if (!vfio_set_irq_signaling(vbasedev, intp->pin, 0, + VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name); qemu_set_fd_handler(fd, NULL, NULL, NULL); + return -EINVAL; } - return ret; + return 0; } /* @@ -355,15 +354,14 @@ static int vfio_set_resample_eventfd(VFIOINTp *intp) int32_t fd = event_notifier_get_fd(intp->unmask); VFIODevice *vbasedev = &intp->vdev->vbasedev; Error *err = NULL; - int ret; qemu_set_fd_handler(fd, NULL, NULL, NULL); - ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0, - VFIO_IRQ_SET_ACTION_UNMASK, fd, &err); - if (ret) { + if (!vfio_set_irq_signaling(vbasedev, intp->pin, 0, + VFIO_IRQ_SET_ACTION_UNMASK, fd, &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name); + return -EINVAL; } - return ret; + return 0; } /** diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index b7bb4f5304..b712799caf 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -207,8 +207,8 @@ void vfio_spapr_container_deinit(VFIOContainer *container); void vfio_disable_irqindex(VFIODevice *vbasedev, int index); void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index); void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index); -int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, - int action, int fd, Error **errp); +bool vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, + int action, int fd, Error **errp); void vfio_region_write(void *opaque, hwaddr addr, uint64_t data, unsigned size); uint64_t vfio_region_read(void *opaque, From c6c6cf91c0ad2a98d2566de162c3e089b75c2fc4 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:00 +0800 Subject: [PATCH 34/47] vfio/helpers: Make vfio_device_get_name() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand in qapi/error.h to return bool for bool-valued functions. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/ap.c | 2 +- hw/vfio/ccw.c | 2 +- hw/vfio/helpers.c | 8 ++++---- hw/vfio/pci.c | 2 +- hw/vfio/platform.c | 5 ++--- include/hw/vfio/vfio-common.h | 2 +- 6 files changed, 10 insertions(+), 11 deletions(-) diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index d8a9615fee..c12531a788 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -158,7 +158,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev); VFIODevice *vbasedev = &vapdev->vdev; - if (vfio_device_get_name(vbasedev, errp) < 0) { + if (!vfio_device_get_name(vbasedev, errp)) { return; } diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 1f578a3c75..8850ca17c8 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -588,7 +588,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) } } - if (vfio_device_get_name(vbasedev, errp) < 0) { + if (!vfio_device_get_name(vbasedev, errp)) { return; } diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c index 9edbc96688..4b079dc383 100644 --- a/hw/vfio/helpers.c +++ b/hw/vfio/helpers.c @@ -607,7 +607,7 @@ bool vfio_has_region_cap(VFIODevice *vbasedev, int region, uint16_t cap_type) return ret; } -int vfio_device_get_name(VFIODevice *vbasedev, Error **errp) +bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp) { ERRP_GUARD(); struct stat st; @@ -616,7 +616,7 @@ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp) if (stat(vbasedev->sysfsdev, &st) < 0) { error_setg_errno(errp, errno, "no such host device"); error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->sysfsdev); - return -errno; + return false; } /* User may specify a name, e.g: VFIO platform device */ if (!vbasedev->name) { @@ -625,7 +625,7 @@ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp) } else { if (!vbasedev->iommufd) { error_setg(errp, "Use FD passing only with iommufd backend"); - return -EINVAL; + return false; } /* * Give a name with fd so any function printing out vbasedev->name @@ -636,7 +636,7 @@ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp) } } - return 0; + return true; } void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 358da4497b..aad012c348 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2999,7 +2999,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) vdev->host.slot, vdev->host.function); } - if (vfio_device_get_name(vbasedev, errp) < 0) { + if (!vfio_device_get_name(vbasedev, errp)) { return; } diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index 3233ca8691..e1a32863d9 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -545,9 +545,8 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) vbasedev->name); } - ret = vfio_device_get_name(vbasedev, errp); - if (ret) { - return ret; + if (!vfio_device_get_name(vbasedev, errp)) { + return -EINVAL; } if (!vfio_attach_device(vbasedev->name, vbasedev, diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index b712799caf..4cb1ab8645 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -279,7 +279,7 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, 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); +bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp); void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp); void vfio_device_init(VFIODevice *vbasedev, int type, VFIODeviceOps *ops, DeviceState *dev, bool ram_discard); From 958609cfeb0502d3e0a59921b5d039c5c77d57ff Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:01 +0800 Subject: [PATCH 35/47] vfio/platform: Make vfio_populate_device() and vfio_base_device_init() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand in qapi/error.h to return bool for bool-valued functions. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/platform.c | 40 +++++++++++++++++----------------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index e1a32863d9..a85c199c76 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -441,7 +441,7 @@ static int vfio_platform_hot_reset_multi(VFIODevice *vbasedev) * @errp: error object * */ -static int vfio_populate_device(VFIODevice *vbasedev, Error **errp) +static bool vfio_populate_device(VFIODevice *vbasedev, Error **errp) { VFIOINTp *intp, *tmp; int i, ret = -1; @@ -450,7 +450,7 @@ static int vfio_populate_device(VFIODevice *vbasedev, Error **errp) if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PLATFORM)) { error_setg(errp, "this isn't a platform device"); - return ret; + return false; } vdev->regions = g_new0(VFIORegion *, vbasedev->num_regions); @@ -487,12 +487,11 @@ static int vfio_populate_device(VFIODevice *vbasedev, Error **errp) irq.flags); intp = vfio_init_intp(vbasedev, irq, errp); if (!intp) { - ret = -1; goto irq_err; } } } - return 0; + return true; irq_err: timer_del(vdev->mmap_timer); QLIST_FOREACH_SAFE(intp, &vdev->intp_list, next, tmp) { @@ -507,7 +506,7 @@ reg_error: g_free(vdev->regions[i]); } g_free(vdev->regions); - return ret; + return false; } /* specialized functions for VFIO Platform devices */ @@ -527,10 +526,8 @@ static VFIODeviceOps vfio_platform_ops = { * fd retrieval, resource query. * Precondition: the device name must be initialized */ -static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) +static bool vfio_base_device_init(VFIODevice *vbasedev, Error **errp) { - int ret; - /* @fd takes precedence over @sysfsdev which takes precedence over @host */ if (vbasedev->fd < 0 && vbasedev->sysfsdev) { g_free(vbasedev->name); @@ -538,7 +535,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) } else if (vbasedev->fd < 0) { if (!vbasedev->name || strchr(vbasedev->name, '/')) { error_setg(errp, "wrong host device name"); - return -EINVAL; + return false; } vbasedev->sysfsdev = g_strdup_printf("/sys/bus/platform/devices/%s", @@ -546,20 +543,20 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) } if (!vfio_device_get_name(vbasedev, errp)) { - return -EINVAL; + return false; } if (!vfio_attach_device(vbasedev->name, vbasedev, &address_space_memory, errp)) { - return -EINVAL; + return false; } - ret = vfio_populate_device(vbasedev, errp); - if (ret) { - vfio_detach_device(vbasedev); + if (vfio_populate_device(vbasedev, errp)) { + return true; } - return ret; + vfio_detach_device(vbasedev); + return false; } /** @@ -576,7 +573,7 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp) VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev); SysBusDevice *sbdev = SYS_BUS_DEVICE(dev); VFIODevice *vbasedev = &vdev->vbasedev; - int i, ret; + int i; qemu_mutex_init(&vdev->intp_mutex); @@ -584,9 +581,8 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp) vbasedev->sysfsdev : vbasedev->name, vdev->compat); - ret = vfio_base_device_init(vbasedev, errp); - if (ret) { - goto out; + if (!vfio_base_device_init(vbasedev, errp)) { + goto init_err; } if (!vdev->compat) { @@ -618,11 +614,9 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp) } sysbus_init_mmio(sbdev, vdev->regions[i]->mem); } -out: - if (!ret) { - return; - } + return; +init_err: if (vdev->vbasedev.name) { error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name); } else { From 040f8d105057383b42c162424a99a468f67a97f4 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:02 +0800 Subject: [PATCH 36/47] vfio/ccw: Make vfio_ccw_get_region() return a bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since vfio_populate_device() takes an 'Error **' argument, best practices suggest to return a bool. See the qapi/error.h Rules section. Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/ccw.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 8850ca17c8..2600e62e37 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -474,7 +474,7 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, event_notifier_cleanup(notifier); } -static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) +static bool vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) { VFIODevice *vdev = &vcdev->vdev; struct vfio_region_info *info; @@ -483,7 +483,7 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) /* Sanity check device */ if (!(vdev->flags & VFIO_DEVICE_FLAGS_CCW)) { error_setg(errp, "vfio: Um, this isn't a vfio-ccw device"); - return; + return false; } /* @@ -493,13 +493,13 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) if (vdev->num_regions < VFIO_CCW_CONFIG_REGION_INDEX + 1) { error_setg(errp, "vfio: too few regions (%u), expected at least %u", vdev->num_regions, VFIO_CCW_CONFIG_REGION_INDEX + 1); - return; + return false; } ret = vfio_get_region_info(vdev, VFIO_CCW_CONFIG_REGION_INDEX, &info); if (ret) { error_setg_errno(errp, -ret, "vfio: Error getting config info"); - return; + return false; } vcdev->io_region_size = info->size; @@ -553,7 +553,7 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) g_free(info); } - return; + return true; out_err: g_free(vcdev->crw_region); @@ -561,7 +561,7 @@ out_err: g_free(vcdev->async_cmd_region); g_free(vcdev->io_region); g_free(info); - return; + return false; } static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) @@ -597,8 +597,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) goto out_attach_dev_err; } - vfio_ccw_get_region(vcdev, &err); - if (err) { + if (!vfio_ccw_get_region(vcdev, &err)) { goto out_region_err; } From 44cd660a992df4bee1723fb8899c1d9bb9f8de0b Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:03 +0800 Subject: [PATCH 37/47] vfio/pci: Make vfio_intx_enable_kvm() return a bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since vfio_intx_enable_kvm() takes an 'Error **' argument, best practices suggest to return a bool. See the qapi/error.h Rules section. Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/pci.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index aad012c348..12fb534d79 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -116,7 +116,7 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); } -static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp) { #ifdef CONFIG_KVM int irq_fd = event_notifier_get_fd(&vdev->intx.interrupt); @@ -124,7 +124,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp) if (vdev->no_kvm_intx || !kvm_irqfds_enabled() || vdev->intx.route.mode != PCI_INTX_ENABLED || !kvm_resamplefds_enabled()) { - return; + return true; } /* Get to a known interrupt state */ @@ -161,7 +161,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp) trace_vfio_intx_enable_kvm(vdev->vbasedev.name); - return; + return true; fail_vfio: kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, &vdev->intx.interrupt, @@ -171,6 +171,9 @@ fail_irqfd: fail: qemu_set_fd_handler(irq_fd, vfio_intx_interrupt, NULL, vdev); vfio_unmask_single_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX); + return false; +#else + return true; #endif } @@ -226,8 +229,7 @@ static void vfio_intx_update(VFIOPCIDevice *vdev, PCIINTxRoute *route) return; } - vfio_intx_enable_kvm(vdev, &err); - if (err) { + if (!vfio_intx_enable_kvm(vdev, &err)) { warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } @@ -302,8 +304,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp) return -errno; } - vfio_intx_enable_kvm(vdev, &err); - if (err) { + if (!vfio_intx_enable_kvm(vdev, &err)) { warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } From 713b59a674b407b894161ada0b6541ce4e0f68d7 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:04 +0800 Subject: [PATCH 38/47] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() return a bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since vfio_pci_relocate_msix() and vfio_msix_early_setup() takes an 'Error **' argument, best practices suggest to return a bool. See the qapi/error.h Rules section. By this chance, pass errp directly to vfio_msix_early_setup() to avoid calling error_propagate(). Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/pci.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 12fb534d79..4fb5fd0c9f 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1450,13 +1450,13 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev) } } -static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp) { int target_bar = -1; size_t msix_sz; if (!vdev->msix || vdev->msix_relo == OFF_AUTOPCIBAR_OFF) { - return; + return true; } /* The actual minimum size of MSI-X structures */ @@ -1479,7 +1479,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp) if (target_bar < 0) { error_setg(errp, "No automatic MSI-X relocation available for " "device %04x:%04x", vdev->vendor_id, vdev->device_id); - return; + return false; } } else { target_bar = (int)(vdev->msix_relo - OFF_AUTOPCIBAR_BAR0); @@ -1489,7 +1489,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp) if (vdev->bars[target_bar].ioport) { error_setg(errp, "Invalid MSI-X relocation BAR %d, " "I/O port BAR", target_bar); - return; + return false; } /* Cannot use a BAR in the "shadow" of a 64-bit BAR */ @@ -1497,7 +1497,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp) target_bar > 0 && vdev->bars[target_bar - 1].mem64) { error_setg(errp, "Invalid MSI-X relocation BAR %d, " "consumed by 64-bit BAR %d", target_bar, target_bar - 1); - return; + return false; } /* 2GB max size for 32-bit BARs, cannot double if already > 1G */ @@ -1505,7 +1505,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp) !vdev->bars[target_bar].mem64) { error_setg(errp, "Invalid MSI-X relocation BAR %d, " "no space to extend 32-bit BAR", target_bar); - return; + return false; } /* @@ -1540,6 +1540,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp) trace_vfio_msix_relo(vdev->vbasedev.name, vdev->msix->table_bar, vdev->msix->table_offset); + return true; } /* @@ -1550,7 +1551,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp) * need to first look for where the MSI-X table lives. So we * unfortunately split MSI-X setup across two functions. */ -static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) { uint8_t pos; uint16_t ctrl; @@ -1562,25 +1563,25 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX); if (!pos) { - return; + return true; } if (pread(fd, &ctrl, sizeof(ctrl), vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) { error_setg_errno(errp, errno, "failed to read PCI MSIX FLAGS"); - return; + return false; } if (pread(fd, &table, sizeof(table), vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) { error_setg_errno(errp, errno, "failed to read PCI MSIX TABLE"); - return; + return false; } if (pread(fd, &pba, sizeof(pba), vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) { error_setg_errno(errp, errno, "failed to read PCI MSIX PBA"); - return; + return false; } ctrl = le16_to_cpu(ctrl); @@ -1598,7 +1599,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) if (ret < 0) { error_setg_errno(errp, -ret, "failed to get MSI-X irq info"); g_free(msix); - return; + return false; } msix->noresize = !!(irq_info.flags & VFIO_IRQ_INFO_NORESIZE); @@ -1630,7 +1631,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) error_setg(errp, "hardware reports invalid configuration, " "MSIX PBA outside of specified BAR"); g_free(msix); - return; + return false; } } @@ -1641,7 +1642,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) vfio_pci_fixup_msix_region(vdev); - vfio_pci_relocate_msix(vdev, errp); + return vfio_pci_relocate_msix(vdev, errp); } static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) @@ -3130,9 +3131,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) vfio_bars_prepare(vdev); - vfio_msix_early_setup(vdev, &err); - if (err) { - error_propagate(errp, err); + if (!vfio_msix_early_setup(vdev, errp)) { goto error; } From e942d8f08dd54c5b4ca309e8ccd22193192543a5 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:05 +0800 Subject: [PATCH 39/47] vfio/pci: Make vfio_populate_device() return a bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since vfio_populate_device() takes an 'Error **' argument, best practices suggest to return a bool. See the qapi/error.h Rules section. By this chance, pass errp directly to vfio_populate_device() to avoid calling error_propagate(). Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/pci.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 4fb5fd0c9f..46d3c61859 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2740,7 +2740,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) return 0; } -static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) { VFIODevice *vbasedev = &vdev->vbasedev; struct vfio_region_info *reg_info; @@ -2750,18 +2750,18 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) /* Sanity check device */ if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) { error_setg(errp, "this isn't a PCI device"); - return; + return false; } if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) { error_setg(errp, "unexpected number of io regions %u", vbasedev->num_regions); - return; + return false; } if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) { error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs); - return; + return false; } for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) { @@ -2773,7 +2773,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) if (ret) { error_setg_errno(errp, -ret, "failed to get region %d info", i); - return; + return false; } QLIST_INIT(&vdev->bars[i].quirks); @@ -2783,7 +2783,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) VFIO_PCI_CONFIG_REGION_INDEX, ®_info); if (ret) { error_setg_errno(errp, -ret, "failed to get config info"); - return; + return false; } trace_vfio_populate_device_config(vdev->vbasedev.name, @@ -2804,7 +2804,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) if (ret) { error_append_hint(errp, "device does not support " "requested feature x-vga\n"); - return; + return false; } } @@ -2821,6 +2821,8 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) "Could not enable error recovery for the device", vbasedev->name); } + + return true; } static void vfio_pci_put_device(VFIOPCIDevice *vdev) @@ -2977,7 +2979,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) VFIOPCIDevice *vdev = VFIO_PCI(pdev); VFIODevice *vbasedev = &vdev->vbasedev; char *subsys; - Error *err = NULL; int i, ret; bool is_mdev; char uuid[UUID_STR_LEN]; @@ -3036,9 +3037,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) goto error; } - vfio_populate_device(vdev, &err); - if (err) { - error_propagate(errp, err); + if (!vfio_populate_device(vdev, errp)) { goto error; } From c32bab074ebac4dc872dabe801efe6581f73daba Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:06 +0800 Subject: [PATCH 40/47] vfio/pci: Make vfio_intx_enable() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand in qapi/error.h to return bool for bool-valued functions. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/pci.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 46d3c61859..7f35cb8a29 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -261,7 +261,7 @@ static void vfio_irqchip_change(Notifier *notify, void *data) vfio_intx_update(vdev, &vdev->intx.route); } -static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp) { uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1); Error *err = NULL; @@ -270,7 +270,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp) if (!pin) { - return 0; + return true; } vfio_disable_interrupts(vdev); @@ -292,7 +292,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp) ret = event_notifier_init(&vdev->intx.interrupt, 0); if (ret) { error_setg_errno(errp, -ret, "event_notifier_init failed"); - return ret; + return false; } fd = event_notifier_get_fd(&vdev->intx.interrupt); qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev); @@ -301,7 +301,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp) VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) { qemu_set_fd_handler(fd, NULL, NULL, vdev); event_notifier_cleanup(&vdev->intx.interrupt); - return -errno; + return false; } if (!vfio_intx_enable_kvm(vdev, &err)) { @@ -311,7 +311,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp) vdev->interrupt = VFIO_INT_INTx; trace_vfio_intx_enable(vdev->vbasedev.name); - return 0; + return true; } static void vfio_intx_disable(VFIOPCIDevice *vdev) @@ -836,8 +836,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev) vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX); vfio_msi_disable_common(vdev); - vfio_intx_enable(vdev, &err); - if (err) { + if (!vfio_intx_enable(vdev, &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } @@ -2450,8 +2449,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev) Error *err = NULL; int nr; - vfio_intx_enable(vdev, &err); - if (err) { + if (!vfio_intx_enable(vdev, &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } @@ -3194,8 +3192,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) vfio_intx_routing_notifier); vdev->irqchip_change_notifier.notify = vfio_irqchip_change; kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier); - ret = vfio_intx_enable(vdev, errp); - if (ret) { + if (!vfio_intx_enable(vdev, errp)) { goto out_deregister; } } From 64410a741d57b62f736562a89b89d0497b3033ce Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:07 +0800 Subject: [PATCH 41/47] vfio/pci: Make vfio_populate_vga() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand in qapi/error.h to return bool for bool-valued functions. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/igd.c | 2 +- hw/vfio/pci.c | 11 +++++------ hw/vfio/pci.h | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index b31ee79c60..ffe57c5954 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -478,7 +478,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) * try to enable it. Probably shouldn't be using legacy mode without VGA, * but also no point in us enabling VGA if disabled in hardware. */ - if (!(gmch & 0x2) && !vdev->vga && vfio_populate_vga(vdev, &err)) { + if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) { error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); error_report("IGD device %s failed to enable VGA access, " "legacy mode disabled", vdev->vbasedev.name); diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 7f35cb8a29..ab8f74299e 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2670,7 +2670,7 @@ static VFIODeviceOps vfio_pci_ops = { .vfio_load_config = vfio_pci_load_config, }; -int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) +bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) { VFIODevice *vbasedev = &vdev->vbasedev; struct vfio_region_info *reg_info; @@ -2681,7 +2681,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) error_setg_errno(errp, -ret, "failed getting region info for VGA region index %d", VFIO_PCI_VGA_REGION_INDEX); - return ret; + return false; } if (!(reg_info->flags & VFIO_REGION_INFO_FLAG_READ) || @@ -2691,7 +2691,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) (unsigned long)reg_info->flags, (unsigned long)reg_info->size); g_free(reg_info); - return -EINVAL; + return false; } vdev->vga = g_new0(VFIOVGA, 1); @@ -2735,7 +2735,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) &vdev->vga->region[QEMU_PCI_VGA_IO_LO].mem, &vdev->vga->region[QEMU_PCI_VGA_IO_HI].mem); - return 0; + return true; } static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) @@ -2798,8 +2798,7 @@ static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) g_free(reg_info); if (vdev->features & VFIO_FEATURE_ENABLE_VGA) { - ret = vfio_populate_vga(vdev, errp); - if (ret) { + if (!vfio_populate_vga(vdev, errp)) { error_append_hint(errp, "device does not support " "requested feature x-vga\n"); return false; diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index a5ac9efd4b..7914f019d5 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -225,7 +225,7 @@ bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name); int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev, struct vfio_pci_hot_reset_info **info_p); -int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp); +bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp); int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev, struct vfio_region_info *info, From b771a40f9eecbffee70a7425e5a96feb58f85d99 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:08 +0800 Subject: [PATCH 42/47] vfio/pci: Make capability related functions return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The functions operating on capability don't have a consistent return style. Below functions are in bool-valued functions style: vfio_msi_setup() vfio_msix_setup() vfio_add_std_cap() vfio_add_capabilities() Below two are integer-valued functions: vfio_add_vendor_specific_cap() vfio_setup_pcie_cap() But the returned integer is only used for check succeed/failure. Change them all to return bool so now all capability related functions follow the coding standand in qapi/error.h to return bool. Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/pci.c | 77 ++++++++++++++++++++++++--------------------------- 1 file changed, 36 insertions(+), 41 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index ab8f74299e..c3323912dd 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1339,7 +1339,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev) } } -static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp) +static bool vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp) { uint16_t ctrl; bool msi_64bit, msi_maskbit; @@ -1349,7 +1349,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp) if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { error_setg_errno(errp, errno, "failed reading MSI PCI_CAP_FLAGS"); - return -errno; + return false; } ctrl = le16_to_cpu(ctrl); @@ -1362,14 +1362,14 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp) ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &err); if (ret < 0) { if (ret == -ENOTSUP) { - return 0; + return true; } error_propagate_prepend(errp, err, "msi_init failed: "); - return ret; + return false; } vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0); - return 0; + return true; } static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev) @@ -1644,7 +1644,7 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) return vfio_pci_relocate_msix(vdev, errp); } -static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) +static bool vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) { int ret; Error *err = NULL; @@ -1660,11 +1660,11 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) if (ret < 0) { if (ret == -ENOTSUP) { warn_report_err(err); - return 0; + return true; } error_propagate(errp, err); - return ret; + return false; } /* @@ -1698,7 +1698,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false); } - return 0; + return true; } static void vfio_teardown_msi(VFIOPCIDevice *vdev) @@ -1977,8 +1977,8 @@ static void vfio_pci_disable_rp_atomics(VFIOPCIDevice *vdev) } } -static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, - Error **errp) +static bool vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, + Error **errp) { uint16_t flags; uint8_t type; @@ -1992,7 +1992,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, error_setg(errp, "assignment of PCIe type 0x%x " "devices is not currently supported", type); - return -EINVAL; + return false; } if (!pci_bus_is_express(pci_get_bus(&vdev->pdev))) { @@ -2025,7 +2025,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, } if (pci_bus_is_express(bus)) { - return 0; + return true; } } else if (pci_bus_is_root(pci_get_bus(&vdev->pdev))) { @@ -2063,7 +2063,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, * Legacy endpoints don't belong on the root complex. Windows * seems to be happier with devices if we skip the capability. */ - return 0; + return true; } } else { @@ -2099,12 +2099,12 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size, errp); if (pos < 0) { - return pos; + return false; } vdev->pdev.exp.exp_cap = pos; - return pos; + return true; } static void vfio_check_pcie_flr(VFIOPCIDevice *vdev, uint8_t pos) @@ -2137,14 +2137,14 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos) } } -static int vfio_add_vendor_specific_cap(VFIOPCIDevice *vdev, int pos, - uint8_t size, Error **errp) +static bool vfio_add_vendor_specific_cap(VFIOPCIDevice *vdev, int pos, + uint8_t size, Error **errp) { PCIDevice *pdev = &vdev->pdev; pos = pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, size, errp); if (pos < 0) { - return pos; + return false; } /* @@ -2156,15 +2156,15 @@ static int vfio_add_vendor_specific_cap(VFIOPCIDevice *vdev, int pos, memset(pdev->cmask + pos + 3, 0, size - 3); } - return pos; + return true; } -static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) +static bool vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) { ERRP_GUARD(); PCIDevice *pdev = &vdev->pdev; uint8_t cap_id, next, size; - int ret; + bool ret; cap_id = pdev->config[pos]; next = pdev->config[pos + PCI_CAP_LIST_NEXT]; @@ -2185,9 +2185,8 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) * will be changed as we unwind the stack. */ if (next) { - ret = vfio_add_std_cap(vdev, next, errp); - if (ret) { - return ret; + if (!vfio_add_std_cap(vdev, next, errp)) { + return false; } } else { /* Begin the rebuild, use QEMU emulated list bits */ @@ -2197,7 +2196,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) ret = vfio_add_virt_caps(vdev, errp); if (ret) { - return ret; + return false; } } @@ -2221,28 +2220,27 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) case PCI_CAP_ID_PM: vfio_check_pm_reset(vdev, pos); vdev->pm_cap = pos; - ret = pci_add_capability(pdev, cap_id, pos, size, errp); + ret = pci_add_capability(pdev, cap_id, pos, size, errp) >= 0; break; case PCI_CAP_ID_AF: vfio_check_af_flr(vdev, pos); - ret = pci_add_capability(pdev, cap_id, pos, size, errp); + ret = pci_add_capability(pdev, cap_id, pos, size, errp) >= 0; break; case PCI_CAP_ID_VNDR: ret = vfio_add_vendor_specific_cap(vdev, pos, size, errp); break; default: - ret = pci_add_capability(pdev, cap_id, pos, size, errp); + ret = pci_add_capability(pdev, cap_id, pos, size, errp) >= 0; break; } - if (ret < 0) { + if (!ret) { error_prepend(errp, "failed to add PCI capability 0x%x[0x%x]@0x%x: ", cap_id, size, pos); - return ret; } - return 0; + return ret; } static int vfio_setup_rebar_ecap(VFIOPCIDevice *vdev, uint16_t pos) @@ -2388,23 +2386,21 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev) return; } -static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp) { PCIDevice *pdev = &vdev->pdev; - int ret; if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) || !pdev->config[PCI_CAPABILITY_LIST]) { - return 0; /* Nothing to add */ + return true; /* Nothing to add */ } - ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp); - if (ret) { - return ret; + if (!vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp)) { + return false; } vfio_add_ext_cap(vdev); - return 0; + return true; } void vfio_pci_pre_reset(VFIOPCIDevice *vdev) @@ -3133,8 +3129,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) vfio_bars_register(vdev); - ret = vfio_add_capabilities(vdev, errp); - if (ret) { + if (!vfio_add_capabilities(vdev, errp)) { goto out_teardown; } From 514855e18fcdc3378fb925046d89a0f1493336db Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:09 +0800 Subject: [PATCH 43/47] vfio/pci: Use g_autofree for vfio_region_info pointer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pointer opregion is freed after vfio_pci_igd_opregion_init(). Use 'g_autofree' to avoid the g_free() calls. Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/pci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index c3323912dd..8379d2284a 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -3143,7 +3143,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) if (!vdev->igd_opregion && vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) { - struct vfio_region_info *opregion; + g_autofree struct vfio_region_info *opregion = NULL; if (vdev->pdev.qdev.hotplugged) { error_setg(errp, @@ -3162,7 +3162,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) } ret = vfio_pci_igd_opregion_init(vdev, opregion, errp); - g_free(opregion); if (ret) { goto out_teardown; } From d3c6a18bc7affd3b89903d98d8a3125a3e1c9861 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:10 +0800 Subject: [PATCH 44/47] vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand in qapi/error.h to return bool for bool-valued functions. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/igd.c | 3 +-- hw/vfio/pci-quirks.c | 8 ++++---- hw/vfio/pci.c | 3 +-- hw/vfio/pci.h | 6 +++--- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index ffe57c5954..402fc5ce1d 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -502,8 +502,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) } /* Setup OpRegion access */ - ret = vfio_pci_igd_opregion_init(vdev, opregion, &err); - if (ret) { + if (!vfio_pci_igd_opregion_init(vdev, opregion, &err)) { error_append_hint(&err, "IGD legacy mode disabled\n"); error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); goto out; diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c index 496fd1ee86..ca27917159 100644 --- a/hw/vfio/pci-quirks.c +++ b/hw/vfio/pci-quirks.c @@ -1169,8 +1169,8 @@ static void vfio_probe_rtl8168_bar2_quirk(VFIOPCIDevice *vdev, int nr) * the table and to write the base address of that memory to the ASLS register * of the IGD device. */ -int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev, - struct vfio_region_info *info, Error **errp) +bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev, + struct vfio_region_info *info, Error **errp) { int ret; @@ -1181,7 +1181,7 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev, error_setg(errp, "failed to read IGD OpRegion"); g_free(vdev->igd_opregion); vdev->igd_opregion = NULL; - return -EINVAL; + return false; } /* @@ -1206,7 +1206,7 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev, pci_set_long(vdev->pdev.wmask + IGD_ASLS, ~0); pci_set_long(vdev->emulated_config_bits + IGD_ASLS, ~0); - return 0; + return true; } /* diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 8379d2284a..76a3931dba 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -3161,8 +3161,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) goto out_teardown; } - ret = vfio_pci_igd_opregion_init(vdev, opregion, errp); - if (ret) { + if (!vfio_pci_igd_opregion_init(vdev, opregion, errp)) { goto out_teardown; } } diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index 7914f019d5..f158681072 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -227,9 +227,9 @@ int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev, bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp); -int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev, - struct vfio_region_info *info, - Error **errp); +bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev, + struct vfio_region_info *info, + Error **errp); void vfio_display_reset(VFIOPCIDevice *vdev); bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp); From 0a0bda0acd529161d5cbed6c603ab89674a9af8c Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:11 +0800 Subject: [PATCH 45/47] vfio/pci-quirks: Make vfio_add_*_cap() return bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to follow the coding standand in qapi/error.h to return bool for bool-valued functions. Include below functions: vfio_add_virt_caps() vfio_add_nv_gpudirect_cap() vfio_add_vmd_shadow_cap() Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/pci-quirks.c | 42 +++++++++++++++++++----------------------- hw/vfio/pci.c | 3 +-- hw/vfio/pci.h | 2 +- 3 files changed, 21 insertions(+), 26 deletions(-) diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c index ca27917159..39dae72497 100644 --- a/hw/vfio/pci-quirks.c +++ b/hw/vfio/pci-quirks.c @@ -1536,7 +1536,7 @@ static bool is_valid_std_cap_offset(uint8_t pos) pos <= (PCI_CFG_SPACE_SIZE - PCI_CAP_SIZEOF)); } -static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) { ERRP_GUARD(); PCIDevice *pdev = &vdev->pdev; @@ -1545,18 +1545,18 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) uint8_t tmp; if (vdev->nv_gpudirect_clique == 0xFF) { - return 0; + return true; } if (!vfio_pci_is(vdev, PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID)) { error_setg(errp, "NVIDIA GPUDirect Clique ID: invalid device vendor"); - return -EINVAL; + return false; } if (pci_get_byte(pdev->config + PCI_CLASS_DEVICE + 1) != PCI_BASE_CLASS_DISPLAY) { error_setg(errp, "NVIDIA GPUDirect Clique ID: unsupported PCI class"); - return -EINVAL; + return false; } /* @@ -1572,7 +1572,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) vdev->config_offset + PCI_CAPABILITY_LIST); if (ret != 1 || !is_valid_std_cap_offset(tmp)) { error_setg(errp, "NVIDIA GPUDirect Clique ID: error getting cap list"); - return -EINVAL; + return false; } do { @@ -1590,13 +1590,13 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) pos = 0xD4; } else { error_setg(errp, "NVIDIA GPUDirect Clique ID: invalid config space"); - return -EINVAL; + return false; } ret = pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, 8, errp); if (ret < 0) { error_prepend(errp, "Failed to add NVIDIA GPUDirect cap: "); - return ret; + return false; } memset(vdev->emulated_config_bits + pos, 0xFF, 8); @@ -1608,7 +1608,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) pci_set_byte(pdev->config + pos++, vdev->nv_gpudirect_clique << 3); pci_set_byte(pdev->config + pos, 0); - return 0; + return true; } /* @@ -1629,7 +1629,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) */ #define VMD_SHADOW_CAP_VER 1 #define VMD_SHADOW_CAP_LEN 24 -static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp) { ERRP_GUARD(); uint8_t membar_phys[16]; @@ -1639,7 +1639,7 @@ static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp) vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, 0x467F) || vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, 0x4C3D) || vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, 0x9A0B))) { - return 0; + return true; } ret = pread(vdev->vbasedev.fd, membar_phys, 16, @@ -1647,14 +1647,14 @@ static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp) if (ret != 16) { error_report("VMD %s cannot read MEMBARs (%d)", vdev->vbasedev.name, ret); - return -EFAULT; + return false; } ret = pci_add_capability(&vdev->pdev, PCI_CAP_ID_VNDR, pos, VMD_SHADOW_CAP_LEN, errp); if (ret < 0) { error_prepend(errp, "Failed to add VMD MEMBAR Shadow cap: "); - return ret; + return false; } memset(vdev->emulated_config_bits + pos, 0xFF, VMD_SHADOW_CAP_LEN); @@ -1664,22 +1664,18 @@ static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp) pci_set_long(vdev->pdev.config + pos, 0x53484457); /* SHDW */ memcpy(vdev->pdev.config + pos + 4, membar_phys, 16); - return 0; + return true; } -int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp) +bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp) { - int ret; - - ret = vfio_add_nv_gpudirect_cap(vdev, errp); - if (ret) { - return ret; + if (!vfio_add_nv_gpudirect_cap(vdev, errp)) { + return false; } - ret = vfio_add_vmd_shadow_cap(vdev, errp); - if (ret) { - return ret; + if (!vfio_add_vmd_shadow_cap(vdev, errp)) { + return false; } - return 0; + return true; } diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 76a3931dba..35ad9b582f 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2194,8 +2194,7 @@ static bool vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) vdev->emulated_config_bits[PCI_CAPABILITY_LIST] = 0xff; vdev->emulated_config_bits[PCI_STATUS] |= PCI_STATUS_CAP_LIST; - ret = vfio_add_virt_caps(vdev, errp); - if (ret) { + if (!vfio_add_virt_caps(vdev, errp)) { return false; } } diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index f158681072..bf67df2fbc 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -212,7 +212,7 @@ void vfio_bar_quirk_setup(VFIOPCIDevice *vdev, int nr); void vfio_bar_quirk_exit(VFIOPCIDevice *vdev, int nr); void vfio_bar_quirk_finalize(VFIOPCIDevice *vdev, int nr); void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev); -int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp); +bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp); void vfio_quirk_reset(VFIOPCIDevice *vdev); VFIOQuirk *vfio_quirk_alloc(int nr_mem); void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr); From 0d3e89bea8ff2fb74b829d80a8e3ebe05b80614a Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:12 +0800 Subject: [PATCH 46/47] vfio: Use g_autofree in all call site of vfio_get_region_info() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are some exceptions when pointer to vfio_region_info is reused. In that case, the pointed memory is freed manually. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/helpers.c | 7 ++----- hw/vfio/igd.c | 5 ++--- hw/vfio/pci.c | 13 +++---------- 3 files changed, 7 insertions(+), 18 deletions(-) diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c index 4b079dc383..27ea26aa48 100644 --- a/hw/vfio/helpers.c +++ b/hw/vfio/helpers.c @@ -343,7 +343,7 @@ static int vfio_setup_region_sparse_mmaps(VFIORegion *region, int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region, int index, const char *name) { - struct vfio_region_info *info; + g_autofree struct vfio_region_info *info = NULL; int ret; ret = vfio_get_region_info(vbasedev, index, &info); @@ -376,8 +376,6 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region, } } - g_free(info); - trace_vfio_region_setup(vbasedev->name, index, name, region->flags, region->fd_offset, region->size); return 0; @@ -594,14 +592,13 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type, bool vfio_has_region_cap(VFIODevice *vbasedev, int region, uint16_t cap_type) { - struct vfio_region_info *info = NULL; + g_autofree struct vfio_region_info *info = NULL; bool ret = false; if (!vfio_get_region_info(vbasedev, region, &info)) { if (vfio_get_region_info_cap(info, cap_type)) { ret = true; } - g_free(info); } return ret; diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index 402fc5ce1d..1e79202f2b 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -367,8 +367,8 @@ static const MemoryRegionOps vfio_igd_index_quirk = { void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) { - struct vfio_region_info *rom = NULL, *opregion = NULL, - *host = NULL, *lpc = NULL; + g_autofree struct vfio_region_info *rom = NULL; + struct vfio_region_info *opregion = NULL, *host = NULL, *lpc = NULL; VFIOQuirk *quirk; VFIOIGDQuirk *igd; PCIDevice *lpc_bridge; @@ -609,7 +609,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, ggms_mb + gms_mb); out: - g_free(rom); g_free(opregion); g_free(host); g_free(lpc); diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 35ad9b582f..74a79bdf61 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -879,7 +879,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev) static void vfio_pci_load_rom(VFIOPCIDevice *vdev) { - struct vfio_region_info *reg_info; + g_autofree struct vfio_region_info *reg_info = NULL; uint64_t size; off_t off = 0; ssize_t bytes; @@ -897,8 +897,6 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) vdev->rom_size = size = reg_info->size; vdev->rom_offset = reg_info->offset; - g_free(reg_info); - if (!vdev->rom_size) { vdev->rom_read_failed = true; error_report("vfio-pci: Cannot read device rom at " @@ -2668,7 +2666,7 @@ static VFIODeviceOps vfio_pci_ops = { bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) { VFIODevice *vbasedev = &vdev->vbasedev; - struct vfio_region_info *reg_info; + g_autofree struct vfio_region_info *reg_info = NULL; int ret; ret = vfio_get_region_info(vbasedev, VFIO_PCI_VGA_REGION_INDEX, ®_info); @@ -2685,7 +2683,6 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) error_setg(errp, "unexpected VGA info, flags 0x%lx, size 0x%lx", (unsigned long)reg_info->flags, (unsigned long)reg_info->size); - g_free(reg_info); return false; } @@ -2694,8 +2691,6 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) vdev->vga->fd_offset = reg_info->offset; vdev->vga->fd = vdev->vbasedev.fd; - g_free(reg_info); - vdev->vga->region[QEMU_PCI_VGA_MEM].offset = QEMU_PCI_VGA_MEM_BASE; vdev->vga->region[QEMU_PCI_VGA_MEM].nr = QEMU_PCI_VGA_MEM; QLIST_INIT(&vdev->vga->region[QEMU_PCI_VGA_MEM].quirks); @@ -2736,7 +2731,7 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) { VFIODevice *vbasedev = &vdev->vbasedev; - struct vfio_region_info *reg_info; + g_autofree struct vfio_region_info *reg_info = NULL; struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) }; int i, ret = -1; @@ -2790,8 +2785,6 @@ static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) } vdev->config_offset = reg_info->offset; - g_free(reg_info); - if (vdev->features & VFIO_FEATURE_ENABLE_VGA) { if (!vfio_populate_vga(vdev, errp)) { error_append_hint(errp, "device does not support " From b4e1670c494165d4186930d56f692857f4fec89b Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Wed, 22 May 2024 12:40:13 +0800 Subject: [PATCH 47/47] vfio/igd: Use g_autofree in vfio_probe_igd_bar4_quirk() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pointer opregion, host and lpc are allocated and freed in vfio_probe_igd_bar4_quirk(). Use g_autofree to automatically free them. Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Signed-off-by: Cédric Le Goater --- hw/vfio/igd.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index 1e79202f2b..d320d032a7 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -368,7 +368,9 @@ static const MemoryRegionOps vfio_igd_index_quirk = { void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) { g_autofree struct vfio_region_info *rom = NULL; - struct vfio_region_info *opregion = NULL, *host = NULL, *lpc = NULL; + g_autofree struct vfio_region_info *opregion = NULL; + g_autofree struct vfio_region_info *host = NULL; + g_autofree struct vfio_region_info *lpc = NULL; VFIOQuirk *quirk; VFIOIGDQuirk *igd; PCIDevice *lpc_bridge; @@ -426,7 +428,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) if ((ret || !rom->size) && !vdev->pdev.romfile) { error_report("IGD device %s has no ROM, legacy mode disabled", vdev->vbasedev.name); - goto out; + return; } /* @@ -437,7 +439,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) error_report("IGD device %s hotplugged, ROM disabled, " "legacy mode disabled", vdev->vbasedev.name); vdev->rom_read_failed = true; - goto out; + return; } /* @@ -450,7 +452,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) if (ret) { error_report("IGD device %s does not support OpRegion access," "legacy mode disabled", vdev->vbasedev.name); - goto out; + return; } ret = vfio_get_dev_region_info(&vdev->vbasedev, @@ -459,7 +461,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) if (ret) { error_report("IGD device %s does not support host bridge access," "legacy mode disabled", vdev->vbasedev.name); - goto out; + return; } ret = vfio_get_dev_region_info(&vdev->vbasedev, @@ -468,7 +470,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) if (ret) { error_report("IGD device %s does not support LPC bridge access," "legacy mode disabled", vdev->vbasedev.name); - goto out; + return; } gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4); @@ -482,7 +484,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); error_report("IGD device %s failed to enable VGA access, " "legacy mode disabled", vdev->vbasedev.name); - goto out; + return; } /* Create our LPC/ISA bridge */ @@ -490,7 +492,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) if (ret) { error_report("IGD device %s failed to create LPC bridge, " "legacy mode disabled", vdev->vbasedev.name); - goto out; + return; } /* Stuff some host values into the VM PCI host bridge */ @@ -498,14 +500,14 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) if (ret) { error_report("IGD device %s failed to modify host bridge, " "legacy mode disabled", vdev->vbasedev.name); - goto out; + return; } /* Setup OpRegion access */ if (!vfio_pci_igd_opregion_init(vdev, opregion, &err)) { error_append_hint(&err, "IGD legacy mode disabled\n"); error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); - goto out; + return; } /* Setup our quirk to munge GTT addresses to the VM allocated buffer */ @@ -607,9 +609,4 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) } trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, ggms_mb + gms_mb); - -out: - g_free(opregion); - g_free(host); - g_free(lpc); }