[PATCH] x86-64: fix and clean up AMD Fam10 MMCONF enabling

[PATCH] x86-64: fix and clean up AMD Fam10 MMCONF enabling

[Jan Beulich]

Candidate memory ranges were not calculated properly (start addresses
got needlessly rounded down, and end addresses didn't get rounded up
at all), address comparison for secondary CPUs was done on only part
of the address, and disabled status wasn't tracked properly.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/kernel/mmconf-fam10h_64.c |   54 +++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 29 deletions(-)

--- linux-2.6.37-rc1/arch/x86/kernel/mmconf-fam10h_64.c
+++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/kernel/mmconf-fam10h_64.c
@@ -25,7 +25,6 @@ struct pci_hostbridge_probe {
 };
 
 static u64 __cpuinitdata fam10h_pci_mmconf_base;
-static int __cpuinitdata fam10h_pci_mmconf_base_status;
 
 static struct pci_hostbridge_probe pci_probes[] __cpuinitdata = {
 	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1200 },
@@ -44,7 +43,9 @@ static int __cpuinit cmp_range(const voi
 	return start1 - start2;
 }
 
-/*[47:0] */
+#define UNIT (1ULL << (5 + 3 + 12))
+#define MASK (~(UNIT - 1))
+#define SIZE (UNIT << 8)
 /* need to avoid (0xfd<<32) and (0xfe<<32), ht used space */
 #define FAM10H_PCI_MMCONF_BASE (0xfcULL<<32)
 #define BASE_VALID(b) ((b != (0xfdULL << 32)) && (b != (0xfeULL << 32)))
@@ -64,12 +65,11 @@ static void __cpuinit get_fam10h_pci_mmc
 	struct range range[8];
 
 	/* only try to get setting from BSP */
-	/* -1 or 1 */
-	if (fam10h_pci_mmconf_base_status)
+	if (fam10h_pci_mmconf_base)
 		return;
 
 	if (!early_pci_allowed())
-		goto fail;
+		return;
 
 	found = 0;
 	for (i = 0; i < ARRAY_SIZE(pci_probes); i++) {
@@ -91,7 +91,7 @@ static void __cpuinit get_fam10h_pci_mmc
 	}
 
 	if (!found)
-		goto fail;
+		return;
 
 	/* SYS_CFG */
 	address = MSR_K8_SYSCFG;
@@ -104,11 +104,11 @@ static void __cpuinit get_fam10h_pci_mmc
 		/* TOP_MEM2 */
 		address = MSR_K8_TOP_MEM2;
 		rdmsrl(address, val);
-		tom2 = val & (0xffffULL<<32);
+		tom2 = val & 0xffffff800000ULL;
 	}
 
 	if (base <= tom2)
-		base = tom2 + (1ULL<<32);
+		base = (tom2 + 2 * UNIT - 1) & MASK;
 
 	/*
 	 * need to check if the range is in the high mmio range that is
@@ -123,9 +123,9 @@ static void __cpuinit get_fam10h_pci_mmc
 		if (!(reg & 3))
 			continue;
 
-		start = (((u64)reg) << 8) & (0xffULL << 32); /* 39:16 on 31:8*/
+		start = (u64)(reg & 0xffffff00) << 8; /* 39:16 on 31:8*/
 		reg = read_pci_config(bus, slot, 1, 0x84 + (i << 3));
-		end = (((u64)reg) << 8) & (0xffULL << 32); /* 39:16 on 31:8*/
+		end = ((u64)(reg & 0xffffff00) << 8) | 0xffff; /* 39:16 on 31:8*/
 
 		if (!end)
 			continue;
@@ -143,32 +143,27 @@ static void __cpuinit get_fam10h_pci_mmc
 
 	if (range[hi_mmio_num - 1].end < base)
 		goto out;
-	if (range[0].start > base)
+	if (range[0].start > base + SIZE)
 		goto out;
 
 	/* need to find one window */
-	base = range[0].start - (1ULL << 32);
+	base = (range[0].start & MASK) - UNIT;
 	if ((base > tom2) && BASE_VALID(base))
 		goto out;
-	base = range[hi_mmio_num - 1].end + (1ULL << 32);
+	base = (range[hi_mmio_num - 1].end + UNIT) & MASK;
 	if ((base > tom2) && BASE_VALID(base))
 		goto out;
 	/* need to find window between ranges */
-	if (hi_mmio_num > 1)
-	for (i = 0; i < hi_mmio_num - 1; i++) {
-		if (range[i + 1].start > (range[i].end + (1ULL << 32))) {
-			base = range[i].end + (1ULL << 32);
-			if ((base > tom2) && BASE_VALID(base))
-				goto out;
-		}
+	for (i = 1; i < hi_mmio_num; i++) {
+		base = (range[i - 1].end + UNIT) & MASK;
+		val = range[i].start & MASK;
+		if (val >= base + SIZE && base > tom2 && BASE_VALID(base))
+			goto out;
 	}
-
-fail:
-	fam10h_pci_mmconf_base_status = -1;
 	return;
+
 out:
 	fam10h_pci_mmconf_base = base;
-	fam10h_pci_mmconf_base_status = 1;
 }
 
 void __cpuinit fam10h_check_enable_mmcfg(void)
@@ -190,11 +185,10 @@ void __cpuinit fam10h_check_enable_mmcfg
 
 		/* only trust the one handle 256 buses, if acpi=off */
 		if (!acpi_pci_disabled || busnbits >= 8) {
-			u64 base;
-			base = val & (0xffffULL << 32);
-			if (fam10h_pci_mmconf_base_status <= 0) {
+			u64 base = val & MASK;
+
+			if (!fam10h_pci_mmconf_base) {
 				fam10h_pci_mmconf_base = base;
-				fam10h_pci_mmconf_base_status = 1;
 				return;
 			} else if (fam10h_pci_mmconf_base ==  base)
 				return;
@@ -206,8 +200,10 @@ void __cpuinit fam10h_check_enable_mmcfg
 	 * with 256 buses
 	 */
 	get_fam10h_pci_mmconf_base();
-	if (fam10h_pci_mmconf_base_status <= 0)
+	if (!fam10h_pci_mmconf_base) {
+		pci_probe &= ~PCI_CHECK_ENABLE_AMD_MMCONF;
 		return;
+	}
 
 	printk(KERN_INFO "Enable MMCONFIG on AMD Family 10h\n");
 	val &= ~((FAM10H_MMIO_CONF_BASE_MASK<<FAM10H_MMIO_CONF_BASE_SHIFT) |

Re: [PATCH] x86-64: fix and clean up AMD Fam10 MMCONF enabling

[Yinghai Lu]

On 11/04/2010 08:22 AM, Jan Beulich wrote:
> Candidate memory ranges were not calculated properly (start addresses
> got needlessly rounded down, and end addresses didn't get rounded up
> at all), address comparison for secondary CPUs was done on only part
> of the address, and disabled status wasn't tracked properly.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  arch/x86/kernel/mmconf-fam10h_64.c |   54 +++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
> 
> --- linux-2.6.37-rc1/arch/x86/kernel/mmconf-fam10h_64.c
> +++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/kernel/mmconf-fam10h_64.c
> @@ -25,7 +25,6 @@ struct pci_hostbridge_probe {
>  };
>  
>  static u64 __cpuinitdata fam10h_pci_mmconf_base;
> -static int __cpuinitdata fam10h_pci_mmconf_base_status;
>  
>  static struct pci_hostbridge_probe pci_probes[] __cpuinitdata = {
>  	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1200 },
> @@ -44,7 +43,9 @@ static int __cpuinit cmp_range(const voi
>  	return start1 - start2;
>  }
>  
> -/*[47:0] */
> +#define UNIT (1ULL << (5 + 3 + 12))
> +#define MASK (~(UNIT - 1))
> +#define SIZE (UNIT << 8)

those MACRO seems too generic.

>  /* need to avoid (0xfd<<32) and (0xfe<<32), ht used space */
>  #define FAM10H_PCI_MMCONF_BASE (0xfcULL<<32)
>  #define BASE_VALID(b) ((b != (0xfdULL << 32)) && (b != (0xfeULL << 32)))
> @@ -64,12 +65,11 @@ static void __cpuinit get_fam10h_pci_mmc
>  	struct range range[8];
>  
>  	/* only try to get setting from BSP */
> -	/* -1 or 1 */
> -	if (fam10h_pci_mmconf_base_status)
> +	if (fam10h_pci_mmconf_base)
>  		return;
>  
>  	if (!early_pci_allowed())
> -		goto fail;
> +		return;
>  
>  	found = 0;
>  	for (i = 0; i < ARRAY_SIZE(pci_probes); i++) {
> @@ -91,7 +91,7 @@ static void __cpuinit get_fam10h_pci_mmc
>  	}
>  
>  	if (!found)
> -		goto fail;
> +		return;
>  
>  	/* SYS_CFG */
>  	address = MSR_K8_SYSCFG;
> @@ -104,11 +104,11 @@ static void __cpuinit get_fam10h_pci_mmc
>  		/* TOP_MEM2 */
>  		address = MSR_K8_TOP_MEM2;
>  		rdmsrl(address, val);
> -		tom2 = val & (0xffffULL<<32);
> +		tom2 = val & 0xffffff800000ULL;
>  	}
>  
>  	if (base <= tom2)
> -		base = tom2 + (1ULL<<32);
> +		base = (tom2 + 2 * UNIT - 1) & MASK;
>  
>  	/*
>  	 * need to check if the range is in the high mmio range that is
> @@ -123,9 +123,9 @@ static void __cpuinit get_fam10h_pci_mmc
>  		if (!(reg & 3))
>  			continue;
>  
> -		start = (((u64)reg) << 8) & (0xffULL << 32); /* 39:16 on 31:8*/
> +		start = (u64)(reg & 0xffffff00) << 8; /* 39:16 on 31:8*/
>  		reg = read_pci_config(bus, slot, 1, 0x84 + (i << 3));
> -		end = (((u64)reg) << 8) & (0xffULL << 32); /* 39:16 on 31:8*/
> +		end = ((u64)(reg & 0xffffff00) << 8) | 0xffff; /* 39:16 on 31:8*/
>  
>  		if (!end)
>  			continue;

if my memory is right, our range add/subtract is taking limit instead of limit - 1.

also reading out from those register is limit already.

Thanks

> @@ -143,32 +143,27 @@ static void __cpuinit get_fam10h_pci_mmc
>  
>  	if (range[hi_mmio_num - 1].end < base)
>  		goto out;
> -	if (range[0].start > base)
> +	if (range[0].start > base + SIZE)
>  		goto out;
>  
>  	/* need to find one window */
> -	base = range[0].start - (1ULL << 32);
> +	base = (range[0].start & MASK) - UNIT;
>  	if ((base > tom2) && BASE_VALID(base))
>  		goto out;
> -	base = range[hi_mmio_num - 1].end + (1ULL << 32);
> +	base = (range[hi_mmio_num - 1].end + UNIT) & MASK;
>  	if ((base > tom2) && BASE_VALID(base))
>  		goto out;
>  	/* need to find window between ranges */
> -	if (hi_mmio_num > 1)
> -	for (i = 0; i < hi_mmio_num - 1; i++) {
> -		if (range[i + 1].start > (range[i].end + (1ULL << 32))) {
> -			base = range[i].end + (1ULL << 32);
> -			if ((base > tom2) && BASE_VALID(base))
> -				goto out;
> -		}
> +	for (i = 1; i < hi_mmio_num; i++) {
> +		base = (range[i - 1].end + UNIT) & MASK;
> +		val = range[i].start & MASK;
> +		if (val >= base + SIZE && base > tom2 && BASE_VALID(base))
> +			goto out;
>  	}
> -
> -fail:
> -	fam10h_pci_mmconf_base_status = -1;
>  	return;
> +
>  out:
>  	fam10h_pci_mmconf_base = base;
> -	fam10h_pci_mmconf_base_status = 1;
>  }
>  
>  void __cpuinit fam10h_check_enable_mmcfg(void)
> @@ -190,11 +185,10 @@ void __cpuinit fam10h_check_enable_mmcfg
>  
>  		/* only trust the one handle 256 buses, if acpi=off */
>  		if (!acpi_pci_disabled || busnbits >= 8) {
> -			u64 base;
> -			base = val & (0xffffULL << 32);
> -			if (fam10h_pci_mmconf_base_status <= 0) {
> +			u64 base = val & MASK;
> +
> +			if (!fam10h_pci_mmconf_base) {
>  				fam10h_pci_mmconf_base = base;
> -				fam10h_pci_mmconf_base_status = 1;
>  				return;
>  			} else if (fam10h_pci_mmconf_base ==  base)
>  				return;
> @@ -206,8 +200,10 @@ void __cpuinit fam10h_check_enable_mmcfg
>  	 * with 256 buses
>  	 */
>  	get_fam10h_pci_mmconf_base();
> -	if (fam10h_pci_mmconf_base_status <= 0)
> +	if (!fam10h_pci_mmconf_base) {
> +		pci_probe &= ~PCI_CHECK_ENABLE_AMD_MMCONF;
>  		return;
> +	}
>  
>  	printk(KERN_INFO "Enable MMCONFIG on AMD Family 10h\n");
>  	val &= ~((FAM10H_MMIO_CONF_BASE_MASK<<FAM10H_MMIO_CONF_BASE_SHIFT) |
> 
> 

Re: [PATCH] x86-64: fix and clean up AMD Fam10 MMCONF enabling

[Yinghai Lu]

On 11/04/2010 08:22 AM, Jan Beulich wrote:
> Candidate memory ranges were not calculated properly (start addresses
> got needlessly rounded down, and end addresses didn't get rounded up
> at all), address comparison for secondary CPUs was done on only part
> of the address, and disabled status wasn't tracked properly.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  arch/x86/kernel/mmconf-fam10h_64.c |   54 +++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
> 
> --- linux-2.6.37-rc1/arch/x86/kernel/mmconf-fam10h_64.c
> +++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/kernel/mmconf-fam10h_64.c
> @@ -25,7 +25,6 @@ struct pci_hostbridge_probe {
>  };
>  
>  static u64 __cpuinitdata fam10h_pci_mmconf_base;
> -static int __cpuinitdata fam10h_pci_mmconf_base_status;
>  
>  static struct pci_hostbridge_probe pci_probes[] __cpuinitdata = {
>  	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1200 },
> @@ -44,7 +43,9 @@ static int __cpuinit cmp_range(const voi
>  	return start1 - start2;
>  }
>  
> -/*[47:0] */
> +#define UNIT (1ULL << (5 + 3 + 12))
> +#define MASK (~(UNIT - 1))
> +#define SIZE (UNIT << 8)

these MACRO should be more meaningful

>  /* need to avoid (0xfd<<32) and (0xfe<<32), ht used space */
>  #define FAM10H_PCI_MMCONF_BASE (0xfcULL<<32)
>  #define BASE_VALID(b) ((b != (0xfdULL << 32)) && (b != (0xfeULL << 32)))
> @@ -64,12 +65,11 @@ static void __cpuinit get_fam10h_pci_mmc
>  	struct range range[8];
>  
>  	/* only try to get setting from BSP */
> -	/* -1 or 1 */
> -	if (fam10h_pci_mmconf_base_status)
> +	if (fam10h_pci_mmconf_base)
>  		return;
>  
>  	if (!early_pci_allowed())
> -		goto fail;
> +		return;
>  
>  	found = 0;
>  	for (i = 0; i < ARRAY_SIZE(pci_probes); i++) {
> @@ -91,7 +91,7 @@ static void __cpuinit get_fam10h_pci_mmc
>  	}
>  
>  	if (!found)
> -		goto fail;
> +		return;
>  
>  	/* SYS_CFG */
>  	address = MSR_K8_SYSCFG;
> @@ -104,11 +104,11 @@ static void __cpuinit get_fam10h_pci_mmc
>  		/* TOP_MEM2 */
>  		address = MSR_K8_TOP_MEM2;
>  		rdmsrl(address, val);
> -		tom2 = val & (0xffffULL<<32);
> +		tom2 = val & 0xffffff800000ULL;
>  	}
>  
>  	if (base <= tom2)
> -		base = tom2 + (1ULL<<32);
> +		base = (tom2 + 2 * UNIT - 1) & MASK;
>  
>  	/*
>  	 * need to check if the range is in the high mmio range that is
> @@ -123,9 +123,9 @@ static void __cpuinit get_fam10h_pci_mmc
>  		if (!(reg & 3))
>  			continue;
>  
> -		start = (((u64)reg) << 8) & (0xffULL << 32); /* 39:16 on 31:8*/
> +		start = (u64)(reg & 0xffffff00) << 8; /* 39:16 on 31:8*/
>  		reg = read_pci_config(bus, slot, 1, 0x84 + (i << 3));
> -		end = (((u64)reg) << 8) & (0xffULL << 32); /* 39:16 on 31:8*/
> +		end = ((u64)(reg & 0xffffff00) << 8) | 0xffff; /* 39:16 on 31:8*/
>  
>  		if (!end)
>  			continue;

those section could be changed to:

                start = reg & 0xffffff00; /* 39:16 on 31:8*/
                start <<= 8;
                reg = read_pci_config(bus, slot, 1, 0x84 + (i << 3));
                end = (reg & 0xffffff00);
                end <<= 8;

                if (!end)
                        continue;
                end |= 0xffff;

                hi_mmio_num = add_range(range, 8, hi_mmio_num, start, end + 1);

1. append 0xffff after !end checking..
2. add_range should take end + 1;

> @@ -143,32 +143,27 @@ static void __cpuinit get_fam10h_pci_mmc
>  
>  	if (range[hi_mmio_num - 1].end < base)
>  		goto out;
> -	if (range[0].start > base)
> +	if (range[0].start > base + SIZE)
>  		goto out;
>  
>  	/* need to find one window */
> -	base = range[0].start - (1ULL << 32);
> +	base = (range[0].start & MASK) - UNIT;
>  	if ((base > tom2) && BASE_VALID(base))
>  		goto out;
> -	base = range[hi_mmio_num - 1].end + (1ULL << 32);
> +	base = (range[hi_mmio_num - 1].end + UNIT) & MASK;
>  	if ((base > tom2) && BASE_VALID(base))
>  		goto out;
>  	/* need to find window between ranges */
> -	if (hi_mmio_num > 1)
> -	for (i = 0; i < hi_mmio_num - 1; i++) {
> -		if (range[i + 1].start > (range[i].end + (1ULL << 32))) {
> -			base = range[i].end + (1ULL << 32);
> -			if ((base > tom2) && BASE_VALID(base))
> -				goto out;
> -		}
> +	for (i = 1; i < hi_mmio_num; i++) {
> +		base = (range[i - 1].end + UNIT) & MASK;
> +		val = range[i].start & MASK;
> +		if (val >= base + SIZE && base > tom2 && BASE_VALID(base))
> +			goto out;
>  	}
> -
> -fail:
> -	fam10h_pci_mmconf_base_status = -1;
>  	return;
> +
>  out:
>  	fam10h_pci_mmconf_base = base;
> -	fam10h_pci_mmconf_base_status = 1;
>  }
>  
>  void __cpuinit fam10h_check_enable_mmcfg(void)
> @@ -190,11 +185,10 @@ void __cpuinit fam10h_check_enable_mmcfg
>  
>  		/* only trust the one handle 256 buses, if acpi=off */
>  		if (!acpi_pci_disabled || busnbits >= 8) {
> -			u64 base;
> -			base = val & (0xffffULL << 32);
> -			if (fam10h_pci_mmconf_base_status <= 0) {
> +			u64 base = val & MASK;
> +
> +			if (!fam10h_pci_mmconf_base) {
>  				fam10h_pci_mmconf_base = base;
> -				fam10h_pci_mmconf_base_status = 1;
>  				return;
>  			} else if (fam10h_pci_mmconf_base ==  base)
>  				return;
> @@ -206,8 +200,10 @@ void __cpuinit fam10h_check_enable_mmcfg
>  	 * with 256 buses
>  	 */
>  	get_fam10h_pci_mmconf_base();
> -	if (fam10h_pci_mmconf_base_status <= 0)
> +	if (!fam10h_pci_mmconf_base) {
> +		pci_probe &= ~PCI_CHECK_ENABLE_AMD_MMCONF;
>  		return;
> +	}
>  
>  	printk(KERN_INFO "Enable MMCONFIG on AMD Family 10h\n");
>  	val &= ~((FAM10H_MMIO_CONF_BASE_MASK<<FAM10H_MMIO_CONF_BASE_SHIFT) |
> 
> 

looks like fam10h_pci_mmconf_base_status should NOT be removed.

-1: mean we can not find right new base on BSP, so should not let AP to mess it again.

Thanks

	Yinghai

Re: [PATCH] x86-64: fix and clean up AMD Fam10 MMCONF enabling

[Jan Beulich]

>>> On 05.11.10 at 20:02, Yinghai Lu <yinghai@kernel.org> wrote:
> On 11/04/2010 08:22 AM, Jan Beulich wrote:
>> @@ -123,9 +123,9 @@ static void __cpuinit get_fam10h_pci_mmc
>>  		if (!(reg & 3))
>>  			continue;
>>  
>> -		start = (((u64)reg) << 8) & (0xffULL << 32); /* 39:16 on 31:8*/
>> +		start = (u64)(reg & 0xffffff00) << 8; /* 39:16 on 31:8*/
>>  		reg = read_pci_config(bus, slot, 1, 0x84 + (i << 3));
>> -		end = (((u64)reg) << 8) & (0xffULL << 32); /* 39:16 on 31:8*/
>> +		end = ((u64)(reg & 0xffffff00) << 8) | 0xffff; /* 39:16 on 31:8*/
>>  
>>  		if (!end)
>>  			continue;
> 
> those section could be changed to:
> 
>                 start = reg & 0xffffff00; /* 39:16 on 31:8*/
>                 start <<= 8;
>                 reg = read_pci_config(bus, slot, 1, 0x84 + (i << 3));
>                 end = (reg & 0xffffff00);
>                 end <<= 8;
> 
>                 if (!end)
>                         continue;
>                 end |= 0xffff;
> 
>                 hi_mmio_num = add_range(range, 8, hi_mmio_num, start, end + 1);
> 
> 1. append 0xffff after !end checking..

Yes, this one I missed, but it gets fixed by the follow-up patch
anyway (if I'm to re-submit, I'll fold in that second patch anyway).

> 2. add_range should take end + 1;

That's only after your change making use of this function.

>> @@ -206,8 +200,10 @@ void __cpuinit fam10h_check_enable_mmcfg
>>  	 * with 256 buses
>>  	 */
>>  	get_fam10h_pci_mmconf_base();
>> -	if (fam10h_pci_mmconf_base_status <= 0)
>> +	if (!fam10h_pci_mmconf_base) {
>> +		pci_probe &= ~PCI_CHECK_ENABLE_AMD_MMCONF;
>>  		return;
>> +	}
>>  
>>  	printk(KERN_INFO "Enable MMCONFIG on AMD Family 10h\n");
>>  	val &= ~((FAM10H_MMIO_CONF_BASE_MASK<<FAM10H_MMIO_CONF_BASE_SHIFT) |
>> 
>> 
> 
> looks like fam10h_pci_mmconf_base_status should NOT be removed.
> 
> -1: mean we can not find right new base on BSP, so should not let AP to mess 
> it again.

Note that is now achieved by clearing PCI_CHECK_ENABLE_AMD_MMCONF
from pci_probe (imo a better way, as it inhibits code elsewhere which
is protected by checks of this bit).

Jan

Re: [PATCH] x86-64: fix and clean up AMD Fam10 MMCONF enabling

[Yinghai Lu]

On 11/08/2010 12:00 AM, Jan Beulich wrote:
>>>> On 05.11.10 at 20:02, Yinghai Lu <yinghai@kernel.org> wrote:
>>
>> looks like fam10h_pci_mmconf_base_status should NOT be removed.
>>
>> -1: mean we can not find right new base on BSP, so should not let AP to mess 
>> it again.
> 
> Note that is now achieved by clearing PCI_CHECK_ENABLE_AMD_MMCONF
> from pci_probe (imo a better way, as it inhibits code elsewhere which
> is protected by checks of this bit).

Then, please put that change in another patch.

Thanks

	Yinghai