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

[Peter Maydell <peter.maydell@linaro.org>]

On 7 May 2013 15:16, Paolo Bonzini <pbonzini@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 <pbonzini@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