On Tue, Feb 27, 2024 at 07:03:27PM +0100, Cédric Le Goater wrote:
> The SaveVMHandlers structure is still in use for complex subsystems
> and devices. Document the handlers since we are going to modify a few
> later.
> 
> Signed-off-by: Cédric Le Goater <c...@redhat.com>

Reviewed-by: Peter Xu <pet...@redhat.com>

Still a few nitpick comments below.

> ---
>  include/migration/register.h | 257 +++++++++++++++++++++++++++++++----
>  1 file changed, 231 insertions(+), 26 deletions(-)
> 
> diff --git a/include/migration/register.h b/include/migration/register.h
> index 
> 2e6a7d766e62f64940086b7b511249c9ff21fa62..2cc71ec45f65bf2884c9e7a823d2968752f15c20
>  100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -16,30 +16,129 @@
>  
>  #include "hw/vmstate-if.h"
>  
> +/**
> + * struct SaveVMHandlers: handler structure to finely control
> + * migration of complex subsystems and devices, such as RAM, block and
> + * VFIO.
> + */
>  typedef struct SaveVMHandlers {
> -    /* This runs inside the BQL.  */
> +
> +    /* The following handlers runs inside the BQL. */
> +
> +    /**
> +     * @save_state
> +     *
> +     * Saves state section on the source using the latest state format
> +     * version.
> +     *
> +     * Legacy method. Should be deprecated when all users are ported
> +     * to VMState.

VMStateDescription?

> +     *
> +     * @f: QEMUFile where to send the data
> +     * @opaque: data pointer passed to register_savevm_live()
> +     */
>      void (*save_state)(QEMUFile *f, void *opaque);
>  
> -    /*
> -     * save_prepare is called early, even before migration starts, and can be
> -     * used to perform early checks.
> +    /**
> +     * @save_prepare
> +     *
> +     * Called early, even before migration starts, and can be used to
> +     * perform early checks.
> +     *
> +     * @opaque: data pointer passed to register_savevm_live()
> +     * @errp: pointer to Error*, to store an error if it happens.
> +     *
> +     * Returns zero to indicate success and negative for error
>       */
>      int (*save_prepare)(void *opaque, Error **errp);
> +
> +    /**
> +     * @save_setup
> +     *
> +     * Initializes the data structures on the source and transmits
> +     * first section containing information on the device
> +     *
> +     * @f: QEMUFile where to send the data
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      int (*save_setup)(QEMUFile *f, void *opaque);
> +
> +    /**
> +     * @save_cleanup
> +     *
> +     * Performs save related cleanup
> +     *
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      void (*save_cleanup)(void *opaque);
> +
> +    /**
> +     * @save_live_complete_postcopy
> +     *
> +     * Called at the end of postcopy for all postcopyiable devices.
> +     *
> +     * @f: QEMUFile where to send the data
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque);
> +
> +    /**
> +     * @save_live_complete_precopy
> +     *
> +     * Transmits the last section for the device containing any
> +     * remaining data.

This doesn't flush all remaining data when postcopy is enabled and
supported on the specific device.  Perhaps attach one more sentence to
describe that case?

  Transmits the last section for the device containing any remaining data
  at the end of a precopy phase.  When postcopy is enabled, devices that
  support postcopy will skip this step, where the final data will be
  flushed at the end of postcopy via @save_live_complete_postcopy instead.

> +     *
> +     * @f: QEMUFile where to send the data
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      int (*save_live_complete_precopy)(QEMUFile *f, void *opaque);
>  
>      /* This runs both outside and inside the BQL.  */
> +
> +    /**
> +     * @is_active
> +     *
> +     * Will skip a state section if not active
> +     *
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns true if state section is active else false
> +     */
>      bool (*is_active)(void *opaque);
> +
> +    /**
> +     * @has_postcopy
> +     *
> +     * checks if a device supports postcopy
> +     *
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns true for postcopy support else false
> +     */
>      bool (*has_postcopy)(void *opaque);
>  
> -    /* is_active_iterate
> -     * If it is not NULL then qemu_savevm_state_iterate will skip iteration 
> if
> -     * it returns false. For example, it is needed for only-postcopy-states,
> -     * which needs to be handled by qemu_savevm_state_setup and
> -     * qemu_savevm_state_pending, but do not need iterations until not in
> -     * postcopy stage.
> +    /**
> +     * @is_active_iterate
> +     *
> +     * As #SaveVMHandlers.is_active(), will skip an inactive state
> +     * section in qemu_savevm_state_iterate.
> +     *
> +     * For example, it is needed for only-postcopy-states, which needs
> +     * to be handled by qemu_savevm_state_setup() and
> +     * qemu_savevm_state_pending(), but do not need iterations until
> +     * not in postcopy stage.
> +     *
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns true if state section is active else false
>       */
>      bool (*is_active_iterate)(void *opaque);
>  
> @@ -48,44 +147,150 @@ typedef struct SaveVMHandlers {
>       * use data that is local to the migration thread or protected
>       * by other locks.
>       */
> +
> +    /**
> +     * @save_live_iterate
> +     *
> +     * Should send a chunk of data until the point that stream
> +     * bandwidth limits tell it to stop. Each call generates one
> +     * section.
> +     *
> +     * @f: QEMUFile where to send the data
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      int (*save_live_iterate)(QEMUFile *f, void *opaque);
>  
>      /* This runs outside the BQL!  */
> -    /* Note for save_live_pending:
> -     * must_precopy:
> -     * - must be migrated in precopy or in stopped state
> -     * - i.e. must be migrated before target start
> -     *
> -     * can_postcopy:
> -     * - can migrate in postcopy or in stopped state
> -     * - i.e. can migrate after target start
> -     * - some can also be migrated during precopy (RAM)
> -     * - some must be migrated after source stops (block-dirty-bitmap)
> -     *
> -     * Sum of can_postcopy and must_postcopy is the whole amount of
> +
> +    /**
> +     * @state_pending_estimate
> +     *
> +     * This estimates the remaining data to transfer
> +     *
> +     * Sum of @can_postcopy and @must_postcopy is the whole amount of
>       * pending data.
> +     *
> +     * @opaque: data pointer passed to register_savevm_live()
> +     * @must_precopy: amount of data that must be migrated in precopy
> +     *                or in stopped state, i.e. that must be migrated
> +     *                before target start.
> +     * @can_postcopy: amount of data that can be migrated in postcopy
> +     *                or in stopped state, i.e. after target start.
> +     *                Some can also be migrated during precopy (RAM).
> +     *                Some must be migrated after source stops
> +     *                (block-dirty-bitmap)
>       */
> -    /* This estimates the remaining data to transfer */
>      void (*state_pending_estimate)(void *opaque, uint64_t *must_precopy,
>                                     uint64_t *can_postcopy);
> -    /* This calculate the exact remaining data to transfer */
> +
> +    /**
> +     * @state_pending_exact
> +     *
> +     * This calculate the exact remaining data to transfer
> +     *
> +     * Sum of @can_postcopy and @must_postcopy is the whole amount of
> +     * pending data.
> +     *
> +     * @opaque: data pointer passed to register_savevm_live()
> +     * @must_precopy: amount of data that must be migrated in precopy
> +     *                or in stopped state, i.e. that must be migrated
> +     *                before target start.
> +     * @can_postcopy: amount of data that can be migrated in postcopy
> +     *                or in stopped state, i.e. after target start.
> +     *                Some can also be migrated during precopy (RAM).
> +     *                Some must be migrated after source stops
> +     *                (block-dirty-bitmap)
> +     */
>      void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
>                                  uint64_t *can_postcopy);
> +
> +    /**
> +     * @load_state
> +     *
> +     * Load sections generated by any of the save functions that
> +     * generate sections.
> +     *
> +     * Legacy method. Should be deprecated when all users are ported
> +     * to VMState.

VMStateDescription.

> +     *
> +     * @f: QEMUFile where to receive the data
> +     * @opaque: data pointer passed to register_savevm_live()
> +     * @version_id: the maximum version_id supported
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      int (*load_state)(QEMUFile *f, void *opaque, int version_id);
> +
> +    /**
> +     * @load_setup
> +     *
> +     * Initializes the data structures on the destination.
> +     *
> +     * @f: QEMUFile where to receive the data
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      int (*load_setup)(QEMUFile *f, void *opaque);
> +
> +    /**
> +     * @load_cleanup
> +     *
> +     * Performs cleanup of load data structures
> +     *
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      int (*load_cleanup)(void *opaque);
> -    /* Called when postcopy migration wants to resume from failure */
> +
> +    /**
> +     * @resume_prepare
> +     *
> +     * Called when postcopy migration wants to resume from failure
> +     *
> +     * @s: Current migration state
> +     * @opaque: data pointer passed to register_savevm_live()
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      int (*resume_prepare)(MigrationState *s, void *opaque);
> -    /* Checks if switchover ack should be used. Called only in dest */
> +
> +    /**
> +     * @switchover_ack_needed
> +     *
> +     * Checks if switchover ack should be used. Called only on
> +     * destination.
> +     *
> +     * @opaque: data pointer passed to register_savevm_live()
> +     */
>      bool (*switchover_ack_needed)(void *opaque);
>  } SaveVMHandlers;
>  
> +/**
> + * register_savevm_live: Register a set of custom migration handlers
> + *
> + * @idstr: state section identifier
> + * @instance_id: instance id
> + * @version_id: version id supported
> + * @ops: SaveVMHandlers structurex
> + * @opaque: data pointer passed SaveVMHandlers handlers
> + */
>  int register_savevm_live(const char *idstr,
>                           uint32_t instance_id,
>                           int version_id,
>                           const SaveVMHandlers *ops,
>                           void *opaque);
>  
> +/**
> + * unregister_savevm: Unregister custom migration handlers
> + *
> + * @obj: object associated with state section
> + * @idstr:  state section identifier
> + * @opaque: data pointer passed to register_savevm_live()
> + */
>  void unregister_savevm(VMStateIf *obj, const char *idstr, void *opaque);
>  
>  #endif
> -- 
> 2.43.2
> 

-- 
Peter Xu


Reply via email to