[PATCH] x86-64: more fixes and cleanup to AMD Fam10 MMCONF enabling

[PATCH] x86-64: more fixes and cleanup to AMD Fam10 MMCONF enabling

[Jan Beulich]

Unfortunately it turned out the original code had more issues: We want
to place the region above 4G in any case (even if TOM2 isn't enabled
or invalid), and the base mask definition was improperly typed (thus
causing shifts by FAM10H_MMIO_CONF_BASE_SHIFT to produce other than
the intended result). Fixing this in turn allowed simplifying the MMIO
region detection code, as regions ending below TOM2 now aren't of
interest anymore.

This will only apply cleanly on top of yesterday's patch titled
"x86-64: fix and clean up AMD Fam10 MMCONF enabling".

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

---
 arch/x86/include/asm/msr-index.h   |    2 +-
 arch/x86/kernel/mmconf-fam10h_64.c |   12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

--- 2.6.37-rc1/arch/x86/include/asm/msr-index.h
+++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/include/asm/msr-index.h
@@ -128,7 +128,7 @@
 #define FAM10H_MMIO_CONF_ENABLE		(1<<0)
 #define FAM10H_MMIO_CONF_BUSRANGE_MASK	0xf
 #define FAM10H_MMIO_CONF_BUSRANGE_SHIFT 2
-#define FAM10H_MMIO_CONF_BASE_MASK	0xfffffff
+#define FAM10H_MMIO_CONF_BASE_MASK	0xfffffffULL
 #define FAM10H_MMIO_CONF_BASE_SHIFT	20
 #define MSR_FAM10H_NODE_ID		0xc001100c
 
--- 2.6.37-rc1-x86_64-mmconf-fam10h.orig/arch/x86/kernel/mmconf-fam10h_64.c
+++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/kernel/mmconf-fam10h_64.c
@@ -43,7 +43,7 @@ static int __cpuinit cmp_range(const voi
 	return start1 - start2;
 }
 
-#define UNIT (1ULL << (5 + 3 + 12))
+#define UNIT (1ULL << FAM10H_MMIO_CONF_BASE_SHIFT)
 #define MASK (~(UNIT - 1))
 #define SIZE (UNIT << 8)
 /* need to avoid (0xfd<<32) and (0xfe<<32), ht used space */
@@ -99,12 +99,12 @@ static void __cpuinit get_fam10h_pci_mmc
 
 	/* TOP_MEM2 is not enabled? */
 	if (!(val & (1<<21))) {
-		tom2 = 0;
+		tom2 = 1ULL << 32;
 	} else {
 		/* TOP_MEM2 */
 		address = MSR_K8_TOP_MEM2;
 		rdmsrl(address, val);
-		tom2 = val & 0xffffff800000ULL;
+		tom2 = max(val & 0xffffff800000ULL, 1ULL << 32);
 	}
 
 	if (base <= tom2)
@@ -127,7 +127,7 @@ static void __cpuinit get_fam10h_pci_mmc
 		reg = read_pci_config(bus, slot, 1, 0x84 + (i << 3));
 		end = ((u64)(reg & 0xffffff00) << 8) | 0xffff; /* 39:16 on 31:8*/
 
-		if (!end)
+		if (end < tom2)
 			continue;
 
 		range[hi_mmio_num].start = start;
@@ -151,13 +151,13 @@ static void __cpuinit get_fam10h_pci_mmc
 	if ((base > tom2) && BASE_VALID(base))
 		goto out;
 	base = (range[hi_mmio_num - 1].end + UNIT) & MASK;
-	if ((base > tom2) && BASE_VALID(base))
+	if (BASE_VALID(base))
 		goto out;
 	/* need to find window between ranges */
 	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))
+		if (val >= base + SIZE && BASE_VALID(base))
 			goto out;
 	}
 	return;

Re: [PATCH] x86-64: more fixes and cleanup to AMD Fam10 MMCONF enabling

[Andreas Herrmann]

On Fri, Nov 05, 2010 at 10:59:10AM +0000, Jan Beulich wrote:
> Unfortunately it turned out the original code had more issues: We want
> to place the region above 4G in any case (even if TOM2 isn't enabled
> or invalid), and the base mask definition was improperly typed (thus

I am just curios. Do you actually have access to a system where TOM2
isn't properly configured?


Thanks,

Andreas

> causing shifts by FAM10H_MMIO_CONF_BASE_SHIFT to produce other than
> the intended result). Fixing this in turn allowed simplifying the MMIO
> region detection code, as regions ending below TOM2 now aren't of
> interest anymore.
> 
> This will only apply cleanly on top of yesterday's patch titled
> "x86-64: fix and clean up AMD Fam10 MMCONF enabling".
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  arch/x86/include/asm/msr-index.h   |    2 +-
>  arch/x86/kernel/mmconf-fam10h_64.c |   12 ++++++------
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> --- 2.6.37-rc1/arch/x86/include/asm/msr-index.h
> +++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/include/asm/msr-index.h
> @@ -128,7 +128,7 @@
>  #define FAM10H_MMIO_CONF_ENABLE		(1<<0)
>  #define FAM10H_MMIO_CONF_BUSRANGE_MASK	0xf
>  #define FAM10H_MMIO_CONF_BUSRANGE_SHIFT 2
> -#define FAM10H_MMIO_CONF_BASE_MASK	0xfffffff
> +#define FAM10H_MMIO_CONF_BASE_MASK	0xfffffffULL
>  #define FAM10H_MMIO_CONF_BASE_SHIFT	20
>  #define MSR_FAM10H_NODE_ID		0xc001100c
>  
> --- 2.6.37-rc1-x86_64-mmconf-fam10h.orig/arch/x86/kernel/mmconf-fam10h_64.c
> +++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/kernel/mmconf-fam10h_64.c
> @@ -43,7 +43,7 @@ static int __cpuinit cmp_range(const voi
>  	return start1 - start2;
>  }
>  
> -#define UNIT (1ULL << (5 + 3 + 12))
> +#define UNIT (1ULL << FAM10H_MMIO_CONF_BASE_SHIFT)
>  #define MASK (~(UNIT - 1))
>  #define SIZE (UNIT << 8)
>  /* need to avoid (0xfd<<32) and (0xfe<<32), ht used space */
> @@ -99,12 +99,12 @@ static void __cpuinit get_fam10h_pci_mmc
>  
>  	/* TOP_MEM2 is not enabled? */
>  	if (!(val & (1<<21))) {
> -		tom2 = 0;
> +		tom2 = 1ULL << 32;
>  	} else {
>  		/* TOP_MEM2 */
>  		address = MSR_K8_TOP_MEM2;
>  		rdmsrl(address, val);
> -		tom2 = val & 0xffffff800000ULL;
> +		tom2 = max(val & 0xffffff800000ULL, 1ULL << 32);
>  	}
>  
>  	if (base <= tom2)
> @@ -127,7 +127,7 @@ static void __cpuinit get_fam10h_pci_mmc
>  		reg = read_pci_config(bus, slot, 1, 0x84 + (i << 3));
>  		end = ((u64)(reg & 0xffffff00) << 8) | 0xffff; /* 39:16 on 31:8*/
>  
> -		if (!end)
> +		if (end < tom2)
>  			continue;
>  
>  		range[hi_mmio_num].start = start;
> @@ -151,13 +151,13 @@ static void __cpuinit get_fam10h_pci_mmc
>  	if ((base > tom2) && BASE_VALID(base))
>  		goto out;
>  	base = (range[hi_mmio_num - 1].end + UNIT) & MASK;
> -	if ((base > tom2) && BASE_VALID(base))
> +	if (BASE_VALID(base))
>  		goto out;
>  	/* need to find window between ranges */
>  	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))
> +		if (val >= base + SIZE && BASE_VALID(base))
>  			goto out;
>  	}
>  	return;
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] x86-64: more fixes and cleanup to AMD Fam10 MMCONF enabling

[Jan Beulich]

>>> On 05.11.10 at 14:07, Andreas Herrmann <herrmann.der.user@googlemail.com>
wrote:
> On Fri, Nov 05, 2010 at 10:59:10AM +0000, Jan Beulich wrote:
>> Unfortunately it turned out the original code had more issues: We want
>> to place the region above 4G in any case (even if TOM2 isn't enabled
>> or invalid), and the base mask definition was improperly typed (thus
> 
> I am just curios. Do you actually have access to a system where TOM2
> isn't properly configured?

No - but since it was relatively obvious that the TOM2-not-set case
needed correction, it seemed simple and reasonable to also adjust
the TOM2-set-to-below-4G case.

Jan

Re: [PATCH] x86-64: more fixes and cleanup to AMD Fam10 MMCONF enabling

[Yinghai Lu]

On 11/05/2010 03:59 AM, Jan Beulich wrote:
> Unfortunately it turned out the original code had more issues: We want
> to place the region above 4G in any case (even if TOM2 isn't enabled
> or invalid), and the base mask definition was improperly typed (thus
> causing shifts by FAM10H_MMIO_CONF_BASE_SHIFT to produce other than
> the intended result). Fixing this in turn allowed simplifying the MMIO
> region detection code, as regions ending below TOM2 now aren't of
> interest anymore.
> 
> This will only apply cleanly on top of yesterday's patch titled
> "x86-64: fix and clean up AMD Fam10 MMCONF enabling".

I don't think we have systems that have Enable bit set, but TOM2 < 4G.

Thanks

	Yinghai

> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  arch/x86/include/asm/msr-index.h   |    2 +-
>  arch/x86/kernel/mmconf-fam10h_64.c |   12 ++++++------
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> --- 2.6.37-rc1/arch/x86/include/asm/msr-index.h
> +++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/include/asm/msr-index.h
> @@ -128,7 +128,7 @@
>  #define FAM10H_MMIO_CONF_ENABLE		(1<<0)
>  #define FAM10H_MMIO_CONF_BUSRANGE_MASK	0xf
>  #define FAM10H_MMIO_CONF_BUSRANGE_SHIFT 2
> -#define FAM10H_MMIO_CONF_BASE_MASK	0xfffffff
> +#define FAM10H_MMIO_CONF_BASE_MASK	0xfffffffULL
>  #define FAM10H_MMIO_CONF_BASE_SHIFT	20
>  #define MSR_FAM10H_NODE_ID		0xc001100c
>  
> --- 2.6.37-rc1-x86_64-mmconf-fam10h.orig/arch/x86/kernel/mmconf-fam10h_64.c
> +++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/kernel/mmconf-fam10h_64.c
> @@ -43,7 +43,7 @@ static int __cpuinit cmp_range(const voi
>  	return start1 - start2;
>  }
>  
> -#define UNIT (1ULL << (5 + 3 + 12))
> +#define UNIT (1ULL << FAM10H_MMIO_CONF_BASE_SHIFT)
>  #define MASK (~(UNIT - 1))
>  #define SIZE (UNIT << 8)
>  /* need to avoid (0xfd<<32) and (0xfe<<32), ht used space */
> @@ -99,12 +99,12 @@ static void __cpuinit get_fam10h_pci_mmc
>  
>  	/* TOP_MEM2 is not enabled? */
>  	if (!(val & (1<<21))) {
> -		tom2 = 0;
> +		tom2 = 1ULL << 32;
>  	} else {
>  		/* TOP_MEM2 */
>  		address = MSR_K8_TOP_MEM2;
>  		rdmsrl(address, val);
> -		tom2 = val & 0xffffff800000ULL;
> +		tom2 = max(val & 0xffffff800000ULL, 1ULL << 32);
>  	}
>  
>  	if (base <= tom2)
> @@ -127,7 +127,7 @@ static void __cpuinit get_fam10h_pci_mmc
>  		reg = read_pci_config(bus, slot, 1, 0x84 + (i << 3));
>  		end = ((u64)(reg & 0xffffff00) << 8) | 0xffff; /* 39:16 on 31:8*/
>  
> -		if (!end)
> +		if (end < tom2)
>  			continue;
>  
>  		range[hi_mmio_num].start = start;
> @@ -151,13 +151,13 @@ static void __cpuinit get_fam10h_pci_mmc
>  	if ((base > tom2) && BASE_VALID(base))
>  		goto out;
>  	base = (range[hi_mmio_num - 1].end + UNIT) & MASK;
> -	if ((base > tom2) && BASE_VALID(base))
> +	if (BASE_VALID(base))
>  		goto out;
>  	/* need to find window between ranges */
>  	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))
> +		if (val >= base + SIZE && BASE_VALID(base))
>  			goto out;
>  	}
>  	return;
> 
> 
> 

Re: [PATCH] x86-64: more fixes and cleanup to AMD Fam10 MMCONF enabling

[Jan Beulich]

>>> On 05.11.10 at 18:51, Yinghai Lu <yinghai@kernel.org> wrote:
> On 11/05/2010 03:59 AM, Jan Beulich wrote:
>> Unfortunately it turned out the original code had more issues: We want
>> to place the region above 4G in any case (even if TOM2 isn't enabled
>> or invalid), and the base mask definition was improperly typed (thus
>> causing shifts by FAM10H_MMIO_CONF_BASE_SHIFT to produce other than
>> the intended result). Fixing this in turn allowed simplifying the MMIO
>> region detection code, as regions ending below TOM2 now aren't of
>> interest anymore.
>> 
>> This will only apply cleanly on top of yesterday's patch titled
>> "x86-64: fix and clean up AMD Fam10 MMCONF enabling".
> 
> I don't think we have systems that have Enable bit set, but TOM2 < 4G.

I suppose that's true, but making the kernel independent of
BIOS flaws (especially when it's that cheap) seems like a good
idea to me. But of course, if that's what would be hindering
acceptance of the patch, I'd re-submit with that part dropped.

Jan

Re: [PATCH] x86-64: more fixes and cleanup to AMD Fam10 MMCONF enabling

[Yinghai Lu]

On 11/08/2010 12:04 AM, Jan Beulich wrote:
>>>> On 05.11.10 at 18:51, Yinghai Lu <yinghai@kernel.org> wrote:
>> On 11/05/2010 03:59 AM, Jan Beulich wrote:
>>> Unfortunately it turned out the original code had more issues: We want
>>> to place the region above 4G in any case (even if TOM2 isn't enabled
>>> or invalid), and the base mask definition was improperly typed (thus
>>> causing shifts by FAM10H_MMIO_CONF_BASE_SHIFT to produce other than
>>> the intended result). Fixing this in turn allowed simplifying the MMIO
>>> region detection code, as regions ending below TOM2 now aren't of
>>> interest anymore.
>>>
>>> This will only apply cleanly on top of yesterday's patch titled
>>> "x86-64: fix and clean up AMD Fam10 MMCONF enabling".
>>
>> I don't think we have systems that have Enable bit set, but TOM2 < 4G.
> 
> I suppose that's true, but making the kernel independent of
> BIOS flaws (especially when it's that cheap) seems like a good
> idea to me. But of course, if that's what would be hindering
> acceptance of the patch, I'd re-submit with that part dropped.

I'm ok with your change.

Please come out one updated version with meaningful MACRO.

Thanks

	Yinghai

Re: [PATCH] x86-64: more fixes and cleanup to AMD Fam10 MMCONF enabling

[H. Peter Anvin]

On 11/05/2010 03:59 AM, Jan Beulich wrote:
>  
> --- 2.6.37-rc1-x86_64-mmconf-fam10h.orig/arch/x86/kernel/mmconf-fam10h_64.c
> +++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/kernel/mmconf-fam10h_64.c
> @@ -43,7 +43,7 @@ static int __cpuinit cmp_range(const voi
>  	return start1 - start2;
>  }
>  
> -#define UNIT (1ULL << (5 + 3 + 12))
> +#define UNIT (1ULL << FAM10H_MMIO_CONF_BASE_SHIFT)
>  #define MASK (~(UNIT - 1))
>  #define SIZE (UNIT << 8)

Could we avoid macros named UNIT, MASK, and SIZE at all?  I realize
they're already in the code, but still...

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] x86-64: more fixes and cleanup to AMD Fam10 MMCONF enabling

[Jan Beulich]

>>> On 08.11.10 at 17:13, "H. Peter Anvin" <hpa@zytor.com> wrote:
> On 11/05/2010 03:59 AM, Jan Beulich wrote:
>>  
>> --- 2.6.37-rc1-x86_64-mmconf-fam10h.orig/arch/x86/kernel/mmconf-fam10h_64.c
>> +++ 2.6.37-rc1-x86_64-mmconf-fam10h/arch/x86/kernel/mmconf-fam10h_64.c
>> @@ -43,7 +43,7 @@ static int __cpuinit cmp_range(const voi
>>  	return start1 - start2;
>>  }
>>  
>> -#define UNIT (1ULL << (5 + 3 + 12))
>> +#define UNIT (1ULL << FAM10H_MMIO_CONF_BASE_SHIFT)
>>  #define MASK (~(UNIT - 1))
>>  #define SIZE (UNIT << 8)
> 
> Could we avoid macros named UNIT, MASK, and SIZE at all?  I realize
> they're already in the code, but still...

I could understand if these were definition in a header, but why
do you think we need to have unnecessarily long identifiers (e.g.
by prefixing all of the defines here with FAM10H_MMIO_CONF_BASE_)
in places like this? After all, one of the two goals of using a macro
here at all is to keep things small and simple...

But sure, if just the names hinder acceptance, I can fold this and
the original patches together and use less ambiguous names.

Jan