Re: [Xen-devel] [PATCH v4 01/10] vt-d: fix the IOMMU flush issue

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
> -static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, 
> unsigned int page_count)
> +static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn,
> +                                   unsigned int page_count)

The new name suggests just one page. Please use e.g.
iommu_flush_iotlb_pages() instead.

>  {
> -    __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
> +    iommu_flush_iotlb(d, gfn, 1, page_count);
>  }

But of course the question is whether having this wrapper is useful in
the first place, the more that ...

> @@ -639,7 +646,7 @@ static void dma_pte_clear_one(struct domain *domain, u64 
> addr)
>      iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
>  
>      if ( !this_cpu(iommu_dont_flush_iotlb) )
> -        __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K, 1, 1);
> +        iommu_flush_iotlb(domain, addr >> PAGE_SHIFT_4K, 1, 1);

... it's being open coded here. IOW if you want to retain the
wrapper, please use it here.

> @@ -1391,13 +1399,19 @@ int domain_context_mapping_one(
>      spin_unlock(&iommu->lock);
>  
>      /* Context entry was previously non-present (with domid 0). */
> -    if ( iommu_flush_context_device(iommu, 0, (((u16)bus) << 8) | devfn,
> -                                    DMA_CCMD_MASK_NOBIT, 1) )
> -        iommu_flush_write_buffer(iommu);
> -    else
> +    rc = iommu_flush_context_device(iommu, 0, (((u16)bus) << 8) | devfn,
> +                                    DMA_CCMD_MASK_NOBIT, 1);
> +
> +    if ( !rc )
>      {
>          int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> -        iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> +        rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);

Please take the opportunity and add the missing blank line (between
declaration(s) and statement(s) in cases like this.

> +    }
> +
> +    if ( rc > 0 )

Can iommu_flush_context_device() return a positive value? If so,
the logic is now likely wrong. If not (which is what I assume) I'd
like to suggest adding a respective ASSERT() (even if only to
document the fact). Or alternatively this if() could move into the
immediately preceding one.

Jan


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