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

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

On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
> > -static void intel_iommu_iotlb_flush(struct domain *d, unsigned long
> > gfn, unsigned int page_count)
> > +static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn,
> > +                                   unsigned int page_count)
> 
> The new name suggests just one page. Please use e.g.
> iommu_flush_iotlb_pages() instead.
> 

Make sense. 

> >  {
> > -    __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
> > +    iommu_flush_iotlb(d, gfn, 1, page_count);
> >  }
> 
> But of course the question is whether having this wrapper is useful in the 
> first
> place,


This wrapper assumes the 'dma_old_pte_present' is '1', but in another caller 
intel_iommu_map_page(), i.e. 


     intel_iommu_map_page()
    {
       ...
             if ( !this_cpu(iommu_dont_flush_iotlb) )
                  iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1);
       ...
    }


the 'dma_old_pte_present' is not sure. 

in intel_iommu_map_page(),  if we can check the 'dma_pte_present(old)':
          -- 1, flush the pages.
          -- 0, don't flush the pages.

Then we can remove this wrapper.

If my description is not clear, I can send out the related change.

> the more that ...
> 
> > @@ -639,7 +646,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);
> 
> ... it's being open coded here. IOW if you want to retain the wrapper, please
> use it here.
>

Waiting for the above discussion, if we still need the wrapper, I will use it 
here.


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

If VT-d QI is enabled, the call tree up to iommu_flush_context_device():
                         -- flush_context_qi()  -- iommu_flush_context_device() 


i.e. 

In flush_context_qi()
{
...
    if ( flush_non_present_entry )
    {
        if ( !cap_caching_mode(iommu->cap) )
            return 1;
        else
            did = 0;
    }
...
}


and the ' flush_non_present_entry '  is really '1' for above code.



Could you tell me why the logic is now likely wrong? I will fix it first.

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