From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53068) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpHvu-0006uN-9L for qemu-devel@nongnu.org; Wed, 19 Jun 2013 08:56:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UpHvt-0001LB-1f for qemu-devel@nongnu.org; Wed, 19 Jun 2013 08:56:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26439) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpHvs-0001L4-PT for qemu-devel@nongnu.org; Wed, 19 Jun 2013 08:56:16 -0400 Message-ID: <51C1AA62.6080400@redhat.com> Date: Wed, 19 Jun 2013 14:56:02 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1371642264-17704-1-git-send-email-armbru@redhat.com> <1371642264-17704-6-git-send-email-armbru@redhat.com> In-Reply-To: <1371642264-17704-6-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/8] exec: Drop incorrect & dead S390 code in qemu_ram_remap() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: peter.maydell@linaro.org, stefano.stabellini@eu.citrix.com, mtosatti@redhat.com, qemu-devel@nongnu.org, agraf@suse.de, borntraeger@de.ibm.com, afaerber@suse.de, rth@twiddle.net 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 > --- > 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: " >