From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50552) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UbtLl-00008T-Ri for qemu-devel@nongnu.org; Mon, 13 May 2013 10:03:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UbtLk-0006pv-FV for qemu-devel@nongnu.org; Mon, 13 May 2013 10:03:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:22397) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UbtLk-0006pn-71 for qemu-devel@nongnu.org; Mon, 13 May 2013 10:03:36 -0400 Message-ID: <5190F2AB.9000700@redhat.com> Date: Mon, 13 May 2013 16:03:23 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1367936238-12196-1-git-send-email-pbonzini@redhat.com> <1367936238-12196-12-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 11/40] memory: add address_space_valid 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 19:40, Peter Maydell ha scritto: > On 7 May 2013 15:16, Paolo Bonzini wrote: >> Checking whether an address space is possible in the old-style >> IOMMU implementation, but there is no equivalent in the memory API. > > This sentence appears to be missing a clause ("whether > an address space is valid" ?) The old-style IOMMU lets you check whether an access is valid in a given DMAContext. There is no equivalent for AddressSpace in the memory API, implement it with a lookup of the dispatch tree. >> Implement it with a lookup of the dispatch tree. >> >> Signed-off-by: Paolo Bonzini >> --- >> dma-helpers.c | 5 +++++ >> exec.c | 24 ++++++++++++++++++++++++ >> include/exec/memory.h | 12 ++++++++++++ >> include/sysemu/dma.h | 3 ++- >> 4 files changed, 43 insertions(+), 1 deletions(-) >> >> diff --git a/dma-helpers.c b/dma-helpers.c >> index 272632f..2962b69 100644 >> --- a/dma-helpers.c >> +++ b/dma-helpers.c >> @@ -298,6 +298,11 @@ bool iommu_dma_memory_valid(DMAContext *dma, dma_addr_t addr, dma_addr_t len, >> plen = len; >> } >> >> + if (!address_space_valid(dma->as, paddr, len, >> + dir == DMA_DIRECTION_FROM_DEVICE)) { >> + return false; >> + } >> + >> len -= plen; >> addr += plen; >> } >> diff --git a/exec.c b/exec.c >> index 1dbd956..405de9f 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -2093,6 +2093,30 @@ static void cpu_notify_map_clients(void) >> } >> } >> >> +bool address_space_valid(AddressSpace *as, hwaddr addr, int len, bool is_write) >> +{ >> + AddressSpaceDispatch *d = as->dispatch; >> + MemoryRegionSection *section; >> + int l; >> + hwaddr page; >> + >> + while (len > 0) { >> + page = addr & TARGET_PAGE_MASK; >> + l = (page + TARGET_PAGE_SIZE) - addr; >> + if (l > len) { >> + l = len; >> + } >> + section = phys_page_find(d, addr >> TARGET_PAGE_BITS); >> + if (section->mr == &io_mem_unassigned) { >> + return false; >> + } > > Shouldn't we also be failing attempts to write to read-only > memory regions? Yes. > What about the case where there's a subpage-sized unassigned > region in the middle of the area we want to access? Indeed subpage ranges are not supported yet. I noticed it when reviewing Jan's patch (which, if salvaged would let us implement that too). I'll document the limitation. >> + >> + len -= l; >> + addr += l; >> + } >> + return true; >> +} >> + >> /* Map a physical memory region into a host virtual address. >> * May map a subset of the requested range, given by and returned in *plen. >> * May return NULL if resources needed to perform the mapping are exhausted. >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 489dc73..c38e974 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -857,6 +857,18 @@ void address_space_write(AddressSpace *as, hwaddr addr, >> */ >> void address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int len); >> >> +/* address_space_valid: check for validity of an address space range >> + * >> + * Check whether access to the given address space range is permitted by >> + * any IOMMU regions that are active for the address space. >> + * >> + * @as: #AddressSpace to be accessed >> + * @addr: address within that address space >> + * @len: pointer to length > > Really a pointer? Function prototype suggests not. > >> + * @is_write: indicates the transfer direction >> + */ >> +bool address_space_valid(AddressSpace *as, hwaddr addr, int len, bool is_write); > > The function name suggests that the functionality ought to > be "check whether this AddressSpace is valid" (whatever that > would mean), rather than "check whether this access to > this memory range is permitted in this AddressSpace". I was mimicking dma_memory_valid, but it's not a good example to follow. Paolo > address_space_access_ok() ? > > (Aside: I notice that address_space_{rw,read,write} don't document their > 'len' parameters.) > > thanks > -- PMM >