Julius Werner 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.
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.
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!). 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), 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).
Or, on the other hand, we could just keep going with what has served us well for a long time and invest our energy into more important things because *we have a perfectly fine solution to hide this in the platform code*! Furquan and I discussed this in b:153593670 (sorry, non-Googlers) and I haven't seen anyone raising any real concern about it yet. Basically, you just make the platform implement a special boot_device_ro() which makes the rdev_mmap() work like on non-memory-mapped flash devices (copying data into SRAM, where the platform does support unaligned accesses, rather than returning a pointer to memory-mapped flash directly), and all of these issues in any file should go away.
Can we *please* just fix the platform rather than trying to change the world again (in a way that would likely never get fully done and just leave things in a mess)?