* making changes to agp code? @ 2007-03-26 20:03 Langsdorf, Mark 2007-03-27 6:41 ` Jan Beulich 2007-03-28 14:05 ` Jan Beulich 0 siblings, 2 replies; 9+ messages in thread From: Langsdorf, Mark @ 2007-03-26 20:03 UTC (permalink / raw) To: xen-devel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: making changes to agp code? 2007-03-26 20:03 making changes to agp code? Langsdorf, Mark @ 2007-03-27 6:41 ` Jan Beulich 2007-03-28 14:05 ` Jan Beulich 1 sibling, 0 replies; 9+ messages in thread From: Jan Beulich @ 2007-03-27 6:41 UTC (permalink / raw) To: Mark Langsdorf; +Cc: xen-devel >>> "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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: making changes to agp code? 2007-03-26 20:03 making changes to agp code? Langsdorf, Mark 2007-03-27 6:41 ` Jan Beulich @ 2007-03-28 14:05 ` Jan Beulich 2007-03-28 15:21 ` Langsdorf, Mark 1 sibling, 1 reply; 9+ messages in thread From: Jan Beulich @ 2007-03-28 14:05 UTC (permalink / raw) To: Mark Langsdorf; +Cc: xen-devel >>> "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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: making changes to agp code? 2007-03-28 14:05 ` Jan Beulich @ 2007-03-28 15:21 ` Langsdorf, Mark 2007-03-28 15:43 ` Keir Fraser 2007-03-28 15:55 ` Jan Beulich 0 siblings, 2 replies; 9+ messages in thread From: Langsdorf, Mark @ 2007-03-28 15:21 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel > >>> "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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: making changes to agp code? 2007-03-28 15:21 ` Langsdorf, Mark @ 2007-03-28 15:43 ` Keir Fraser 2007-03-28 15:53 ` Jan Beulich 2007-03-28 15:55 ` Jan Beulich 1 sibling, 1 reply; 9+ messages in thread From: Keir Fraser @ 2007-03-28 15:43 UTC (permalink / raw) To: Langsdorf, Mark, Jan Beulich; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: making changes to agp code? 2007-03-28 15:43 ` Keir Fraser @ 2007-03-28 15:53 ` Jan Beulich 0 siblings, 0 replies; 9+ messages in thread From: Jan Beulich @ 2007-03-28 15:53 UTC (permalink / raw) To: Mark Langsdorf, Keir Fraser; +Cc: xen-devel >>> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: making changes to agp code? 2007-03-28 15:21 ` Langsdorf, Mark 2007-03-28 15:43 ` Keir Fraser @ 2007-03-28 15:55 ` Jan Beulich 2007-03-28 22:37 ` Langsdorf, Mark 1 sibling, 1 reply; 9+ messages in thread From: Jan Beulich @ 2007-03-28 15:55 UTC (permalink / raw) To: Mark Langsdorf; +Cc: xen-devel [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: making changes to agp code? 2007-03-28 15:55 ` Jan Beulich @ 2007-03-28 22:37 ` Langsdorf, Mark 2007-03-29 7:23 ` Jan Beulich 0 siblings, 1 reply; 9+ messages in thread From: Langsdorf, Mark @ 2007-03-28 22:37 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel > >> 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: making changes to agp code? 2007-03-28 22:37 ` Langsdorf, Mark @ 2007-03-29 7:23 ` Jan Beulich 0 siblings, 0 replies; 9+ messages in thread From: Jan Beulich @ 2007-03-29 7:23 UTC (permalink / raw) To: Mark Langsdorf; +Cc: xen-devel >>> "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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2007-03-29 7:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-03-26 20:03 making changes to agp code? Langsdorf, Mark 2007-03-27 6:41 ` Jan Beulich 2007-03-28 14:05 ` Jan Beulich 2007-03-28 15:21 ` Langsdorf, Mark 2007-03-28 15:43 ` Keir Fraser 2007-03-28 15:53 ` Jan Beulich 2007-03-28 15:55 ` Jan Beulich 2007-03-28 22:37 ` Langsdorf, Mark 2007-03-29 7:23 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.