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 <[email protected]>
Cc: Yinghai Lu <[email protected]>
---
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;
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 <[email protected]>
> Cc: Yinghai Lu <[email protected]>
>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>>> On 05.11.10 at 14:07, Andreas Herrmann <[email protected]>
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
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 <[email protected]>
> Cc: Yinghai Lu <[email protected]>
>
> ---
> 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;
>
>
>
>>> On 05.11.10 at 18:51, Yinghai Lu <[email protected]> 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
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.
>>> On 08.11.10 at 17:13, "H. Peter Anvin" <[email protected]> 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
On 11/08/2010 12:04 AM, Jan Beulich wrote:
>>>> On 05.11.10 at 18:51, Yinghai Lu <[email protected]> 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