aperture_64.c: corner case wrong

aperture_64.c: corner case wrong

[Pavel Machek]

If 

fix == 0, aper_enabled == 1, gart_fix_e820 == 0

	if (!fix && !aper_enabled)
		return;

	if (gart_fix_e820 && !fix && aper_enabled) {
		if (e820_any_mapped(aper_base, aper_base + aper_size,
				    E820_RAM)) {
			/* 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();
		}
		return;
	}

	/* different nodes have different setting, disable them all atfirst*/

we'll fall back here and disable all the settings, even when they were
all consistent.

What about this? (I hope it compiles...)

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..aa4876a 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -344,14 +323,16 @@ out:
 	if (gart_fix_e820 && !fix && aper_enabled) {
 		if (!e820_all_mapped(aper_base, aper_base + aper_size,
 				    E820_RESERVED)) {
 			/* 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();
 		}
-		return;
 	}
 
+	if (!fix)
+		return;
+
 	/* different nodes have different setting, disable them all at first*/
 	for (i = 0; i < ARRAY_SIZE(bus_dev_ranges); i++) {
 		int bus;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: aperture_64.c: corner case wrong

[Yinghai Lu]

On Mon, May 26, 2008 at 12:17 PM, Pavel Machek <pavel@suse.cz> wrote:
>
> If
>
> fix == 0, aper_enabled == 1, gart_fix_e820 == 0
>
>        if (!fix && !aper_enabled)
>                return;
>
>        if (gart_fix_e820 && !fix && aper_enabled) {
>                if (e820_any_mapped(aper_base, aper_base + aper_size,
>                                    E820_RAM)) {
>                        /* 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();
>                }
>                return;
>        }
>
>        /* different nodes have different setting, disable them all atfirst*/
>
> we'll fall back here and disable all the settings, even when they were
> all consistent.
>
> What about this? (I hope it compiles...)
>
> 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..aa4876a 100644
> --- a/arch/x86/kernel/aperture_64.c
> +++ b/arch/x86/kernel/aperture_64.c
> @@ -344,14 +323,16 @@ out:
>        if (gart_fix_e820 && !fix && aper_enabled) {
>                if (!e820_all_mapped(aper_base, aper_base + aper_size,
>                                    E820_RESERVED)) {
>                        /* 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();
>                }
> -               return;
>        }
>
> +       if (!fix)
> +               return;
> +
>        /* different nodes have different setting, disable them all at first*/
>        for (i = 0; i < ARRAY_SIZE(bus_dev_ranges); i++) {
>                int bus;
>
good catch.

Acked-by: Yinghai Lu <yhlu.kernel@gmail.com>

YH

Re: aperture_64.c: corner case wrong

[Pavel Machek]

Try to convert comments in aperture_64.c closer to
english. Unfortunately, I was not able to understand some of the
sentences :-(. Help welcome.

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 5373f78..183d907 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -75,21 +75,20 @@ static u32 __init allocate_aperture(void
 	/*
 	 * Aperture has to be naturally aligned. This means a 2GB aperture
 	 * won't have much chance of finding a place in the lower 4GB of
-	 * memory. Unfortunately we cannot move it up because that would
+	 * memory. Unfortunately, we cannot move it up because that would
 	 * make the IOMMU useless.
-	 */
-	/*
-	 * using 512M as goal, in case kexec will load kernel_big
+	 *
+	 * Using 512M as goal, in case kexec will load kernel_big
 	 * that will do the on position decompress, and  could overlap with
 	 * that positon with gart that is used.
-	 * sequende:
+	 * sequence:
 	 * kernel_small
 	 * ==> kexec (with kdump trigger path or previous doesn't shutdown gart)
-	 * ==> kernel_small(gart area become e820_reserved)
+	 * ==> kernel_small (gart area becomes e820_reserved)
 	 * ==> kexec (with kdump trigger path or previous doesn't shutdown gart)
-	 * ==> kerne_big (uncompressed size will be big than 64M or 128M)
+	 * ==> kernel_big (uncompressed size will be bigger than 64M or 128M)
 	 * so don't use 512M below as gart iommu, leave the space for kernel
-	 * code for safe
+	 * code to be safe.
 	 */
 	p = __alloc_bootmem_nopanic(aper_size, aper_size, 512ULL<<20);
 	if (!p || __pa(p)+aper_size > 0xffffffff) {
@@ -262,14 +261,15 @@ early_param("gart_fix_e820", parse_gart_
 void __init early_gart_iommu_check(void)
 {
 	/*
-	 * in case it is enabled before, esp for kexec/kdump,
-	 * previous kernel already enable that. memset called
-	 * by allocate_aperture/__alloc_bootmem_nopanic cause restart.
-	 * or second kernel have different position for GART hole. and new
+	 * In case it is enabled before, esp for kexec/kdump,
+	 * previous kernel already enabled that. memset called
+	 * by allocate_aperture/__alloc_bootmem_nopanic causes restart.
+	 * Or second kernel could have different position for GART hole, and new
 	 * kernel could use hole as RAM that is still used by GART set by
-	 * first kernel
-	 * or BIOS forget to put that in reserved.
-	 * try to update e820 to make that region as reserved.
+	 * first kernel.
+	 * Or maybe BIOS forgot to put that in reserved.
+	 *
+	 * Try to update e820 to make that region as reserved.
 	 */
 	int fix, slot;
 	u32 ctl;


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: aperture_64.c: corner case wrong

[Andrew Morton]

On Mon, 26 May 2008 21:17:30 +0200
Pavel Machek <pavel@suse.cz> wrote:

> 
> If 
> 
> fix == 0, aper_enabled == 1, gart_fix_e820 == 0
> 
> 	if (!fix && !aper_enabled)
> 		return;
> 
> 	if (gart_fix_e820 && !fix && aper_enabled) {
> 		if (e820_any_mapped(aper_base, aper_base + aper_size,
> 				    E820_RAM)) {
> 			/* 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();
> 		}
> 		return;
> 	}
> 
> 	/* different nodes have different setting, disable them all atfirst*/
> 
> we'll fall back here and disable all the settings, even when they were
> all consistent.
> 
> What about this? (I hope it compiles...)
> 
> 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..aa4876a 100644
> --- a/arch/x86/kernel/aperture_64.c
> +++ b/arch/x86/kernel/aperture_64.c
> @@ -344,14 +323,16 @@ out:
>  	if (gart_fix_e820 && !fix && aper_enabled) {
>  		if (!e820_all_mapped(aper_base, aper_base + aper_size,
>  				    E820_RESERVED)) {
>  			/* 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();
>  		}
> -		return;
>  	}
>  
> +	if (!fix)
> +		return;
> +
>  	/* different nodes have different setting, disable them all at first*/
>  	for (i = 0; i < ARRAY_SIZE(bus_dev_ranges); i++) {
>  		int bus;
> 

So what happened with this?  Thomas has merged an "x86: aperture_64.c:
cleanups" from yourself but it appears that half of this bugfix got
applied somewhere and the below part did not?


diff -puN arch/x86/kernel/aperture_64.c~aperture_64c-corner-case-wrong arch/x86/kernel/aperture_64.c
--- a/arch/x86/kernel/aperture_64.c~aperture_64c-corner-case-wrong
+++ a/arch/x86/kernel/aperture_64.c
@@ -331,6 +331,9 @@ out:
 		return;
 	}
 
+	if (!fix)
+		return;
+
 	/* different nodes have different setting, disable them all at first*/
 	for (i = 0; i < ARRAY_SIZE(bus_dev_ranges); i++) {
 		int bus;
_

Re: aperture_64.c: corner case wrong

[Thomas Gleixner]

On Mon, 2 Jun 2008, Andrew Morton wrote:
> On Mon, 26 May 2008 21:17:30 +0200
> Pavel Machek <pavel@suse.cz> wrote:
> > diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
> > index 02f4dba..aa4876a 100644
> > --- a/arch/x86/kernel/aperture_64.c
> > +++ b/arch/x86/kernel/aperture_64.c
> > @@ -344,14 +323,16 @@ out:
> >  	if (gart_fix_e820 && !fix && aper_enabled) {
> >  		if (!e820_all_mapped(aper_base, aper_base + aper_size,
> >  				    E820_RESERVED)) {
> >  			/* 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();
> >  		}
> > -		return;
> >  	}
> >  
> > +	if (!fix)
> > +		return;
> > +
> >  	/* different nodes have different setting, disable them all at first*/
> >  	for (i = 0; i < ARRAY_SIZE(bus_dev_ranges); i++) {
> >  		int bus;
> > 
> 
> So what happened with this?  Thomas has merged an "x86: aperture_64.c:
> cleanups" from yourself but it appears that half of this bugfix got
> applied somewhere and the below part did not?

Err, no. I seem to have dropped the patch completely. I'm picking it
up again.

> diff -puN arch/x86/kernel/aperture_64.c~aperture_64c-corner-case-wrong arch/x86/kernel/aperture_64.c
> --- a/arch/x86/kernel/aperture_64.c~aperture_64c-corner-case-wrong
> +++ a/arch/x86/kernel/aperture_64.c
> @@ -331,6 +331,9 @@ out:
>  		return;

This should be:
-  		return;

right?

>  	}
>  
> +	if (!fix)
> +		return;
> +
>  	/* different nodes have different setting, disable them all at first*/
>  	for (i = 0; i < ARRAY_SIZE(bus_dev_ranges); i++) {
>  		int bus;
> _

Thanks,
	tglx

Re: aperture_64.c: corner case wrong

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> > So what happened with this?  Thomas has merged an "x86: 
> > aperture_64.c: cleanups" from yourself but it appears that half of 
> > this bugfix got applied somewhere and the below part did not?
> 
> Err, no. I seem to have dropped the patch completely. I'm picking it 
> up again.

no, it was not dropped (we dont drop patches :), here are all the 
arch/x86/kernel/aperture_64.c patches in -tip:

 earth4:~/tip> tip-log arch/x86/kernel/aperture_64.c | grep ^#
 # x86/gart: 1edc1ab: x86: agp_gart size checking for buggy device
 # x86/gart: 8c9fd91: x86: checking aperture size order
 # x86/gart: 7677b2e: x86_64: allocate gart aperture from 512M
 # x86/gart: 55c0d72: x86: clean up aperture_64.c
 # x86/gart: 0abbc78: x86, aperture_64: use symbolic constants
 # x86/gart: dd564d0: x86: aperture_64.c: cleanups

so it's commit dd564d0 - but that second chunk Andrew noticed indeed 
went missing in action.

	Ingo

Re: aperture_64.c: corner case wrong

[Thomas Gleixner]

On Wed, 4 Jun 2008, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > > So what happened with this?  Thomas has merged an "x86: 
> > > aperture_64.c: cleanups" from yourself but it appears that half of 
> > > this bugfix got applied somewhere and the below part did not?
> > 
> > Err, no. I seem to have dropped the patch completely. I'm picking it 
> > up again.
> 
> no, it was not dropped (we dont drop patches :), here are all the 
> arch/x86/kernel/aperture_64.c patches in -tip:
> 
>  earth4:~/tip> tip-log arch/x86/kernel/aperture_64.c | grep ^#
>  # x86/gart: 1edc1ab: x86: agp_gart size checking for buggy device
>  # x86/gart: 8c9fd91: x86: checking aperture size order
>  # x86/gart: 7677b2e: x86_64: allocate gart aperture from 512M
>  # x86/gart: 55c0d72: x86: clean up aperture_64.c
>  # x86/gart: 0abbc78: x86, aperture_64: use symbolic constants
>  # x86/gart: dd564d0: x86: aperture_64.c: cleanups
> 
> so it's commit dd564d0 - but that second chunk Andrew noticed indeed 
> went missing in action.

No, the patch was titled: ".... - corner case wrong" its missing and
I'm wondering why.

/me digs into git branches

Thanks,
	tglx