Re: [Xen-devel] [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
> When IOMMU mapping is failed, we issue a best effort rollback, stopping
> IOMMU mapping, unmapping the previous IOMMU maps and then reporting the
> error up to the call trees. When rollback is not feasible (in early
> initialization phase or trade-off of complexity) for the hardware domain,
> we do things on a best effort basis, only throwing out an error message.
> 
> IOMMU unmapping should perhaps continue despite an error, in an attempt
> to do best effort cleanup.
> 
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> 
> CC: Keir Fraser <keir@xxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> ---

Somewhere here I continue to miss a summary on what has changed
compared to the previous version. For review especially of larger
patches (where preferably one wouldn't want to re-review the entire
thing) this is more than just a nice-to-have.

> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long 
> gfn, mfn_t mfn,
>      rc = atomic_write_ept_entry(ept_entry, new_entry, target);
>      if ( unlikely(rc) )
>          old_entry.epte = 0;
> -    else if ( p2mt != p2m_invalid &&
> -              (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
> -        /* Track the highest gfn for which we have ever had a valid mapping 
> */
> -        p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
> +    else
> +    {
> +        entry_written = 1;
> +
> +        if ( p2mt != p2m_invalid &&
> +             (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
> +            /* Track the highest gfn for which we have ever had a valid 
> mapping */
> +            p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
> +    }
>  
>  out:
>      if ( needs_sync )
>          ept_sync_domain(p2m);
>  
>      /* For host p2m, may need to change VT-d page table.*/
> -    if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> +    if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
>           need_modify_vtd_table )
>      {

I'd prefer this conditional to remain untouched, but I'll leave the
decision to the maintainers of the file.

> @@ -831,10 +837,28 @@ out:
>          {
>              if ( iommu_flags )
>                  for ( i = 0; i < (1 << order); i++ )
> -                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
> +                {
> +                    ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, 
> iommu_flags);
> +
> +                    if ( unlikely(ret) )
> +                    {
> +                        while ( i-- )
> +                            iommu_unmap_page(p2m->domain, gfn + i);

Loops like this are, btw., good examples of the log spam issue I've
been pointing out on an earlier patch of this series.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long 
> gfn, unsigned long mfn,
>      mfn_t mfn_return;
>      p2m_type_t t;
>      p2m_access_t a;
> +    int rc = 0, ret;
>  
>      if ( !paging_mode_translate(p2m->domain) )
>      {
>          if ( need_iommu(p2m->domain) )
>              for ( i = 0; i < (1 << page_order); i++ )
> -                iommu_unmap_page(p2m->domain, mfn + i);
> -        return 0;
> +            {
> +               ret = iommu_unmap_page(p2m->domain, mfn + i);
> +
> +               if ( !rc )
> +                   rc = ret;
> +            }
> +
> +        return rc;
>      }

In code like this, btw., restricting the scope of "ret" to the innermost
block would help future readers see immediately that the value of
"ret" is of no further interest outside of that block.

Having reached the end of the patch, I'm missing the __must_check
additions that you said you would do in this new iteration. Is there
any reason for their absence? Did I overlook something?

Jan

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