I have pushed a CL to document the discussion here and added more details about the resource allocator as well: https://review.coreboot.org/c/coreboot/+/43603. I have moved questions from Aaron to the CL to continue discussion there. Thanks!
On Wed, May 27, 2020 at 8:58 AM Aaron Durbin adurbin@google.com wrote:
On Fri, May 22, 2020 at 10:01 AM Nico Huber nico.h@gmx.de wrote:
On 19.05.20 02:07, Furquan Shaikh wrote:
On Sun, May 17, 2020 at 2:01 PM Nico Huber nico.h@gmx.de wrote:
I told people that I had something similar in mind. But actually, I don't like it very much. I fear, if we move things aside, they can be left behind and we might soon have to discuss platform deprecation again.
Having both old and new allocators gives us the option to move the chipsets/boards to the older one in case issues are identified even later on and not in the first pass of testing. So, I like the idea of having both allocators in the tree at least for a while.
ok, let's do this.
As mentioned above, the old allocator is currently limited to only the AMD chipsets that are known to have breakages and require more than a quick fix: https://review.coreboot.org/c/coreboot/+/41444. The other chipsets that were identified as problematic last week have all been fixed since: https://review.coreboot.org/c/coreboot/+/41374.
Another reason why I like having both in the tree is it serves as a quick A/B test without anyone having to revert series of patches just to test the old allocator.
That's a great idea. I will likely pick up the opportunity to add more, optional things to test.
It would also be harder to make changes in generic code like device/pci_device when one has to be sure that both allocators work fine.
For the most part, the rest of the code should not really be aware of the internals of any of the allocators. (In practice it is not always true). I understand that we had some assumptions in find_pci_tolm() and how invalid resources are handled. With the following changes: https://review.coreboot.org/c/coreboot/+/41524, those assumptions should no longer be required. We just need a set of rules saying what goes into the allocator and what is the state coming out.
Let's try it. We can put this in Documentation/ once we agree on the rules.
INPUT
The allocator processes, per domain sub-tree, all resources with IORESOURCE_IO or IORESOURCE_MEM set. From here on, these are simply referred to as *resources*.
Resources with the IORESOURCE_FIXED bit set are referred to as *fixed resources*, resources with the IORESOURCE_BRIDGE bit set as *bridge resources*, and resources with the IORESOURCE_ASSIGNED bit set as *assigned resources*.
We should elaborate on the semantics of 'assigned' and its meaning.
Resources with IORESOURCE_IO set must not have IORESOURCE_MEM set.
Resources that should not be changed by the allocator must be fixed resources.
Fixed resources must be assigned resources and have a valid `base` address pre-assigned.
A fixed resource must not overlap with any other fixed resource within the same address space (IO / MEM), unless either of them is a bridge resource.
A fixed bridge resource must not overlap with any other fixed resource within the same address space and on the same bus (i.e. the same device or its siblings), unless exactly one of them has IORESOURCE_SUBTRACTIVE set.
What are you assuming w.r.t. decode priority with SUBTRACTIVE being set?
OUTPUT
All device attributes beside non-fixed resources are left unaltered.
What did you mean by device attributes?
-Aaron