From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37041) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZlsM-0003bA-Mr for qemu-devel@nongnu.org; Tue, 07 May 2013 13:40:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UZlsK-0006gp-J8 for qemu-devel@nongnu.org; Tue, 07 May 2013 13:40:30 -0400 Received: from mail-la0-x22d.google.com ([2a00:1450:4010:c03::22d]:50518) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZlsK-0006gS-AN for qemu-devel@nongnu.org; Tue, 07 May 2013 13:40:28 -0400 Received: by mail-la0-f45.google.com with SMTP id fp12so858422lab.32 for ; Tue, 07 May 2013 10:40:27 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1367936238-12196-12-git-send-email-pbonzini@redhat.com> References: <1367936238-12196-1-git-send-email-pbonzini@redhat.com> <1367936238-12196-12-git-send-email-pbonzini@redhat.com> From: Peter Maydell Date: Tue, 7 May 2013 18:40:07 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 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: Paolo Bonzini Cc: aik@ozlabs.ru, jan.kiszka@siemens.com, qemu-devel@nongnu.org, qemulist@gmail.com, stefanha@redhat.com, david@gibson.dropbear.id.au 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" ?) > 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? What about the case where there's a subpage-sized unassigned region in the middle of the area we want to access? > + > + 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". address_space_access_ok() ? (Aside: I notice that address_space_{rw,read,write} don't document their 'len' parameters.) thanks -- PMM