Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40243 )
Change subject: fmap: Avoid unaligned access ......................................................................
Patch Set 2:
The general philosophy is that unaligned accesses need to be supported in common code.
I think that's a bad philosophy. IMHO, that's not even proper C.
Uhh... it isn't? Note that the fields in question here *are* marked with __attribute__((packed)), so the compiler is well aware that they may be unaligned. It still generates normal armv7 LDR instructions because under normal circumstances, it's perfectly legal to make unaligned accesses with those. The problem here is that this one single platform has a weird situation where the pointers aren't real memory and page tables cannot be set up the way we usually can on armv7 systems and therefore GCCs assumptions are not valid. Do we really want to replace *all* code that accesses any unaligned fields ( off the top of my head that would be FMAP, CBFS, timestamps, USB, probably plenty of other stuff) with doing memcpy()s for every single field access, rather than making a platform-specific fix? We had the same discussion when adding RISC-V, and after initially trying to hotfix small pieces of CBFS code one by one they quickly discovered how huge that task would actually get and turned around to put in an arch-specific fix instead, which I think everyone has been very happy with since.
Yes, I would rather fix the code than continue to work around it. There is more to it than just the alignment btw. Endianness for instance. In case you haven't noticed, about once every one or two years somebody tries to add some Power(PC) platform and gives up because of our shitty code.
There are many places we would have to fix (not just here but also in CBFS and elsewhere) if that wasn't the case.
Why not start here?
...and then, how far would we get? According to my experience I would bet that we would merge this patch here and that's all that ever happens, because this platform (which doesn't not support unaligned accesses in general, it *only* doesn't support it buffers rdev_mmap()ed from the flash) only happens to see the problem in this code. And then we will forever live with a mess where some code does unaligned accesses and some doesn't depending on which platform that requires which weird special restrictions in only this or only that stage happens to link that specific piece in the configurations that people happen to test.
Sorry, when I said "here" I meant the FMAP code as a whole without making a mess of it. Any patch that only adapts parts of the code or even adds another `if` would be wrong ofc.
Without looking at the code, I guess one straight forward way would be to remove the struct definitions, and provide getters for the fields instead. Tedious, yes, but shouldn't be hard.
In a project with as many different platforms as coreboot, we need consistent rules about stuff and stick to them consistently throughout the code. Right now the (de facto) rule is "common code may do unaligned accesses on any buffers it gets its hands on and your platform needs to support that". I think it's a good rule because it keeps things simple and the code clean of weird special accessor macros, and we have always managed to find a solution to support any weird special case (including for this platform, see below!).
Ok, how about this simple rule: "No packed structs."? It's easy to ensure that we only progress forward by adding a lint check that no new definitions are added.
If you want to change that you would have to both rewrite a mountain of code (which I bet wouldn't really happen in the end and leave us with an inconsistent mess, as mentioned above),
There are people in the community that love to rewrite and refactor things. All that would be needed is a little coordination, IMHO.
and you would also make writing new code harder for everyone in practice because they will always have to keep unaligned access issues in mind for any future development (which, let's face it, would in practice mean that people mostly forget about it because most people develop for x86, and the weird other platforms will forever end up chasing the resulting bugs).
Not a problem if we ban packed structs.
Or, on the other hand, we could just keep going with what has served us well for a long time ...
Who's us? It has served you well. I don't see how this applies to the whole community.