Re: [Qemu-devel] [PATCH 11/40] memory: add address_space_valid

[Paolo Bonzini]

Il 07/05/2013 19:40, Peter Maydell ha scritto:
> On 7 May 2013 15:16, Paolo Bonzini <address@hidden> 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 <address@hidden>
>> ---
>>  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
>