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

[Xu, Quan]

On May 12, 2016 9:38 PM, Jan Beulich  wrote:
> >>> On 12.05.16 at 15:29,  wrote:
> > On May 12, 2016 4:53 PM, Jan Beulich  wrote:
> >> >>> On 12.05.16 at 09:50,  wrote:
> >> > On May 10, 2016 12:10 AM, Jan Beulich  wrote:
> >> >> >>> On 06.05.16 at 10:54,  wrote:
> >> >> > @@ -1391,13 +1399,19 @@ int domain_context_mapping_one(
> >> >> >  spin_unlock(>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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 12.05.16 at 15:29,  wrote:
> On May 12, 2016 4:53 PM, Jan Beulich  wrote:
>> >>> On 12.05.16 at 09:50,  wrote:
>> > On May 10, 2016 12:10 AM, Jan Beulich  wrote:
>> >> >>> On 06.05.16 at 10:54,  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.
>> 
>> I'm sorry, but you're looking at this backwards: I suggested to remove the
>> wrapper, not to move any check into iommu_flush_iotlb().
>> Removing the wrapper simply means to move the passing of the hard coded 1
>> into the current callers of that wrapper.
>> 
> 
> A little bit confused.
> Check one thing, do the wrappers refer to iommu_flush_iotlb_page() and 
> iommu_flush_iotlb_all() ?
> 
> If yes, we can't ignore another thing:
> 
> These two wrappers are also initialized for 2 .callbacks at the bottom of 
> this file:
> 
> 
> .iotlb_flush = iommu_flush_iotlb_pages,
> .iotlb_flush_all = iommu_flush_iotlb_all,
> 

Ah, good point. With the renaming going on I didn't realize these
are used here. So in fact they're not just wrappers. Please disregard
my respective comments then.

>> >> > @@ -1391,13 +1399,19 @@ int domain_context_mapping_one(
>> >> >  spin_unlock(>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.

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

Jan

___
Xen-devel mailing list
Xen-devel@lists.xen.org

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

[Xu, Quan]

On May 12, 2016 4:53 PM, Jan Beulich  wrote:
> >>> On 12.05.16 at 09:50,  wrote:
> > On May 10, 2016 12:10 AM, Jan Beulich  wrote:
> >> >>> On 06.05.16 at 10:54,  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.
> 
> I'm sorry, but you're looking at this backwards: I suggested to remove the
> wrapper, not to move any check into iommu_flush_iotlb().
> Removing the wrapper simply means to move the passing of the hard coded 1
> into the current callers of that wrapper.
> 

A little bit confused.
Check one thing, do the wrappers refer to iommu_flush_iotlb_page() and 
iommu_flush_iotlb_all() ?

If yes, we can't ignore another thing:

These two wrappers are also initialized for 2 .callbacks at the bottom of this 
file:


.iotlb_flush = iommu_flush_iotlb_pages,
.iotlb_flush_all = iommu_flush_iotlb_all,





> >> > @@ -1391,13 +1399,19 @@ int domain_context_mapping_one(
> >> >  spin_unlock(>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.

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.


Quan










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

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

[Jan Beulich]

>>> On 12.05.16 at 09:50,  wrote:
> On May 10, 2016 12:10 AM, Jan Beulich  wrote:
>> >>> On 06.05.16 at 10:54,  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. 

I'm sorry, but you're looking at this backwards: I suggested to
remove the wrapper, not to move any check into iommu_flush_iotlb().
Removing the wrapper simply means to move the passing of the
hard coded 1 into the current callers of that wrapper.

>> > @@ -1391,13 +1399,19 @@ int domain_context_mapping_one(
>> >  spin_unlock(>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.

Jan


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

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

[Xu, Quan]

On May 10, 2016 12:10 AM, Jan Beulich  wrote:
> >>> On 06.05.16 at 10:54,  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(>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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 06.05.16 at 10:54,  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.

>  {
> -__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, 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.

> @@ -1391,13 +1399,19 @@ int domain_context_mapping_one(
>  spin_unlock(>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.

Jan


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

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

[Quan Xu]

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 

CC: Kevin Tian 
CC: Feng Wu 
CC: Keir Fraser 
CC: Jan Beulich 
CC: Andrew Cooper 
---
 xen/drivers/passthrough/vtd/iommu.c | 99 -
 xen/include/asm-x86/iommu.h |  2 +-
 2 files changed, 65 insertions(+), 36 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c 
b/xen/drivers/passthrough/vtd/iommu.c
index db83949..64093a9 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -557,14 +557,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 domain_iommu *hd = dom_iommu(d);
 struct acpi_drhd_unit *drhd;
 struct iommu *iommu;
 int flush_dev_iotlb;
 int iommu_domid;
+int rc = 0;
 
 /*
  * No need pcideves_lock here because we have flush
@@ -583,29 +585,34 @@ 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);
+rc = 0;
 }
 }
+
+return rc;
 }
 
-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)
 {
-__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)
+static void iommu_flush_iotlb_all(struct domain *d)
 {
-__intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0);
+iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
 }
 
 /* clear one page's page table */
@@ -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);
 
 unmap_vtd_domain_page(page);
 }
@@ -1278,6 +1285,7 @@ int domain_context_mapping_one(
 u64 maddr, pgd_maddr;
 u16 seg = iommu->intel->drhd->segment;
 int agaw;
+int rc;
 
 ASSERT(pcidevs_locked());
 spin_lock(>lock);
@@ -1391,13 +1399,19 @@ int domain_context_mapping_one(
 spin_unlock(>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);
+}
+
+if ( rc > 0 )
+{
+iommu_flush_write_buffer(iommu);
+rc = 0;
 }
 
 set_bit(iommu->index, >arch.iommu_bitmap);
@@ -1407,7 +1421,7 @@ int domain_context_mapping_one(
 if ( !seg )
 me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
 
-return 0;
+return rc;
 }
 
 static int domain_context_mapping(
@@ -1502,6 +1516,7 @@ int domain_context_unmap_one(