If the firmware or kernel has arranged memory for PCIe devices in a way
that doesn't provide enough space for BARs of a new hotplugged device, the
kernel can pause the drivers of the "obstructing" devices and move their
BARs, so the new BARs can fit into the freed spaces.
To rearrange the BARs and bridge windows these patches releases all of them
after a rescan and re-assigns in the same way as during the initial PCIe
topology scan at system boot.
When a driver is un-paused by the kernel after the PCIe rescan, it should
check if its BARs had moved, and ioremap() them.
Drivers indicate their support of the feature by implementing the new hooks
.rescan_prepare() and .rescan_done() in the struct pci_driver. If a driver
doesn't yet support the feature, BARs of its devices will be considered as
immovable (by checking the pci_dev_movable_bars_supported(dev)) and handled
in the same way as resources with the IORESOURCE_PCI_FIXED flag.
If a driver doesn't yet support the feature, its devices are guaranteed to
have their BARs remaining untouched.
Tested on:
- x86_64 with "pci=realloc,assign-busses,use_crs,pcie_bus_peer2peer";
- POWER8 PowerNV+OPAL+PHB3 ppc64le with [1] applied and the following:
"pci=realloc,pcie_bus_peer2peer";
- both platforms [with extra pacthes (yet to be submitted) for movable bus
numbers]: manually initiated (via sysfs) rescan has found and turned on
a hotplugged bridge.
Not so many platforms and test cases were covered, so all who are
interested are highly welcome to test on your setups - the more exotic the
better!
This patchset is a part of our work on adding support for hotplugging
bridges full of other bridges, NVME drives, SAS HBAs and GPUs without
special requirements such as Hot-Plug Controller, reservation of bus
numbers or memory regions by firmware, etc. The next patchset to submit
will implement the movable bus numbers.
[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2019-August/195272.html
[PATCH v6 0/5] powerpc/powernv/pci: Make hotplug self-sufficient, independent of FW and DT
Changes since v4:
- Feature is enabled by default (turned on by one of the latest patches);
- Add pci_dev_movable_bars_supported(dev) instead of marking the immovable
BARs with the IORESOURCE_PCI_FIXED flag;
- Set up PCIe bridges during rescan via sysfs, so MPS settings are now
configured not only during system boot or pcihp events;
- Allow movement of switch's BARs if claimed by portdrv;
- Update EEH address caches after rescan for powerpc;
- Don't disable completely hot-added devices which can't have BARs being
fit - just disable their BARs, so they are still visible in lspci etc;
- Clearer names: fixed_range_hard -> immovable_range, fixed_range_soft ->
realloc_range;
- Drop the patch for pci_restore_config_space() - fixed by properly using
the runtime PM.
Changes since v3:
- Rebased to the upstream, so the patches apply cleanly again.
Changes since v2:
- Fixed double-assignment of bridge windows;
- Fixed assignment of fixed prefetched resources;
- Fixed releasing of fixed resources;
- Fixed a debug message;
- Removed auto-enabling the movable BARs for x86 - let's rely on the
"pcie_movable_bars=force" option for now;
- Reordered the patches - bugfixes first.
Changes since v1:
- Add a "pcie_movable_bars={ off | force }" command line argument;
- Handle the IORESOURCE_PCI_FIXED flag properly;
- Don't move BARs of devices which don't support the feature;
- Guarantee that new hotplugged devices will not steal memory from working
devices by ignoring the failing new devices with the new PCI_DEV_IGNORE
flag;
- Add rescan_prepare()+rescan_done() to the struct pci_driver instead of
using the reset_prepare()+reset_done() from struct pci_error_handlers;
- Add a bugfix of a race condition;
- Fixed hotplug in a non-pre-enabled (by BIOS/firmware) bridge;
- Fix the compatibility of the feature with pm_runtime and D3-state;
- Hotplug events from pciehp also can move BARs;
- Add support of the feature to the NVME driver.
Sergey Miroshnichenko (23):
PCI: Fix race condition in pci_enable/disable_device()
PCI: Enable bridge's I/O and MEM access for hotplugged devices
PCI: hotplug: Add a flag for the movable BARs feature
PCI: Define PCI-specific version of the release_child_resources()
PCI: hotplug: movable BARs: Fix reassigning the released bridge
windows
PCI: hotplug: movable BARs: Recalculate all bridge windows during
rescan
PCI: hotplug: movable BARs: Don't allow added devices to steal
resources
PCI: Include fixed and immovable BARs into the bus size calculating
PCI: Prohibit assigning BARs and bridge windows to non-direct parents
PCI: hotplug: movable BARs: Try to assign unassigned resources only
once
PCI: hotplug: movable BARs: Calculate immovable parts of bridge
windows
PCI: hotplug: movable BARs: Compute limits for relocated bridge
windows
PCI: Make sure bridge windows include their fixed BARs
PCI: Fix assigning the fixed prefetchable resources
PCI: hotplug: movable BARs: Assign fixed and immovable BARs before
others
PCI: hotplug: movable BARs: Don't reserve IO/mem bus space
powerpc/pci: Fix crash with enabled movable BARs
powerpc/pci: Handle BAR movement
PCI: hotplug: Configure MPS for hot-added bridges during bus rescan
PCI: hotplug: movable BARs: Enable the feature by default
nvme-pci: Handle movable BARs
PCI/portdrv: Declare support of movable BARs
PCI: pciehp: movable BARs: Trigger a domain rescan on hp events
.../admin-guide/kernel-parameters.txt | 7 +
arch/powerpc/kernel/pci-hotplug.c | 10 +
arch/powerpc/platforms/powernv/pci-ioda.c | 3 +-
drivers/nvme/host/pci.c | 21 +-
drivers/pci/bus.c | 2 +-
drivers/pci/hotplug/pciehp_pci.c | 5 +
drivers/pci/pci.c | 58 +++-
drivers/pci/pci.h | 30 ++
drivers/pci/pcie/portdrv_pci.c | 11 +
drivers/pci/probe.c | 295 +++++++++++++++++-
drivers/pci/setup-bus.c | 276 +++++++++++++---
drivers/pci/setup-res.c | 48 ++-
include/linux/pci.h | 21 ++
13 files changed, 739 insertions(+), 48 deletions(-)
--
2.21.0
Hotplugged devices can affect the existing ones by moving their BARs. The
PCI subsystem will inform the NVME driver about this by invoking the
.rescan_prepare() and .rescan_done() hooks, so the BARs can by re-mapped.
Tested under the "randrw" mode of the fio tool. Before the hotplugging:
% sudo cat /proc/iomem
...
3fe800000000-3fe8007fffff : PCI Bus 0020:0b
3fe800000000-3fe8007fffff : PCI Bus 0020:18
3fe800000000-3fe8000fffff : 0020:18:00.0
3fe800000000-3fe8000fffff : nvme
3fe800100000-3fe80017ffff : 0020:18:00.0
...
, then another NVME drive was hot-added, so BARs of the 0020:18:00.0 are
moved:
% sudo cat /proc/iomem
...
3fe800000000-3fe800ffffff : PCI Bus 0020:0b
3fe800000000-3fe8007fffff : PCI Bus 0020:10
3fe800000000-3fe800003fff : 0020:10:00.0
3fe800000000-3fe800003fff : nvme
3fe800010000-3fe80001ffff : 0020:10:00.0
3fe800800000-3fe800ffffff : PCI Bus 0020:18
3fe800800000-3fe8008fffff : 0020:18:00.0
3fe800800000-3fe8008fffff : nvme
3fe800900000-3fe80097ffff : 0020:18:00.0
...
During the rescanning, both READ and WRITE speeds drop to zero for a while
due to driver's pause, then restore.
Cc: linux-nvme@lists.infradead.org
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/nvme/host/pci.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
The PCI_COMMAND_IO and PCI_COMMAND_MEMORY bits of the bridge must be
updated not only when enabling the bridge for the first time, but also if a
hotplugged device requests these types of resources.
Originally these bits were set by the pci_enable_device_flags() only, which
exits early if the bridge is already pci_is_enabled(). So if the bridge was
empty initially (an edge case), then hotplugged devices fail to IO/MEM.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/pci.c | 8 ++++++++
1 file changed, 8 insertions(+)
This is a yet another approach to fix an old [1-2] concurrency issue, when:
- two or more devices are being hot-added into a bridge which was
initially empty;
- a bridge with two or more devices is being hot-added;
- during boot, if BIOS/bootloader/firmware doesn't pre-enable bridges.
The problem is that a bridge is reported as enabled before the MEM/IO bits
are actually written to the PCI_COMMAND register, so another driver thread
starts memory requests through the not-yet-enabled bridge:
CPU0 CPU1
pci_enable_device_mem() pci_enable_device_mem()
pci_enable_bridge() pci_enable_bridge()
pci_is_enabled()
return false;
atomic_inc_return(enable_cnt)
Start actual enabling the bridge
... pci_is_enabled()
... return true;
... Start memory requests <-- FAIL
...
Set the PCI_COMMAND_MEMORY bit <-- Must wait for this
Protect the pci_enable/disable_device() and pci_enable_bridge(), which is
similar to the previous solution from commit 40f11adc7cd9 ("PCI: Avoid race
while enabling upstream bridges"), but adding a per-device mutexes and
preventing the dev->enable_cnt from from incrementing early.
CC: Srinath Mannam <redacted>
CC: Marta Rybczynska <redacted>
Signed-off-by: Sergey Miroshnichenko <redacted>
[1] https://lore.kernel.org/linux-pci/1501858648-22228-1-git-send-email-srinath.mannam@broadcom.com/T/#u
[RFC PATCH v3] pci: Concurrency issue during pci enable bridge
[2] https://lore.kernel.org/linux-pci/744877924.5841545.1521630049567.JavaMail.zimbra@kalray.eu/T/#u
[RFC PATCH] nvme: avoid race-conditions when enabling devices
---
drivers/pci/pci.c | 26 ++++++++++++++++++++++----
drivers/pci/probe.c | 1 +
include/linux/pci.h | 1 +
3 files changed, 24 insertions(+), 4 deletions(-)
@@ -1660,11 +1663,14 @@ static void pci_enable_bridge(struct pci_dev *dev)pci_err(dev,"Error enabling bridge (%d), continuing\n",retval);pci_set_master(dev);+mutex_unlock(&dev->enable_mutex);}staticintpci_enable_device_flags(structpci_dev*dev,unsignedlongflags){structpci_dev*bridge;+/* Enable-locking of bridges is performed within the pci_enable_bridge() */+boolneed_lock=!dev->subordinate;interr;inti,bars=0;
@@ -1680,8 +1686,13 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)dev->current_state=(pmcsr&PCI_PM_CTRL_STATE_MASK);}-if(atomic_inc_return(&dev->enable_cnt)>1)+if(need_lock)+mutex_lock(&dev->enable_mutex);+if(pci_is_enabled(dev)){+if(need_lock)+mutex_unlock(&dev->enable_mutex);return0;/* already enabled */+}bridge=pci_upstream_bridge(dev);if(bridge)
@@ -1696,8 +1707,10 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)bars|=(1<<i);err=do_pci_enable_device(dev,bars);-if(err<0)-atomic_dec(&dev->enable_cnt);+if(err>=0)+atomic_inc(&dev->enable_cnt);+if(need_lock)+mutex_unlock(&dev->enable_mutex);returnerr;}
@@ -425,6 +425,7 @@ struct pci_dev {unsignedintno_vf_scan:1;/* Don't scan for VFs after IOV enablement */pci_dev_flags_tdev_flags;atomic_tenable_cnt;/* pci_enable_device has been called */+structmutexenable_mutex;u32saved_config_space[16];/* Config space saved at suspend time */structhlist_headsaved_cap_space;
When hot-adding a device, the bridge may have windows not big enough (or
fragmented too much) for newly requested BARs to fit in. And expanding
these bridge windows may be impossible because blocked by "neighboring"
BARs and bridge windows.
Still, it may be possible to allocate a memory region for new BARs with the
following procedure:
1) notify all the drivers which support movable BARs to pause and release
the BARs; the rest of the drivers are guaranteed that their devices will
not get BARs moved;
2) release all the bridge windows except of root bridges;
3) try to recalculate new bridge windows that will fit all the BAR types:
- fixed;
- immovable;
- movable;
- newly requested by hot-added devices;
4) if the previous step fails, disable BARs for one of the hot-added
devices and retry from step 3;
5) notify the drivers, so they remap BARs and resume.
This makes the prior reservation of memory by BIOS/bootloader/firmware not
required anymore for the PCI hotplug.
Drivers indicate their support of movable BARs by implementing the new
.rescan_prepare() and .rescan_done() hooks in the struct pci_driver. All
device's activity must be paused during a rescan, and iounmap()+ioremap()
must be applied to every used BAR.
The platform also may need to prepare to BAR movement, so new hooks added:
pcibios_rescan_prepare(pci_dev) and pcibios_rescan_prepare(pci_dev).
This patch is a preparation for future patches with actual implementation,
and for now it just does the following:
- declares the feature;
- defines pci_movable_bars_enabled(), pci_dev_movable_bars_supported(dev);
- invokes the .rescan_prepare() and .rescan_done() driver notifiers;
- declares and invokes the pcibios_rescan_prepare()/_done() hooks;
- adds the PCI_IMMOVABLE_BARS flag.
The feature is disabled by default (via PCI_IMMOVABLE_BARS) until the final
patch of the series. It can be overridden per-arch using this flag or by
the following command line option:
pcie_movable_bars={ off | force }
CC: Sam Bobroff <redacted>
CC: Rajat Jain <redacted>
CC: Lukas Wunner <lukas@wunner.de>
CC: Oliver O'Halloran <oohall@gmail.com>
CC: David Laight <redacted>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
.../admin-guide/kernel-parameters.txt | 7 ++
drivers/pci/pci-driver.c | 2 +
drivers/pci/pci.c | 24 ++++++
drivers/pci/pci.h | 2 +
drivers/pci/probe.c | 86 ++++++++++++++++++-
include/linux/pci.h | 7 ++
6 files changed, 126 insertions(+), 2 deletions(-)
@@ -3526,6 +3526,13 @@ nomsi Do not use MSI for native PCIe PME signaling (this makes all PCIe root ports use INTx for all services).+ pcie_movable_bars=[PCIE]+ Override the movable BARs support detection:+ off+ Disable even if supported by the platform+ force+ Enable even if not explicitly declared as supported+ pcmv= [HW,PCMCIA] BadgePAD 4 pd_ignore_unused
@@ -139,6 +139,30 @@ static int __init pcie_port_pm_setup(char *str)}__setup("pcie_port_pm=",pcie_port_pm_setup);+staticboolpcie_movable_bars_off;+staticboolpcie_movable_bars_force;+staticint__initpcie_movable_bars_setup(char*str)+{+if(!strcmp(str,"off"))+pcie_movable_bars_off=true;+elseif(!strcmp(str,"force"))+pcie_movable_bars_force=true;+return1;+}+__setup("pcie_movable_bars=",pcie_movable_bars_setup);++boolpci_movable_bars_enabled(void)+{+if(pcie_movable_bars_off)+returnfalse;++if(pcie_movable_bars_force)+returntrue;++return!pci_has_flag(PCI_IMMOVABLE_BARS);+}+EXPORT_SYMBOL(pci_movable_bars_enabled);+/* Time to wait after a reset for device to become responsive */#define PCIE_RESET_READY_POLL_MS 60000
@@ -838,6 +838,8 @@ struct pci_driver {int(*resume)(structpci_dev*dev);/* Device woken up */void(*shutdown)(structpci_dev*dev);int(*sriov_configure)(structpci_dev*dev,intnum_vfs);/* On PF */+void(*rescan_prepare)(structpci_dev*dev);+void(*rescan_done)(structpci_dev*dev);conststructpci_error_handlers*err_handler;conststructattribute_group**groups;structdevice_driverdriver;
@@ -924,6 +926,7 @@ enum {PCI_ENABLE_PROC_DOMAINS=0x00000010,/* Enable domains in /proc */PCI_COMPAT_DOMAIN_0=0x00000020,/* ... except domain 0 */PCI_SCAN_ALL_PCIE_DEVS=0x00000040,/* Scan all, not just dev 0 */+PCI_IMMOVABLE_BARS=0x00000080,/* Disable runtime BAR reassign */};/* These external functions are only available when PCI support is enabled */
@@ -1266,6 +1269,9 @@ unsigned int pci_rescan_bus(struct pci_bus *bus);voidpci_lock_rescan_remove(void);voidpci_unlock_rescan_remove(void);+voidpcibios_rescan_prepare(structpci_dev*dev);+voidpcibios_rescan_done(structpci_dev*dev);+/* Vital Product Data routines */ssize_tpci_read_vpd(structpci_dev*dev,loff_tpos,size_tcount,void*buf);ssize_tpci_write_vpd(structpci_dev*dev,loff_tpos,size_tcount,constvoid*buf);
If release the bridge resources with standard release_child_resources(), it
drops the .start field of children's BARs to zero, but with the STARTALIGN
flag remaining set, which makes the resource invalid for reassignment.
Some resources must preserve their offset and size: those marked with the
PCI_FIXED and the immovable ones - which are bound by drivers without
support of the movable BARs feature.
Add the pci_release_child_resources() to replace release_child_resources()
in handling the described PCI-specific cases.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/setup-bus.c | 54 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 53 insertions(+), 1 deletion(-)
@@ -1482,6 +1482,55 @@ static void __pci_bridge_assign_resources(const struct pci_dev *bridge,(IORESOURCE_IO|IORESOURCE_MEM|IORESOURCE_PREFETCH|\IORESOURCE_MEM_64)+/*+*Similartogenericrelease_child_resources(),butawareofimmovableBARsand+*PCI_FIXEDandSTARTALIGNflags+*/+staticvoidpci_release_child_resources(structpci_bus*bus,structresource*r)+{+structpci_dev*dev;++if(!bus||!r)+return;++if(r->flags&IORESOURCE_PCI_FIXED)+return;++r->child=NULL;++list_for_each_entry(dev,&bus->devices,bus_list){+inti;++for(i=0;i<PCI_NUM_RESOURCES;i++){+structresource*tmp=&dev->resource[i];+resource_size_tsize=resource_size(tmp);++if(!tmp->flags||tmp->parent!=r)+continue;++tmp->parent=NULL;+tmp->sibling=NULL;++pci_release_child_resources(dev->subordinate,tmp);++if((tmp->flags&IORESOURCE_PCI_FIXED)||+!pci_dev_movable_bars_supported(dev)){+pci_dbg(dev,"release immovable %pR (%s), keep its flags, base and size\n",+tmp,tmp->name);+continue;+}++pci_dbg(dev,"release %pR (%s)\n",tmp,tmp->name);++tmp->start=0;+tmp->end=size-1;++tmp->flags&=~IORESOURCE_STARTALIGN;+tmp->flags|=IORESOURCE_SIZEALIGN;+}+}+}+staticvoidpci_bridge_release_resources(structpci_bus*bus,unsignedlongtype){
@@ -1522,7 +1571,10 @@ static void pci_bridge_release_resources(struct pci_bus *bus,return;/* If there are children, release them all */-release_child_resources(r);+if(pci_movable_bars_enabled())+pci_release_child_resources(bus,r);+else+release_child_resources(r);if(!release_resource(r)){type=old_flags=r->flags&PCI_RES_TYPE_MASK;pci_info(dev,"resource %d %pR released\n",
When a bridge window is temporarily released during the rescan, its old
size is not relevant anymore - it will be recreated from pbus_size_*(), so
it's start value should be zero.
If such window can't be reassigned, don't apply reset_resource(), so the
next retry may succeed.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/setup-bus.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
@@ -295,7 +295,8 @@ static void assign_requested_resources_sorted(struct list_head *head,0/* don't care */,0/* don't care */);}-reset_resource(res);+if(!pci_movable_bars_enabled())+reset_resource(res);}}}
@@ -1579,8 +1580,8 @@ static void pci_bridge_release_resources(struct pci_bus *bus,type=old_flags=r->flags&PCI_RES_TYPE_MASK;pci_info(dev,"resource %d %pR released\n",PCI_BRIDGE_RESOURCES+idx,r);-/* Keep the old size */-r->end=resource_size(r)-1;+/* Don't keep the old size if the bridge will be recalculated */+r->end=pci_movable_bars_enabled()?0:(resource_size(r)-1);r->start=0;r->flags=0;
When the movable BARs feature is enabled and a rescan has been requested,
release all the bridge windows and recalculate them from scratch, taking
into account all kinds for BARs: fixed, immovable, movable, new.
This increases the chances to find a memory space to fit BARs for newly
hotplugged devices, especially if no/not enough gaps were reserved by the
BIOS/bootloader/firmware.
The last step of writing the recalculated windows to the bridges is done
by the new pci_setup_bridges() function.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/pci.h | 1 +
drivers/pci/probe.c | 22 ++++++++++++++++++++++
drivers/pci/setup-bus.c | 16 ++++++++++++++++
3 files changed, 39 insertions(+)
The only difference between the fixed/immovable and movable BARs is a size
and offset preservation after they are released (the corresponding struct
resource* detached from a bridge window for a while during a bus rescan).
Include fixed/immovable BARs into result of pbus_size_mem() and prohibit
assigning them to non-direct parents.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/setup-bus.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
@@ -1011,12 +1011,21 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,structresource*r=&dev->resource[i];resource_size_tr_size;-if(r->parent||(r->flags&IORESOURCE_PCI_FIXED)||+if(r->parent||((r->flags&mask)!=type&&(r->flags&mask)!=type2&&(r->flags&mask)!=type3))continue;r_size=resource_size(r);++if((r->flags&IORESOURCE_PCI_FIXED)||+!pci_dev_movable_bars_supported(dev)){+if(pci_movable_bars_enabled())+size+=r_size;++continue;+}+#ifdef CONFIG_PCI_IOV/* Put SRIOV requested res to the optional list */if(realloc_head&&i>=PCI_IOV_RESOURCES&&
When movable BARs are enabled, the PCI subsystem at first releases all the
bridge windows and then attempts to assign resources both to previously
working devices and to the newly hotplugged ones, with the same priority.
If a hotplugged device gets its BARs first, this may lead to lack of space
for already working devices, which is unacceptable. If that happens, mark
one of the new devices with the newly introduced flag PCI_DEV_DISABLED_BARS
(if it is not yet marked) and retry the BAR recalculation.
The worst case would be no BARs for hotplugged devices, while all the rest
just continue working.
The algorithm is simple and it doesn't retry different subsets of hot-added
devices in case of a failure, e.g. if there are no space to allocate BARs
for both hotplugged devices A and B, but is enough for just A, the A will
be marked with PCI_DEV_DISABLED_BARS first, then (after the next failure) -
B. As a result, A will not get BARs while it could. This issue is only
relevant when hotplugging two and more devices simultaneously.
Add a new res_mask bitmask to the struct pci_dev for storing the indices of
assigned BARs.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/pci.h | 11 +++++
drivers/pci/probe.c | 101 ++++++++++++++++++++++++++++++++++++++--
drivers/pci/setup-bus.c | 15 ++++++
include/linux/pci.h | 1 +
4 files changed, 125 insertions(+), 3 deletions(-)
@@ -3493,6 +3512,83 @@ static void pci_setup_bridges(struct pci_bus *bus)pci_setup_bridge(bus);}+staticstructpci_dev*pci_find_next_new_device(structpci_bus*bus)+{+structpci_dev*dev;++if(!bus)+returnNULL;++list_for_each_entry(dev,&bus->devices,bus_list){+structpci_bus*child_bus=dev->subordinate;++if(!pci_dev_is_added(dev)&&pci_dev_bars_enabled(dev))+returndev;++if(child_bus){+structpci_dev*next_new_dev;++next_new_dev=pci_find_next_new_device(child_bus);+if(next_new_dev)+returnnext_new_dev;+}+}++returnNULL;+}++staticboolpci_bus_check_all_bars_reassigned(structpci_bus*bus)+{+structpci_dev*dev;+boolret=true;++if(!bus)+returnfalse;++list_for_each_entry(dev,&bus->devices,bus_list){+structpci_bus*child=dev->subordinate;+unsignedintres_mask=pci_dev_count_res_mask(dev);++if(!pci_dev_bars_enabled(dev))+continue;++if(dev->res_mask&~res_mask){+pci_err(dev,"Non-re-enabled resources found: 0x%x -> 0x%x\n",+dev->res_mask,res_mask);+ret=false;+}++if(child&&!pci_bus_check_all_bars_reassigned(child))+ret=false;+}++returnret;+}++staticvoidpci_reassign_root_bus_resources(structpci_bus*root)+{+do{+structpci_dev*next_new_dev;++pci_bus_release_root_bridge_resources(root);+pci_assign_unassigned_root_bus_resources(root);++if(pci_bus_check_all_bars_reassigned(root))+break;++next_new_dev=pci_find_next_new_device(root);+if(!next_new_dev){+dev_err(&root->dev,"failed to re-assign resources even after ignoring all the hotplugged devices\n");+break;+}++dev_warn(&root->dev,"failed to re-assign resources, disable the next hotplugged device %s and retry\n",+dev_name(&next_new_dev->dev));++pci_dev_disable_bars(next_new_dev);+}while(true);+}+/***pci_rescan_bus-ScanaPCIbusfordevices*@bus:PCIbustoscan
@@ -3515,8 +3611,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)max=pci_scan_child_bus(root);-pci_bus_release_root_bridge_resources(root);-pci_assign_unassigned_root_bus_resources(root);+pci_reassign_root_bus_resources(root);pci_setup_bridges(root);pci_bus_rescan_done(root);
With enabled BAR movement, BARs and bridge windows can only be assigned to
their direct parents, so there can be only one variant of resource tree,
thus every retry within the pci_assign_unassigned_root_bus_resources() will
result in the same tree, and it is enough to try just once.
In case of failures the pci_reassign_root_bus_resources() disables BARs for
one of the hotplugged devices and tries the assignment again.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/setup-bus.c | 7 +++++++
1 file changed, 7 insertions(+)
@@ -1819,6 +1819,13 @@ void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus)intpci_try_num=1;enumenable_typeenable_local;+if(pci_movable_bars_enabled()){+__pci_bus_size_bridges(bus,NULL);+__pci_bus_assign_resources(bus,NULL,NULL);++gotodump;+}+/* Don't realloc if asked to do so */enable_local=pci_realloc_detect(bus,pci_realloc_enable);if(pci_realloc_enabled(enable_local)){
When movable BARs are enabled, the feature of resource relocating from
commit 2bbc6942273b5 ("PCI : ability to relocate assigned pci-resources")
is not used. Instead, inability to assign a resource is used as a signal
to retry BAR assignment with other configuration of bridge windows.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/setup-bus.c | 2 ++
drivers/pci/setup-res.c | 12 ++++++++++++
2 files changed, 14 insertions(+)
When movable BARs are enabled, and if a bridge contains a device with fixed
(IORESOURCE_PCI_FIXED) or immovable BARs, the corresponing windows can't be
moved too far away from their original positions - they must still contain
all the fixed/immovable BARs, like that:
1) Window position before a bus rescan:
| <-- root bridge window --> |
| |
| | <-- bridge window --> | |
| | movable BARs | **fixed BAR** | |
2) Possible valid outcome after rescan and move:
| <-- root bridge window --> |
| |
| | <-- bridge window --> | |
| | **fixed BAR** | Movable BARs | |
An immovable area of a bridge (separare for IO, MEM and MEM64 window types)
is a range that covers all the fixed and immovable BARs of direct children,
and all the fixed area of children bridges:
| <-- root bridge window --> |
| |
| | <-- bridge window level 1 --> | |
| | ******** immovable area of this bridge window ******** | |
| | | |
| | **fixed BAR** | <-- bridge window level 2 --> | BARs | |
| | | ***** fixed area of this bridge ***** | | |
| | | | | |
| | | ***fixed BAR*** | | ***fixed BAR*** | | |
To store these areas, the .immovable_range field has been added to struct
pci_bus. It is filled recursively from leaves to the root before a rescan.
Also make pbus_size_io() and pbus_size_mem() return their usual result OR
the size of an immovable range of according type, depending on which one is
larger.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/pci.h | 14 +++++++
drivers/pci/probe.c | 88 +++++++++++++++++++++++++++++++++++++++++
drivers/pci/setup-bus.c | 17 ++++++++
include/linux/pci.h | 6 +++
4 files changed, 125 insertions(+)
@@ -3512,6 +3518,87 @@ static void pci_setup_bridges(struct pci_bus *bus)pci_setup_bridge(bus);}+staticvoidpci_bus_update_immovable_range(structpci_bus*bus)+{+structpci_dev*dev;+intidx;+resource_size_tstart,end;++for(idx=0;idx<PCI_BRIDGE_RESOURCE_NUM;++idx){+bus->immovable_range[idx].start=0;+bus->immovable_range[idx].end=0;+}++list_for_each_entry(dev,&bus->devices,bus_list)+if(dev->subordinate)+pci_bus_update_immovable_range(dev->subordinate);++list_for_each_entry(dev,&bus->devices,bus_list){+inti;+booldev_is_movable=pci_dev_movable_bars_supported(dev);+structpci_bus*child=dev->subordinate;++for(i=0;i<PCI_BRIDGE_RESOURCES;++i){+structresource*r=&dev->resource[i];++if(!r->flags||(r->flags&IORESOURCE_UNSET)||!r->parent)+continue;++if(!dev_is_movable||(r->flags&IORESOURCE_PCI_FIXED)){+idx=pci_get_bridge_resource_idx(r);+start=bus->immovable_range[idx].start;+end=bus->immovable_range[idx].end;++if(!start||start>r->start)+start=r->start;+if(end<r->end)+end=r->end;++if(bus->immovable_range[idx].start!=start||+bus->immovable_range[idx].end!=end){+dev_dbg(&bus->dev,"Found fixed 0x%llx-0x%llx in %s, expand the fixed bridge window %d to 0x%llx-0x%llx\n",+(unsignedlonglong)r->start,+(unsignedlonglong)r->end,+dev_name(&dev->dev),idx,+(unsignedlonglong)start,+(unsignedlonglong)end);+bus->immovable_range[idx].start=start;+bus->immovable_range[idx].end=end;+}+}+}++if(child){+for(idx=0;idx<PCI_BRIDGE_RESOURCE_NUM;++idx){+structresource*child_immovable_range=+&child->immovable_range[idx];++if(child_immovable_range->start>=+child_immovable_range->end)+continue;++start=bus->immovable_range[idx].start;+end=bus->immovable_range[idx].end;++if(!start||start>child_immovable_range->start)+start=child_immovable_range->start;+if(end<child_immovable_range->end)+end=child_immovable_range->end;++if(start<bus->immovable_range[idx].start||+end>bus->immovable_range[idx].end){+dev_dbg(&bus->dev,"Expand the fixed bridge window %d from %s to 0x%llx-0x%llx\n",+idx,dev_name(&child->dev),+(unsignedlonglong)start,+(unsignedlonglong)end);+bus->immovable_range[idx].start=start;+bus->immovable_range[idx].end=end;+}+}+}+}+}+staticstructpci_dev*pci_find_next_new_device(structpci_bus*bus){structpci_dev*dev;
@@ -3610,6 +3697,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)pci_bus_rescan_prepare(root);max=pci_scan_child_bus(root);+pci_bus_update_immovable_range(root);pci_reassign_root_bus_resources(root);
@@ -581,6 +581,12 @@ struct pci_bus {structlist_headresources;/* Address space routed to this bus */structresourcebusn_res;/* Bus numbers routed to this bus */+/*+*Iftherearefixedorimmovableresourcesinthebridgewindow,thisrange+*containstheloweststartaddressandhighestendaddressofthem.+*/+structresourceimmovable_range[PCI_BRIDGE_RESOURCE_NUM];+structpci_ops*ops;/* Configuration access functions */structmsi_controller*msi;/* MSI controller */void*sysdata;/* Hook for sys-specific extension */
With enabled movable BARs, bridge windows are recalculated during each pci
rescan. Some of the BARs below the bridge may be fixed/immovable: these
areas are represented by the .immovable_range field in struct pci_bus.
If a bridge window size is equal to its immovable range, it can only be
assigned to the start of this range. But if a bridge window size is larger,
and this difference in size is denoted as "delta", the window can start
from (immovable_range.start - delta) to (immovable_range.start), and it can
end from (immovable_range.end) to (immovable_range.end + delta). This range
(the new .realloc_range field in struct pci_bus) must then be compared with
immovable ranges of neighbouring bridges to guarantee no intersections.
This patch only calculates valid ranges for reallocated bridges during pci
rescan, and the next one will make use of these values during allocation.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/setup-bus.c | 67 +++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 6 ++++
2 files changed, 73 insertions(+)
@@ -1819,6 +1819,72 @@ static enum enable_type pci_realloc_detect(struct pci_bus *bus,}#endif+/*+*Calculatetheaddressmarginswherethebridgewindowsmaybeallocatedtofitall+*thefixedandimmovableBARsbeneath.+*/+staticvoidpci_bus_update_realloc_range(structpci_bus*bus)+{+structpci_dev*dev;+structpci_bus*parent=bus->parent;+intidx;++list_for_each_entry(dev,&bus->devices,bus_list)+if(dev->subordinate)+pci_bus_update_realloc_range(dev->subordinate);++if(!parent||!bus->self)+return;++for(idx=0;idx<PCI_BRIDGE_RESOURCE_NUM;++idx){+structresource*immovable_range=&bus->immovable_range[idx];+resource_size_twindow_size=resource_size(bus->resource[idx]);+resource_size_trealloc_start,realloc_end;++bus->realloc_range[idx].start=0;+bus->realloc_range[idx].end=0;++/* Check if there any immovable BARs under the bridge */+if(immovable_range->start>=immovable_range->end)+continue;++/* The lowest possible address where the bridge window can start */+realloc_start=immovable_range->end-window_size+1;+/* The highest possible address where the bridge window can end */+realloc_end=immovable_range->start+window_size-1;++if(realloc_start>immovable_range->start)+realloc_start=immovable_range->start;++if(realloc_end<immovable_range->end)+realloc_end=immovable_range->end;++/*+*Checkthatreallocrangedoesn'tintersectwithhardfixedranges+*ofneighboringbridges+*/+list_for_each_entry(dev,&parent->devices,bus_list){+structpci_bus*neighbor=dev->subordinate;+structresource*n_imm_range;++if(!neighbor||neighbor==bus)+continue;++n_imm_range=&neighbor->immovable_range[idx];++if(n_imm_range->start>=n_imm_range->end)+continue;++if(n_imm_range->end<immovable_range->start&&+n_imm_range->end>realloc_start)+realloc_start=n_imm_range->end;+}++bus->realloc_range[idx].start=realloc_start;+bus->realloc_range[idx].end=realloc_end;+}+}+/**FirsttrywillnottouchPCIbridgeres.*Secondandlatertrywillclearsmallleafbridgeres.
When the time comes to select a start address for the bridge window during
the root bus rescan, it should be not just a lowest possible address: this
window must cover all the underlying fixed and immovable BARs. The lowest
address that satisfies this requirement is the .realloc_range field of
struct pci_bus, which is calculated during the preparation to the rescan.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/bus.c | 2 +-
drivers/pci/setup-res.c | 28 ++++++++++++++++++++++++++--
2 files changed, 27 insertions(+), 3 deletions(-)
@@ -287,6 +298,19 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,ret=pci_bus_alloc_resource(bus,res,size,align,min,0,pcibios_align_resource,dev);+check_fixed:+if(ret==0&&start<end){+if(res->start>start||res->end<end){+dev_err(&bus->dev,"fixed area 0x%llx-0x%llx for %s doesn't fit in the allocated %pR (0x%llx-0x%llx)",+(unsignedlonglong)start,(unsignedlonglong)end,+dev_name(&dev->dev),+res,(unsignedlonglong)res->start,+(unsignedlonglong)res->end);+release_resource(res);+return-1;+}+}+returnret;}
A hotplugged bridge with many hotplug-capable ports may request
reserving more IO space than the machine has. This could be overridden
with the "hpiosize=" kernel argument though.
But when BARs are movable, there are no need to reserve space anymore:
new BARs are allocated not from reserved gaps, but via rearranging the
existing BARs. Requesting a precise amount of space for bridge windows
increases the chances of adding the new bridge successfully.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/setup-bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Add a check for the UNSET resource flag to skip the released BARs
CC: Alexey Kardashevskiy <redacted>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
arch/powerpc/platforms/powernv/pci-ioda.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Reassign resources during rescan in two steps: first the fixed/immovable
BARs and bridge windows that have fixed areas, so the movable ones will not
steal these reserved areas; then the rest - so the movable BARs will divide
the rest of the space.
With this change, pci_assign_resource() is now able to assign all types of
BARs, so the pdev_assign_fixed_resources() became unused and thus removed.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/pci.h | 2 ++
drivers/pci/setup-bus.c | 79 ++++++++++++++++++++++++-----------------
drivers/pci/setup-res.c | 8 +++--
3 files changed, 55 insertions(+), 34 deletions(-)
@@ -348,8 +348,12 @@ int pci_assign_resource(struct pci_dev *dev, int resno)resource_size_talign,size;intret;-if(res->flags&IORESOURCE_PCI_FIXED)-return0;+if((res->flags&IORESOURCE_PCI_FIXED)||+(resno<PCI_BRIDGE_RESOURCES&&+!pci_dev_movable_bars_supported(dev)&&+res->start)){+returnassign_fixed_resource_on_bus(dev->bus,res);+}res->flags|=IORESOURCE_UNSET;align=pci_resource_alignment(dev,res);
Add pcibios_rescan_prepare()/_done() hooks for the powerpc platform. Now if
the device's driver supports movable BARs, pcibios_rescan_prepare() will be
called after the device is stopped, and pcibios_rescan_done() - before it
resumes. There are no memory requests to this device between the hooks, so
it it safe to rebuild the EEH address cache during that.
CC: Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
arch/powerpc/kernel/pci-hotplug.c | 10 ++++++++++
1 file changed, 10 insertions(+)
Assure that MPS settings are set up for bridges which are discovered
during manually triggered rescan via sysfs. This sequence of bridge
init (using pci_rescan_bus()) will be used for pciehp hot-add events
when BARs are movable.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/probe.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
This is the last patch in the series which implements the essentials of the
Movable BARs feature, so it is turned by default now. Tested on:
- x86_64 with "pci=realloc,assign-busses,use_crs,pcie_bus_peer2peer"
command line argument;
- POWER8 PowerNV+PHB3 ppc64le with "pci=realloc,pcie_bus_peer2peer".
In case of problems it is still can be overridden by the following command
line option:
pcie_movable_bars=off
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/pci-driver.c | 2 --
1 file changed, 2 deletions(-)
Switch's BARs are not used by the portdrv driver, but they are still
considered as immovable until the .rescan_prepare() and .rescan_done()
hooks are added. Add these hooks to increase chances to allocate new BARs.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/pcie/portdrv_pci.c | 11 +++++++++++
1 file changed, 11 insertions(+)
With movable BARs, adding a hotplugged device is not local to its bridge
anymore, but it affects the whole domain: BARs, bridge windows and bus
numbers can be substantially rearranged. So instead of trying to fit the
new devices into preallocated reserved gaps, initiate a full domain rescan.
The pci_rescan_bus() covers all the operations of the replaced functions:
- assigning new bus numbers, as the pci_hp_add_bridge() does it;
- allocating BARs (pci_assign_unassigned_bridge_resources());
- cofiguring MPS settings (pcie_bus_configure_settings());
- binding devices to their drivers (pci_bus_add_devices()).
CC: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/hotplug/pciehp_pci.c | 5 +++++
1 file changed, 5 insertions(+)
From: Marta Rybczynska <hidden> Date: 2019-08-22 17:07:35
----- On 16 Aug, 2019, at 18:50, Sergey Miroshnichenko s.miroshnichenko@yadro.com wrote:
quoted hunk
This is a yet another approach to fix an old [1-2] concurrency issue, when:
- two or more devices are being hot-added into a bridge which was
initially empty;
- a bridge with two or more devices is being hot-added;
- during boot, if BIOS/bootloader/firmware doesn't pre-enable bridges.
The problem is that a bridge is reported as enabled before the MEM/IO bits
are actually written to the PCI_COMMAND register, so another driver thread
starts memory requests through the not-yet-enabled bridge:
CPU0 CPU1
pci_enable_device_mem() pci_enable_device_mem()
pci_enable_bridge() pci_enable_bridge()
pci_is_enabled()
return false;
atomic_inc_return(enable_cnt)
Start actual enabling the bridge
... pci_is_enabled()
... return true;
... Start memory requests <-- FAIL
...
Set the PCI_COMMAND_MEMORY bit <-- Must wait for this
Protect the pci_enable/disable_device() and pci_enable_bridge(), which is
similar to the previous solution from commit 40f11adc7cd9 ("PCI: Avoid race
while enabling upstream bridges"), but adding a per-device mutexes and
preventing the dev->enable_cnt from from incrementing early.
CC: Srinath Mannam <redacted>
CC: Marta Rybczynska <redacted>
Signed-off-by: Sergey Miroshnichenko <redacted>
[1]
https://lore.kernel.org/linux-pci/1501858648-22228-1-git-send-email-srinath.mannam@broadcom.com/T/#u
[RFC PATCH v3] pci: Concurrency issue during pci enable bridge
[2]
https://lore.kernel.org/linux-pci/744877924.5841545.1521630049567.JavaMail.zimbra@kalray.eu/T/#u
[RFC PATCH] nvme: avoid race-conditions when enabling devices
---
drivers/pci/pci.c | 26 ++++++++++++++++++++++----
drivers/pci/probe.c | 1 +
include/linux/pci.h | 1 +
3 files changed, 24 insertions(+), 4 deletions(-)
if (pci_is_enabled(dev)) {
if (!dev->is_busmaster)
pci_set_master(dev);
+ mutex_unlock(&dev->enable_mutex);
return;
}
This code is used by numerous drivers and when we've seen that issue I was wondering
if there are some use-cases when this (or pci_disable_device) is called with interrupts
disabled. It seems that it shouldn't be, but a BUG_ON or error when someone calls
it this way would be helpful when debugging.
Marta
From: Oliver O'Halloran <oohall@gmail.com> Date: 2019-09-04 05:39:17
On Fri, 2019-08-16 at 19:50 +0300, Sergey Miroshnichenko wrote:
quoted hunk
Add pcibios_rescan_prepare()/_done() hooks for the powerpc platform. Now if
the device's driver supports movable BARs, pcibios_rescan_prepare() will be
called after the device is stopped, and pcibios_rescan_done() - before it
resumes. There are no memory requests to this device between the hooks, so
it it safe to rebuild the EEH address cache during that.
CC: Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
arch/powerpc/kernel/pci-hotplug.c | 10 ++++++++++
1 file changed, 10 insertions(+)
Is this actually sufficent? The PE number for a device is largely
determined by the location of the MMIO BARs. If you move a BAR far
enough the PE number stored in the eeh_pe would need to be updated as
well.
From: Oliver O'Halloran <oohall@gmail.com> Date: 2019-09-04 05:44:49
On Fri, 2019-08-16 at 19:50 +0300, Sergey Miroshnichenko wrote:
A hotplugged bridge with many hotplug-capable ports may request
reserving more IO space than the machine has. This could be overridden
with the "hpiosize=" kernel argument though.
But when BARs are movable, there are no need to reserve space anymore:
new BARs are allocated not from reserved gaps, but via rearranging the
existing BARs. Requesting a precise amount of space for bridge windows
increases the chances of adding the new bridge successfully.
It wouldn't hurt to reserve some memory space to prevent unnecessary
BAR shuffling at runtime. If it turns out that we need more space then
we can always fall back to re-assigning the whole tree.
On Fri, 2019-08-16 at 19:50 +0300, Sergey Miroshnichenko wrote:
quoted
A hotplugged bridge with many hotplug-capable ports may request
reserving more IO space than the machine has. This could be overridden
with the "hpiosize=" kernel argument though.
But when BARs are movable, there are no need to reserve space anymore:
new BARs are allocated not from reserved gaps, but via rearranging the
existing BARs. Requesting a precise amount of space for bridge windows
increases the chances of adding the new bridge successfully.
It wouldn't hurt to reserve some memory space to prevent unnecessary
BAR shuffling at runtime. If it turns out that we need more space then
we can always fall back to re-assigning the whole tree.
Hi Oliver,
Thank you for your comments!
We had an issue on a x86_64 PC with a small amount of IO space: after
hotplugging an empty bridge of 32 ports even a DEFAULT_HOTPLUG_IO_SIZE
(which is 256) was enough to exhaust the space. So another patch of
this series ("Don't allow added devices to steal resources") had
disabled the BAR allocating for this bridge. It took some time for me
to guess that "hpiosize=0" can solve that.
For MEM and MEM64 spaces it will be harder to reproduce the same, but
there can be a similar problem when fitting between two immovable BARs.
To implement a fallback it would need to add some flag indicating that
allocating this bridge with reserved spaces has failed, so its windows
should be recalculated without reserved spaces - and try again. Maybe
even two types of retrials: with and without the full re-assignment.
We've tried to avoid adding execution paths and code complicatedness.
Serge
Hi Oliver,
On 9/4/19 8:37 AM, Oliver O'Halloran wrote:
On Fri, 2019-08-16 at 19:50 +0300, Sergey Miroshnichenko wrote:
quoted
Add pcibios_rescan_prepare()/_done() hooks for the powerpc platform. Now if
the device's driver supports movable BARs, pcibios_rescan_prepare() will be
called after the device is stopped, and pcibios_rescan_done() - before it
resumes. There are no memory requests to this device between the hooks, so
it it safe to rebuild the EEH address cache during that.
CC: Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
arch/powerpc/kernel/pci-hotplug.c | 10 ++++++++++
1 file changed, 10 insertions(+)
Is this actually sufficent? The PE number for a device is largely
determined by the location of the MMIO BARs. If you move a BAR far
enough the PE number stored in the eeh_pe would need to be updated as
well.
Thanks for the hint! I've checked on our PowerNV: for bridges with MEM
only it allocates PE numbers starting from 0xff down, and when there
are MEM64 - starting from 0 up, one PE number per 4GiB.
PEs are allocated during call to pnv_pci_setup_bridge(), and the I've
added invocation of pci_setup_bridge() after a hotplug event in the
"Recalculate all bridge windows during rescan" patch of this series.
Currently, if a bus already has a PE, pnv_ioda_setup_bus_PE() takes it
and returns. I can see two ways to change it, both are not difficult to
implement:
a.1) check if MEM64 BARs appeared below the bus - allocate and assign
a new master PE with required number of slave PEs;
a.2) if the bus now has more MEM64 than before - check if more slave
PEs must be reserved;
b) release all the PEs before a PCI rescan and allocate+assign them
again after - with this approach the "Hook up the writes to
PCI_SECONDARY_BUS register" patch may be eliminated.
Do you find any of these suitable?
Serge
On Sat, Sep 7, 2019 at 2:25 AM Sergey Miroshnichenko
[off-list ref] wrote:
Hi Oliver,
On 9/4/19 8:37 AM, Oliver O'Halloran wrote:
quoted
On Fri, 2019-08-16 at 19:50 +0300, Sergey Miroshnichenko wrote:
quoted
Add pcibios_rescan_prepare()/_done() hooks for the powerpc platform. Now if
the device's driver supports movable BARs, pcibios_rescan_prepare() will be
called after the device is stopped, and pcibios_rescan_done() - before it
resumes. There are no memory requests to this device between the hooks, so
it it safe to rebuild the EEH address cache during that.
CC: Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
arch/powerpc/kernel/pci-hotplug.c | 10 ++++++++++
1 file changed, 10 insertions(+)
Is this actually sufficent? The PE number for a device is largely
determined by the location of the MMIO BARs. If you move a BAR far
enough the PE number stored in the eeh_pe would need to be updated as
well.
Thanks for the hint! I've checked on our PowerNV: for bridges with MEM
only it allocates PE numbers starting from 0xff down, and when there
are MEM64 - starting from 0 up, one PE number per 4GiB.
PEs are allocated during call to pnv_pci_setup_bridge(), and the I've
added invocation of pci_setup_bridge() after a hotplug event in the
"Recalculate all bridge windows during rescan" patch of this series.
Sort of.
On PHB3 both the 32bit and the 64bit MMIO windows are split into 256
segments each of which is mapped to a PE number. For the 32bit space
there's a remapping table in hardware that allows arbitrary mapping of
segments to PE numbers, but in the 64bit space the mapping is fixed
with the first segment being PE0, etc. If there's a 64 bit BAR under a
bridge the PE is really "allocated" during the BAR assignment process,
and the setup_bridge() step sets up the EEH state based on that.
It's worth pointing out that this is why the 64bit window is usually
4GB. Bridge windows need to be aligned to a segment boundary to ensure
the devices under them are placed into a unique PE.
Currently, if a bus already has a PE, pnv_ioda_setup_bus_PE() takes it
and returns. I can see two ways to change it, both are not difficult to
implement:
a.1) check if MEM64 BARs appeared below the bus - allocate and assign
a new master PE with required number of slave PEs;
a.2) if the bus now has more MEM64 than before - check if more slave
PEs must be reserved;
b) release all the PEs before a PCI rescan and allocate+assign them
again after - with this approach the "Hook up the writes to
PCI_SECONDARY_BUS register" patch may be eliminated.
Do you find any of these suitable?
I'm not sure a) would work, but even if it does b) is preferable.
There's a lot of strangeness in the powerpc PCI code as-is without
adding extra code paths to deal with. Keeping what happens at hotplug
consistent with what happens at boot will help keep things sane.
FYI in the next few days I'm going to post a series that rips out the
use of pci_dn in powernv and the generic parts of EEH (pseries still
uses it). Assuming Bjorn isn't picking this up for 5.4 you might want
to wait for that before getting too deep into this.
Oliver
On Fri, Aug 16, 2019 at 07:50:39PM +0300, Sergey Miroshnichenko wrote:
This is a yet another approach to fix an old [1-2] concurrency issue, when:
- two or more devices are being hot-added into a bridge which was
initially empty;
- a bridge with two or more devices is being hot-added;
- during boot, if BIOS/bootloader/firmware doesn't pre-enable bridges.
The problem is that a bridge is reported as enabled before the MEM/IO bits
are actually written to the PCI_COMMAND register, so another driver thread
starts memory requests through the not-yet-enabled bridge:
CPU0 CPU1
pci_enable_device_mem() pci_enable_device_mem()
pci_enable_bridge() pci_enable_bridge()
pci_is_enabled()
return false;
atomic_inc_return(enable_cnt)
Start actual enabling the bridge
... pci_is_enabled()
... return true;
... Start memory requests <-- FAIL
...
Set the PCI_COMMAND_MEMORY bit <-- Must wait for this
Protect the pci_enable/disable_device() and pci_enable_bridge(), which is
similar to the previous solution from commit 40f11adc7cd9 ("PCI: Avoid race
while enabling upstream bridges"), but adding a per-device mutexes and
preventing the dev->enable_cnt from from incrementing early.
This isn't directly related to the movable BARs functionality; is it
here because you see the problem more frequently when moving BARs?
On Fri, Aug 16, 2019 at 07:50:40PM +0300, Sergey Miroshnichenko wrote:
The PCI_COMMAND_IO and PCI_COMMAND_MEMORY bits of the bridge must be
updated not only when enabling the bridge for the first time, but also if a
hotplugged device requests these types of resources.
Yeah, this assumption that pci_is_enabled() means PCI_COMMAND_IO and
PCI_COMMAND_MEMORY are set correctly even though we may now need
*different* settings than when we incremented pdev->enable_cnt is
quite broken.
quoted hunk
Originally these bits were set by the pci_enable_device_flags() only, which
exits early if the bridge is already pci_is_enabled(). So if the bridge was
empty initially (an edge case), then hotplugged devices fail to IO/MEM.
Signed-off-by: Sergey Miroshnichenko <redacted>
---
drivers/pci/pci.c | 8 ++++++++
1 file changed, 8 insertions(+)
On Fri, Aug 16, 2019 at 07:50:41PM +0300, Sergey Miroshnichenko wrote:
When hot-adding a device, the bridge may have windows not big enough (or
fragmented too much) for newly requested BARs to fit in. And expanding
these bridge windows may be impossible because blocked by "neighboring"
BARs and bridge windows.
Still, it may be possible to allocate a memory region for new BARs with the
following procedure:
1) notify all the drivers which support movable BARs to pause and release
the BARs; the rest of the drivers are guaranteed that their devices will
not get BARs moved;
2) release all the bridge windows except of root bridges;
3) try to recalculate new bridge windows that will fit all the BAR types:
- fixed;
- immovable;
- movable;
- newly requested by hot-added devices;
4) if the previous step fails, disable BARs for one of the hot-added
devices and retry from step 3;
5) notify the drivers, so they remap BARs and resume.
You don't do the actual recalculation in *this* patch, but since you
mention the procedure here, are we confident that we never make things
worse?
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.
quoted hunk
This makes the prior reservation of memory by BIOS/bootloader/firmware not
required anymore for the PCI hotplug.
Drivers indicate their support of movable BARs by implementing the new
.rescan_prepare() and .rescan_done() hooks in the struct pci_driver. All
device's activity must be paused during a rescan, and iounmap()+ioremap()
must be applied to every used BAR.
The platform also may need to prepare to BAR movement, so new hooks added:
pcibios_rescan_prepare(pci_dev) and pcibios_rescan_prepare(pci_dev).
This patch is a preparation for future patches with actual implementation,
and for now it just does the following:
- declares the feature;
- defines pci_movable_bars_enabled(), pci_dev_movable_bars_supported(dev);
- invokes the .rescan_prepare() and .rescan_done() driver notifiers;
- declares and invokes the pcibios_rescan_prepare()/_done() hooks;
- adds the PCI_IMMOVABLE_BARS flag.
The feature is disabled by default (via PCI_IMMOVABLE_BARS) until the final
patch of the series. It can be overridden per-arch using this flag or by
the following command line option:
pcie_movable_bars={ off | force }
CC: Sam Bobroff <redacted>
CC: Rajat Jain <redacted>
CC: Lukas Wunner <lukas@wunner.de>
CC: Oliver O'Halloran <oohall@gmail.com>
CC: David Laight <redacted>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
.../admin-guide/kernel-parameters.txt | 7 ++
drivers/pci/pci-driver.c | 2 +
drivers/pci/pci.c | 24 ++++++
drivers/pci/pci.h | 2 +
drivers/pci/probe.c | 86 ++++++++++++++++++-
include/linux/pci.h | 7 ++
6 files changed, 126 insertions(+), 2 deletions(-)
@@ -3526,6 +3526,13 @@ nomsi Do not use MSI for native PCIe PME signaling (this makes all PCIe root ports use INTx for all services).+ pcie_movable_bars=[PCIE]
This isn't a PCIe-specific feature, it's just a function of whether
drivers are smart enough, so we shouldn't tie it specifically to PCIe.
We could eventually do this for conventional PCI as well.
+ Override the movable BARs support detection:
+ off
+ Disable even if supported by the platform
+ force
+ Enable even if not explicitly declared as supported
What's the need for "force"? If it's possible, I think we should
enable this functionality all the time and just have a disable switch
in case we trip over cases where it doesn't work, e.g., something
like:
pci=no_movable_bars
@@ -3406,6 +3406,74 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)returnmax;}+boolpci_dev_movable_bars_supported(structpci_dev*dev)
This name suggests that movable BARs is a property of the device, but
it's mostly a property of the *driver*. Most uses are in conjuction
with checking IORESOURCE_PCI_FIXED for some resource, so I think this
might read more naturally and simplify the callers slightly:
bool pci_dev_movable(struct pci_dev *dev)
{
if (dev->driver && dev->driver->rescan_prepare)
return true;
if ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
return false;
if (!dev->driver)
return true;
return false;
}
bool pci_dev_bar_movable(struct pci_dev *dev, struct resource *res)
{
if (res->flags & IORESOURCE_PCI_FIXED)
return false;
return pci_dev_movable(dev);
}
I'm not sure why the PCI_CLASS_DISPLAY_VGA special case is there; can
you add a comment about why that's needed? Obviously we can't move
the 0xa0000 legacy frame buffer because I think devices are allowed to
claim that region even if no BAR describes it. But I would think
*other* BARs of VGA devices could be movable.
Can you add the pcibios_rescan_prepare() and pcibios_rescan_done()
stubs at the point where they're needed, i.e., where you add the
powerpc implementations? We can't see the need for them at this point
in the series.
@@ -838,6 +838,8 @@ struct pci_driver {int(*resume)(structpci_dev*dev);/* Device woken up */void(*shutdown)(structpci_dev*dev);int(*sriov_configure)(structpci_dev*dev,intnum_vfs);/* On PF */+void(*rescan_prepare)(structpci_dev*dev);+void(*rescan_done)(structpci_dev*dev);conststructpci_error_handlers*err_handler;conststructattribute_group**groups;structdevice_driverdriver;
@@ -924,6 +926,7 @@ enum {PCI_ENABLE_PROC_DOMAINS=0x00000010,/* Enable domains in /proc */PCI_COMPAT_DOMAIN_0=0x00000020,/* ... except domain 0 */PCI_SCAN_ALL_PCIE_DEVS=0x00000040,/* Scan all, not just dev 0 */+PCI_IMMOVABLE_BARS=0x00000080,/* Disable runtime BAR reassign */
There are no uses of PCI_IMMOVABLE_BARS left at the end of the series,
so I'd rather not add it if we can avoid it.
quoted hunk
};
/* These external functions are only available when PCI support is enabled */
I would really like it if this were simply
extern bool pci_no_movable_bars;
in drivers/pci/pci.h. It would default to false since it's
uninitialized, and "pci=no_movable_bars" would set it to true.
We have similar "=off" and "=force" parameters for ASPM and other
things, and it makes the code really hard to analyze.
From: David Laight <hidden> Date: 2019-09-30 08:46:44
From: Bjorn Helgaas
Sent: 27 September 2019 23:02
On Fri, Aug 16, 2019 at 07:50:41PM +0300, Sergey Miroshnichenko wrote:
quoted
When hot-adding a device, the bridge may have windows not big enough (or
fragmented too much) for newly requested BARs to fit in. And expanding
these bridge windows may be impossible because blocked by "neighboring"
BARs and bridge windows.
Still, it may be possible to allocate a memory region for new BARs with the
following procedure:
1) notify all the drivers which support movable BARs to pause and release
the BARs; the rest of the drivers are guaranteed that their devices will
not get BARs moved;
2) release all the bridge windows except of root bridges;
3) try to recalculate new bridge windows that will fit all the BAR types:
- fixed;
- immovable;
- movable;
- newly requested by hot-added devices;
4) if the previous step fails, disable BARs for one of the hot-added
devices and retry from step 3;
5) notify the drivers, so they remap BARs and resume.
You don't do the actual recalculation in *this* patch, but since you
mention the procedure here, are we confident that we never make things
worse?
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.
Yep, removing everything and starting again is probably OTT and most of the churn won't help.
I think you need to work out what can be moved in order to make the required resources available
to each bus and then make the required changes.
In the simplest case you are trying to add resource below a bridge so need to 'shuffle'
everything allocated after that bridge to later addresses (etc).
Many devices that support address reassignment might not need to be moved - so there is
no point remmapping them.
There is also the case when a device that is present but not currently is use could be taken
through a remove+insert sequence in order to change its resources.
Much easier to implement than 'remap while active'.
This would require a call into the driver (than can sleep) to request whether it is idle.
(and probably one at the end if the remove wasn't done).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hello Bjorn,
On 9/28/19 12:59 AM, Bjorn Helgaas wrote:
On Fri, Aug 16, 2019 at 07:50:39PM +0300, Sergey Miroshnichenko wrote:
quoted
This is a yet another approach to fix an old [1-2] concurrency issue, when:
- two or more devices are being hot-added into a bridge which was
initially empty;
- a bridge with two or more devices is being hot-added;
- during boot, if BIOS/bootloader/firmware doesn't pre-enable bridges.
The problem is that a bridge is reported as enabled before the MEM/IO bits
are actually written to the PCI_COMMAND register, so another driver thread
starts memory requests through the not-yet-enabled bridge:
CPU0 CPU1
pci_enable_device_mem() pci_enable_device_mem()
pci_enable_bridge() pci_enable_bridge()
pci_is_enabled()
return false;
atomic_inc_return(enable_cnt)
Start actual enabling the bridge
... pci_is_enabled()
... return true;
... Start memory requests <-- FAIL
...
Set the PCI_COMMAND_MEMORY bit <-- Must wait for this
Protect the pci_enable/disable_device() and pci_enable_bridge(), which is
similar to the previous solution from commit 40f11adc7cd9 ("PCI: Avoid race
while enabling upstream bridges"), but adding a per-device mutexes and
preventing the dev->enable_cnt from from incrementing early.
This isn't directly related to the movable BARs functionality; is it
here because you see the problem more frequently when moving BARs?
First two patches of this series (including this one) are fixes for
the boot and for the hotplug, not related to movable BARs.
Before these fixes, we were suffering from this issue on PowerNV until
commit db2173198b9513f7add8009f225afa1f1c79bcc6 "powerpc/powernv/pci:
Work around races in PCI bridge enabling" was backported to distros:
NVMEs randomly failed to start during system boot. So we've tested the
fixes with that commit reverted.
On x86 the BIOS does pre-enable the bridges, but they were still prone
to races when hot-added or was initially "empty".
Serge
Hello Bjorn,
On 9/28/19 1:02 AM, Bjorn Helgaas wrote:
On Fri, Aug 16, 2019 at 07:50:41PM +0300, Sergey Miroshnichenko wrote:
quoted
When hot-adding a device, the bridge may have windows not big enough (or
fragmented too much) for newly requested BARs to fit in. And expanding
these bridge windows may be impossible because blocked by "neighboring"
BARs and bridge windows.
Still, it may be possible to allocate a memory region for new BARs with the
following procedure:
1) notify all the drivers which support movable BARs to pause and release
the BARs; the rest of the drivers are guaranteed that their devices will
not get BARs moved;
2) release all the bridge windows except of root bridges;
3) try to recalculate new bridge windows that will fit all the BAR types:
- fixed;
- immovable;
- movable;
- newly requested by hot-added devices;
4) if the previous step fails, disable BARs for one of the hot-added
devices and retry from step 3;
5) notify the drivers, so they remap BARs and resume.
You don't do the actual recalculation in *this* patch, but since you
mention the procedure here, are we confident that we never make things
worse?
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.
On x86 we had no choice - most of the machines we used just can't boot
with even an "empty" 16-port switch connected. So we hot-add it after
the boot, then trigger a rescan via 'echo 1 > /sys/bus/pci/rescan'.
And reserved bridge windows wasn't enough, and they can't expand
because are blocked by the next device.
quoted
This makes the prior reservation of memory by BIOS/bootloader/firmware not
required anymore for the PCI hotplug.
Drivers indicate their support of movable BARs by implementing the new
.rescan_prepare() and .rescan_done() hooks in the struct pci_driver. All
device's activity must be paused during a rescan, and iounmap()+ioremap()
must be applied to every used BAR.
The platform also may need to prepare to BAR movement, so new hooks added:
pcibios_rescan_prepare(pci_dev) and pcibios_rescan_prepare(pci_dev).
This patch is a preparation for future patches with actual implementation,
and for now it just does the following:
- declares the feature;
- defines pci_movable_bars_enabled(), pci_dev_movable_bars_supported(dev);
- invokes the .rescan_prepare() and .rescan_done() driver notifiers;
- declares and invokes the pcibios_rescan_prepare()/_done() hooks;
- adds the PCI_IMMOVABLE_BARS flag.
The feature is disabled by default (via PCI_IMMOVABLE_BARS) until the final
patch of the series. It can be overridden per-arch using this flag or by
the following command line option:
pcie_movable_bars={ off | force }
CC: Sam Bobroff <redacted>
CC: Rajat Jain <redacted>
CC: Lukas Wunner <lukas@wunner.de>
CC: Oliver O'Halloran <oohall@gmail.com>
CC: David Laight <redacted>
Signed-off-by: Sergey Miroshnichenko <redacted>
---
.../admin-guide/kernel-parameters.txt | 7 ++
drivers/pci/pci-driver.c | 2 +
drivers/pci/pci.c | 24 ++++++
drivers/pci/pci.h | 2 +
drivers/pci/probe.c | 86 ++++++++++++++++++-
include/linux/pci.h | 7 ++
6 files changed, 126 insertions(+), 2 deletions(-)
@@ -3526,6 +3526,13 @@ nomsi Do not use MSI for native PCIe PME signaling (this makes all PCIe root ports use INTx for all services).+ pcie_movable_bars=[PCIE]
This isn't a PCIe-specific feature, it's just a function of whether
drivers are smart enough, so we shouldn't tie it specifically to PCIe.
We could eventually do this for conventional PCI as well.
quoted
+ Override the movable BARs support detection:
+ off
+ Disable even if supported by the platform
+ force
+ Enable even if not explicitly declared as supported
What's the need for "force"? If it's possible, I think we should
enable this functionality all the time and just have a disable switch
in case we trip over cases where it doesn't work, e.g., something
like:
pci=no_movable_bars
Thanks, I'll simplify that, replace the pci_movable_bars_enabled()
with a bool variable, and remove the flag as you advice.
@@ -3406,6 +3406,74 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)returnmax;}+boolpci_dev_movable_bars_supported(structpci_dev*dev)
This name suggests that movable BARs is a property of the device, but
it's mostly a property of the *driver*. Most uses are in conjuction
with checking IORESOURCE_PCI_FIXED for some resource, so I think this
might read more naturally and simplify the callers slightly:
bool pci_dev_movable(struct pci_dev *dev)
{
if (dev->driver && dev->driver->rescan_prepare)
return true;
if ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
return false;
if (!dev->driver)
return true;
return false;
}
bool pci_dev_bar_movable(struct pci_dev *dev, struct resource *res)
{
if (res->flags & IORESOURCE_PCI_FIXED)
return false;
return pci_dev_movable(dev);
}
Nice, I'll do that, thanks.
Theoretically, there may be a number of identical devices in the
system, some of them are bound to an "immovable" driver, and the rest
is not, which makes them movable. Also modprobe+rmmod may affect
device's BARs mobility. But I agree, the "_supported" suffix was
confusing.
I'm not sure why the PCI_CLASS_DISPLAY_VGA special case is there; can
you add a comment about why that's needed? Obviously we can't move
the 0xa0000 legacy frame buffer because I think devices are allowed to
claim that region even if no BAR describes it. But I would think
*other* BARs of VGA devices could be movable.
Sure, I'll add a comment to the code.
The issue that we are avoiding by that is the "nomodeset" command line
argument, which prevents a video driver from being bound, so the BARs
are seems to be used, but can't be moved, otherwise machines just hang
after hotplug events. That was the only special ugly case we've
spotted during testing. I'll check if it will be enough just to work
around the 0xa0000.
Can you add the pcibios_rescan_prepare() and pcibios_rescan_done()
stubs at the point where they're needed, i.e., where you add the
powerpc implementations? We can't see the need for them at this point
in the series.
The connection between these two conditions is pretty complicated to
figure out. Can you explain why you need both?
Yeah, this is what was meant, if would be written clearer:
if (pci_dev_movable_bars_supported(dev)) {
if (dev->driver &&
dev->driver->rescan_prepare)
dev->driver->rescan_prepare(dev);
pcibios_rescan_prepare(dev);
}
But anyway the pcibios_rescan_prepare(dev) was for PowerNV, and after
consulting with Oliver it was decided to make that a single call for
the whole domain instead of per-device.
quoted
+ pcibios_rescan_prepare(dev);
+ }
+ }
+}
+
+static void pci_bus_rescan_done(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ struct pci_bus *child = dev->subordinate;
+
+ if (dev->driver &&
+ dev->driver->rescan_done) {
+ pcibios_rescan_done(dev);
+ dev->driver->rescan_done(dev);
+ } else if (pci_dev_movable_bars_supported(dev)) {
+ pcibios_rescan_done(dev);
+ }
+
+ if (child)
+ pci_bus_rescan_done(child);
+ }
+
+ if (bus->self)
+ pci_config_pm_runtime_put(bus->self);
+}
+
/**
* pci_rescan_bus - Scan a PCI bus for devices
* @bus: PCI bus to scan
@@ -3418,9 +3486,23 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge) unsigned int pci_rescan_bus(struct pci_bus *bus) { unsigned int max;+ struct pci_bus *root = bus;++ while (!pci_is_root_bus(root))+ root = root->parent;++ if (pci_movable_bars_enabled()) {+ pci_bus_rescan_prepare(root);++ max = pci_scan_child_bus(root);+ pci_assign_unassigned_root_bus_resources(root);++ pci_bus_rescan_done(root);+ } else {+ max = pci_scan_child_bus(bus);+ pci_assign_unassigned_bus_resources(bus);+ }- max = pci_scan_child_bus(bus);- pci_assign_unassigned_bus_resources(bus); pci_bus_add_devices(bus); return max;
@@ -838,6 +838,8 @@ struct pci_driver {int(*resume)(structpci_dev*dev);/* Device woken up */void(*shutdown)(structpci_dev*dev);int(*sriov_configure)(structpci_dev*dev,intnum_vfs);/* On PF */+void(*rescan_prepare)(structpci_dev*dev);+void(*rescan_done)(structpci_dev*dev);conststructpci_error_handlers*err_handler;conststructattribute_group**groups;structdevice_driverdriver;
@@ -924,6 +926,7 @@ enum {PCI_ENABLE_PROC_DOMAINS=0x00000010,/* Enable domains in /proc */PCI_COMPAT_DOMAIN_0=0x00000020,/* ... except domain 0 */PCI_SCAN_ALL_PCIE_DEVS=0x00000040,/* Scan all, not just dev 0 */+PCI_IMMOVABLE_BARS=0x00000080,/* Disable runtime BAR reassign */
There are no uses of PCI_IMMOVABLE_BARS left at the end of the series,
so I'd rather not add it if we can avoid it.
quoted
};
/* These external functions are only available when PCI support is enabled */
I would really like it if this were simply
extern bool pci_no_movable_bars;
in drivers/pci/pci.h. It would default to false since it's
uninitialized, and "pci=no_movable_bars" would set it to true.
I have a premonition of platforms that will not support the feature.
Wouldn't be better to put this variable-flag to include/linux/pci.h ,
so code in arch/* can set it, so they could work by default, without
the command line argument?
Serge
We have similar "=off" and "=force" parameters for ASPM and other
things, and it makes the code really hard to analyze.
Hello David,
On 9/30/19 11:44 AM, David Laight wrote:
From: Bjorn Helgaas
quoted
Sent: 27 September 2019 23:02
On Fri, Aug 16, 2019 at 07:50:41PM +0300, Sergey Miroshnichenko wrote:
quoted
When hot-adding a device, the bridge may have windows not big enough (or
fragmented too much) for newly requested BARs to fit in. And expanding
these bridge windows may be impossible because blocked by "neighboring"
BARs and bridge windows.
Still, it may be possible to allocate a memory region for new BARs with the
following procedure:
1) notify all the drivers which support movable BARs to pause and release
the BARs; the rest of the drivers are guaranteed that their devices will
not get BARs moved;
2) release all the bridge windows except of root bridges;
3) try to recalculate new bridge windows that will fit all the BAR types:
- fixed;
- immovable;
- movable;
- newly requested by hot-added devices;
4) if the previous step fails, disable BARs for one of the hot-added
devices and retry from step 3;
5) notify the drivers, so they remap BARs and resume.
You don't do the actual recalculation in *this* patch, but since you
mention the procedure here, are we confident that we never make things
worse?
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.
Yep, removing everything and starting again is probably OTT and most of the churn won't help.
I think you need to work out what can be moved in order to make the required resources available
to each bus and then make the required changes.
In the simplest case you are trying to add resource below a bridge so need to 'shuffle'
everything allocated after that bridge to later addresses (etc).
Thank you for the review and suggestions!
But a bridge window may be fragmented: its total free space is enough
to fit everything, but no sufficient gaps for the new BARs. And this
bridge window may be jammed between two immovable/fixed BARs.
Or there may be lots of empty spaces in lower addresses after un-plugs,
but everything if fixed/immovable on higher addresses.
I've spent some time thinking on an optimization technique which can
be efficient enough (touch as few BARs as possible) with as high
success rate as calculating from scratch - and concluded that it is
not worth it: if only release the "obstructing" BARs and bridge
windows, a hotplug event will affect a half of (n+m) on average, which
is still O(n+m), where n is a number of endpoints, and m is a
number of bridges. But it's still need to resize windows of a root and
other common bridges.
Calculating bridge windows from scratch is relatively straightforward
and fast, so I have just added support for fixed/immovable BARs there
and reused.
Many devices that support address reassignment might not need to be moved - so there is
no point remmapping them.
And it's the same algorithm that allocated BARs in first place, so it
will reassign the same BARs for the non-affected part of the topology.
There is also the case when a device that is present but not currently is use could be taken
through a remove+insert sequence in order to change its resources.
Much easier to implement than 'remap while active'.
This would require a call into the driver (than can sleep) to request whether it is idle.
(and probably one at the end if the remove wasn't done).
Unbind+rebind the "immovable" drivers of non-opened devices may
increase the probability of successful BAR allocation, but I'm afraid
this will produce some amount of false hotplug-like events in the logs.
Probably also some undesired effects like spikes in power consumption
because of driver initialization.
Best regards,
Serge
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, Sep 30, 2019 at 03:59:25PM +0300, Sergey Miroshnichenko wrote:
Hello Bjorn,
On 9/28/19 1:02 AM, Bjorn Helgaas wrote:
quoted
On Fri, Aug 16, 2019 at 07:50:41PM +0300, Sergey Miroshnichenko wrote:
quoted
When hot-adding a device, the bridge may have windows not big enough (or
fragmented too much) for newly requested BARs to fit in. And expanding
these bridge windows may be impossible because blocked by "neighboring"
BARs and bridge windows.
Still, it may be possible to allocate a memory region for new BARs with the
following procedure:
1) notify all the drivers which support movable BARs to pause and release
the BARs; the rest of the drivers are guaranteed that their devices will
not get BARs moved;
2) release all the bridge windows except of root bridges;
3) try to recalculate new bridge windows that will fit all the BAR types:
- fixed;
- immovable;
- movable;
- newly requested by hot-added devices;
4) if the previous step fails, disable BARs for one of the hot-added
devices and retry from step 3;
5) notify the drivers, so they remap BARs and resume.
You don't do the actual recalculation in *this* patch, but since you
mention the procedure here, are we confident that we never make things
worse?
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.
quoted
I'm not sure why the PCI_CLASS_DISPLAY_VGA special case is there; can
you add a comment about why that's needed? Obviously we can't move
the 0xa0000 legacy frame buffer because I think devices are allowed to
claim that region even if no BAR describes it. But I would think
*other* BARs of VGA devices could be movable.
Sure, I'll add a comment to the code.
The issue that we are avoiding by that is the "nomodeset" command line
argument, which prevents a video driver from being bound, so the BARs
are seems to be used, but can't be moved, otherwise machines just hang
after hotplug events. That was the only special ugly case we've
spotted during testing. I'll check if it will be enough just to work
around the 0xa0000.
"nomodeset" is not really documented and is a funny way to say "don't
bind video drivers that know about it", but OK. Thanks for checking
on the other BARs.
quoted
quoted
+bool pci_movable_bars_enabled(void);
I would really like it if this were simply
extern bool pci_no_movable_bars;
in drivers/pci/pci.h. It would default to false since it's
uninitialized, and "pci=no_movable_bars" would set it to true.
I have a premonition of platforms that will not support the feature.
Wouldn't be better to put this variable-flag to include/linux/pci.h ,
so code in arch/* can set it, so they could work by default, without
the command line argument?
In general I don't see why a platform wouldn't support this since
there really isn't anything platform-specific here. But if a platform
does need to disable it, having arch code set this flag sounds
reasonable. We shouldn't make it globally visible until we actually
need that, though.
quoted
We have similar "=off" and "=force" parameters for ASPM and other
things, and it makes the code really hard to analyze.
The "=off" and "=force" things are the biggest things I'd like to
avoid.
Bjorn
On Mon, Sep 30, 2019 at 03:59:25PM +0300, Sergey Miroshnichenko wrote:
quoted
Hello Bjorn,
On 9/28/19 1:02 AM, Bjorn Helgaas wrote:
quoted
On Fri, Aug 16, 2019 at 07:50:41PM +0300, Sergey Miroshnichenko wrote:
quoted
When hot-adding a device, the bridge may have windows not big enough (or
fragmented too much) for newly requested BARs to fit in. And expanding
these bridge windows may be impossible because blocked by "neighboring"
BARs and bridge windows.
Still, it may be possible to allocate a memory region for new BARs with the
following procedure:
1) notify all the drivers which support movable BARs to pause and release
the BARs; the rest of the drivers are guaranteed that their devices will
not get BARs moved;
2) release all the bridge windows except of root bridges;
3) try to recalculate new bridge windows that will fit all the BAR types:
- fixed;
- immovable;
- movable;
- newly requested by hot-added devices;
4) if the previous step fails, disable BARs for one of the hot-added
devices and retry from step 3;
5) notify the drivers, so they remap BARs and resume.
You don't do the actual recalculation in *this* patch, but since you
mention the procedure here, are we confident that we never make things
worse?
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.
quoted
quoted
I'm not sure why the PCI_CLASS_DISPLAY_VGA special case is there; can
you add a comment about why that's needed? Obviously we can't move
the 0xa0000 legacy frame buffer because I think devices are allowed to
claim that region even if no BAR describes it. But I would think
*other* BARs of VGA devices could be movable.
Sure, I'll add a comment to the code.
The issue that we are avoiding by that is the "nomodeset" command line
argument, which prevents a video driver from being bound, so the BARs
are seems to be used, but can't be moved, otherwise machines just hang
after hotplug events. That was the only special ugly case we've
spotted during testing. I'll check if it will be enough just to work
around the 0xa0000.
"nomodeset" is not really documented and is a funny way to say "don't
bind video drivers that know about it", but OK. Thanks for checking
on the other BARs.
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)
return false;
#endif
return pci_dev_movable(dev, res->child);
}
quoted
quoted
quoted
+bool pci_movable_bars_enabled(void);
I would really like it if this were simply
extern bool pci_no_movable_bars;
in drivers/pci/pci.h. It would default to false since it's
uninitialized, and "pci=no_movable_bars" would set it to true.
I have a premonition of platforms that will not support the feature.
Wouldn't be better to put this variable-flag to include/linux/pci.h ,
so code in arch/* can set it, so they could work by default, without
the command line argument?
In general I don't see why a platform wouldn't support this since
there really isn't anything platform-specific here. But if a platform
does need to disable it, having arch code set this flag sounds
reasonable. We shouldn't make it globally visible until we actually
need that, though.
On powerpc the Extended Error Handling hardware facility doesn't allow
to shuffle the BARs (without notifying the platform code), otherwise
it reports errors.
I'm working on adding support for powerpc/powernv, but powerpc/pseries
also has EEH, and I don't have a hardware to test there.
So the arch/powerpc/platforms/pseries/setup.c will be modified as
follows in the next version of this patchset:
On Wed, Oct 16, 2019 at 06:50:30PM +0300, Sergey Miroshnichenko wrote:
On 10/16/19 1:14 AM, Bjorn Helgaas wrote:
quoted
On Mon, Sep 30, 2019 at 03:59:25PM +0300, Sergey Miroshnichenko wrote:
quoted
On 9/28/19 1:02 AM, Bjorn Helgaas wrote:
quoted
quoted
quoted
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.