diff mbox series

hostmem: Apply merge property after the memory region is initialized

Message ID 20240915233117.478169-1-gshan@redhat.com (mailing list archive)
State New, archived
Headers show
Series hostmem: Apply merge property after the memory region is initialized | expand

Commit Message

Gavin Shan Sept. 15, 2024, 11:31 p.m. UTC
The semantic change has been introduced by commit 5becdc0ab0 ("hostmem:
simplify the code for merge and dump properties") even it clarifies that
no senmatic change has been introduced. After the commit, the merge
property can be applied even the corresponding memory region isn't
initialized yet. This leads to crash dump by the following command
lines.

  # /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64  \
    -accel kvm  -machine virt -cpu host                      \
    -object memory-backend-ram,id=mem-memN0,size=4096M,merge=off
    :
    qemu-system-aarch64: ../system/memory.c:2419: memory_region_get_ram_ptr: \
    Assertion `mr->ram_block' failed.

Fix it by applying the merge property only when the memory region is
initialized.

Fixes: 5becdc0ab083 ("hostmem: simplify the code for merge and dump properties")
Reported-by: Zhenyu Zhang <zhenyzha@redhat.com>
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 backends/hostmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Zhenyu Zhang Sept. 16, 2024, 12:13 p.m. UTC | #1
[PATCH] hostmem: Apply merge property after the memory region is initialized

Test on 4k and 64k basic page size aarch64
The patches work well on my Ampere host.
The test results are as expected.

# /home/test/qemu.main/build/qemu-system-aarch64  \
   -accel kvm  -machine virt -cpu host                           \
   -object memory-backend-ram,id=mem-memN0,size=4096M,merge=off

Tested-by: Zhenyu Zhang <zhenyzha@redhat.com>


On Mon, Sep 16, 2024 at 7:31 AM Gavin Shan <gshan@redhat.com> wrote:
>
> The semantic change has been introduced by commit 5becdc0ab0 ("hostmem:
> simplify the code for merge and dump properties") even it clarifies that
> no senmatic change has been introduced. After the commit, the merge
> property can be applied even the corresponding memory region isn't
> initialized yet. This leads to crash dump by the following command
> lines.
>
>   # /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64  \
>     -accel kvm  -machine virt -cpu host                      \
>     -object memory-backend-ram,id=mem-memN0,size=4096M,merge=off
>     :
>     qemu-system-aarch64: ../system/memory.c:2419: memory_region_get_ram_ptr: \
>     Assertion `mr->ram_block' failed.
>
> Fix it by applying the merge property only when the memory region is
> initialized.
>
> Fixes: 5becdc0ab083 ("hostmem: simplify the code for merge and dump properties")
> Reported-by: Zhenyu Zhang <zhenyzha@redhat.com>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  backends/hostmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 4e5576a4ad..181446626a 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -178,7 +178,7 @@ static void host_memory_backend_set_merge(Object *obj, bool value, Error **errp)
>          return;
>      }
>
> -    if (!host_memory_backend_mr_inited(backend) &&
> +    if (host_memory_backend_mr_inited(backend) &&
>          value != backend->merge) {
>          void *ptr = memory_region_get_ram_ptr(&backend->mr);
>          uint64_t sz = memory_region_size(&backend->mr);
> --
> 2.45.2
>
David Hildenbrand Sept. 17, 2024, 8:29 a.m. UTC | #2
On 16.09.24 01:31, Gavin Shan wrote:
> The semantic change has been introduced by commit 5becdc0ab0 ("hostmem:
> simplify the code for merge and dump properties") even it clarifies that
> no senmatic change has been introduced. After the commit, the merge
> property can be applied even the corresponding memory region isn't
> initialized yet. This leads to crash dump by the following command
> lines.
> 
>    # /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64  \
>      -accel kvm  -machine virt -cpu host                      \
>      -object memory-backend-ram,id=mem-memN0,size=4096M,merge=off
>      :
>      qemu-system-aarch64: ../system/memory.c:2419: memory_region_get_ram_ptr: \
>      Assertion `mr->ram_block' failed.
> 
> Fix it by applying the merge property only when the memory region is
> initialized.
> 
> Fixes: 5becdc0ab083 ("hostmem: simplify the code for merge and dump properties")
> Reported-by: Zhenyu Zhang <zhenyzha@redhat.com>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   backends/hostmem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 4e5576a4ad..181446626a 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -178,7 +178,7 @@ static void host_memory_backend_set_merge(Object *obj, bool value, Error **errp)
>           return;
>       }
>   
> -    if (!host_memory_backend_mr_inited(backend) &&
> +    if (host_memory_backend_mr_inited(backend) &&
>           value != backend->merge) {
>           void *ptr = memory_region_get_ram_ptr(&backend->mr);
>           uint64_t sz = memory_region_size(&backend->mr);

Thanks, queued to

https://github.com/davidhildenbrand/qemu.git mem-next
diff mbox series

Patch

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4e5576a4ad..181446626a 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -178,7 +178,7 @@  static void host_memory_backend_set_merge(Object *obj, bool value, Error **errp)
         return;
     }
 
-    if (!host_memory_backend_mr_inited(backend) &&
+    if (host_memory_backend_mr_inited(backend) &&
         value != backend->merge) {
         void *ptr = memory_region_get_ram_ptr(&backend->mr);
         uint64_t sz = memory_region_size(&backend->mr);