On Tue, 18 Mar 2025 at 15:29, Cédric Le Goater <c...@redhat.com> wrote:
> This routine is related to VFIO migration. It belongs to "migration.c".
> While at it, rename it to better reflect the namespace it belongs to.
>
> Signed-off-by: Cédric Le Goater <c...@redhat.com>
> ---
> hw/vfio/migration.h | 1 +
> hw/vfio/dirty-tracking.c | 19 +++++--------------
> hw/vfio/migration.c | 7 +++++++
> 3 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/hw/vfio/migration.h b/hw/vfio/migration.h
> index
> 7ad2141d06a7c97f034db908f9ce19fd06f415b9..9b57d7dc1a6c6143c19e1ee85807d036b1363624
> 100644
> --- a/hw/vfio/migration.h
> +++ b/hw/vfio/migration.h
> @@ -68,5 +68,6 @@ int vfio_migration_set_state(VFIODevice *vbasedev,
> enum vfio_device_mig_state recover_state,
> Error **errp);
> #endif
> +void vfio_migration_set_error(int ret);
>
> #endif /* HW_VFIO_MIGRATION_H */
> diff --git a/hw/vfio/dirty-tracking.c b/hw/vfio/dirty-tracking.c
> index
> 441f9d9a08c06a88dda44ef143dcee5f0a89a900..447e09ed84993e3fbe1ed9b27a8269a9f0f46339
> 100644
> --- a/hw/vfio/dirty-tracking.c
> +++ b/hw/vfio/dirty-tracking.c
> @@ -35,8 +35,6 @@
> #include "system/runstate.h"
> #include "trace.h"
> #include "qapi/error.h"
> -#include "migration/misc.h"
> -#include "migration/qemu-file.h"
> #include "system/tcg.h"
> #include "system/tpm.h"
> #include "migration.h"
> @@ -47,13 +45,6 @@
> * Device state interfaces
> */
>
> -static void vfio_set_migration_error(int ret)
> -{
> - if (migration_is_running()) {
> - migration_file_set_error(ret, NULL);
> - }
> -}
> -
> static bool vfio_devices_all_device_dirty_tracking_started(
> const VFIOContainerBase *bcontainer)
> {
> @@ -175,7 +166,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
> IOMMUTLBEntry *iotlb)
> if (iotlb->target_as != &address_space_memory) {
> error_report("Wrong target AS \"%s\", only system memory is allowed",
> iotlb->target_as->name ? iotlb->target_as->name :
> "none");
> - vfio_set_migration_error(-EINVAL);
> + vfio_migration_set_error(-EINVAL);
> return;
> }
>
> @@ -212,7 +203,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
> IOMMUTLBEntry *iotlb)
> "0x%"HWADDR_PRIx") = %d (%s)",
> bcontainer, iova,
> iotlb->addr_mask + 1, ret, strerror(-ret));
> - vfio_set_migration_error(ret);
> + vfio_migration_set_error(ret);
> }
> }
> out:
> @@ -995,7 +986,7 @@ static void vfio_listener_log_global_stop(MemoryListener
> *listener)
> error_prepend(&local_err,
> "vfio: Could not stop dirty page tracking - ");
> error_report_err(local_err);
> - vfio_set_migration_error(ret);
> + vfio_migration_set_error(ret);
> }
> }
>
> @@ -1137,7 +1128,7 @@ out_unlock:
>
> out:
> if (ret) {
> - vfio_set_migration_error(ret);
> + vfio_migration_set_error(ret);
> }
> }
>
> @@ -1271,7 +1262,7 @@ static void vfio_listener_log_sync(MemoryListener
> *listener,
> ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
> if (ret) {
> error_report_err(local_err);
> - vfio_set_migration_error(ret);
> + vfio_migration_set_error(ret);
> }
> }
> }
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index
> 46c4cfecce25ba1146a1d8f2de0d7c51425afe8e..6fd825e435bde96d1008ec03dfaba25db3b616fc
> 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -1239,3 +1239,10 @@ bool vfio_device_state_is_precopy(VFIODevice *vbasedev)
> return migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ||
> migration->device_state == VFIO_DEVICE_STATE_PRE_COPY_P2P;
> }
> +
> +void vfio_migration_set_error(int ret)
> +{
> + if (migration_is_running()) {
> + migration_file_set_error(ret, NULL);
> + }
> +}
> --
* The change looks okay. But with the 'Error *err = NULL' parameter,
the error (ret) is also not passed on. Could we call
migration_file_set_error(ret, errp), instead of defining
'vfio_migration_set_error'? Because currently it accepts only one
'ret' parameter, later adding *errp to it would entail changing all
call sites.
Thank you.
---
- Prasad
