Messages in this thread Patch in this message | | | Date | Fri, 05 Nov 2010 12:02:41 -0700 | From | Yinghai Lu <> | Subject | Re: [PATCH] x86-64: fix and clean up AMD Fam10 MMCONF enabling |
| |
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
| |