lkml.org 
[lkml]   [2010]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    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    [W:0.037 / U:0.012 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site