On 7 May 2013 15:16, Paolo Bonzini <pbonz...@redhat.com> 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 <pbonz...@redhat.com> > --- > 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