Sorry I've been out of the loop. Here are Stefan's emails and my responses.
As I stated in one of my previous emails, I think that this confusion of having two places where we parse ELF is enough reason to get rid of elfboot in v3.
On Fri, Feb 15, 2008 at 12:50 PM, Myles Watson mylesgw@gmail.com wrote:
ron minnich wrote:
per stefan's comment, let's see how much space we save. This is bootblock space, which could be significant, but can we see a number?
Elfboot still uses bootblock space if it is disabled in Kconfig? That's definitely something we need to fix. That space is tight.
PAYLOAD_NONE doesn't mean no elfboot. Elfboot is included always so that if someone adds an ELF to the lar later it will still work. My contention is that we should make people preparse the ELF when adding it to the lar, so that v3 doesn't have to understand ELF. That doesn't mean that lar shouldn't. Just the opposite.
... as long as they do not remove functionality that is still used.
Please don't just remove this code. If you don't like to compile it in, create a config option to disable it. (There is such a config option already, so I really don't see the gain)
I didn't see one.
I say: NACK.
Why?
Because with this patch it is no longer possible to unpack a lar.
v3 doesn't unpack a lar. lar unpacks them.
Please don't do stuff like that with levity.
Stefan
ron minnich wrote:
Stefan, one thing I don't understand, why does removing elfboot make it so we can't unpack LAR?
Because the LAR header information is lost, things like the entry point.
You unpack it, repack it, and the segments become ordinary files with their lar headers containing bogus information.
So either we recreate an ELF file on unpack, or we dump the metadata to a MANIFEST file.
Or we forget about lars having the feature to be unpacked. Not sure that its ever needed, except when you want to migrate a payload from one image to another one.
This is a lar problem, not a v3 problem.
Stefan.
I hope this clears up some of the confusion. I'm looking forward to understanding your concerns better. I thought this would be an easy choice.
Thanks, Myles
OK, well elf is out of v3, at this point, so let's put ELF creation into the lar for when we extact ... sound ok?
ron
On Fri, Feb 15, 2008 at 2:09 PM, ron minnich rminnich@gmail.com wrote:
OK, well elf is out of v3, at this point, so let's put ELF creation into the lar for when we extact ... sound ok?
ron
Can you remind me why you'd want to put an ELF file into a lar, then take it out and put it into an ELF again? What about all the sections we throw away when we put an ELF into the lar?
Myles
On Feb 15, 2008 1:37 PM, Myles Watson mylesgw@gmail.com wrote:
Can you remind me why you'd want to put an ELF file into a lar, then take it out and put it into an ELF again? What about all the sections we throw away when we put an ELF into the lar?
When we take a file out of LAR, we lose the L
On Feb 15, 2008 1:37 PM, Myles Watson mylesgw@gmail.com wrote:
Can you remind me why you'd want to put an ELF file into a lar, then take it out and put it into an ELF again? What about all the sections we throw away when we put an ELF into the lar?
Sorry for that last, google's automagic is screwing up sometimes.
When we take a file out of LAR, we lose the LAR header. That header, for payloads, is derived from ELF. So the extract from LAR loses information.
If we had an option to pull a file from LAR, and generate an ELF, we can then put that file in another LAR later. That's my understanding of it.
ron
From: ron minnich [mailto:rminnich@gmail.com]
On Feb 15, 2008 1:37 PM, Myles Watson mylesgw@gmail.com wrote:
Can you remind me why you'd want to put an ELF file into a lar, then take it out and put it into an ELF again? What about all the sections we throw away when we put an ELF into the lar?
When we take a file out of LAR, we lose the LAR header. That header, for payloads, is derived from ELF. So the extract from LAR loses information.
If we had an option to pull a file from LAR, and generate an ELF, we can then put that file in another LAR later. That's my understanding of it.
So the use case is that you have a lar file, but don't have the ELF that you started with originally, and you want to put it into a different lar file? It might be easier to remove the other things in the lar instead, or do a lar copy from one lar to a new lar, which wouldn't lose the entry points.
Sorry to be dense, but it would help me know if it's even possible if I could understand the use case. When we parse the ELF we lose information like section names, that we won't be able to recover.
Myles
On Feb 15, 2008 2:59 PM, Myles Watson mylesgw@gmail.com wrote:
So the use case is that you have a lar file, but don't have the ELF that you started with originally, and you want to put it into a different lar file?
yes. Because you have a board, and a new (e.g.) boot block, which you have been sent via email from a company that is supporting your platform, and can not for whatever reason recreate the payload. You may have even lost the source to it. This happens.
It might be easier to remove the other things in the lar instead, or do a lar copy from one lar to a new lar, which wouldn't lose the entry points.
Sure.
Sorry to be dense, but it would help me know if it's even possible if I could understand the use case. When we parse the ELF we lose information like section names, that we won't be able to recover.
We don't care about ELF section names. With the current setup, one can not mkdir xyz cd xyz lar -x /tmp/file.lar lar -c /tmp/file2.lar *
because the lar -x loses all the lar headers.
This is not a huge concern to me, but the concern has been raised, it is not unrealistic, so it might be worth while to support it :-)
ron
-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Friday, February 15, 2008 4:12 PM To: Myles Watson Cc: Coreboot; Stefan Reinauer Subject: Re: [coreboot] v3 patch rm elfboot
On Feb 15, 2008 2:59 PM, Myles Watson mylesgw@gmail.com wrote:
So the use case is that you have a lar file, but don't have the ELF that
you
started with originally, and you want to put it into a different lar
file?
yes. Because you have a board, and a new (e.g.) boot block, which you have been sent via email from a company that is supporting your platform, and can not for whatever reason recreate the payload. You may have even lost the source to it. This happens.
It might be easier to remove the other things in the lar instead, or do
a
lar copy from one lar to a new lar, which wouldn't lose the entry
points.
Sure.
Given that, I'd be willing to implement "add from lar", so that you can move lar files from one archive into another. It would be something like lar -aL larfile:larpath:newlarpath. I think going back to ELF is problematic because it wouldn't be a true ELF anymore. Would that be enough, Stefan? Anyone else?
Myles
On Fri, Feb 15, 2008 at 6:19 PM, Myles Watson mylesgw@gmail.com wrote:
-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Friday, February 15, 2008 4:12 PM To: Myles Watson Cc: Coreboot; Stefan Reinauer Subject: Re: [coreboot] v3 patch rm elfboot
On Feb 15, 2008 2:59 PM, Myles Watson mylesgw@gmail.com wrote:
So the use case is that you have a lar file, but don't have the ELF
that
you
started with originally, and you want to put it into a different lar
file?
yes. Because you have a board, and a new (e.g.) boot block, which you have been sent via email from a company that is supporting your platform, and can not for whatever reason recreate the payload. You may have even lost the source to it. This happens.
It might be easier to remove the other things in the lar instead, or
do
a
lar copy from one lar to a new lar, which wouldn't lose the entry
points.
Sure.
Given that, I'd be willing to implement "add from lar", so that you can move lar files from one archive into another. It would be something like lar -aL larfile:larpath:newlarpath. I think going back to ELF is problematic because it wouldn't be a true ELF anymore. Would that be enough, Stefan? Anyone else?
Myles
That sounds like the best plan to me. I think we can fairly safely assume a couple things: 1) Whoever rolled the lar probably still has either the elf or the sources from it kicking around. 2) Anyone extracting it is probably just looking to use it in another lar.
What we lose is the ability for someone to take a payload from a v3-based system to a previous version, but do/will we care?
-Corey
On 16.02.2008 00:19, Myles Watson wrote:
-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Friday, February 15, 2008 4:12 PM To: Myles Watson Cc: Coreboot; Stefan Reinauer Subject: Re: [coreboot] v3 patch rm elfboot
On Feb 15, 2008 2:59 PM, Myles Watson mylesgw@gmail.com wrote:
So the use case is that you have a lar file, but don't have the ELF that you started with originally, and you want to put it into a different lar file?
yes. Because you have a board, and a new (e.g.) boot block, which you have been sent via email from a company that is supporting your platform, and can not for whatever reason recreate the payload. You may have even lost the source to it. This happens.
It might be easier to remove the other things in the lar instead, or do
a lar copy from one lar to a new lar, which wouldn't lose the entry points.
Sure.
Given that, I'd be willing to implement "add from lar", so that you can move lar files from one archive into another. It would be something like lar -aL larfile:larpath:newlarpath. I think going back to ELF is problematic because it wouldn't be a true ELF anymore. Would that be enough, Stefan? Anyone else?
You are not seriously proposing that "add from lar" mode, right? Even parsing a MANIFEST file would be better than that. Sorry.
The easiest thing would be to create a MANIFEST file during unpacking which looks like the output of "lar -l". All the information you'll ever need to repack in one nice human-readable file. You can then decide how lar should parse the file when recreating the archive, but in case the original lar does not exist anymore, you have to do something useful with the extracted contents.
The best thing would be to extract the LAR into sort of a fake ELF file, just conforming enough to be parsed by lar without problems.
Regards, Carl-Daniel
On Feb 15, 2008 4:23 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
The best thing would be to extract the LAR into sort of a fake ELF file, just conforming enough to be parsed by lar without problems.
This is still the best option. It's pretty simple.
ron
* ron minnich rminnich@gmail.com [080215 22:09]:
OK, well elf is out of v3, at this point.
Can we please put this back in? preparsing added so many problems and complications that it almost seems right to drop the idea again. The ELF loader wasn't all that big of an issue all the time, so I really don't understand the rush without having a correctly working alternative.
On 16.02.2008 16:07, Stefan Reinauer wrote:
- ron minnich rminnich@gmail.com [080215 22:09]:
OK, well elf is out of v3, at this point.
Can we please put this back in? preparsing added so many problems and complications that it almost seems right to drop the idea again. The ELF loader wasn't all that big of an issue all the time, so I really don't understand the rush without having a correctly working alternative.
It's not out of v3 yet AFAICS.
Regards, Carl-Daniel
On Feb 16, 2008 7:07 AM, Stefan Reinauer stepan@coresystems.de wrote:
- ron minnich rminnich@gmail.com [080215 22:09]:
OK, well elf is out of v3, at this point.
Can we please put this back in? preparsing added so many problems and complications that it almost seems right to drop the idea again. The ELF loader wasn't all that big of an issue all the time, so I really don't understand the rush without having a correctly working alternative.
Let's discuss the problems it added. I have not yet had any, but if we added problems, let's talk about what they are and what the fix might be. I am sorry if it all seemed like a rush.
I was, myself, glad to get ELF processing out of coreboot, or at least make it possible not to use it. The ELF processing in the firmware was always an issue. What I don't like is the the following: we had to decompress the elf to some location in memory just to find out where to put the ELF segments. It was a multi-step process with lots of potential for failure. So, the steps are 1. decompress elf to memory (where? Well, you have to pick a place. How? a type of malloc)
2. parse the elf and look for errors. If there are errors, stop; you can't boot. But you can figure this out at build time, and you should. Figuring it out requires pre-parsing the ELF. If you have to pre-parse the ELF, why not just put the ELF sections into LAR in the first place?
3. Parse the ELF and figure out where to put the elf segments. If the elf segments need to go where the decompressed data is, then the code has to move memory around. And, note, you can't assume that the ELF headers are at the beginning of the file -- they can be anywhere in the file. [I believe the ELF parser in v2 got this wrong -- I had a problem with it at some point. The ELF parser in v2 is not sophisticated enough to deal with all the possible variations in an ELF file. ] THIS ADDITIONAL "RELOCATE" STEP WAS IN v2, but NOT in v3. That's why the v3 code is so simple -- it can not handle all cases and will fail if the ELF needs to be adjusted. elfboot.c in v3 is 215 lines, and in v2 it is almost 700. The binary for elfboot in v2 is 1600 bytes, for v3 it is 1 kbyte, and we've shown we don't need it. Saving 1K in the boot block is a good thing. Adding back 600 bytes is a bad thing. We would have to grow the boot block to add true ELF support back into v3.
Compare to the pre-parsed elf: 1. Look at LAR entry, see where it goes, decompress to its destination, done. And, the handling for payload is the same as for all other LAR files -- no difference.
All error checking on ELF can be done when the firmware image is created, not when the ELF is unpacked. There is not an extra unnecessary copy step. The v2 ELF parsing is complex and I find the code hard to understand, especially if I have not looked it in a while. I saw the "error message and die" scenario more than once in v2, and it never happens in v3.
To really do the v2 code correctly, we're going to have to add malloc back. We're going to have to grow that code to handle headers in different locations.
ELF pre-parsing is a big win in my view. I think resolving the issues that it caused is easy -- we just have to agree on how to resolve them. If nobody else wants to write it, I can write the code to extract LAR entries into ELF files. It's really not that hard.
If we have decided that there is not sufficient information in a LAR header to really recreate the ELF, then we have other options. One is to make the LAR headers just be ELF headers. Second is to extend the LAR headers to contain all the info we need.
But I really, really do not want to go back to using an ELF loader in coreboot. I think removing ELF support in coreboot is the right thing to do.
thanks
ron
* Myles Watson mylesgw@gmail.com [080215 20:56]:
Elfboot still uses bootblock space if it is disabled in Kconfig? That's definitely something we need to fix. That space is tight.
PAYLOAD_NONE doesn't mean no elfboot. Elfboot is included always so that if someone adds an ELF to the lar later it will still work. My contention is that we should make people preparse the ELF when adding it to the lar, so that v3 doesn't have to understand ELF. That doesn't mean that lar shouldn't. Just the opposite.
I am talking about CONFIG_PAYLOAD_PREPARSE_ELF, not at all about CONFIG_PAYLOAD_NONE.
Please don't just remove this code. If you don't like to compile it in, create a config option to disable it. (There is such a config option already, so I really don't see the gain)
I didn't see one.
One might say CONFIG_PAYLOAD_PREPARSE_ELF implicates that the ELF parsing is ommitted in coreboot. I agree that that's misleading. Alle the ELF preparsing pretty much messed up the code to begin with.
Because with this patch it is no longer possible to unpack a lar.
v3 doesn't unpack a lar. lar unpacks them.
C'mon. lar is part of v3, and lar can not unpack lar files without loosing information anymore. This is broken, and it breaks all of the v3 concept. If people want a config option to choose a broken concept, well, go ahead. But please leave a method in for the rest of us who need to rely on a safe and reliable method.
I don't think it is appropriate talking about dropping ELF support from coreboot as long as the lar handling is as broken as it currently is. Regardless whether that happens in our code under lib/ or under util/
Or we forget about lars having the feature to be unpacked. Not sure that its ever needed, except when you want to migrate a payload from one image to another one.
This is a lar problem, not a v3 problem.
You keep repeating this like a mantra. Would you mind explaining the difference? I don't care what part is broken. I just don't think it is sane to drop working code while the broken code doing something similar is not fixed.
Sorry.
I am talking about CONFIG_PAYLOAD_PREPARSE_ELF, not at all about CONFIG_PAYLOAD_NONE.
Please don't just remove this code. If you don't like to compile it
in,
create a config option to disable it. (There is such a config option already, so I really don't see the gain)
I didn't see one.
One might say CONFIG_PAYLOAD_PREPARSE_ELF implicates that the ELF parsing is ommitted in coreboot. I agree that that's misleading. Alle the ELF preparsing pretty much messed up the code to begin with.
Because with this patch it is no longer possible to unpack a lar.
v3 doesn't unpack a lar. lar unpacks them.
C'mon. lar is part of v3, and lar can not unpack lar files without loosing information anymore. This is broken, and it breaks all of the v3 concept. If people want a config option to choose a broken concept, well, go ahead. But please leave a method in for the rest of us who need to rely on a safe and reliable method.
I don't think it is appropriate talking about dropping ELF support from coreboot as long as the lar handling is as broken as it currently is. Regardless whether that happens in our code under lib/ or under util/
I agree. What I was expecting was something like "These are the issues with your patch, but before it commits lar needs to be able to do X, Y and Z."
Or we forget about lars having the feature to be unpacked. Not sure
that
its ever needed, except when you want to migrate a payload from one image to another one.
This is a lar problem, not a v3 problem.
You keep repeating this like a mantra. Would you mind explaining the difference? I don't care what part is broken. I just don't think it is sane to drop working code while the broken code doing something similar is not fixed.
The difference is whether or not the patch I submitted is broken. I only patched v3. I left lar alone in this patch.
I know that one use case is "I forgot where I put the ELF I packed into the lar.", but a more interesting case to me is when you want to grab the payload from someone else's lar. In that case you're stuck if they chose the PREPARSE_ELF option.
Sorry.
No need to be. I don't expect every patch I submit to be committed within the hour :)
Myles
I dropped this idea on Stefan and he tells me I am not insane. I even did it BEFORE coffee :-)
Could a LAR contain more LAR files? i.e. could LAR be recursive.
The recursive nature of LAR files could be trivially mirrored in the code.
So, we could have this: normal/payload.lar
then, the payload would BE a LAR, and to move it to another LAR, lar -x bios.bin normal/payload.lar lar -a newbios.bin payload.lar:normal/payload.lar
Information preserved. Very little code need be added, since LAR parsing is already in the rom, you just need to cover the case of supporting LAR-in-LAR. No new sub-LARs needed.
Comments?
ron
On 18.02.2008 18:19, ron minnich wrote:
I dropped this idea on Stefan and he tells me I am not insane. I even did it BEFORE coffee :-)
Could a LAR contain more LAR files? i.e. could LAR be recursive.
Please don't! See the end of the mail for my reasons.
The recursive nature of LAR files could be trivially mirrored in the code.
So, we could have this: normal/payload.lar
then, the payload would BE a LAR, and to move it to another LAR, lar -x bios.bin normal/payload.lar lar -a newbios.bin payload.lar:normal/payload.lar
Why not have an option/compression specifier like "nocompress:"? We could call it "copy:" and it would copy the individual members of the old LAR to the new LAR. No code changes in lib/lar.c needed.
Information preserved. Very little code need be added, since LAR parsing is already in the rom, you just need to cover the case of supporting LAR-in-LAR. No new sub-LARs needed.
The piece of code which decides where to look for the next LAR member in lib/lar.c is the most fragile piece of code in v3. I'd rather avoid any changes to the code unless absolutely necessary, since I very well remember how much time we lost each time we hit a bug in a corner case.
Regards, Carl-Daniel
-----Original Message----- From: Carl-Daniel Hailfinger [mailto:c-d.hailfinger.devel.2006@gmx.net] Sent: Monday, February 18, 2008 10:38 AM To: ron minnich Cc: Myles Watson; Stefan Reinauer; Coreboot Subject: Re: [coreboot] v3 patch rm elfboot
On 18.02.2008 18:19, ron minnich wrote:
I dropped this idea on Stefan and he tells me I am not insane. I even did it BEFORE coffee :-)
Could a LAR contain more LAR files? i.e. could LAR be recursive.
Please don't! See the end of the mail for my reasons.
I agree. I don't think it's needed.
The recursive nature of LAR files could be trivially mirrored in the
code.
So, we could have this: normal/payload.lar
then, the payload would BE a LAR, and to move it to another LAR, lar -x bios.bin normal/payload.lar lar -a newbios.bin payload.lar:normal/payload.lar
I don't see the need for the intermediate step. Since lar reads lar files, why not make it so that lar can add entries from other lar files?
Why not have an option/compression specifier like "nocompress:"? We could call it "copy:" and it would copy the individual members of the old LAR to the new LAR. No code changes in lib/lar.c needed.
I think we can still use compression flags, and have the code decide whether it needs to re-compress the entries.
Thanks, Myles
-
Carl-Daniel Hailfinger -
Corey Osgood -
Myles Watson -
ron minnich -
Stefan Reinauer