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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 17 Jan 2020 17:45:16 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> If ACPI blob length modifications happens after the initial
> virt_acpi_build() call, and the changed blob length is within
> the PAGE size boundary, then the revised size is not seen by
> the firmware on Guest reboot. The is because in the
> virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> path, qemu_ram_resize() uses used_length (ram_block size which is
> aligned to PAGE size) and the "resize callback" to update the size
> seen by firmware is not getting invoked.
> 
> Hence make sure callback is called if the new size is different
> from original requested size.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> Please find the previous discussions on this issue here,
> https://patchwork.kernel.org/patch/11174947/
> 
> But this one attempts a different solution to fix it by introducing
> req_length var to RAMBlock struct. 
> 

looks fine to me, so
Acked-by: Igor Mammedov <imammedo@redhat.com>

CCing David who touches this area in his latest series for and
might have an opinion on how it should be handled.

> ---
>  exec.c                  | 36 +++++++++++++++++++++++-------------
>  include/exec/ram_addr.h |  5 +++--
>  2 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index d4b769d0d4..9ce33992f8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2123,16 +2123,18 @@ static int memory_try_enable_merging(void *addr, size_t len)
>   * resize callback to update device state and/or add assertions to detect
>   * misuse, if necessary.
>   */
> -int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
> +int qemu_ram_resize(RAMBlock *block, ram_addr_t size, Error **errp)
>  {
> -    assert(block);
> +    ram_addr_t newsize;
>  
> -    newsize = HOST_PAGE_ALIGN(newsize);
> +    assert(block);
>  
> -    if (block->used_length == newsize) {
> +    if (block->req_length == size) {
>          return 0;
>      }
>  
> +    newsize = HOST_PAGE_ALIGN(size);
> +
>      if (!(block->flags & RAM_RESIZEABLE)) {
>          error_setg_errno(errp, EINVAL,
>                           "Length mismatch: %s: 0x" RAM_ADDR_FMT
> @@ -2149,13 +2151,19 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
>          return -EINVAL;
>      }
>  
> -    cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
> -    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);
> +    block->req_length = size;
> +
> +    if (newsize != block->used_length) {
> +        cpu_physical_memory_clear_dirty_range(block->offset,
> +                                              block->used_length);
> +        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);
> +    }
> +
>      if (block->resized) {
> -        block->resized(block->idstr, newsize, block->host);
> +        block->resized(block->idstr, block->req_length, block->host);
>      }
>      return 0;
>  }
> @@ -2412,16 +2420,18 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
>                                    MemoryRegion *mr, Error **errp)
>  {
>      RAMBlock *new_block;
> +    ram_addr_t newsize;
>      Error *local_err = NULL;
>  
> -    size = HOST_PAGE_ALIGN(size);
> +    newsize = HOST_PAGE_ALIGN(size);
>      max_size = HOST_PAGE_ALIGN(max_size);
>      new_block = g_malloc0(sizeof(*new_block));
>      new_block->mr = mr;
>      new_block->resized = resized;
> -    new_block->used_length = size;
> +    new_block->req_length = size;
> +    new_block->used_length = newsize;
>      new_block->max_length = max_size;
> -    assert(max_size >= size);
> +    assert(max_size >= newsize);
>      new_block->fd = -1;
>      new_block->page_size = qemu_real_host_page_size;
>      new_block->host = host;
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 5adebb0bc7..fd13082224 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -31,8 +31,9 @@ struct RAMBlock {
>      uint8_t *host;
>      uint8_t *colo_cache; /* For colo, VM's ram cache */
>      ram_addr_t offset;
> -    ram_addr_t used_length;
> -    ram_addr_t max_length;
> +    ram_addr_t req_length; /* Original requested size, used if RAM_RESIZEABLE */
> +    ram_addr_t used_length; /* aligned to qemu_host_page_size */
> +    ram_addr_t max_length; /*  aligned to qemu_host_page_size */
>      void (*resized)(const char*, uint64_t length, void *host);
>      uint32_t flags;
>      /* Protected by iothread lock.  */