Re: [Qemu-devel] [PATCH 5/8] exec: Drop incorrect & dead S390 code in qemu_ram_remap()

[Paolo Bonzini <pbonzini@redhat.com>]

Il 19/06/2013 13:44, Markus Armbruster ha scritto:
> Old S390 KVM wants guest RAM mapped in a peculiar way.  Commit 6b02494
> implemented that.
> 
> When qemu_ram_remap() got added in commit cd19cfa, its code carefully
> mimicked the allocation code: peculiar way if defined(TARGET_S390X) &&
> defined(CONFIG_KVM), else normal way.
> 
> For new S390 KVM, we actually want the normal way.  Commit fdec991
> changed qemu_ram_alloc_from_ptr() accordingly, but forgot to update
> qemu_ram_remap().  If qemu_ram_alloc_from_ptr() maps RAM the normal
> way, but qemu_ram_remap() remaps it the peculiar way, remapping
> changes protection and flags, which it shouldn't.
> 
> Fortunately, this can't happen, as we never remap on S390.
> 
> Replace the incorrect code with an assertion.
> 
> Thanks to Christian Borntraeger for help with assessing the bug's
> (non-)impact.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  exec.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index c45eb33..a0f18fe 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1229,15 +1229,16 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
>                      area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
>                                  flags, block->fd, offset);
>                  } else {
> -#if defined(TARGET_S390X) && defined(CONFIG_KVM)
> -                    flags |= MAP_SHARED | MAP_ANONYMOUS;
> -                    area = mmap(vaddr, length, PROT_EXEC|PROT_READ|PROT_WRITE,
> -                                flags, -1, 0);
> -#else
> +                    /*
> +                     * Remap needs to match alloc.  Accelerators that
> +                     * set phys_mem_alloc never remap.  If they did,
> +                     * we'd need a remap hook here.
> +                     */
> +                    assert(!phys_mem_alloc);

Probably "assert(phys_mem_alloc == qemu_anon_ram_alloc)"?

Otherwise all looks fine.

Paolo

>                      flags |= MAP_PRIVATE | MAP_ANONYMOUS;
>                      area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
>                                  flags, -1, 0);
> -#endif
>                  }
>                  if (area != vaddr) {
>                      fprintf(stderr, "Could not remap addr: "
>