Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory
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
Re: [Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory
Hi Peter, On 2014/10/14 12:54, Peter Maydell wrote: On 14 October 2014 04:36, zhuyijun zhuyi...@huawei.com wrote: From: Zhu Yijun zhuyi...@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
Il 14/10/2014 07:42, Peter Maydell ha scritto:
On 14 October 2014 07:10, Paolo Bonzini pbonz...@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
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
On 14 October 2014 14:39, Paolo Bonzini pbonz...@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
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
[Qemu-devel] [PATCH] hw/arm/virt: Replace memory_region_init_ram with memory_region_allocate_system_memory
From: Zhu Yijun zhuyi...@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 zhuyi...@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
On 14 October 2014 04:36, zhuyijun zhuyi...@huawei.com wrote: From: Zhu Yijun zhuyi...@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
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
On 14 October 2014 07:10, Paolo Bonzini pbonz...@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
