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); > + > + /* 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 Myron -- Myron Stowe HP Open Source Linux Lab (OSLL) -- 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
