On 27 June 2017 at 17:48, Leif Lindholm leif.lindholm@linaro.org wrote:
On Tue, Jun 27, 2017 at 05:24:30PM +0000, Ard Biesheuvel wrote:
On 27 June 2017 at 17:00, Leif Lindholm leif.lindholm@linaro.org wrote:
On Tue, Jun 27, 2017 at 04:39:47PM +0000, Ard Biesheuvel wrote:
On 27 June 2017 at 16:33, Leif Lindholm leif.lindholm@linaro.org wrote:
On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
This implements a UEFI Shell application that updates the EFI NOR
standalone UEFI Shell application
partition with the contents of STYX_EFI.Fv. Note that this means that a) EFI variables are preserved, and b) this updater can only be used on systems that have already been flashed with a firmware build based on this open source branch (and not on systems still running AMI firmware)
Request for clarification based on your explanation below: Presumably b) is for one or both of these reasons:
- AMI firmware will not produce all protocols needed for this application to run (just like this firmware will not produce all protocols the AMI updater needs)?
- This application does not zero out the variable area?
The AMI firmware is also based on SeattleFDK, so it does produce the ISCP protocol that exposes the NOR read/write/erase methods. This is what makes it dangerous, given that the flasher will most likely run happily, but will not leave the NOR in a state where a varstore FV header appears in the expected place. This is what will make the firmware crash.
Note that none of these issues are particularly difficult to deal with, but it increases the complexity of this tools, which is not intended as a production quality firmware update distribution method but simply to allow me to distribute firmware updates to people like Lorenzo and Marc, and now some Cello users as well which shouldn't have been shipped boards to begin with.
All I'm really getting at with these questions around a very much welcome new tool is that I have a feeling it isn't necessarily end-user proof at this stage, so I think I would like to see either a few warnings in this commit message (which currently reads more like it's saying more along the lines of "it will only work if" than "you will become a customer of Dediprog if you try") or some attempt to determine whether we are on a platform where it is safe to attempt an upgrade.
Oh, this is absolutely unsuitable for general use. For instance, it does not distinguish between Cello, Overdrive and Overdrive1000, nor does it perform any kind of verification after the flash, all of which are not that difficult to implement, but simply not things that I can be bothered to care about at the moment, given the sorry state of Cello, and the fact that the only three users of this firmware for Overdrive are one floor down from where your desk is.
So for now, it is either merging this code, with perhaps some discouraging capitalized scary words added to the commit message, or just disregarding the patch for now, in the hope that someone else (Alan perhaps?) can be bothered to pick it up later and polish it up a bit. I honestly don't care either way.
In order for the flasher to have access to the various PCDs that describe where STYX_EFI.Fv lives in the NOR, it needs to be built as part of the platform, but after STYX_EFI.Fv has been generated, which results in a chicken-and-egg situation. Therefore, the recommended way of generating the flasher is
- delete the Build/<platform> directory entirely
- build the platform
- build the platform again, but with -D DO_FLASHER=TRUE appended
Build with -m .../StyxFlashUefi.inf instead of rebuilding the whole platform?
Yes, that should be sufficient. I will add that.
The flasher application is called 'StyxFlashUefi'
Only style comments below, but one silly question before that: Is there some particular reason why the application embeds the FV instead of reading it from a filesystem? Or just that you figured it's small enough that it doesn't matter?
Two reasons:
- simplicity; the actual code is *very* simply due to the fact that it
can refer to the payload directly, and I am not a seasoned shell application hacker that has a clue how one would go about implementing it using file I/O etc
- a generic flasher needs metadata that describes where the payload
needs to go, and the .Fv file does not contain that, so it would involve a file format or a command line option with all the associated failure modes.
Right, so basically because PcdFvBaseAddress and PcdFdBaseAddress could change? But would such a change not mean that variables would not be preserved after all?
Not just because they could change, but because their value is set in the .FDF, and so any attempt to obtain the values elsewhere would involve adding a redundant definition, which I would like to avoid.
Oh, indeed.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds | 86 ++++++++++++++++++ Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S | 25 +++++ Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c | 96 ++++++++++++++++++++ Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf | 53 +++++++++++ Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc | 12 ++- Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc | 8 ++ Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc | 8 ++ 7 files changed, 286 insertions(+), 2 deletions(-)