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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On May 04, 2016 9:26 AM, Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
> > 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.
>

Agreed, this is really a good idea.
 
> >          }
> > +        else if ( rc < 0 )
> > +            ret = rc;
> 
> Then this change is not required
> 

Ditto.

> >      }
> > +
> > +    return ret;
> 
> Then return 'rc' instead.
> 

Ditto.

> >  }
> >
> >  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...
>

Sure, I am ok.
I wonder why to remove "Intel_" prefix.
In this xen/drivers/passthrough/vtd/iommu.c file, most of functions are 
beginning with "intel_" as intel specific.
In xen/drivers/passthrough/iommu.c file, most of functions are beginning with 
'iommu_' as common part.

Quan


 
> >  {
> > -    __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);
> >  }
> >
> 


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