Pulling out some of the discussions that we already had inside Google to this wider audience, I am strongly against parallelization mechanisms that pull this much extra complexity into existing code (particularly the already very complicated CBFS framework). I think https://review.coreboot.org/c/coreboot/+/56049 is a perfect example of the maintainability drawbacks of this approach, requiring to weave large new chunks of code throughout the whole stack downwards, from CBFS into the rdev layer and finally into the platform SPI driver, just to be able to asynchronously execute a single read operation. The CBFS patch isn't even complete, there will be more code needed to support other primitives (e.g. cbfs_map()), decompression, etc. And it's duplicating a lot of the existing flow from the synchronous code in _cbfs_alloc().
I think if we want a parallelization mechanism, we should converge onto a single one that can solve a variety of current and future use cases, and can be applied optionally where individual platforms need it without having to grow tentacles into every other part of coreboot. And I think the much better model for that would be cooperative multithreading (e.g. setting up a second stack and execution context and then having the CPU ping-pong back and forth between them while the other one is idle and waiting on hardware somewhere). In fact coreboot already has all this implemented in src/lib/threads.c (CONFIG_COOP_MULTITASKING), although it seems that nobody has really used it in a while. The clear advantage of cooperative multithreading is that almost none of the code, especially not most of these in-between layers like CBFS and rdev, need to be modified to support it. You can create a thread and send it off to, say, begin loading the payload while the rest of the boot state machine is still running platform initialization code, and it can run through all the CBFS motions (including verification and whatever other optional features are enabled) without any of those having been written specifically with asynchronous operation in mind. You still need to make sure that the two threads don't both try to access shared resources at the same time, but you need to do that with futures too. In the file loading case, it would probably be enough to put a few critical sections in the SPI driver itself and there should be no need to pull it up into all the platform-agnostic general framework parts. The cost of cooperative multithreading is mostly just that you need the space for a second stack. In ramstage that should be a given anyway, and in romstage it can probably still be easily done on most (particularly recent) platforms. For an optional performance enhancement feature, I think that trade-off makes sense. On Wed, Jul 7, 2021 at 1:18 PM Raul Rangel <[email protected]> wrote: > > On Wed, Jul 7, 2021 at 12:49 PM Peter Stuge <[email protected]> wrote: > > > > Raul Rangel wrote: > > > I'm currently working on improving the boot time for the AMD Cezanne > > > platform. > > .. > > > Another difference between the latest AMD SoCs (Picasso, Cezanne), is > > > that RAM is available in bootblock. > > > > As I have understood, the PSP has both trained RAM and copied firmware from > > SPI to RAM when x86 comes out of reset. > > > > Is that accurate, false, or only partially accurate? > > It's partially accurate. The PSP will load bootblock into RAM. So > coreboot still needs to access the SPI flash to load everything else. > > > > > > One place where we spend a decent amount of time is reading from SPI > > > flash. We have the SPI speed/modes set to the optimal settings for > > > our platforms, but there is still room for improvement. > > > > Please provide numbers? > > Sorry, that sentence didn't come out how I wanted. I was just saying > that we could improve the boot time by exploring other avenues. > > > > > > The question is, how do we model these asynchronous operations, how is > > > data ownership handled, and how does the BSP know the operation is > > > done? > > > > I haven't yet reveiewed your API proposal, but find it an absolutely > > horrible idea to create a *general* API for asynchronous operations in > > coreboot, because - as you recognize - it can easily be misused to great > > detriment of the codebase, which already suffers chronically from such > > trivial problems as copy-paste:itis. Don't do it. > > > > There is zero incentive for developers to improve the source beyond > > making it work for their deadline; more complexity *will* create more > > problems. (I understand that you have good intentions proposing this > > change!) > > There has been a lot of work in refactoring the AMD codebases and > fixing things throughout the stack. We have reduced a good amount of > copy/paste when bringing up a new AMD SoC. So I don't agree that we > are all writing abandonware. Please have a look at the proposal before > making such hasty judgments. > > > > > > I'm curious to see what the community thinks, and welcome any feedback. > > > > A special purpose DMA API is another matter to me, because it's very well > > defined. It could still be useful beyond x86 and I think a blocking > > "wait-for-DMA-to-finish" API is easy to understand, easy to implement and > > to use, and sufficient since runtime flow is serial, especially when > > measuring. > > That was my original thought, but there are plenty of other things > that can be modeled as asynchronous operations. Stuffing it into the > rdev API felt wrong. > > Thanks, > Raul > > p.s., > > Just to add some more data. I performed an experiment where I switched > the payload compression from LZMA to LZ4. Since we no longer pay the > SPI read cost (thanks async!), we essentially load the payload for > free: > > ## Payload LZMZ compression (current) > ``` > Name Offset Type Size > fallback/payload 0x224dc0 simple elf 131417 (128 KiB) > ``` > ``` > 90:starting to load payload 1,123,962 (13) > 15:starting LZMA decompress 1,123,967 (5) > 16:finished LZMA decompress 1,131,201 (7,234) > ``` > > ## Payload LZ4 compression > ``` > Name Offset Type Size > fallback/payload 0x224dc0 simple elf 173233 (169 KiB) > ``` > > ``` > 90:starting to load payload 1,130,877 (12) > 17:starting LZ4 decompress 1,130,882 (5) > 18:finished LZ4 decompress 1,131,091 (209) > ``` > > ``` > Payload increase: > KiB: 169 - 128 = 41 KiB > %: (169 - 128) / 128 = 0.3203125 > > Decompression decrease: > us: 209 - 7234 = -7025 > %: (209 - 7234) / 7234 = -0.9711086535803152 > ``` > _______________________________________________ > coreboot mailing list -- [email protected] > To unsubscribe send an email to [email protected] _______________________________________________ coreboot mailing list -- [email protected] To unsubscribe send an email to [email protected]
