Arthur, your proposal would actually make things worse, surprisingly. While your proposal would fix a problem, it would change the binary layout, and create a problem.
Consider the case of a 10y old coreboot, with a modern kernel (Linux) booting from it. How does linux parse the structures? They're going to be different, as you fixed the problem Rudolf describes. The kernel needs two parsers: which one should it use? Well, you can put a version tag in -- see the MP table. That generally works poorly -- in the end, there was only ever one version of the MP table, because ... what do you do if you have an old kernel and a newer MP table, compiled in such a way that the layout has somehow changed? oops. Well, ok, maybe you can find some way to make it work for, say, 15 years of gcc, coreboot, and linux. Now I want to boot plan 9. Plan 9 rules for struct layout mainly match gcc, but not quite. In particular, Plan 9, having been written by the inventors of C, was averse to packed. In fact, they did not use the packed keyword: in Plan 9 c, you got packed with #pragma hjdicks And, yes, this word was an expression of the opinion of the inventors of the C language on the idea of packed structures, Humorous back and forth from years ago: https://comp.os.plan9.narkive.com/P06B6nkZ/9fans-am-i-nuts-does-8c-support-packed-structs, which I found by accident just now. I don't think we want to version lock coreboot to particular versions of linux :-) Rudolf's point is crucial: "Challenge accepted. They aren't [self defining] because they are defined with ABI/compiler:" As Rudolf points out, we are defining a binary layout with a c compiler. That's known not to work. It's why things like xdr compilers (and protobufs) exist. You don't have to a use an XDR compiler. If you look in the linux kernel, you'll see stuff like this: req = p9_client_rpc(clnt, P9_TLOCK, "dbdqqds", fid->fid, flock->type, What is that thing in quotes? It's the data layout of the packet: dword, byte, dword, quad, string, etc. s in this case means 2 bytes of length, then bytes. I'm not saying we can use this, but if you use this string to generate an array of uint8_t, then you package the string with the array, you now have a self-describing structure, I believe. Again, what we have today is not self-describing, not portable to non-gcc toolchains or other kernels, and not portable even across kernel and compiler versions (gcc 2 to gcc 3 exposed this kind of thing, years ago). Further, because coreboot depends on gcc features, kernels like Plan 9 will not compile those structs the same way. coreboot tables are nice, but like it or not (and I was part of the problem, I was there for the creation), they have a gcc and x86 bias -- I've seen this in Plan 9. We can do better. And as the kernel source I referenced shows, it doesn't have to be super complex. On Tue, Apr 19, 2022 at 11:04 PM Rudolf Marek <r.ma...@assembler.cz> wrote: > > Hi, > > On 19. 04. 22 11:42, Arthur Heymans wrote: > > Nice catch! > > Regardless of the upshot of this it's worth fixing this problem in the > > coreboot tables implementation. > > I'm not very knowledgeable on the topic but don't a lot of CPU ARCH support > > unaligned pointer access in hardware but it slows things down? > > Yes a bit. But mostly for SIMD instructions. > > > The way you find coreboot table structs is by looping over ->size, since > > payloads may not know > > some tags. So aligning the size up to 8 bytes when generating the coreboot > > tables should do the trick. > > Yes and no. Problem is if you have following layout in your struct: > > u32 x > u64 y > > On 64-bit you will get "space" in the middle of your struct. > > u32 x > u32 hidden_padding > u64 y (aligned to 8 byte boundary) > > and on 32-bit you will get: > > u32 x > u64 y (aligned to 4 byte boundary) > > And similar thing happens at the end of the struct where extra padding is > inserted, but the size depends on the actual ABI/Architecture. > Linux uses other ABI than UEFI for example. You can play with that using gcc. > Just add -Wpadded to your > compiler flags. > > The problem above exactly happens with coreboot memory entries (when there is > more then one). See libpayload/include/coreboot_tables.h > which introduces for this reason struct cbuint64 which is 64-bit datatype > split to 2 32-bit parts to fix this oversight. > > Have a look what multiboot2 folks did. In general multiboot2 [1] got this > "right". It aligns the start of the entries and I think there are no such > issues. Also it defines the machine state at the point of handoff which is > nice. Also, it has some infrastructure to pass various strange future memory > lists in the memory tag. > > > Thanks, > Rudolf > > [1] https://www.gnu.org/software/grub/manual/multiboot2/multiboot.html > > > > > Kind regards > > Arthur > > > > > > On Tue, Apr 19, 2022 at 10:44 AM Rudolf Marek <r.ma...@assembler.cz > > <mailto:r.ma...@assembler.cz>> wrote: > > > > Dne 12. 04. 22 v 0:04 Peter Stuge napsal(a): > > > I propose that coreboot tables are a good alternative - fight me! :) > > > > Challenge accepted. They aren't because they are defined with > > ABI/compiler: > > > > - 64-bit data type alignment is different in 32-bit ABI (4 bytes) and > > different > > in 64-bit ABI (8 bytes). Not even sure how the other ABIs got this. > > > > - CB structures like framebuffer struct are not padded to the size > > compiler likes. > > > > The "packed" attribute would be bit dangerous because then compiler > > might > > complain like "error: taking address of packed member of 'struct > > cb_framebuffer' > > may result in an unaligned pointer value > > [-Werror=address-of-packed-member]" > > > > Alternatively one could add "pad[2]" member to the end to the fb > > structure to > > fix this, but maybe this will break some payloads which don't check the > > ->size > > of specific tag... > > > > Anyway, hope this illustrates the problem a bit. > > > > Thanks, > > Rudolf > > > > _______________________________________________ > > coreboot mailing list -- coreboot@coreboot.org > > <mailto:coreboot@coreboot.org> > > To unsubscribe send an email to coreboot-le...@coreboot.org > > <mailto:coreboot-le...@coreboot.org> > > > _______________________________________________ > coreboot mailing list -- coreboot@coreboot.org > To unsubscribe send an email to coreboot-le...@coreboot.org _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
