WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: "Mark Langsdorf" <mark.langsdorf@xxxxxxx>
Subject: Re: [Xen-devel] [PATCH] Make AMD GART work as a mini IOMMU [4/4]
From: "Jan Beulich" <jbeulich@xxxxxxxxxx>
Date: Tue, 06 Mar 2007 09:22:54 +0000
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Tue, 06 Mar 2007 01:21:29 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <1449F58C868D8D4E9C72945771150BDFD966B8@xxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1449F58C868D8D4E9C72945771150BDFD966B8@xxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
>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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>