aperture_64: use symbolic constants

aperture_64: use symbolic constants

[Pavel Machek]

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

Re: aperture_64: use symbolic constants

[Andi Kleen]

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

Re: aperture_64: use symbolic constants

[Pavel Machek]

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

Re: aperture_64: use symbolic constants

[Dave Jones]

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

Re: aperture_64: use symbolic constants

[Pavel Machek]

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

Re: aperture_64: use symbolic constants

[Dave Jones]

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

Re: aperture_64: use symbolic constants

[Ingo Molnar]

* 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

Re: aperture_64: use symbolic constants

[Andi Kleen]

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

Re: aperture_64: use symbolic constants

[Yinghai Lu]

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