Re: [PATCH v5 16/23] PCI: hotplug: movable BARs: Don't reserve IO/mem bus space

From: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
To: Oliver O'Halloran <oohall@gmail.com>
Cc: <linux-pci@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>,
	Bjorn Helgaas <helgaas@kernel.org>, <linux@yadro.com>
Subject: Re: [PATCH v5 16/23] PCI: hotplug: movable BARs: Don't reserve IO/mem bus space
Date: Wed, 4 Sep 2019 14:22:54 +0300	[thread overview]
Message-ID: <e11fa8c6-4892-82a9-78ad-bc6bdb6f4037@yadro.com> (raw)
In-Reply-To: <1edd23fd93386d8a69bc9280329b2d7c819155d4.camel@gmail.com>

On 9/4/19 8:42 AM, Oliver O'Halloran wrote:
> 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.
> 

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

>> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
>> ---
>>  drivers/pci/setup-bus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index c7b7e30c6284..7d64ec8e7088 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -1287,7 +1287,7 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
>>  
>>  	case PCI_HEADER_TYPE_BRIDGE:
>>  		pci_bridge_check_ranges(bus);
>> -		if (bus->self->is_hotplug_bridge) {
>> +		if (bus->self->is_hotplug_bridge && !pci_movable_bars_enabled()) {
>>  			additional_io_size  = pci_hotplug_io_size;
>>  			additional_mem_size = pci_hotplug_mem_size;
>>  		}
> 