Re: [Qemu-devel] [PULL v3 2/7] rdma: new QEMUFileOps hooks

[Orit Wasserman <owasserm@redhat.com>]

On 04/17/2013 07:20 AM, mrhines@linux.vnet.ibm.com wrote:
> From: "Michael R. Hines" <mrhines@us.ibm.com>
> 
> These are the prototypes and implementation of new hooks that
> RDMA takes advantage of to perform dynamic page registration.
> 
> An optional hook is also introduced for a custom function
> to be able to override the default save_page function.
> 
> Also included are the prototypes and accessor methods used by
> arch_init.c which invoke funtions inside savevm.c to call out
> to the hooks that may or may not have been overridden
> inside of QEMUFileOps.
> 
> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
> ---
>  include/migration/migration.h |   19 ++++++++++
>  include/migration/qemu-file.h |   31 ++++++++++++++++
>  savevm.c                      |   79 ++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 121 insertions(+), 8 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index e2acec6..8e02391 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -21,6 +21,7 @@
>  #include "qapi/error.h"
>  #include "migration/vmstate.h"
>  #include "qapi-types.h"
> +#include "exec/cpu-common.h"

Is this include needed here?

>  
>  struct MigrationParams {
>      bool blk;
> @@ -127,4 +128,22 @@ int migrate_use_xbzrle(void);
>  int64_t migrate_xbzrle_cache_size(void);
>  
>  int64_t xbzrle_cache_resize(int64_t new_size);
> +
> +void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
> +
> +bool migrate_chunk_register_destination(void);
> +void ram_control_before_iterate(QEMUFile *f, uint64_t flags);
> +void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
> +void ram_control_load_hook(QEMUFile *f, uint64_t flags);
> +
> +/* Whenever this is found in the data stream, the flags
> + * will be passed to ram_control_load_hook in the incoming-migration
> + * side. This lets before_ram_iterate/after_ram_iterate add
> + * transport-specific sections to the RAM migration data.
> + */
> +#define RAM_SAVE_FLAG_HOOK     0x80
> +
> +size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> +                             ram_addr_t offset, size_t size);
> +
>  #endif
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 7519464..5166a42 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -23,6 +23,7 @@
>   */
>  #ifndef QEMU_FILE_H
>  #define QEMU_FILE_H 1
> +#include "exec/cpu-common.h"
>  
>  /* This function writes a chunk of data to a file at the given position.
>   * The pos argument can be ignored if the file is only being used for
> @@ -57,12 +58,39 @@ typedef int (QEMUFileGetFD)(void *opaque);
>  typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
>                                             int iovcnt, int64_t pos);
>  
> +/*
> + * This function provides hooks around different
> + * stages of RAM migration.
> + */
> +typedef int (QEMURamHookFunc)(QEMUFile *f, void *opaque, uint64_t flags);
> +
> +/*
> + * Constants used by ram_control_* hooks
> + */
> +#define RAM_CONTROL_SETUP    0
> +#define RAM_CONTROL_ROUND    1
> +#define RAM_CONTROL_HOOK     2
> +#define RAM_CONTROL_FINISH   3
> +
> +/*
> + * This function allows override of where the RAM page
> + * is saved (such as RDMA, for example.)
> + */
> +typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
> +                               ram_addr_t block_offset,
> +                               ram_addr_t offset,
> +                               size_t size);
> +
>  typedef struct QEMUFileOps {
>      QEMUFilePutBufferFunc *put_buffer;
>      QEMUFileGetBufferFunc *get_buffer;
>      QEMUFileCloseFunc *close;
>      QEMUFileGetFD *get_fd;
>      QEMUFileWritevBufferFunc *writev_buffer;
> +    QEMURamHookFunc *before_ram_iterate;
> +    QEMURamHookFunc *after_ram_iterate;
> +    QEMURamHookFunc *hook_ram_load;
> +    QEMURamSaveFunc *save_page;
>  } QEMUFileOps;
>  
>  QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
> @@ -81,6 +109,8 @@ void qemu_put_byte(QEMUFile *f, int v);
>   */
>  void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size);
>  
> +bool qemu_file_mode_is_not_valid(const char *mode);
> +

This should be in a separate patch as it not related to the new hooks 
you can add a new patch for adding qemu_file_mode_is_invalid func

>  static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
>  {
>      qemu_put_byte(f, (int)v);
> @@ -110,6 +140,7 @@ void qemu_file_reset_rate_limit(QEMUFile *f);
>  void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
>  int64_t qemu_file_get_rate_limit(QEMUFile *f);
>  int qemu_file_get_error(QEMUFile *f);
> +void qemu_fflush(QEMUFile *f);

This also should not be in this patch but in a separate patch
 (probably didn't read the rest of the patches yet)

Orit

>  
>  static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
>  {
> diff --git a/savevm.c b/savevm.c
> index 53515cb..cdb1690 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -409,14 +409,23 @@ static const QEMUFileOps socket_write_ops = {
>      .close =      socket_close
>  };
>  
> -QEMUFile *qemu_fopen_socket(int fd, const char *mode)
> +bool qemu_file_mode_is_not_valid(const char *mode)
>  {
> -    QEMUFileSocket *s = g_malloc0(sizeof(QEMUFileSocket));
> -
>      if (mode == NULL ||
>          (mode[0] != 'r' && mode[0] != 'w') ||
>          mode[1] != 'b' || mode[2] != 0) {
>          fprintf(stderr, "qemu_fopen: Argument validity check failed\n");
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +QEMUFile *qemu_fopen_socket(int fd, const char *mode)
> +{
> +    QEMUFileSocket *s = g_malloc0(sizeof(QEMUFileSocket));
> +
> +    if (qemu_file_mode_is_not_valid(mode)) {
>          return NULL;
>      }
>  
> @@ -434,10 +443,7 @@ QEMUFile *qemu_fopen(const char *filename, const char *mode)
>  {
>      QEMUFileStdio *s;
>  
> -    if (mode == NULL ||
> -	(mode[0] != 'r' && mode[0] != 'w') ||
> -	mode[1] != 'b' || mode[2] != 0) {
> -        fprintf(stderr, "qemu_fopen: Argument validity check failed\n");
> +    if (qemu_file_mode_is_not_valid(mode)) {
>          return NULL;
>      }
>  
> @@ -542,7 +548,7 @@ static inline bool qemu_file_is_writable(QEMUFile *f)
>   * If there is writev_buffer QEMUFileOps it uses it otherwise uses
>   * put_buffer ops.
>   */
> -static void qemu_fflush(QEMUFile *f)
> +void qemu_fflush(QEMUFile *f)
>  {
>      ssize_t ret = 0;
>  
> @@ -569,6 +575,63 @@ static void qemu_fflush(QEMUFile *f)
>      }
>  }
>  
> +void ram_control_before_iterate(QEMUFile *f, uint64_t flags)
> +{
> +    int ret = 0;
> +
> +    if (f->ops->before_ram_iterate) {
> +        ret = f->ops->before_ram_iterate(f, f->opaque, flags);
> +        if (ret < 0) {
> +            qemu_file_set_error(f, ret);
> +        }
> +    }
> +}
> +
> +void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
> +{
> +    int ret = 0;
> +
> +    if (f->ops->after_ram_iterate) {
> +        ret = f->ops->after_ram_iterate(f, f->opaque, flags);
> +        if (ret < 0) {
> +            qemu_file_set_error(f, ret);
> +        }
> +    }
> +}
> +
> +void ram_control_load_hook(QEMUFile *f, uint64_t flags)
> +{
> +    int ret = 0;
> +
> +    if (f->ops->hook_ram_load) {
> +        ret = f->ops->hook_ram_load(f, f->opaque, flags);
> +        if (ret < 0) {
> +            qemu_file_set_error(f, ret);
> +        }
> +    } else {
> +        qemu_file_set_error(f, ret);
> +    }
> +}
> +
> +size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> +                             ram_addr_t offset, size_t size)
> +{
> +    if (f->ops->save_page) {
> +        int64_t bytes;
> +        bytes = f->ops->save_page(f, f->opaque, block_offset, offset, size);
> +
> +        if (bytes >= 0) {
> +            f->pos += bytes;
> +        } else {
> +            qemu_file_set_error(f, bytes);
> +        }
> +
> +        return bytes;
> +    }
> +
> +    return -1;
> +}
> +
>  static void qemu_fill_buffer(QEMUFile *f)
>  {
>      int len;
>