Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932174Ab2HFPIL (ORCPT ); Mon, 6 Aug 2012 11:08:11 -0400 Received: from g1t0027.austin.hp.com ([15.216.28.34]:12929 "EHLO g1t0027.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932074Ab2HFPIJ (ORCPT ); Mon, 6 Aug 2012 11:08:09 -0400 Message-ID: <1344265686.2486.13.camel@lorien2> Subject: Re: [PATCH RESEND] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is disabled From: Shuah Khan Reply-To: shuah.khan@hp.com To: Konrad Rzeszutek Wilk Cc: fujita.tomonori@lab.ntt.co.jp, akpm@linux-foundation.org, paul.gortmaker@windriver.com, bhelgaas@google.com, amwang@redhat.com, rob@landley.net, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, shuahkhan@gmail.com Date: Mon, 06 Aug 2012 09:08:06 -0600 In-Reply-To: <20120725004846.GB29692@phenom.dumpdata.com> References: <1343159187.3165.2.camel@lorien2> <20120725004846.GB29692@phenom.dumpdata.com> Organization: ISS-Linux Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10972 Lines: 279 On Tue, 2012-07-24 at 20:48 -0400, Konrad Rzeszutek Wilk wrote: > On Tue, Jul 24, 2012 at 01:46:27PM -0600, Shuah Khan wrote: > > Disable iotlb overflow support when CONFIG_ISA is disabled. Add deprecation > > You need to check one more thing. In the email I mentioned that the bulk > of the drivers that utilize this are ISA, but there are also some that > are PCI dependent. > > So I grepped for anything that does 'dma_map_page' and some of them are > even PCIe, for example: drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c > > Which means that the check for CONFIG_ISA is not sufficient. Thanks for the review and comments. Got back from a two week vacation and going to start work on this. I started sending fixes to drivers I can find that don't check dma mapping errors as well. -- Shuah > > > notice warning message and deprecation schedule documentation. This is the > > first step towards removing overflow support, to be consistent with other > > iommu implementations and return DMA_ERROR_CODE. This disabling step is for > > finding drivers that don't call dma_mapping_error to check for errors returned > > by the mapping interface. Once drivers are fixed overflow support can be > > removed. > > > > Signed-off-by: Shuah Khan > > Did you do a cross compile on IA64 just to double check? Comments below. > > > --- > > Documentation/feature-removal-schedule.txt | 22 ++++++-- > > lib/swiotlb.c | 79 +++++++++++++++++++++------- > > 2 files changed, 79 insertions(+), 22 deletions(-) > > > > diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt > > index 5979c3e..ce6c0ae 100644 > > --- a/Documentation/feature-removal-schedule.txt > > +++ b/Documentation/feature-removal-schedule.txt > > @@ -35,7 +35,7 @@ What: x86_32 "no-hlt" cmdline param > > When: 2012 > > Why: remove a branch from idle path, simplify code used by everybody. > > This option disabled the use of HLT in idle and machine_halt() > > - for hardware that was flakey 15-years ago. Today we have > > + for hardware that was flaky 15-years ago. Today we have > > Ummm, that is not part of what this patch should be doing. > > > "idle=poll" that removed HLT from idle, and so if such a machine > > is still running the upstream kernel, "idle=poll" is likely sufficient. > > Who: Len Brown > > @@ -160,7 +160,7 @@ Files: arch/*/kernel/*_ksyms.c > > Check: kernel_thread > > Why: kernel_thread is a low-level implementation detail. Drivers should > > use the API instead which shields them from > > - implementation details and provides a higherlevel interface that > > + implementation details and provides a higher level interface that > > Neither is this. > > prevents bugs and code duplication > > Who: Christoph Hellwig > > > > @@ -236,7 +236,7 @@ Who: David Brownell > > > > What: b43 support for firmware revision < 410 > > When: The schedule was July 2008, but it was decided that we are going to keep the > > - code as long as there are no major maintanance headaches. > > + code as long as there are no major maintenance headaches. > > Or this. > > > So it _could_ be removed _any_ time now, if it conflicts with something new. > > Why: The support code for the old firmware hurts code readability/maintainability > > and slightly hurts runtime performance. Bugfixes for the old firmware > > @@ -608,3 +608,19 @@ When: June 2013 > > Why: Unsupported/unmaintained/unused since 2.6 > > > > ---------------------------- > > + > > +What: SWIOTLB overflow buffer support. > > +When: 3.8 > > +Why: Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE > > + (a value of zero) to make it consistent with iommu implementation > > + on Intel, AMD, and swiotlb-xen. In 3.6, Disable iotlb overflow > > + support when CONFIG_ISA is disabled with the intent to find drivers > > + that don't call dma_mapping_error to check for errors returned by the > > + mapping interface. Once drivers are fixed overflow support can be > > + removed. > > + If you see any problems related to disabling SWIOTLB overflow buffer, > > + please report to us! > > + E-mail us at: linux-kernel@vger.kernel.org > > +Who: Shuah Khan > > + > > +---------------------------- > > diff --git a/lib/swiotlb.c b/lib/swiotlb.c > > index 45bc1f8..0123bb8 100644 > > --- a/lib/swiotlb.c > > +++ b/lib/swiotlb.c > > @@ -15,6 +15,9 @@ > > * 05/09/10 linville Add support for syncing ranges, support syncing for > > * DMA_BIDIRECTIONAL mappings, miscellaneous cleanup. > > * 08/12/11 beckyb Add highmem support > > + * 07/2012 shuahkhan Disable iotlb overflow support when CONFIG_ISA > > + * is enabled. Remove it for all configs when drivers > > + * that don't check for mapping errors are fixed. > > Don't bother. The changelog is now in the git commit. > > > */ > > > > #include > > @@ -68,7 +71,11 @@ static unsigned long io_tlb_nslabs; > > /* > > * When the IOMMU overflows we return a fallback buffer. This sets the size. > > */ > > +#if defined(CONFIG_ISA) > > static unsigned long io_tlb_overflow = 32*1024; > > +#else > > +static unsigned long io_tlb_overflow; > > +#endif > > > > static void *io_tlb_overflow_buffer; > > > > @@ -92,6 +99,24 @@ static DEFINE_SPINLOCK(io_tlb_lock); > > > > static int late_alloc; > > > > +static void swiotlb_print_overflow_deprecation_notice(void) > > +{ > > + if (io_tlb_overflow) { > > + pr_warn("SWIOTLB overflow buffer will be deprecated.\n" > > + " If you have a driver that depends on this feature\n" > > + " please Email us at: linux-kernel@vger.kernel.org,\n" > > + " Shuah Khan (shuahkhan@gmail.com), and\n" > > + " Konrad Wilk (konrad.wilk@oracle.com)\n"); > > + } else { > > + pr_warn("SWIOTLB overflow buffer is disabled and will be\n" > > + " deprecated. Please report problems related to\n" > > + " disabling overflow buffer to\n" > > + " linux-kernel@vger.kernel.org,\n" > > + " Shuah Khan (shuahkhan@gmail.com), and\n" > > + " Konrad Wilk (konrad.wilk@oracle.com)\n"); > > + } > > +} > > + > > static int __init > > setup_io_tlb_npages(char *str) > > { > > @@ -108,7 +133,6 @@ setup_io_tlb_npages(char *str) > > return 1; > > } > > __setup("swiotlb=", setup_io_tlb_npages); > > -/* make io_tlb_overflow tunable too? */ > > > > unsigned long swiotlb_nr_tbl(void) > > { > > @@ -156,12 +180,18 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose) > > io_tlb_index = 0; > > io_tlb_orig_addr = alloc_bootmem_pages(PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t))); > > > > - /* > > - * Get the overflow emergency buffer > > - */ > > - io_tlb_overflow_buffer = alloc_bootmem_low_pages(PAGE_ALIGN(io_tlb_overflow)); > > - if (!io_tlb_overflow_buffer) > > - panic("Cannot allocate SWIOTLB overflow buffer!\n"); > > + if (io_tlb_overflow) { > > + /* > > + * Get the overflow emergency buffer > > + */ > > + io_tlb_overflow_buffer = alloc_bootmem_low_pages( > > + PAGE_ALIGN(io_tlb_overflow)); > > + if (!io_tlb_overflow_buffer) > > + panic("Cannot allocate SWIOTLB overflow buffer!\n"); > > + } > > + > > + swiotlb_print_overflow_deprecation_notice(); > > + > > if (verbose) > > swiotlb_print_info(); > > } > > @@ -264,14 +294,17 @@ swiotlb_late_init_with_default_size(size_t default_size) > > > > memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(phys_addr_t)); > > > > - /* > > - * Get the overflow emergency buffer > > - */ > > - io_tlb_overflow_buffer = (void *)__get_free_pages(GFP_DMA, > > - get_order(io_tlb_overflow)); > > - if (!io_tlb_overflow_buffer) > > - goto cleanup4; > > + if (io_tlb_overflow) { > > + /* > > + * Get the overflow emergency buffer > > + */ > > + io_tlb_overflow_buffer = (void *) > > + __get_free_pages(GFP_DMA, get_order(io_tlb_overflow)); > > + if (!io_tlb_overflow_buffer) > > + goto cleanup4; > > + } > > > > + swiotlb_print_overflow_deprecation_notice(); > > swiotlb_print_info(); > > > > late_alloc = 1; > > @@ -297,12 +330,13 @@ cleanup1: > > > > void __init swiotlb_free(void) > > { > > - if (!io_tlb_overflow_buffer) > > + if (!io_tlb_orig_addr) > > return; > > > > if (late_alloc) { > > - free_pages((unsigned long)io_tlb_overflow_buffer, > > - get_order(io_tlb_overflow)); > > + if (io_tlb_overflow_buffer) > > + free_pages((unsigned long)io_tlb_overflow_buffer, > > + get_order(io_tlb_overflow)); > > free_pages((unsigned long)io_tlb_orig_addr, > > get_order(io_tlb_nslabs * sizeof(phys_addr_t))); > > free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs * > > @@ -310,8 +344,9 @@ void __init swiotlb_free(void) > > free_pages((unsigned long)io_tlb_start, > > get_order(io_tlb_nslabs << IO_TLB_SHIFT)); > > } else { > > - free_bootmem_late(__pa(io_tlb_overflow_buffer), > > - PAGE_ALIGN(io_tlb_overflow)); > > + if (io_tlb_overflow_buffer) > > + free_bootmem_late(__pa(io_tlb_overflow_buffer), > > + PAGE_ALIGN(io_tlb_overflow)); > > free_bootmem_late(__pa(io_tlb_orig_addr), > > PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t))); > > free_bootmem_late(__pa(io_tlb_list), > > @@ -681,6 +716,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, > > map = map_single(dev, phys, size, dir); > > if (!map) { > > swiotlb_full(dev, size, dir, 1); > > + if (!io_tlb_overflow) > > + return DMA_ERROR_CODE; > > map = io_tlb_overflow_buffer; > > } > > > > @@ -691,6 +728,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, > > */ > > if (!dma_capable(dev, dev_addr, size)) { > > swiotlb_tbl_unmap_single(dev, map, size, dir); > > + if (!io_tlb_overflow) > > + return DMA_ERROR_CODE; > > dev_addr = swiotlb_virt_to_bus(dev, io_tlb_overflow_buffer); > > } > > > > @@ -910,6 +949,8 @@ EXPORT_SYMBOL(swiotlb_sync_sg_for_device); > > int > > swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr) > > { > > + if (!io_tlb_overflow) > > + return DMA_ERROR_CODE; > > return (dma_addr == swiotlb_virt_to_bus(hwdev, io_tlb_overflow_buffer)); > > } > > EXPORT_SYMBOL(swiotlb_dma_mapping_error); > > -- > > 1.7.9.5 > > > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/