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

[Paolo Bonzini]

Il 17/04/2013 12:13, Orit Wasserman ha scritto:
> On 04/17/2013 07:20 AM, address@hidden wrote:
>> From: "Michael R. Hines" <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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?

I think it's for ram_addr_t.

Paolo

>>  
>>  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;
>>
>