2013/6/24 Laszlo Ersek <[email protected]>: > On 06/21/13 10:27, Michael Chang wrote: >> My fresh vm installation with OVMF firmware has problem that the OS boot >> entry are not displayed by boot manager. It happens only when the EFI >> System Partition are created from scratch, that is there's no NvVars in >> it initially. >> >> After some testing and debugging, the cause is modified BootOrder gets >> overwritten by the loaded NvVars, which is using a stale dump created >> from previous boot. >> >> This patch fixes the problem for me by handing the initial case (ie NvVars >> file not found) with loading NvVars immediately after it's been saved. Then >> the system is flagged as NvVars loaded and would not attempt to restore it >> in reboot which could always use stale dumps. > > ... I think I can see interaction between ReserveEmuVariableNvStore(), > FvbInitialize(), and LoadNvVarsFromFs() now. When > ReserveEmuVariableNvStore() runs after VM reboot, AllocateRuntimePool() > will return the same address (overlaying the previous memory buffer) > because it runs "early on". What an exorbitant hack! > > We have a four-dimensional state space here: > > file store "NvVars" exists > state VM online exists in RAM buffer RAM buffer dirty > ----- --------- ---------- --------------- ---------------- > A no no no no > B no yes no no > C yes yes no no > D yes yes no yes > E yes yes yes no > F yes yes yes yes > > I've only assigned states to when the VM is "at rest". This is the > current state diagram: > > +-----+ +-----+ > | | | | > | | | |<-----------------------+ > | A | +-->| B | | > | | | | |<-----------+ | > | | | | | | | > +-----+ | +-----+ | | > | | | ^ | | > | | | | | | > | | | | +----+ | | > | | | | | | | | > v | v | v | | | > +-----+ | +-----+ | +-----+ | > | |----+ | |---+ | | | > | | | | | | | > +-->| C |------->| E |-------->| F |<--+ | > | | | | | | | | | > | | |----+ | |<--------| | | | > | +-----+ | +-----+ +-----+ | | > | | | ^ | | | > | | | | | | | > +------+ | | +------+ | > | +-----+ | > | | | | > +-->| | | > | D |------------------------+ > +-->| | > | | | > | +-----+ > | | > | | > +------+ > > A->C: Very first cold boot. LoadNvVarsFromFs() doesn't find the "NvVars" > variable in RAM. It doesn't find the file store either. SaveNvVarsToFs() > (called by ConnectNvVarsToFileSystem()) creates the file store. > > B->E: Second or later cold boot. LoadNvVarsFromFs() doesn't find the > "NvVars" variable in RAM, but it finds the file store, and sets the > "NvVars" variable. > > C->B: Power off before ExitBootServices(), after first cold boot. No > variable data is lost. > > C->C: A variable is updated before ExitBootServices(), and synced to the > file store immediately. > > C->D: ExitBootServices() is called, and a variable is subsequently > changed at runtime. The RAM buffer diverges from the file store. > > C->E: VM reboot before ExitBootServices(), after the very first cold > boot. No variable contents is lost. LoadNvVarsFromFs() doesn't find the > "NvVars" variable in RAM, but it finds the file store, and sets the > "NvVars" variable. > > D->B: Power off after ExitBootServices(). Dirty variable data (ie. > what's only in RAM) is lost. > > D->D: A variable is changed at runtime. The RAM buffer diverges from the > file store even more. > > D->E: VM reboot after ExitBootServices(), after the very first cold > boot. LoadNvVarsFromFs() doesn't find the "NvVars" variable. It finds > the file store and loads it, and sets the "NvVars" variable. Dirty > variable data (changed at runtime) from before the VM reboot is lost, > and that's the bug we're trying to fix here. > > E->B: Poweroff. No data is lost. > > E->E: This loop represents two kinds of transitions actually. (a) Boot > time variable update sometime after VM reboot; data is synced to the > file store immediately. (b) VM reboot after VM reboot, before > ExitBootServices() in the most recent boot. The "NvVars" variable *is* > found, hence reload of the file store is skipped. The file store is > re-saved with identical contents from before the most recent VM reboot. > > E->F: ExitBootServices() is called after the VM has been rebooted at > least once, and a variable is subsequently changed at runtime. The RAM > buffer diverges from the file store. > > F->B: Poweroff. Dirty variable data is lost. > > F->E: VM reboot at runtime, after the VM has been rebooted earlier at > least once. LoadNvVarsFromFs() *does* find "NvVars", skips loading the > stale file store, and ConnectNvVarsToFileSystem()/SaveNvVarsToFs() > writes out the dirty variable data to the file store. This is correct. > > F->F: A variable is changed at runtime. The RAM buffer diverges from the > file store even more.
Very sincerely appreciate your work to summary the states. I spend some time to walk through them and found they are very precise in describing what's going on on every combination of possibilities, thus is good for me since it baffles me a lot in trying to sort everything out while investigating this problem. If my patch could result in no regression that's being luck. :) Hope you feel this ok, as my intention in the most part is to get this problem addressed or nailed down and it may not be any fix that's right on the spot. > > > Let's see the patch: > >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> >> Signed-off-by: Michael Chang <[email protected]> >> --- >> OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c | 19 ++++++++++++++++++- >> 1 file changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c >> b/OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c >> index c89bb4a..fcbfcbb 100644 >> --- a/OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c >> +++ b/OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c >> @@ -36,12 +36,20 @@ ConnectNvVarsToFileSystem ( >> ) >> { >> EFI_STATUS Status; >> + BOOLEAN Initial = FALSE; >> >> // >> // We might fail to load the variable, since the file system initially >> // will not have the NvVars file. >> // >> - LoadNvVarsFromFs (FsHandle); >> + Status = LoadNvVarsFromFs (FsHandle); >> + >> + // >> + // The NvVars file is initially not exist. >> + // >> + if (Status == EFI_NOT_FOUND) { >> + Initial = TRUE; >> + } > > OK, so we're setting Initial to TRUE when leaving state "A", which is > only possible on the A->C transition. > >> >> // >> // We must be able to save the variables successfully to the file system >> @@ -52,6 +60,15 @@ ConnectNvVarsToFileSystem ( >> mNvVarsFileLibFsHandle = FsHandle; >> } >> >> + // >> + // In initial case we load the NvVars right after it's created. By doing >> + // this we can avoid the problem caused by loading saved NvVars in next >> + // (re)boot, which could contain stale data saved from previous boot. >> + // >> + if (Initial) { >> + LoadNvVarsFromFs (FsHandle); >> + } >> + >> return Status; >> } >> >> > > After the A->C transition, ie. in state "C", we call LoadNvVarsFromFs() > again: > - LoadNvVarsFromFs() doesn't find the "NvVars" variable, > - the file store is found and loaded (with contents we just saved), > - the "NvVars" variable is set, immediately transitioning to state "E" > (basically imitating a C->E transition, a VM reboot, see above). > > I think the underlying idea, which should be the following, is correct: > - merge state "A" into state "B", and > - merge state "C" into state "E", and > - merge state "D" into state "F", and > - the edges touching A, C, or D decay into the preexistent edges below: > A->C | B->E > C->B | E->B > C->C | E->E > C->D | E->F > C->E | E->E > D->B | F->B > D->D | F->F > D->E | F->E -- this is the bugfix > > The resultant states and transitions are > > file store "NvVars" exists > state VM online exists in RAM buffer RAM buffer dirty > ----- --------- ---------- --------------- ---------------- > B no *maybe* no no > E yes yes *yes* no > F yes yes *yes* yes > > +-----+ > | | > | | > | B | > | |<-----------+ > | | | > +-----+ | > | ^ | > | | | > | | +----+ | > | | | | | > v | v | | > +-----+ | +-----+ > | |---+ | | > | | | | > | E |-------->| F |<--+ > | | | | | > | |<--------| | | > +-----+ +-----+ | > | | > | | > +------+ > > On the other hand, I think we should implement this more simply. What do > you think of the attached patch? I agree with you should fix it more simply. I have no problem with it, actually I had a similar idea, but finally pick the other way as I wasn't sure about LoadNvVarsFromFs() would be called in other context other than in the ConnectNvVarsToFileSystem () thus changing the implication of NvVars variable in this function would potentially create contradict in the future ? Although ConnectNvVarsToFileSystem() is also a library function but it's not so core as LoadNvVarsFromFs() IMHO. And I know my concern could be a little boring and picky. :) Feel free to correct me if you think that's not necessary. Thanks for your review. Michael > > Thanks, > Laszlo ------------------------------------------------------------------------------ This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
