Re: [RFC PATCH v3 09/30] migration/ram: Add incoming 'fixed-ram' migration

[Peter Xu <peterx@redhat.com>]

On Mon, Nov 27, 2023 at 05:25:51PM -0300, Fabiano Rosas wrote:
> Add the necessary code to parse the format changes for the 'fixed-ram'
> capability.
> 
> One of the more notable changes in behavior is that in the 'fixed-ram'
> case ram pages are restored in one go rather than constantly looping
> through the migration stream.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> - added sanity check for pages_offset alignment
> - s/parsing/reading
> - used Error
> - fixed buffer size computation, now allowing an arbitrary limit
> - fixed dereference of pointer to packed struct member in endianness
>   conversion
> ---
>  migration/ram.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 4a0ab8105f..08604222f2 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -106,6 +106,12 @@
>   */
>  #define FIXED_RAM_FILE_OFFSET_ALIGNMENT 0x100000
>  
> +/*
> + * When doing fixed-ram migration, this is the amount we read from the
> + * pages region in the migration file at a time.
> + */
> +#define FIXED_RAM_LOAD_BUF_SIZE 0x100000
> +
>  XBZRLECacheStats xbzrle_counters;
>  
>  /* used by the search for pages to send */
> @@ -2996,6 +3002,35 @@ static void fixed_ram_insert_header(QEMUFile *file, RAMBlock *block)
>      qemu_put_buffer(file, (uint8_t *) header, header_size);
>  }
>  
> +static bool fixed_ram_read_header(QEMUFile *file, FixedRamHeader *header,
> +                                  Error **errp)
> +{
> +    size_t ret, header_size = sizeof(FixedRamHeader);
> +
> +    ret = qemu_get_buffer(file, (uint8_t *)header, header_size);
> +    if (ret != header_size) {
> +        error_setg(errp, "Could not read whole fixed-ram migration header "
> +                   "(expected %zd, got %zd bytes)", header_size, ret);
> +        return false;
> +    }
> +
> +    /* migration stream is big-endian */
> +    header->version = be32_to_cpu(header->version);
> +
> +    if (header->version > FIXED_RAM_HDR_VERSION) {
> +        error_setg(errp, "Migration fixed-ram capability version mismatch "
> +                   "(expected %d, got %d)", FIXED_RAM_HDR_VERSION,
> +                   header->version);
> +        return false;
> +    }
> +
> +    header->page_size = be64_to_cpu(header->page_size);
> +    header->bitmap_offset = be64_to_cpu(header->bitmap_offset);
> +    header->pages_offset = be64_to_cpu(header->pages_offset);
> +
> +    return true;
> +}
> +
>  /*
>   * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
>   * long-running RCU critical section.  When rcu-reclaims in the code
> @@ -3892,6 +3927,80 @@ void colo_flush_ram_cache(void)
>      trace_colo_flush_ram_cache_end();
>  }
>  
> +static void read_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block,
> +                                    long num_pages, unsigned long *bitmap)
> +{
> +    unsigned long set_bit_idx, clear_bit_idx;
> +    ram_addr_t offset;
> +    void *host;
> +    size_t read, unread, size, buf_size = FIXED_RAM_LOAD_BUF_SIZE;
> +
> +    for (set_bit_idx = find_first_bit(bitmap, num_pages);
> +         set_bit_idx < num_pages;
> +         set_bit_idx = find_next_bit(bitmap, num_pages, clear_bit_idx + 1)) {
> +
> +        clear_bit_idx = find_next_zero_bit(bitmap, num_pages, set_bit_idx + 1);
> +
> +        unread = TARGET_PAGE_SIZE * (clear_bit_idx - set_bit_idx);
> +        offset = set_bit_idx << TARGET_PAGE_BITS;
> +
> +        while (unread > 0) {
> +            host = host_from_ram_block_offset(block, offset);
> +            size = MIN(unread, buf_size);

Use the macro directly?  buf_size can be dropped then.

> +
> +            read = qemu_get_buffer_at(f, host, size,
> +                                      block->pages_offset + offset);

Error detection missing?  qemu_get_buffer_at() returns 0 if error, then it
dead loops.

> +            offset += read;
> +            unread -= read;
> +        }
> +    }
> +}
> +
> +static int parse_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block,
> +                                    ram_addr_t length, Error **errp)

For new code, start to keep using boolean as retval when Error** exists?

> +{
> +    g_autofree unsigned long *bitmap = NULL;
> +    FixedRamHeader header;
> +    size_t bitmap_size;
> +    long num_pages;
> +
> +    if (!fixed_ram_read_header(f, &header, errp)) {
> +        return -EINVAL;
> +    }
> +
> +    block->pages_offset = header.pages_offset;
> +
> +    /*
> +     * Check the alignment of the file region that contains pages. We
> +     * don't enforce FIXED_RAM_FILE_OFFSET_ALIGNMENT to allow that
> +     * value to change in the future. Do only a sanity check with page
> +     * size alignment.
> +     */
> +    if (!QEMU_IS_ALIGNED(block->pages_offset, TARGET_PAGE_SIZE)) {
> +        error_setg(errp,
> +                   "Error reading ramblock %s pages, region has bad alignment",
> +                   block->idstr);
> +        return -EINVAL;
> +    }
> +
> +    num_pages = length / header.page_size;
> +    bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long);
> +
> +    bitmap = g_malloc0(bitmap_size);
> +    if (qemu_get_buffer_at(f, (uint8_t *)bitmap, bitmap_size,
> +                           header.bitmap_offset) != bitmap_size) {
> +        error_setg(errp, "Error reading dirty bitmap");
> +        return -EINVAL;
> +    }
> +
> +    read_ramblock_fixed_ram(f, block, num_pages, bitmap);

Detect error and fail properly?

> +
> +    /* Skip pages array */
> +    qemu_set_offset(f, block->pages_offset + length, SEEK_SET);
> +
> +    return 0;
> +}
> +
>  static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
>  {
>      int ret = 0;
> @@ -3900,6 +4009,16 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
>  
>      assert(block);
>  
> +    if (migrate_fixed_ram()) {
> +        Error *local_err = NULL;
> +
> +        ret = parse_ramblock_fixed_ram(f, block, length, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
> +        return ret;

We can optionally add one pre-requisite patch to convert parse_ramblock()
to return boolean too.  I remember it was done somewhere before, but maybe
not merged.

> +    }
> +
>      if (!qemu_ram_is_migratable(block)) {
>          error_report("block %s should not be migrated !", block->idstr);
>          return -EINVAL;
> -- 
> 2.35.3
> 

-- 
Peter Xu