| Message ID | 1465376344-28290-10-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Xu, Quan > Sent: Wednesday, June 08, 2016 4:59 PM > @@ -545,18 +549,42 @@ static int __must_check iommu_flush_all(void) > { > struct acpi_drhd_unit *drhd; > struct iommu *iommu; > - int flush_dev_iotlb; > + int rc = 0; > > flush_all_cache(); > for_each_drhd_unit ( drhd ) > { > iommu = drhd->iommu; > - iommu_flush_context_global(iommu, 0); > - flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; > - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); > + /* > + * The current logic for rc returns: > + * - positive invoke iommu_flush_write_buffer to flush cache. > + * - zero on success. > + * - negative on failure. Continue to flush IOMMU IOTLB on a > + * best effort basis. > + * > + * Moreover, IOMMU flush handlers flush_context_qi and flush_iotlb_qi > + * (or flush_context_reg and flush_iotlb_reg, deep functions in the > + * call trees of iommu_flush_context_global and iommu_flush_iotlb_global) > + * are with the same logic to bubble up positive return value. > + */ > + rc = iommu_flush_context_global(iommu, 0); > + if ( rc <= 0 ) > + { > + int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; > + int ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); > + > + ASSERT(ret <= 0); > + if ( !rc ) > + rc = ret; I'm dubious about the assertion here. Why can't above call return 1 upon error on earlier flush? I digged back your earlier reply like: > Yes, the iommu_flush_iotlb_dsi() can also return 1. > Look at the call tree, at the beginning of > flush_context_qi()/flush_iotlb_qi(), or > flush_context_reg()/flush_iotlb_reg().. > > If rc was negative when we call iommu_flush_context_device(), it is > impossible to return 1 for iommu_flush_iotlb_dsi(). But I don't think it a good idea of making so much assumptions about internal implementations of those low level interfaces. Also flush_context may fail for one specific reason which doesn't block flush_iotlb which could get 1 returned when caching mode is disabled. We'd better have return-1 case correctly handled here. Thanks Kevin
On June 12, 2016 3:33 PM, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Xu, Quan > > Sent: Wednesday, June 08, 2016 4:59 PM @@ -545,18 +549,42 @@ static > > int __must_check iommu_flush_all(void) { > > struct acpi_drhd_unit *drhd; > > struct iommu *iommu; > > - int flush_dev_iotlb; > > + int rc = 0; > > > > flush_all_cache(); > > for_each_drhd_unit ( drhd ) > > { > > iommu = drhd->iommu; > > - iommu_flush_context_global(iommu, 0); > > - flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; > > - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); > > + /* > > + * The current logic for rc returns: > > + * - positive invoke iommu_flush_write_buffer to flush cache. > > + * - zero on success. > > + * - negative on failure. Continue to flush IOMMU IOTLB on a > > + * best effort basis. > > + * > > + * Moreover, IOMMU flush handlers flush_context_qi and > flush_iotlb_qi > > + * (or flush_context_reg and flush_iotlb_reg, deep functions in the > > + * call trees of iommu_flush_context_global and > iommu_flush_iotlb_global) > > + * are with the same logic to bubble up positive return value. > > + */ > > + rc = iommu_flush_context_global(iommu, 0); > > + if ( rc <= 0 ) > > + { > > + int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; > > + int ret = iommu_flush_iotlb_global(iommu, 0, > > + flush_dev_iotlb); > > + > > + ASSERT(ret <= 0); > > + if ( !rc ) > > + rc = ret; > > I'm dubious about the assertion here. Why can't above call return 1 upon error > on earlier flush? I digged back your earlier reply like: > > > Yes, the iommu_flush_iotlb_dsi() can also return 1. > > Look at the call tree, at the beginning of > > flush_context_qi()/flush_iotlb_qi(), or > > flush_context_reg()/flush_iotlb_reg().. > > > > If rc was negative when we call iommu_flush_context_device(), it is > > impossible to return 1 for iommu_flush_iotlb_dsi(). > > But I don't think it a good idea of making so much assumptions about internal > implementations of those low level interfaces. > Also flush_context may fail for one specific reason which doesn't block > flush_iotlb which could get 1 returned when caching mode is disabled. We'd > better have return-1 case correctly handled here. > Your comment looks reasonable here. Could I change it as below: -static int iommu_flush_iotlb_psi( - struct iommu *iommu, u16 did, u64 addr, unsigned int order, - int flush_non_present_entry, int flush_dev_iotlb) +static int __must_check iommu_flush_iotlb_psi(struct iommu *iommu, u16 did, + u64 addr, unsigned int order, + int flush_non_present_entry, + int flush_dev_iotlb) { struct iommu_flush *flush = iommu_get_flush(iommu); int status; @@ -546,17 +550,35 @@ static int __must_check iommu_flush_all(void) struct acpi_drhd_unit *drhd; struct iommu *iommu; int flush_dev_iotlb; + int rc = 0; flush_all_cache(); for_each_drhd_unit ( drhd ) { + int ret; + iommu = drhd->iommu; - iommu_flush_context_global(iommu, 0); + /* + * The current logic for rc returns: + * - positive invoke iommu_flush_write_buffer to flush cache. + * - zero on success. + * - negative on failure. Continue to flush IOMMU IOTLB on a + * best effort basis. + */ + rc = iommu_flush_context_global(iommu, 0); flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); + ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); + if ( !rc ) + rc = ret; + + if ( rc > 0 || ret > 0 ) + iommu_flush_write_buffer(iommu); } - return 0; + if ( rc > 0 ) + rc = 0; + + return rc; } Also, Jan, what's your opinion? Quan
On June 12, 2016 5:27 PM, Xu, Quan <quan.xu@intel.com> wrote: > On June 12, 2016 3:33 PM, Tian, Kevin <kevin.tian@intel.com> wrote: > > > From: Xu, Quan > > > Sent: Wednesday, June 08, 2016 4:59 PM @@ -545,18 +549,42 @@ > static > > > int __must_check iommu_flush_all(void) { > > > struct acpi_drhd_unit *drhd; > > > struct iommu *iommu; > > > - int flush_dev_iotlb; > > > + int rc = 0; > > > > > > flush_all_cache(); > > > for_each_drhd_unit ( drhd ) > > > { > > > iommu = drhd->iommu; > > > - iommu_flush_context_global(iommu, 0); > > > - flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; > > > - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); > > > + /* > > > + * The current logic for rc returns: > > > + * - positive invoke iommu_flush_write_buffer to flush cache. > > > + * - zero on success. > > > + * - negative on failure. Continue to flush IOMMU IOTLB on a > > > + * best effort basis. > > > + * > > > + * Moreover, IOMMU flush handlers flush_context_qi and > > flush_iotlb_qi > > > + * (or flush_context_reg and flush_iotlb_reg, deep functions in the > > > + * call trees of iommu_flush_context_global and > > iommu_flush_iotlb_global) > > > + * are with the same logic to bubble up positive return value. > > > + */ > > > + rc = iommu_flush_context_global(iommu, 0); > > > + if ( rc <= 0 ) > > > + { > > > + int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; > > > + int ret = iommu_flush_iotlb_global(iommu, 0, > > > + flush_dev_iotlb); > > > + > > > + ASSERT(ret <= 0); > > > + if ( !rc ) > > > + rc = ret; > > > > I'm dubious about the assertion here. Why can't above call return 1 > > upon error on earlier flush? I digged back your earlier reply like: > > > > > Yes, the iommu_flush_iotlb_dsi() can also return 1. > > > Look at the call tree, at the beginning of > > > flush_context_qi()/flush_iotlb_qi(), or > > > flush_context_reg()/flush_iotlb_reg().. > > > > > > If rc was negative when we call iommu_flush_context_device(), it is > > > impossible to return 1 for iommu_flush_iotlb_dsi(). > > > > But I don't think it a good idea of making so much assumptions about > > internal implementations of those low level interfaces. > > Also flush_context may fail for one specific reason which doesn't > > block flush_iotlb which could get 1 returned when caching mode is > > disabled. We'd better have return-1 case correctly handled here. > > > > Your comment looks reasonable here. Could I change it as below: > > -static int iommu_flush_iotlb_psi( > - struct iommu *iommu, u16 did, u64 addr, unsigned int order, > - int flush_non_present_entry, int flush_dev_iotlb) > +static int __must_check iommu_flush_iotlb_psi(struct iommu *iommu, u16 > did, > + u64 addr, unsigned int order, > + int flush_non_present_entry, > + int flush_dev_iotlb) > { > struct iommu_flush *flush = iommu_get_flush(iommu); > int status; > @@ -546,17 +550,35 @@ static int __must_check iommu_flush_all(void) > struct acpi_drhd_unit *drhd; > struct iommu *iommu; > int flush_dev_iotlb; > + int rc = 0; > > flush_all_cache(); > for_each_drhd_unit ( drhd ) > { > + int ret; > + > iommu = drhd->iommu; > - iommu_flush_context_global(iommu, 0); > + /* > + * The current logic for rc returns: > + * - positive invoke iommu_flush_write_buffer to flush cache. > + * - zero on success. > + * - negative on failure. Continue to flush IOMMU IOTLB on a > + * best effort basis. > + */ > + rc = iommu_flush_context_global(iommu, 0); > flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; > - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); > + ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); > + if ( !rc ) > + rc = ret; > + > + if ( rc > 0 || ret > 0 ) > + iommu_flush_write_buffer(iommu); > } > > - return 0; > + if ( rc > 0 ) > + rc = 0; > + > + return rc; > } > > Ah, this change is not correct, as the previous error return value may be erased by the later positive / zero value. I'll highlight this change is not under Jan's R-b in next v8. .. I think the below is correct. +static int __must_check iommu_flush_iotlb_psi(struct iommu *iommu, u16 did, + u64 addr, unsigned int order, + int flush_non_present_entry, + int flush_dev_iotlb) { struct iommu_flush *flush = iommu_get_flush(iommu); int status; @@ -546,17 +550,37 @@ static int __must_check iommu_flush_all(void) struct acpi_drhd_unit *drhd; struct iommu *iommu; int flush_dev_iotlb; + int rc = 0; flush_all_cache(); for_each_drhd_unit ( drhd ) { + int iommu_rc, iommu_ret; + iommu = drhd->iommu; - iommu_flush_context_global(iommu, 0); + iommu_rc = iommu_flush_context_global(iommu, 0); flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); + iommu_ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); + + /* + * The current logic for returns: + * - positive invoke iommu_flush_write_buffer to flush cache. + * - zero on success. + * - negative on failure. Continue to flush IOMMU IOTLB on a + * best effort basis. + */ + if ( iommu_rc > 0 || iommu_ret > 0 ) + iommu_flush_write_buffer(iommu); + if ( rc >= 0 ) + rc = iommu_rc; + if ( rc >= 0 ) + rc = iommu_ret; } - return 0; + if ( rc > 0 ) + rc = 0; + + return rc; } > > Also, Jan, what's your opinion? > > Quan > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 48edb67..2a55985 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -388,17 +388,18 @@ static int flush_context_reg( return 0; } -static int iommu_flush_context_global( - struct iommu *iommu, int flush_non_present_entry) +static int __must_check iommu_flush_context_global(struct iommu *iommu, + int flush_non_present_entry) { struct iommu_flush *flush = iommu_get_flush(iommu); return flush->context(iommu, 0, 0, 0, DMA_CCMD_GLOBAL_INVL, flush_non_present_entry); } -static int iommu_flush_context_device( - struct iommu *iommu, u16 did, u16 source_id, - u8 function_mask, int flush_non_present_entry) +static int __must_check iommu_flush_context_device(struct iommu *iommu, + u16 did, u16 source_id, + u8 function_mask, + int flush_non_present_entry) { struct iommu_flush *flush = iommu_get_flush(iommu); return flush->context(iommu, did, source_id, function_mask, @@ -473,8 +474,9 @@ static int flush_iotlb_reg(void *_iommu, u16 did, return 0; } -static int iommu_flush_iotlb_global(struct iommu *iommu, - int flush_non_present_entry, int flush_dev_iotlb) +static int __must_check iommu_flush_iotlb_global(struct iommu *iommu, + int flush_non_present_entry, + int flush_dev_iotlb) { struct iommu_flush *flush = iommu_get_flush(iommu); int status; @@ -491,8 +493,9 @@ static int iommu_flush_iotlb_global(struct iommu *iommu, return status; } -static int iommu_flush_iotlb_dsi(struct iommu *iommu, u16 did, - int flush_non_present_entry, int flush_dev_iotlb) +static int __must_check iommu_flush_iotlb_dsi(struct iommu *iommu, u16 did, + int flush_non_present_entry, + int flush_dev_iotlb) { struct iommu_flush *flush = iommu_get_flush(iommu); int status; @@ -509,9 +512,10 @@ static int iommu_flush_iotlb_dsi(struct iommu *iommu, u16 did, return status; } -static int iommu_flush_iotlb_psi( - struct iommu *iommu, u16 did, u64 addr, unsigned int order, - int flush_non_present_entry, int flush_dev_iotlb) +static int __must_check iommu_flush_iotlb_psi(struct iommu *iommu, u16 did, + u64 addr, unsigned int order, + int flush_non_present_entry, + int flush_dev_iotlb) { struct iommu_flush *flush = iommu_get_flush(iommu); int status; @@ -545,18 +549,42 @@ static int __must_check iommu_flush_all(void) { struct acpi_drhd_unit *drhd; struct iommu *iommu; - int flush_dev_iotlb; + int rc = 0; flush_all_cache(); for_each_drhd_unit ( drhd ) { iommu = drhd->iommu; - iommu_flush_context_global(iommu, 0); - flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); + /* + * The current logic for rc returns: + * - positive invoke iommu_flush_write_buffer to flush cache. + * - zero on success. + * - negative on failure. Continue to flush IOMMU IOTLB on a + * best effort basis. + * + * Moreover, IOMMU flush handlers flush_context_qi and flush_iotlb_qi + * (or flush_context_reg and flush_iotlb_reg, deep functions in the + * call trees of iommu_flush_context_global and iommu_flush_iotlb_global) + * are with the same logic to bubble up positive return value. + */ + rc = iommu_flush_context_global(iommu, 0); + if ( rc <= 0 ) + { + int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; + int ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); + + ASSERT(ret <= 0); + if ( !rc ) + rc = ret; + } + else + { + iommu_flush_write_buffer(iommu); + rc = 0; + } } - return 0; + return rc; } static int __must_check iommu_flush_iotlb(struct domain *d, @@ -569,6 +597,7 @@ static int __must_check iommu_flush_iotlb(struct domain *d, struct iommu *iommu; int flush_dev_iotlb; int iommu_domid; + int rc = 0; /* * No need pcideves_lock here because we have flush @@ -587,21 +616,23 @@ static int __must_check iommu_flush_iotlb(struct domain *d, 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 0; + return rc; } static int __must_check iommu_flush_iotlb_pages(struct domain *d, @@ -1291,6 +1322,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); @@ -1404,13 +1436,34 @@ 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, PCI_BDF2(bus, devfn), + DMA_CCMD_MASK_NOBIT, 1); + + /* + * The current logic for rc returns: + * - positive invoke iommu_flush_write_buffer to flush cache. + * - zero on success. + * - negative on failure. Continue to flush IOMMU IOTLB on a + * best effort basis. + * + * Moreover, IOMMU flush handlers flush_context_qi or flush_iotlb_qi + * (or flush_context_reg and flush_iotlb_reg, deep functions in the + * call trees of iommu_flush_context_device and iommu_flush_iotlb_dsi) + * are with the same logic to bubble up positive return value. + */ + if ( rc <= 0 ) { int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb); + int ret = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb); + + ASSERT(ret <= 0); + if ( !rc ) + rc = ret; + } + else + { + iommu_flush_write_buffer(iommu); + rc = 0; } set_bit(iommu->index, &hd->arch.iommu_bitmap); @@ -1420,7 +1473,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( @@ -1515,6 +1568,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); @@ -1542,14 +1596,35 @@ int domain_context_unmap_one( return -EINVAL; } - if ( iommu_flush_context_device(iommu, iommu_domid, - (((u16)bus) << 8) | devfn, - DMA_CCMD_MASK_NOBIT, 0) ) - iommu_flush_write_buffer(iommu); - else + rc = iommu_flush_context_device(iommu, iommu_domid, + PCI_BDF2(bus, devfn), + DMA_CCMD_MASK_NOBIT, 0); + + /* + * The current logic for rc returns: + * - positive invoke iommu_flush_write_buffer to flush cache. + * - zero on success. + * - negative on failure. Continue to flush IOMMU IOTLB on a + * best effort basis. + * + * Moreover, IOMMU flush handlers flush_context_qi or flush_iotlb_qi + * (or flush_context_reg and flush_iotlb_reg, deep functions in the + * call trees of iommu_flush_context_device and iommu_flush_iotlb_dsi) + * are with the same logic to bubble up positive return value. + */ + if ( rc <= 0 ) { int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb); + int ret = iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb); + + ASSERT(ret <= 0); + if ( !rc ) + rc = ret; + } + else + { + iommu_flush_write_buffer(iommu); + rc = 0; } spin_unlock(&iommu->lock); @@ -1558,7 +1633,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( @@ -1772,6 +1847,7 @@ int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, 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)); @@ -1785,13 +1861,18 @@ int 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 0; + return rc; } static int __init vtd_ept_page_compatible(struct iommu *iommu)