Re: [Qemu-devel] [PATCH 2/3] exec: simplify address_space_get_iotlb_entry

[Michael S. Tsirkin]

On Fri, Jun 02, 2017 at 07:50:53PM +0800, Peter Xu wrote:
> This patch let address_space_get_iotlb_entry() to use the newly
> introduced page_mask parameter in address_space_do_translate(). Then we
> will be sure the IOTLB can be aligned to page mask, also we should
> nicely support huge pages now when introducing a764040.
> 
> Fixes: a764040 ("exec: abstract address_space_do_translate()")
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  exec.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 63a3ff0..1f86253 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -544,14 +544,14 @@ IOMMUTLBEntry 
> address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
>                                              bool is_write)
>  {
>      MemoryRegionSection section;
> -    hwaddr xlat, plen;
> +    hwaddr xlat, page_mask;
>  
> -    /* Try to get maximum page mask during translation. */
> -    plen = (hwaddr)-1;
> -
> -    /* This can never be MMIO. */
> -    section = address_space_do_translate(as, addr, &xlat, &plen,
> -                                         NULL, is_write, false);
> +    /*
> +     * This can never be MMIO, and we don't really care about plen,
> +     * but page mask.
> +     */
> +    section = address_space_do_translate(as, addr, &xlat, NULL,
> +                                         &page_mask, is_write, false);
>  
>      /* Illegal translation */
>      if (section.mr == &io_mem_unassigned) {


Can we just use section.size - xlat here?

> @@ -562,20 +562,11 @@ IOMMUTLBEntry 
> address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
>      xlat += section.offset_within_address_space -
>          section.offset_within_region;
>  
> -    if (plen == (hwaddr)-1) {
> -        /* If not specified during translation, use default mask */
> -        plen = TARGET_PAGE_MASK;
> -    } else {
> -        /* Make it a valid page mask */
> -        assert(plen);
> -        plen = pow2floor(plen) - 1;
> -    }
> -
>      return (IOMMUTLBEntry) {
>          .target_as = section.address_space,
> -        .iova = addr & ~plen,
> -        .translated_addr = xlat & ~plen,
> -        .addr_mask = plen,
> +        .iova = addr & ~page_mask,
> +        .translated_addr = xlat & ~page_mask,
> +        .addr_mask = page_mask,
>          /* IOTLBs are for DMAs, and DMA only allows on RAMs. */

BTW this comment is pretty confusing. What does it mean?

>          .perm = IOMMU_RW,
>      };

Looks like we should change IOMMUTLBEntry to pass size and not mask -
then we could simply pass info from section as is. iova would be
addr - xlat.

> -- 
> 2.7.4