Commit Message

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(-)

Comments

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

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