Re: [Xen-devel] [PATCH v5 4/7] VT-d: Refactor iommu_ops .iotlb_flush() and iotlb_flush_all()

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

> On February 17, 2016 10:20pm, <JBeulich@xxxxxxxx> wrote:
> >>> On 05.02.16 at 11:18, <quan.xu@xxxxxxxxx> wrote:
> 
> (Side note: The VT-d: prefix of the subject seems inadequate here.)
> 
> > to pass down a flag indicating whether the lock is being held.
> 
> "the lock" being which one?

"pcidevs_lock".


> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1100,7 +1100,7 @@ tlbflush:
> >      if ( flush )
> >      {
> >          flush_tlb_domain(d);
> > -        iommu_iotlb_flush(d, sgfn, egfn - sgfn);
> > +        iommu_iotlb_flush(d, sgfn, egfn - sgfn, NONE_LOCK);
> >      }
> >
> >  out:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -631,9 +631,9 @@ static int xenmem_add_to_physmap(struct domain
> *d,
> >      if ( need_iommu(d) )
> >      {
> >          this_cpu(iommu_dont_flush_iotlb) = 0;
> > -        rc = iommu_iotlb_flush(d, xatp->idx - done, done);
> > +        rc = iommu_iotlb_flush(d, xatp->idx - done, done, NONE_LOCK);
> >          if ( !rc )
> > -            rc = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> > +            rc = iommu_iotlb_flush(d, xatp->gpfn - done, done,
> > + NONE_LOCK);
> >      }
> 
> Considering both of these changes, I think it would be better to hide this
> implementation detail from callers outside of xen/drivers/passthrough/, by
> making iommu_iotlb_flush() a wrapper around the actual implementation. (This
> at once would limit the amount of ack-s such a patch will require.)
> 

Good idea.

> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -131,6 +131,13 @@ struct page_info;
> >   * callback pair.
> >   */
> >  typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, u32 id,
> > void *ctxt);
> > +/*
> > + * A flag indicates whether the lock is being held.
> > + * NONE_LOCK - no lock is being held.
> > + * PCIDEVS_LOCK - pcidevs_lock is being held.
> > + */
> > +#define NONE_LOCK 0
> > +#define PCIDEVS_LOCK 1
> 
> If you really assume further lock holding might need to be communicated here,
> this should be made more obviously a bit mask (by e.g. using hex constants, 
> not
> having a NON_LOCKS constant, and not making the first part of the comment
> refer to just one lock). If, however, the pcidevs_lock is all you care about, 
> I think
> code readability would benefit from making the respective function parameters
> bool_t, and adding (besides the already mentioned wrapper) another wrapper
> named e.g.
> iommu_iotlb_flush_all_locked().
> 

Yes, the pcidevs_lock is all I care about. I would make the respective function 
parameters
bool_t, and add another wrapper in v6.

e.g.
   iommu_iotlb_flush_all_locked() -- The pcidevs_lock is being held.
   iommu_iotlb_flush_all() -- The pcidevs_lock is NOT being held.
...

iommu_iotlb_flush()
iommu_iotlb_flush_locked()
...
.etc.

Quan

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