Hello coreboot community,
For our work on POWER9 coreboot port we were using Skiboot [1] packed into FIT payload. While it worked fine, we wanted to provide users with easier way of testing it on hardware. I believe it may be possible to modify only one of PNOR (fancy name for flash given by POWER people) partitions. In order to do this, we would have to fit whole CBFS into 512k (actually 1M including ECC, but that would require non-power-of-2 size), which should be doable with LTO and compressed payload.
According to [2], whole file compression is not available for FIT payloads. I understand that this makes sense, thanks to this we can decompress/load individual parts of payload (e.g. kernel and initramfs) once, otherwise we would have to decompress it to read metadata first and then move code around in memory.
However, FIT format doesn't hold information about uncompressed sizes of its components, required by decompressing functions in coreboot. Instead, their compressed sizes [3] are used to initialize output region sizes [4] and those are then passed to decompressors [5]. Only FDT has a workaround to this problem [6], and kernel's size is calculated in an arm64-specific way in [7]. On other architectures components (other than FDT) are partially decompressed until they run of of space, silently ignoring the rest.
What should be "the proper way" of getting uncompressed size? I can think of the following options, listed in no particular order:
1. Extend hack used for FDT to other components, use it for all compressed parts. Arbitrary size multiplier may either waste some space potentially overlapping other component's memory range or not be enough in edge cases (e.g. mostly zeroed file).
2. For LZMA you may get decompressed size from the header, unfortunately this is not possible with LZ4.
3. Dry decompression just to check proper size - sounds too costly.
4. New properties for components' nodes in ITS file. May need changes in mkimage, haven't looked yet. Preferably shouldn't have to be manually written to ITS as that would be prone to error. While playing with mkimage, may add automatic compression, right now it expects already compressed files in ITS, although this can also be scripted.
5. Add more arch- and payload-specific parsing of images, as was done for arm64. This may require some kind of mark that a payload needs special handling (new CBFS attribute maybe?)
6. Enable whole file compression of FIT payloads, which results in juggling in RAM.
7. Mix of the above.
For our current issue we can also load Skiboot from another PNOR partition (from where it is normally loaded by Hostboot [8] which we are basically substituting with coreboot) and manually create FDT for it. This would however break normal coreboot boot flow and we would like to avoid that.
[1] https://github.com/open-power/skiboot [2] https://doc.coreboot.org/lib/payloads/fit.html#supported-compression [3] https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src... [4] https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src... [5] https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src... [6] https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src... [7] https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src... [8] https://github.com/open-power/hostboot
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.
Nico
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). 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.
Note that this doesn't apply to QEMU port that is currently under review on gerrit. In that case FDT is created by QEMU and its address is passed in one of the registers; this is not the case on hardware platform where it has to be created either from scratch or, as in our case, from preexisting skeleton.
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).
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?
Nico
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.
On 21.12.2021 20:34, Krystian Hebel wrote:
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.
Just FYI, we decided to switch to ELF anyway. Some of the code will be duplicated, but it still is less than the glue code that would have to be used otherwise.
Problem with FIT payload (de-)compression is thus not resolved.
-
Krystian Hebel -
Nico Huber