| Message ID | 20181211021617.24072-1-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | pci: avoid bridge feature re-probing on hotplug | expand |
> -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: Tuesday, December 11, 2018 10:19 AM > To: linux-kernel@vger.kernel.org > Cc: xuyandong <xuyandong2@huawei.com>; stable@vger.kernel.org; Yinghai > Lu <yinghai@kernel.org>; Jesse Barnes <jbarnes@virtuousgeek.org>; Bjorn > Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org > Subject: [PATCH] pci: avoid bridge feature re-probing on hotplug > > commit 1f82de10d6 ("PCI/x86: don't assume prefetchable ranges are > 64bit") added probing of bridge support for 64 bit memory each time bridge is > re-enumerated. > > Unfortunately this probing is destructive if any device behind the bridge is in > use at this time. > > There's no real need to re-probe the bridge features as the regiters in question > never change - detect that using the memory flag being set and skip the > probing. > Avoiding repeated calls to pci_bridge_check_ranges might be even nicer would > be a bigger patch and probably not appropriate on stable. > > Reported-by: xuyandong <xuyandong2@huawei.com> > Cc: stable@vger.kernel.org > Cc: Yinghai Lu <yinghai@kernel.org> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Tested-by: xuyandong <xuyandong2@huawei.com> > --- > > This issue has been reported on upstream Linux and Centos. > > drivers/pci/setup-bus.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index > ed960436df5e..7ab42f76579e 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -741,6 +741,13 @@ static void pci_bridge_check_ranges(struct pci_bus > *bus) > struct resource *b_res; > > b_res = &bridge->resource[PCI_BRIDGE_RESOURCES]; > + > + /* Don't re-check after this was called once already: > + * important since bridge might be in use. > + */ > + if (b_res[1].flags & IORESOURCE_MEM) > + return; > + > b_res[1].flags |= IORESOURCE_MEM; > > pci_read_config_word(bridge, PCI_IO_BASE, &io); > -- > MST
On Tue, Dec 11, 2018 at 02:45:44AM +0000, xuyandong wrote: > > > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > Sent: Tuesday, December 11, 2018 10:19 AM > > To: linux-kernel@vger.kernel.org > > Cc: xuyandong <xuyandong2@huawei.com>; stable@vger.kernel.org; Yinghai > > Lu <yinghai@kernel.org>; Jesse Barnes <jbarnes@virtuousgeek.org>; Bjorn > > Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org > > Subject: [PATCH] pci: avoid bridge feature re-probing on hotplug > > > > commit 1f82de10d6 ("PCI/x86: don't assume prefetchable ranges are > > 64bit") added probing of bridge support for 64 bit memory each time bridge is > > re-enumerated. > > > > Unfortunately this probing is destructive if any device behind the bridge is in > > use at this time. > > > > There's no real need to re-probe the bridge features as the regiters in question > > never change - detect that using the memory flag being set and skip the > > probing. > > Avoiding repeated calls to pci_bridge_check_ranges might be even nicer would > > be a bigger patch and probably not appropriate on stable. > > > > Reported-by: xuyandong <xuyandong2@huawei.com> > > Cc: stable@vger.kernel.org > > Cc: Yinghai Lu <yinghai@kernel.org> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Tested-by: xuyandong <xuyandong2@huawei.com> Bjorn could you queue this for this release? > > --- > > > > This issue has been reported on upstream Linux and Centos. > > > > drivers/pci/setup-bus.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index > > ed960436df5e..7ab42f76579e 100644 > > --- a/drivers/pci/setup-bus.c > > +++ b/drivers/pci/setup-bus.c > > @@ -741,6 +741,13 @@ static void pci_bridge_check_ranges(struct pci_bus > > *bus) > > struct resource *b_res; > > > > b_res = &bridge->resource[PCI_BRIDGE_RESOURCES]; > > + > > + /* Don't re-check after this was called once already: > > + * important since bridge might be in use. > > + */ > > + if (b_res[1].flags & IORESOURCE_MEM) > > + return; > > + > > b_res[1].flags |= IORESOURCE_MEM; > > > > pci_read_config_word(bridge, PCI_IO_BASE, &io); > > -- > > MST
Hi Michael, Please run "git log --oneline drivers/pci/setup-bus.c" and follow the usual style. On Mon, Dec 10, 2018 at 09:18:40PM -0500, Michael S. Tsirkin wrote: > commit 1f82de10d6 ("PCI/x86: don't assume prefetchable ranges are > 64bit") added probing of bridge support for 64 bit memory > each time bridge is re-enumerated. Use conventional SHA1 reference (12-char SHA1). > Unfortunately this probing is destructive if any device behind > the bridge is in use at this time. Agreed, this sounds like a problem. > There's no real need to re-probe the bridge features as the > regiters in question never change - detect that using > the memory flag being set and skip the probing. s/regiters/registers/ > Avoiding repeated calls to pci_bridge_check_ranges might be even nicer > would be a bigger patch and probably not appropriate on stable. Maybe so. The ideal thing might be to have a trivial patch like this that can be marked for stable, immediately followed by the nicer patch. Trivial band-aids tend to accumulate and make things harder in the future. I'd have to take a much harder look at the problem to understand 1f82de10d6b1. The comment about "double check" seems misleading -- as you say, the hardware doesn't change and checking once should be enough. And if we're calling pci_bridge_check_ranges() more than necessary, that sounds like a problem, too. > Reported-by: xuyandong <xuyandong2@huawei.com> > Cc: stable@vger.kernel.org > Cc: Yinghai Lu <yinghai@kernel.org> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > This issue has been reported on upstream Linux and Centos. Are there URLs to these reports that we could include in the changelog? > drivers/pci/setup-bus.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index ed960436df5e..7ab42f76579e 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -741,6 +741,13 @@ static void pci_bridge_check_ranges(struct pci_bus *bus) > struct resource *b_res; > > b_res = &bridge->resource[PCI_BRIDGE_RESOURCES]; > + > + /* Don't re-check after this was called once already: > + * important since bridge might be in use. > + */ > + if (b_res[1].flags & IORESOURCE_MEM) > + return; Use conventional multi-line comment style. This test isn't 100%: devices below the bridge could be using only IO, or theoretically could be even using just config space. If it's safe to bail out if the bridge is in use, why isn't it safe to bail out *always*? > b_res[1].flags |= IORESOURCE_MEM; > > pci_read_config_word(bridge, PCI_IO_BASE, &io); > -- > MST
On Tue, Dec 11, 2018 at 08:18:08AM -0600, Bjorn Helgaas wrote: > Hi Michael, > > Please run "git log --oneline drivers/pci/setup-bus.c" and follow > the usual style. > > On Mon, Dec 10, 2018 at 09:18:40PM -0500, Michael S. Tsirkin wrote: > > commit 1f82de10d6 ("PCI/x86: don't assume prefetchable ranges are > > 64bit") added probing of bridge support for 64 bit memory > > each time bridge is re-enumerated. > > Use conventional SHA1 reference (12-char SHA1). > > > Unfortunately this probing is destructive if any device behind > > the bridge is in use at this time. > > Agreed, this sounds like a problem. > > > There's no real need to re-probe the bridge features as the > > regiters in question never change - detect that using > > the memory flag being set and skip the probing. > > s/regiters/registers/ Will address above. > > Avoiding repeated calls to pci_bridge_check_ranges might be even nicer > > would be a bigger patch and probably not appropriate on stable. > > Maybe so. The ideal thing might be to have a trivial patch like this > that can be marked for stable, immediately followed by the nicer > patch. Trivial band-aids tend to accumulate and make things harder in > the future. I understand, and I looked at it briefly, but it's not a simple change, with probing taking detours through acpi etc. I plan to look at it some more but should we release another linux with this bug? > I'd have to take a much harder look at the problem to understand > 1f82de10d6b1. The comment about "double check" seems misleading -- as > you say, the hardware doesn't change and checking once should be > enough. And if we're calling pci_bridge_check_ranges() more than > necessary, that sounds like a problem, too. So that will kind of make it a non issue. Should we still worry? > > Reported-by: xuyandong <xuyandong2@huawei.com> > > Cc: stable@vger.kernel.org > > Cc: Yinghai Lu <yinghai@kernel.org> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > This issue has been reported on upstream Linux and Centos. > > Are there URLs to these reports that we could include in the changelog? https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01711.html and specifically https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02082.html > > drivers/pci/setup-bus.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > index ed960436df5e..7ab42f76579e 100644 > > --- a/drivers/pci/setup-bus.c > > +++ b/drivers/pci/setup-bus.c > > @@ -741,6 +741,13 @@ static void pci_bridge_check_ranges(struct pci_bus *bus) > > struct resource *b_res; > > > > b_res = &bridge->resource[PCI_BRIDGE_RESOURCES]; > > + > > + /* Don't re-check after this was called once already: > > + * important since bridge might be in use. > > + */ > > + if (b_res[1].flags & IORESOURCE_MEM) > > + return; > > Use conventional multi-line comment style. > > This test isn't 100%: devices below the bridge could be using only IO, > or theoretically could be even using just config space. > > If it's safe to bail out if the bridge is in use, why isn't it safe to > bail out *always*? > > > b_res[1].flags |= IORESOURCE_MEM; > > > > pci_read_config_word(bridge, PCI_IO_BASE, &io); > > -- > > MST
On Tue, Dec 11, 2018 at 08:18:08AM -0600, Bjorn Helgaas wrote: > Hi Michael, > > Please run "git log --oneline drivers/pci/setup-bus.c" and follow > the usual style. > > On Mon, Dec 10, 2018 at 09:18:40PM -0500, Michael S. Tsirkin wrote: > > commit 1f82de10d6 ("PCI/x86: don't assume prefetchable ranges are > > 64bit") added probing of bridge support for 64 bit memory > > each time bridge is re-enumerated. > > Use conventional SHA1 reference (12-char SHA1). > > > Unfortunately this probing is destructive if any device behind > > the bridge is in use at this time. > > Agreed, this sounds like a problem. > > > There's no real need to re-probe the bridge features as the > > regiters in question never change - detect that using > > the memory flag being set and skip the probing. > > s/regiters/registers/ > > > Avoiding repeated calls to pci_bridge_check_ranges might be even nicer > > would be a bigger patch and probably not appropriate on stable. > > Maybe so. The ideal thing might be to have a trivial patch like this > that can be marked for stable, immediately followed by the nicer > patch. Trivial band-aids tend to accumulate and make things harder in > the future. > > I'd have to take a much harder look at the problem to understand > 1f82de10d6b1. The comment about "double check" seems misleading -- as > you say, the hardware doesn't change and checking once should be > enough. And if we're calling pci_bridge_check_ranges() more than > necessary, that sounds like a problem, too. > > > Reported-by: xuyandong <xuyandong2@huawei.com> > > Cc: stable@vger.kernel.org > > Cc: Yinghai Lu <yinghai@kernel.org> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > This issue has been reported on upstream Linux and Centos. > > Are there URLs to these reports that we could include in the changelog? > > > drivers/pci/setup-bus.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > index ed960436df5e..7ab42f76579e 100644 > > --- a/drivers/pci/setup-bus.c > > +++ b/drivers/pci/setup-bus.c > > @@ -741,6 +741,13 @@ static void pci_bridge_check_ranges(struct pci_bus *bus) > > struct resource *b_res; > > > > b_res = &bridge->resource[PCI_BRIDGE_RESOURCES]; > > + > > + /* Don't re-check after this was called once already: > > + * important since bridge might be in use. > > + */ > > + if (b_res[1].flags & IORESOURCE_MEM) > > + return; > > Use conventional multi-line comment style. Will fix. > This test isn't 100%: devices below the bridge could be using only IO, > or theoretically could be even using just config space. Right but 1. memory is not an optional capability for bridges. All pci bridges support memory bridging (unlike e.g. IO). 2. the line below accordingly sets IORESOURCE_MEM unconditionally even if devices below the bridge only use IO for now. So IIUC this really results in bailing out if bridge has already been configured. Did I miss something? > If it's safe to bail out if the bridge is in use, why isn't it safe to > bail out *always*? Well IIUC we need to probe support for optional capabilities such as IO and 64 bit bridging. this is what this function does, doesn't it? > > b_res[1].flags |= IORESOURCE_MEM; > > > > pci_read_config_word(bridge, PCI_IO_BASE, &io); > > -- > > MST
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index ed960436df5e..7ab42f76579e 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -741,6 +741,13 @@ static void pci_bridge_check_ranges(struct pci_bus *bus) struct resource *b_res; b_res = &bridge->resource[PCI_BRIDGE_RESOURCES]; + + /* Don't re-check after this was called once already: + * important since bridge might be in use. + */ + if (b_res[1].flags & IORESOURCE_MEM) + return; + b_res[1].flags |= IORESOURCE_MEM; pci_read_config_word(bridge, PCI_IO_BASE, &io);
commit 1f82de10d6 ("PCI/x86: don't assume prefetchable ranges are 64bit") added probing of bridge support for 64 bit memory each time bridge is re-enumerated. Unfortunately this probing is destructive if any device behind the bridge is in use at this time. There's no real need to re-probe the bridge features as the regiters in question never change - detect that using the memory flag being set and skip the probing. Avoiding repeated calls to pci_bridge_check_ranges might be even nicer would be a bigger patch and probably not appropriate on stable. Reported-by: xuyandong <xuyandong2@huawei.com> Cc: stable@vger.kernel.org Cc: Yinghai Lu <yinghai@kernel.org> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- This issue has been reported on upstream Linux and Centos. drivers/pci/setup-bus.c | 7 +++++++ 1 file changed, 7 insertions(+)