Re: [Qemu-devel] [PATCH v4 40/47] Postcopy: Use helpers to map pages during migration

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8060 bytes --]

On Fri, Oct 03, 2014 at 06:47:46PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> In postcopy, the destination guest is running at the same time
> as it's receiving pages; as we receive new pages we must put
> them into the guests address space atomically to avoid a running
> CPU accessing a partially written page.
> 
> Use the helpers in postcopy-ram.c to map these pages.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  arch_init.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 87 insertions(+), 9 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 2f4345a..0ba627b 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1458,9 +1458,20 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
>      return 0;
>  }
>  
> +/*
> + * Read a RAMBlock ID from the stream f, find the host address of the
> + * start of that block and add on 'offset'
> + *
> + * f: Stream to read from
> + * mis: MigrationIncomingState
> + * offset: Offset within the block
> + * flags: Page flags (mostly to see if it's a continuation of previous block)
> + * rb: Pointer to RAMBlock* that gets filled in with the RB we find
> + */
>  static inline void *host_from_stream_offset(QEMUFile *f,
> +                                            MigrationIncomingState *mis,
>                                              ram_addr_t offset,
> -                                            int flags)
> +                                            int flags, RAMBlock **rb)
>  {
>      static RAMBlock *block = NULL;
>      char id[256];
> @@ -1471,8 +1482,11 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>              error_report("Ack, bad migration stream!");
>              return NULL;
>          }
> +        if (rb) {
> +            *rb = block;
> +        }
>  
> -        return memory_region_get_ram_ptr(block->mr) + offset;
> +        goto gotit;

This is an ugly use of goto - it looks kind of like the exception
handling goto idiom, but it's not.  I think it would be nicer to make
the code fragment after gotit into a helper function.

>      }
>  
>      len = qemu_get_byte(f);
> @@ -1480,12 +1494,22 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>      id[len] = 0;
>  
>      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> -        if (!strncmp(id, block->idstr, sizeof(id)))
> -            return memory_region_get_ram_ptr(block->mr) + offset;
> +        if (!strncmp(id, block->idstr, sizeof(id))) {
> +            if (rb) {
> +                *rb = block;
> +            }
> +            goto gotit;
> +        }
>      }
>  
>      error_report("Can't find block %s!", id);
>      return NULL;
> +
> +gotit:
> +    postcopy_hook_early_receive(mis,
> +        (offset + (*rb)->offset) >> TARGET_PAGE_BITS);
> +    return memory_region_get_ram_ptr(block->mr) + offset;
> +
>  }
>  
>  /*
> @@ -1515,6 +1539,13 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>      ram_addr_t addr;
>      int flags, ret = 0;
>      static uint64_t seq_iter;
> +    /*
> +     * System is running in postcopy mode, page inserts to host memory must be
> +     * atomic
> +     */
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    bool postcopy_running = mis->postcopy_ram_state >=
> +                            POSTCOPY_RAM_INCOMING_LISTENING;
>  
>      seq_iter++;
>  
> @@ -1523,6 +1554,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>      }
>  
>      while (!ret) {
> +        RAMBlock *rb = 0; /* =0 needed to silence compiler */
>          addr = qemu_get_be64(f);
>  
>          flags = addr & ~TARGET_PAGE_MASK;
> @@ -1570,7 +1602,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>              void *host;
>              uint8_t ch;
>  
> -            host = host_from_stream_offset(f, addr, flags);
> +            host = host_from_stream_offset(f, mis, addr, flags, &rb);
>              if (!host) {
>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>                  ret = -EINVAL;
> @@ -1578,20 +1610,66 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>              }
>  
>              ch = qemu_get_byte(f);
> -            ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
> +            if (!postcopy_running) {
> +                ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
> +            } else {
> +                if (!ch) {
> +                    ret = postcopy_place_zero_page(mis, host,
> +                              (addr + rb->offset) >> TARGET_PAGE_BITS);
> +                } else {
> +                    void *tmp;
> +                    tmp = postcopy_get_tmp_page(mis, (addr + rb->offset) >>
> +                                                      TARGET_PAGE_BITS);
> +
> +                    if (!tmp) {
> +                        return -ENOMEM;
> +                    }
> +                    memset(tmp, ch, TARGET_PAGE_SIZE);
> +                    ret = postcopy_place_page(mis, host, tmp,
> +                              (addr + rb->offset) >> TARGET_PAGE_BITS);
> +                }
> +                if (ret) {
> +                    error_report("ram_load: Failure in postcopy compress @"
> +                                 "%zx/%p;%s+%zx",
> +                                 addr, host, rb->idstr, rb->offset);
> +                    return ret;
> +                }
> +            }

Might be nicer to fold this logic into ram_handle_compressed(), since
there's no obvious reason it should not be used for the postcopy path.

>          } else if (flags & RAM_SAVE_FLAG_PAGE) {
>              void *host;
>  
> -            host = host_from_stream_offset(f, addr, flags);
> +            host = host_from_stream_offset(f, mis, addr, flags, &rb);
>              if (!host) {
>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>                  ret = -EINVAL;
>                  break;
>              }
>  
> -            qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> +            if (!postcopy_running) {
> +                qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> +            } else {
> +                void *tmp = postcopy_get_tmp_page(mis, (addr + rb->offset) >>
> +                                                        TARGET_PAGE_BITS);
> +
> +                if (!tmp) {
> +                    return -ENOMEM;
> +                }
> +                qemu_get_buffer(f, tmp, TARGET_PAGE_SIZE);
> +                ret = postcopy_place_page(mis, host, tmp,
> +                          (addr + rb->offset) >> TARGET_PAGE_BITS);
> +                if (ret) {
> +                    error_report("ram_load: Failure in postcopy simple"
> +                                 "@%zx/%p;%s+%zx",
> +                                 addr, host, rb->idstr, rb->offset);
> +                    return ret;
> +                }
> +            }
>          } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
> -            void *host = host_from_stream_offset(f, addr, flags);
> +            if (postcopy_running) {
> +                error_report("XBZRLE RAM block in postcopy mode @%zx\n", addr);
> +                return -EINVAL;
> +            }

Hrm, there doesn't seem like an inherent reason XBZRLE shouldn't be
possible in postcopy.  Obviously a temporary buffer would be
necessary.

> +            void *host = host_from_stream_offset(f, mis, addr, flags, &rb);
>              if (!host) {
>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>                  ret = -EINVAL;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]