Patchwork [3/3] memory_region_init_ram_ptr: only allow n*TARGET_PAGE_SIZE memory sizes

login
register
mail settings
Submitter Igor Mitsyanko
Date March 10, 2013, 2:21 p.m.
Message ID <1362925309-3852-4-git-send-email-i.mitsyanko@gmail.com>
Download mbox | patch
Permalink /patch/226448/
State New
Headers show

Comments

Igor Mitsyanko - March 10, 2013, 2:21 p.m.
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(-)
Peter Maydell - March 10, 2013, 2:27 p.m.
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
Igor Mitsyanko - March 10, 2013, 6:39 p.m.
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

Patch

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;