Message-ID: <20070207083254.GA24561@lst.de>
Date:	Wed, 7 Feb 2007 09:32:54 +0100
From:	Christoph Hellwig <hch@....de>
To:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [IA64] swiotlb abstraction (e.g. for Xen)

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 <asm/machvec.h>
> +
> +#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 <asm/io.h>
>  #include <asm/dma.h>
>  #include <asm/scatterlist.h>
> +#include <asm/swiotlb.h>
>  
>  #include <linux/init.h>
>  #include <linux/bootmem.h>
> @@ -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);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists