Re: [Xen-devel] [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one

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

On May 18, 2016 5:29 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 18.05.16 at 10:53, <quan.xu@xxxxxxxxx> wrote:
> > On May 17, 2016 8:37 PM, Jan Beulich <JBeulich@xxxxxxxx>wrote:
> >> >>> On 22.04.16 at 12:54, <quan.xu@xxxxxxxxx> wrote:
> >> > -static void queue_invalidate_iotlb(struct iommu *iommu,
> >> > -    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
> >> > +static int __must_check queue_invalidate_iotlb_sync(struct iommu
> >> *iommu,
> >> > +                                                    u8 granu, u8 dr, u8 
> >> > dw,
> >> > +                                                    u16 did, u8 am, u8 
> >> > ih,
> >> > +                                                    u64 addr)
> >> >  {
> >> >      unsigned long flags;
> >> >      unsigned int index;
> >> > @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct
> >> > iommu
> >> *iommu,
> >> >      unmap_vtd_domain_page(qinval_entries);
> >> >      qinval_update_qtail(iommu, index);
> >> >      spin_unlock_irqrestore(&iommu->register_lock, flags);
> >> > +
> >> > +    return invalidate_sync(iommu);
> >> >  }
> >>
> >> With this, ...
> >>
> >> > @@ -346,9 +353,13 @@ static int flush_iotlb_qi(
> >> >          if (cap_read_drain(iommu->cap))
> >> >              dr = 1;
> >> >          /* Need to conside the ih bit later */
> >> > -        queue_invalidate_iotlb(iommu,
> >> > -                               type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
> >> > -                               dw, did, size_order, 0, addr);
> >> > +        ret = queue_invalidate_iotlb_sync(iommu,
> >> > +                                          type >> 
> >> > DMA_TLB_FLUSH_GRANU_OFFSET,
> >> > +                                          dr, dw, did, size_order,
> >> > + 0, addr);
> >> > +
> >> > +        if ( ret )
> >> > +            return ret;
> >> > +
> >> >          if ( flush_dev_iotlb )
> >> >              ret = dev_invalidate_iotlb(iommu, did, addr, size_order, 
> >> > type);
> >> >          rc = invalidate_sync(iommu);
> >>
> >> ... why does this invalidate_sync() not go away?
> >>
> >
> > Oh, it is your suggestion -- leaving the existing logic as is would be
> > better - best effort invalidation even when an error has occurred.
> >
> > http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg00523.h
> > tml
> 
> Look like this was a bad comment of mine (resulting from
> dev_invalidate_iotlb(), other than the other respective functions, not 
> getting a
> _sync tag added), and I would have appreciated if you had simply pointed out
> the redundancy.

I just issued an open for this point in v9 discussion. I felt a strange, but 
really didn't have obvious reasons at that time.
--
I'll  drop this invalidate_sync() in v11.

> Please remember that the review process is bi-directional,
> and hence doesn't mean you need to blindly do everything a reviewer asks for:
> Things you agree with should be changed in code. For things you don't agree
> with you should reply verbally, explaining why a requested change shouldn't
> be done.
> 
Thanks. I will try to follow it.


Quan

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