RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 12 February 2020 18:21
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com;
> dgilbert@redhat.com; Juan Jose Quintela Carreira <quintela@redhat.com>
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback

[...]

> > Hmm..it breaks x86 + seabios boot. The issue is seabios expects RSDP in FSEG
> > memory. With the above proposed change, RSDP will be aligned to PAGE_SIZE
> and
> > seabios mem allocation for RSDP fails at,
> >
> >
> https://github.com/coreboot/seabios/blob/master/src/fw/romfile_loader.c#L8
> 5
> >
> > To get pass the above, I changed "alloc_fseg" flag to false in build_rsdp(), but
> > seabios seems to make the assumption that RSDP has to be placed in FSEG
> memory
> > here,
> > https://github.com/coreboot/seabios/blob/master/src/fw/biostables.c#L126
> >
> > So doesn’t look like there is an easy fix for this without changing the seabios
> code.
> >
> > Between, OVMF works fine with the aligned size on x86.
> >
> > One thing we can do is treat the RSDP case separately or only use the aligned
> > page size for "etc/acpi/tables" as below,

[...]

> >
> > Thoughts?
> 
> I don't think introducing memory_region_get_used_length() is a
> good idea. I also dislike, that the ram block size can differ
> to the memory region size. I wasn't aware of that condition, sorry!

Right. They can differ in size and is the case here.

> Making the memory region always store an aligned size might break other use
> cases.
> 
> Summarizing the issue:
> 1. Memory regions contain ram blocks with a different size, if the size is
>    not properly aligned. While memory regions can have an unaligned size,
>    ram blocks can't. This is true when creating resizable memory region with
>    an unaligned size.
> 2. When resizing a ram block/memory region, the size of the memory region
>    is set to the aligned size. The callback is called with the aligned size.
>    The unaligned piece is lost.
> 3. When migrating, we migrate the aligned size.
> 
>
> What about something like this: (untested)

Thanks for that. I had a go with the below patch and it indeed fixes the issue
of callback not being called on resize. But the migration fails with the below
error,

For x86
---------
qemu-system-x86_64: Unknown combination of migration flags: 0x14
qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
qemu-system-x86_64: load of migration failed: Invalid argument 

For arm64
--------------
qemu-system-aarch64: Received an unexpected compressed page
qemu-system-aarch64: error while loading state for instance 0x0 of device 'ram'
qemu-system-aarch64: load of migration failed: Invalid argument
 
I haven’t debugged this further but looks like there is a corruption happening.
Please let me know if you have any clue.

Thanks,
Shameer

> 
> From d84c21bc67e15acdac2f6265cd1576d8dd920211 Mon Sep 17 00:00:00
> 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Wed, 12 Feb 2020 19:16:34 +0100
> Subject: [PATCH v1] tmp
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  exec.c          | 14 ++++++++++++--
>  migration/ram.c | 44 ++++++++++++++++++++++++++++++++------------
>  2 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 05cfe868ab..d41a1e11b5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2130,11 +2130,21 @@ static int memory_try_enable_merging(void
> *addr, size_t len)
>   */
>  int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
>  {
> +    const ram_addr_t unaligned_size = newsize;
> +
>      assert(block);
> 
>      newsize = HOST_PAGE_ALIGN(newsize);
> 
>      if (block->used_length == newsize) {
> +        /*
> +         * We don't have to resize the ram block (which only knows aligned
> +         * sizes), however, we have to notify if the unaligned size changed.
> +         */
> +        if (block->resized && unaligned_size !=
> memory_region_size(block->mr)) {
> +            block->resized(block->idstr, unaligned_size, block->host);
> +            memory_region_set_size(block->mr, unaligned_size);
> +        }
>          return 0;
>      }
> 
> @@ -2158,9 +2168,9 @@ int qemu_ram_resize(RAMBlock *block,
> ram_addr_t newsize, Error **errp)
>      block->used_length = newsize;
>      cpu_physical_memory_set_dirty_range(block->offset,
> block->used_length,
>                                          DIRTY_CLIENTS_ALL);
> -    memory_region_set_size(block->mr, newsize);
> +    memory_region_set_size(block->mr, unaligned_size);
>      if (block->resized) {
> -        block->resized(block->idstr, newsize, block->host);
> +        block->resized(block->idstr, unaligned_size, block->host);
>      }
>      return 0;
>  }
> diff --git a/migration/ram.c b/migration/ram.c
> index ed23ed1c7c..2acc4b85ca 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1761,28 +1761,43 @@ void acct_update_position(QEMUFile *f, size_t
> size, bool zero)
>      }
>  }
> 
> -static uint64_t ram_bytes_total_common(bool count_ignored)
> +static uint64_t ramblock_ram_bytes_migration(RAMBlock *block)
> +{
> +    /*
> +     * When resizing on the target, we need the unaligned size,
> +     * otherwise, we lose the unaligned size during migration.
> +     */
> +    if (block->mr && (block->flags & RAM_RESIZEABLE)) {
> +        return memory_region_size(block->mr);
> +    } else {
> +        return block->used_length;
> +    }
> +}
> +
> +static uint64_t ram_bytes_total_migration(void)
>  {
>      RAMBlock *block;
>      uint64_t total = 0;
> 
>      RCU_READ_LOCK_GUARD();
> 
> -    if (count_ignored) {
> -        RAMBLOCK_FOREACH_MIGRATABLE(block) {
> -            total += block->used_length;
> -        }
> -    } else {
> -        RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> -            total += block->used_length;
> -        }
> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        total += ramblock_ram_bytes_migration(block);
>      }
>      return total;
>  }
> 
>  uint64_t ram_bytes_total(void)
>  {
> -    return ram_bytes_total_common(false);
> +    RAMBlock *block;
> +    uint64_t total = 0;
> +
> +    RCU_READ_LOCK_GUARD();
> +
> +    RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> +        total += block->used_length;
> +    }
> +    return total;
>  }
> 
>  static void xbzrle_load_setup(void)
> @@ -2432,12 +2447,17 @@ static int ram_save_setup(QEMUFile *f, void
> *opaque)
>      (*rsp)->f = f;
> 
>      WITH_RCU_READ_LOCK_GUARD() {
> -        qemu_put_be64(f, ram_bytes_total_common(true) |
> RAM_SAVE_FLAG_MEM_SIZE);
> +        qemu_put_be64(f, ram_bytes_total_migration() |
> RAM_SAVE_FLAG_MEM_SIZE);
> 
>          RAMBLOCK_FOREACH_MIGRATABLE(block) {
>              qemu_put_byte(f, strlen(block->idstr));
>              qemu_put_buffer(f, (uint8_t *)block->idstr,
> strlen(block->idstr));
> -            qemu_put_be64(f, block->used_length);
> +            /*
> +             * When resizing on the target, we need the unaligned size,
> +             * otherwise we lose the unaligned sise during migration.
> +             */
> +            qemu_put_be64(f, ramblock_ram_bytes_migration(block));
> +
>              if (migrate_postcopy_ram() && block->page_size !=
>                                            qemu_host_page_size) {
>                  qemu_put_be64(f, block->page_size);
> --
> 2.24.1
> 
> 
> --
> Thanks,
> 
> David / dhildenb