| Message ID | 20190729145229.4333-2-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390: stop abusing memory_region_allocate_system_memory() | expand |
* Igor Mammedov (imammedo@redhat.com) wrote: > use qemu_ram_alloc_from_ptr() to create aliased RAMBlock > to the part of original memory region. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > exec.c | 7 ++++--- > memory.c | 5 +++++ > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/exec.c b/exec.c > index 3e78de3b8f..daef0cd54f 100644 > --- a/exec.c > +++ b/exec.c > @@ -2313,7 +2313,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared) > new_block->used_length, > DIRTY_CLIENTS_ALL); > > - if (new_block->host) { > + if (new_block->host && !new_block->mr->alias) { > qemu_ram_setup_dump(new_block->host, new_block->max_length); > qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_HUGEPAGE); > /* MADV_DONTFORK is also needed by KVM in absence of synchronous MMU */ > @@ -2671,7 +2671,8 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset, > > rcu_read_lock(); > block = atomic_rcu_read(&ram_list.mru_block); > - if (block && block->host && host - block->host < block->max_length) { > + if (block && !block->mr->alias && block->host && > + host - block->host < block->max_length) { > goto found; > } > > @@ -2680,7 +2681,7 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset, > if (block->host == NULL) { > continue; > } > - if (host - block->host < block->max_length) { > + if (!block->mr->alias && host - block->host < block->max_length) { > goto found; > } > } > diff --git a/memory.c b/memory.c > index 5d8c9a9234..d710c17a26 100644 > --- a/memory.c > +++ b/memory.c > @@ -1678,6 +1678,11 @@ void memory_region_init_alias(MemoryRegion *mr, > memory_region_init(mr, owner, name, size); > mr->alias = orig; > mr->alias_offset = offset; > + if (orig->ram_block && size) { > + mr->ram_block = qemu_ram_alloc_from_ptr(size, > + orig->ram_block->host + offset, > + mr, &error_fatal); > + } > } Doesn't this cause new memory regions to be created in other existing machines, e.g. x86's mem-smram, or the various PCI vga hacks? Dave > > void memory_region_init_rom_nomigrate(MemoryRegion *mr, > -- > 2.18.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Mon, 29 Jul 2019 18:53:15 +0100 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Igor Mammedov (imammedo@redhat.com) wrote: > > use qemu_ram_alloc_from_ptr() to create aliased RAMBlock > > to the part of original memory region. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > exec.c | 7 ++++--- > > memory.c | 5 +++++ > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index 3e78de3b8f..daef0cd54f 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -2313,7 +2313,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared) > > new_block->used_length, > > DIRTY_CLIENTS_ALL); > > > > - if (new_block->host) { > > + if (new_block->host && !new_block->mr->alias) { > > qemu_ram_setup_dump(new_block->host, new_block->max_length); > > qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_HUGEPAGE); > > /* MADV_DONTFORK is also needed by KVM in absence of synchronous MMU */ > > @@ -2671,7 +2671,8 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset, > > > > rcu_read_lock(); > > block = atomic_rcu_read(&ram_list.mru_block); > > - if (block && block->host && host - block->host < block->max_length) { > > + if (block && !block->mr->alias && block->host && > > + host - block->host < block->max_length) { > > goto found; > > } > > > > @@ -2680,7 +2681,7 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset, > > if (block->host == NULL) { > > continue; > > } > > - if (host - block->host < block->max_length) { > > + if (!block->mr->alias && host - block->host < block->max_length) { > > goto found; > > } > > } > > diff --git a/memory.c b/memory.c > > index 5d8c9a9234..d710c17a26 100644 > > --- a/memory.c > > +++ b/memory.c > > @@ -1678,6 +1678,11 @@ void memory_region_init_alias(MemoryRegion *mr, > > memory_region_init(mr, owner, name, size); > > mr->alias = orig; > > mr->alias_offset = offset; > > + if (orig->ram_block && size) { > > + mr->ram_block = qemu_ram_alloc_from_ptr(size, > > + orig->ram_block->host + offset, > > + mr, &error_fatal); > > + } > > } > > Doesn't this cause new memory regions to be created in other existing > machines, e.g. x86's mem-smram, or the various PCI vga hacks? I'd guess you've meant RAMBlocks instead of memory regions, if that's it then yes, every alias pointing to RAM backed memory region will have RAMBlock that's points to aliased part of aliased memory region. (I didn't not intended to limit it to s390 only as it potentially could be used for x86 initial memory refactoring as well) If it's issue we probably can address that by adding additional API memory_region_alias_make_migratable() and call it on selected aliases, but the less APIs we have the better so I went for reusing exiting vmstate_register_ram_global(). (I admit faking RAMBlock is a hack (albeit working one), but my attempt to teach migration code to accept aliases directly didn't work for me well and was very fragile, I don't remember specifics any more and probably code as well since it was utter failure) PS: this patch is by no means a complete one (I probably cut corners on cleanup path) but should be sufficient to demo idea and see if it's acceptable. > Dave > > > > void memory_region_init_rom_nomigrate(MemoryRegion *mr, > > -- > > 2.18.1 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 30/07/19 15:25, Igor Mammedov wrote: > I'd guess you've meant RAMBlocks instead of memory regions, if that's it > then yes, every alias pointing to RAM backed memory region will have > RAMBlock that's points to aliased part of aliased memory region. The question is just, does it break migration from old QEMU to new QEMU on x86 (which has plenty of RAM-backed aliases)? If not, explain that in the commit message or in a code comment. Paolo
On Tue, 30 Jul 2019 15:34:54 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 30/07/19 15:25, Igor Mammedov wrote: > > I'd guess you've meant RAMBlocks instead of memory regions, if that's it > > then yes, every alias pointing to RAM backed memory region will have > > RAMBlock that's points to aliased part of aliased memory region. > > The question is just, does it break migration from old QEMU to new QEMU > on x86 (which has plenty of RAM-backed aliases)? If not, explain that > in the commit message or in a code comment. pc/q35 old<->new new<->old ping pong tests worked fine (qemu with default devices). My understanding was that one needs to call vmstate_register_ram() to make memory region migratable, which marks specific RAMBlock as migratable. So just assigning new RAMBlock to mr->ram_block shouldn't affect migration, unless vmstate_register_ram() is called on alias. I'll add it to commit message. > > Paolo >
* Igor Mammedov (imammedo@redhat.com) wrote: > On Tue, 30 Jul 2019 15:34:54 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 30/07/19 15:25, Igor Mammedov wrote: > > > I'd guess you've meant RAMBlocks instead of memory regions, if that's it > > > then yes, every alias pointing to RAM backed memory region will have > > > RAMBlock that's points to aliased part of aliased memory region. > > > > The question is just, does it break migration from old QEMU to new QEMU > > on x86 (which has plenty of RAM-backed aliases)? If not, explain that > > in the commit message or in a code comment. > > pc/q35 old<->new new<->old ping pong tests worked fine (qemu with default devices). > > My understanding was that one needs to call vmstate_register_ram() > to make memory region migratable, which marks specific RAMBlock > as migratable. So just assigning new RAMBlock to mr->ram_block shouldn't > affect migration, unless vmstate_register_ram() is called on alias. You're right; that's actually relatively new and I forget about it. From Cedric's b895de50271 in 3.0, before that we migrated every RAMBlock. So yes, I think you should be OK. Dave > I'll add it to commit message. > > > > > Paolo > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/exec.c b/exec.c index 3e78de3b8f..daef0cd54f 100644 --- a/exec.c +++ b/exec.c @@ -2313,7 +2313,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared) new_block->used_length, DIRTY_CLIENTS_ALL); - if (new_block->host) { + if (new_block->host && !new_block->mr->alias) { qemu_ram_setup_dump(new_block->host, new_block->max_length); qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_HUGEPAGE); /* MADV_DONTFORK is also needed by KVM in absence of synchronous MMU */ @@ -2671,7 +2671,8 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset, rcu_read_lock(); block = atomic_rcu_read(&ram_list.mru_block); - if (block && block->host && host - block->host < block->max_length) { + if (block && !block->mr->alias && block->host && + host - block->host < block->max_length) { goto found; } @@ -2680,7 +2681,7 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset, if (block->host == NULL) { continue; } - if (host - block->host < block->max_length) { + if (!block->mr->alias && host - block->host < block->max_length) { goto found; } } diff --git a/memory.c b/memory.c index 5d8c9a9234..d710c17a26 100644 --- a/memory.c +++ b/memory.c @@ -1678,6 +1678,11 @@ void memory_region_init_alias(MemoryRegion *mr, memory_region_init(mr, owner, name, size); mr->alias = orig; mr->alias_offset = offset; + if (orig->ram_block && size) { + mr->ram_block = qemu_ram_alloc_from_ptr(size, + orig->ram_block->host + offset, + mr, &error_fatal); + } } void memory_region_init_rom_nomigrate(MemoryRegion *mr,
use qemu_ram_alloc_from_ptr() to create aliased RAMBlock to the part of original memory region. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- exec.c | 7 ++++--- memory.c | 5 +++++ 2 files changed, 9 insertions(+), 3 deletions(-)