[08/18] migration-local: introduce qemu_fopen_local()

Introduce read/write backend of QEMUFileLocal used by localhost
migration. The unix domain socket will be replaced by PIPE with
vmsplice mechanism.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 Makefile.objs     |    1 +
 migration-local.c |  211 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 212 insertions(+), 0 deletions(-)
 create mode 100644 migration-local.c

On 08/21/2013 03:18 AM, Lei Li wrote:
> Introduce read/write backend of QEMUFileLocal used by localhost
> migration. The unix domain socket will be replaced by PIPE with
> vmsplice mechanism.
>
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>   Makefile.objs     |    1 +
>   migration-local.c |  211 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 212 insertions(+), 0 deletions(-)
>   create mode 100644 migration-local.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index f46a4cd..30670cc 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -54,6 +54,7 @@ common-obj-y += migration.o migration-tcp.o
>   common-obj-$(CONFIG_RDMA) += migration-rdma.o
>   common-obj-y += qemu-char.o #aio.o
>   common-obj-y += block-migration.o
> +common-obj-y += migration-local.o
>   common-obj-y += page_cache.o xbzrle.o
>
>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
> diff --git a/migration-local.c b/migration-local.c
> new file mode 100644
> index 0000000..93190fd
> --- /dev/null
> +++ b/migration-local.c
> @@ -0,0 +1,211 @@
> +/*
> + * QEMU localhost migration
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later.
> + *
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "config-host.h"
> +#include "qemu-common.h"
> +#include "migration/migration.h"
> +#include "exec/cpu-common.h"
> +#include "config.h"
> +#include "exec/cpu-all.h"
> +#include "monitor/monitor.h"
> +#include "migration/qemu-file.h"
> +#include "qemu/iov.h"
> +#include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
> +#include "block/block.h"
> +#include "qemu/sockets.h"
> +#include "migration/block.h"
> +#include "qemu/thread.h"
> +#include "qmp-commands.h"
> +#include "trace.h"
> +#include "qemu/osdep.h"
> +
> +//#define DEBUG_MIGRATION_LOCAL
> +
> +#ifdef DEBUG_MIGRATION_LOCAL
> +#define DPRINTF(fmt, ...) \
> +    do { printf("migration-local: " fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +    do { } while (0)
> +#endif
> +
> +/*
> + * Interface for the local migration.
> + */
> +typedef struct QEMUFileLocal {
> +    QEMUFile *file;
> +    int fd;
> +    int state;
> +
> +    /*
> +     * This is the last block from where we have sent data
> +     * for local migration
> +     */
> +    RAMBlock *last_block_sent;
> +} QEMUFileLocal;
> +
> +
> +static int qemu_local_get_buffer(void *opaque, uint8_t *buf,
> +                                 int64_t pos, int size)
> +{
> +    QEMUFileLocal *s = opaque;
> +    ssize_t len;
> +
> +    for (;;) {
> +        len = qemu_recv(s->fd, buf, size, 0);
> +        if (len != -1) {
> +            break;
> +        }
> +        if (socket_error() == EAGAIN) {
> +            yield_until_fd_readable(s->fd);
> +        } else if (socket_error() != EINTR) {
> +            break;
> +        }
> +    }
> +
> +    if (len == -1) {
> +        len = -socket_error();
> +    }
> +    return len;
> +}
> +

This looks like a line-for-line copy of socket_get_buffer()......

Since you're just going to end up replacing this with vmsplice(),
could you just call socket_get_buffer() temporarily until
your next patch is ready?

> +static int qemu_local_get_fd(void *opaque)
> +{
> +    QEMUFileLocal *s = opaque;
> +
> +    return s->fd;
> +}
> +
> +static int qemu_local_close(void *opaque)
> +{
> +    QEMUFileLocal *s = opaque;
> +
> +    closesocket(s->fd);
> +    g_free(s);
> +
> +    return 0;
> +}
> +
> +static size_t qemu_local_put_buffer(void *opaque, struct iovec *iov,
> +                                    int iovcnt, int64_t pos)
> +{
> +    QEMUFileLocal *s = opaque;
> +    ssize_t len;
> +    ssize_t size = iov_size(iov, iovcnt);
> +
> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
> +    if (len < size) {
> +        len = -socket_error();
> +    }
> +
> +    return len;
> +}
> +
> +static size_t local_save_page(QEMUFile *f, RAMBlock *block,
> +                              ram_addr_t offset, int flags)
> +{
> +    MemoryRegion *mr = block->mr;
> +    uint8_t *p;
> +
> +    p = memory_region_get_ram_ptr(mr) + offset;
> +
> +    if (buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE)) {
> +        qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_COMPRESS);
> +        if (!flags) {
> +            qemu_put_byte(f, strlen(block->idstr));
> +            qemu_put_buffer(f, (uint8_t *)block->idstr,
> +                            strlen(block->idstr));
> +        }
> +        qemu_put_byte(f, *p);
> +        return 0;
> +    }
> +
> +    qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_PAGE);
> +    if (!flags) {
> +        qemu_put_byte(f, strlen(block->idstr));
> +        qemu_put_buffer(f, (uint8_t *)block->idstr,
> +                        strlen(block->idstr));
> +    }
> +    qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
> +
> +    return TARGET_PAGE_SIZE;
> +}
> +
> +static size_t qemu_local_ram_save(QEMUFile *f, void *opaque,
> +                                  ram_addr_t block_offset, ram_addr_t offset,
> +                                  size_t size, int *bytes_sent)
> +{
> +    QEMUFileLocal *s = opaque;
> +    uint64_t current_addr = block_offset + offset;
> +    RAMBlock *block = qemu_get_ram_block(current_addr);
> +    MemoryRegion *mr = block->mr;

RAMBlock structs are not visible outside of exec.c and arch_init.c,
how did you do this?
> +    void *ram;
> +    int ret;
> +    int cont;
> +
> +    ret = qemu_file_get_error(f);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    qemu_fflush(f);
> +
> +    cont = (block == s->last_block_sent) ? RAM_SAVE_FLAG_CONTINUE : 0;
> +
> +    ram = memory_region_get_ram_ptr(mr) + offset;
> +    s->last_block_sent = block;
> +
> +    *bytes_sent = local_save_page(f, block, offset, cont);
> +    if (!bytes_sent || *bytes_sent < 0) {
> +        return RAM_SAVE_CONTROL_DELAYED;
> +    }
> +
RAM_SAVE_CONTROL_DELAYED is only if you have *not* finished moving the 
bytes.

If you've finished moving the bytes, then you should return zero.

> +    /* DONTNEED the RAM page that has already been copied. */
> +    qemu_madvise(ram, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> +

This should be ram_handle_compressed().

> +    return 0;
> +}
> +
> +const QEMUFileOps local_read_ops = {
> +    .get_fd     = qemu_local_get_fd,
> +    .get_buffer = qemu_local_get_buffer,
> +    .close      = qemu_local_close,
> +};
> +
> +const QEMUFileOps local_write_ops = {
> +    .get_fd             = qemu_local_get_fd,
> +    .writev_buffer      = qemu_local_put_buffer,
> +    .close              = qemu_local_close,
> +    .save_page          = qemu_local_ram_save,
> +};
> +
> +static void *qemu_fopen_local(int fd, const char *mode)
> +{
> +    QEMUFileLocal *s;
> +
> +    if (qemu_file_mode_is_not_valid(mode)) {
> +        return NULL;
> +    }
> +
> +    s = g_malloc0(sizeof(QEMUFileLocal));
> +    s->fd = fd;
> +
> +    if (mode[0] == 'w') {
> +        qemu_set_block(s->fd);
> +        s->file = qemu_fopen_ops(s, &local_write_ops);
> +    } else {
> +        s->file = qemu_fopen_ops(s, &local_read_ops);
> +    }
> +
> +    return s->file;
> +}

On 08/23/2013 04:42 AM, Michael R. Hines wrote:
> On 08/21/2013 03:18 AM, Lei Li wrote:
>> Introduce read/write backend of QEMUFileLocal used by localhost
>> migration. The unix domain socket will be replaced by PIPE with
>> vmsplice mechanism.
>>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   Makefile.objs     |    1 +
>>   migration-local.c |  211 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 212 insertions(+), 0 deletions(-)
>>   create mode 100644 migration-local.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index f46a4cd..30670cc 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -54,6 +54,7 @@ common-obj-y += migration.o migration-tcp.o
>>   common-obj-$(CONFIG_RDMA) += migration-rdma.o
>>   common-obj-y += qemu-char.o #aio.o
>>   common-obj-y += block-migration.o
>> +common-obj-y += migration-local.o
>>   common-obj-y += page_cache.o xbzrle.o
>>
>>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o 
>> migration-fd.o
>> diff --git a/migration-local.c b/migration-local.c
>> new file mode 100644
>> index 0000000..93190fd
>> --- /dev/null
>> +++ b/migration-local.c
>> @@ -0,0 +1,211 @@
>> +/*
>> + * QEMU localhost migration
>> + *
>> + * Copyright IBM, Corp. 2013
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.
>> + *
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "config-host.h"
>> +#include "qemu-common.h"
>> +#include "migration/migration.h"
>> +#include "exec/cpu-common.h"
>> +#include "config.h"
>> +#include "exec/cpu-all.h"
>> +#include "monitor/monitor.h"
>> +#include "migration/qemu-file.h"
>> +#include "qemu/iov.h"
>> +#include "sysemu/arch_init.h"
>> +#include "sysemu/sysemu.h"
>> +#include "block/block.h"
>> +#include "qemu/sockets.h"
>> +#include "migration/block.h"
>> +#include "qemu/thread.h"
>> +#include "qmp-commands.h"
>> +#include "trace.h"
>> +#include "qemu/osdep.h"
>> +
>> +//#define DEBUG_MIGRATION_LOCAL
>> +
>> +#ifdef DEBUG_MIGRATION_LOCAL
>> +#define DPRINTF(fmt, ...) \
>> +    do { printf("migration-local: " fmt, ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) \
>> +    do { } while (0)
>> +#endif
>> +
>> +/*
>> + * Interface for the local migration.
>> + */
>> +typedef struct QEMUFileLocal {
>> +    QEMUFile *file;
>> +    int fd;
>> +    int state;
>> +
>> +    /*
>> +     * This is the last block from where we have sent data
>> +     * for local migration
>> +     */
>> +    RAMBlock *last_block_sent;
>> +} QEMUFileLocal;
>> +
>> +
>> +static int qemu_local_get_buffer(void *opaque, uint8_t *buf,
>> +                                 int64_t pos, int size)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +    ssize_t len;
>> +
>> +    for (;;) {
>> +        len = qemu_recv(s->fd, buf, size, 0);
>> +        if (len != -1) {
>> +            break;
>> +        }
>> +        if (socket_error() == EAGAIN) {
>> +            yield_until_fd_readable(s->fd);
>> +        } else if (socket_error() != EINTR) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (len == -1) {
>> +        len = -socket_error();
>> +    }
>> +    return len;
>> +}
>> +
>
> This looks like a line-for-line copy of socket_get_buffer()......
>
> Since you're just going to end up replacing this with vmsplice(),
> could you just call socket_get_buffer() temporarily until
> your next patch is ready?
>
>> +static int qemu_local_get_fd(void *opaque)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +
>> +    return s->fd;
>> +}
>> +
>> +static int qemu_local_close(void *opaque)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +
>> +    closesocket(s->fd);
>> +    g_free(s);
>> +
>> +    return 0;
>> +}
>> +
>> +static size_t qemu_local_put_buffer(void *opaque, struct iovec *iov,
>> +                                    int iovcnt, int64_t pos)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +    ssize_t len;
>> +    ssize_t size = iov_size(iov, iovcnt);
>> +
>> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
>> +    if (len < size) {
>> +        len = -socket_error();
>> +    }
>> +
>> +    return len;
>> +}
>> +
>> +static size_t local_save_page(QEMUFile *f, RAMBlock *block,
>> +                              ram_addr_t offset, int flags)
>> +{
>> +    MemoryRegion *mr = block->mr;
>> +    uint8_t *p;
>> +
>> +    p = memory_region_get_ram_ptr(mr) + offset;
>> +
>> +    if (buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE)) {
>> +        qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_COMPRESS);
>> +        if (!flags) {
>> +            qemu_put_byte(f, strlen(block->idstr));
>> +            qemu_put_buffer(f, (uint8_t *)block->idstr,
>> +                            strlen(block->idstr));
>> +        }
>> +        qemu_put_byte(f, *p);
>> +        return 0;
>> +    }
>> +
>> +    qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_PAGE);
>> +    if (!flags) {
>> +        qemu_put_byte(f, strlen(block->idstr));
>> +        qemu_put_buffer(f, (uint8_t *)block->idstr,
>> +                        strlen(block->idstr));
>> +    }
>> +    qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
>> +
>> +    return TARGET_PAGE_SIZE;
>> +}
>> +
>> +static size_t qemu_local_ram_save(QEMUFile *f, void *opaque,
>> +                                  ram_addr_t block_offset, 
>> ram_addr_t offset,
>> +                                  size_t size, int *bytes_sent)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +    uint64_t current_addr = block_offset + offset;
>> +    RAMBlock *block = qemu_get_ram_block(current_addr);
>> +    MemoryRegion *mr = block->mr;
>
> RAMBlock structs are not visible outside of exec.c and arch_init.c,
> how did you do this?

Hi Michael,

Good catch!
Actually this is the 'Known issue' that I listed in the cover letter, and I
planed to ask suggestions on this.

Currently the implementation of override of the RDMA hook like save_page for
localhost migration needs to have knowledge of MemoryRegion and RAMBlock, say,
when saving ram page, need to know which RAMBlock the given ram address save_page
hook passed is belong to, or which is the last ram block has been sent. But
seems that such structs can not be exported to the private code in migration-local.c.

My guess for now is that there might be two possible ways to handle this:

1) Choose another way around, like representation of a RAMBlock and MemoryRegion
    from localhost migration perspective. This might need more work to handle
    the descriptions.

2) It could be exported directly into private code like migration-local.c
    through some ways that I don't know yet. I remembered that the implementation
    of postcopy migration did this.

So I'd like to post and ask for your suggestions to make sure it's the right
and best way to handle.


>> +    void *ram;
>> +    int ret;
>> +    int cont;
>> +
>> +    ret = qemu_file_get_error(f);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    qemu_fflush(f);
>> +
>> +    cont = (block == s->last_block_sent) ? RAM_SAVE_FLAG_CONTINUE : 0;
>> +
>> +    ram = memory_region_get_ram_ptr(mr) + offset;
>> +    s->last_block_sent = block;
>> +
>> +    *bytes_sent = local_save_page(f, block, offset, cont);
>> +    if (!bytes_sent || *bytes_sent < 0) {
>> +        return RAM_SAVE_CONTROL_DELAYED;
>> +    }
>> +
> RAM_SAVE_CONTROL_DELAYED is only if you have *not* finished moving the 
> bytes.
>
> If you've finished moving the bytes, then you should return zero.

Acknowledge, thanks.

>
>> +    /* DONTNEED the RAM page that has already been copied. */
>> +    qemu_madvise(ram, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
>> +
>
> This should be ram_handle_compressed().
>
>> +    return 0;
>> +}
>> +
>> +const QEMUFileOps local_read_ops = {
>> +    .get_fd     = qemu_local_get_fd,
>> +    .get_buffer = qemu_local_get_buffer,
>> +    .close      = qemu_local_close,
>> +};
>> +
>> +const QEMUFileOps local_write_ops = {
>> +    .get_fd             = qemu_local_get_fd,
>> +    .writev_buffer      = qemu_local_put_buffer,
>> +    .close              = qemu_local_close,
>> +    .save_page          = qemu_local_ram_save,
>> +};
>> +
>> +static void *qemu_fopen_local(int fd, const char *mode)
>> +{
>> +    QEMUFileLocal *s;
>> +
>> +    if (qemu_file_mode_is_not_valid(mode)) {
>> +        return NULL;
>> +    }
>> +
>> +    s = g_malloc0(sizeof(QEMUFileLocal));
>> +    s->fd = fd;
>> +
>> +    if (mode[0] == 'w') {
>> +        qemu_set_block(s->fd);
>> +        s->file = qemu_fopen_ops(s, &local_write_ops);
>> +    } else {
>> +        s->file = qemu_fopen_ops(s, &local_read_ops);
>> +    }
>> +
>> +    return s->file;
>> +}
>
>

On 08/23/2013 03:44 PM, Lei Li wrote:
> On 08/23/2013 04:42 AM, Michael R. Hines wrote:
>> On 08/21/2013 03:18 AM, Lei Li wrote:
>>> Introduce read/write backend of QEMUFileLocal used by localhost
>>> migration. The unix domain socket will be replaced by PIPE with
>>> vmsplice mechanism.
>>>
>>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>>> ---
>>>   Makefile.objs     |    1 +
>>>   migration-local.c |  211 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 212 insertions(+), 0 deletions(-)
>>>   create mode 100644 migration-local.c
>>>
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index f46a4cd..30670cc 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -54,6 +54,7 @@ common-obj-y += migration.o migration-tcp.o
>>>   common-obj-$(CONFIG_RDMA) += migration-rdma.o
>>>   common-obj-y += qemu-char.o #aio.o
>>>   common-obj-y += block-migration.o
>>> +common-obj-y += migration-local.o
>>>   common-obj-y += page_cache.o xbzrle.o
>>>
>>>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o 
>>> migration-fd.o
>>> diff --git a/migration-local.c b/migration-local.c
>>> new file mode 100644
>>> index 0000000..93190fd
>>> --- /dev/null
>>> +++ b/migration-local.c
>>> @@ -0,0 +1,211 @@
>>> +/*
>>> + * QEMU localhost migration
>>> + *
>>> + * Copyright IBM, Corp. 2013
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> + * later.
>>> + *
>>> + * See the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +
>>> +#include "config-host.h"
>>> +#include "qemu-common.h"
>>> +#include "migration/migration.h"
>>> +#include "exec/cpu-common.h"
>>> +#include "config.h"
>>> +#include "exec/cpu-all.h"
>>> +#include "monitor/monitor.h"
>>> +#include "migration/qemu-file.h"
>>> +#include "qemu/iov.h"
>>> +#include "sysemu/arch_init.h"
>>> +#include "sysemu/sysemu.h"
>>> +#include "block/block.h"
>>> +#include "qemu/sockets.h"
>>> +#include "migration/block.h"
>>> +#include "qemu/thread.h"
>>> +#include "qmp-commands.h"
>>> +#include "trace.h"
>>> +#include "qemu/osdep.h"
>>> +
>>> +//#define DEBUG_MIGRATION_LOCAL
>>> +
>>> +#ifdef DEBUG_MIGRATION_LOCAL
>>> +#define DPRINTF(fmt, ...) \
>>> +    do { printf("migration-local: " fmt, ## __VA_ARGS__); } while (0)
>>> +#else
>>> +#define DPRINTF(fmt, ...) \
>>> +    do { } while (0)
>>> +#endif
>>> +
>>> +/*
>>> + * Interface for the local migration.
>>> + */
>>> +typedef struct QEMUFileLocal {
>>> +    QEMUFile *file;
>>> +    int fd;
>>> +    int state;
>>> +
>>> +    /*
>>> +     * This is the last block from where we have sent data
>>> +     * for local migration
>>> +     */
>>> +    RAMBlock *last_block_sent;
>>> +} QEMUFileLocal;
>>> +
>>> +
>>> +static int qemu_local_get_buffer(void *opaque, uint8_t *buf,
>>> +                                 int64_t pos, int size)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +    ssize_t len;
>>> +
>>> +    for (;;) {
>>> +        len = qemu_recv(s->fd, buf, size, 0);
>>> +        if (len != -1) {
>>> +            break;
>>> +        }
>>> +        if (socket_error() == EAGAIN) {
>>> +            yield_until_fd_readable(s->fd);
>>> +        } else if (socket_error() != EINTR) {
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    if (len == -1) {
>>> +        len = -socket_error();
>>> +    }
>>> +    return len;
>>> +}
>>> +
>>
[...]

>>
>>> +static int qemu_local_get_fd(void *opaque)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +
>>> +    return s->fd;
>>> +}
>>> +
>>> +static int qemu_local_close(void *opaque)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +
>>> +    closesocket(s->fd);
>>> +    g_free(s);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static size_t qemu_local_put_buffer(void *opaque, struct iovec *iov,
>>> +                                    int iovcnt, int64_t pos)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +    ssize_t len;
>>> +    ssize_t size = iov_size(iov, iovcnt);
>>> +
>>> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
>>> +    if (len < size) {
>>> +        len = -socket_error();
>>> +    }
>>> +
>>> +    return len;
>>> +}
>>> +
>>> +static size_t local_save_page(QEMUFile *f, RAMBlock *block,
>>> +                              ram_addr_t offset, int flags)
>>> +{
>>> +    MemoryRegion *mr = block->mr;
>>> +    uint8_t *p;
>>> +
>>> +    p = memory_region_get_ram_ptr(mr) + offset;
>>> +
>>> +    if (buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE)) {
>>> +        qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_COMPRESS);
>>> +        if (!flags) {
>>> +            qemu_put_byte(f, strlen(block->idstr));
>>> +            qemu_put_buffer(f, (uint8_t *)block->idstr,
>>> +                            strlen(block->idstr));
>>> +        }
>>> +        qemu_put_byte(f, *p);
>>> +        return 0;
>>> +    }
>>> +
>>> +    qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_PAGE);
>>> +    if (!flags) {
>>> +        qemu_put_byte(f, strlen(block->idstr));
>>> +        qemu_put_buffer(f, (uint8_t *)block->idstr,
>>> +                        strlen(block->idstr));
>>> +    }
>>> +    qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
>>> +
>>> +    return TARGET_PAGE_SIZE;
>>> +}
>>> +
>>> +static size_t qemu_local_ram_save(QEMUFile *f, void *opaque,
>>> +                                  ram_addr_t block_offset, 
>>> ram_addr_t offset,
>>> +                                  size_t size, int *bytes_sent)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +    uint64_t current_addr = block_offset + offset;
>>> +    RAMBlock *block = qemu_get_ram_block(current_addr);
>>> +    MemoryRegion *mr = block->mr;
>>
>> RAMBlock structs are not visible outside of exec.c and arch_init.c,
>> how did you do this?
>
> Hi Michael,
>
> Good catch!
> Actually this is the 'Known issue' that I listed in the cover letter, 
> and I
> planed to ask suggestions on this.
>
> Currently the implementation of override of the RDMA hook like 
> save_page for
> localhost migration needs to have knowledge of MemoryRegion and 
> RAMBlock, say,
> when saving ram page, need to know which RAMBlock the given ram 
> address save_page
> hook passed is belong to, or which is the last ram block has been 
> sent. But
> seems that such structs can not be exported to the private code in 
> migration-local.c.
>
> My guess for now is that there might be two possible ways to handle this:
>
> 1) Choose another way around, like representation of a RAMBlock and 
> MemoryRegion
>    from localhost migration perspective. This might need more work to 
> handle
>    the descriptions.
>
> 2) It could be exported directly into private code like migration-local.c
>    through some ways that I don't know yet. I remembered that the 
> implementation
>    of postcopy migration did this.
>
> So I'd like to post and ask for your suggestions to make sure it's the 
> right
> and best way to handle.
>
Hi Paolo,

Any suggestions on this?

Thanks

>
>

Il 28/08/2013 05:26, Lei Li ha scritto:
>>>
>>> RAMBlock structs are not visible outside of exec.c and arch_init.c,
>>> how did you do this?
>>
>> Hi Michael,
>>
>> Good catch!
>> Actually this is the 'Known issue' that I listed in the cover
>> letter, and I planed to ask suggestions on this.
>>
>> Currently the implementation of override of the RDMA hook like 
>> save_page for localhost migration needs to have knowledge of
>> MemoryRegion and RAMBlock, say, when saving ram page, need to know
>> which RAMBlock the given ram address save_page hook passed is
>> belong to, or which is the last ram block has been sent. But seems
>> that such structs can not be exported to the private code in 
>> migration-local.c.
>>
>> My guess for now is that there might be two possible ways to handle this:
>>
>> 1) Choose another way around, like representation of a RAMBlock and
>> MemoryRegion from localhost migration perspective. This might need
>> more work to handle the descriptions.
>>
>> 2) It could be exported directly into private code like migration-local.c
>> through some ways that I don't know yet. I remembered that the 
>> implementation of postcopy migration did this.
>>
>> So I'd like to post and ask for your suggestions to make sure it's the
>> right and best way to handle.
>
> Hi Paolo,
> 
> Any suggestions on this?

Unlike the RAM constants and migration states, I think exporting
MemoryRegion (not sure about RAMBlock) is fine.  In fact, replacing the
save_page hook's block_offset argument with a MemoryRegion would be a
nice cleanup.

Paolo

On 08/28/2013 02:37 PM, Paolo Bonzini wrote:
> Il 28/08/2013 05:26, Lei Li ha scritto:
>>>> RAMBlock structs are not visible outside of exec.c and arch_init.c,
>>>> how did you do this?
>>> Hi Michael,
>>>
>>> Good catch!
>>> Actually this is the 'Known issue' that I listed in the cover
>>> letter, and I planed to ask suggestions on this.
>>>
>>> Currently the implementation of override of the RDMA hook like
>>> save_page for localhost migration needs to have knowledge of
>>> MemoryRegion and RAMBlock, say, when saving ram page, need to know
>>> which RAMBlock the given ram address save_page hook passed is
>>> belong to, or which is the last ram block has been sent. But seems
>>> that such structs can not be exported to the private code in
>>> migration-local.c.
>>>
>>> My guess for now is that there might be two possible ways to handle this:
>>>
>>> 1) Choose another way around, like representation of a RAMBlock and
>>> MemoryRegion from localhost migration perspective. This might need
>>> more work to handle the descriptions.
>>>
>>> 2) It could be exported directly into private code like migration-local.c
>>> through some ways that I don't know yet. I remembered that the
>>> implementation of postcopy migration did this.
>>>
>>> So I'd like to post and ask for your suggestions to make sure it's the
>>> right and best way to handle.
>> Hi Paolo,
>>
>> Any suggestions on this?
> Unlike the RAM constants and migration states, I think exporting
> MemoryRegion (not sure about RAMBlock) is fine.  In fact, replacing the
> save_page hook's block_offset argument with a MemoryRegion would be a
> nice cleanup.

I'll see if could find way to export RAMBlock as well. If not, will represent
it like RDMA did.
Replacing the block_offset argument of save_page with MemoryRegion might be a
good idea, I will give it a try.

Thanks for your suggestions!

>
> Paolo
>

On 08/28/2013 02:37 AM, Paolo Bonzini wrote:
> Il 28/08/2013 05:26, Lei Li ha scritto:
>>>> RAMBlock structs are not visible outside of exec.c and arch_init.c,
>>>> how did you do this?
>>> Hi Michael,
>>>
>>> Good catch!
>>> Actually this is the 'Known issue' that I listed in the cover
>>> letter, and I planed to ask suggestions on this.
>>>
>>> Currently the implementation of override of the RDMA hook like
>>> save_page for localhost migration needs to have knowledge of
>>> MemoryRegion and RAMBlock, say, when saving ram page, need to know
>>> which RAMBlock the given ram address save_page hook passed is
>>> belong to, or which is the last ram block has been sent. But seems
>>> that such structs can not be exported to the private code in
>>> migration-local.c.
>>>
>>> My guess for now is that there might be two possible ways to handle this:
>>>
>>> 1) Choose another way around, like representation of a RAMBlock and
>>> MemoryRegion from localhost migration perspective. This might need
>>> more work to handle the descriptions.
>>>
>>> 2) It could be exported directly into private code like migration-local.c
>>> through some ways that I don't know yet. I remembered that the
>>> implementation of postcopy migration did this.
>>>
>>> So I'd like to post and ask for your suggestions to make sure it's the
>>> right and best way to handle.
>> Hi Paolo,
>>
>> Any suggestions on this?
> Unlike the RAM constants and migration states, I think exporting
> MemoryRegion (not sure about RAMBlock) is fine.  In fact, replacing the
> save_page hook's block_offset argument with a MemoryRegion would be a
> nice cleanup.
>
> Paolo
>
Sounds like a good plan to me.

Once the localhost patch is fully reviewed - I'll submit a similar patch 
to make use of the exported MemoryRegion.

- Michael