Re: [PATCH v2 13/13] [RFC] iommu: AMD-Vi: Squash map_pages/unmap_pages with map_page/unmap_page

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

Hi, all.

Any comments?

On Tue, Jul 25, 2017 at 8:26 PM, Oleksandr Tyshchenko
<olekstysh@gmail.com> wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Reduce the scope of the TODO by squashing single-page stuff with
> multi-page one. Next target is to use large pages whenever possible
> in the case that hardware supports them.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> ---
>    Changes in v1:
>       -
>
>    Changes in v2:
>       -
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  xen/drivers/passthrough/amd/iommu_map.c | 250 ++++++++++++++++----------------
>  1 file changed, 121 insertions(+), 129 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
> index ea3a728..22d0cc6 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -631,188 +631,180 @@ static int update_paging_mode(struct domain *d, unsigned long gfn)
>      return 0;
>  }
>
> -static int __must_check amd_iommu_map_page(struct domain *d, unsigned long gfn,
> -                                           unsigned long mfn,
> -                                           unsigned int flags)
> +/*
> + * TODO: Optimize by using large pages whenever possible in the case
> + * that hardware supports them.
> + */
> +int __must_check amd_iommu_map_pages(struct domain *d, unsigned long gfn,
> +                                     unsigned long mfn,
> +                                     unsigned int order,
> +                                     unsigned int flags)
>  {
> -    bool_t need_flush = 0;
>      struct domain_iommu *hd = dom_iommu(d);
>      int rc;
> -    unsigned long pt_mfn[7];
> -    unsigned int merge_level;
> +    unsigned long orig_gfn = gfn;
> +    unsigned long i;
>
>      if ( iommu_use_hap_pt(d) )
>          return 0;
>
> -    memset(pt_mfn, 0, sizeof(pt_mfn));
> -
>      spin_lock(&hd->arch.mapping_lock);
> -
>      rc = amd_iommu_alloc_root(hd);
> +    spin_unlock(&hd->arch.mapping_lock);
>      if ( rc )
>      {
> -        spin_unlock(&hd->arch.mapping_lock);
>          AMD_IOMMU_DEBUG("Root table alloc failed, gfn = %lx\n", gfn);
>          domain_crash(d);
>          return rc;
>      }
>
> -    /* Since HVM domain is initialized with 2 level IO page table,
> -     * we might need a deeper page table for lager gfn now */
> -    if ( is_hvm_domain(d) )
> +    for ( i = 0; i < (1UL << order); i++, gfn++, mfn++ )
>      {
> -        if ( update_paging_mode(d, gfn) )
> +        bool_t need_flush = 0;
> +        unsigned long pt_mfn[7];
> +        unsigned int merge_level;
> +
> +        memset(pt_mfn, 0, sizeof(pt_mfn));
> +
> +        spin_lock(&hd->arch.mapping_lock);
> +
> +        /* Since HVM domain is initialized with 2 level IO page table,
> +         * we might need a deeper page table for lager gfn now */
> +        if ( is_hvm_domain(d) )
> +        {
> +            if ( update_paging_mode(d, gfn) )
> +            {
> +                spin_unlock(&hd->arch.mapping_lock);
> +                AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
> +                domain_crash(d);
> +                rc = -EFAULT;
> +                goto err;
> +            }
> +        }
> +
> +        if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
>          {
>              spin_unlock(&hd->arch.mapping_lock);
> -            AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
> +            AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
>              domain_crash(d);
> -            return -EFAULT;
> +            rc = -EFAULT;
> +            goto err;
>          }
> -    }
>
> -    if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
> -    {
> -        spin_unlock(&hd->arch.mapping_lock);
> -        AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
> -        domain_crash(d);
> -        return -EFAULT;
> -    }
> +        /* Install 4k mapping first */
> +        need_flush = set_iommu_pte_present(pt_mfn[1], gfn, mfn,
> +                                           IOMMU_PAGING_MODE_LEVEL_1,
> +                                           !!(flags & IOMMUF_writable),
> +                                           !!(flags & IOMMUF_readable));
>
> -    /* Install 4k mapping first */
> -    need_flush = set_iommu_pte_present(pt_mfn[1], gfn, mfn,
> -                                       IOMMU_PAGING_MODE_LEVEL_1,
> -                                       !!(flags & IOMMUF_writable),
> -                                       !!(flags & IOMMUF_readable));
> +        /* Do not increase pde count if io mapping has not been changed */
> +        if ( !need_flush )
> +        {
> +            spin_unlock(&hd->arch.mapping_lock);
> +            continue;
> +        }
>
> -    /* Do not increase pde count if io mapping has not been changed */
> -    if ( !need_flush )
> -        goto out;
> +        /* 4K mapping for PV guests never changes,
> +         * no need to flush if we trust non-present bits */
> +        if ( is_hvm_domain(d) )
> +            amd_iommu_flush_pages(d, gfn, 0);
>
> -    /* 4K mapping for PV guests never changes,
> -     * no need to flush if we trust non-present bits */
> -    if ( is_hvm_domain(d) )
> -        amd_iommu_flush_pages(d, gfn, 0);
> -
> -    for ( merge_level = IOMMU_PAGING_MODE_LEVEL_2;
> -          merge_level <= hd->arch.paging_mode; merge_level++ )
> -    {
> -        if ( pt_mfn[merge_level] == 0 )
> -            break;
> -        if ( !iommu_update_pde_count(d, pt_mfn[merge_level],
> -                                     gfn, mfn, merge_level) )
> -            break;
> -
> -        if ( iommu_merge_pages(d, pt_mfn[merge_level], gfn,
> -                               flags, merge_level) )
> +        for ( merge_level = IOMMU_PAGING_MODE_LEVEL_2;
> +              merge_level <= hd->arch.paging_mode; merge_level++ )
>          {
> -            spin_unlock(&hd->arch.mapping_lock);
> -            AMD_IOMMU_DEBUG("Merge iommu page failed at level %d, "
> -                            "gfn = %lx mfn = %lx\n", merge_level, gfn, mfn);
> -            domain_crash(d);
> -            return -EFAULT;
> +            if ( pt_mfn[merge_level] == 0 )
> +                break;
> +            if ( !iommu_update_pde_count(d, pt_mfn[merge_level],
> +                                         gfn, mfn, merge_level) )
> +                break;
> +
> +            if ( iommu_merge_pages(d, pt_mfn[merge_level], gfn,
> +                                   flags, merge_level) )
> +            {
> +                spin_unlock(&hd->arch.mapping_lock);
> +                AMD_IOMMU_DEBUG("Merge iommu page failed at level %d, "
> +                                "gfn = %lx mfn = %lx\n", merge_level, gfn, mfn);
> +                domain_crash(d);
> +                rc = -EFAULT;
> +                goto err;
> +            }
> +
> +            /* Deallocate lower level page table */
> +            free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
>          }
>
> -        /* Deallocate lower level page table */
> -        free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
> +        spin_unlock(&hd->arch.mapping_lock);
>      }
>
> -out:
> -    spin_unlock(&hd->arch.mapping_lock);
>      return 0;
> +
> +err:
> +    while ( i-- )
> +        /* If statement to satisfy __must_check. */
> +        if ( amd_iommu_unmap_pages(d, orig_gfn + i, 0) )
> +            continue;
> +
> +    return rc;
>  }
>
> -static int __must_check amd_iommu_unmap_page(struct domain *d,
> -                                             unsigned long gfn)
> +int __must_check amd_iommu_unmap_pages(struct domain *d,
> +                                       unsigned long gfn,
> +                                       unsigned int order)
>  {
> -    unsigned long pt_mfn[7];
>      struct domain_iommu *hd = dom_iommu(d);
> +    int rt = 0;
> +    unsigned long i;
>
>      if ( iommu_use_hap_pt(d) )
>          return 0;
>
> -    memset(pt_mfn, 0, sizeof(pt_mfn));
> -
> -    spin_lock(&hd->arch.mapping_lock);
> -
>      if ( !hd->arch.root_table )
> -    {
> -        spin_unlock(&hd->arch.mapping_lock);
>          return 0;
> -    }
>
> -    /* Since HVM domain is initialized with 2 level IO page table,
> -     * we might need a deeper page table for lager gfn now */
> -    if ( is_hvm_domain(d) )
> +    for ( i = 0; i < (1UL << order); i++, gfn++ )
>      {
> -        int rc = update_paging_mode(d, gfn);
> +        unsigned long pt_mfn[7];
>
> -        if ( rc )
> -        {
> -            spin_unlock(&hd->arch.mapping_lock);
> -            AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
> -            if ( rc != -EADDRNOTAVAIL )
> -                domain_crash(d);
> -            return rc;
> -        }
> -    }
> +        memset(pt_mfn, 0, sizeof(pt_mfn));
>
> -    if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
> -    {
> -        spin_unlock(&hd->arch.mapping_lock);
> -        AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
> -        domain_crash(d);
> -        return -EFAULT;
> -    }
> -
> -    /* mark PTE as 'page not present' */
> -    clear_iommu_pte_present(pt_mfn[1], gfn);
> -    spin_unlock(&hd->arch.mapping_lock);
> +        spin_lock(&hd->arch.mapping_lock);
>
> -    amd_iommu_flush_pages(d, gfn, 0);
> -
> -    return 0;
> -}
> -
> -/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
> -int __must_check amd_iommu_map_pages(struct domain *d, unsigned long gfn,
> -                                     unsigned long mfn, unsigned int order,
> -                                     unsigned int flags)
> -{
> -    unsigned long i;
> -    int rc = 0;
> -
> -    for ( i = 0; i < (1UL << order); i++ )
> -    {
> -        rc = amd_iommu_map_page(d, gfn + i, mfn + i, flags);
> -        if ( unlikely(rc) )
> +        /* Since HVM domain is initialized with 2 level IO page table,
> +         * we might need a deeper page table for lager gfn now */
> +        if ( is_hvm_domain(d) )
>          {
> -            while ( i-- )
> -                /* If statement to satisfy __must_check. */
> -                if ( amd_iommu_unmap_page(d, gfn + i) )
> -                    continue;
> +            int rc = update_paging_mode(d, gfn);
>
> -            break;
> +            if ( rc )
> +            {
> +                spin_unlock(&hd->arch.mapping_lock);
> +                AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
> +                if ( rc != -EADDRNOTAVAIL )
> +                    domain_crash(d);
> +                if ( !rt )
> +                    rt = rc;
> +                continue;
> +            }
>          }
> -    }
> -
> -    return rc;
> -}
>
> -int __must_check amd_iommu_unmap_pages(struct domain *d, unsigned long gfn,
> -                                       unsigned int order)
> -{
> -    unsigned long i;
> -    int rc = 0;
> +        if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
> +        {
> +            spin_unlock(&hd->arch.mapping_lock);
> +            AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
> +            domain_crash(d);
> +            if ( !rt )
> +                rt = -EFAULT;
> +            continue;
> +        }
>
> -    for ( i = 0; i < (1UL << order); i++ )
> -    {
> -        int ret = amd_iommu_unmap_page(d, gfn + i);
> +        /* mark PTE as 'page not present' */
> +        clear_iommu_pte_present(pt_mfn[1], gfn);
> +        spin_unlock(&hd->arch.mapping_lock);
>
> -        if ( !rc )
> -            rc = ret;
> +        amd_iommu_flush_pages(d, gfn, 0);
>      }
>
> -    return rc;
> +    return rt;
>  }
>
>  int amd_iommu_reserve_domain_unity_map(struct domain *domain,
> @@ -831,7 +823,7 @@ int amd_iommu_reserve_domain_unity_map(struct domain *domain,
>      gfn = phys_addr >> PAGE_SHIFT;
>      for ( i = 0; i < npages; i++ )
>      {
> -        rt = amd_iommu_map_page(domain, gfn +i, gfn +i, flags);
> +        rt = amd_iommu_map_pages(domain, gfn +i, gfn +i, flags, 0);
>          if ( rt != 0 )
>              return rt;
>      }
> --
> 2.7.4
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel