[Qemu-devel] [PATCH 0/3] Fix memory migration for exynos 4210 SoC

[Qemu-devel] [PATCH 0/3] Fix memory migration for exynos 4210 SoC

[Igor Mitsyanko]

First two patches fix issues in exynos4210 code which were blocking proper memory
migration.

Third patch makes memory_region_init_ram_ptr assert if memory region size is not a
multiple of TARGET_PAGE_SIZE.

Igor Mitsyanko (3):
  hw/exynos4210.c: set chipid_and_omr array size to TARGET_PAGE_SIZE
  exynos4210.c: register chipid_mem and rom_mem with vmstate
  memory.c: only allow memory sizes which are a multiple of target page
    size

 hw/exynos4210.c       |    4 +++-
 include/exec/memory.h |    2 +-
 memory.c              |    1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCH 1/3] hw/exynos4210.c: set chipid_and_omr array size to TARGET_PAGE_SIZE

[Igor Mitsyanko]

During initialization, memory region size is aligned to page size,
but size of chipid_and_omr array is less then TARGET_PAGE_SIZE. This could result
in errors in some cases, specifically, it could cause errors during VM migration.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@gmail.com>
---
 hw/exynos4210.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/exynos4210.c b/hw/exynos4210.c
index fa54e42..280d5d4 100644
--- a/hw/exynos4210.c
+++ b/hw/exynos4210.c
@@ -76,7 +76,7 @@
 /* EHCI */
 #define EXYNOS4210_EHCI_BASE_ADDR           0x12580000
 
-static uint8_t chipid_and_omr[] = { 0x11, 0x02, 0x21, 0x43,
+static uint8_t chipid_and_omr[TARGET_PAGE_SIZE] = { 0x11, 0x02, 0x21, 0x43,
                                     0x09, 0x00, 0x00, 0x00 };
 
 void exynos4210_write_secondary(ARMCPU *cpu,
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/3] exynos4210.c: register chipid_mem and rom_mem with vmstate

[Igor Mitsyanko]

Even if we do not register newly created RAM MemoryRegion for migration with
vmstate_register_ram_global() function, ram_save_setup() still saves this region
to snapshot file with empty idstr=="". Consequently this results in error during
VM loading in ram_load().
Register chipid_mem and rom_mem for migration.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@gmail.com>
---
 hw/exynos4210.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/exynos4210.c b/hw/exynos4210.c
index 280d5d4..af741e2 100644
--- a/hw/exynos4210.c
+++ b/hw/exynos4210.c
@@ -221,6 +221,7 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
     /* Chip-ID and OMR */
     memory_region_init_ram_ptr(&s->chipid_mem, "exynos4210.chipid",
             sizeof(chipid_and_omr), chipid_and_omr);
+    vmstate_register_ram_global(&s->chipid_mem);
     memory_region_set_readonly(&s->chipid_mem, true);
     memory_region_add_subregion(system_mem, EXYNOS4210_CHIPID_ADDR,
                                 &s->chipid_mem);
@@ -228,6 +229,7 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
     /* Internal ROM */
     memory_region_init_ram(&s->irom_mem, "exynos4210.irom",
                            EXYNOS4210_IROM_SIZE);
+    vmstate_register_ram_global(&s->irom_mem);
     memory_region_set_readonly(&s->irom_mem, true);
     memory_region_add_subregion(system_mem, EXYNOS4210_IROM_BASE_ADDR,
                                 &s->irom_mem);
-- 
1.7.5.4

[Qemu-devel] [PATCH 3/3] memory_region_init_ram_ptr: only allow n*TARGET_PAGE_SIZE memory sizes

[Igor Mitsyanko]

Registering memory regions using preallocated memory which size is not a multiple of
target page size will result in inconsistency in QEMU memory system. Do not
allow to do that at all by checking for that case (and asserting) in
memory_region_init_ram_ptr().

Signed-off-by: Igor Mitsyanko <i.mitsyanko@gmail.com>
---
 include/exec/memory.h |    2 +-
 memory.c              |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2322732..87b9292 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -279,7 +279,7 @@ void memory_region_init_ram(MemoryRegion *mr,
  *
  * @mr: the #MemoryRegion to be initialized.
  * @name: the name of the region.
- * @size: size of the region.
+ * @size: size of the region. Must be a multiple of target page size.
  * @ptr: memory to be mapped; must contain at least @size bytes.
  */
 void memory_region_init_ram_ptr(MemoryRegion *mr,
diff --git a/memory.c b/memory.c
index 92a2196..15cb47f 100644
--- a/memory.c
+++ b/memory.c
@@ -949,6 +949,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
                                 uint64_t size,
                                 void *ptr)
 {
+    assert((size & (TARGET_PAGE_SIZE - 1)) == 0);
     memory_region_init(mr, name, size);
     mr->ram = true;
     mr->terminates = true;
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 3/3] memory_region_init_ram_ptr: only allow n*TARGET_PAGE_SIZE memory sizes

[Peter Maydell]

On 10 March 2013 22:21, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
> Registering memory regions using preallocated memory which size is not a multiple of
> target page size will result in inconsistency in QEMU memory system. Do not
> allow to do that at all by checking for that case (and asserting) in
> memory_region_init_ram_ptr().

This is too vague. What exactly is the problem and why can't we
just fix the memory system to correctly handle being passed
small preallocated memory areas?

> --- a/memory.c
> +++ b/memory.c
> @@ -949,6 +949,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>                                  uint64_t size,
>                                  void *ptr)
>  {
> +    assert((size & (TARGET_PAGE_SIZE - 1)) == 0);

This in particular seems like a bad idea, because TARGET_PAGE_SIZE
is a per-CPU thing, and we shouldn't be adding more code to QEMU which
will need to be fixed if/when we ever support multiple CPU types in
a single binary. (Also TARGET_PAGE_SIZE isn't necessarily what you
think it is: for instance on ARM it's actually only 1K even though
the standard ARM setup is 4K pages.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] memory_region_init_ram_ptr: only allow n*TARGET_PAGE_SIZE memory sizes

[Igor Mitsyanko]

On 10.03.2013 18:27, Peter Maydell wrote:
> On 10 March 2013 22:21, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
>> Registering memory regions using preallocated memory which size is not a multiple of
>> target page size will result in inconsistency in QEMU memory system. Do not
>> allow to do that at all by checking for that case (and asserting) in
>> memory_region_init_ram_ptr().
> This is too vague. What exactly is the problem and why can't we
> just fix the memory system to correctly handle being passed
> small preallocated memory areas?

The problem I've personally encountered is the one I described in PATCH 
1. When saving a VM state, QEMU is looping forever in ram_save_block() 
trying to save chipid_and_omr memory region. This is because 
migration_bitmap_find_and_reset_dirty() works on memory blocks of 
TARGET_PAGE_SIZE length.

There could be other places where problem could occur I think. Its not 
really related to an actual TARGET_PAGE_SIZE and its length, what 
important is to be consistent in minimal memory length requirements. For 
example, when we pass size=1 byte to memory_region_init_ram_ptr(), it 
sets MemoryRegion::size to 1 byte, but at the same time, it sets 
corresponding RAMBlock::size to TARGET_PAGE_ALIGN(1). And now we have a 
situation when some parts of QEMU think that it can access the whole 
TARGET_PAGE_SIZE region, while we actually allocated only 1 byte for it.
Same goes for migration, it operates on TARGET_PAGE_SIZE-length data 
blocks only.

What I mean to say is, it looks like QEMU has an implicit assumption 
that RAM length should be a multiple of page size length?

>
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -949,6 +949,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>>                                   uint64_t size,
>>                                   void *ptr)
>>   {
>> +    assert((size & (TARGET_PAGE_SIZE - 1)) == 0);
> This in particular seems like a bad idea, because TARGET_PAGE_SIZE
> is a per-CPU thing, and we shouldn't be adding more code to QEMU which
> will need to be fixed if/when we ever support multiple CPU types in
> a single binary. (Also TARGET_PAGE_SIZE isn't necessarily what you
> think it is: for instance on ARM it's actually only 1K even though
> the standard ARM setup is 4K pages.)
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 0/3] Fix memory migration for exynos 4210 SoC

[Peter Maydell]

On 10 March 2013 14:21, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
> First two patches fix issues in exynos4210 code which were blocking proper memory
> migration.
>
> Third patch makes memory_region_init_ram_ptr assert if memory region size is not a
> multiple of TARGET_PAGE_SIZE.
>
> Igor Mitsyanko (3):
>   hw/exynos4210.c: set chipid_and_omr array size to TARGET_PAGE_SIZE
>   exynos4210.c: register chipid_mem and rom_mem with vmstate
>   memory.c: only allow memory sizes which are a multiple of target page
>     size

I was talking to Paolo about this patchset on IRC, and we came to
the conclusion that for the chipid_and_omr bit the right solution
is not to misuse memory_region_init_ram_ptr() for this, but just
to implement it as a device with an mmio region.

We should probably still look at fixing the bugs with small
ramblocks and/or asserting if you try to create them, though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] Fix memory migration for exynos 4210 SoC

[Paolo Bonzini]

Il 07/05/2013 15:09, Peter Maydell ha scritto:
> On 10 March 2013 14:21, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
>> First two patches fix issues in exynos4210 code which were blocking proper memory
>> migration.
>>
>> Third patch makes memory_region_init_ram_ptr assert if memory region size is not a
>> multiple of TARGET_PAGE_SIZE.
>>
>> Igor Mitsyanko (3):
>>   hw/exynos4210.c: set chipid_and_omr array size to TARGET_PAGE_SIZE
>>   exynos4210.c: register chipid_mem and rom_mem with vmstate
>>   memory.c: only allow memory sizes which are a multiple of target page
>>     size
> 
> I was talking to Paolo about this patchset on IRC, and we came to
> the conclusion that for the chipid_and_omr bit the right solution
> is not to misuse memory_region_init_ram_ptr() for this, but just
> to implement it as a device with an mmio region.

... or at least, just use memory_region_init_ram() and copy the initial
contents to it with address_space_rw.  There are probably some bugs in
small memory_region_init_ram regions too, but some problems you're
seeing are exclusive to memory_region_init_ram_ptr, and the others are
more easily fixed if you concentrate on memory_region_init_ram.

I think memory_region_init_ram_ptr should assert that the size is a
multiple of the page size.  It is a special interface that is meant to
be used for things like mmap-ed files (not coincidentially, pci-assign
and VFIO are the main users).  Besides exynos4210, hw/display/g364fb.c
would also be better off using address_space_map/unmap.

Paolo

> We should probably still look at fixing the bugs with small
> ramblocks and/or asserting if you try to create them, though.
> 
> thanks
> -- PMM
> 

[Qemu-devel] [PATCH 0/3 RESEND] Exynos4210: license and RAM vmstate fixes

[Igor Mitsyanko]

PATCH1 drops "All rights reserved" from all exynos-related file's licenses.

PATCH2 registers RO MemoryRegions for live migration. This is needed because
ram_save_live saves all MemoryRegions no matter if they were registered or not.
This is probably a bug in QEMU because It's not written anywhere that vmstate_register_ram_global()
is mandatory for every created MemoryRegion.

PATCH3 sets chipid_and_omr static array size to TARGET_PAGE_SIZE because memory_region_init_ram_ptr()
implicitly assumes that its size argument is a multiple of TARGET_PAGE_SIZE. This is probably a bug too,
but still..

Igor Mitsyanko (3):
  exynos4210: drop "All rights reserved" line from files license
  exynos4210.c: register chipid_mem and rom_mem with vmstate
  hw/exynos4210.c: set chipid_and_omr array size to TARGET_PAGE_SIZE

 hw/exynos4210.c          |    6 ++++--
 hw/exynos4210.h          |    2 +-
 hw/exynos4210_combiner.c |    3 +--
 hw/exynos4210_fimd.c     |    3 +--
 hw/exynos4210_gic.c      |    3 +--
 hw/exynos4210_mct.c      |    3 +--
 hw/exynos4210_pwm.c      |    3 +--
 hw/exynos4_boards.c      |    2 +-
 8 files changed, 11 insertions(+), 14 deletions(-)

-- 
1.7.4.1

[Qemu-devel] [PATCH 2/3] exynos4210.c: register chipid_mem and rom_mem with vmstate

[Igor Mitsyanko]

Even if we do not register newly created RAM MemoryRegion for migration with
vmstate_register_ram_global() function, ram_save_live() still saves this region
to snapshot file with empty idstr=="". Consequently this results in error during
VM restoration in ram_load().
Register chipid_mem and rom_mem for migration to avoid this problem.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/exynos4210.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/exynos4210.c b/hw/exynos4210.c
index a3a06f7..94d2e41 100644
--- a/hw/exynos4210.c
+++ b/hw/exynos4210.c
@@ -201,6 +201,7 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
     memory_region_init_ram_ptr(&s->chipid_mem, "exynos4210.chipid",
             sizeof(chipid_and_omr), chipid_and_omr);
     memory_region_set_readonly(&s->chipid_mem, true);
+    vmstate_register_ram_global(&s->chipid_mem);
     memory_region_add_subregion(system_mem, EXYNOS4210_CHIPID_ADDR,
                                 &s->chipid_mem);
 
@@ -208,6 +209,7 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
     memory_region_init_ram(&s->irom_mem, "exynos4210.irom",
                            EXYNOS4210_IROM_SIZE);
     memory_region_set_readonly(&s->irom_mem, true);
+    vmstate_register_ram_global(&s->irom_mem);
     memory_region_add_subregion(system_mem, EXYNOS4210_IROM_BASE_ADDR,
                                 &s->irom_mem);
     /* mirror of iROM */
-- 
1.7.4.1