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

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

On May 12, 2016 9:38 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 12.05.16 at 15:29, <quan.xu@xxxxxxxxx> wrote:
> > On May 12, 2016 4:53 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >>> On 12.05.16 at 09:50, <quan.xu@xxxxxxxxx> wrote:
> >> > On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >> >>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
> >> >> > @@ -1391,13 +1399,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);
> >> >>
> >> >> Please take the opportunity and add the missing blank line
> >> >> (between
> >> >> declaration(s) and statement(s) in cases like this.
> >> >>
> >> >> > +    }
> >> >> > +
> >> >> > +    if ( rc > 0 )
> >> >>
> >> >> Can iommu_flush_context_device() return a positive value? If so,
> >> >> the logic is now likely wrong. If not (which is what I assume) I'd
> >> >> like to suggest adding a respective ASSERT() (even if only to
> >> >> document the fact). Or alternatively this
> >> >> if() could move into the immediately preceding one.
> >> >
> >> > Check it again. iommu_flush_context_device() can return a positive value.
> >> > [...]
> >> > Could you tell me why the logic is now likely wrong? I will fix it first.
> >>
> >> With
> >>
> >>     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;
> >>         rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> >>     }
> >>
> >>     if ( rc > 0 )
> >>     {
> >>         iommu_flush_write_buffer(iommu);
> >>         rc = 0;
> >>     }
> >>
> >> it seems pretty clear that you won't call iommu_flush_iotlb_dsi() if
> >> iommu_flush_context_device() returned 1, which doesn't look like what
> >> is wanted at the first glance. But I may be wrong, hence the "likely"
> >> in my
> > earlier
> >> reply.
> >>
> >
> > Oh, this was on purpose.
> >
> > If iommu_flush_context_device() returned 1,  the
> > iommu_flush_iotlb_dsi() returned 1 too.
> > As both flush_context_qi() and  flush_iotlb_qi () are the same at the
> > beginning of the  functions.
> 
> Such implications need to be commented on, so readers (like me) don't
> assume brokenness.
> 

ok, I will add a comment.

> > One concern is if iommu_flush_context_device() is failed, then we
> > won't call iommu_flush_iotlb_dsi(),  which is not best effort to flush.
> 
> Indeed.
> 

I'll fix it as well.


Quan

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