[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
From: |
Shameerali Kolothum Thodi |
Subject: |
RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback |
Date: |
Thu, 13 Feb 2020 16:38:56 +0000 |
> -----Original Message-----
> From: David Hildenbrand [mailto:address@hidden]
> Sent: 12 February 2020 18:21
> 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;
> address@hidden; Juan Jose Quintela Carreira <address@hidden>
> 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 <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
- RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, (continued)
- RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, Shameerali Kolothum Thodi, 2020/02/06
- Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, David Hildenbrand, 2020/02/06
- RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, Shameerali Kolothum Thodi, 2020/02/06
- Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, David Hildenbrand, 2020/02/06
- RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, Shameerali Kolothum Thodi, 2020/02/07
- Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, David Hildenbrand, 2020/02/10
- RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, Shameerali Kolothum Thodi, 2020/02/10
- Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, David Hildenbrand, 2020/02/10
- RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, Shameerali Kolothum Thodi, 2020/02/12
- Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, David Hildenbrand, 2020/02/12
- RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback,
Shameerali Kolothum Thodi <=
- Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, David Hildenbrand, 2020/02/13
- Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, David Hildenbrand, 2020/02/13
- RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, Shameerali Kolothum Thodi, 2020/02/28
- Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback, David Hildenbrand, 2020/02/28