On 01/14/2010 02:23 PM, Myron Stowe wrote: > On Wed, 2010-01-13 at 23:16 -0800, Yinghai Lu wrote: >> On 01/13/2010 01:52 PM, Myron Stowe wrote: >>> On Wed, 2010-01-13 at 13:34 -0800, Yinghai Lu wrote: >>>> On 01/13/2010 01:19 PM, Myron Stowe wrote: >>>>> Intel's x58 chipset (IOH) includes a VTBAR (Base Address Register for >>>>> Intel VT-d Chipset Registers) that specifies an 8K aligned system memory >>>>> address for VT-d memory mapped registers. If BIOS has placed the VT-d >>>>> register window within the Local MMIOL range, it will not be available >>>>> for downstream PCI devices, so remove it from the host bridge's MMIOL >>>>> range. >>>>> >>>>> This patch determines if the VTBAR is enabled and modifies the IOH's >>>>> Local MMIOL range if it is programmed within such. >>>>> >>>>> >>>>> Reference: "Intel X58 Express Chipset Datasheet" >>>>> http://www.intel.com/Assets/PDF/datasheet/320838.pdf >>>>> >>>>> >>>>> Signed-off-by: Myron Stowe <myron.stowe@xxxxxx> >>>>> --- >>>>> >>>>> arch/x86/pci/intel_bus.c | 20 +++++++++++++++++++- >>>>> 1 files changed, 19 insertions(+), 1 deletions(-) >>>>> >>>>> diff --git a/arch/x86/pci/intel_bus.c b/arch/x86/pci/intel_bus.c >>>>> index b7a55dc..f58136c 100644 >>>>> --- a/arch/x86/pci/intel_bus.c >>>>> +++ b/arch/x86/pci/intel_bus.c >>>>> @@ -39,13 +39,16 @@ static inline void print_ioh_resources(struct pci_root_info *info) >>>>> #define IOH_LMMIOH_LIMITU 0x118 >>>>> #define IOH_LCFGBUS 0x11c >>>>> >>>>> +#define IOH_VTBAR 0x180 >>>>> +#define IOH_VTSIZE 0x2000 /* Fixed HW size (not programmable) */ >>>>> + >>>>> static void __devinit pci_root_bus_res(struct pci_dev *dev) >>>>> { >>>>> u16 word; >>>>> u32 dword; >>>>> struct pci_root_info *info; >>>>> u16 io_base, io_end; >>>>> - u32 mmiol_base, mmiol_end; >>>>> + u32 mmiol_base, mmiol_end, vtbar; >>>>> u64 mmioh_base, mmioh_end; >>>>> int bus_base, bus_end; >>>>> >>>>> @@ -72,6 +75,21 @@ static void __devinit pci_root_bus_res(struct pci_dev *dev) >>>>> pci_read_config_dword(dev, IOH_LMMIOL, &dword); >>>>> mmiol_base = (dword & 0xff00) << (24 - 8); >>>>> mmiol_end = (dword & 0xff000000) | 0xffffff; >>>>> + pci_read_config_dword(dev, IOH_VTBAR, &dword); >>>>> + vtbar = dword & 0xfffffffe; >>>>> + if (dword & 0x1 && >>>>> + (mmiol_base < vtbar + IOH_VTSIZE - 1 && vtbar < mmiol_end)) { >>>>> + /* remove VT-d DRDH from Local MMIOL window */ >>>>> + if (vtbar <= mmiol_base) >>>>> + mmiol_base = vtbar + IOH_VTSIZE; >>>>> + else if (mmiol_end <= vtbar + IOH_VTSIZE - 1) >>>>> + mmiol_end = vtbar; >>>>> + else { >>>>> + update_res(info, mmiol_base, vtbar - 1, >>>>> + IORESOURCE_MEM, 0); >>>>> + mmiol_base = vtbar + IOH_VTSIZE; >>>>> + } >>>>> + } >>>>> update_res(info, mmiol_base, mmiol_end, IORESOURCE_MEM, 0); >>>>> >>>>> pci_read_config_dword(dev, IOH_LMMIOH, &dword); >>>> >>>> can you add vt_base and vt_end instead of use + IOH_VTSIZE everywhere? >>> >>> I can. I purposely chose not to, since the VTBAR does not work in the >>> same manner as the other range related registers that this file >>> accesses, thinking that doing such could be a little misleading. >>> >>> Knowing that the VTBAR functions differently from the other range >>> registers do you still feel such a change would have some benefits? >> >> i mean sth like this >> >> Subject: [PATCH -v2] PCI: Exclude VTBAR range from Local MMIOL if necessary >> To: jbarnes@xxxxxxxxxxxxxxxx >> From: Myron Stowe <myron.stowe@xxxxxx> >> Cc: linux-pci@xxxxxxxxxxxxxxx, yinghai@xxxxxxxxxx >> >> Intel's x58 chipset (IOH) includes a VTBAR (Base Address Register for >> Intel VT-d Chipset Registers) that specifies an 8K aligned system memory >> address for VT-d memory mapped registers. If BIOS has placed the VT-d >> register window within the Local MMIOL range, it will not be available >> for downstream PCI devices, so remove it from the host bridge's MMIOL >> range. >> >> This patch determines if the VTBAR is enabled and modifies the IOH's >> Local MMIOL range if it is programmed within such. >> >> >> Reference: "Intel X58 Express Chipset Datasheet" >> http://www.intel.com/Assets/PDF/datasheet/320838.pdf >> >> -v2: yinghai update it with more complete range comparing >> and change 0x120 to 0x200 again >> >> --- >> >> arch/x86/pci/intel_bus.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 43 insertions(+), 2 deletions(-) >> >> Index: linux-2.6/arch/x86/pci/intel_bus.c >> =================================================================== >> --- linux-2.6.orig/arch/x86/pci/intel_bus.c >> +++ linux-2.6/arch/x86/pci/intel_bus.c >> @@ -41,6 +41,9 @@ static inline void print_ioh_resources(s >> #define IOH_LMMIOH_LIMITU 0x118 >> #define IOH_LCFGBUS 0x11c >> >> +#define IOH_VTBAR 0x180 >> +#define IOH_VTSIZE 0x2000 /* Fixed HW size (not programmable) */ >> + >> static void __devinit pci_root_bus_res(struct pci_dev *dev) >> { >> u16 word; >> @@ -52,7 +55,7 @@ static void __devinit pci_root_bus_res(s >> int bus_base, bus_end; >> >> /* some sys doesn't get mmconf enabled */ >> - if (dev->cfg_size < 0x120) >> + if (dev->cfg_size < 0x200) >> return; >> >> if (pci_root_num >= PCI_ROOT_NR) { >> @@ -78,7 +81,45 @@ static void __devinit pci_root_bus_res(s >> pci_read_config_dword(dev, IOH_LMMIOL, &dword); >> mmiol_base = (dword & 0xff00) << (24 - 8); >> mmiol_end = (dword & 0xff000000) | 0xffffff; >> - update_res(info, mmiol_base, mmiol_end, IORESOURCE_MEM, 0); >> + pci_read_config_dword(dev, IOH_VTBAR, &dword); >> + if (dword & 0x1) { >> + u32 vt_base, vt_end; >> + u32 overlap_base, overlap_end; >> + >> + vt_base = dword & 0xfffffffe; >> + vt_end = vt_base + IOH_VTSIZE - 1; >> + >> + /* totally covered by vt range? */ >> + if (mmiol_base >= vt_base && mmiol_end <= vt_end) >> + goto out; >> + >> + /* vt range is totally covered? */ >> + if (mmiol_base < vt_base && mmiol_end > vt_end) { >> + update_res(info, mmiol_base, vt_base - 1, >> + IORESOURCE_MEM, 0); >> + update_res(info, vt_end + 1, mmiol_end, >> + IORESOURCE_MEM, 0); >> + goto out; >> + } >> + >> + /* partially covered */ >> + overlap_base = max(vt_base, mmiol_base); >> + overlap_end = min(vt_end, mmiol_end); >> + if (overlap_base > overlap_end) >> + update_res(info, mmiol_base, mmiol_end, >> + IORESOURCE_MEM, 0); missed one goto here... >> + >> + /* left range could be head or tail */ >> + if (mmiol_base < overlap_base) >> + update_res(info, mmiol_base, overlap_base - 1, >> + IORESOURCE_MEM, 0); >> + else >> + update_res(info, overlap_end + 1, mmiol_end, >> + IORESOURCE_MEM, 0); >> +out: >> + ; >> + } else >> + update_res(info, mmiol_base, mmiol_end, IORESOURCE_MEM, 0); >> >> pci_read_config_dword(dev, IOH_LMMIOH, &dword); >> mmioh_base = ((u64)(dword & 0xfc00)) << (26 - 10); >> > > I understood what you were suggesting as I originally started out with > basically the same approach. I then started re-working the code, and > with some refactoring, I was able to both simplify it and denote the > difference in how the VTBAR register functions with respect to the MMIO > related range registers. I believe that if one were to refactor the > approach you suggest the result would basically be what I ended up with. > > Perhaps it can be argued that my refactoring resulted in code that is > too optimized or dense. This is what I would like to receive feedback > upon from others. Is this your main concern? > > > In reviewing the alternate approach I encountered a number of issues. I > attempted to try and correct the issues but gave up. Here is a set of > scenarios that the patch covers and the errors I discovered with the > alternative - > > The Local MMIOL range is depected by "====" and the VTBAR register > window is depicted by "++++". Example address values for both are to the > right. The scenarios, a-i, cover the possible cases (scenarios a and b > are effectively the same (as are h and i)). I substituted the example > values for the code and annotated where the issues are for each scenario > at issue. > > ============ 0xb4000000 - 0xb4ffffff (mmiol_base - mmiol_end) > | | > a +++++ | | see scenario b > b +++++| | 0xb3ffe000 - 0xb3ffffff > c +++++ | 0xb3fff000 - 0xb4000fff > d +++++ | 0xb4000000 - 0xb4001fff > e | +++++ | 0xb4004000 - 0xb4005fff > f | +++++ 0xb4ffe000 - 0xb4ffffff > g | +++++ 0xb4fff000 - 0xb5000fff > h | |+++++ 0xb5000000 - 0xb5001fff > i | | +++++ see scenario h > | | > > mmiol_base = 0xb4000000 > mmiol_end = 0xb4ffffff > > Scenario b [VTBAR = 0xb3ffe000 - 0xb3ffffff] > > if (0xb4000000 >= 0xb3ffe000 && 0xb4ffffff <= 0xb3ffffff) > not taken > if (0xb4000000 < 0xb3ffe000 ... short circuits if block > > overlap_base = 0xb4000000; > overlap_end = 0xb3ffe000; > if (0xb4000000 > 0xb3ffe000) > update_res(info, 0xb4000000, 0xb4ffffff, x, 0); > if (0xb4000000 < 0xb4000000) > not taken > else > *: update_res(info, 0xb3ffe001, 0xb3ffffff, x 0); > > *Incorrect: Doesn't detect that the VTBAR does not reside within the > LMMIOL range and errornously adds a range that is larger than LMMIOL > > > Scenario f [VTBAR = 0xb4ffe000 - 0xb4ffffff] > > if (0xb4000000 >= 0xb4ffe000 ... short circuits if block > > if (0xb4000000 < 0xb4ffe000 && 0xb4ffffff > 0xb4ffffff) > not taken > > overlap_base = 0xb4ffe000; > overlap_end = 0xb4ffffff; > if (0xb4ffe000 > 0xb4ffffff) > *: update_res(info, 0xb4000000, 0xb4ffffff, x, 0); > if (0xb4000000 < 0xb4ffe000) > update_res(info, 0xb4000000, 0xb4ffdfff, x, 0); > else > not taken > > *Incorrect: This range doesn't take the VTBAR into effect and ends up > overlapping with the next part of the range > > > Scenario h [VTBAR = 0xb5000000 - 0xb5001fff] > > if (0xb4000000 >= 0xb5000000 ... short circuits if block > > if (0xb4000000 < 0xb5000000 && 0xb4ffffff > 0xb5001fff) > not taken > > overlap_base = 0xb5000000; > overlap_end = 0xb4000000; > if (0xb5000000 > 0xb4000000) > update_res(info, 0xb4000000, 0xb4ffffff, x, 0); > if (0xb4000000 < 0xb5000000) > *: update_res(info, 0xb4000000, 0xb4ffffff, x, 0); > else > not taken > > *Incorrect: Adds a duplicate range > Subject: [PATCH -v2] PCI: Exclude VTBAR range from Local MMIOL if necessary To: jbarnes@xxxxxxxxxxxxxxxx From: Myron Stowe <myron.stowe@xxxxxx> Cc: linux-pci@xxxxxxxxxxxxxxx, yinghai@xxxxxxxxxx Intel's x58 chipset (IOH) includes a VTBAR (Base Address Register for Intel VT-d Chipset Registers) that specifies an 8K aligned system memory address for VT-d memory mapped registers. If BIOS has placed the VT-d register window within the Local MMIOL range, it will not be available for downstream PCI devices, so remove it from the host bridge's MMIOL range. This patch determines if the VTBAR is enabled and modifies the IOH's Local MMIOL range if it is programmed within such. Reference: "Intel X58 Express Chipset Datasheet" http://www.intel.com/Assets/PDF/datasheet/320838.pdf -v2: yinghai update it with more complete range comparing and change 0x120 to 0x200 again -v3: yinghai update it with add/subtract_range --- arch/x86/pci/intel_bus.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) Index: linux-2.6/arch/x86/pci/intel_bus.c =================================================================== --- linux-2.6.orig/arch/x86/pci/intel_bus.c +++ linux-2.6/arch/x86/pci/intel_bus.c @@ -41,6 +41,11 @@ static inline void print_ioh_resources(s #define IOH_LMMIOH_LIMITU 0x118 #define IOH_LCFGBUS 0x11c +#define IOH_VTBAR 0x180 +#define IOH_VTSIZE 0x2000 /* Fixed HW size (not programmable) */ + +#define RANGE_NUM 16 + static void __devinit pci_root_bus_res(struct pci_dev *dev) { u16 word; @@ -50,9 +55,11 @@ static void __devinit pci_root_bus_res(s u32 mmiol_base, mmiol_end; u64 mmioh_base, mmioh_end; int bus_base, bus_end; + struct range range[RANGE_NUM]; + int i; /* some sys doesn't get mmconf enabled */ - if (dev->cfg_size < 0x120) + if (dev->cfg_size < 0x200) return; if (pci_root_num >= PCI_ROOT_NR) { @@ -78,7 +85,25 @@ static void __devinit pci_root_bus_res(s pci_read_config_dword(dev, IOH_LMMIOL, &dword); mmiol_base = (dword & 0xff00) << (24 - 8); mmiol_end = (dword & 0xff000000) | 0xffffff; - update_res(info, mmiol_base, mmiol_end, IORESOURCE_MEM, 0); + memset(range, 0, sizeof(range)); + add_range(range, RANGE_NUM, 0, mmiol_base, (u64)mmiol_end + 1); + pci_read_config_dword(dev, IOH_VTBAR, &dword); + if (dword & 0x1) { + u32 vt_base, vt_end; + + vt_base = dword & 0xfffffffe; + vt_end = vt_base + IOH_VTSIZE - 1; + + subtract_range(range, RANGE_NUM, vt_base, vt_end + 1); + } + for (i = 0; i < RANGE_NUM; i++) { + if (!range[i].end) + continue; + + update_res(info, cap_resource(range[i].start), + cap_resource(range[i].end - 1), + IORESOURCE_MEM, 0); + } pci_read_config_dword(dev, IOH_LMMIOH, &dword); mmioh_base = ((u64)(dword & 0xfc00)) << (26 - 10); -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
