| Message ID | 1462524880-67205-2-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> 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(&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. Jan
On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 06.05.16 at 10:54, <quan.xu@intel.com> 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
>>> On 12.05.16 at 09:50, <quan.xu@intel.com> wrote: > On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> 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(&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. Jan
On May 12, 2016 4:53 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 12.05.16 at 09:50, <quan.xu@intel.com> wrote: > > On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> 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(&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. 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
>>> On 12.05.16 at 15:29, <quan.xu@intel.com> wrote: > On May 12, 2016 4:53 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 12.05.16 at 09:50, <quan.xu@intel.com> wrote: >> > On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote: >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> 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(&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. > 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
On May 12, 2016 9:38 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 12.05.16 at 15:29, <quan.xu@intel.com> wrote: > > On May 12, 2016 4:53 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 12.05.16 at 09:50, <quan.xu@intel.com> wrote: > >> > On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote: > >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> 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
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(&iommu->lock); @@ -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); + } + + if ( rc > 0 ) + { + iommu_flush_write_buffer(iommu); + rc = 0; } set_bit(iommu->index, &hd->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( struct context_entry *context, *context_entries; u64 maddr; int iommu_domid; + int rc; ASSERT(pcidevs_locked()); spin_lock(&iommu->lock); @@ -1529,14 +1544,20 @@ int domain_context_unmap_one( return -EINVAL; } - if ( iommu_flush_context_device(iommu, iommu_domid, + rc = iommu_flush_context_device(iommu, iommu_domid, (((u16)bus) << 8) | devfn, - DMA_CCMD_MASK_NOBIT, 0) ) - iommu_flush_write_buffer(iommu); - else + DMA_CCMD_MASK_NOBIT, 0); + + if ( !rc ) { int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb); + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb); + } + + if ( rc > 0 ) + { + iommu_flush_write_buffer(iommu); + rc = 0; } spin_unlock(&iommu->lock); @@ -1545,7 +1566,7 @@ int domain_context_unmap_one( if ( !iommu->intel->drhd->segment ) me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC); - return 0; + return rc; } static int domain_context_unmap( @@ -1734,7 +1755,7 @@ static int intel_iommu_map_page( unmap_vtd_domain_page(page); if ( !this_cpu(iommu_dont_flush_iotlb) ) - __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1); + iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); return 0; } @@ -1750,14 +1771,15 @@ static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn) return 0; } -void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, - int order, int present) +int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, + int order, bool_t present) { struct acpi_drhd_unit *drhd; struct iommu *iommu = NULL; struct domain_iommu *hd = dom_iommu(d); int flush_dev_iotlb; int iommu_domid; + int rc = 0; iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); @@ -1771,11 +1793,18 @@ void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, iommu_domid= domain_iommu_domid(d, iommu); if ( iommu_domid == -1 ) continue; - if ( iommu_flush_iotlb_psi(iommu, iommu_domid, + + rc = iommu_flush_iotlb_psi(iommu, iommu_domid, (paddr_t)gfn << PAGE_SHIFT_4K, - order, !present, flush_dev_iotlb) ) + order, !present, flush_dev_iotlb); + if ( rc > 0 ) + { iommu_flush_write_buffer(iommu); + rc = 0; + } } + + return rc; } static int __init vtd_ept_page_compatible(struct iommu *iommu) @@ -2553,8 +2582,8 @@ const struct iommu_ops intel_iommu_ops = { .resume = vtd_resume, .share_p2m = iommu_set_pgd, .crash_shutdown = vtd_crash_shutdown, - .iotlb_flush = intel_iommu_iotlb_flush, - .iotlb_flush_all = intel_iommu_iotlb_flush_all, + .iotlb_flush = iommu_flush_iotlb_page, + .iotlb_flush_all = iommu_flush_iotlb_all, .get_reserved_device_memory = intel_iommu_get_reserved_device_memory, .dump_p2m_table = vtd_dump_p2m_table, }; diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h index e82a2f0..43f1620 100644 --- a/xen/include/asm-x86/iommu.h +++ b/xen/include/asm-x86/iommu.h @@ -27,7 +27,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); /* While VT-d specific, this must get declared in a generic header. */ int adjust_vtd_irq_affinities(void); -void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present); +int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, bool_t present); bool_t iommu_supports_eim(void); int iommu_enable_x2apic_IR(void); void iommu_disable_x2apic_IR(void);
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 <quan.xu@intel.com> CC: Kevin Tian <kevin.tian@intel.com> CC: Feng Wu <feng.wu@intel.com> CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/drivers/passthrough/vtd/iommu.c | 99 ++++++++++++++++++++++++------------- xen/include/asm-x86/iommu.h | 2 +- 2 files changed, 65 insertions(+), 36 deletions(-)