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.
I don't think FMAP endianness has ever been defined anywhere (neither in parsers nor in writers). I agree that's worth fixing, but it's a totally orthogonal issue to this one.
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.
My point is that unaligned accesses happen in *way* more code than the FMAP parser. CBFS entries can be misaligned (e.g. 'entry' and 'load' in struct cbfs_stage). Timestamp entries are misaligned. Some USB descriptors are misaligned. Even memcpy() can actually do misaligned accesses on some architectures because it usually only aligns to the destination and not the source pointer. Do you want to rewrite all these things? How would you even figure out all the other places where this applies too? I think this would be a huge effort to change consistently across the repository, and for what? I really don't see the big problem with how it currently works.
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.
I mean, apart from the fact that I'm counting 554 instances of __packed in the source tree and we have plenty of well-established data structures we would want to keep stable that just *have* unaligned fields already...
I'm not saying any of this couldn't be done but I think it would be a large effort, and I really don't see the comparable benefit. I also think a random patch is probably not the best way to discuss this for real (because it is a complete departure from what a lot of our code has been doing for 10+ years, and because there was already a half-hearted attempt to do something like this a few years ago that quickly found out it would be *way* more effort than they bargained for). If people really feel strongly about this it should probably be discussed on the mailing list, and if there's support in the community and someone signs up to really track down and change all affected code and cleanly change this across the whole repository (even those arch-specific assembly functions that may do this implicitly), I'm fine with that. I don't have a stake in unaligned accesses in particular, I just want us to have a consistent programming model across the repo.
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.
I... uhh... what? You make it sound like I'm secretly on the payroll of Big Unalignment or something. I don't think I even wrote any of the code we have doing unaligned accesses, I am just trying to clarify the state our repository is currently in because I don't think people realize the extent of what they're suggesting when they're proposing to just quickly change this. It is my personal assessment that the existing approach has served the coreboot community well, based on that this is only the second time I recall it being a problem for any platform in the last 5+ years and in both cases there was a relatively simple (compared to rewriting all affected code across the repo) way to provide an isolated fix in platform code, and because I think having to handle this explicitly everywhere would put unnecessary extra burden on programmers (one more thing to keep in mind that's hard to notice in testing unless you have *just* the right platform), leading to more bugs. You're welcome to have a different opinion, of course, although I still haven't really understood what you think the big problem with this is other than that you just somehow subjectively don't like it.
To view, visit change 40243. To unsubscribe, or for help writing mail filters, visit settings.