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

["Jan Beulich" <jbeulich@xxxxxxxxxx>]

>>> "Langsdorf, Mark" <mark.langsdorf@xxxxxxx> 23.02.07 23:32 >>>
>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 simplies 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.
>
>Signed-off-by: Mark Langsdorf <mark.langsdorf@xxxxxxx>
>
>--- a/linux-2.6-xen-sparse/lib/Makefile        Fri Feb 23 15:18:00 2007 -0600
>+++ b/linux-2.6-xen-sparse/lib/Makefile        Fri Feb 23 17:51:54 2007 -0600

The whole change to this file would really belong into patch 1 I think. Also, 
patch
1 should remove arch/i386/kernel/swiotlb.c, not just add the new file.

>@@ -64,3 +63,12 @@ quiet_cmd_crc32 = GEN     $@
> 
> $(obj)/crc32table.h: $(obj)/gen_crc32table
>       $(call cmd,crc32)
>+
>+ifdef CONFIG_XEN
>+include $(srctree)/scripts/Makefile.xen
>+
>+obj-y := $(call filterxen, $(obj-y), $(n-obj-xen))
>+obj-y := $(call cherrypickxen, $(obj-y))
>+extra-y := $(call cherrypickxen, $(extra-y))
>+endif
>+

I think the first obj-y := line and the extra-y := line are irrelevant here.

>--- a/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-swiotlb-xen.c        Fri Feb 
>23 15:18:00 2007 -0600
>+++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-swiotlb-xen.c        Fri Feb 
>23 17:51:54 2007 -0600
>...
>-      .sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
>-      .sync_single_range_for_device = swiotlb_sync_single_range_for_device,
>+//    .sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
>+//    .sync_single_range_for_device = swiotlb_sync_single_range_for_device,

If this is really unnecessary, why do you keep it commented? Otherwise, what's
the problem with adding the swiotlb_sync_single_range_for_* functions (which the
merge with lib/swiotlb.c would let us have anyway)?

>--- a/linux-2.6-xen-sparse/lib/swiotlb-xen.c   Fri Feb 23 15:18:00 2007 -0600
>+++ b/linux-2.6-xen-sparse/lib/swiotlb-xen.c   Fri Feb 23 17:51:54 2007 -0600
>...
>@@ -50,6 +52,7 @@ int swiotlb_force;
> int swiotlb_force;
> 
> static char *iotlb_virt_start;
>+unsigned long iotlb_size;
> static unsigned long iotlb_nslabs;
> 
> /*

This new variable seems pointless to me, and moves us farther from native. The
only place it is needed in is

>+void
>+swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>+                      dma_addr_t dma_handle)
>+{
>+      if (!(vaddr >= (void *)iotlb_virt_start
>+                  && vaddr < (void *)(iotlb_virt_start + iotlb_size)))
>+              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);
>+}

which I would think should use in_swiotlb_aperture() just as other places do.

>--- /dev/null  Thu Jan 01 00:00:00 1970 +0000
>+++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-dma-xen.c    Fri Feb 23 
>17:51:54 2007 -0600
>...
>+              /* Hardcode 31 address bits for now: aacraid limitation. */
>+              if (xen_create_contiguous_region((unsigned long)memory, 
>get_order(size), 31) != 0) {
>+                      free_pages((unsigned long)memory, get_order(size));
>+                      return NULL;
>+              }

Please don't re-introduce hardcoded numbers here. i386 does this purely based
on device requirements, and so should x86-64.

Jan

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