From: David Hildenbrand <david@redhat.com>

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.

Because of the above, 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.

Hence make sure callback is called if memory region size is changed,
irrespective of aligned or not.

Signed-off-by: David Hildenbrand <david@redhat.com>
[Shameer: added commit log]
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
Please find the discussion here,
https://patchwork.kernel.org/patch/11339591/
---
 exec.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index 0cc500d53a..f8974cd303 100644
--- a/exec.c
+++ b/exec.c
@@ -2073,11 +2073,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;
     }
 
@@ -2101,9 +2111,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;
 }
-- 
2.17.1

On 11.03.20 18:20, Shameer Kolothum wrote:
> From: David Hildenbrand <david@redhat.com>
> 
> 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.
> 
> Because of the above, 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.
> 
> Hence make sure callback is called if memory region size is changed,
> irrespective of aligned or not.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> [Shameer: added commit log]
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> Please find the discussion here,
> https://patchwork.kernel.org/patch/11339591/
> ---
>  exec.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0cc500d53a..f8974cd303 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2073,11 +2073,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);
> +        }

I guess we should do

if (unaligned_size != memory_region_size(block->mr)) {
    memory_region_set_size(block->mr, unaligned_size);
    if (block->resized) {
        block->resized(block->idstr, unaligned_size, block->host);
    }
}

Instead - like in the case below.


Note: This is not completely clean, the RAM block code should'n have to
care about unaligned stuff. Also, the resized() callback for the RAM
block is ugly, it should be a resized callback for the memory region.
But these things imply requires bigger refactorings, so I guess this is
good and simple enough for now.

-- 
Thanks,

David / dhildenb

On Wed, 11 Mar 2020 17:20:07 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> From: David Hildenbrand <david@redhat.com>
> 
> 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.
> 
> Because of the above, 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.
> 
> Hence make sure callback is called if memory region size is changed,
> irrespective of aligned or not.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> [Shameer: added commit log]
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> Please find the discussion here,
> https://patchwork.kernel.org/patch/11339591/
> ---
>  exec.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0cc500d53a..f8974cd303 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2073,11 +2073,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;
>      }
>  
> @@ -2101,9 +2111,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;
>  }