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

[David Hildenbrand]

On 12.02.20 18:07, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: David Hildenbrand [mailto:address@hidden]
>> Sent: 10 February 2020 09:54
>> To: Shameerali Kolothum Thodi <address@hidden>;
>> Igor Mammedov <address@hidden>
>> Cc: address@hidden; address@hidden;
>> address@hidden; address@hidden; address@hidden;
>> xuwei (O) <address@hidden>; Linuxarm <address@hidden>;
>> address@hidden; address@hidden; address@hidden
>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>>
>> On 10.02.20 10:50, Shameerali Kolothum Thodi wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: David Hildenbrand [mailto:address@hidden]
>>>> Sent: 10 February 2020 09:29
>>>> To: Shameerali Kolothum Thodi <address@hidden>;
>>>> Igor Mammedov <address@hidden>
>>>> Cc: address@hidden; address@hidden;
>>>> address@hidden; address@hidden; address@hidden;
>>>> xuwei (O) <address@hidden>; Linuxarm <address@hidden>;
>>>> address@hidden; address@hidden; address@hidden
>>>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>>>>
>>>>>> Can you look the original value up somehow and us the resize callback
>>>>>> only as a notification that something changed? (that value would have to
>>>>>> be stored somewhere and migrated I assume - maybe that's already
>> being
>>>>>> done)
>>>>>
>>>>> Ok. I will take a look at that. But can we instead pass the
>> block->used_length
>>>> to
>>>>> fw_cfg_add_file_callback(). That way we don’t have to change the
>>>> qemu_ram_resize()
>>>>> as well. I think Igor has suggested this before[1] and I had a go at it 
>>>>> before
>>>> coming up
>>>>> with the "req_length" proposal here.
>>>>
>>>> You mean, passing the old size as well? I don't see how that will solve
>>>> the issue, but yeah, nothing speaks against simply sending the old and
>>>> the new size.
>>>
>>> Nope. I actually meant using the block->used_length to store in the
>>> s->files->f[index].size.
>>>
>>> virt_acpi_setup()
>>>   acpi_add_rom_blob()
>>>     rom_add_blob()
>>>       rom_set_mr()  --> used_length  = page aligned blob size
>>>         fw_cfg_add_file_callback()  --> uses actual blob size.
>>>
>>>
>>> Right now what we do is use the actual blob size to store in FWCfgEntry.
>>> Instead pass the RAMBlock used_length to fw_cfg_add_file_callback().
>>> Of course by this, the firmware will see an aligned size, but that is fine 
>>> I think.
>>> But at the same time this means the qemu_ram_resize() can stay as it is
>>> because it will invoke the callback when the size changes beyond the aligned
>>> page size. And also during migration, there won't be any inconsistency as
>> everyone
>>> works on aligned page size.
>>>
>>> Does that make sense? Or I am again missing something here?
>>
>> Oh, you mean simply rounding up to full pages in the fw entries? If we
>> can drop the "sub-page" restriction, that would be awesome!
>>
>> Need to double check if that could be an issue for fw/migration/whatever.
> 
> 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#L85
> 
> 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,
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index d1b78f60cd..f07f6a7a35 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -60,6 +60,7 @@
>  #include "hw/boards.h"
>  #include "qemu/cutils.h"
>  #include "sysemu/runstate.h"
> +#include "hw/acpi/aml-build.h"
>  
>  #include <zlib.h>
>  
> @@ -1056,6 +1057,7 @@ MemoryRegion *rom_add_blob(const char *name, const void 
> *blob, size_t len,
>      if (fw_file_name && fw_cfg) {
>          char devpath[100];
>          void *data;
> +        size_t size = rom->datasize;
>  
>          if (read_only) {
>              snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
> @@ -1066,13 +1068,21 @@ MemoryRegion *rom_add_blob(const char *name, const 
> void *blob, size_t len,
>          if (mc->rom_file_has_mr) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, read_only);
>              mr = rom->mr;
> +           /*
> +            * Use the RAMblk used_length for acpi tables as this avoids any
> +            * inconsistency with table size changes during hot add and during
> +            * migration.
> +            */
> +           if (strcmp(fw_file_name, ACPI_BUILD_TABLE_FILE) == 0) {
> +                size = memory_region_get_used_length(mr);
> +           }
>          } else {
>              data = rom->data;
>          }
>  
>          fw_cfg_add_file_callback(fw_cfg, fw_file_name,
>                                   fw_callback, NULL, callback_opaque,
> -                                 data, rom->datasize, read_only);
> +                                 data, size, read_only);
>      }
>      return mr;
>  }
> 
> 
> 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!

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)


>From d84c21bc67e15acdac2f6265cd1576d8dd920211 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <address@hidden>
Date: Wed, 12 Feb 2020 19:16:34 +0100
Subject: [PATCH v1] tmp

Signed-off-by: David Hildenbrand <address@hidden>
---
 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