making changes to agp code?

making changes to agp code?

[Langsdorf, Mark]

As part of my endless quest to enable GART/IOMMU, I
realized I need to make a slight change to a static
function inside of agp-amd64.c.  Currently Xen doesn't
have -xen variants of the AGP code.  Is there a 
better way to handle this than sucking in the entire
AGP tree into xen-sparse?

As far what I need to change:
   pci-gart calls agp_amd64_init() to determine if
the aperture is provided by the BIOS, or if one 
needs to be allocated.  agp_amd64_init() calls
agp_amd64_probe() which calls another function
and so forth, and eventually aperture_valid()
calls 
PageReserved(pfn_to_page(aperture >> PAGE_SHIFT)).
The page isn't actually reserved, but dom0 thinks
it is, and the operation fails.  I would like to
do something more intelligent.

-Mark Langsdorf
Operating Systems Research Center
AMD, Inc.

Re: making changes to agp code?

[Jan Beulich]

>>> "Langsdorf, Mark" <mark.langsdorf@amd.com> 26.03.07 22:03 >>>
>As part of my endless quest to enable GART/IOMMU, I
>realized I need to make a slight change to a static
>function inside of agp-amd64.c.  Currently Xen doesn't
>have -xen variants of the AGP code.  Is there a 
>better way to handle this than sucking in the entire
>AGP tree into xen-sparse?

If the change is as small as you describe, I'd suggest doing it in the
file itself by means of adding a patch in patches/linux-2.6.18/, with
the change properly protected by #ifdef CONFIG_XEN or alike.

>As far what I need to change:
>   pci-gart calls agp_amd64_init() to determine if
>the aperture is provided by the BIOS, or if one 
>needs to be allocated.  agp_amd64_init() calls
>agp_amd64_probe() which calls another function
>and so forth, and eventually aperture_valid()
>calls 
>PageReserved(pfn_to_page(aperture >> PAGE_SHIFT)).
>The page isn't actually reserved, but dom0 thinks
>it is, and the operation fails.  I would like to
>do something more intelligent.

>From that description I'm getting afraid that this code is currently
broken anyway, i.e. the change you intend to make is needed
immediately and regardless of your iommu work. May I ask what
your intended replacement is?

Jan

Re: making changes to agp code?

[Jan Beulich]

>>> "Langsdorf, Mark" <mark.langsdorf@amd.com> 26.03.07 22:03 >>>
>As part of my endless quest to enable GART/IOMMU, I
>realized I need to make a slight change to a static
>function inside of agp-amd64.c.  Currently Xen doesn't
>have -xen variants of the AGP code.  Is there a 
>better way to handle this than sucking in the entire
>AGP tree into xen-sparse?
>
>As far what I need to change:
>   pci-gart calls agp_amd64_init() to determine if
>the aperture is provided by the BIOS, or if one 
>needs to be allocated.  agp_amd64_init() calls
>agp_amd64_probe() which calls another function
>and so forth, and eventually aperture_valid()
>calls 
>PageReserved(pfn_to_page(aperture >> PAGE_SHIFT)).
>The page isn't actually reserved, but dom0 thinks
>it is, and the operation fails.  I would like to
>do something more intelligent.

On a second look I believe the implementation is broken even on native,
as long as !CONFIG_FLATMEM, since there's an assumption that an
invalid PFN cannot be followed by a valid one. For that reason, I think
the code needs to be changed to call e820_any_mapped() (just like
aperture.c does). I have a tentative patch to do that, but don't have
a working box with an 8151.

Jan

RE: making changes to agp code?

[Langsdorf, Mark]

> >>> "Langsdorf, Mark" <mark.langsdorf@amd.com> 26.03.07 22:03 >>>
> >As part of my endless quest to enable GART/IOMMU, I
> >realized I need to make a slight change to a static
> >function inside of agp-amd64.c.  Currently Xen doesn't
> >have -xen variants of the AGP code.  Is there a 
> >better way to handle this than sucking in the entire
> >AGP tree into xen-sparse?
> >
> >As far what I need to change:
> >   pci-gart calls agp_amd64_init() to determine if
> >the aperture is provided by the BIOS, or if one 
> >needs to be allocated.  agp_amd64_init() calls
> >agp_amd64_probe() which calls another function
> >and so forth, and eventually aperture_valid()
> >calls 
> >PageReserved(pfn_to_page(aperture >> PAGE_SHIFT)).
> >The page isn't actually reserved, but dom0 thinks
> >it is, and the operation fails.  I would like to
> >do something more intelligent.
> 
> On a second look I believe the implementation is broken even 
> on native, as long as !CONFIG_FLATMEM, since there's an
> assumption that an invalid PFN cannot be followed by a valid
> one. For that reason, I think the code needs to be changed to
> call e820_any_mapped() (just like aperture.c does). I have a
> tentative patch to do that, but don't have a working box with
> an 8151.

I do.  You can send it to me for testing.

I was playing with that box yesterday, and I discovered
that Xen allocates guest virtual memory over the AGP
aperture if dom0 memory is greater than 4G, even though
the e820 map says it shouldn't.  I didn't have a lot of
spare cycles yesterday to deal with the implications of
that, and maybe it can be safely ignored.  Any thoughts?

-Mark Langsdorf
AMD, Inc. 

Re: making changes to agp code?

[Keir Fraser]

On 28/3/07 16:21, "Langsdorf, Mark" <mark.langsdorf@amd.com> wrote:

> I was playing with that box yesterday, and I discovered
> that Xen allocates guest virtual memory over the AGP
> aperture if dom0 memory is greater than 4G, even though
> the e820 map says it shouldn't.  I didn't have a lot of
> spare cycles yesterday to deal with the implications of
> that, and maybe it can be safely ignored.  Any thoughts?

What exactly do you mean? That we allocate pages from the physical address
range that includes the AGP aperture, even though this range is not marked
as RAM in the e820 map? That would be a very bad bug.

 -- Keir

Re: making changes to agp code?

[Jan Beulich]

>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 28.03.07 17:43 >>>
>On 28/3/07 16:21, "Langsdorf, Mark" <mark.langsdorf@amd.com> wrote:
>
>> I was playing with that box yesterday, and I discovered
>> that Xen allocates guest virtual memory over the AGP
>> aperture if dom0 memory is greater than 4G, even though
>> the e820 map says it shouldn't.  I didn't have a lot of
>> spare cycles yesterday to deal with the implications of
>> that, and maybe it can be safely ignored.  Any thoughts?
>
>What exactly do you mean? That we allocate pages from the physical address
>range that includes the AGP aperture, even though this range is not marked
>as RAM in the e820 map? That would be a very bad bug.

I think he's seeing PFNs used that match the MFNs of the aperture,
which is exactly the bug I was referring to in my first reply - by
switching the whole thing to use e820_any_mapped(), this problem
would be gone. Otherwise, #idef CONFIG_XEN code will be needed
in there.

Jan

RE: making changes to agp code?

[Jan Beulich]

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

>> On a second look I believe the implementation is broken even 
>> on native, as long as !CONFIG_FLATMEM, since there's an
>> assumption that an invalid PFN cannot be followed by a valid
>> one. For that reason, I think the code needs to be changed to
>> call e820_any_mapped() (just like aperture.c does). I have a
>> tentative patch to do that, but don't have a working box with
>> an 8151.
>
>I do.  You can send it to me for testing.

Attached - depending on what tree you want to apply it on you
may have to tweak it a little.

Jan

[-- Attachment #2: xen-amd64-agp.patch --]
[-- Type: text/plain, Size: 6782 bytes --]

From: jbeulich@novell.com
Subject: fix amd64-agp aperture validation
Patch-mainline: obsolete

Under Xen, pfn_valid() on a machine address makes no sense. But even on
native, under CONFIG_DISCONTIGMEM, assuming that a !pfn_valid() implies
all subsequent pfn-s are also invalid is wrong. Thus replace this by
explicitly checking against the E820 map.

Index: head-2007-03-19/arch/i386/kernel/e820-xen.c
===================================================================
--- head-2007-03-19.orig/arch/i386/kernel/e820-xen.c	2007-03-21 13:41:48.000000000 +0100
+++ head-2007-03-19/arch/i386/kernel/e820-xen.c	2007-03-28 15:32:08.000000000 +0200
@@ -249,7 +249,7 @@ static void __init probe_roms(void)
 }
 
 #ifdef CONFIG_XEN
-static struct e820map machine_e820 __initdata;
+static struct e820map machine_e820;
 #define e820 machine_e820
 #endif
 
@@ -887,6 +887,35 @@ void __init limit_regions(unsigned long 
 	print_memory_map("limit_regions endfunc");
 }
 
+/*
+ * This function checks if any part of the range <start,end> is mapped
+ * with type.
+ */
+int
+e820_any_mapped(u64 start, u64 end, unsigned type)
+{
+	int i;
+
+#ifndef CONFIG_XEN
+	for (i = 0; i < e820.nr_map; i++) {
+		const struct e820entry *ei = &e820.map[i];
+#else
+	if (!is_initial_xendomain())
+		return 0;
+	for (i = 0; i < machine_e820.nr_map; ++i) {
+		const struct e820entry *ei = &machine_e820.map[i];
+#endif
+
+		if (type && ei->type != type)
+			continue;
+		if (ei->addr >= end || ei->addr + ei->size <= start)
+			continue;
+		return 1;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(e820_any_mapped);
+
  /*
   * This function checks if the entire range <start,end> is mapped with type.
   *
Index: head-2007-03-19/arch/i386/kernel/e820.c
===================================================================
--- head-2007-03-19.orig/arch/i386/kernel/e820.c	2007-03-19 14:15:28.000000000 +0100
+++ head-2007-03-19/arch/i386/kernel/e820.c	2007-03-28 15:32:24.000000000 +0200
@@ -818,6 +818,26 @@ void __init limit_regions(unsigned long 
 	print_memory_map("limit_regions endfunc");
 }
 
+/*
+ * This function checks if any part of the range <start,end> is mapped
+ * with type.
+ */
+int
+e820_any_mapped(u64 start, u64 end, unsigned type)
+{
+	int i;
+	for (i = 0; i < e820.nr_map; i++) {
+		const struct e820entry *ei = &e820.map[i];
+		if (type && ei->type != type)
+			continue;
+		if (ei->addr >= end || ei->addr + ei->size <= start)
+			continue;
+		return 1;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(e820_any_mapped);
+
  /*
   * This function checks if the entire range <start,end> is mapped with type.
   *
Index: head-2007-03-19/arch/x86_64/kernel/e820-xen.c
===================================================================
--- head-2007-03-19.orig/arch/x86_64/kernel/e820-xen.c	2007-03-21 10:49:19.000000000 +0100
+++ head-2007-03-19/arch/x86_64/kernel/e820-xen.c	2007-03-28 15:28:20.000000000 +0200
@@ -28,7 +28,7 @@
 
 struct e820map e820 __initdata;
 #ifdef CONFIG_XEN
-struct e820map machine_e820 __initdata;
+struct e820map machine_e820;
 #endif
 
 /* 
@@ -105,17 +105,25 @@ 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.
  */
-int __meminit
+int
 e820_any_mapped(unsigned long start, unsigned long end, unsigned type)
 { 
 	int i;
+
+#ifndef CONFIG_XEN
 	for (i = 0; i < e820.nr_map; i++) { 
 		struct e820entry *ei = &e820.map[i]; 
+#else
+	if (!is_initial_xendomain())
+		return 0;
+	for (i = 0; i < machine_e820.nr_map; i++) {
+		const struct e820entry *ei = &machine_e820.map[i];
+#endif
+
 		if (type && ei->type != type) 
 			continue;
 		if (ei->addr >= end || ei->addr + ei->size <= start)
@@ -124,7 +132,7 @@ e820_any_mapped(unsigned long start, uns
 	} 
 	return 0;
 }
-#endif
+EXPORT_SYMBOL_GPL(e820_any_mapped);
 
 /*
  * This function checks if the entire range <start,end> is mapped with type.
Index: head-2007-03-19/arch/x86_64/kernel/e820.c
===================================================================
--- head-2007-03-19.orig/arch/x86_64/kernel/e820.c	2007-03-19 14:16:32.000000000 +0100
+++ head-2007-03-19/arch/x86_64/kernel/e820.c	2007-03-28 15:26:08.000000000 +0200
@@ -98,7 +98,7 @@ static inline int bad_addr(unsigned long
  * This function checks if any part of the range <start,end> is mapped
  * with type.
  */
-int __meminit
+int
 e820_any_mapped(unsigned long start, unsigned long end, unsigned type)
 { 
 	int i;
@@ -112,6 +112,7 @@ e820_any_mapped(unsigned long start, uns
 	} 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(e820_any_mapped);
 
 /*
  * This function checks if the entire range <start,end> is mapped with type.
Index: head-2007-03-19/drivers/char/agp/amd64-agp.c
===================================================================
--- head-2007-03-19.orig/drivers/char/agp/amd64-agp.c	2007-03-19 14:15:43.000000000 +0100
+++ head-2007-03-19/drivers/char/agp/amd64-agp.c	2007-03-28 15:20:49.000000000 +0200
@@ -14,6 +14,7 @@
 #include <linux/agp_backend.h>
 #include <linux/mmzone.h>
 #include <asm/page.h>		/* PAGE_SIZE */
+#include <asm/e820.h>
 #include <asm/k8.h>
 #include "agp.h"
 
@@ -259,7 +260,6 @@ static const struct agp_bridge_driver am
 /* Some basic sanity checks for the aperture. */
 static int __devinit aperture_valid(u64 aper, u32 size)
 {
-	u32 pfn, c;
 	if (aper == 0) {
 		printk(KERN_ERR PFX "No aperture\n");
 		return 0;
@@ -272,14 +272,9 @@ static int __devinit aperture_valid(u64 
 		printk(KERN_ERR PFX "Aperture out of bounds\n");
 		return 0;
 	}
-	pfn = aper >> PAGE_SHIFT;
-	for (c = 0; c < size/PAGE_SIZE; c++) {
-		if (!pfn_valid(pfn + c))
-			break;
-		if (!PageReserved(pfn_to_page(pfn + c))) {
-			printk(KERN_ERR PFX "Aperture pointing to RAM\n");
-			return 0;
-		}
+	if (e820_any_mapped(aper, aper + size, E820_RAM)) {
+		printk(KERN_ERR PFX "Aperture pointing to RAM\n");
+		return 0;
 	}
 
 	/* Request the Aperture. This catches cases when someone else
Index: head-2007-03-19/include/asm-i386/e820.h
===================================================================
--- head-2007-03-19.orig/include/asm-i386/e820.h	2007-02-04 19:44:54.000000000 +0100
+++ head-2007-03-19/include/asm-i386/e820.h	2007-03-28 15:21:47.000000000 +0200
@@ -38,6 +38,7 @@ extern struct e820map e820;
 
 extern int e820_all_mapped(unsigned long start, unsigned long end,
 			   unsigned type);
+extern int e820_any_mapped(u64 start, u64 end, unsigned type);
 extern void find_max_pfn(void);
 extern void register_bootmem_low_pages(unsigned long max_low_pfn);
 extern void e820_register_memory(void);

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

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

RE: making changes to agp code?

[Langsdorf, Mark]

> >> On a second look I believe the implementation is broken even 
> >> on native, as long as !CONFIG_FLATMEM, since there's an
> >> assumption that an invalid PFN cannot be followed by a valid
> >> one. For that reason, I think the code needs to be changed to
> >> call e820_any_mapped() (just like aperture.c does). I have a
> >> tentative patch to do that, but don't have a working box with
> >> an 8151.
> >
> >I do.  You can send it to me for testing.
> 
> Attached - depending on what tree you want to apply it on you
> may have to tweak it a little.

I applied it to xen-unstable with some tweaking (my version
doesn't seem to have an i386 e820-xen.c ??) and to 2.6.20.

System booted correctly and ran fine.

Acked-by: Mark Langsdorf <mark.langsdorf@amd.com>

-Mark Langsdorf
Operating Systems Research Center
AMD, Inc.

RE: making changes to agp code?

[Jan Beulich]

>>> "Langsdorf, Mark" <mark.langsdorf@amd.com> 29.03.07 00:37 >>>
>> >> On a second look I believe the implementation is broken even 
>> >> on native, as long as !CONFIG_FLATMEM, since there's an
>> >> assumption that an invalid PFN cannot be followed by a valid
>> >> one. For that reason, I think the code needs to be changed to
>> >> call e820_any_mapped() (just like aperture.c does). I have a
>> >> tentative patch to do that, but don't have a working box with
>> >> an 8151.
>> >
>> >I do.  You can send it to me for testing.
>> 
>> Attached - depending on what tree you want to apply it on you
>> may have to tweak it a little.
>
>I applied it to xen-unstable with some tweaking (my version
>doesn't seem to have an i386 e820-xen.c ??) and to 2.6.20.

Yes, i386's e820.c is a 2.6.20 addition, which I followed in our Xen
port. But I assume the patch is not likely to be applied to -unstable
anyway, because of it touching a few files not in the sparse tree
(unless Keir feels otherwise, or if your iommu stuff will have a strict
dependency on this). The primary intention is to post the non-Xen
parts to kernel.org and pick up when it got merged.

>System booted correctly and ran fine.
>
>Acked-by: Mark Langsdorf <mark.langsdorf@amd.com>

Thanks!

Jan