| Message ID | 1461921917-48394-2-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Xu, Quan > Sent: Friday, April 29, 2016 5:25 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, 64 insertions(+), 32 deletions(-) > > diff --git a/xen/drivers/passthrough/vtd/iommu.c > b/xen/drivers/passthrough/vtd/iommu.c > index 5ad25dc..6e2e43a 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, > + bool_t 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, ret = 0; > > /* > * No need pcideves_lock here because we have flush > @@ -584,29 +586,35 @@ 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); > + ret = 0; You don't need 'ret' here. Just change 'rc' to 0. > } > + else if ( rc < 0 ) > + ret = rc; Then this change is not required > } > + > + return ret; Then return 'rc' instead. > } > > static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int > page_count) could we remove "Intel_" prefix completely? You can rename this one to iommu_flush_iotlb_page... > { > - __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) and this one just iommu_flush_iotlb_all > { > - __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0); > + iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > } > Thanks Kevin
On May 04, 2016 9:26 AM, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Xu, Quan > > Sent: Friday, April 29, 2016 5:25 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, 64 insertions(+), 32 deletions(-) > > > > diff --git a/xen/drivers/passthrough/vtd/iommu.c > > b/xen/drivers/passthrough/vtd/iommu.c > > index 5ad25dc..6e2e43a 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, > > + bool_t 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, ret = 0; > > > > /* > > * No need pcideves_lock here because we have flush @@ -584,29 > > +586,35 @@ 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); > > + ret = 0; > > You don't need 'ret' here. Just change 'rc' to 0. > Agreed, this is really a good idea. > > } > > + else if ( rc < 0 ) > > + ret = rc; > > Then this change is not required > Ditto. > > } > > + > > + return ret; > > Then return 'rc' instead. > Ditto. > > } > > > > static void intel_iommu_iotlb_flush(struct domain *d, unsigned long > > gfn, unsigned int > > page_count) > > could we remove "Intel_" prefix completely? You can rename this one to > iommu_flush_iotlb_page... > Sure, I am ok. I wonder why to remove "Intel_" prefix. In this xen/drivers/passthrough/vtd/iommu.c file, most of functions are beginning with "intel_" as intel specific. In xen/drivers/passthrough/iommu.c file, most of functions are beginning with 'iommu_' as common part. Quan > > { > > - __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) > > and this one just iommu_flush_iotlb_all > > > { > > - __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0); > > + iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > > } > > >
>>> On 04.05.16 at 14:09, <quan.xu@intel.com> wrote: > On May 04, 2016 9:26 AM, Tian, Kevin <kevin.tian@intel.com> wrote: >> > From: Xu, Quan >> > Sent: Friday, April 29, 2016 5:25 PM >> > static void intel_iommu_iotlb_flush(struct domain *d, unsigned long >> > gfn, unsigned int >> > page_count) >> >> could we remove "Intel_" prefix completely? You can rename this one to >> iommu_flush_iotlb_page... >> > > Sure, I am ok. > I wonder why to remove "Intel_" prefix. > In this xen/drivers/passthrough/vtd/iommu.c file, most of functions are > beginning with "intel_" as intel specific. > In xen/drivers/passthrough/iommu.c file, most of functions are beginning > with 'iommu_' as common part. For non-static functions I'd suggest to keep the prefix (or use vtd_ alternatively). For static functions there's no need to disambiguate them, and hence an intel_ prefix is just mostly useless baggage. Jan
On May 04, 2016 9:52 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 04.05.16 at 14:09, <quan.xu@intel.com> wrote: > > On May 04, 2016 9:26 AM, Tian, Kevin <kevin.tian@intel.com> wrote: > >> > From: Xu, Quan > >> > Sent: Friday, April 29, 2016 5:25 PM static void > >> > intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, > >> > unsigned int > >> > page_count) > >> > >> could we remove "Intel_" prefix completely? You can rename this one > >> to iommu_flush_iotlb_page... > >> > > > > Sure, I am ok. > > I wonder why to remove "Intel_" prefix. > > In this xen/drivers/passthrough/vtd/iommu.c file, most of functions > > are beginning with "intel_" as intel specific. > > In xen/drivers/passthrough/iommu.c file, most of functions are > > beginning with 'iommu_' as common part. > > For non-static functions I'd suggest to keep the prefix (or use vtd_ > alternatively). For static functions there's no need to disambiguate them, and > hence an intel_ prefix is just mostly useless baggage. > Thanks. The above two are both static functions. So I will follow Kevin's suggestion. Quan
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 5ad25dc..6e2e43a 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, + bool_t 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, ret = 0; /* * No need pcideves_lock here because we have flush @@ -584,29 +586,35 @@ 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); + ret = 0; } + else if ( rc < 0 ) + ret = rc; } + + return ret; } 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 +648,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 +1289,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 +1403,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 +1425,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 +1520,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 +1548,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 +1570,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 +1760,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 +1776,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 hvm_iommu *hd = domain_hvm_iommu(d); int flush_dev_iotlb; int iommu_domid; + int rc, ret = 0; iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); @@ -1775,11 +1798,20 @@ 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); + ret = 0; + } + else if ( rc < 0 ) + ret = rc; } + + return ret; } 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..1ae0c5a 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, 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 | 94 +++++++++++++++++++++++++------------ xen/include/asm-x86/iommu.h | 2 +- 2 files changed, 64 insertions(+), 32 deletions(-)