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

From: "Xu, Quan" <quan.xu@intel.com>
To: "Tian, Kevin" <kevin.tian@intel.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: Keir Fraser <keir@xen.org>,
	"dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	"Wu, Feng" <feng.wu@intel.com>, Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
Date: Tue, 19 Apr 2016 08:33:09 +0000	[thread overview]
Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B88D34B@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D15F821AC5@SHSMSX101.ccr.corp.intel.com>

On April 19, 2016 2:33pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Monday, April 18, 2016 10:00 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@intel.com>
> >
> > CC: Kevin Tian <kevin.tian@intel.com>
> > CC: Feng Wu <feng.wu@intel.com>
> > CC: Keir Fraser <keir@xen.org>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> >  xen/drivers/passthrough/vtd/iommu.c | 94
> > ++++++++++++++++++++++++-------------
> >  xen/include/asm-x86/iommu.h         |  2 +-
> >  2 files changed, 63 insertions(+), 33 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > b/xen/drivers/passthrough/vtd/iommu.c
> > index 5ad25dc..50d98ac 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,
> 
> any reason for the renaming here?
> 

As Jan mentioned, 
"this would be a good opportunity to also drop the stray double underscores: You need to touch all callers anyway."
http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg02456.html 

think twice, I dropped the stray double underscores.
both intel_iommu_iotlb_flush and iommu_iotlb_flush were already in use,
So I tried to rename it to ' iommu_flush_iotlb'.

> > +                             int 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 = 0;
> >
> >      /*
> >       * No need pcideves_lock here because we have flush @@ -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 )
> > +            break;
> >      }
> > +
> > +    return rc;
> >  }
> >
> >  static void intel_iommu_iotlb_flush(struct domain *d, unsigned long
> > gfn, unsigned int
> > page_count)
> >  {
> > -    __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)  {
> > -    __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0);
> > +    iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> >  }
> 
> Do you plan to change return value of above 2 functions in another patch, or
> will keep it current form w/o propagation?
> 

Change return value of above 2 functions in another patches.

> >
> >  /* clear one page's page table */
> > @@ -640,7 +647,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);
> >
> >      unmap_vtd_domain_page(page);
> >  }
> > @@ -1281,6 +1288,7 @@ int domain_context_mapping_one(
> >      u64 maddr, pgd_maddr;
> >      u16 seg = iommu->intel->drhd->segment;
> >      int agaw;
> > +    int rc;
> >
> >      ASSERT(pcidevs_locked());
> >      spin_lock(&iommu->lock);
> > @@ -1394,13 +1402,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);
> > +    }
> > +
> > +    if ( rc > 0 )
> > +    {
> > +        iommu_flush_write_buffer(iommu);
> > +        rc = 0;
> 
> what about 'rc>0' in iommu_flush_context_device while "rc<0"
> in iommu_flush_iotlb_dsi, which will leave write buffer not flushed?

That's true, but IMO it is impossible.
IOW, if 'rc>0' is in iommu_flush_context_device, the rc should be "rc>0" in iommu_flush_iotlb_dsi too.
As the flush_non_present_entry parameter is '1' to both, and read the same register 'iommu->cap' at the beginning of the functions.

To be honest, this modification is an aggressive approach and looks good to me, but I am open for any other suggestions.

Quan

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