[Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[zhuyijun]

From: Zhu Yijun <zhuyijun@huawei.com>

Commit 0b183fc871:"memory: move mem_path handling to
memory_region_allocate_system_memory" split memory_region_init_ram and
memory_region_init_ram_from_file. Also it moved mem-path handling a step
up from memory_region_init_ram to memory_region_allocate_system_memory.

Therefore for any board that uses memory_region_init_ram directly,
-mem-path is not supported.

Commit e938ba0c35:"ppc: memory: Replace memory_region_init_ram with
memory_region_allocate_system_memory" have already fixed this on ppc.

arm/arm64 board also occurs, this patch is only for arm64 board(virt).

Signed-off-by: Zhu Yijun <zhuyijun@huawei.com>
---
 hw/arm/virt.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8c6b171..32646a1 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -580,9 +580,8 @@ static void machvirt_init(MachineState *machine)
     fdt_add_cpu_nodes(vbi);
     fdt_add_psci_node(vbi);
 
-    memory_region_init_ram(ram, NULL, "mach-virt.ram", machine->ram_size,
-                           &error_abort);
-    vmstate_register_ram_global(ram);
+    memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram",
+                                         machine->ram_size);
     memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram);
 
     create_flash(vbi);
-- 
1.7.1

Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Peter Maydell]

On 14 October 2014 04:36, zhuyijun <zhuyijun@huawei.com> wrote:
> From: Zhu Yijun <zhuyijun@huawei.com>
>
> Commit 0b183fc871:"memory: move mem_path handling to
> memory_region_allocate_system_memory" split memory_region_init_ram and
> memory_region_init_ram_from_file. Also it moved mem-path handling a step
> up from memory_region_init_ram to memory_region_allocate_system_memory.
>
> Therefore for any board that uses memory_region_init_ram directly,
> -mem-path is not supported.
>
> Commit e938ba0c35:"ppc: memory: Replace memory_region_init_ram with
> memory_region_allocate_system_memory" have already fixed this on ppc.
>
> arm/arm64 board also occurs, this patch is only for arm64 board(virt).

Why is this patch only changing this board? What's special
about virt that means we don't want to also make this
change for the other ARM boards? What about all the other
boards for the other architectures?

The commit message for 0b183fc871 suggests we decided to
just break -mem-path for all those boards, which seems an
odd thing to do... Paolo?

Incidentally I can't see anything that guards against
calling memory_region_allocate_system_memory() twice,
so I think you would end up with two blocks of RAM
both backed by the same file then. Or have I misread
the code?

-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Paolo Bonzini]

Il 14/10/2014 06:54, Peter Maydell ha scritto:
> Why is this patch only changing this board? What's special
> about virt that means we don't want to also make this
> change for the other ARM boards? What about all the other
> boards for the other architectures?

-mem-path is not too useful without KVM (TCG is too slow to
notice the difference.  PPC and S390 have already been fixed.

For other boards, I have patches but I had no time to submit
them until now.

> The commit message for 0b183fc871 suggests we decided to
> just break -mem-path for all those boards, which seems an
> odd thing to do... Paolo?

The plan _was_ in fact to fix it up. :(

> Incidentally I can't see anything that guards against
> calling memory_region_allocate_system_memory() twice,
> so I think you would end up with two blocks of RAM
> both backed by the same file then. Or have I misread
> the code?

That would be a bug in the board, it is caught here:

        if (memory_region_is_mapped(seg)) {
            char *path = object_get_canonical_path_component(OBJECT(backend));
            error_report("memory backend %s is used multiple times. Each "
                         "-numa option must use a different memdev value.",
                         path);
            exit(1);
        }

The error message gives the common user error rather than
the less common developer error, but still you catch it.

Paolo

Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Peter Maydell]

On 14 October 2014 07:10, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 14/10/2014 06:54, Peter Maydell ha scritto:
>> Why is this patch only changing this board? What's special
>> about virt that means we don't want to also make this
>> change for the other ARM boards? What about all the other
>> boards for the other architectures?
>
> -mem-path is not too useful without KVM (TCG is too slow to
> notice the difference.  PPC and S390 have already been fixed.

MIPS has KVM too now...

> For other boards, I have patches but I had no time to submit
> them until now.
>
>> The commit message for 0b183fc871 suggests we decided to
>> just break -mem-path for all those boards, which seems an
>> odd thing to do... Paolo?
>
> The plan _was_ in fact to fix it up. :(
>
>> Incidentally I can't see anything that guards against
>> calling memory_region_allocate_system_memory() twice,
>> so I think you would end up with two blocks of RAM
>> both backed by the same file then. Or have I misread
>> the code?
>
> That would be a bug in the board, it is caught here:
>
>         if (memory_region_is_mapped(seg)) {
>             char *path = object_get_canonical_path_component(OBJECT(backend));
>             error_report("memory backend %s is used multiple times. Each "
>                          "-numa option must use a different memdev value.",
>                          path);
>             exit(1);
>         }
>
> The error message gives the common user error rather than
> the less common developer error, but still you catch it.

That's only in the NUMA code path though, isn't it?
I was looking at the non-numa codepath, which is what
all the boards I care about are going to be taking :-)

What's the right thing for a board where the system
RAM isn't contiguous, by the way?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Yijun Zhu]

Hi Peter,

On 2014/10/14 12:54, Peter Maydell wrote:

> On 14 October 2014 04:36, zhuyijun <zhuyijun@huawei.com> wrote:
>> From: Zhu Yijun <zhuyijun@huawei.com>
>>
>> Commit 0b183fc871:"memory: move mem_path handling to
>> memory_region_allocate_system_memory" split memory_region_init_ram and
>> memory_region_init_ram_from_file. Also it moved mem-path handling a step
>> up from memory_region_init_ram to memory_region_allocate_system_memory.
>>
>> Therefore for any board that uses memory_region_init_ram directly,
>> -mem-path is not supported.
>>
>> Commit e938ba0c35:"ppc: memory: Replace memory_region_init_ram with
>> memory_region_allocate_system_memory" have already fixed this on ppc.
>>
>> arm/arm64 board also occurs, this patch is only for arm64 board(virt).
> 
> Why is this patch only changing this board? What's special
> about virt that means we don't want to also make this
> change for the other ARM boards? What about all the other
> boards for the other architectures?
> 

I think all of the other ARM boards should be changed. If changes in virt is ok,
I will make the patch again including the modification of other ARM boards.

Best regards,
Yijun

> The commit message for 0b183fc871 suggests we decided to
> just break -mem-path for all those boards, which seems an
> odd thing to do... Paolo?
> 
> Incidentally I can't see anything that guards against
> calling memory_region_allocate_system_memory() twice,
> so I think you would end up with two blocks of RAM
> both backed by the same file then. Or have I misread
> the code?
> 
> -- PMM
> 
> 

Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Paolo Bonzini]

Il 14/10/2014 07:42, Peter Maydell ha scritto:
> On 14 October 2014 07:10, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 14/10/2014 06:54, Peter Maydell ha scritto:
>>> Why is this patch only changing this board? What's special
>>> about virt that means we don't want to also make this
>>> change for the other ARM boards? What about all the other
>>> boards for the other architectures?
>>
>> -mem-path is not too useful without KVM (TCG is too slow to
>> notice the difference.  PPC and S390 have already been fixed.
> 
> MIPS has KVM too now...

Yes.

>>> Incidentally I can't see anything that guards against
>>> calling memory_region_allocate_system_memory() twice,
>>> so I think you would end up with two blocks of RAM
>>> both backed by the same file then. Or have I misread
>>> the code?
>>
>> That would be a bug in the board, it is caught here:
>>
>>         if (memory_region_is_mapped(seg)) {
>>             char *path = object_get_canonical_path_component(OBJECT(backend));
>>             error_report("memory backend %s is used multiple times. Each "
>>                          "-numa option must use a different memdev value.",
>>                          path);
>>             exit(1);
>>         }
>>
>> The error message gives the common user error rather than
>> the less common developer error, but still you catch it.
> 
> That's only in the NUMA code path though, isn't it?
> I was looking at the non-numa codepath, which is what
> all the boards I care about are going to be taking :-)

The non-NUMA path will allocate memory from two separate files.
-mem-path takes a path, not a file.

> What's the right thing for a board where the system
> RAM isn't contiguous, by the way?

x86 allocates a big RAM region and uses aliases to split it into
non-contiguous areas.

Paolo

Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Paolo Bonzini]

Il 14/10/2014 08:04, Yijun Zhu ha scritto:
> I think all of the other ARM boards should be changed. If changes in virt is ok,
> I will make the patch again including the modification of other ARM boards.

Don't worry, it's not your fault. :)  I introduced the bug, and if you
prefer I will rebase and post the patches I had.

Paolo

Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Peter Maydell]

On 14 October 2014 14:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 14/10/2014 07:42, Peter Maydell ha scritto:
>> That's only in the NUMA code path though, isn't it?
>> I was looking at the non-numa codepath, which is what
>> all the boards I care about are going to be taking :-)
>
> The non-NUMA path will allocate memory from two separate files.
> -mem-path takes a path, not a file.

Thanks. I hadn't followed the code all the way down...

>> What's the right thing for a board where the system
>> RAM isn't contiguous, by the way?
>
> x86 allocates a big RAM region and uses aliases to split it into
> non-contiguous areas.

Why don't we just do that automatically for all RAM regions
the machine creates? We're already splitting up a contiguous
chunk of RAM to parcel out to RAM regions, that's what the
indirection through ramaddrs is all about.

-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Yijun Zhu]

On 2014/10/14 20:40, Paolo Bonzini wrote:

> Il 14/10/2014 08:04, Yijun Zhu ha scritto:
>> I think all of the other ARM boards should be changed. If changes in virt is ok,
>> I will make the patch again including the modification of other ARM boards.
> 
> Don't worry, it's not your fault. :)  I introduced the bug, and if you
> prefer I will rebase and post the patches I had.
> 

OK, I'll test it at that time. thanks!

Best regards,
Yijun

> Paolo
> 
> 
> 

Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Paolo Bonzini]

Il 14/10/2014 14:48, Peter Maydell ha scritto:
> Why don't we just do that automatically for all RAM regions
> the machine creates? We're already splitting up a contiguous
> chunk of RAM to parcel out to RAM regions, that's what the
> indirection through ramaddrs is all about.

I can think of one reason to not do this.  If we just mmap-ed RAM
regions consecutively from a file, the offset in the file should be
aligned to the huge page size for "main" RAM region(s).  For example,
assume gigabyte-sized huge pages; the offset for main RAM must be
gigabyte aligned.  If the board creates 16MiB of VRAM first, and 2 GiB
of main RAM second, you will end up wasting 1024-16 MiB of RAM for VRAM.
 This same waste happened before the introduction of
memory_region_allocate_system_memory, in fact.

Of course this could in principle be fixed by having boards create main
RAM first, but it would be an even more complicated change.  You could
come up with a way of reserving ram_addr_t ranges, which would also help
hotplug; however, I am not sure if all architectures require the maximum
hotplugged memory size to be specified upfront.

Ultimately, if a RAM region other than main RAM has benefits from huge
pages, the device that creates it probably should have a memdev
property.  This is not the case for any current device, except perhaps
ivshmem.

Paolo