| Message ID | 1460988011-17758-2-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Xu, Quan > Sent: Monday, April 18, 2016 10:00 PM > > 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 | 94 > ++++++++++++++++++++++++------------- > xen/include/asm-x86/iommu.h | 2 +- > 2 files changed, 63 insertions(+), 33 deletions(-) > > diff --git a/xen/drivers/passthrough/vtd/iommu.c > b/xen/drivers/passthrough/vtd/iommu.c > index 5ad25dc..50d98ac 100644 > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -558,14 +558,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, any reason for the renaming here? > + int dma_old_pte_present, > + unsigned int page_count) > { > struct hvm_iommu *hd = domain_hvm_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 > @@ -584,29 +586,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; > + } else if ( rc < 0 ) > + break; > } > + > + return rc; > } > > static void intel_iommu_iotlb_flush(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) > { > - __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0); > + iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > } Do you plan to change return value of above 2 functions in another patch, or will keep it current form w/o propagation? > > /* clear one page's page table */ > @@ -640,7 +647,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); > } > @@ -1281,6 +1288,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); > @@ -1394,13 +1402,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; what about 'rc>0' in iommu_flush_context_device while "rc<0" in iommu_flush_iotlb_dsi, which will leave write buffer not flushed? > } > > set_bit(iommu->index, &hd->arch.iommu_bitmap); > @@ -1410,7 +1424,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( > @@ -1505,6 +1519,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); > @@ -1532,14 +1547,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; > } ditto Thanks Kevin
On April 19, 2016 2:33pm, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Xu, Quan > > Sent: Monday, April 18, 2016 10:00 PM > > > > 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 | 94 > > ++++++++++++++++++++++++------------- > > xen/include/asm-x86/iommu.h | 2 +- > > 2 files changed, 63 insertions(+), 33 deletions(-) > > > > diff --git a/xen/drivers/passthrough/vtd/iommu.c > > b/xen/drivers/passthrough/vtd/iommu.c > > index 5ad25dc..50d98ac 100644 > > --- a/xen/drivers/passthrough/vtd/iommu.c > > +++ b/xen/drivers/passthrough/vtd/iommu.c > > @@ -558,14 +558,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, > > any reason for the renaming here? > As Jan mentioned, "this would be a good opportunity to also drop the stray double underscores: You need to touch all callers anyway." http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg02456.html think twice, I dropped the stray double underscores. both intel_iommu_iotlb_flush and iommu_iotlb_flush were already in use, So I tried to rename it to ' iommu_flush_iotlb'. > > + int dma_old_pte_present, > > + unsigned int page_count) > > { > > struct hvm_iommu *hd = domain_hvm_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 @@ -584,29 > > +586,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; > > + } else if ( rc < 0 ) > > + break; > > } > > + > > + return rc; > > } > > > > static void intel_iommu_iotlb_flush(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) { > > - __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0); > > + iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > > } > > Do you plan to change return value of above 2 functions in another patch, or > will keep it current form w/o propagation? > Change return value of above 2 functions in another patches. > > > > /* clear one page's page table */ > > @@ -640,7 +647,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); > > } > > @@ -1281,6 +1288,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); > > @@ -1394,13 +1402,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; > > what about 'rc>0' in iommu_flush_context_device while "rc<0" > in iommu_flush_iotlb_dsi, which will leave write buffer not flushed? That's true, but IMO it is impossible. IOW, if 'rc>0' is in iommu_flush_context_device, the rc should be "rc>0" in iommu_flush_iotlb_dsi too. As the flush_non_present_entry parameter is '1' to both, and read the same register 'iommu->cap' at the beginning of the functions. To be honest, this modification is an aggressive approach and looks good to me, but I am open for any other suggestions. Quan
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -558,14 +558,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, > + int dma_old_pte_present, With the rename, please also make this boolean parameter bool_t. > + unsigned int page_count) > { > struct hvm_iommu *hd = domain_hvm_iommu(d); > struct acpi_drhd_unit *drhd; > struct iommu *iommu; > int flush_dev_iotlb; > int iommu_domid; > + int rc = 0; Pointless initializer. > @@ -584,29 +586,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; > + } else if ( rc < 0 ) Coding style. > + break; I thought we had agreed on best effort flushing when an error occurs. That means you shouldn't break out of the loop here, but accumulate errors. (Breaking out of the loop would be okay if it was conditional upon the domain having got crashed.) (The same issue exists again near the end of the patch). Jan
On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > > I thought we had agreed on best effort flushing when an error occurs. That > means you shouldn't break out of the loop here, but accumulate errors. > (Breaking out of the loop would be okay if it was conditional upon the domain > having got crashed.) > I will follow it for coming version. I indeed agreed to that single point, however, I wondered whether I could extend it as a pattern for this series or not. Now I am clear. Quan > (The same issue exists again near the end of the patch). >
On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > > --- a/xen/drivers/passthrough/vtd/iommu.c > > +++ b/xen/drivers/passthrough/vtd/iommu.c > > @@ -558,14 +558,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, > > + int dma_old_pte_present, > > + unsigned int page_count) > > { > > struct hvm_iommu *hd = domain_hvm_iommu(d); > > struct acpi_drhd_unit *drhd; > > struct iommu *iommu; > > int flush_dev_iotlb; > > int iommu_domid; > > + int rc = 0; > > Pointless initializer. > I am afraid not. In case I don't initialize 'rc', the compiler prints out " error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]". Quan
>>> On 26.04.16 at 04:18, <quan.xu@intel.com> wrote: > On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: >> > --- a/xen/drivers/passthrough/vtd/iommu.c >> > +++ b/xen/drivers/passthrough/vtd/iommu.c >> > @@ -558,14 +558,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, >> > + int dma_old_pte_present, >> > + unsigned int page_count) >> > { >> > struct hvm_iommu *hd = domain_hvm_iommu(d); >> > struct acpi_drhd_unit *drhd; >> > struct iommu *iommu; >> > int flush_dev_iotlb; >> > int iommu_domid; >> > + int rc = 0; >> >> Pointless initializer. > > I am afraid not. > In case I don't initialize 'rc', the compiler prints out " error: 'rc' may > be used uninitialized in this function [-Werror=maybe-uninitialized]". Looking at the patch again I can't see why that would be the case. Are you certain this isn't a result of subsequent patches, IOW did you try this with just this one patch applied? rc gets initialized in both the if() and the else branches, and there's no label allowing that initialization to be bypassed... Jan
On April 26, 2016 5:11 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 26.04.16 at 04:18, <quan.xu@intel.com> wrote: > > On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > >> > --- a/xen/drivers/passthrough/vtd/iommu.c > >> > +++ b/xen/drivers/passthrough/vtd/iommu.c > >> > @@ -558,14 +558,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, > >> > + int dma_old_pte_present, > >> > + unsigned int page_count) > >> > { > >> > struct hvm_iommu *hd = domain_hvm_iommu(d); > >> > struct acpi_drhd_unit *drhd; > >> > struct iommu *iommu; > >> > int flush_dev_iotlb; > >> > int iommu_domid; > >> > + int rc = 0; > >> > >> Pointless initializer. > > > > I am afraid not. > > In case I don't initialize 'rc', the compiler prints out " error: > > 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]". > > Looking at the patch again I can't see why that would be the case. > Are you certain this isn't a result of subsequent patches, IOW did you try this > with just this one patch applied? Yes, I test it with this only patch again. The same result. > rc gets initialized in both the if() and the else > branches, and there's no label allowing that initialization to be bypassed... look at this function again, >> func iommu_flush_iotlb() { int rc; for_each_drhd_unit ( drhd ) { if () rc .. else rc .. } return rc; } << func The case drhd is NULL, the rc may be not initialized. Development ENV: Ubuntu 14.04.1, GCC 4.8 Quan
>>> On 26.04.16 at 12:15, <quan.xu@intel.com> wrote: > On April 26, 2016 5:11 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 26.04.16 at 04:18, <quan.xu@intel.com> wrote: >> > On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c >> >> > @@ -558,14 +558,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, >> >> > + int dma_old_pte_present, >> >> > + unsigned int page_count) >> >> > { >> >> > struct hvm_iommu *hd = domain_hvm_iommu(d); >> >> > struct acpi_drhd_unit *drhd; >> >> > struct iommu *iommu; >> >> > int flush_dev_iotlb; >> >> > int iommu_domid; >> >> > + int rc = 0; >> >> >> >> Pointless initializer. >> > >> > I am afraid not. >> > In case I don't initialize 'rc', the compiler prints out " error: >> > 'rc' may be used uninitialized in this function > [-Werror=maybe-uninitialized]". >> >> Looking at the patch again I can't see why that would be the case. >> Are you certain this isn't a result of subsequent patches, IOW did you try > this >> with just this one patch applied? > > Yes, I test it with this only patch again. > > The same result. > >> rc gets initialized in both the if() and the else >> branches, and there's no label allowing that initialization to be > bypassed... > > look at this function again, > >>> func > iommu_flush_iotlb() > { > int rc; > > for_each_drhd_unit ( drhd ) > { > if () > rc .. > else > rc .. > } > > return rc; > } Ah, I'm sorry - the patch context didn't make it obvious this is in a loop. Jan
On April 26, 2016 6:53 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 26.04.16 at 12:15, <quan.xu@intel.com> wrote: > > On April 26, 2016 5:11 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 26.04.16 at 04:18, <quan.xu@intel.com> wrote: > >> > On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c > >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c > >> >> > @@ -558,14 +558,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, > >> >> > + int dma_old_pte_present, > >> >> > + unsigned int page_count) > >> >> > { > >> >> > struct hvm_iommu *hd = domain_hvm_iommu(d); > >> >> > struct acpi_drhd_unit *drhd; > >> >> > struct iommu *iommu; > >> >> > int flush_dev_iotlb; > >> >> > int iommu_domid; > >> >> > + int rc = 0; > >> >> > >> >> Pointless initializer. > >> > > >> > I am afraid not. > >> > In case I don't initialize 'rc', the compiler prints out " error: > >> > 'rc' may be used uninitialized in this function > > [-Werror=maybe-uninitialized]". > >> > >> Looking at the patch again I can't see why that would be the case. > >> Are you certain this isn't a result of subsequent patches, IOW did > >> you try > > this > >> with just this one patch applied? > > > > Yes, I test it with this only patch again. > > > > The same result. > > > >> rc gets initialized in both the if() and the else branches, and > >> there's no label allowing that initialization to be > > bypassed... > > > > look at this function again, > > > >>> func > > iommu_flush_iotlb() > > { > > int rc; > > > > for_each_drhd_unit ( drhd ) > > { > > if () > > rc .. > > else > > rc .. > > } > > > > return rc; > > } > > Ah, I'm sorry - the patch context didn't make it obvious this is in a loop. > Never mind. It is _not_ quite as obvious, but compiler helps us. Quan
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 5ad25dc..50d98ac 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -558,14 +558,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, + int dma_old_pte_present, + unsigned int page_count) { struct hvm_iommu *hd = domain_hvm_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 @@ -584,29 +586,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; + } else if ( rc < 0 ) + break; } + + return rc; } static void intel_iommu_iotlb_flush(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) { - __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0); + iommu_flush_iotlb(d, INVALID_GFN, 0, 0); } /* clear one page's page table */ @@ -640,7 +647,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); } @@ -1281,6 +1288,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); @@ -1394,13 +1402,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); @@ -1410,7 +1424,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( @@ -1505,6 +1519,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); @@ -1532,14 +1547,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); @@ -1548,7 +1569,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( @@ -1738,7 +1759,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; } @@ -1754,14 +1775,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, int present) { struct acpi_drhd_unit *drhd; struct iommu *iommu = NULL; struct hvm_iommu *hd = domain_hvm_iommu(d); int flush_dev_iotlb; int iommu_domid; + int rc = 0; iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); @@ -1775,11 +1797,19 @@ 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; + } else if ( rc < 0 ) + break; } + + return rc; } static int __init vtd_ept_page_compatible(struct iommu *iommu) diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h index f22b3a5..790701e 100644 --- a/xen/include/asm-x86/iommu.h +++ b/xen/include/asm-x86/iommu.h @@ -26,7 +26,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, int 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 | 94 ++++++++++++++++++++++++------------- xen/include/asm-x86/iommu.h | 2 +- 2 files changed, 63 insertions(+), 33 deletions(-)