diff mbox series

[RFC,v4,05/21] PCI: hotplug: Add a flag for the movable BARs feature

Message ID 20190311133122.11417-6-s.miroshnichenko@yadro.com
State Changes Requested
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Allow BAR movement during hotplug | expand

Commit Message

Sergei Miroshnichenko March 11, 2019, 1:31 p.m. UTC 
If a new PCIe device has been hot-plugged between the two active ones
without big enough gap between their BARs, these BARs should be moved
if their drivers support this feature. The drivers should be notified
and paused during the procedure:

1)                 dev 8 (new)
                       |
                       v
.. |  dev 3  |  dev 3  |  dev 5  |  dev 7  |
.. |  BAR 0  |  BAR 1  |  BAR 0  |  BAR 0  |

2)                             dev 8
                                 |
                                 v
.. |  dev 3  |  dev 3  | -->           --> |  dev 5  |  dev 7  |
.. |  BAR 0  |  BAR 1  | -->           --> |  BAR 0  |  BAR 0  |

 3)

.. |  dev 3  |  dev 3  |  dev 8  |  dev 8  |  dev 5  |  dev 7  |
.. |  BAR 0  |  BAR 1  |  BAR 0  |  BAR 1  |  BAR 0  |  BAR 0  |

Thus, prior reservation of memory regions by BIOS/bootloader/firmware
is not required anymore for the PCIe hotplug.

The PCI_MOVABLE_BARS flag is set by the platform is this feature is
supported and tested, but can be overridden by the following command
line option:
    pcie_movable_bars={ off | force }

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 .../admin-guide/kernel-parameters.txt         |  7 ++++++
 drivers/pci/pci.c                             | 24 +++++++++++++++++++
 include/linux/pci.h                           |  2 ++
 3 files changed, 33 insertions(+)

Comments

Bjorn Helgaas March 26, 2019, 7:24 p.m. UTC | #1 
On Mon, Mar 11, 2019 at 04:31:06PM +0300, Sergey Miroshnichenko wrote:
> If a new PCIe device has been hot-plugged between the two active ones
> without big enough gap between their BARs, 

Just to speak precisely here, a hot-added device is not "between" two
active ones because the new device has zeros in its BARs.

BARs from different devices can be interleaved arbitrarily, subject to
bridge window constraints, so we can really only speak about a *BAR*
(not the entire device) being between two other BARs.

Also, I don't think there's anything here that is PCIe-specific, so we
should talk about "PCI", not "PCIe".

> these BARs should be moved
> if their drivers support this feature. The drivers should be notified
> and paused during the procedure:
> 
> 1)                 dev 8 (new)
>                        |
>                        v
> .. |  dev 3  |  dev 3  |  dev 5  |  dev 7  |
> .. |  BAR 0  |  BAR 1  |  BAR 0  |  BAR 0  |
> 
> 2)                             dev 8
>                                  |
>                                  v
> .. |  dev 3  |  dev 3  | -->           --> |  dev 5  |  dev 7  |
> .. |  BAR 0  |  BAR 1  | -->           --> |  BAR 0  |  BAR 0  |
> 
>  3)
> 
> .. |  dev 3  |  dev 3  |  dev 8  |  dev 8  |  dev 5  |  dev 7  |
> .. |  BAR 0  |  BAR 1  |  BAR 0  |  BAR 1  |  BAR 0  |  BAR 0  |
> 
> Thus, prior reservation of memory regions by BIOS/bootloader/firmware
> is not required anymore for the PCIe hotplug.
> 
> The PCI_MOVABLE_BARS flag is set by the platform is this feature is
> supported and tested, but can be overridden by the following command
> line option:
>     pcie_movable_bars={ off | force }

A chicken switch to turn this functionality off is OK, but I think it
should be enabled by default.  There isn't anything about this that's
platform-specific, is there?

> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  7 ++++++
>  drivers/pci/pci.c                             | 24 +++++++++++++++++++
>  include/linux/pci.h                           |  2 ++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 2b8ee90bb644..d40eaf993f80 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3417,6 +3417,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
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 69898fe5255e..4dac49a887ec 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -139,6 +139,30 @@ static int __init pcie_port_pm_setup(char *str)
>  }
>  __setup("pcie_port_pm=", pcie_port_pm_setup);
>  
> +static bool pcie_movable_bars_off;
> +static bool pcie_movable_bars_force;
> +static int __init pcie_movable_bars_setup(char *str)
> +{
> +	if (!strcmp(str, "off"))
> +		pcie_movable_bars_off = true;
> +	else if (!strcmp(str, "force"))
> +		pcie_movable_bars_force = true;
> +	return 1;
> +}
> +__setup("pcie_movable_bars=", pcie_movable_bars_setup);
> +
> +bool pci_movable_bars_enabled(void)
> +{
> +	if (pcie_movable_bars_off)
> +		return false;
> +
> +	if (pcie_movable_bars_force)
> +		return true;
> +
> +	return pci_has_flag(PCI_MOVABLE_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
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index cb2760a31fe2..cbe661aff9f5 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -866,6 +866,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_MOVABLE_BARS	= 0x00000080,	/* Runtime BAR reassign after hotplug */
>  };
>  
>  /* These external functions are only available when PCI support is enabled */
> @@ -1345,6 +1346,7 @@ unsigned char pci_bus_max_busnr(struct pci_bus *bus);
>  void pci_setup_bridge(struct pci_bus *bus);
>  resource_size_t pcibios_window_alignment(struct pci_bus *bus,
>  					 unsigned long type);
> +bool pci_movable_bars_enabled(void);
>  
>  #define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
>  #define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)
> -- 
> 2.20.1
>
Sergei Miroshnichenko March 27, 2019, 5:16 p.m. UTC | #2 
On 3/26/19 10:24 PM, Bjorn Helgaas wrote:
> On Mon, Mar 11, 2019 at 04:31:06PM +0300, Sergey Miroshnichenko wrote:
>> If a new PCIe device has been hot-plugged between the two active ones
>> without big enough gap between their BARs, 
> 
> Just to speak precisely here, a hot-added device is not "between" two
> active ones because the new device has zeros in its BARs.
> 
> BARs from different devices can be interleaved arbitrarily, subject to
> bridge window constraints, so we can really only speak about a *BAR*
> (not the entire device) being between two other BARs.
> 
> Also, I don't think there's anything here that is PCIe-specific, so we
> should talk about "PCI", not "PCIe".
> 

I agree, that should be rephrased. This patchset intends to solve the problem when a
bridge window is not big enough (or fragmented too much) to fit new BARs, and it can't be
expanded enough because blocked by "neighboring" BARs.

>> these BARs should be moved
>> if their drivers support this feature. The drivers should be notified
>> and paused during the procedure:
>>
>> 1)                 dev 8 (new)
>>                        |
>>                        v
>> .. |  dev 3  |  dev 3  |  dev 5  |  dev 7  |
>> .. |  BAR 0  |  BAR 1  |  BAR 0  |  BAR 0  |
>>
>> 2)                             dev 8
>>                                  |
>>                                  v
>> .. |  dev 3  |  dev 3  | -->           --> |  dev 5  |  dev 7  |
>> .. |  BAR 0  |  BAR 1  | -->           --> |  BAR 0  |  BAR 0  |
>>
>>  3)
>>
>> .. |  dev 3  |  dev 3  |  dev 8  |  dev 8  |  dev 5  |  dev 7  |
>> .. |  BAR 0  |  BAR 1  |  BAR 0  |  BAR 1  |  BAR 0  |  BAR 0  |
>>
>> Thus, prior reservation of memory regions by BIOS/bootloader/firmware
>> is not required anymore for the PCIe hotplug.
>>
>> The PCI_MOVABLE_BARS flag is set by the platform is this feature is
>> supported and tested, but can be overridden by the following command
>> line option:
>>     pcie_movable_bars={ off | force }
> 
> A chicken switch to turn this functionality off is OK, but I think it
> should be enabled by default.  There isn't anything about this that's
> platform-specific, is there?
> 

I'm a bit afraid to suppose that; I was once surprised that bus numbers can't be assigned
arbitrarily on some platforms [1], so probably there are some similar restrictions on BARs
too.

Was going to propose adding pci_add_flags(PCI_MOVABLE_BARS) into arch/.../init.c for
tested platforms, so there will be less upset people with their BARs suddenly broken. But
this logic can be reversed: pci_clear_flags(PCI_MOVABLE_BARS) for platforms where movable
BARs can't work.

Serge

[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-September/178103.html

>> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
>> ---
>>  .../admin-guide/kernel-parameters.txt         |  7 ++++++
>>  drivers/pci/pci.c                             | 24 +++++++++++++++++++
>>  include/linux/pci.h                           |  2 ++
>>  3 files changed, 33 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 2b8ee90bb644..d40eaf993f80 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -3417,6 +3417,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
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 69898fe5255e..4dac49a887ec 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -139,6 +139,30 @@ static int __init pcie_port_pm_setup(char *str)
>>  }
>>  __setup("pcie_port_pm=", pcie_port_pm_setup);
>>  
>> +static bool pcie_movable_bars_off;
>> +static bool pcie_movable_bars_force;
>> +static int __init pcie_movable_bars_setup(char *str)
>> +{
>> +	if (!strcmp(str, "off"))
>> +		pcie_movable_bars_off = true;
>> +	else if (!strcmp(str, "force"))
>> +		pcie_movable_bars_force = true;
>> +	return 1;
>> +}
>> +__setup("pcie_movable_bars=", pcie_movable_bars_setup);
>> +
>> +bool pci_movable_bars_enabled(void)
>> +{
>> +	if (pcie_movable_bars_off)
>> +		return false;
>> +
>> +	if (pcie_movable_bars_force)
>> +		return true;
>> +
>> +	return pci_has_flag(PCI_MOVABLE_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
>>  
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index cb2760a31fe2..cbe661aff9f5 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -866,6 +866,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_MOVABLE_BARS	= 0x00000080,	/* Runtime BAR reassign after hotplug */
>>  };
>>  
>>  /* These external functions are only available when PCI support is enabled */
>> @@ -1345,6 +1346,7 @@ unsigned char pci_bus_max_busnr(struct pci_bus *bus);
>>  void pci_setup_bridge(struct pci_bus *bus);
>>  resource_size_t pcibios_window_alignment(struct pci_bus *bus,
>>  					 unsigned long type);
>> +bool pci_movable_bars_enabled(void);
>>  
>>  #define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
>>  #define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)
>> -- 
>> 2.20.1
>>
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2b8ee90bb644..d40eaf993f80 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3417,6 +3417,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
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 69898fe5255e..4dac49a887ec 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -139,6 +139,30 @@  static int __init pcie_port_pm_setup(char *str)
 }
 __setup("pcie_port_pm=", pcie_port_pm_setup);
 
+static bool pcie_movable_bars_off;
+static bool pcie_movable_bars_force;
+static int __init pcie_movable_bars_setup(char *str)
+{
+	if (!strcmp(str, "off"))
+		pcie_movable_bars_off = true;
+	else if (!strcmp(str, "force"))
+		pcie_movable_bars_force = true;
+	return 1;
+}
+__setup("pcie_movable_bars=", pcie_movable_bars_setup);
+
+bool pci_movable_bars_enabled(void)
+{
+	if (pcie_movable_bars_off)
+		return false;
+
+	if (pcie_movable_bars_force)
+		return true;
+
+	return pci_has_flag(PCI_MOVABLE_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
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index cb2760a31fe2..cbe661aff9f5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -866,6 +866,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_MOVABLE_BARS	= 0x00000080,	/* Runtime BAR reassign after hotplug */
 };
 
 /* These external functions are only available when PCI support is enabled */
@@ -1345,6 +1346,7 @@  unsigned char pci_bus_max_busnr(struct pci_bus *bus);
 void pci_setup_bridge(struct pci_bus *bus);
 resource_size_t pcibios_window_alignment(struct pci_bus *bus,
 					 unsigned long type);
+bool pci_movable_bars_enabled(void);
 
 #define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
 #define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)