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

["Jan Beulich" <jbeulich@xxxxxxxxxx>]

>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