Re: [Xen-devel] [PATCH v7 09/11] vt-d: fix the IOMMU flush issue

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

> From: Xu, Quan
> Sent: Wednesday, June 08, 2016 4:59 PM
> @@ -545,18 +549,42 @@ static int __must_check iommu_flush_all(void)
>  {
>      struct acpi_drhd_unit *drhd;
>      struct iommu *iommu;
> -    int flush_dev_iotlb;
> +    int rc = 0;
> 
>      flush_all_cache();
>      for_each_drhd_unit ( drhd )
>      {
>          iommu = drhd->iommu;
> -        iommu_flush_context_global(iommu, 0);
> -        flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> -        iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> +        /*
> +         * The current logic for rc returns:
> +         *   - positive  invoke iommu_flush_write_buffer to flush cache.
> +         *   - zero      on success.
> +         *   - negative  on failure. Continue to flush IOMMU IOTLB on a
> +         *               best effort basis.
> +         *
> +         * Moreover, IOMMU flush handlers flush_context_qi and flush_iotlb_qi
> +         * (or flush_context_reg and flush_iotlb_reg, deep functions in the
> +         * call trees of iommu_flush_context_global and 
> iommu_flush_iotlb_global)
> +         * are with the same logic to bubble up positive return value.
> +         */
> +        rc = iommu_flush_context_global(iommu, 0);
> +        if ( rc <= 0 )
> +        {
> +            int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> +            int ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> +
> +            ASSERT(ret <= 0);
> +            if ( !rc )
> +                rc = ret;

I'm dubious about the assertion here. Why can't above call
return 1 upon error on earlier flush? I digged back your
earlier reply like:

> Yes, the iommu_flush_iotlb_dsi() can also return 1.
> Look at the call tree, at the beginning of 
> flush_context_qi()/flush_iotlb_qi(), or 
> flush_context_reg()/flush_iotlb_reg()..
> 
> If rc was negative when we call iommu_flush_context_device(), it is 
> impossible to return 1 for iommu_flush_iotlb_dsi().

But I don't think it a good idea of making so much assumptions
about internal implementations of those low level interfaces.
Also flush_context may fail for one specific reason which doesn't 
block flush_iotlb which could get 1 returned when caching mode
is disabled. We'd better have return-1 case correctly handled here.

Thanks
Kevin

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