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.

Maybe I was thinking ahead too fast. Once we'd have defined the endian-
ness, packed structs wouldn't work anymore. Even if we'd simply define
"host endiannes" it would still break in the utils.

These problems may be orthogonal, but the solution is the same.

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...

You're the one who was asking for a simple rule. I'm ok with mixing
things. In some contained area like chip-specific code I see no trouble
at all. But if you make it a one-or-the-other choice for the whole tree,
I'm in favor of the model that always works and not the one that happens
to work good enough for most of us.

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,

That's neither what I meant nor what I wrote. I was just asking whom you
were referring to because it obviously doesn't serve everybody well.

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.

Ok, I'm done here. It seems I'm incapable to get the message into your
head. I don't see how one thing work and the other doesn't is subjective.

View Change

To view, visit change 40243. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ac83766616cb2107b75eba6995cd09b4fc90968
Gerrit-Change-Number: 40243
Gerrit-PatchSet: 2
Gerrit-Owner: Simon Glass <sjg@chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner@chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth@google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com>
Gerrit-Reviewer: Raul Rangel <rrangel@chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-CC: Aaron Durbin <adurbin@chromium.org>
Gerrit-CC: Furquan Shaikh <furquan@google.com>
Gerrit-CC: Nico Huber <nico.h@gmx.de>
Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net>
Gerrit-CC: Simon Glass <sjg@chromium.org>
Gerrit-Comment-Date: Sun, 21 Jun 2020 12:54:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment