# 2022-03-29 - coreboot UEFI working group Next meeting: 2022-04-26
## Attendees: Jay, Werner, Martin, Sheng, Ron, David
## Minutes:
* In the coreboot meeting last week, we discussed a proposal from Intel to change the handoff mechanism to payloads [1]. After that discussion, the decision was made to move further discussion of that topic over to the UEFI working group meeting. There was another meeting on Monday, March 28th where we got a much better understanding of what’s going on and the reasons behind the proposal. * Intel wants to look at modifying the payload handoff as a part of their universal-scalable-firmware [2] initiative. * The current payload handoff method has a number of flaws that they’d like to fix, such as the address for stack being hardcoded. * This likely also gives coreboot an opportunity to address other parts of the USF spec that cause issues or incompatibilities for us. * The coreboot project got involved when Sheng suggested to Intel that it would be good to align the payload-handoff format with other projects such as coreboot. * Thanks Sheng! * Using CBOR is the current proposal, but Intel was open to discussing different formats than CBOR if anyone has a better suggestion. * If anyone is not familiar with CBOR, you can get more information at cbor.io [3]. * In the coreboot meeting, it was suggested that we push to just use coreboot tables as they’re already supported in a number of payloads. This really isn’t practical however. Intel would need to be able to modify the list of known cbmem IDs and they would need to be able to change the description format to something that’s more self-describing. * The coreboot project can, however, encapsulate a CBOR-based handoff-structure into cbmem, similar to what we currently do with ACPI tables. * Additionally Intel was willing to look at using CBOR structures as input and output to the FSP, so we could get rid of both the UPDs & HOBs. * The coreboot community can help to drive this, and Intel has said that they will send an email to the coreboot mailing list. * Since this is a part of the Universal-Scalable-Firmware initiative, it would be good to look that over for issues as well. Currently, it’s very Intel oriented, and specifies that the FSP will be used as the silicon initialization layer, which obviously doesn’t work for non-x86 platforms. * The details for the weekly meeting are below.
* Self Describing Blob discussion.
A copy of this meeting has been added to the coreboot calendar [4].
Biweekly on Monday, 4:00 – 5:00pm CET/CEST Berlin (Currently UTC+2) Video call link: https://meet.google.com/gha-shza-wnw Or dial: (DE) +49 30 300195187 PIN: 848 847 031# More phone numbers: https://tel.meet/gha-shza-wnw?pin=2380921555764 Self Describing Blob discussion Meeting minutes [5].
* Sean Rhodes from StarLabs has worked to get all of the coreboot related EDK2 patches submitted to their repo. These are still currently in review.
[1]: https://docs.google.com/document/d/1NRXqXcLBp5pFkHiJbrLdv3Spqh1Hu086HYkKrgKj... [2]: https://universalscalablefirmware.github.io/documentation/ [3]: https://cbor.io/ [4]: https://calendar.google.com/event?action=TEMPLATE&tmeid=NHVvZDRuYXZ2bjdt... [5]: https://docs.google.com/document/d/1IDQqr4cv8dQHa9faSxBc29xHSeKuXNTiMP3a3bLJ...
Hi
I would like to add a few notes to the meeting notes to clarify things a bit better.
* In the coreboot meeting, it was suggested that we push to just use
coreboot tables as they’re already supported in a number of payloads. This really isn’t practical however. Intel would need to be able to modify the list of known cbmem IDs and they would need to be able to change the description format to something that’s more self-describing.
CBMEM is an internal in memory database that coreboot uses. Payloads don't need to know cbmem and most actually don't. The handoff structure to payloads are 'coreboot tables'. These are far older than cbmem. Inside the coreboot tree those are even still called 'lb_table', so it dates back from when the project was called LinuxBIOS.
'really isn't practical' is a very relative term here... Monday I asked what existing firmware payloads (fwiw this is really a coreboot concept to begin with) don't support coreboot tables that would benefit from having a 'universal' self describing handoff. The answer is none: all existing firmware payloads support coreboot tables. So the only thing not 'practical' here is that UEFI teams don't have control over the handoff structure format that is inside coreboot and is used by coreboot payloads (coreboot tables). The proposed solution is a new format that all payloads and coreboot ought to support. Needless to say that this is a lot of work (adapting both coreboot and all existing payloads) with very little benefits for coreboot.
The current payload handoff method has a number of flaws that
they’d like to fix, such as the address for stack being hardcoded.
Normally payloads set up their own stack very early on so this is not a problem. The context here, was that I voiced some practical concerns about using CBOR as a handoff structure. LinuxBIOS or coreboot tables were carefully designed to be very easy to parse. In fact so easy to parse that Linux payloads on x86 are loaded the following way: - cbfstool has an assembly written trampoline (~150LOC) that parses the coreboot tables and fills in the zero page of Linux - This trampoline is position independent and stackless for maximum flexibility - cbfstool appends this trampoline to Linux payloads inside cbfs - This is done so that coreboots runtime only knows how to load the 'SELF' format (simple ELF) which abstracts all the complexities of payload formats away at buidtime (cbfstool). This is also how other formats are handled (elf, raw bin, FV, ...)
My objection to a new format like cbor was that it is likely very hard to parse using the same trampoline scheme. It is likely possible to write a trampoline using a stack in C, but then again that just complicates things a lot needlessly just to adopt a new format with probably little to gain.
* The coreboot project can, however, encapsulate a CBOR-based
handoff-structure into cbmem, similar to what we currently do with ACPI tables.
I think this is about supporting both a CBOR-based handoff and coreboot tables at the same time. My concerns here are that is requires some synchronization between both codepaths and just increases maintenance in general. Introducing multiple codepaths to do roughly the same is an error we get bit by way too often. I think we should be careful about this...
Additionally Intel was willing to look at using CBOR structures as
input and output to the FSP, so we could get rid of both the UPDs & HOBs.
This seems like the real positive upshot of that conversation!
Kind regards Arthur
On Tue, Mar 29, 2022 at 9:03 PM coreboot org coreboot.org@gmail.com wrote:
# 2022-03-29 - coreboot UEFI working group Next meeting: 2022-04-26
## Attendees: Jay, Werner, Martin, Sheng, Ron, David
## Minutes:
In the coreboot meeting last week, we discussed a proposal from Intel to change the handoff mechanism to payloads [1]. After that discussion, the decision was made to move further discussion of that topic over to the UEFI working group meeting. There was another meeting on Monday, March 28th where we got a much better understanding of what’s going on and the reasons behind the proposal.
- Intel wants to look at modifying the payload handoff as a part of their universal-scalable-firmware [2] initiative.
The current payload handoff method has a number of flaws that they’d like to fix, such as the address for stack being hardcoded.
This likely also gives coreboot an opportunity to address other parts of the USF spec that cause issues or incompatibilities for us.
- The coreboot project got involved when Sheng suggested to Intel that it would be good to align the payload-handoff format with other projects such as coreboot.
Thanks Sheng!
- Using CBOR is the current proposal, but Intel was open to discussing different formats than CBOR if anyone has a better suggestion.
If anyone is not familiar with CBOR, you can get more information at cbor.io [3].
- In the coreboot meeting, it was suggested that we push to just use coreboot tables as they’re already supported in a number of payloads. This really isn’t practical however. Intel would need to be able to modify the list of known cbmem IDs and they would need to be able to change the description format to something that’s more self-describing.
- The coreboot project can, however, encapsulate a CBOR-based handoff-structure into cbmem, similar to what we currently do with ACPI tables.
- Additionally Intel was willing to look at using CBOR structures as input and output to the FSP, so we could get rid of both the UPDs & HOBs.
- The coreboot community can help to drive this, and Intel has said that they will send an email to the coreboot mailing list.
- Since this is a part of the Universal-Scalable-Firmware initiative, it would be good to look that over for issues as well. Currently, it’s very Intel oriented, and specifies that the FSP will be used as the silicon initialization layer, which obviously doesn’t work for non-x86 platforms.
- The details for the weekly meeting are below.
Self Describing Blob discussion.
A copy of this meeting has been added to the coreboot calendar [4].
Biweekly on Monday, 4:00 – 5:00pm CET/CEST Berlin (Currently UTC+2) Video call link: https://meet.google.com/gha-shza-wnw Or dial: (DE) +49 30 300195187 PIN: 848 847 031# More phone numbers: https://tel.meet/gha-shza-wnw?pin=2380921555764 Self Describing Blob discussion Meeting minutes [5].Sean Rhodes from StarLabs has worked to get all of the coreboot related EDK2 patches submitted to their repo. These are still currently in review.
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Hi,
Arthur Heymans wrote:
I would like to add a few notes to the meeting notes to clarify things a bit better.
Thank you for that.
So the only thing not 'practical' here is that UEFI teams don't have control over the handoff structure format that is inside coreboot and is used by coreboot payloads (coreboot tables).
Right. The spec (overview graphic) makes it clear that USF is an embrace-and-extend type of effort to supercede existing projects and de-facto standards in the space.
The current payload handoff method has a number of flaws that they’d like to fix, such as the address for stack being hardcoded.
Normally payloads set up their own stack very early on so this is not a problem.
It has always been a primary design goal for coreboot to leave no trace when the payload takes over. Payloads have total control of the system and need not look back. Anything that violates this principle goes against the primary design goal of coreboot to not stay resident.
This primary design goal was very much on purpose.
The context here, was that I voiced some practical concerns about using CBOR as a handoff structure. LinuxBIOS or coreboot tables were carefully designed to be very easy to parse.
Your concern is valid and I think a key point. CBOR may not be bad over a socket, but such a complex and arbitrarily extensible format is much too error prone to be a good technical choice during boot.
The same properties that make it technically unsuitable can of course make it a perfect choice politically, for someone.
My objection to a new format like cbor was that it is likely very hard to parse using the same trampoline scheme. It is likely possible to write a trampoline using a stack in C, but then again that just complicates things a lot needlessly just to adopt a new format with probably little to gain.
I see zero gain for coreboot.
The gain is political for someone outside of coreboot; using a free form and extensible data structure instead of coreboot tables moves handover standardization out of the coreboot project and enables arbitrary extension at will by someone not coreboot.
I believe that would be a net loss for the firmware community at large.
- The coreboot project can, however, encapsulate a CBOR-based
handoff-structure into cbmem, similar to what we currently do with ACPI tables.
I think this is about supporting both a CBOR-based handoff and coreboot tables at the same time. My concerns here are that is requires some synchronization between both codepaths and just increases maintenance in general. Introducing multiple codepaths to do roughly the same is an error we get bit by way too often. I think we should be careful about this...
Yes! Double trouble would be silly. If someone wants to use CBOR then why not just create a payload for that, rather than trying to mess up coreboot itself?
Additionally Intel was willing to look at using CBOR structures as input and output to the FSP, so we could get rid of both the UPDs & HOBs.
This seems like the real positive upshot of that conversation!
I agree that it could be a step forward, but I think the devil is in the details. CBOR data structures can also be unneccessarily complex and error prone, beyond the parser itself.
Kind regards
//Peter
Apr 1, 2022, 05:43 by peter@stuge.se:
Arthur Heymans wrote:
The context here, was that I voiced some practical concerns about
using CBOR as a handoff structure. LinuxBIOS or coreboot tables were carefully designed to be very easy to parse.
Your concern is valid and I think a key point. CBOR may not be bad over a socket, but such a complex and arbitrarily extensible format is much too error prone to be a good technical choice during boot.
The same properties that make it technically unsuitable can of course make it a perfect choice politically, for someone.
So if the idea is to create a payload handoff format that can be shared and used by multiple different firmware packages, do you have a better option? Yes, coreboot can just continue with just the coreboot tables, but that seems a little like sticking our head in the sand and refusing to recognize that other boot firmware exists.
If there is going to be a new, "universal" handoff method, my thought is that it's better for us to participate in that and try to influence it to be as usable as possible. If we have a chance to replace HOBs with something better, what's the best we can get? If CBOR isn't better, what is?
My objection to a new format like cbor was that it is likely very hard to parse using the same trampoline scheme. It is likely possible to write a trampoline using a stack in C, but then again that just complicates things a lot needlessly just to adopt a new format with probably little to gain.
I see zero gain for coreboot.
The gain is political for someone outside of coreboot; using a free form and extensible data structure instead of coreboot tables moves handover standardization out of the coreboot project and enables arbitrary extension at will by someone not coreboot.
I believe that would be a net loss for the firmware community at large.
Is there any suggestion for something better? We know that if Intel / UEFI want to update this, it's going to happen whether we want it or not. So how do we improve it?
- The coreboot project can, however, encapsulate a CBOR-based
handoff-structure into cbmem, similar to what we currently do with ACPI tables.
I think this is about supporting both a CBOR-based handoff and coreboot tables at the same time. My concerns here are that is requires some synchronization between both codepaths and just increases maintenance in general. Introducing multiple codepaths to do roughly the same is an error we get bit by way too often. I think we should be careful about this...
Yes! Double trouble would be silly. If someone wants to use CBOR then why not just create a payload for that, rather than trying to mess up coreboot itself?
Sure, we can serialize this, make a translation layer for the cbtables to CBOR. It can be a coreboot option, or an intermediate payload. We decided that we didn't want to put all of the HOB creation code into coreboot itself, and we can do the same for whatever the next method is. If the FSP is going to use CBOR instead of UPDs, we'll already have the generation code in coreboot though, which isn't the case for the HOBs. We can currently read some HOBs, but to my knowledge, we don't generate any.
Additionally Intel was willing to look at using CBOR structures as input and output to the FSP, so we could get rid of both the UPDs & HOBs.
This seems like the real positive upshot of that conversation!
I agree that it could be a step forward, but I think the devil is in the details. CBOR data structures can also be unneccessarily complex and error prone, beyond the parser itself.
So maybe we try to limit the complexity? I'm not really familar with CBOR, so I don't know the issues with it. Intel did say that they were willing to look at other alternatives if we had any. If anyone has thoughts on alternatives, please suggest them.
I hope nobody takes any of this as criticism - I appreciate the open discussion, and am sincerely looking for the best path forward here.
Thanks, and take care. Martin
Martin Roth via coreboot wrote:
Your concern is valid and I think a key point. CBOR may not be bad over a socket, but such a complex and arbitrarily extensible format is much too error prone to be a good technical choice during boot.
So if the idea is to create a payload handoff format that can be shared and used by multiple different firmware packages, do you have a better option? Yes, coreboot can just continue with just the coreboot tables, but that seems a little like sticking our head in the sand and refusing to recognize that other boot firmware exists.
I'd ask what other boot firmware is missing from coreboot tables for them to be universally acceptable.
I agree that it could be a step forward, but I think the devil is in the details. CBOR data structures can also be unneccessarily complex and error prone, beyond the parser itself.
So maybe we try to limit the complexity? I'm not really familar with CBOR, so I don't know the issues with it.
CBOR (RFC 8949) is a binary serialization of JSON with some extensions.
So "CBOR" itself says nothing about the data within.
Intel did say that they were willing to look at other alternatives if we had any.
That's a positive signal!
I propose that coreboot tables are a good alternative - fight me! :)
I hope nobody takes any of this as criticism - I appreciate the open discussion, and am sincerely looking for the best path forward here.
Not at all.
Let's see if coreboot tables can grow to cover all needs?
Kind regards
//Peter
peter, you are right about CBOR, and that says to me it does not really meet the original goal of self-describing data. But coreboot tables, at least in my understanding, is also not self-describing.
Do you have some thoughts on a good format that is self-describing?
On Mon, Apr 11, 2022 at 3:05 PM Peter Stuge peter@stuge.se wrote:
Martin Roth via coreboot wrote:
Your concern is valid and I think a key point. CBOR may not be bad over a socket, but such a complex and arbitrarily extensible format is much too error prone to be a good technical choice during boot.
So if the idea is to create a payload handoff format that can be shared and used by multiple different firmware packages, do you have a better option? Yes, coreboot can just continue with just the coreboot tables, but that seems a little like sticking our head in the sand and refusing to recognize that other boot firmware exists.
I'd ask what other boot firmware is missing from coreboot tables for them to be universally acceptable.
I agree that it could be a step forward, but I think the devil is in the details. CBOR data structures can also be unneccessarily complex and error prone, beyond the parser itself.
So maybe we try to limit the complexity? I'm not really familar with CBOR, so I don't know the issues with it.
CBOR (RFC 8949) is a binary serialization of JSON with some extensions.
So "CBOR" itself says nothing about the data within.
Intel did say that they were willing to look at other alternatives if we had any.
That's a positive signal!
I propose that coreboot tables are a good alternative - fight me! :)
I hope nobody takes any of this as criticism - I appreciate the open discussion, and am sincerely looking for the best path forward here.
Not at all.
Let's see if coreboot tables can grow to cover all needs?
Kind regards
//Peter _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
ron minnich wrote:
peter, you are right about CBOR, and that says to me it does not really meet the original goal of self-describing data.
Hm, whose goal is that?
Anyway, using some data structure serialized in CBOR requires defining the structure somewhere. Using coreboot tables requires definitions too, they are currently defined in coreboot, standardizing coreboot tables would probably see them move to a repo of their own.
But coreboot tables, at least in my understanding, is also not self-describing.
I don't know? What do you mean by self-describing actually?
Do you have some thoughts on a good format that is self-describing?
So what's the expectation there; what does a self-describing format enable or need to enable? And what's the complexity tradeoff involved?
As Arthur pointed out, coreboot tables have the quite significant advantage of being very very simple to read and write.
I think this is still interesting to pursue:
So if the idea is to create a payload handoff format that can be shared and used by multiple different firmware packages, do you have a better option?
I'd ask what other boot firmware is missing from coreboot tables for them to be universally acceptable.
Martin wrote that the goal is to create a handoff format that can be shared and I'm asking what coreboot tables are missing to serve others, because I think we have a really good (simple) technical solution there.
//Peter
My goal was pretty simple: Kill the UEFI HOBs, and the FSP UPD, and put something better in their place. coreboot tables could easily replace HOBs, save that intel will never accept that; but I don't see coreboot tables replacing UPD.
[one might argue that what Intel will accept matters a lot less than it did 5 years ago, and I would agree. So maybe we can worry less about what Intel will accept, but still ... :-) ]
I like self describing data as it avoids that mess that we are in with UPD today, where you can end up with problems if the compilers you use for FSP and (e.g.) coreboot don't agree totally on how to lay out data structures. UPD are also a major pain for non-C firmware, such as oreboot. So I'd like a data format that is not defined by a compiler or language. But maybe I'm the only person who wants that :-)
On Tue, Apr 12, 2022 at 11:45 AM Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
peter, you are right about CBOR, and that says to me it does not really meet the original goal of self-describing data.
Hm, whose goal is that?
Anyway, using some data structure serialized in CBOR requires defining the structure somewhere. Using coreboot tables requires definitions too, they are currently defined in coreboot, standardizing coreboot tables would probably see them move to a repo of their own.
But coreboot tables, at least in my understanding, is also not self-describing.
I don't know? What do you mean by self-describing actually?
Do you have some thoughts on a good format that is self-describing?
So what's the expectation there; what does a self-describing format enable or need to enable? And what's the complexity tradeoff involved?
As Arthur pointed out, coreboot tables have the quite significant advantage of being very very simple to read and write.
I think this is still interesting to pursue:
So if the idea is to create a payload handoff format that can be shared and used by multiple different firmware packages, do you have a better option?
I'd ask what other boot firmware is missing from coreboot tables for them to be universally acceptable.
Martin wrote that the goal is to create a handoff format that can be shared and I'm asking what coreboot tables are missing to serve others, because I think we have a really good (simple) technical solution there.
//Peter _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
ron minnich wrote:
My goal was pretty simple: Kill the UEFI HOBs, and the FSP UPD, and put something better in their place. coreboot tables could easily replace HOBs, save that intel will never accept that;
Thanks, now I understand.
but I don't see coreboot tables replacing UPD.
Why couldn't they? (Politics aside, for now.)
I like self describing data as it avoids that mess that we are in with UPD today, where you can end up with problems if the compilers you use for FSP and (e.g.) coreboot don't agree totally on how to lay out data structures.
Understand! Not even having a well-defined serialization is a disaster. CBOR is a solution to that but I think it has way too many primitives.
UPD are also a major pain for non-C firmware, such as oreboot.
Again because of undefined alignment, or something else?
So I'd like a data format that is not defined by a compiler or language. But maybe I'm the only person who wants that :-)
I completely agree that well-defined serialization is neccessary for interoperability, it's pretty amazing that Intel overlooked that. I wonder what else..
You asked about other serializations, the one I was last pleasantly surprised by was the Kafka protocol, which isn't as arduous as one might think based on the name. :) Like coreboot tables it does not describe structure, only values, so reader and writer must agree on what they transfer out-of-band based on a shared definition, but certainly not based on any compiler properties.
It is however a network and disk serialization - far simpler than CBOR but maybe still unneccessarily complex or at least somewhat unfitting for firmware. It mostly has signed values and later versions (there's a version in a header) add varints to save bytes, because the protocol is made for very high message volume - so not really applicable to firmware.
How about using cbtable primitives to express UPD and calling it something new?
//Peter
Dne 12. 04. 22 v 0:04 Peter Stuge napsal(a):
I propose that coreboot tables are a good alternative - fight me! :)
Challenge accepted. They aren't because they are defined with ABI/compiler:
- 64-bit data type alignment is different in 32-bit ABI (4 bytes) and different in 64-bit ABI (8 bytes). Not even sure how the other ABIs got this.
- CB structures like framebuffer struct are not padded to the size compiler likes.
The "packed" attribute would be bit dangerous because then compiler might complain like "error: taking address of packed member of 'struct cb_framebuffer' may result in an unaligned pointer value [-Werror=address-of-packed-member]"
Alternatively one could add "pad[2]" member to the end to the fb structure to fix this, but maybe this will break some payloads which don't check the ->size of specific tag...
Anyway, hope this illustrates the problem a bit.
Thanks, Rudolf
Challenge accepted. They aren't because they are defined with ABI/compiler:
- 64-bit data type alignment is different in 32-bit ABI (4 bytes) and
different in 64-bit ABI (8 bytes). Not even sure how the other ABIs got this.
- CB structures like framebuffer struct are not padded to the size
compiler likes.
The "packed" attribute would be bit dangerous because then compiler might complain like "error: taking address of packed member of 'struct cb_framebuffer' may result in an unaligned pointer value [-Werror=address-of-packed-member]"
Alternatively one could add "pad[2]" member to the end to the fb structure to fix this, but maybe this will break some payloads which don't check the ->size of specific tag...
Anyway, hope this illustrates the problem a bit.
Nice catch! Regardless of the upshot of this it's worth fixing this problem in the coreboot tables implementation. I'm not very knowledgeable on the topic but don't a lot of CPU ARCH support unaligned pointer access in hardware but it slows things down? The way you find coreboot table structs is by looping over ->size, since payloads may not know some tags. So aligning the size up to 8 bytes when generating the coreboot tables should do the trick.
Kind regards Arthur
On Tue, Apr 19, 2022 at 10:44 AM Rudolf Marek r.marek@assembler.cz wrote:
Dne 12. 04. 22 v 0:04 Peter Stuge napsal(a):
I propose that coreboot tables are a good alternative - fight me! :)
Challenge accepted. They aren't because they are defined with ABI/compiler:
- 64-bit data type alignment is different in 32-bit ABI (4 bytes) and
different in 64-bit ABI (8 bytes). Not even sure how the other ABIs got this.
- CB structures like framebuffer struct are not padded to the size
compiler likes.
The "packed" attribute would be bit dangerous because then compiler might complain like "error: taking address of packed member of 'struct cb_framebuffer' may result in an unaligned pointer value [-Werror=address-of-packed-member]"
Alternatively one could add "pad[2]" member to the end to the fb structure to fix this, but maybe this will break some payloads which don't check the ->size of specific tag...
Anyway, hope this illustrates the problem a bit.
Thanks, Rudolf
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Hi,
On 19. 04. 22 11:42, Arthur Heymans wrote:
Nice catch! Regardless of the upshot of this it's worth fixing this problem in the coreboot tables implementation. I'm not very knowledgeable on the topic but don't a lot of CPU ARCH support unaligned pointer access in hardware but it slows things down?
Yes a bit. But mostly for SIMD instructions.
The way you find coreboot table structs is by looping over ->size, since payloads may not know some tags. So aligning the size up to 8 bytes when generating the coreboot tables should do the trick.
Yes and no. Problem is if you have following layout in your struct:
u32 x u64 y
On 64-bit you will get "space" in the middle of your struct.
u32 x u32 hidden_padding u64 y (aligned to 8 byte boundary)
and on 32-bit you will get:
u32 x u64 y (aligned to 4 byte boundary)
And similar thing happens at the end of the struct where extra padding is inserted, but the size depends on the actual ABI/Architecture. Linux uses other ABI than UEFI for example. You can play with that using gcc. Just add -Wpadded to your compiler flags.
The problem above exactly happens with coreboot memory entries (when there is more then one). See libpayload/include/coreboot_tables.h which introduces for this reason struct cbuint64 which is 64-bit datatype split to 2 32-bit parts to fix this oversight.
Have a look what multiboot2 folks did. In general multiboot2 [1] got this "right". It aligns the start of the entries and I think there are no such issues. Also it defines the machine state at the point of handoff which is nice. Also, it has some infrastructure to pass various strange future memory lists in the memory tag.
Thanks, Rudolf
[1] https://www.gnu.org/software/grub/manual/multiboot2/multiboot.html
Kind regards Arthur
On Tue, Apr 19, 2022 at 10:44 AM Rudolf Marek <r.marek@assembler.cz mailto:r.marek@assembler.cz> wrote:
Dne 12. 04. 22 v 0:04 Peter Stuge napsal(a): > I propose that coreboot tables are a good alternative - fight me! :) Challenge accepted. They aren't because they are defined with ABI/compiler: - 64-bit data type alignment is different in 32-bit ABI (4 bytes) and different in 64-bit ABI (8 bytes). Not even sure how the other ABIs got this. - CB structures like framebuffer struct are not padded to the size compiler likes. The "packed" attribute would be bit dangerous because then compiler might complain like "error: taking address of packed member of 'struct cb_framebuffer' may result in an unaligned pointer value [-Werror=address-of-packed-member]" Alternatively one could add "pad[2]" member to the end to the fb structure to fix this, but maybe this will break some payloads which don't check the ->size of specific tag... Anyway, hope this illustrates the problem a bit. Thanks, Rudolf _______________________________________________ coreboot mailing list -- coreboot@coreboot.org <mailto:coreboot@coreboot.org> To unsubscribe send an email to coreboot-leave@coreboot.org <mailto:coreboot-leave@coreboot.org>
Arthur, your proposal would actually make things worse, surprisingly.
While your proposal would fix a problem, it would change the binary layout, and create a problem.
Consider the case of a 10y old coreboot, with a modern kernel (Linux) booting from it. How does linux parse the structures? They're going to be different, as you fixed the problem Rudolf describes. The kernel needs two parsers: which one should it use?
Well, you can put a version tag in -- see the MP table. That generally works poorly -- in the end, there was only ever one version of the MP table, because ... what do you do if you have an old kernel and a newer MP table, compiled in such a way that the layout has somehow changed? oops.
Well, ok, maybe you can find some way to make it work for, say, 15 years of gcc, coreboot, and linux.
Now I want to boot plan 9. Plan 9 rules for struct layout mainly match gcc, but not quite. In particular, Plan 9, having been written by the inventors of C, was averse to packed. In fact, they did not use the packed keyword: in Plan 9 c, you got packed with #pragma hjdicks And, yes, this word was an expression of the opinion of the inventors of the C language on the idea of packed structures, Humorous back and forth from years ago: https://comp.os.plan9.narkive.com/P06B6nkZ/9fans-am-i-nuts-does-8c-support-p..., which I found by accident just now.
I don't think we want to version lock coreboot to particular versions of linux :-)
Rudolf's point is crucial: "Challenge accepted. They aren't [self defining] because they are defined with ABI/compiler:"
As Rudolf points out, we are defining a binary layout with a c compiler. That's known not to work. It's why things like xdr compilers (and protobufs) exist.
You don't have to a use an XDR compiler. If you look in the linux kernel, you'll see stuff like this: req = p9_client_rpc(clnt, P9_TLOCK, "dbdqqds", fid->fid, flock->type, What is that thing in quotes? It's the data layout of the packet: dword, byte, dword, quad, string, etc. s in this case means 2 bytes of length, then bytes.
I'm not saying we can use this, but if you use this string to generate an array of uint8_t, then you package the string with the array, you now have a self-describing structure, I believe.
Again, what we have today is not self-describing, not portable to non-gcc toolchains or other kernels, and not portable even across kernel and compiler versions (gcc 2 to gcc 3 exposed this kind of thing, years ago).
Further, because coreboot depends on gcc features, kernels like Plan 9 will not compile those structs the same way.
coreboot tables are nice, but like it or not (and I was part of the problem, I was there for the creation), they have a gcc and x86 bias -- I've seen this in Plan 9.
We can do better. And as the kernel source I referenced shows, it doesn't have to be super complex.
On Tue, Apr 19, 2022 at 11:04 PM Rudolf Marek r.marek@assembler.cz wrote:
Hi,
On 19. 04. 22 11:42, Arthur Heymans wrote:
Nice catch! Regardless of the upshot of this it's worth fixing this problem in the coreboot tables implementation. I'm not very knowledgeable on the topic but don't a lot of CPU ARCH support unaligned pointer access in hardware but it slows things down?
Yes a bit. But mostly for SIMD instructions.
The way you find coreboot table structs is by looping over ->size, since payloads may not know some tags. So aligning the size up to 8 bytes when generating the coreboot tables should do the trick.
Yes and no. Problem is if you have following layout in your struct:
u32 x u64 y
On 64-bit you will get "space" in the middle of your struct.
u32 x u32 hidden_padding u64 y (aligned to 8 byte boundary)
and on 32-bit you will get:
u32 x u64 y (aligned to 4 byte boundary)
And similar thing happens at the end of the struct where extra padding is inserted, but the size depends on the actual ABI/Architecture. Linux uses other ABI than UEFI for example. You can play with that using gcc. Just add -Wpadded to your compiler flags.
The problem above exactly happens with coreboot memory entries (when there is more then one). See libpayload/include/coreboot_tables.h which introduces for this reason struct cbuint64 which is 64-bit datatype split to 2 32-bit parts to fix this oversight.
Have a look what multiboot2 folks did. In general multiboot2 [1] got this "right". It aligns the start of the entries and I think there are no such issues. Also it defines the machine state at the point of handoff which is nice. Also, it has some infrastructure to pass various strange future memory lists in the memory tag.
Thanks, Rudolf
[1] https://www.gnu.org/software/grub/manual/multiboot2/multiboot.html
Kind regards Arthur
On Tue, Apr 19, 2022 at 10:44 AM Rudolf Marek <r.marek@assembler.cz mailto:r.marek@assembler.cz> wrote:
Dne 12. 04. 22 v 0:04 Peter Stuge napsal(a): > I propose that coreboot tables are a good alternative - fight me! :) Challenge accepted. They aren't because they are defined with ABI/compiler: - 64-bit data type alignment is different in 32-bit ABI (4 bytes) and different in 64-bit ABI (8 bytes). Not even sure how the other ABIs got this. - CB structures like framebuffer struct are not padded to the size compiler likes. The "packed" attribute would be bit dangerous because then compiler might complain like "error: taking address of packed member of 'struct cb_framebuffer' may result in an unaligned pointer value [-Werror=address-of-packed-member]" Alternatively one could add "pad[2]" member to the end to the fb structure to fix this, but maybe this will break some payloads which don't check the ->size of specific tag... Anyway, hope this illustrates the problem a bit. Thanks, Rudolf _______________________________________________ coreboot mailing list -- coreboot@coreboot.org <mailto:coreboot@coreboot.org> To unsubscribe send an email to coreboot-leave@coreboot.org <mailto:coreboot-leave@coreboot.org>
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Rudolf, thanks a lot for challenging me and clarifying the problem!
ron minnich wrote:
Rudolf's point is crucial: "Challenge accepted. They aren't [self defining] because they are defined with ABI/compiler:"
As Rudolf points out, we are defining a binary layout with a c compiler. That's known not to work.
Ron, I now understand what you mean by self-describing.
But I think we should focus only on the lack of explicit serialization.
I don't think self-describing is important; it would in fact be redundant and thus a source of possibly contradictory information which I absolutely do not want at boot. And it would make parsing a lot more complex without real benefit.
I think the key requirement is that we must use a well-defined, explicit serialization, even if simply "never any padding" aka "packed".
When considering tables in memory not as structs but using Ron's framing "a packet" transfered to somewhere else then serializing the fields without padding is not unreasonable at all!
I'm not saying we can use this, but if you use this string to generate an array of uint8_t, then you package the string with the array, you now have a self-describing structure, I believe.
The current cbtable tag-and-size system parsing a lot easier and quicker than a pack pattern would, we should not give that up.
I however agree completely that we should define serialization!
But I see no reason to introduce a pack pattern.
The tag already implies which specific members are contained and we can add tags when needed.
Again, what we have today is not self-describing,
Not a problem.
not portable to non-gcc toolchains or other kernels, and not portable even across kernel and compiler versions
This is the problem, and we should fix it for sure!
Further, because coreboot depends on gcc features,
As an aside, I disagree with Julius on this; even if we do so in some places we shouldn't do so frivolously. But let's stay on topic.
Consider the case of a 10y old coreboot, with a modern kernel (Linux) booting from it. How does linux parse the structures?
Existing tags will have to always mean "GCC ABI" alignment, but we can invest some time into explicitly defining those alignments that have been output by GCC up til now and explicitly serialize to that in order to ensure forward compatibility with old kernels.
Then there are at least never any further variations, and being explicit is always better than letting the compiler pick.
More importantly though, once we have defined a "coreboot ABI" serialization we should add new tags which only ever use that, while continuing to output also GCC ABI tags, until they demonstrably break all possible consumers, which I expect will be never.
Modern Linux then learns to prefer coreboot ABI tags with well-defined serialization and could output warnings about GCC-ABI tags and optionally try its best to parse them as it always has.
We can't magically fix 10y old coreboot from within Linux, that machine then deserves a firmware update, but at least we can fix this while maintaining both backwards and forward compatibility.
I propose that coreboot ABI serializes table members without padding.
What issues do you see there? x86 can access single bytes anywhere (really only relevant until RAM is available, right?) but would we have a problem on other architechtures?
(This btw also seems like a good example of a change that could and should benefit every single board that we've ever had code for but could require duplicated engineering to apply on diverged branches.)
Kind regards
//Peter
-
Arthur Heymans -
coreboot org -
Martin Roth -
Peter Stuge -
ron minnich -
Rudolf Marek