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*.
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.
OUTPUT ------
All device attributes beside non-fixed resources are left unaltered.
The structure of resource lists (`next` pointers) is left unaltered.
The `index` and `gran` attributes of all resources are left unaltered.
The `size`, `limit` and `align` attributes of all non-bridge resources are left unaltered.
With exception of IORESOURCE_ASSIGNED, all `flags` of all resources are left unaltered.
All non-fixed assigned resources downstream of a bridge must be covered by an assigned bridge resource of that bridge.
For all assigned resources, the INPUT rules must apply, where all assigned resources would be treated as fixed resources.
Another option: we could identify the differences in the new allocator that had a part in the breakage and only provide a backward compatible implementation for those. Actually, I think it's only one: the resource constraining strategy for the domain. To make this compatible, we could a) drop all but the biggest memrange below 4G for the domain, and b) assign/steal resources from the end of the memrange at the domain level. It would closely mimic what the old code tried to achieve, so seems safe, somehow.
I don't think it is right to assume that the biggest window should always be chosen.
To be clear. I meant to add an optional mode with these features, selectable in Kconfig. The thought was, such a "compatibility mode" should choose the biggest window, because that's what the old allocator did.
Yes, we can start by allocating resources top-down by always selecting the last window and using its top. I already have patches uploaded for this (which need a small change to select the top of last memory): https://review.coreboot.org/c/coreboot/+/41418. But, I don't really want to take that change in because we will continue masking the issues for chipsets/mainboards which were not really doing the right thing. Instead, it would be better to identify such chipsets and fix them.
Right. My current idea is to add a bunch of optional allocator features. On one hand, to allow testing of the existing code. For instance, if a) and b) can be tested individually, that can give us a hint what might be wrong with the chipset code. And on the other hand, to test some new features. I'll have to see how invasive that will be and then either push patches for v4 or start a v4.5 just for experiments ;)
* The above-4G memrange should be aligned to a power of 2 for better MTRR usage (if that isn't the case already?).All mem resource allocation already happens at 4KiB alignment.
Right, my bad, wrong words, bear with me. I meant, should be rounded up to a power of 2.
Nico