Message-ID: <PS2P216MB0642DA40B66B36581A238EF580480@PS2P216MB0642.KORP216.PROD.OUTLOOK.COM>
Date:   Mon, 11 Mar 2019 16:22:47 +0000
From:   Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
To:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
        "corbet@....net" <corbet@....net>,
        Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
Subject: [PATCH v2 0/4] PCI: Patch series to support Thunderbolt without any
 BIOS support

_________________________________________________________________

If possible, please try to include this in the upcoming release. I have 
been slow in getting PATCH v2 out but overall, it should not be too much 
to do.

You have seen the first patch before, which is the most complicated. It 
is based entirely on Mika Westerberg's code and he was happy that the 
changes are fine. So this should help a little in getting it through.

The second is merely comments and no code - easy to inspect.

The third has very few lines and should be easy to test and revert in 
the worst case.

The fourth brings the most changes functionally, but has nothing in it 
algorithmically. It is just taking command line parameters and setting 
the variables - hence it should be easy to visually inspect for 
correctness.

I have worked very hard on this and would be very appreciative if it 
could make the merge window so that the next eight weeks can be spent by 
me responding to any potential concerns about it.

Thank you!
_________________________________________________________________


This new revision of my patches solves some small problems and tidies up 
the commit notes, migrating background information into the cover 
letter.

Together, these patches allow for a Thunderbolt 3 Titan Ridge add-in 
card to be supported on any system with a PCI bus, without an inkling of 
firmware support. I have been using the following (somewhat overkill) 
kernel arguments for many weeks now with two dual-port add-in cards:

pci=assign-busses,hpbussize=0x33,realloc,hp_io_size=0,hp_mmio_size=256M,hp_mmio_pref_size=64G,nocrs 
[required on some other platforms] pcie_ports=native

This exceeds the official Intel Thunderbolt implementation in that 
arbitrary memory may be easily assigned, allowing for cards like Xeon 
Phi coprocessor with massive BARs (the size of their onboard RAM) to be 
placed on Thunderbolt.

PATCH 0001 solves the resource alignment bug here, which also applies to 
external graphics: https://bugzilla.kernel.org/show_bug.cgi?id=199581 
The resource alignment bug is not Thunderbolt-specific, and applies to 
nested PCI hotplug bridges. Hence, it needed to be fixed regardless of 
Thunderbolt. But Thunderbolt is the most likely use case affected by the 
bug.

PATCH 0002 is just comment clean-up and no code is changed. I noticed 
that a lot of the block comments in drivers/pci/setup-bus.c did not meet 
the kernel coding style guidelines so I fixed them all when I needed to 
change a comment. This has been separated into its own patch upon 
request of the maintainers.

PATCH 0003 fixes a bug that is nasty, but rarely shows. This bugfix is 
critical for that next Thunderbolt patch. I will provide the previously 
requested dmesg output showing the bug in another email. For this, I 
will be using an unmodified kernel and show how requesting 
hpmemsize=256M on my machine results in 512M being allocated in MMIO 
window, and hence the assignment failing because there is not enough 
space. I can show it assigning 256M in MMIO successfully after my patch 
is applied.

Symptoms: When the kernel makes multiple passes at assigning resources, 
it can sometimes assign double the hpmemsize specified in kernel 
parameters into the MMIO bridge window.

The cause: pbus_size_io() and pbus_size_mem() both call 
find_free_bus_resource() to identify which bridge window of the bus 
matches the required resource type. Because this function explicitly 
skips resources with a parent, it will return NULL if the desired 
resource has been successfully assigned in a previous pass. The 
functions pbus_size_io() and pbus_size_mem() return -ENOSPC if 
find_free_bus_resource() returns NULL, meaning an assigned resource is 
interpreted as "out of space", which can result in 
__pci_bus_size_bridges allocating the required size again in another 
window.

The solution: my proposed patch renames find_free_bus_resource() to 
find_bus_resource_of_type() and modifies it to not skip resources with 
r->parent. The name change is because returning an assigned resource 
makes the resource potentially not "free". The calling functions, 
pbus_size_io() and pbus_size_mem() have checks added for b_res->parent 
and they return 0 (success) in this case. This is because a resource 
with a parent is already assigned and should be interpreted as a 
success, not a failure.

Testing: I have been using this proposed patch for many weeks now and it 
appears to work flawlessly. It has the added benefit of slashing the 
number of attempts taken to assign a given BAR, meaning a much cleaner 
dmesg. I am so confident in it that if it were not to be accepted, I 
would continue to compile my own kernel each release with it included, 
for my own use.

PATCH 0004 allows MMIO and MMIO_PREF sizes to be requested independently 
in kernel parameters. This is the user-facing patch that delivers the 
functional requirement of being able to specify the resource sizes 
independently. The other patches are prep-work for this patch, allowing 
it to work flawlessly. This will also make at least one other person 
very happy, providing a solution where none previously existed (if this 
is accepted, I will be answering their question with this patch): 
https://superuser.com/questions/1054657/how-can-i-reserve-hotplug-bridges-memory-only-for-prefetchable-memory-using-the

Nicholas Johnson (4):
  PCI: Consider alignment of hot-added bridges when distributing
    available resources
  PCI: Cleanup comments in setup-bus.c to meet kernel coding style
    guidelines
  PCI: Fix serious bug when sizing bridges with additional size
  PCI: modify kernel parameters to differentiate between MMIO and
    MMIO_PREF sizes

 .../admin-guide/kernel-parameters.txt         |  21 +-
 drivers/pci/pci.c                             |  39 +-
 drivers/pci/setup-bus.c                       | 513 +++++++++---------
 include/linux/pci.h                           |   3 +-
 4 files changed, 323 insertions(+), 253 deletions(-)

-- 
2.20.1

Powered by blists - more mailing lists