Change in coreboot[master]: fmap: Avoid unaligned access

[Simon Glass (Code Review)]

Simon Glass has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

fmap: Avoid unaligned access

The fmap header has unaligned fields in it. Some CPUs do not like this,
and it is not very friendly to hide this sort of thing in innocuous code.

Fix it by copying the data into an aligned location first.

BUG=b:153203967
TEST=See this output now:
Getting SPI ROM info.
FMAP: Found "FLASH" version 1.1 at 0xb04000.
FMAP: base = 0xff000000 size = 0x1000000 #areas = 29

Signed-off-by: Simon Glass 
Change-Id: I6ac83766616cb2107b75eba6995cd09b4fc90968
---
M src/lib/fmap.c
1 file changed, 12 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/40243/1

diff --git a/src/lib/fmap.c b/src/lib/fmap.c
index ecd23f6..4a67053 100644
--- a/src/lib/fmap.c
+++ b/src/lib/fmap.c
@@ -35,10 +35,15 @@

static void report(const struct fmap *fmap)
 {
+	uint64_t base;
+	uint32_t size;
+
    print_once(BIOS_DEBUG, "FMAP: Found "%s" version %d.%d at %#x.\n",
           fmap->name, fmap->ver_major, fmap->ver_minor, FMAP_OFFSET);
+	memcpy(&base, &fmap->base, sizeof(fmap->base));
+	memcpy(&size, &fmap->size, sizeof(fmap->size));
    print_once(BIOS_DEBUG, "FMAP: base = %#llx size = %#x #areas = %d\n",
-	       (long long)fmap->base, fmap->size, fmap->nareas);
+		   base, size, fmap->nareas);
    fmap_print_once = 1;
 }

@@ -155,6 +160,7 @@

while (1) {
    	struct fmap_area *area;
+		uint32_t area_offset, area_size;

area = rdev_mmap(&fmrd, offset, sizeof(*area));

@@ -166,12 +172,14 @@
    		offset += sizeof(struct fmap_area);
    		continue;
    	}
+		memcpy(&area_offset, &area->offset, sizeof(area->offset));
+		memcpy(&area_size, &area->size, sizeof(area->size));

printk(BIOS_DEBUG, "FMAP: area %s found @ %x (%d bytes)\n",
-		       name, area->offset, area->size);
+		       name, area_offset, area_size);

-		ar->offset = area->offset;
-		ar->size = area->size;
+		ar->offset = area_offset;
+		ar->size = area_size;

rdev_munmap(&fmrd, area);

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

Patch Set 1: Code-Review-1

The general philosophy is that unaligned accesses need to be supported in common 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. All supported architectures have a way to allow unaligned accesses (although some of them require some platform-specific setup in early init code first).

The Zork PSP is an Arm platform (right)? In that case, you should enable paging and caching to get it to support unaligned accesses. You generally just need to run something like

mmu_init();
 mmu_config_range(0, 4096, DCACHE_OFF);
 mmu_config_range((uintptr_t)_sram / KiB, REGION_SIZE(sram) / KiB, DCACHE_WRITETHROUGH);
 dcache_mmu_enable();

in your bootblock_soc_init(). (Make sure to set the SRAM_START and SRAM_END symbols in your memlayout.ld and provide a 16KB TTB() region.)

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

Patch Set 1:

Verstage is running in the PSP as a userspace app.  We can request that AMD set the caching up for us, but it's not something we can do directly.

[Martin Roth (Code Review)]

Martin Roth has uploaded a new patch set (#2) to the change originally created by Simon Glass. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

fmap: Avoid unaligned access

The fmap header has unaligned fields in it. Some CPUs do not like this,
and it is not very friendly to hide this sort of thing in innocuous code.

Fix it by copying the data into an aligned location first only for
vboot_before_bootblock. Because verstage runs as a userspace
application, we can't set up caching as would be the typical way
to handle this.

BUG=b:153203967
TEST=See this output now in psp_verstage
Getting SPI ROM info.
FMAP: base = 0xff000000 size = 0x1000000 #areas = 29

Signed-off-by: Martin Roth 
Signed-off-by: Simon Glass 
Change-Id: I6ac83766616cb2107b75eba6995cd09b4fc90968
---
M src/lib/fmap.c
1 file changed, 21 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/40243/2

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

Patch Set 2: Code-Review-1

What's the status of this CL? I still think that this is the wrong way to go about this problem. From my understanding of b/153593670, we were still hoping for AMD to change the cache settings for memory-mapped ROM, and if that's impossible we'd prefer to handle this with a platform-specific boot_device rdev. We shouldn't be hacking up common code for it.

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review-1
>
> What's the status of this CL? I still think that this is the wrong way to go about this problem. From my understanding of b/153593670, we were still hoping for AMD to change the cache settings for memory-mapped ROM, and if that's impossible we'd prefer to handle this with a platform-specific boot_device rdev. We shouldn't be hacking up common code for it.

AMD has been very resistant to enabling caching of the spi rom.  I need to get this running at coreboot.org so we can switch over.  I'll come up with a different solution.

[Martin Roth (Code Review)]

Martin Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

Abandoned

Julius doesn't like this solution.

[Nico Huber (Code Review)]

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

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

Patch Set 2:
> 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.
> > 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?

I can take a stab at fixing the fmap stuff if people would like. Let me know.

[Simon Glass (Code Review)]

Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

Patch Set 2:

+1 from me. U-Boot (the main ARM bootloader) avoids unaligned access.

Also I think when creating new data structures it would be worth taking alignment into account. I don't think it is that difficult, generally.

[Julius Werner (Code Review)]

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)?

[Nico Huber (Code Review)]

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.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

Patch Set 2:
> 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.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40243 )

Change subject: fmap: Avoid unaligned access
......................................................................

Patch Set 2:
> > 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.