From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57492) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UeNXM-0005Pd-1F for qemu-devel@nongnu.org; Mon, 20 May 2013 06:41:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UeNXH-0003Dk-9a for qemu-devel@nongnu.org; Mon, 20 May 2013 06:41:51 -0400 Received: from mail-yh0-x232.google.com ([2607:f8b0:4002:c01::232]:65419) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UeNXH-0003Df-4G for qemu-devel@nongnu.org; Mon, 20 May 2013 06:41:47 -0400 Received: by mail-yh0-f50.google.com with SMTP id z20so1572932yhz.23 for ; Mon, 20 May 2013 03:41:46 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <5199FDE0.30200@redhat.com> Date: Mon, 20 May 2013 12:41:36 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1367936238-12196-1-git-send-email-pbonzini@redhat.com> <1367936238-12196-13-git-send-email-pbonzini@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 12/40] memory: add address_space_translate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: aik@ozlabs.ru, jan.kiszka@siemens.com, qemu-devel@nongnu.org, qemulist@gmail.com, stefanha@redhat.com, david@gibson.dropbear.id.au Il 07/05/2013 20:08, Peter Maydell ha scritto: >> > >> > - section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS); >> > + section = address_space_translate(&address_space_memory, addr, &addr, &l, >> > + false); > I find it a little confusing reusing 'addr' for the returned > offset from address_space_translate(); maybe using a different > variable would be clearer? (don't feel very strongly about it) True, on the other hand this matches usage before this patch. - addr = memory_region_section_addr(section, addr); >> > + if (l < 4) { >> > + return -1; >> > + } >> > >> > if (!(memory_region_is_ram(section->mr) || >> > memory_region_is_romd(section->mr))) { >> > /* I/O case */ >> > - addr = memory_region_section_addr(section, addr); >> > val = io_mem_read(section->mr, addr, 4); >> > #if defined(TARGET_WORDS_BIGENDIAN) >> > if (endian == DEVICE_LITTLE_ENDIAN) { >> > @@ -2249,7 +2240,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr, >> > /* RAM case */ >> > ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr) >> > & TARGET_PAGE_MASK) >> > - + memory_region_section_addr(section, addr)); >> > + + addr); >> >> - section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS); >> + section = address_space_translate(&address_space_memory, addr, &addr, &l, >> + false); >> + if (l < 2) { >> + return -1; >> + } > > This kind of "let's just return -1 if the read failed" kind > of bothers me, because "memory access aborted" is really > completely different from "memory access succeeded and > returned -1". But there are a lot of APIs which we'd need > to fix to properly communicate the problem back to the > caller, so let it slide for now. (What used to happen > in the old code in this case?) It would call unassigned_mem_read and return 0. I'll change this to call unassigned_mem_read, which also requires fixing the "double use" of addr that you pointed out above. Paolo