* aperture_64: use symbolic constants
@ 2008-05-19 12:39 Pavel Machek
2008-05-20 11:20 ` Andi Kleen
[not found] ` <20080519125425.GD13546@elte.hu>
0 siblings, 2 replies; 9+ messages in thread
From: Pavel Machek @ 2008-05-19 12:39 UTC (permalink / raw)
To: Ingo Molnar, kernel list
Use symbolic constants in aperture_64.c where possible (and make a
comment slightly more helpful).
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
commit 4cc4fa2a3ff78277430201c74580e99bbaf7a1d0
tree 6ceafd1f7c4d020f084e9b96d30619680d393274
parent 033a68acc1817ba2bd9f25c2384e4b0c22ff4288
author Pavel <pavel@amd.ucw.cz> Mon, 19 May 2008 14:38:59 +0200
committer Pavel <pavel@amd.ucw.cz> Mon, 19 May 2008 14:38:59 +0200
arch/x86/kernel/aperture_64.c | 20 +++++++++++---------
1 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
index 479926d..6b3757e 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -263,11 +263,11 @@ void __init early_gart_iommu_check(void)
if (!early_is_k8_nb(read_pci_config(0, num, 3, 0x00)))
continue;
- ctl = read_pci_config(0, num, 3, 0x90);
+ ctl = read_pci_config(0, num, 3, AMD64_GARTAPERTURECTL);
aper_enabled = ctl & 1;
aper_order = (ctl >> 1) & 7;
aper_size = (32 * 1024 * 1024) << aper_order;
- aper_base = read_pci_config(0, num, 3, 0x94) & 0x7fff;
+ aper_base = read_pci_config(0, num, 3, AMD64_GARTAPERTUREBASE) & 0x7fff;
aper_base <<= 25;
if ((last_aper_order && aper_order != last_aper_order) ||
@@ -303,9 +303,9 @@ void __init early_gart_iommu_check(void)
if (!early_is_k8_nb(read_pci_config(0, num, 3, 0x00)))
continue;
- ctl = read_pci_config(0, num, 3, 0x90);
+ ctl = read_pci_config(0, num, 3, AMD64_GARTAPERTURECTL);
ctl &= ~1;
- write_pci_config(0, num, 3, 0x90, ctl);
+ write_pci_config(0, num, 3, AMD64_GARTAPERTURECTL, ctl);
}
}
@@ -332,9 +332,9 @@ void __init gart_iommu_hole_init(void)
iommu_detected = 1;
gart_iommu_aperture = 1;
- aper_order = (read_pci_config(0, num, 3, 0x90) >> 1) & 7;
+ aper_order = (read_pci_config(0, num, 3, AMD64_GARTAPERTURECTL) >> 1) & 7;
aper_size = (32 * 1024 * 1024) << aper_order;
- aper_base = read_pci_config(0, num, 3, 0x94) & 0x7fff;
+ aper_base = read_pci_config(0, num, 3, AMD64_GARTAPERTUREBASE) & 0x7fff;
aper_base <<= 25;
printk(KERN_INFO "Node %d: aperture @ %Lx size %u MB\n",
@@ -406,11 +406,13 @@ void __init gart_iommu_hole_init(void)
continue;
/*
- * Don't enable translation yet. That is done later.
+ * Don't enable translation yet. That is done later
+ * by enable_gart_translation.
+ *
* Assume this BIOS didn't initialise the GART so
* just overwrite all previous bits
*/
- write_pci_config(0, num, 3, 0x90, aper_order<<1);
- write_pci_config(0, num, 3, 0x94, aper_alloc>>25);
+ write_pci_config(0, num, 3, AMD64_GARTAPERTURECTL, aper_order<<1);
+ write_pci_config(0, num, 3, AMD64_GARTAPERTUREBASE, aper_alloc>>25);
}
}
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: aperture_64: use symbolic constants
2008-05-19 12:39 aperture_64: use symbolic constants Pavel Machek
@ 2008-05-20 11:20 ` Andi Kleen
[not found] ` <20080519125425.GD13546@elte.hu>
1 sibling, 0 replies; 9+ messages in thread
From: Andi Kleen @ 2008-05-20 11:20 UTC (permalink / raw)
To: Pavel Machek; +Cc: Ingo Molnar, kernel list
Pavel Machek <pavel@suse.cz> writes:
> if ((last_aper_order && aper_order != last_aper_order) ||
> @@ -303,9 +303,9 @@ void __init early_gart_iommu_check(void)
> if (!early_is_k8_nb(read_pci_config(0, num, 3, 0x00)))
> continue;
>
> - ctl = read_pci_config(0, num, 3, 0x90);
> + ctl = read_pci_config(0, num, 3, AMD64_GARTAPERTURECTL);
Strictly "AMD64_" is not the correct prefix, because these registers
are generic standard AGP3 registers.
-Andi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: aperture_64: use symbolic constants
[not found] ` <20080519125425.GD13546@elte.hu>
@ 2008-05-20 14:27 ` Pavel Machek
2008-05-20 15:06 ` Dave Jones
2008-05-20 15:42 ` Ingo Molnar
0 siblings, 2 replies; 9+ messages in thread
From: Pavel Machek @ 2008-05-20 14:27 UTC (permalink / raw)
To: Ingo Molnar; +Cc: kernel list
Hi!
> * Pavel Machek <pavel@suse.cz> wrote:
>
> > Use symbolic constants in aperture_64.c where possible (and make a
> > comment slightly more helpful).
>
> this code has already been changed in the latest -tip tree:
>
> http://people.redhat.com/mingo/tip.git/README
>
> iommu/gart code is covered by the tip/x86/gart subtopic, which carries 7
> commits right now:
Thanks, I have a copy now. And this one is relative to it:
---
Factor-out common aperture_valid code.
Signed-off-by: Pavel Machek <pavel@suse.cz>
diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
index 02f4dba..5373f78 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -109,27 +109,6 @@ static u32 __init allocate_aperture(void
return (u32)__pa(p);
}
-static int __init aperture_valid(u64 aper_base, u32 aper_size, u32 min_size)
-{
- if (!aper_base)
- return 0;
-
- if (aper_base + aper_size > 0x100000000UL) {
- printk(KERN_ERR "Aperture beyond 4GB. Ignoring.\n");
- return 0;
- }
- if (e820_any_mapped(aper_base, aper_base + aper_size, E820_RAM)) {
- printk(KERN_ERR "Aperture pointing to e820 RAM. Ignoring.\n");
- return 0;
- }
- if (aper_size < min_size) {
- printk(KERN_ERR "Aperture too small (%d MB) than (%d MB)\n",
- aper_size>>20, min_size>>20);
- return 0;
- }
-
- return 1;
-}
/* Find a PCI capability */
static __u32 __init find_cap(int bus, int slot, int func, int cap)
@@ -344,7 +323,7 @@ out:
if (gart_fix_e820 && !fix && aper_enabled) {
if (!e820_all_mapped(aper_base, aper_base + aper_size,
E820_RESERVED)) {
- /* reserved it, so we can resuse it in second kernel */
+ /* reserve it, so we can reuse it in second kernel */
printk(KERN_INFO "update e820 for GART\n");
add_memory_region(aper_base, aper_size, E820_RESERVED);
update_e820();
diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index fea1313..3b53ea0 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -228,24 +228,10 @@ static const struct agp_bridge_driver am
};
/* Some basic sanity checks for the aperture. */
-static int __devinit aperture_valid(u64 aper, u32 size)
+static int __devinit agp_aperture_valid(u64 aper, u32 size)
{
- if (aper == 0) {
- printk(KERN_ERR PFX "No aperture\n");
+ if (!aperture_valid(aper, size, 32*1024*1024))
return 0;
- }
- if ((u64)aper + size > 0x100000000ULL) {
- printk(KERN_ERR PFX "Aperture out of bounds\n");
- return 0;
- }
- if (e820_any_mapped(aper, aper + size, E820_RAM)) {
- printk(KERN_ERR PFX "Aperture pointing to RAM\n");
- return 0;
- }
- if (size < 32*1024*1024) {
- printk(KERN_ERR PFX "Aperture too small (%d MB)\n", size>>20);
- return 0;
- }
/* Request the Aperture. This catches cases when someone else
already put a mapping in there - happens with some very broken BIOS
@@ -282,7 +268,7 @@ static __devinit int fix_northbridge(str
nb_order = (nb_order >> 1) & 7;
pci_read_config_dword(nb, AMD64_GARTAPERTUREBASE, &nb_base);
nb_aper = nb_base << 25;
- if (aperture_valid(nb_aper, (32*1024*1024)<<nb_order)) {
+ if (agp_aperture_valid(nb_aper, (32*1024*1024)<<nb_order)) {
return 0;
}
@@ -313,7 +299,7 @@ static __devinit int fix_northbridge(str
}
printk(KERN_INFO PFX "Aperture from AGP @ %Lx size %u MB\n", aper, 32 << order);
- if (order < 0 || !aperture_valid(aper, (32*1024*1024)<<order))
+ if (order < 0 || !agp_aperture_valid(aper, (32*1024*1024)<<order))
return -1;
pci_write_config_dword(nb, AMD64_GARTAPERTURECTL, order << 1);
diff --git a/include/asm-x86/gart.h b/include/asm-x86/gart.h
index 6f22786..854cf7f 100644
--- a/include/asm-x86/gart.h
+++ b/include/asm-x86/gart.h
@@ -1,6 +1,8 @@
#ifndef _ASM_X8664_IOMMU_H
#define _ASM_X8664_IOMMU_H 1
+#include <asm/e820.h>
+
extern void pci_iommu_shutdown(void);
extern void no_iommu_init(void);
extern int force_iommu, no_iommu;
@@ -69,4 +71,26 @@ static inline void enable_gart_translati
pci_write_config_dword(dev, AMD64_GARTAPERTURECTL, ctl);
}
+static inline int aperture_valid(u64 aper_base, u32 aper_size, u32 min_size)
+{
+ if (!aper_base)
+ return 0;
+
+ if (aper_base + aper_size > 0x100000000ULL) {
+ printk(KERN_ERR "Aperture beyond 4GB. Ignoring.\n");
+ return 0;
+ }
+ if (e820_any_mapped(aper_base, aper_base + aper_size, E820_RAM)) {
+ printk(KERN_ERR "Aperture pointing to e820 RAM. Ignoring.\n");
+ return 0;
+ }
+ if (aper_size < min_size) {
+ printk(KERN_ERR "Aperture too small (%d MB) than (%d MB)\n",
+ aper_size>>20, min_size>>20);
+ return 0;
+ }
+
+ return 1;
+}
+
#endif
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: aperture_64: use symbolic constants
2008-05-20 14:27 ` Pavel Machek
@ 2008-05-20 15:06 ` Dave Jones
2008-05-20 15:32 ` Pavel Machek
2008-05-20 15:42 ` Ingo Molnar
1 sibling, 1 reply; 9+ messages in thread
From: Dave Jones @ 2008-05-20 15:06 UTC (permalink / raw)
To: Pavel Machek; +Cc: Ingo Molnar, kernel list
On Tue, May 20, 2008 at 04:27:17PM +0200, Pavel Machek wrote:
> +static inline int aperture_valid(u64 aper_base, u32 aper_size, u32 min_size)
> +{
> + if (!aper_base)
> + return 0;
> +
> + if (aper_base + aper_size > 0x100000000ULL) {
> + printk(KERN_ERR "Aperture beyond 4GB. Ignoring.\n");
> + return 0;
> + }
> + if (e820_any_mapped(aper_base, aper_base + aper_size, E820_RAM)) {
> + printk(KERN_ERR "Aperture pointing to e820 RAM. Ignoring.\n");
> + return 0;
> + }
> + if (aper_size < min_size) {
> + printk(KERN_ERR "Aperture too small (%d MB) than (%d MB)\n",
> + aper_size>>20, min_size>>20);
> + return 0;
> + }
> +
> + return 1;
> +}
Instead of making this an inline, we could add it to the agpgart code
and export it, and have the gart-iommu code call it.
You can't build the IOMMU code without agpgart anyway, and having this inlined
in both places seems a bit wasteful.
Additionally, it would mean not having a function in a header file,
which always strikes me as a wrong thing to do.
Dave
--
http://www.codemonkey.org.uk
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: aperture_64: use symbolic constants
2008-05-20 15:06 ` Dave Jones
@ 2008-05-20 15:32 ` Pavel Machek
2008-05-20 15:42 ` Dave Jones
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Machek @ 2008-05-20 15:32 UTC (permalink / raw)
To: Dave Jones, Ingo Molnar, kernel list
Hi!
> > +static inline int aperture_valid(u64 aper_base, u32 aper_size, u32 min_size)
> > +{
> > + if (!aper_base)
> > + return 0;
> > +
> > + if (aper_base + aper_size > 0x100000000ULL) {
> > + printk(KERN_ERR "Aperture beyond 4GB. Ignoring.\n");
> > + return 0;
> > + }
> > + if (e820_any_mapped(aper_base, aper_base + aper_size, E820_RAM)) {
> > + printk(KERN_ERR "Aperture pointing to e820 RAM. Ignoring.\n");
> > + return 0;
> > + }
> > + if (aper_size < min_size) {
> > + printk(KERN_ERR "Aperture too small (%d MB) than (%d MB)\n",
> > + aper_size>>20, min_size>>20);
> > + return 0;
> > + }
> > +
> > + return 1;
> > +}
>
> Instead of making this an inline, we could add it to the agpgart code
> and export it, and have the gart-iommu code call it.
> You can't build the IOMMU code without agpgart anyway, and having this inlined
> in both places seems a bit wasteful.
> Additionally, it would mean not having a function in a header file,
> which always strikes me as a wrong thing to do.
Can you elaborate? Yes, it would be nicer if this went to .c
somewhere, but aperture_64.c seems unsuitable (we need it on 32-bit,
too, right?)... plus it was __init in one place, and __devinit in the
other, so I figured out "inline it so that it works automagically".
Plus, I don't think it should go into drivers/agp, as iommu code in
arch/x86/kernel seems to be able to work without that...?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: aperture_64: use symbolic constants
2008-05-20 15:32 ` Pavel Machek
@ 2008-05-20 15:42 ` Dave Jones
2008-05-21 1:23 ` Andi Kleen
0 siblings, 1 reply; 9+ messages in thread
From: Dave Jones @ 2008-05-20 15:42 UTC (permalink / raw)
To: Pavel Machek; +Cc: Ingo Molnar, kernel list
On Tue, May 20, 2008 at 05:32:15PM +0200, Pavel Machek wrote:
> > Instead of making this an inline, we could add it to the agpgart code
> > and export it, and have the gart-iommu code call it.
> > You can't build the IOMMU code without agpgart anyway, and having this inlined
> > in both places seems a bit wasteful.
> > Additionally, it would mean not having a function in a header file,
> > which always strikes me as a wrong thing to do.
>
> Can you elaborate? Yes, it would be nicer if this went to .c
> somewhere, but aperture_64.c seems unsuitable (we need it on 32-bit,
> too, right?)... plus it was __init in one place, and __devinit in the
> other, so I figured out "inline it so that it works automagically".
>
> Plus, I don't think it should go into drivers/agp, as iommu code in
> arch/x86/kernel seems to be able to work without that...?
If you enable IOMMU, you _have_ to enable AGP. (well, you don't have to,
it does a 'select AGP' for you when you enable it :-)
It does this because the agpgart driver needs to know how much of the
aperture has been stolen for IOMMU use, and I think it already uses some
functions from that driver already.
Dave
--
http://www.codemonkey.org.uk
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: aperture_64: use symbolic constants
2008-05-20 14:27 ` Pavel Machek
2008-05-20 15:06 ` Dave Jones
@ 2008-05-20 15:42 ` Ingo Molnar
2008-05-21 1:59 ` Yinghai Lu
1 sibling, 1 reply; 9+ messages in thread
From: Ingo Molnar @ 2008-05-20 15:42 UTC (permalink / raw)
To: Pavel Machek; +Cc: kernel list, Thomas Gleixner, H. Peter Anvin
* Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> > * Pavel Machek <pavel@suse.cz> wrote:
> >
> > > Use symbolic constants in aperture_64.c where possible (and make a
> > > comment slightly more helpful).
> >
> > this code has already been changed in the latest -tip tree:
> >
> > http://people.redhat.com/mingo/tip.git/README
> >
> > iommu/gart code is covered by the tip/x86/gart subtopic, which carries 7
> > commits right now:
>
> Thanks, I have a copy now. And this one is relative to it:
>
> ---
>
> Factor-out common aperture_valid code.
applied, thanks Pavel.
Ingo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: aperture_64: use symbolic constants
2008-05-20 15:42 ` Dave Jones
@ 2008-05-21 1:23 ` Andi Kleen
0 siblings, 0 replies; 9+ messages in thread
From: Andi Kleen @ 2008-05-21 1:23 UTC (permalink / raw)
To: Dave Jones; +Cc: Pavel Machek, Ingo Molnar, kernel list
Dave Jones <davej@redhat.com> writes:
>
> If you enable IOMMU, you _have_ to enable AGP. (well, you don't have to,
> it does a 'select AGP' for you when you enable it :-)
That's actually not needed. The IOMMU code was carefully
written to not require AGP. The only requirement it has is that
if AGP is enabled it has to be built in.
-Andi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: aperture_64: use symbolic constants
2008-05-20 15:42 ` Ingo Molnar
@ 2008-05-21 1:59 ` Yinghai Lu
0 siblings, 0 replies; 9+ messages in thread
From: Yinghai Lu @ 2008-05-21 1:59 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Pavel Machek, kernel list, Thomas Gleixner, H. Peter Anvin
On Tue, May 20, 2008 at 8:42 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Pavel Machek <pavel@ucw.cz> wrote:
>
>> Hi!
>>
>> > * Pavel Machek <pavel@suse.cz> wrote:
>> >
>> > > Use symbolic constants in aperture_64.c where possible (and make a
>> > > comment slightly more helpful).
>> >
>> > this code has already been changed in the latest -tip tree:
>> >
>> > http://people.redhat.com/mingo/tip.git/README
>> >
>> > iommu/gart code is covered by the tip/x86/gart subtopic, which carries 7
>> > commits right now:
>>
>> Thanks, I have a copy now. And this one is relative to it:
>>
>> ---
>>
>> Factor-out common aperture_valid code.
>
> applied, thanks Pavel.
can you change the titile?
YH
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-05-21 1:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-05-19 12:39 aperture_64: use symbolic constants Pavel Machek
2008-05-20 11:20 ` Andi Kleen
[not found] ` <20080519125425.GD13546@elte.hu>
2008-05-20 14:27 ` Pavel Machek
2008-05-20 15:06 ` Dave Jones
2008-05-20 15:32 ` Pavel Machek
2008-05-20 15:42 ` Dave Jones
2008-05-21 1:23 ` Andi Kleen
2008-05-20 15:42 ` Ingo Molnar
2008-05-21 1:59 ` Yinghai Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).