Il 07/05/2013 19:40, Peter Maydell ha scritto:
> 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" ?)
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 <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?
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
>
