[PATCH] Retry 3: Use i386 swiotlb code in lib/swiotlb-xen.c [2/2]

[PATCH] Retry 3: Use i386 swiotlb code in lib/swiotlb-xen.c [2/2]

[Langsdorf, Mark]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

Move the arch/i386/kernel/swiotlb.c code to lib/swiotlb-xen.c
code in order to simplify maintenance of Xen in the future.

The first patch simply moves the code to lib/swiotlb-xen.c;
the second patch adds the necessary changes to the Xen code
base to allow x86_64 systems to boot with SWIOTLB and the
dma_ops framework.

The first patch also copies the standard 
arch/x86_64/kernel/pci-dma.c to arch/x86_64/kernel/pci-dma-xen.c,
which should make reviewing the second patch a little easier.

[-- Attachment #2: merge-swiotlb-for-xen.patch --]
[-- Type: application/octet-stream, Size: 9270 bytes --]

diff -r 06d53b6b5fcf linux-2.6-xen-sparse/arch/x86_64/kernel/Makefile
--- a/linux-2.6-xen-sparse/arch/x86_64/kernel/Makefile	Mon Feb 26 15:52:16 2007 -0600
+++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/Makefile	Mon Feb 26 15:58:43 2007 -0600
@@ -59,7 +59,7 @@ alternative-y			+= ../../i386/kernel/alt
 
 ifdef CONFIG_XEN
 time-y				+= ../../i386/kernel/time-xen.o
-pci-dma-y			+= ../../i386/kernel/pci-dma-xen.o
+pci-dma-y			+= pci-dma-xen.o
 microcode-$(subst m,y,$(CONFIG_MICROCODE))  := ../../i386/kernel/microcode-xen.o
 intel_cacheinfo-y		:= ../../i386/kernel/cpu/intel_cacheinfo-xen.o
 quirks-y			:= ../../i386/kernel/quirks-xen.o
diff -r 06d53b6b5fcf linux-2.6-xen-sparse/arch/x86_64/kernel/pci-dma-xen.c
--- a/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-dma-xen.c	Mon Feb 26 15:52:16 2007 -0600
+++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-dma-xen.c	Mon Feb 26 15:58:43 2007 -0600
@@ -10,6 +10,8 @@
 #include <asm/io.h>
 #include <asm/proto.h>
 #include <asm/calgary.h>
+#include <xen/balloon.h>
+#include <asm/tlbflush.h>
 
 int iommu_merge __read_mostly = 0;
 EXPORT_SYMBOL(iommu_merge);
@@ -129,6 +131,12 @@ dma_alloc_coherent(struct device *dev, s
 			return NULL;
 		}
 
+		/* Hardcode 31 address bits for now: aacraid limitation. */
+		if (xen_create_contiguous_region((unsigned long)memory, get_order(size), fls64(dma_mask)) != 0) {
+			free_pages((unsigned long)memory, get_order(size));
+			return NULL;
+		}
+
 		memset(memory, 0, size);
 		if (!mmu) {
 			*dma_handle = virt_to_bus(memory);
@@ -166,6 +174,7 @@ void dma_free_coherent(struct device *de
 {
 	if (dma_ops->unmap_single)
 		dma_ops->unmap_single(dev, bus, size, 0);
+	xen_destroy_contiguous_region((unsigned long) vaddr, get_order(size));
 	free_pages((unsigned long)vaddr, get_order(size));
 }
 EXPORT_SYMBOL(dma_free_coherent);
diff -r 06d53b6b5fcf linux-2.6-xen-sparse/arch/x86_64/kernel/pci-swiotlb-xen.c
--- a/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-swiotlb-xen.c	Mon Feb 26 15:52:16 2007 -0600
+++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-swiotlb-xen.c	Mon Feb 26 15:58:43 2007 -0600
@@ -8,13 +8,10 @@
 #include <asm/swiotlb.h>
 #include <asm/dma.h>
 
-#if 0
 int swiotlb __read_mostly;
 EXPORT_SYMBOL(swiotlb);
-#endif
 
 struct dma_mapping_ops swiotlb_dma_ops = {
-#if 0
 	.mapping_error = swiotlb_dma_mapping_error,
 	.alloc_coherent = swiotlb_alloc_coherent,
 	.free_coherent = swiotlb_free_coherent,
@@ -29,14 +26,12 @@ struct dma_mapping_ops swiotlb_dma_ops =
 	.map_sg = swiotlb_map_sg,
 	.unmap_sg = swiotlb_unmap_sg,
 	.dma_supported = NULL,
-#endif
 };
 
 void pci_swiotlb_init(void)
 {
-#if 0
 	/* don't initialize swiotlb if iommu=off (no_iommu=1) */
-	if (!iommu_detected && !no_iommu && end_pfn > MAX_DMA32_PFN)
+	if (!iommu_detected && !no_iommu)
 	       swiotlb = 1;
 	if (swiotlb_force)
 		swiotlb = 1;
@@ -45,11 +40,4 @@ void pci_swiotlb_init(void)
 		swiotlb_init();
 		dma_ops = &swiotlb_dma_ops;
 	}
-#else
-	swiotlb_init();
-	if (swiotlb) {
-		printk(KERN_INFO "PCI-DMA: Using software bounce buffering for IO (SWIOTLB)\n");
-		dma_ops = &swiotlb_dma_ops;
-	}
-#endif
 }
diff -r 06d53b6b5fcf linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/dma-mapping.h
--- a/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/dma-mapping.h	Mon Feb 26 15:52:16 2007 -0600
+++ b/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/dma-mapping.h	Mon Feb 26 15:58:43 2007 -0600
@@ -62,7 +62,13 @@ static inline int valid_dma_direction(in
 		(dma_direction == DMA_FROM_DEVICE));
 }
 
-#if 0
+#ifndef CONFIG_XEN
+#define global_need_iommu() (end_pfn > MAX_DMA32_PFN)
+#else
+#include <xen/interface/memory.h>
+#define global_need_iommu() (HYPERVISOR_memory_op(XENMEM_maximum_ram_page, NULL)>MAX_DMA32_PFN)
+#endif
+
 static inline int dma_mapping_error(dma_addr_t dma_addr)
 {
 	if (dma_ops->mapping_error)
@@ -200,8 +206,23 @@ dma_cache_sync(void *vaddr, size_t size,
 
 extern struct device fallback_dev;
 extern int panic_on_overflow;
-#endif
+
+static inline int
+address_needs_mapping(struct device *hwdev, dma_addr_t addr)
+{
+	dma_addr_t mask = 0xffffffff;
+	/* If the device has a mask, use it, otherwise default to 32 bits */
+	if (hwdev && hwdev->dma_mask)
+		mask = *hwdev->dma_mask;
+	return (addr & ~mask) != 0;
+}
+
+static inline int
+range_straddles_page_boundary(void *p, size_t size)
+{
+	extern unsigned long *contiguous_bitmap;
+	return (((((unsigned long)p & ~PAGE_MASK) + size) > PAGE_SIZE) &&
+		!test_bit(__pa(p) >> PAGE_SHIFT, contiguous_bitmap));
+}
 
 #endif /* _X8664_DMA_MAPPING_H */
-
-#include <asm-i386/mach-xen/asm/dma-mapping.h>
diff -r 06d53b6b5fcf linux-2.6-xen-sparse/lib/Makefile
--- a/linux-2.6-xen-sparse/lib/Makefile	Mon Feb 26 15:52:16 2007 -0600
+++ b/linux-2.6-xen-sparse/lib/Makefile	Mon Feb 26 15:58:43 2007 -0600
@@ -52,7 +52,6 @@ obj-$(CONFIG_AUDIT_GENERIC) += audit.o
 obj-$(CONFIG_AUDIT_GENERIC) += audit.o
 
 obj-$(CONFIG_SWIOTLB) += swiotlb.o
-swiotlb-$(CONFIG_XEN) := ../arch/i386/kernel/swiotlb.o
 
 hostprogs-y	:= gen_crc32table
 clean-files	:= crc32table.h
@@ -64,3 +63,10 @@ quiet_cmd_crc32 = GEN     $@
 
 $(obj)/crc32table.h: $(obj)/gen_crc32table
 	$(call cmd,crc32)
+
+ifdef CONFIG_XEN
+include $(srctree)/scripts/Makefile.xen
+
+obj-y := $(call cherrypickxen, $(obj-y))
+endif
+
diff -r 06d53b6b5fcf linux-2.6-xen-sparse/lib/swiotlb-xen.c
--- a/linux-2.6-xen-sparse/lib/swiotlb-xen.c	Mon Feb 26 15:52:16 2007 -0600
+++ b/linux-2.6-xen-sparse/lib/swiotlb-xen.c	Mon Feb 26 15:58:43 2007 -0600
@@ -27,8 +27,10 @@
 #include <asm/uaccess.h>
 #include <xen/interface/memory.h>
 
+#ifndef CONFIG_X86_64_XEN
 int swiotlb;
 EXPORT_SYMBOL(swiotlb);
+#endif
 
 #define OFFSET(val,align) ((unsigned long)((val) & ( (align) - 1)))
 
@@ -691,6 +693,70 @@ swiotlb_dma_mapping_error(dma_addr_t dma
 	return (dma_addr == virt_to_bus(io_tlb_overflow_buffer));
 }
 
+void *
+swiotlb_alloc_coherent(struct device *hwdev, size_t size,
+			dma_addr_t *dma_handle, gfp_t flags)
+{
+	unsigned long dev_addr;
+	void *ret;
+	int order = get_order(size);
+
+	/*
+	 * XXX fix me: the DMA API should pass us an explicit DMA mask
+	 * instead, or use ZONE_DMA32 (ia64 overloads ZONE_DMA to be a ~32
+	 * bit range instead of a 16MB one).
+	 */
+	flags |= GFP_DMA;
+
+	ret = (void *)__get_free_pages(flags, order);
+	if (ret && address_needs_mapping(hwdev, virt_to_bus(ret))) {
+		/*
+		 * The allocated memory isn't reachable by the device.
+		 * Fall back on swiotlb_map_single().
+		 */
+		free_pages((unsigned long) ret, order);
+		ret = NULL;
+	}
+	if (!ret) {
+		/*
+		 * We are either out of memory or the device can't DMA
+		 * to GFP_DMA memory; fall back on
+		 * swiotlb_map_single(), which will grab memory from
+		 * the lowest available address range.
+		 */
+		dma_addr_t handle;
+		handle = swiotlb_map_single(NULL, NULL, size, DMA_FROM_DEVICE);
+		if (swiotlb_dma_mapping_error(handle))
+			return NULL;
+
+		ret = phys_to_virt(handle);
+	}
+
+	memset(ret, 0, size);
+	dev_addr = virt_to_bus(ret);
+
+	/* Confirm address can be DMA'd by device */
+	if (address_needs_mapping(hwdev, dev_addr)) {
+		printk("hwdev DMA mask = 0x%016Lx, dev_addr = 0x%016lx\n",
+			(unsigned long long)*hwdev->dma_mask, dev_addr);
+		panic("swiotlb_alloc_coherent: allocated memory is out of "
+			"range for device");
+	}
+	*dma_handle = dev_addr;
+	return ret;
+}
+
+void
+swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
+			dma_addr_t dma_handle)
+{
+	if (in_swiotlb_aperture((dma_addr_t) vaddr))
+		free_pages((unsigned long) vaddr, get_order(size));
+	else
+		/* DMA_TO_DEVICE to avoid memcpy in unmap_single */
+		swiotlb_unmap_single (hwdev, dma_handle, size, DMA_TO_DEVICE);
+}
+
 /*
  * Return whether the given PCI device DMA address mask can be supported
  * properly.  For example, if your device can only drive the low 24-bits
@@ -701,6 +767,32 @@ swiotlb_dma_supported (struct device *hw
 swiotlb_dma_supported (struct device *hwdev, u64 mask)
 {
 	return (mask >= ((1UL << dma_bits) - 1));
+}
+
+static inline void
+swiotlb_sync_single_range(struct device *hwdev, dma_addr_t dev_addr,
+				unsigned long offset, size_t size,
+				int dir)
+{
+	char *dma_addr = phys_to_virt(dev_addr) + offset;
+
+	BUG_ON(dir == DMA_NONE);
+	if (in_swiotlb_aperture(dma_addr))
+		sync_single(hwdev, dma_addr, size, dir);
+}
+
+void
+swiotlb_sync_single_range_for_cpu(struct device *hwdev, dma_addr_t dev_addr,
+				unsigned long offset, size_t size, int dir)
+{
+	swiotlb_sync_single_range(hwdev, dev_addr, offset, size, dir);
+}
+
+void
+swiotlb_sync_single_range_for_device(struct device *hwdev, dma_addr_t dev_addr,
+				unsigned long offset, size_t size, int dir)
+{
+	swiotlb_sync_single_range(hwdev, dev_addr, offset, size, dir);
 }
 
 EXPORT_SYMBOL(swiotlb_init);
@@ -710,7 +802,11 @@ EXPORT_SYMBOL(swiotlb_unmap_sg);
 EXPORT_SYMBOL(swiotlb_unmap_sg);
 EXPORT_SYMBOL(swiotlb_sync_single_for_cpu);
 EXPORT_SYMBOL(swiotlb_sync_single_for_device);
+EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_cpu);
+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);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Retry 3: Use i386 swiotlb code in lib/swiotlb-xen.c [2/2]

[Jan Beulich]

>>> "Langsdorf, Mark" <mark.langsdorf@amd.com> 26.02.07 21:37 >>>
>Move the arch/i386/kernel/swiotlb.c code to lib/swiotlb-xen.c
>code in order to simplify maintenance of Xen in the future.
>
>The first patch simply moves the code to lib/swiotlb-xen.c;

Without the lib/Makefile adjustment the first patch can't work without
the second one.

>the second patch adds the necessary changes to the Xen code
>base to allow x86_64 systems to boot with SWIOTLB and the
>dma_ops framework.

>--- a/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-dma-xen.c	Mon Feb 26 15:52:16 2007 -0600
>+++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-dma-xen.c	Mon Feb 26 15:58:43 2007 -0600
>...
>+#include <xen/balloon.h>
>+#include <asm/tlbflush.h>

Why xen/balloon.h? asm/hypervisor.h is what declares
xen_{create,destroy}_contiguous_region().

>...
>+		/* Hardcode 31 address bits for now: aacraid limitation. */
>+		if (xen_create_contiguous_region((unsigned long)memory, get_order(size), fls64(dma_mask)) != 0) {
>+			free_pages((unsigned long)memory, get_order(size));
>+			return NULL;
>+		}
>+

Could we also get rid of the (now wrong) comment?

>--- a/linux-2.6-xen-sparse/lib/swiotlb-xen.c	Mon Feb 26 15:52:16 2007 -0600
>+++ b/linux-2.6-xen-sparse/lib/swiotlb-xen.c	Mon Feb 26 15:58:43 2007 -0600
>...
>+void
>+swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>+			dma_addr_t dma_handle)
>+{
>+	if (in_swiotlb_aperture((dma_addr_t) vaddr))
>+		free_pages((unsigned long) vaddr, get_order(size));
>+	else
>+		/* DMA_TO_DEVICE to avoid memcpy in unmap_single */
>+		swiotlb_unmap_single (hwdev, dma_handle, size, DMA_TO_DEVICE);
>+}

I'm pretty certain this is wrong: dma_handle is what should be passed
to in_swiotlb_aperture().

>The first patch also copies the standard 
>arch/x86_64/kernel/pci-dma.c to arch/x86_64/kernel/pci-dma-xen.c,
>which should make reviewing the second patch a little easier.

While the second patch assumes the first patch does what you describe,
the first patch really just moves swiotlb.c.

Jan

RE: [PATCH] Retry 3: Use i386 swiotlb code in lib/swiotlb-xen.c [2/2]

[Langsdorf, Mark]

> >>> "Langsdorf, Mark" <mark.langsdorf@amd.com> 26.02.07 21:37 >>>
> >Move the arch/i386/kernel/swiotlb.c code to lib/swiotlb-xen.c
> >code in order to simplify maintenance of Xen in the future.
> >
> >The first patch simply moves the code to lib/swiotlb-xen.c;
> 
> Without the lib/Makefile adjustment the first patch can't work without
> the second one.

Okay.
 
> >--- a/linux-2.6-xen-sparse/lib/swiotlb-xen.c	Mon Feb 26 
> 15:52:16 2007 -0600
> >+++ b/linux-2.6-xen-sparse/lib/swiotlb-xen.c	Mon Feb 26 
> 15:58:43 2007 -0600
> >...
> >+void
> >+swiotlb_free_coherent(struct device *hwdev, size_t size, 
> void *vaddr,
> >+			dma_addr_t dma_handle)
> >+{
> >+	if (in_swiotlb_aperture((dma_addr_t) vaddr))
> >+		free_pages((unsigned long) vaddr, get_order(size));
> >+	else
> >+		/* DMA_TO_DEVICE to avoid memcpy in unmap_single */
> >+		swiotlb_unmap_single (hwdev, dma_handle, size, 
> DMA_TO_DEVICE);
> >+}
> 
> I'm pretty certain this is wrong: dma_handle is what should be passed
> to in_swiotlb_aperture().

The original code is 
	if (!(vaddr >= (void *)io_tlb_start) && vaddr < (void *)
io_tlb_end))
		free_pages((unsigned long) vaddr, get_order(size));

I can't see why I wouldn't check the vaddr address in Xen, also.

-Mark Langsdorf
AMD, Inc.

Re: [PATCH] Retry 3: Use i386 swiotlb code in lib/swiotlb-xen.c [2/2]

[Keir Fraser]

On 27/2/07 20:30, "Langsdorf, Mark" <mark.langsdorf@amd.com> wrote:

>> I'm pretty certain this is wrong: dma_handle is what should be passed
>> to in_swiotlb_aperture().
> 
> The original code is
> if (!(vaddr >= (void *)io_tlb_start) && vaddr < (void *)
> io_tlb_end))
> free_pages((unsigned long) vaddr, get_order(size));
> 
> I can't see why I wouldn't check the vaddr address in Xen, also.

You can either use the vaddr check from original lib/swiotlb.c *or* you can
pass the dma_handle to in_swiotlb_aperture(). You cannot pass a vaddr to
in_swiotlb_aperture(): it makes no sense!

Actually I think the dma_alloc_coherent() you've hauled in from native
x86/64 code won't even work on Xen as it is. The dma_alloc_pages() function
it uses first won't guarantee to return contiguous memory on Xen, but that
is implicitly assumed by the caller.

I'm afraid various rather serious niggles like these make me wary of
applying these patches until they've been very thoroughly audited.

 -- Keir

Re: [PATCH] Retry 3: Use i386 swiotlb code in lib/swiotlb-xen.c [2/2]

[Keir Fraser]

On 27/2/07 20:49, "Keir Fraser" <Keir.Fraser@cl.cam.ac.uk> wrote:

> You can either use the vaddr check from original lib/swiotlb.c *or* you can
> pass the dma_handle to in_swiotlb_aperture(). You cannot pass a vaddr to
> in_swiotlb_aperture(): it makes no sense!
> 
> Actually I think the dma_alloc_coherent() you've hauled in from native
> x86/64 code won't even work on Xen as it is. The dma_alloc_pages() function
> it uses first won't guarantee to return contiguous memory on Xen, but that
> is implicitly assumed by the caller.

Perhaps it would be best to focus on just the changes required to meet your
main objective, which (I think?) is to move Xen x86/64 over to dma_ops so
that you can conveniently slide the AMD GART implementation in place of the
swiotlb. If this is the case, relocating swiotlb.c to lib/swiotlb-xen.c and
doing a halfway merge with lib/swiotlb.c is not really on the critical path.
Moreover, Jan has some patches pending for mainline Linux which will change
things around in that area anyway if they get accepted. Now is probably not
the time to churn the swiotlb.c code.

Instead can you not just define a swiotlb-based dma_mapping_ops structure in
a suitable arch/x86_64 file (e.g., pci-swiotlb-xen.c would be an obvious
place) and then work on moving us towards using dma_ops hooks for all the
dma-mapping operations. You could do this latter step piecemeal across a
number of patches if you like (i.e., so that intermediate steps some dma
operations are using the dma_ops hooks while others are still statically
falling back to calling into swiotlb code directly). Probably you'd start
off taking a copy of i386/kernel/pci-dma-xen.c into x86_64, and then
progressively pull in dma_ops logic from x86_64/kernel/pci_dma.c. At the
same time you'd be progressively reintroducing dma_ops usage into
mach-xen/asm/dma-mapping.h as well.

 -- Keir