Re: [PATCH v2 03/21] migration: Add documentation for SaveVMHandlers
On 3/4/24 10:05, Avihai Horon wrote:
Hi Cedric,
A few nits below.
Just in time for v3 I was about to send ! I will include these
suggestions.
Thanks,
C.
On 27/02/2024 20:03, Cédric Le Goater wrote:
External email: Use caution opening links or attachments
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
---
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. */
s/runs/run
+
+ /**
+ * @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.
+ *
+ * @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
Use save_setup phrasing?
Uninitializes the data structures on the source.
+ *
+ * @opaque: data pointer passed to register_savevm_live()
+ *
+ * Returns zero to indicate success and negative for error
This can be dropped as it's a void function.
+ */
void (*save_cleanup)(void *opaque);
+
+ /**
+ * @save_live_complete_postcopy
+ *
+ * Called at the end of postcopy for all postcopyiable devices.
s/postcopyiable/postcopyable>
+ *
+ * @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.
+ *
+ * @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
s/checks/Checks
+ *
+ * @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
Re: [PATCH v2 03/21] migration: Add documentation for SaveVMHandlers
Hi Cedric,
A few nits below.
On 27/02/2024 20:03, Cédric Le Goater wrote:
External email: Use caution opening links or attachments
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
---
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. */
s/runs/run
+
+/**
+ * @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.
+ *
+ * @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
Use save_setup phrasing?
Uninitializes the data structures on the source.
+ *
+ * @opaque: data pointer passed to register_savevm_live()
+ *
+ * Returns zero to indicate success and negative for error
This can be dropped as it's a void function.
+ */
void (*save_cleanup)(void *opaque);
+
+/**
+ * @save_live_complete_postcopy
+ *
+ * Called at the end of postcopy for all postcopyiable devices.
s/postcopyiable/postcopyable
+ *
+ * @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.
+ *
+ * @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
s/checks/Checks
+ *
+ * @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.
+ *
+
Re: [PATCH v2 03/21] migration: Add documentation for SaveVMHandlers
On 2/29/24 05:10, Peter Xu wrote: 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 Reviewed-by: Peter Xu Still a few nitpick comments below. I have applied your suggestions for the next spin. Thanks, C.
Re: [PATCH v2 03/21] migration: Add documentation for SaveVMHandlers
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
Reviewed-by: Peter Xu
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
[PATCH v2 03/21] migration: Add documentation for SaveVMHandlers
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
---
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.
+ *
+ * @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.
+ *
+ * @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.
*/
