Xen devel - Mar 2007 - [PATCH] Make AMD GART work as a mini IOMMU [4/4]

Every AMD processor has an AGP aperture and 
Graphics Address Transalation Table (GART) built
into its Northbridge.  The aperture is a
physically contiguous range of memory addresses
under 0xFFFFFFFF that devices can write to; the
GART is a table that translates GART addresses 
into defined virtual addresses.  A DMA device 
can transfer data through the aperture using
32-bit accesses and have the processor automatically
transfer the data to system memory above 4GB without
needing to use software bounce buffering.  On
systems using 32-bit DMA devices, it can speed 
up I/O performance by 15-20% or more.

This patch set enables the AGP aperture and GART
in Xen dom0.  In order to do that easily and 
without interfering with the swiotlb code, it
enables dma_ops for dom0 guests and modifies 
the x86_64 pci-swiotlb-xen.c code to work with
dma_ops.

The first patch creates the 
arch/x86_64/kernel/pci-dma-xen.c file based on the 
standard pci-dma.c.

The second patch modifies 
arch/x86_64/kernel/pci-swiotlb-xen.c to work with 
dma_ops, and modifies arch/x86_64/kernel/pci-dma-xen.c
slightly to be Xen-safe by copying over some functions
from arch/i386/kernel/pci-dma-xen.c.

The third patch creates the 
arch/x86_64/kernel/pci-gart-xen.c and
arch/x86_64/kernel/aperture-xen.c files based on the
standard pci-gart.c and aperture.c files.

The fourth patch modifies pci-gart-xen.c and 
aperture-xen.c to work with Xen.  It adds a
hypervisor call to clear the aperture address
range from the hypervisor''s memory tables,
which is necessary to avoid a cache coherency
issue, and ups the range of contiguous memory
that Xen provides, which is necessary since
the aperture must be at least 32 MB.


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

>The fourth patch modifies pci-gart-xen.c and 
>aperture-xen.c to work with Xen.  It adds a
>hypervisor call to clear the aperture address
>range from the hypervisor''s memory tables,
>which is necessary to avoid a cache coherency
>issue, and ups the range of contiguous memory
>that Xen provides, which is necessary since
>the aperture must be at least 32 MB.
>--- a/linux-2.6-xen-sparse/arch/i386/mm/hypervisor.c	Thu Mar 01 15:04:47
2007 -0600
>+++ b/linux-2.6-xen-sparse/arch/i386/mm/hypervisor.c	Mon Mar 05 16:01:11
2007 -0600
>@@ -252,7 +252,7 @@ static void contiguous_bitmap_clear(
> }
> 
> /* Protected by balloon_lock. */
>-#define MAX_CONTIG_ORDER 9 /* 2MB */
>+#define MAX_CONTIG_ORDER 16 /* 256MB */
> static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
> static multicall_entry_t cr_mcl[1<<MAX_CONTIG_ORDER];
 
Keir had asked you to not statically bump MAX_CONTIG_ORDER here as a
minimal acceptance criteria. 
>--- a/linux-2.6-xen-sparse/arch/x86_64/kernel/aperture-xen.c	Thu Mar 01
15:04:47 2007 -0600
>+++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/aperture-xen.c	Mon Mar 05
16:01:11 2007 -0600
>@@ -53,7 +53,7 @@ static u32 __init allocate_aperture(void
> 	 * IOMMU useless.
> 	 */
> 	p = __alloc_bootmem_node(nd0, aper_size, aper_size, 0); 
>-	if (!p || __pa(p)+aper_size > 0xffffffff) {
>+	if (!p || (xen_create_contiguous_region((unsigned long) p,
get_order(aper_size), 31) != 0) ||
virt_to_bus(p)+aper_size >
>0xffffffff) {
> 		printk("Cannot allocate aperture memory hole (%p,%uK)\n",
> 		       p, aper_size>>10);
> 		if (p)
Why 31 when native code uses a check for 32 bits?
Also, I don''t think the range check is necessary after the call to
xen_create_contiguous_region() - if at all you should BUG() if the
call was successful but the resulting address isn''t fitting.
>--- a/linux-2.6-xen-sparse/arch/x86_64/kernel/e820-xen.c	Thu Mar 01 15:04:47
2007 -0600
>+++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/e820-xen.c	Mon Mar 05 16:01:11
2007 -0600
>@@ -97,7 +97,6 @@ static inline int bad_addr(unsigned long
> 	return 0;
> } 
> 
>-#ifndef CONFIG_XEN
> /*
>  * This function checks if any part of the range <start,end> is
mapped
>  * with type.
>@@ -116,7 +115,6 @@ e820_any_mapped(unsigned long start, uns
> 	} 
> 	return 0;
> }
>-#endif
> 
> /*
>  * This function checks if the entire range <start,end> is mapped
with type.
Simply enabling this function is insufficient: It must check against
machine_e820
for Xen (see e820_all_mapped), which is populated only under
is_initial_xen_domain() (though that''s not a problem, as domU-s
shouldn''t find
any AGP bridge in the first place).
>--- a/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-gart-xen.c	Thu Mar 01
15:04:47 2007 -0600
>+++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/pci-gart-xen.c	Mon Mar 05
16:01:11 2007 -0600
>...
>@@ -523,6 +523,9 @@ static __init int init_k8_gatt(struct ag
> 	gatt = (void *)__get_free_pages(GFP_KERNEL, get_order(gatt_size)); 
> 	if (!gatt) 
> 		panic("Cannot allocate GATT table"); 
>+	if (xen_create_contiguous_region((unsigned long) gatt,
get_order(gatt_size), 31))
>+		panic("Cannot create a contiguous memory region below 4GB for the
GATT table");
>+
> 	memset(gatt, 0, gatt_size); 
> 	agp_gatt_table = gatt;
> 
Why 31, and more importantly, why a restriction at all? Native uses GFP_KERNEL
(not GFP_DMA32), so why is there a restriction here under Xen?
>@@ -531,7 +534,7 @@ static __init int init_k8_gatt(struct ag
> 		u32 gatt_reg; 
> 
> 		dev = k8_northbridges[i];
>-		gatt_reg = __pa(gatt) >> 12; 
>+		gatt_reg = phys_to_machine(__pa(gatt)) >> 12; 
> 		gatt_reg <<= 4; 
> 		pci_write_config_dword(dev, 0x98, gatt_reg);
> 		pci_read_config_dword(dev, 0x90, &ctl); 
Wouldn''t virt_to_bus() be more appropriate/consistent here?
>--- a/linux-2.6-xen-sparse/arch/x86_64/mm/init-xen.c	Thu Mar 01 15:04:47
2007 -0600
>+++ b/linux-2.6-xen-sparse/arch/x86_64/mm/init-xen.c	Mon Mar 05 16:01:11
2007 -0600
>...
>@@ -855,13 +856,10 @@ void __init clear_kernel_mapping(unsigne
> 		pmd = pmd_offset(pud, address);
> 		if (!pmd || pmd_none(*pmd))
> 			continue; 
>-		if (0 == (pmd_val(*pmd) & _PAGE_PSE)) { 
>-			/* Could handle this, but it should not happen currently. */
>-			printk(KERN_ERR 
>-	       "clear_kernel_mapping: mapping has been split. will leak
memory\n");
>-			pmd_ERROR(*pmd); 
>-		}
>-		set_pmd(pmd, __pmd(0)); 		
>+		pte = pte_offset_map(pmd, address);
>+		if (!pte || pte_none(*pte))                                     
>+			continue;
>+		pte_clear(&init_mm,address,pte);
I''d prefer keeping the original code by means of a #ifndef CONFIG_XEN
construct.
>--- a/xen/arch/x86/mm.c	Thu Mar 01 15:04:47 2007 -0600
>+++ b/xen/arch/x86/mm.c	Mon Mar 05 16:01:11 2007 -0600
>@@ -2098,6 +2098,13 @@ int do_mmuext_op(
>                 }
>             }
>             break;
>+
>+	case MMUEXT_UNMAP_REGION:
>+		map_pages_to_xen(
>+			PAGE_OFFSET + (op.arg1.mfn << PAGE_SHIFT),
>+			op.arg1.mfn, op.arg2.nr_ents, PAGE_HYPERVISOR &
>+			~_PAGE_PRESENT);
>+	    break;
> #endif
>         
>         case MMUEXT_TLB_FLUSH_LOCAL:
This must be refused for non-privileged domains. I would also think that it
should be verified that the pages are not in use elsewhere.
Finally, hypervisor sources use 4-space indentation, not tabs.

Jan

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

On 6/3/07 09:22, "Jan Beulich" <jbeulich@novell.com> wrote:
>> /* Protected by balloon_lock. */
>> -#define MAX_CONTIG_ORDER 9 /* 2MB */
>> +#define MAX_CONTIG_ORDER 16 /* 256MB */
>> static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
>> static multicall_entry_t cr_mcl[1<<MAX_CONTIG_ORDER];
>  
> Keir had asked you to not statically bump MAX_CONTIG_ORDER here as a
> minimal acceptance criteria.
Indeed. In fact I think the right answer here is probably to prove a new
steal-memory platform operation which will allocate a chunk of RAM, unmap it
in the hypervisor, and mark it as belonging to the I/O domain. The
clear_kernel_mapping() call is then not needed if the aperture was allocated
by the BIOS (since it will be outside any published RAM area, so Xen will
have carefully not mapped that range) and with a new platform op for the
allocation will not be needed in the fallback case either. Nor would
xen_create_contiguous_region() need to be modified.

Whatever, with 3.0.5 just a few weeks away I don''t think a complex
patchset
this far from completion has a chance of making it. It''s way too scary.

 -- Keir


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