[Qemu-devel] [PATCH for-2.3] cris: memory: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Qemu-devel] [PATCH for-2.3] cris: memory: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Dirk Müller]

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.

Fix this by replacing memory_region_init_ram with
memory_region_allocate_system_memory.

Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Signed-off-by: Dirk Mueller <dmueller@suse.com>
---
 hw/cris/axis_dev88.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/cris/axis_dev88.c b/hw/cris/axis_dev88.c
index 0479196..3cae480 100644
--- a/hw/cris/axis_dev88.c
+++ b/hw/cris/axis_dev88.c
@@ -270,9 +270,8 @@ void axisdev88_init(MachineState *machine)
     env = &cpu->env;

     /* allocate RAM */
-    memory_region_init_ram(phys_ram, NULL, "axisdev88.ram", ram_size,
-                           &error_abort);
-    vmstate_register_ram_global(phys_ram);
+    memory_region_allocate_system_memory(phys_ram, NULL, "axisdev88.ram",
+                                         ram_size);
     memory_region_add_subregion(address_space_mem, 0x40000000, phys_ram);

     /* The ETRAX-FS has 128Kb on chip ram, the docs refer to it as the
-- 
2.0.4

Re: [Qemu-devel] [PATCH for-2.3] cris: memory: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Edgar E. Iglesias]

On Sat, Apr 04, 2015 at 02:15:10PM +0200, Dirk Müller wrote:
> 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.
> 
> Fix this by replacing memory_region_init_ram with
> memory_region_allocate_system_memory.
> 
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Signed-off-by: Dirk Mueller <dmueller@suse.com>
> ---
>  hw/cris/axis_dev88.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)


Hi,

A question, should this only be done for one of the memories?
BTW, I'm having problems git am:ing this patch, not sure why...

Cheers,
Edgar


> 
> diff --git a/hw/cris/axis_dev88.c b/hw/cris/axis_dev88.c
> index 0479196..3cae480 100644
> --- a/hw/cris/axis_dev88.c
> +++ b/hw/cris/axis_dev88.c
> @@ -270,9 +270,8 @@ void axisdev88_init(MachineState *machine)
>      env = &cpu->env;
> 
>      /* allocate RAM */
> -    memory_region_init_ram(phys_ram, NULL, "axisdev88.ram", ram_size,
> -                           &error_abort);
> -    vmstate_register_ram_global(phys_ram);
> +    memory_region_allocate_system_memory(phys_ram, NULL, "axisdev88.ram",
> +                                         ram_size);
>      memory_region_add_subregion(address_space_mem, 0x40000000, phys_ram);
> 
>      /* The ETRAX-FS has 128Kb on chip ram, the docs refer to it as the
> -- 
> 2.0.4

Re: [Qemu-devel] [PATCH for-2.3] cris: memory: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Peter Maydell]

On 9 April 2015 at 04:47, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Sat, Apr 04, 2015 at 02:15:10PM +0200, Dirk Müller wrote:
>> 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.
>>
>> Fix this by replacing memory_region_init_ram with
>> memory_region_allocate_system_memory.
>>
>> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>> Signed-off-by: Dirk Mueller <dmueller@suse.com>
>> ---
>>  hw/cris/axis_dev88.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>
>
> Hi,
>
> A question, should this only be done for one of the memories?

Yes; as I understand it every board should call
memory_region_allocate_system_memory once and only once,
to allocate the "big lump" of RAM which should be backed by
the -mem-path memory (hugepages, etc). If a board's major
RAM is actually in two parts, you can deal with this by
calling memory_region_allocate_system_memory once to get
an MR with enough RAM for both parts, and then creating
alias MemoryRegions for each part which point into that.
[cf: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05073.html]

In the case of this cris board it looks to me from a
quick scan of the board init code like it has one big
lump of RAM (the one converted by this code) and then
a few smaller "internal ram" type things. So just doing
this for the big lump is correct.

(The only use of -mem-path here is to allow the user to
cause us to back QEMU's guest RAM allocations with host
huge pages, which is in turn only something you care about
for KVM. So for the TCG-only CPUs and boards this is
to some extent academic, except it would be nice to be
able to enable an assert that the board init does call
memory_region_allocate_system_memory exactly once so we
can catch cases where we forgot for KVM-enabled archs.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.3] cris: memory: Replace memory_region_init_ram with memory_region_allocate_system_memory

[Edgar E. Iglesias]

On Fri, Apr 10, 2015 at 03:29:56PM +0100, Peter Maydell wrote:
> On 9 April 2015 at 04:47, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Sat, Apr 04, 2015 at 02:15:10PM +0200, Dirk Müller wrote:
> >> 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.
> >>
> >> Fix this by replacing memory_region_init_ram with
> >> memory_region_allocate_system_memory.
> >>
> >> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> >> Signed-off-by: Dirk Mueller <dmueller@suse.com>
> >> ---
> >>  hw/cris/axis_dev88.c | 5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> >
> > Hi,
> >
> > A question, should this only be done for one of the memories?
> 
> Yes; as I understand it every board should call
> memory_region_allocate_system_memory once and only once,
> to allocate the "big lump" of RAM which should be backed by
> the -mem-path memory (hugepages, etc). If a board's major
> RAM is actually in two parts, you can deal with this by
> calling memory_region_allocate_system_memory once to get
> an MR with enough RAM for both parts, and then creating
> alias MemoryRegions for each part which point into that.
> [cf: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05073.html]
> 
> In the case of this cris board it looks to me from a
> quick scan of the board init code like it has one big
> lump of RAM (the one converted by this code) and then
> a few smaller "internal ram" type things. So just doing
> this for the big lump is correct.
> 
> (The only use of -mem-path here is to allow the user to
> cause us to back QEMU's guest RAM allocations with host
> huge pages, which is in turn only something you care about
> for KVM. So for the TCG-only CPUs and boards this is
> to some extent academic, except it would be nice to be
> able to enable an assert that the board init does call
> memory_region_allocate_system_memory exactly once so we
> can catch cases where we forgot for KVM-enabled archs.)
> 

I see, thanks for explaining. I gave a quick try with a CRIS image and it works fine.

Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

The patch has some kind of whitespace issue, but it applies
with --ignore-whitespace. I've pushed it now, thanks!

Cheers,
Edgar