sorry, this should of course go to the people to blame aswell :) On Wed, Feb 07, 2007 at 09:32:54AM +0100, Christoph Hellwig wrote: > On Wed, Feb 07, 2007 at 07:59:18AM +0000, Linux Kernel Mailing List wrote: > > [IA64] swiotlb abstraction (e.g. for Xen) > > > > Add abstraction so that the file can be used by environments other than IA64 > > and EM64T, namely for Xen. > > Tony, this code is more than ugly, and even further not needed for anything > we actually need. Can you please revert it. > > Some comments below in case we need justification.. > > If Jan actually had a goal with that except making the code utterly > unreadable he should try again with small patches that are well > explained and do one thing at a at time. (And cane be reviewed an > improved on if needed. > > > > diff --git a/include/asm-ia64/swiotlb.h b/include/asm-ia64/swiotlb.h > > new file mode 100644 > > index 0000000..452c162 > > --- /dev/null > > +++ b/include/asm-ia64/swiotlb.h > > @@ -0,0 +1,9 @@ > > +#ifndef _ASM_SWIOTLB_H > > +#define _ASM_SWIOTLB_H 1 > > + > > +#include > > + > > +#define SWIOTLB_ARCH_NEED_LATE_INIT > > +#define SWIOTLB_ARCH_NEED_ALLOC > > > > > + > > +#endif /* _ASM_SWIOTLB_H */ > > diff --git a/include/asm-x86_64/swiotlb.h b/include/asm-x86_64/swiotlb.h > > index f9c5895..ab913ff 100644 > > --- a/include/asm-x86_64/swiotlb.h > > +++ b/include/asm-x86_64/swiotlb.h > > @@ -44,6 +44,7 @@ extern void swiotlb_init(void); > > extern int swiotlb_force; > > > > #ifdef CONFIG_SWIOTLB > > +#define SWIOTLB_ARCH_NEED_ALLOC > > extern int swiotlb; > > #else > > #define swiotlb 0 > > diff --git a/lib/swiotlb.c b/lib/swiotlb.c > > index 067eed5..50a4380 100644 > > --- a/lib/swiotlb.c > > +++ b/lib/swiotlb.c > > @@ -28,6 +28,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -35,8 +36,10 @@ > > #define OFFSET(val,align) ((unsigned long) \ > > ( (val) & ( (align) - 1))) > > > > +#ifndef SG_ENT_VIRT_ADDRESS > > #define SG_ENT_VIRT_ADDRESS(sg) (page_address((sg)->page) + (sg)->offset) > > #define SG_ENT_PHYS_ADDRESS(sg) virt_to_bus(SG_ENT_VIRT_ADDRESS(sg)) > > +#endif > > This is always needed, so there is no point in adding #ifndef. > If you really want to cleanup the code remove these useless macros > entirely. > > > > /* > > * Maximum allowable number of contiguous slabs to map, > > @@ -101,13 +104,25 @@ static unsigned int io_tlb_index; > > * We need to save away the original address corresponding to a mapped entry > > * for the sync operations. > > */ > > -static unsigned char **io_tlb_orig_addr; > > +#ifndef SWIOTLB_ARCH_HAS_IO_TLB_ADDR_T > > > > +typedef char *io_tlb_addr_t; > > +#define swiotlb_orig_addr_null(buffer) (!(buffer)) > > +#define ptr_to_io_tlb_addr(ptr) (ptr) > > +#define page_to_io_tlb_addr(pg, off) (page_address(pg) + (off)) > > +#define sg_to_io_tlb_addr(sg) SG_ENT_VIRT_ADDRESS(sg) > > +#endif > > +static io_tlb_addr_t *io_tlb_orig_addr; > > “WIOTLB_ARCH_HAS_IO_TLB_ADDR_T is never ever defined, and these > abstractions are an utter mess. Please don't put this in. > > > +#ifdef SWIOTLB_EXTRA_VARIABLES > > +SWIOTLB_EXTRA_VARIABLES; > > +#endif > > No way we'd put this in. > > > +#ifndef swiotlb_adjust_size > > +#define swiotlb_adjust_size(size) ((void)0) > > +#endif > > + > > +#ifndef swiotlb_adjust_seg > > +#define swiotlb_adjust_seg(start, size) ((void)0) > > +#endif > > + > > +#ifndef swiotlb_print_info > > +#define swiotlb_print_info(bytes) \ > > + printk(KERN_INFO "Placing %luMB software IO TLB between 0x%lx - " \ > > + "0x%lx\n", bytes >> 20, \ > > + virt_to_bus(io_tlb_start), virt_to_bus(io_tlb_end)) > > +#endif > > + > > /* > > * Statically reserve bounce buffer space and initialize bounce buffer data > > * structures for the software IO TLB used to implement the DMA API. > > @@ -138,6 +169,8 @@ swiotlb_init_with_default_size(size_t default_size) > > io_tlb_nslabs = (default_size >> IO_TLB_SHIFT); > > io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE); > > } > > + swiotlb_adjust_size(io_tlb_nslabs); > > + swiotlb_adjust_size(io_tlb_overflow); > > > > bytes = io_tlb_nslabs << IO_TLB_SHIFT; > > > > @@ -155,10 +188,14 @@ swiotlb_init_with_default_size(size_t default_size) > > * between io_tlb_start and io_tlb_end. > > */ > > io_tlb_list = alloc_bootmem(io_tlb_nslabs * sizeof(int)); > > - for (i = 0; i < io_tlb_nslabs; i++) > > + for (i = 0; i < io_tlb_nslabs; i++) { > > + if ( !(i % IO_TLB_SEGSIZE) ) > > + swiotlb_adjust_seg(io_tlb_start + (i << IO_TLB_SHIFT), > > + IO_TLB_SEGSIZE << IO_TLB_SHIFT); > > io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE); > > + } > > io_tlb_index = 0; > > - io_tlb_orig_addr = alloc_bootmem(io_tlb_nslabs * sizeof(char *)); > > + io_tlb_orig_addr = alloc_bootmem(io_tlb_nslabs * sizeof(io_tlb_addr_t)); > > > > /* > > * Get the overflow emergency buffer > > @@ -166,17 +203,21 @@ swiotlb_init_with_default_size(size_t default_size) > > io_tlb_overflow_buffer = alloc_bootmem_low(io_tlb_overflow); > > if (!io_tlb_overflow_buffer) > > panic("Cannot allocate SWIOTLB overflow buffer!\n"); > > + swiotlb_adjust_seg(io_tlb_overflow_buffer, io_tlb_overflow); > > > > - printk(KERN_INFO "Placing software IO TLB between 0x%lx - 0x%lx\n", > > - virt_to_bus(io_tlb_start), virt_to_bus(io_tlb_end)); > > + swiotlb_print_info(bytes); > > } > > +#ifndef __swiotlb_init_with_default_size > > +#define __swiotlb_init_with_default_size swiotlb_init_with_default_size > > +#endif > > > > void __init > > swiotlb_init(void) > > { > > - swiotlb_init_with_default_size(64 * (1<<20)); /* default to 64MB */ > > + __swiotlb_init_with_default_size(64 * (1<<20)); /* default to 64MB */ > > } > > > > +#ifdef SWIOTLB_ARCH_NEED_LATE_INIT > > /* > > * Systems with larger DMA zones (those that don't support ISA) can > > * initialize the swiotlb later using the slab allocator if needed. > > @@ -234,12 +275,12 @@ swiotlb_late_init_with_default_size(size_t default_size) > > io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE); > > io_tlb_index = 0; > > > > - io_tlb_orig_addr = (unsigned char **)__get_free_pages(GFP_KERNEL, > > - get_order(io_tlb_nslabs * sizeof(char *))); > > + io_tlb_orig_addr = (io_tlb_addr_t *)__get_free_pages(GFP_KERNEL, > > + get_order(io_tlb_nslabs * sizeof(io_tlb_addr_t))); > > if (!io_tlb_orig_addr) > > goto cleanup3; > > > > - memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(char *)); > > + memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(io_tlb_addr_t)); > > > > /* > > * Get the overflow emergency buffer > > @@ -249,19 +290,17 @@ swiotlb_late_init_with_default_size(size_t default_size) > > if (!io_tlb_overflow_buffer) > > goto cleanup4; > > > > - printk(KERN_INFO "Placing %luMB software IO TLB between 0x%lx - " > > - "0x%lx\n", bytes >> 20, > > - virt_to_bus(io_tlb_start), virt_to_bus(io_tlb_end)); > > + swiotlb_print_info(bytes); > > > > return 0; > > > > cleanup4: > > - free_pages((unsigned long)io_tlb_orig_addr, get_order(io_tlb_nslabs * > > - sizeof(char *))); > > + free_pages((unsigned long)io_tlb_orig_addr, > > + get_order(io_tlb_nslabs * sizeof(io_tlb_addr_t))); > > io_tlb_orig_addr = NULL; > > cleanup3: > > - free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs * > > - sizeof(int))); > > + free_pages((unsigned long)io_tlb_list, > > + get_order(io_tlb_nslabs * sizeof(int))); > > io_tlb_list = NULL; > > cleanup2: > > io_tlb_end = NULL; > > @@ -271,7 +310,9 @@ cleanup1: > > io_tlb_nslabs = req_nslabs; > > return -ENOMEM; > > } > > +#endif > > > > +#ifndef SWIOTLB_ARCH_HAS_NEEDS_MAPPING > > static inline int > > address_needs_mapping(struct device *hwdev, dma_addr_t addr) > > { > > @@ -282,11 +323,35 @@ address_needs_mapping(struct device *hwdev, dma_addr_t addr) > > return (addr & ~mask) != 0; > > } > > > > +static inline int range_needs_mapping(const void *ptr, size_t size) > > +{ > > + return swiotlb_force; > > +} > > + > > +static inline int order_needs_mapping(unsigned int order) > > +{ > > + return 0; > > +} > > +#endif > > + > > +static void > > +__sync_single(io_tlb_addr_t buffer, char *dma_addr, size_t size, int dir) > > +{ > > +#ifndef SWIOTLB_ARCH_HAS_SYNC_SINGLE > > + if (dir == DMA_TO_DEVICE) > > + memcpy(dma_addr, buffer, size); > > + else > > + memcpy(buffer, dma_addr, size); > > +#else > > + __swiotlb_arch_sync_single(buffer, dma_addr, size, dir); > > +#endif > > +} > > + > > /* > > * Allocates bounce buffer and returns its kernel virtual address. > > */ > > static void * > > -map_single(struct device *hwdev, char *buffer, size_t size, int dir) > > +map_single(struct device *hwdev, io_tlb_addr_t buffer, size_t size, int dir) > > { > > unsigned long flags; > > char *dma_addr; > > @@ -359,7 +424,7 @@ map_single(struct device *hwdev, char *buffer, size_t size, int dir) > > */ > > io_tlb_orig_addr[index] = buffer; > > if (dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL) > > - memcpy(dma_addr, buffer, size); > > + __sync_single(buffer, dma_addr, size, DMA_TO_DEVICE); > > > > return dma_addr; > > } > > @@ -373,17 +438,18 @@ unmap_single(struct device *hwdev, char *dma_addr, size_t size, int dir) > > unsigned long flags; > > int i, count, nslots = ALIGN(size, 1 << IO_TLB_SHIFT) >> IO_TLB_SHIFT; > > int index = (dma_addr - io_tlb_start) >> IO_TLB_SHIFT; > > - char *buffer = io_tlb_orig_addr[index]; > > + io_tlb_addr_t buffer = io_tlb_orig_addr[index]; > > > > /* > > * First, sync the memory before unmapping the entry > > */ > > - if (buffer && ((dir == DMA_FROM_DEVICE) || (dir == DMA_BIDIRECTIONAL))) > > + if (!swiotlb_orig_addr_null(buffer) > > + && ((dir == DMA_FROM_DEVICE) || (dir == DMA_BIDIRECTIONAL))) > > /* > > * bounce... copy the data back into the original buffer * and > > * delete the bounce buffer. > > */ > > - memcpy(buffer, dma_addr, size); > > + __sync_single(buffer, dma_addr, size, DMA_FROM_DEVICE); > > > > /* > > * Return the buffer to the free list by setting the corresponding > > @@ -416,18 +482,18 @@ sync_single(struct device *hwdev, char *dma_addr, size_t size, > > int dir, int target) > > { > > int index = (dma_addr - io_tlb_start) >> IO_TLB_SHIFT; > > - char *buffer = io_tlb_orig_addr[index]; > > + io_tlb_addr_t buffer = io_tlb_orig_addr[index]; > > > > switch (target) { > > case SYNC_FOR_CPU: > > if (likely(dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL)) > > - memcpy(buffer, dma_addr, size); > > + __sync_single(buffer, dma_addr, size, DMA_FROM_DEVICE); > > else > > BUG_ON(dir != DMA_TO_DEVICE); > > break; > > case SYNC_FOR_DEVICE: > > if (likely(dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL)) > > - memcpy(dma_addr, buffer, size); > > + __sync_single(buffer, dma_addr, size, DMA_TO_DEVICE); > > else > > BUG_ON(dir != DMA_FROM_DEVICE); > > break; > > @@ -436,6 +502,8 @@ sync_single(struct device *hwdev, char *dma_addr, size_t size, > > } > > } > > > > +#ifdef SWIOTLB_ARCH_NEED_ALLOC > > + > > void * > > swiotlb_alloc_coherent(struct device *hwdev, size_t size, > > dma_addr_t *dma_handle, gfp_t flags) > > @@ -451,7 +519,10 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, > > */ > > flags |= GFP_DMA; > > > > - ret = (void *)__get_free_pages(flags, order); > > + if (!order_needs_mapping(order)) > > + ret = (void *)__get_free_pages(flags, order); > > + else > > + ret = NULL; > > if (ret && address_needs_mapping(hwdev, virt_to_bus(ret))) { > > /* > > * The allocated memory isn't reachable by the device. > > @@ -489,6 +560,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, > > *dma_handle = dev_addr; > > return ret; > > } > > +EXPORT_SYMBOL(swiotlb_alloc_coherent); > > > > void > > swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > > @@ -501,6 +573,9 @@ swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > > /* DMA_TO_DEVICE to avoid memcpy in unmap_single */ > > swiotlb_unmap_single (hwdev, dma_handle, size, DMA_TO_DEVICE); > > } > > +EXPORT_SYMBOL(swiotlb_free_coherent); > > + > > +#endif > > > > static void > > swiotlb_full(struct device *dev, size_t size, int dir, int do_panic) > > @@ -542,13 +617,14 @@ swiotlb_map_single(struct device *hwdev, void *ptr, size_t size, int dir) > > * we can safely return the device addr and not worry about bounce > > * buffering it. > > */ > > - if (!address_needs_mapping(hwdev, dev_addr) && !swiotlb_force) > > + if (!range_needs_mapping(ptr, size) > > + && !address_needs_mapping(hwdev, dev_addr)) > > return dev_addr; > > > > /* > > * Oh well, have to allocate and map a bounce buffer. > > */ > > - map = map_single(hwdev, ptr, size, dir); > > + map = map_single(hwdev, ptr_to_io_tlb_addr(ptr), size, dir); > > if (!map) { > > swiotlb_full(hwdev, size, dir, 1); > > map = io_tlb_overflow_buffer; > > @@ -676,17 +752,16 @@ int > > swiotlb_map_sg(struct device *hwdev, struct scatterlist *sg, int nelems, > > int dir) > > { > > - void *addr; > > dma_addr_t dev_addr; > > int i; > > > > BUG_ON(dir == DMA_NONE); > > > > for (i = 0; i < nelems; i++, sg++) { > > - addr = SG_ENT_VIRT_ADDRESS(sg); > > - dev_addr = virt_to_bus(addr); > > - if (swiotlb_force || address_needs_mapping(hwdev, dev_addr)) { > > - void *map = map_single(hwdev, addr, sg->length, dir); > > + dev_addr = SG_ENT_PHYS_ADDRESS(sg); > > + if (range_needs_mapping(SG_ENT_VIRT_ADDRESS(sg), sg->length) > > + || address_needs_mapping(hwdev, dev_addr)) { > > + void *map = map_single(hwdev, sg_to_io_tlb_addr(sg), sg->length, dir); > > if (!map) { > > /* Don't panic here, we expect map_sg users > > to do proper error handling. */ > > @@ -760,6 +835,44 @@ swiotlb_sync_sg_for_device(struct device *hwdev, struct scatterlist *sg, > > swiotlb_sync_sg(hwdev, sg, nelems, dir, SYNC_FOR_DEVICE); > > } > > > > +#ifdef SWIOTLB_ARCH_NEED_MAP_PAGE > > + > > +dma_addr_t > > +swiotlb_map_page(struct device *hwdev, struct page *page, > > + unsigned long offset, size_t size, > > + enum dma_data_direction direction) > > +{ > > + dma_addr_t dev_addr; > > + char *map; > > + > > + dev_addr = page_to_bus(page) + offset; > > + if (address_needs_mapping(hwdev, dev_addr)) { > > + map = map_single(hwdev, page_to_io_tlb_addr(page, offset), size, direction); > > + if (!map) { > > + swiotlb_full(hwdev, size, direction, 1); > > + map = io_tlb_overflow_buffer; > > + } > > + dev_addr = virt_to_bus(map); > > + } > > + > > + return dev_addr; > > +} > > + > > +void > > +swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr, > > + size_t size, enum dma_data_direction direction) > > +{ > > + char *dma_addr = bus_to_virt(dev_addr); > > + > > + BUG_ON(direction == DMA_NONE); > > + if (dma_addr >= io_tlb_start && dma_addr < io_tlb_end) > > + unmap_single(hwdev, dma_addr, size, direction); > > + else if (direction == DMA_FROM_DEVICE) > > + dma_mark_clean(dma_addr, size); > > +} > > + > > +#endif > > + > > int > > swiotlb_dma_mapping_error(dma_addr_t dma_addr) > > { > > @@ -772,10 +885,13 @@ swiotlb_dma_mapping_error(dma_addr_t dma_addr) > > * during bus mastering, then you would pass 0x00ffffff as the mask to > > * this function. > > */ > > +#ifndef __swiotlb_dma_supported > > +#define __swiotlb_dma_supported(hwdev, mask) (virt_to_bus(io_tlb_end - 1) <= (mask)) > > +#endif > > int > > swiotlb_dma_supported(struct device *hwdev, u64 mask) > > { > > - return virt_to_bus(io_tlb_end - 1) <= mask; > > + return __swiotlb_dma_supported(hwdev, mask); > > } > > > > EXPORT_SYMBOL(swiotlb_init); > > @@ -790,6 +906,4 @@ EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_device); > > EXPORT_SYMBOL(swiotlb_sync_sg_for_cpu); > > EXPORT_SYMBOL(swiotlb_sync_sg_for_device); > > EXPORT_SYMBOL(swiotlb_dma_mapping_error); > > -EXPORT_SYMBOL(swiotlb_alloc_coherent); > > -EXPORT_SYMBOL(swiotlb_free_coherent); > > EXPORT_SYMBOL(swiotlb_dma_supported); ---end quoted text---