Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue

From: "Jan Beulich" <JBeulich@suse.com>
To: Quan Xu <quan.xu@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	dario.faggioli@citrix.com, xen-devel@lists.xen.org,
	Feng Wu <feng.wu@intel.com>
Subject: Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
Date: Mon, 25 Apr 2016 03:22:06 -0600	[thread overview]
Message-ID: <571DFDDE02000078000E5339@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1460988011-17758-2-git-send-email-quan.xu@intel.com>

>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- 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,
> +                             int dma_old_pte_present,

With the rename, please also make this boolean parameter bool_t.

> +                             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 = 0;

Pointless initializer.

> @@ -584,29 +586,34 @@ 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);
> +            rc = 0;
> +        } else if ( rc < 0 )

Coding style.

> +            break;

I thought we had agreed on best effort flushing when an error
occurs. That means you shouldn't break out of the loop here, but
accumulate errors. (Breaking out of the loop would be okay if it
was conditional upon the domain having got crashed.)

(The same issue exists again near the end of the patch).

Jan

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