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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Xu, Quan
> Sent: Friday, April 29, 2016 5:25 PM
> 
> The propagation value from IOMMU flush interfaces may be positive, which
> indicates callers need to flush cache, not one of faliures.
> 
> when the propagation value is positive, this patch fixes this flush issue
> as follows:
>   - call iommu_flush_write_buffer() to flush cache.
>   - return zero.
> 
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> 
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> CC: Feng Wu <feng.wu@xxxxxxxxx>
> CC: Keir Fraser <keir@xxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
>  xen/drivers/passthrough/vtd/iommu.c | 94
> +++++++++++++++++++++++++------------
>  xen/include/asm-x86/iommu.h         |  2 +-
>  2 files changed, 64 insertions(+), 32 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 5ad25dc..6e2e43a 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -558,14 +558,16 @@ static void iommu_flush_all(void)
>      }
>  }
> 
> -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> -        int dma_old_pte_present, unsigned int page_count)
> +static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
> +                             bool_t dma_old_pte_present,
> +                             unsigned int page_count)
>  {
>      struct hvm_iommu *hd = domain_hvm_iommu(d);
>      struct acpi_drhd_unit *drhd;
>      struct iommu *iommu;
>      int flush_dev_iotlb;
>      int iommu_domid;
> +    int rc, ret = 0;
> 
>      /*
>       * No need pcideves_lock here because we have flush
> @@ -584,29 +586,35 @@ static void __intel_iommu_iotlb_flush(struct domain *d,
> unsigned long gfn,
>              continue;
> 
>          if ( page_count != 1 || gfn == INVALID_GFN )
> -        {
> -            if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
> -                        0, flush_dev_iotlb) )
> -                iommu_flush_write_buffer(iommu);
> -        }
> +            rc = iommu_flush_iotlb_dsi(iommu, iommu_domid,
> +                                       0, flush_dev_iotlb);
>          else
> +            rc = iommu_flush_iotlb_psi(iommu, iommu_domid,
> +                                       (paddr_t)gfn << PAGE_SHIFT_4K,
> +                                       PAGE_ORDER_4K,
> +                                       !dma_old_pte_present,
> +                                       flush_dev_iotlb);
> +
> +        if ( rc > 0 )
>          {
> -            if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
> -                        (paddr_t)gfn << PAGE_SHIFT_4K, PAGE_ORDER_4K,
> -                        !dma_old_pte_present, flush_dev_iotlb) )
> -                iommu_flush_write_buffer(iommu);
> +            iommu_flush_write_buffer(iommu);
> +            ret = 0;

You don't need 'ret' here. Just change 'rc' to 0.

>          }
> +        else if ( rc < 0 )
> +            ret = rc;

Then this change is not required

>      }
> +
> +    return ret;

Then return 'rc' instead.

>  }
> 
>  static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, 
> unsigned int
> page_count)

could we remove "Intel_" prefix completely? You can rename this
one to iommu_flush_iotlb_page...

>  {
> -    __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
> +    iommu_flush_iotlb(d, gfn, 1, page_count);
>  }
> 
>  static void intel_iommu_iotlb_flush_all(struct domain *d)

and this one just iommu_flush_iotlb_all

>  {
> -    __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0);
> +    iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
>  }
> 


Thanks
Kevin

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