From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33272) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qa9Ne-0006ra-Rd for qemu-devel@nongnu.org; Fri, 24 Jun 2011 12:37:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qa9Nd-0004rB-5o for qemu-devel@nongnu.org; Fri, 24 Jun 2011 12:37:18 -0400 Received: from mail-pv0-f173.google.com ([74.125.83.173]:34711) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qa9Nc-0004r5-O9 for qemu-devel@nongnu.org; Fri, 24 Jun 2011 12:37:16 -0400 Received: by pvg3 with SMTP id 3so1898338pvg.4 for ; Fri, 24 Jun 2011 09:37:15 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1308913718-7201-1-git-send-email-stefano.stabellini@eu.citrix.com> References: <1308913718-7201-1-git-send-email-stefano.stabellini@eu.citrix.com> Date: Fri, 24 Jun 2011 17:37:15 +0100 Message-ID: From: Peter Maydell Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH] qemu_ram_ptr_length: take ram_addr_t as arguments List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: stefano.stabellini@eu.citrix.com Cc: xen-devel@lists.xensource.com, qemu-devel@nongnu.org, agraf@suse.de On 24 June 2011 12:08, wrote: > From: Stefano Stabellini > > qemu_ram_ptr_length should take ram_addr_t as argument rather than > target_phys_addr_t because is doing comparisons with RAMBlock addresses. > > cpu_physical_memory_map should create a ram_addr_t address to pass to > qemu_ram_ptr_length from PhysPageDesc phys_offset. > > Remove code after abort() in qemu_ram_ptr_length. This does fix vexpress. However I think we're still doing the wrong thing if the bounce buffer is already in use and addr points at an IO page. In the old code, we would break out of the loop on the if (done || bounce.buffer) condition, set *plen to 0 [because done==0 since this is the first page] and return. Now we break out of the loop but will fall into the call to qemu_ram_ptr_length() with a bogus addr1 and probably abort(). You probably want to only call qemu_ram_ptr_length() if (todo). (I don't know if anybody ever calls this routine with a zero input length, but that would handle that case too.) It would also be better to either (a) not initialise addr1, if the compiler is smart enough to know it can't get to the use without it being initialised or (b) initialise it to an obviously bogus value if we have to do so to shut the compiler up. (Also 'addr1' is not a fantastic variable name :-)) -- PMM