lkml.org 
[lkml]   [2010]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: your patch "x86/PCI: host mmconfig detect clean up"
On 11/05/2010 03:08 AM, Jan Beulich wrote:
>>>> On 04.11.10 at 21:22, Yinghai Lu <yinghai@kernel.org> wrote:
>> Subject: [PATCH] x86/pci: Add mmconf range into e820 for when it is from MSR
>> with amd faml0h
>>
>> for AMD Fam10h, it we read mmconf from MSR early, we should just trust it
>> because we check it and correct it already.
>>
>> so add it to e820
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>> arch/x86/kernel/mmconf-fam10h_64.c | 40 +++++++++++++++++++++----------------
>> 1 file changed, 23 insertions(+), 17 deletions(-)
>>
>> Index: linux-2.6/arch/x86/kernel/mmconf-fam10h_64.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/mmconf-fam10h_64.c
>> +++ linux-2.6/arch/x86/kernel/mmconf-fam10h_64.c
>> @@ -16,6 +16,7 @@
>> #include <asm/acpi.h>
>> #include <asm/mmconfig.h>
>> #include <asm/pci_x86.h>
>> +#include <asm/e820.h>
>>
>> struct pci_hostbridge_probe {
>> u32 bus;
>> @@ -27,23 +28,26 @@ struct pci_hostbridge_probe {
>> static u64 __cpuinitdata fam10h_pci_mmconf_base;
>> static int __cpuinitdata fam10h_pci_mmconf_base_status;
>>
>> +/* only on BSP */
>> +static void __init_refok e820_add_mmconf_range(int busnbits)
>> +{
>> + u64 end;
>> +
>> + end = fam10h_pci_mmconf_base + (1ULL<<(busnbits + 20)) - 1;
>> + if (!e820_all_mapped(fam10h_pci_mmconf_base, end+1, E820_RESERVED)) {
>> + printk(KERN_DEBUG "Fam 10h mmconf [%llx, %llx]\n",
>> + fam10h_pci_mmconf_base, end);
>> + e820_add_region(fam10h_pci_mmconf_base, 1ULL<<(busnbits + 20),
>> + E820_RESERVED);
>> + sanitize_e820_map();
>
> This needs parameters passed, at least up to .37-rc1.

yes, that is in another e820 cleanup series.... it is delayed for a while because of memblock for x86.
>
> But it's pointless anyway - eventual overlaps won't matter
> anymore since memory got passed to the page allocator
> already. Consequently you really need to make sure there's
> no overlap *before* assigning the region, or (given that the
> range is being placed above TOM2 anyway), you may simply
> ignore the potential for overlaps here.

yes. we trust reading from TOM2 and MMIO split between HT chain register more.

>
>> + }
>> +}
>> +
>> static struct pci_hostbridge_probe pci_probes[] __cpuinitdata = {
>> { 0, 0x18, PCI_VENDOR_ID_AMD, 0x1200 },
>> { 0xff, 0, PCI_VENDOR_ID_AMD, 0x1200 },
>> };
>>
>> -static int __cpuinit cmp_range(const void *x1, const void *x2)
>> -{
>> - const struct range *r1 = x1;
>> - const struct range *r2 = x2;
>> - int start1, start2;
>> -
>> - start1 = r1->start >> 32;
>> - start2 = r2->start >> 32;
>> -
>> - return start1 - start2;
>> -}
>> -
>
> This (and related changes further down) doesn't seem to be
> related to addressing the problem at hand.
>
>> @@ -191,10 +195,12 @@ 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);
>> + base = val & (FAM10H_MMIO_CONF_BASE_MASK <<
>> + FAM10H_MMIO_CONF_BASE_SHIFT);
>
>
> Neither is this. I sent a fix for this (and other problems in this file)
> yesterday, and I'm afraid there's another problem here yielding
> the variant you propose incorrect: FAM10H_MMIO_CONF_BASE_MASK
> is being defined as 0xfffffff, thus shifting it by 20 bits will produce
> 0xfffffffffff00000 (sign extended from 0xfff00000) instead of the
> expected 0xfffffff00000. This is also affecting the other two uses of
> this constant (though I admit that it is only a theoretical problem as
> the upper bits are defined as read-as-zero). I'll soon send a fix for
> this and some other problem I found in this code just now.

yes.
FAM10H_MMIO_CONF_BASE_MASK should have ULL

Thanks

Yinghai


\
 
 \ /
  Last update: 2010-11-05 19:09    [from the cache]
©2003-2011 Jasper Spaans