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.
Let's see the patch:
>
> Contributed-under: TianoCore Contribution Agreement 1.0
>
> Signed-off-by: Michael Chang <mch...@suse.com>
> ---
> 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?
Thanks,
Laszlo
>From 6881e7c915f34f2d66a8c864660c407ec1e1b7d1 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <ler...@redhat.com>
Date: Mon, 24 Jun 2013 14:53:00 +0200
Subject: [PATCH] OvmfPkg: never attempt to load the NvVars file after warm VM
reboot
To that end, always set the "NvVars" variable in LoadNvVarsFromFs().
Currently, if the NvVars file is missing after cold boot,
LoadNvVarsFromFs() doesn't set the "NvVars" variable. Therefore a
subsequent VM reboot (ie. warm boot) will load the "NvVars" file created
during the first run after cold boot, trampling over any runtime variable
updates pending in memory from between ExitBootServices() and the warm VM
reboot.
Reported-by: Michael Chang <mch...@suse.com>
Analyzed-by: Michael Chang <mch...@suse.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---
OvmfPkg/Library/NvVarsFileLib/FsAccess.c | 21 ++++++---------------
1 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/OvmfPkg/Library/NvVarsFileLib/FsAccess.c
b/OvmfPkg/Library/NvVarsFileLib/FsAccess.c
index 190a564..ef841ba 100644
--- a/OvmfPkg/Library/NvVarsFileLib/FsAccess.c
+++ b/OvmfPkg/Library/NvVarsFileLib/FsAccess.c
@@ -314,7 +314,7 @@ LoadNvVarsFromFs (
(VOID*) &VarData
);
if (Status == EFI_SUCCESS) {
- DEBUG ((EFI_D_INFO, "NV Variables were already loaded\n"));
+ DEBUG ((EFI_D_INFO, "won't load NV Variables on reboot\n"));
return EFI_ALREADY_STARTED;
}
@@ -322,15 +322,13 @@ LoadNvVarsFromFs (
// Attempt to restore the variables from the NvVars file.
//
Status = ReadNvVarsFile (FsHandle);
- if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_INFO, "Error while restoring NV variable data\n"));
- return Status;
- }
+ DEBUG ((EFI_D_INFO,
+ EFI_ERROR (Status) ? "Error while restoring NV variable data\n" :
+ "FsAccess.c: Read NV Variables file\n"));
//
- // Write a variable to indicate we've already loaded the
- // variable data. If it is found, we skip the loading on
- // subsequent attempts.
+ // Independently of the current existence of the NvVars file, don't try to
+ // load it after reboots; see justification in comment above.
//
Size = sizeof (VarData);
VarData = TRUE;
@@ -343,13 +341,6 @@ LoadNvVarsFromFs (
Size,
(VOID*) &VarData
);
-
- DEBUG ((
- EFI_D_INFO,
- "FsAccess.c: Read NV Variables file (size=%d)\n",
- Size
- ));
-
return Status;
}
--
1.7.1
------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
