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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.05.16 at 15:57, <quan.xu@xxxxxxxxx> wrote:
> @@ -1404,13 +1438,35 @@ 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, PCI_BDF2(bus, devfn),
> +                                    DMA_CCMD_MASK_NOBIT, 1);
> +
> +    /*
> +     * 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 or flush_iotlb_qi
> +     * (or flush_context_reg and flush_iotlb_reg, deep functions in the
> +     * call trees of iommu_flush_context_device and iommu_flush_iotlb_dsi)
> +     * are with the same logic to bubble up positive return value.
> +     */
> +    if ( rc <= 0 )
>      {
>          int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> -        iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> +        int ret;
> +
> +        ret = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);

Please make this the initializer again (at least one more such case
further down).

> @@ -1535,6 +1592,7 @@ int domain_context_unmap_one(
>      iommu_flush_cache_entry(context, sizeof(struct context_entry));
>  
>      iommu_domid= domain_iommu_domid(domain, iommu);
> +
>      if ( iommu_domid == -1 )

Once again a stray addition of a blank line, contradicting point 1 of
your v6 list of changes. Please actually _look_ at your patches
before sending them out.

> @@ -1542,14 +1600,36 @@ int domain_context_unmap_one(
>          return -EINVAL;
>      }
>  
> -    if ( iommu_flush_context_device(iommu, iommu_domid,
> -                                    (((u16)bus) << 8) | devfn,
> -                                    DMA_CCMD_MASK_NOBIT, 0) )
> -        iommu_flush_write_buffer(iommu);
> -    else
> +    rc = iommu_flush_context_device(iommu, iommu_domid,
> +                                    PCI_BDF2(bus, devfn),
> +                                    DMA_CCMD_MASK_NOBIT, 0);
> +
> +    /*
> +     * 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 or flush_iotlb_qi
> +     * (or flush_context_reg and flush_iotlb_reg, deep functions in the
> +     * call trees of iommu_flush_context_device and iommu_flush_iotlb_dsi)
> +     * are with the same logic to bubble up positive return value.
> +     */

This is the 3rd instance of that comment. I'd prefer the latter ones to
simply refer to the first one, but I'll obviously leave it to the maintainers
to decide.

With those cosmetic issues taken care of
Reviewed-by: Jen Beulich <jbeulich@xxxxxxxx>

Jan


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