On 18.12.2021 18:07, Nico Huber wrote:
On 15.12.21 13:24, Krystian Hebel wrote:
Hi Nico,
On 15.12.2021 12:21, Nico Huber wrote:
Hi Krystian,
On 14.12.21 13:00, Krystian Hebel wrote:
For our work on POWER9 coreboot port we were using Skiboot [1] packed into FIT payload.
I might just miss it because I never worked with FIT, but maybe I'm not the only one: Can you elaborate why you chose FIT? Reading through your mail (and without further knowledge) it would just seem like the wrong choice.
Good point, I spent so much time in this project that I treat too many things as obvious. Don't hesitate to ask if I omit something important.
Skiboot must be supplied with information about hardware. Some of that is generated by code based on current configuration (e.g. number of cores, their IDs, memory amount and associativity), but a lot is always the same for a given platform or even whole architecture (BMC sensors, interrupt controller, register address space, LPC controller).
Isn't this all or mostly information that coreboot has already available? It seems much like the situation on x86 with ACPI. We also had static ACPI tables that redundantly stored infor- mation at first. However, static ACPI tables were never treated first-class, sometimes even like an alien, and were prone to rot. Today, we try a good compromise between runtime generation and static tables (which can be extended at runtime).
Not everything is defined in coreboot sources, AFAICT only XSCOM, LPC and one out of 3 I2C buses are used, rest (almost 600 lines in dts) is just static data. There is a lot of stuff that coreboot doesn't have to worry about, but Skiboot and OS do.
This can be passed either in HDAT structure (Hostboot data, proprietary and mostly undocumented format in supposedly open firmware) or, as we learned after asking on OpenPower-Firmware mailing list, using FDT:
https://lists.ozlabs.org/pipermail/openpower-firmware/2021-May/000641.html
In our current setup that second, constant part is supplied as FDT in FIT image so it can be added as .dts file, which is easier to read and understand than C code that would be used for creating these nodes.
Hmmm, I read above mailinglist thread. But I still miss the link to FIT. coreboot would have a dtb (doesn't matter if it generates it at runtime or a file in CBFS) and a payload (in CBFS or some other flash partition), right? Why do you need FIT?
We don't _need_ FIT. We also don't need to implement new way of loading the payload if existing one is working as expected, more or less. Do you suggest removing FIT from coreboot completely? :)
FIT code already does the heavy lifting: it parses dtb to modifiable format, appends firmware, compat and memory nodes (although the latter are added in different format than Skiboot expects and we have to modify it - both formats are compatible with spec, but not with each other...), gives opportunity for fixups to DT before repacking it, sets entry point and argument. Doing the same with dtb loaded from CBFS is possible, but it would result in mostly duplicated code. The only pieces of FIT that our code doesn't use are overlays and initramfs.
Another option could be to split fit_payload() into two halves, first one would just load FIT from CBFS and choose appropriate config, second would be given kernel, initramfs and dtb as 'struct region' and work from there. That way we could mix various ways of obtaining pieces of the puzzle without duplicating code. Note that this would still leave bugs (normally I would called it "lack of implementation", but documentation says explicitly that this is supported) in the FIT decompression unresolved.