| Message ID | 1418160871-5605-1-git-send-email-yinghai@kernel.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Dec 9, 2014 at 2:34 PM, Yinghai Lu <yinghai@kernel.org> wrote: > So we could use bridge 64bit mem pref for children mem pref instead of > forcing them into bridge mem. > > Could help Marek's system as his system is using _CRS, and all mem res is under > 4G. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=85491 > Reported-by: Marek Kordik <kordikmarek@gmail.com> > Fixes: 5b28541552ef ("PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources") > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > --- > drivers/pci/host-bridge.c | 7 +++++++ > drivers/pci/pci.h | 1 + > drivers/pci/probe.c | 9 +++++++++ > drivers/pci/setup-bus.c | 3 +++ > include/linux/pci.h | 1 + > 5 files changed, 21 insertions(+) > > Index: linux-2.6/drivers/pci/host-bridge.c > =================================================================== > --- linux-2.6.orig/drivers/pci/host-bridge.c > +++ linux-2.6/drivers/pci/host-bridge.c > @@ -31,6 +31,13 @@ void pci_set_host_bridge_release(struct > bridge->release_data = release_data; > } > > +bool pcibios_host_bridge_has_mem64_res(struct pci_bus *bus) > +{ > + struct pci_host_bridge *bridge = find_pci_host_bridge(bus); > + > + return bridge->has_mem64_res; > +} > + > void pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region, > struct resource *res) > { > Index: linux-2.6/drivers/pci/pci.h > =================================================================== > --- linux-2.6.orig/drivers/pci/pci.h > +++ linux-2.6/drivers/pci/pci.h > @@ -196,6 +196,7 @@ enum pci_bar_type { > pci_bar_mem64, /* A 64-bit memory BAR */ > }; > > +bool pcibios_host_bridge_has_mem64_res(struct pci_bus *bus); > bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl, > int crs_timeout); > int pci_setup_device(struct pci_dev *dev); > Index: linux-2.6/drivers/pci/probe.c > =================================================================== > --- linux-2.6.orig/drivers/pci/probe.c > +++ linux-2.6/drivers/pci/probe.c > @@ -1980,6 +1980,15 @@ struct pci_bus *pci_create_root_bus(stru > dev_info(&b->dev, "root bus resource %pR%s\n", res, bus_addr); > } > > + list_for_each_entry(window, &bridge->windows, list) { > + res = window->res; > + if (resource_type(res) == IORESOURCE_MEM || > + res->end > 0xffffffff) { > + bridge->has_mem64_res = true; This is an interesting idea, but I think you're checking CPU addresses here, and you need to check PCI bus addresses. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 9, 2014 at 1:53 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Tue, Dec 9, 2014 at 2:34 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> + list_for_each_entry(window, &bridge->windows, list) { >> + res = window->res; >> + if (resource_type(res) == IORESOURCE_MEM || >> + res->end > 0xffffffff) { >> + bridge->has_mem64_res = true; > > This is an interesting idea, but I think you're checking CPU addresses > here, and you need to check PCI bus addresses. Looks like those IBM platforms have res > 4g, but pci bus address < 4g. If we check pci bus address, and then we would break those platforms. Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
My mutt hang, in case the mail isn't sent out, I resend it. On Wed, Dec 10, 2014 at 10:15:37PM +0800, Wei Yang wrote: >On Tue, Dec 09, 2014 at 03:13:49PM -0800, Yinghai Lu wrote: >>On Tue, Dec 9, 2014 at 1:53 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> On Tue, Dec 9, 2014 at 2:34 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>>> + list_for_each_entry(window, &bridge->windows, list) { >>>> + res = window->res; >>>> + if (resource_type(res) == IORESOURCE_MEM || >>>> + res->end > 0xffffffff) { >>>> + bridge->has_mem64_res = true; >>> >>> This is an interesting idea, but I think you're checking CPU addresses >>> here, and you need to check PCI bus addresses. >> >>Looks like those IBM platforms have res > 4g, but pci bus address < 4g. >>If we check pci bus address, and then we would break those platforms. >> Hi, Yinghai I did some test with patch on my machine. It looks good. While as Bjorn mentioned, I think we could change it a little. The pci space on our machine is like this: cpu address pci address MEM 0x00003ff280000000..0x00003ff2fffeffff -> 0x00000000 8000 0000 MEM64 0x00003d5000000000..0x00003d5fffffffff -> 0x00003d50 0000 0000 Each root bridge has two MMIO window. From this output we could see the pci address for 64-bit window is above 4G. I did a little change on your code, like this list_for_each_entry(window, &bridge->windows, list) { res = window->res; if (resource_type(res) == IORESOURCE_MEM || - res->end > 0xffffffff) { + (res->end - window->offset) > 0xffffffff) { bridge->has_mem64_res = true; break; } } This also works on my machine. One more question, with this patch, do we still need previous patch for Marek's problem? >>Yinghai > >-- >Richard Yang >Help you, Help me
On Wed, Dec 10, 2014 at 6:34 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote: > My mutt hang, in case the mail isn't sent out, I resend it. > > On Wed, Dec 10, 2014 at 10:15:37PM +0800, Wei Yang wrote: >>On Tue, Dec 09, 2014 at 03:13:49PM -0800, Yinghai Lu wrote: >>>On Tue, Dec 9, 2014 at 1:53 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>>> On Tue, Dec 9, 2014 at 2:34 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>>>> + list_for_each_entry(window, &bridge->windows, list) { >>>>> + res = window->res; >>>>> + if (resource_type(res) == IORESOURCE_MEM || >>>>> + res->end > 0xffffffff) { >>>>> + bridge->has_mem64_res = true; >>>> >>>> This is an interesting idea, but I think you're checking CPU addresses >>>> here, and you need to check PCI bus addresses. >>> >>>Looks like those IBM platforms have res > 4g, but pci bus address < 4g. >>>If we check pci bus address, and then we would break those platforms. >>> > > Hi, Yinghai > > I did some test with patch on my machine. It looks good. > > While as Bjorn mentioned, I think we could change it a little. The pci space > on our machine is like this: > > cpu address pci address > MEM 0x00003ff280000000..0x00003ff2fffeffff -> 0x00000000 8000 0000 > MEM64 0x00003d5000000000..0x00003d5fffffffff -> 0x00003d50 0000 0000 > > Each root bridge has two MMIO window. From this output we could see the pci > address for 64-bit window is above 4G. > > I did a little change on your code, like this > > list_for_each_entry(window, &bridge->windows, list) { > res = window->res; > if (resource_type(res) == IORESOURCE_MEM || > - res->end > 0xffffffff) { > + (res->end - window->offset) > 0xffffffff) { > bridge->has_mem64_res = true; > break; > } > } good, then we should change to compare region end. > > This also works on my machine. > > One more question, with this patch, do we still need previous patch for > Marek's problem? Only need this to make Marek's working. but does not help on Zermond's Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 09, 2014 at 01:34:31PM -0800, Yinghai Lu wrote: >So we could use bridge 64bit mem pref for children mem pref instead of >forcing them into bridge mem. > >Could help Marek's system as his system is using _CRS, and all mem res is under >4G. > >Link: https://bugzilla.kernel.org/show_bug.cgi?id=85491 >Reported-by: Marek Kordik <kordikmarek@gmail.com> >Fixes: 5b28541552ef ("PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources") >Signed-off-by: Yinghai Lu <yinghai@kernel.org> > >--- > drivers/pci/host-bridge.c | 7 +++++++ > drivers/pci/pci.h | 1 + > drivers/pci/probe.c | 9 +++++++++ > drivers/pci/setup-bus.c | 3 +++ > include/linux/pci.h | 1 + > 5 files changed, 21 insertions(+) > >Index: linux-2.6/drivers/pci/host-bridge.c >=================================================================== >--- linux-2.6.orig/drivers/pci/host-bridge.c >+++ linux-2.6/drivers/pci/host-bridge.c >@@ -31,6 +31,13 @@ void pci_set_host_bridge_release(struct > bridge->release_data = release_data; > } > >+bool pcibios_host_bridge_has_mem64_res(struct pci_bus *bus) >+{ >+ struct pci_host_bridge *bridge = find_pci_host_bridge(bus); >+ >+ return bridge->has_mem64_res; >+} >+ > void pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region, > struct resource *res) > { >Index: linux-2.6/drivers/pci/pci.h >=================================================================== >--- linux-2.6.orig/drivers/pci/pci.h >+++ linux-2.6/drivers/pci/pci.h >@@ -196,6 +196,7 @@ enum pci_bar_type { > pci_bar_mem64, /* A 64-bit memory BAR */ > }; > >+bool pcibios_host_bridge_has_mem64_res(struct pci_bus *bus); > bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl, > int crs_timeout); > int pci_setup_device(struct pci_dev *dev); >Index: linux-2.6/drivers/pci/probe.c >=================================================================== >--- linux-2.6.orig/drivers/pci/probe.c >+++ linux-2.6/drivers/pci/probe.c >@@ -1980,6 +1980,15 @@ struct pci_bus *pci_create_root_bus(stru > dev_info(&b->dev, "root bus resource %pR%s\n", res, bus_addr); > } > >+ list_for_each_entry(window, &bridge->windows, list) { >+ res = window->res; >+ if (resource_type(res) == IORESOURCE_MEM || >+ res->end > 0xffffffff) { Should we replace "||" with "&&" ? Thanks, Gavin >+ bridge->has_mem64_res = true; >+ break; >+ } >+ } >+ > down_write(&pci_bus_sem); > list_add_tail(&b->node, &pci_root_buses); > up_write(&pci_bus_sem); >Index: linux-2.6/include/linux/pci.h >=================================================================== >--- linux-2.6.orig/include/linux/pci.h >+++ linux-2.6/include/linux/pci.h >@@ -407,6 +407,7 @@ struct pci_host_bridge { > struct list_head windows; /* pci_host_bridge_windows */ > void (*release_fn)(struct pci_host_bridge *); > void *release_data; >+ bool has_mem64_res; > }; > > #define to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev) >Index: linux-2.6/drivers/pci/setup-bus.c >=================================================================== >--- linux-2.6.orig/drivers/pci/setup-bus.c >+++ linux-2.6/drivers/pci/setup-bus.c >@@ -693,6 +693,9 @@ static void pci_bridge_check_ranges(stru > } > } > >+ if (!pcibios_host_bridge_has_mem64_res(bus)) >+ b_res[2].flags &= ~IORESOURCE_MEM_64; >+ > /* double check if bridge does support 64 bit pref */ > if (b_res[2].flags & IORESOURCE_MEM_64) { > u32 mem_base_hi, tmp; > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 10, 2014 at 2:30 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: > On Tue, Dec 09, 2014 at 01:34:31PM -0800, Yinghai Lu wrote: >> >>+ list_for_each_entry(window, &bridge->windows, list) { >>+ res = window->res; >>+ if (resource_type(res) == IORESOURCE_MEM || >>+ res->end > 0xffffffff) { > > Should we replace "||" with "&&" ? oh, I missed that. Will send updated version. Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-2.6/drivers/pci/host-bridge.c =================================================================== --- linux-2.6.orig/drivers/pci/host-bridge.c +++ linux-2.6/drivers/pci/host-bridge.c @@ -31,6 +31,13 @@ void pci_set_host_bridge_release(struct bridge->release_data = release_data; } +bool pcibios_host_bridge_has_mem64_res(struct pci_bus *bus) +{ + struct pci_host_bridge *bridge = find_pci_host_bridge(bus); + + return bridge->has_mem64_res; +} + void pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region, struct resource *res) { Index: linux-2.6/drivers/pci/pci.h =================================================================== --- linux-2.6.orig/drivers/pci/pci.h +++ linux-2.6/drivers/pci/pci.h @@ -196,6 +196,7 @@ enum pci_bar_type { pci_bar_mem64, /* A 64-bit memory BAR */ }; +bool pcibios_host_bridge_has_mem64_res(struct pci_bus *bus); bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl, int crs_timeout); int pci_setup_device(struct pci_dev *dev); Index: linux-2.6/drivers/pci/probe.c =================================================================== --- linux-2.6.orig/drivers/pci/probe.c +++ linux-2.6/drivers/pci/probe.c @@ -1980,6 +1980,15 @@ struct pci_bus *pci_create_root_bus(stru dev_info(&b->dev, "root bus resource %pR%s\n", res, bus_addr); } + list_for_each_entry(window, &bridge->windows, list) { + res = window->res; + if (resource_type(res) == IORESOURCE_MEM || + res->end > 0xffffffff) { + bridge->has_mem64_res = true; + break; + } + } + down_write(&pci_bus_sem); list_add_tail(&b->node, &pci_root_buses); up_write(&pci_bus_sem); Index: linux-2.6/include/linux/pci.h =================================================================== --- linux-2.6.orig/include/linux/pci.h +++ linux-2.6/include/linux/pci.h @@ -407,6 +407,7 @@ struct pci_host_bridge { struct list_head windows; /* pci_host_bridge_windows */ void (*release_fn)(struct pci_host_bridge *); void *release_data; + bool has_mem64_res; }; #define to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev) Index: linux-2.6/drivers/pci/setup-bus.c =================================================================== --- linux-2.6.orig/drivers/pci/setup-bus.c +++ linux-2.6/drivers/pci/setup-bus.c @@ -693,6 +693,9 @@ static void pci_bridge_check_ranges(stru } } + if (!pcibios_host_bridge_has_mem64_res(bus)) + b_res[2].flags &= ~IORESOURCE_MEM_64; + /* double check if bridge does support 64 bit pref */ if (b_res[2].flags & IORESOURCE_MEM_64) { u32 mem_base_hi, tmp;
So we could use bridge 64bit mem pref for children mem pref instead of forcing them into bridge mem. Could help Marek's system as his system is using _CRS, and all mem res is under 4G. Link: https://bugzilla.kernel.org/show_bug.cgi?id=85491 Reported-by: Marek Kordik <kordikmarek@gmail.com> Fixes: 5b28541552ef ("PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources") Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/pci/host-bridge.c | 7 +++++++ drivers/pci/pci.h | 1 + drivers/pci/probe.c | 9 +++++++++ drivers/pci/setup-bus.c | 3 +++ include/linux/pci.h | 1 + 5 files changed, 21 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html