From: Bjorn Helgaas <helgaas@kernel.org> To: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> Cc: linux-pci@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux@yadro.com, Sam Bobroff <sbobroff@linux.ibm.com>, Rajat Jain <rajatja@google.com>, Lukas Wunner <lukas@wunner.de>, Oliver O'Halloran <oohall@gmail.com>, David Laight <David.Laight@ACULAB.COM> Subject: Re: [PATCH v5 03/23] PCI: hotplug: Add a flag for the movable BARs feature Date: Wed, 16 Oct 2019 12:29:21 -0500 Message-ID: <20191016172921.GA42365@google.com> (raw) In-Reply-To: <63c6c630-fbfb-175c-2a1a-c9a1f732498a@yadro.com> On Wed, Oct 16, 2019 at 06:50:30PM +0300, Sergey Miroshnichenko wrote: > On 10/16/19 1:14 AM, Bjorn Helgaas wrote: > > On Mon, Sep 30, 2019 at 03:59:25PM +0300, Sergey Miroshnichenko wrote: > > > On 9/28/19 1:02 AM, Bjorn Helgaas wrote: > > > > It's possible that a hot-add will trigger this attempt to move things > > > > around, and it's possible that we won't find space for the new device > > > > even if we move things around. But are we certain that every device > > > > that worked *before* the hot-add will still work *afterwards*? > > > > > > > > Much of the assignment was probably done by the BIOS using different > > > > algorithms than Linux has, so I think there's some chance that the > > > > BIOS did a better job and if we lose that BIOS assignment, we might > > > > not be able to recreate it. > > > > > > If a hardware has some special constraints on BAR assignment that the > > > kernel is not aware of yet, the movable BARs may break things after a > > > hotplug event. So the feature must be disabled there (manually) until > > > the kernel get support for that special needs. > > > > I'm not talking about special constraints on BAR assignment. (I'm not > > sure what those constraints would be -- AFAIK the constraints for a > > spec-compliant device are all discoverable via the BAR size and type > > (or the Enhanced Allocation capability)). > > > > What I'm concerned about is the case where we boot with a working > > assignment, we hot-add a device, we move things around to try to > > accommodate the new device, and not only do we fail to find resources > > for the new device, we also fail to find a working assignment for the > > devices that were present at boot. We've moved things around from > > what BIOS did, and since we use a different algorithm than the BIOS, > > there's no guarantee that we'll be able to find the assignment BIOS > > did. > > If BAR assignment fails with a hot-added device, these patches will > disable BARs for this device and retry, falling back to the situation > where number of BARs and their size are the same as they were before > the hotplug event. > > If all the BARs are immovable - they will just remain on their > positions. Nothing to break here I guess. > > If almost all the BARs are immovable and there is one movable BAR, > after releasing the bridge windows there will be a free gap - right > where this movable BAR was. These patches are keeping the size of > released BARs, not requesting the size from the devices again - so the > device can't ask for a larger BAR. The space reserving is disabled by > this patchset, so the kernel will request the same size for the bridge > window containing this movable BAR. So there always will be a gap for > this BAR - in the same location it was before. > > Based on these considerations I assume that the kernel is always able > to arrange BARs from scratch if a BIOS was able to make it before. > > But! There is an implicit speculation that there will be the same > amount of BARs after the fallback (which is equivalent to a PCI rescan > triggered on unchanged topology). And two week ago I've found that > this is not always true! > > I was testing on a "new" x86_64 PC, where BIOS doesn't reserve a space > for SR-IOV BARs (of a network adapter). On the boot, the kernel wasn't > arranging BARs itself - it took values written by the BIOS. And the > bridge window was "jammed" between immovable BARs, so it can't expand. > BARs of this device are also immovable, so the bridge window can't be > moved away. During the PCI rescan, the kernel tried to allocate both > "regular" and SR-IOV BARs - and failed. Even without changes in the > PCI topology. > > So in the next version of this series there will be one more patch, > that allows the kernel to ignore BIOS's setting for the "safe" (non-IO > and non-VGA) BARs, so these BARs will be arranged kernel-way - and > also those forgotten by the BIOS. This still seems a little scary, so I'll probably ask about it again :) > After modifying the code as you advised, it became possible to mark > only some BARs of the device as immovable. So the code is less ugly > now, and it also works for drivers/video/fbdev/efifb.c , which uses > the BAR in a weird way (dev->driver is NULL, but not the res->child): > > static bool pci_dev_movable(struct pci_dev *dev, > bool res_has_children) > { > if (!pci_can_move_bars) > return false; > > if (dev->driver && dev->driver->rescan_prepare) > return true; > > if (!dev->driver && !res_has_children) > return true; > > return false; > } > > bool pci_dev_bar_movable(struct pci_dev *dev, struct resource *res) > { > if (res->flags & IORESOURCE_PCI_FIXED) > return false; > > #ifdef CONFIG_X86 > /* Workaround for the legacy VGA memory 0xa0000-0xbffff */ > if (res->start == 0xa0000) Nit here; "res->start" is the CPU address, but what you need to check is the PCI bus address, e.g., something from pcibios_resource_to_bus(). And this is not x86-specific. 0xa0000 is magic on PCI no matter what the processor architecture. > return false; > #endif > > return pci_dev_movable(dev, res->child); > }
next prev parent reply index Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-16 16:50 [PATCH v5 00/23] PCI: Allow BAR movement during hotplug Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 01/23] PCI: Fix race condition in pci_enable/disable_device() Sergey Miroshnichenko 2019-08-22 12:37 ` Marta Rybczynska 2019-09-27 21:59 ` Bjorn Helgaas 2019-09-30 8:53 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 02/23] PCI: Enable bridge's I/O and MEM access for hotplugged devices Sergey Miroshnichenko 2019-09-27 22:01 ` Bjorn Helgaas 2019-08-16 16:50 ` [PATCH v5 03/23] PCI: hotplug: Add a flag for the movable BARs feature Sergey Miroshnichenko 2019-09-27 22:02 ` Bjorn Helgaas 2019-09-30 8:44 ` David Laight 2019-09-30 16:17 ` Sergey Miroshnichenko 2019-09-30 12:59 ` Sergey Miroshnichenko 2019-10-15 22:14 ` Bjorn Helgaas 2019-10-16 15:50 ` Sergey Miroshnichenko 2019-10-16 17:29 ` Bjorn Helgaas [this message] 2019-08-16 16:50 ` [PATCH v5 04/23] PCI: Define PCI-specific version of the release_child_resources() Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 05/23] PCI: hotplug: movable BARs: Fix reassigning the released bridge windows Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 06/23] PCI: hotplug: movable BARs: Recalculate all bridge windows during rescan Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 07/23] PCI: hotplug: movable BARs: Don't allow added devices to steal resources Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 08/23] PCI: Include fixed and immovable BARs into the bus size calculating Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 09/23] PCI: Prohibit assigning BARs and bridge windows to non-direct parents Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 10/23] PCI: hotplug: movable BARs: Try to assign unassigned resources only once Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 11/23] PCI: hotplug: movable BARs: Calculate immovable parts of bridge windows Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 12/23] PCI: hotplug: movable BARs: Compute limits for relocated " Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 13/23] PCI: Make sure bridge windows include their fixed BARs Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 14/23] PCI: Fix assigning the fixed prefetchable resources Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 15/23] PCI: hotplug: movable BARs: Assign fixed and immovable BARs before others Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 16/23] PCI: hotplug: movable BARs: Don't reserve IO/mem bus space Sergey Miroshnichenko 2019-09-04 5:42 ` Oliver O'Halloran 2019-09-04 11:22 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 17/23] powerpc/pci: Fix crash with enabled movable BARs Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 18/23] powerpc/pci: Handle BAR movement Sergey Miroshnichenko 2019-09-04 5:37 ` Oliver O'Halloran 2019-09-06 16:24 ` Sergey Miroshnichenko 2019-09-09 14:02 ` Oliver O'Halloran 2019-08-16 16:50 ` [PATCH v5 19/23] PCI: hotplug: Configure MPS for hot-added bridges during bus rescan Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 20/23] PCI: hotplug: movable BARs: Enable the feature by default Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 21/23] nvme-pci: Handle movable BARs Sergey Miroshnichenko 2019-08-16 16:51 ` [PATCH v5 22/23] PCI/portdrv: Declare support of " Sergey Miroshnichenko 2019-08-16 16:51 ` [PATCH v5 23/23] PCI: pciehp: movable BARs: Trigger a domain rescan on hp events Sergey Miroshnichenko
Reply instructions: You may reply publically to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191016172921.GA42365@google.com \ --to=helgaas@kernel.org \ --cc=David.Laight@ACULAB.COM \ --cc=linux-pci@vger.kernel.org \ --cc=linux@yadro.com \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=lukas@wunner.de \ --cc=oohall@gmail.com \ --cc=rajatja@google.com \ --cc=s.miroshnichenko@yadro.com \ --cc=sbobroff@linux.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Linux-PCI Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \ linux-pci@vger.kernel.org public-inbox-index linux-pci Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci AGPL code for this site: git clone https://public-inbox.org/public-inbox.git