OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
In the commit 4f173db8b4 "OvmfPkg/PlatformInitLib: Add functions for
EmuVariableNvStore", it introduced a PlatformValidateNvVarStore() function
for checking the integrity of NvVarStore.
In some cases when the VariableHeader->StartId is VARIABLE_DATA, the
VariableHeader->State is not just one of the four primary states:
VAR_IN_DELETED_TRANSITION, VAR_DELETED, VAR_HEADER_VALID_ONLY, VAR_ADDED.
The state may combined two or three states, e.g.
0x3C = (VAR_IN_DELETED_TRANSITION & VAR_ADDED) & VAR_DELETED
or
0x3D = VAR_ADDED & VAR_DELETED
When the variable store has those variables, system booting/rebooting will
hangs in a ASSERT:
NvVarStore Variable header State was invalid.
ASSERT
/mnt/working/source_code-git/edk2/OvmfPkg/Library/PlatformInitLib/Platform.c(819):
((BOOLEAN)(0==1))
Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), we
saw some variables which have 0x3C or 0x3D state in store.
e.g.
UpdateVariable(), VariableName=BootOrder
L1871, State=0000003F <-- VAR_ADDED
State &= VAR_DELETED=0000003D
FlushHobVariableToFlash(), VariableName=BootOrder
...
UpdateVariable(), VariableName=InitialAttemptOrder
L1977, State=0000003F
State &= VAR_IN_DELETED_TRANSITION=0000003E
L2376, State=0000003E
State &= VAR_DELETED=0000003C
FlushHobVariableToFlash(), VariableName=InitialAttemptOrder
...
UpdateVariable(), VariableName=ConIn
L1977, State=0000003F
State &= VAR_IN_DELETED_TRANSITION=0000003E
L2376, State=0000003E
State &= VAR_DELETED=0000003C
FlushHobVariableToFlash(), VariableName=ConIn
...
So, only allowing the four primary states is not enough. This patch changes
the falid states list (Follow Jiewen Yao's suggestion):
1. VAR_HEADER_VALID_ONLY (0x7F)
- Header added (*)
2. VAR_ADDED (0x3F)
- Header + data added
3. VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E)
- marked as deleted, but still valid, before new data is added. (*)
4. VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED (0x3C)
- deleted, after new data is added.
5. VAR_ADDED & VAR_DELETED (0x3D)
- deleted directly, without new data.
(*) means to support surprise shutdown.
And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) because they are
invalid states.
v2:
Follow Jiewen Yao's suggestion to add the following valid states:
VAR_ADDED & VAR_DELETED (0x3D)
VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E)
VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED (0x3C)
and removed the following invalid states:
VAR_IN_DELETED_TRANSITION
VAR_DELETED
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
index 77f22de046..6963c47e0b 100644
--- a/OvmfPkg/Library/PlatformInitLib/Platform.c
+++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
@@ -702,10 +702,11 @@ PlatformValidateNvVarStore (
VariableOffset = NvVarStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER);
} else {
- if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) ||
- (VariableHeader->State == VAR_DELETED) ||
- (VariableHeader->State == VAR_HEADER_VALID_ONLY) ||
- (VariableHeader->State == VAR_ADDED)))
+ if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) ||
+ (VariableHeader->State == VAR_ADDED) ||
+ (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) ||
+ (VariableHeader->State == (VAR_ADDED & VAR_IN_DELETED_TRANSITION)) ||
+ (VariableHeader->State == (VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED))))
{
DEBUG ((DEBUG_ERROR, "NvVarStore Variable header State was invalid.\n"));
return FALSE;
--
2.35.3
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97581): https://edk2.groups.io/g/devel/message/97581
Mute This Topic: https://groups.io/mt/95777531/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Thanks for the fix. Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> Question: Have you run tiano CI by yourself, before submit the patch? > -----Original Message----- > From: Lee, Chun-Yi <joeyli.kernel@gmail.com> > Sent: Thursday, December 15, 2022 10:27 PM > To: devel@edk2.groups.io > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > <kraxel@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky > <thomas.lendacky@amd.com>; James Bottomley <jejb@linux.ibm.com>; > Aktas, Erdem <erdemaktas@google.com>; Lee, Chun-Yi <jlee@suse.com> > Subject: [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of > NvVarStore in some cases > > In the commit 4f173db8b4 "OvmfPkg/PlatformInitLib: Add functions for > EmuVariableNvStore", it introduced a PlatformValidateNvVarStore() function > for checking the integrity of NvVarStore. > > In some cases when the VariableHeader->StartId is VARIABLE_DATA, the > VariableHeader->State is not just one of the four primary states: > VAR_IN_DELETED_TRANSITION, VAR_DELETED, VAR_HEADER_VALID_ONLY, > VAR_ADDED. > The state may combined two or three states, e.g. > > 0x3C = (VAR_IN_DELETED_TRANSITION & VAR_ADDED) & VAR_DELETED > or > 0x3D = VAR_ADDED & VAR_DELETED > > When the variable store has those variables, system booting/rebooting will > hangs in a ASSERT: > > NvVarStore Variable header State was invalid. > ASSERT > /mnt/working/source_code- > git/edk2/OvmfPkg/Library/PlatformInitLib/Platform.c(819): > ((BOOLEAN)(0==1)) > > Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), we > saw some variables which have 0x3C or 0x3D state in store. > e.g. > > UpdateVariable(), VariableName=BootOrder > L1871, State=0000003F <-- VAR_ADDED > State &= VAR_DELETED=0000003D > FlushHobVariableToFlash(), VariableName=BootOrder > ... > UpdateVariable(), VariableName=InitialAttemptOrder > L1977, State=0000003F > State &= VAR_IN_DELETED_TRANSITION=0000003E > L2376, State=0000003E > State &= VAR_DELETED=0000003C > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder > ... > UpdateVariable(), VariableName=ConIn > L1977, State=0000003F > State &= VAR_IN_DELETED_TRANSITION=0000003E > L2376, State=0000003E > State &= VAR_DELETED=0000003C > FlushHobVariableToFlash(), VariableName=ConIn > ... > > So, only allowing the four primary states is not enough. This patch changes > the falid states list (Follow Jiewen Yao's suggestion): > > 1. VAR_HEADER_VALID_ONLY (0x7F) > - Header added (*) > 2. VAR_ADDED (0x3F) > - Header + data added > 3. VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > - marked as deleted, but still valid, before new data is added. (*) > 4. VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED (0x3C) > - deleted, after new data is added. > 5. VAR_ADDED & VAR_DELETED (0x3D) > - deleted directly, without new data. > (*) means to support surprise shutdown. > > And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) because > they are > invalid states. > > v2: > Follow Jiewen Yao's suggestion to add the following valid states: > VAR_ADDED & VAR_DELETED (0x3D) > VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED > (0x3C) > and removed the following invalid states: > VAR_IN_DELETED_TRANSITION > VAR_DELETED > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > --- > OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > b/OvmfPkg/Library/PlatformInitLib/Platform.c > index 77f22de046..6963c47e0b 100644 > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( > > VariableOffset = NvVarStoreHeader->Size - sizeof > (VARIABLE_STORE_HEADER); > } else { > - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > - (VariableHeader->State == VAR_DELETED) || > - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > - (VariableHeader->State == VAR_ADDED))) > + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || > + (VariableHeader->State == VAR_ADDED) || > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > + (VariableHeader->State == (VAR_ADDED & > VAR_IN_DELETED_TRANSITION)) || > + (VariableHeader->State == (VAR_ADDED & > VAR_IN_DELETED_TRANSITION & VAR_DELETED)))) > { > DEBUG ((DEBUG_ERROR, "NvVarStore Variable header State was > invalid.\n")); > return FALSE; > -- > 2.35.3 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97523): https://edk2.groups.io/g/devel/message/97523 Mute This Topic: https://groups.io/mt/95724215/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Jiewen, Sorry for I didn't create tiano CI on github in time. And thanks for your help to merge my patch. I will create tiano CI in next time after getting our Review-by tag. Thanks! Joey Lee On Sat, Dec 17, 2022 at 03:17:42AM +0000, Yao, Jiewen via groups.io wrote: > Thanks for the fix. > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > -----Original Message----- > > From: Lee, Chun-Yi <joeyli.kernel@gmail.com> > > Sent: Thursday, December 15, 2022 10:27 PM > > To: devel@edk2.groups.io > > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > > <kraxel@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky > > <thomas.lendacky@amd.com>; James Bottomley <jejb@linux.ibm.com>; > > Aktas, Erdem <erdemaktas@google.com>; Lee, Chun-Yi <jlee@suse.com> > > Subject: [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of > > NvVarStore in some cases > > > > In the commit 4f173db8b4 "OvmfPkg/PlatformInitLib: Add functions for > > EmuVariableNvStore", it introduced a PlatformValidateNvVarStore() function > > for checking the integrity of NvVarStore. > > > > In some cases when the VariableHeader->StartId is VARIABLE_DATA, the > > VariableHeader->State is not just one of the four primary states: > > VAR_IN_DELETED_TRANSITION, VAR_DELETED, VAR_HEADER_VALID_ONLY, > > VAR_ADDED. > > The state may combined two or three states, e.g. > > > > 0x3C = (VAR_IN_DELETED_TRANSITION & VAR_ADDED) & VAR_DELETED > > or > > 0x3D = VAR_ADDED & VAR_DELETED > > > > When the variable store has those variables, system booting/rebooting will > > hangs in a ASSERT: > > > > NvVarStore Variable header State was invalid. > > ASSERT > > /mnt/working/source_code- > > git/edk2/OvmfPkg/Library/PlatformInitLib/Platform.c(819): > > ((BOOLEAN)(0==1)) > > > > Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), we > > saw some variables which have 0x3C or 0x3D state in store. > > e.g. > > > > UpdateVariable(), VariableName=BootOrder > > L1871, State=0000003F <-- VAR_ADDED > > State &= VAR_DELETED=0000003D > > FlushHobVariableToFlash(), VariableName=BootOrder > > ... > > UpdateVariable(), VariableName=InitialAttemptOrder > > L1977, State=0000003F > > State &= VAR_IN_DELETED_TRANSITION=0000003E > > L2376, State=0000003E > > State &= VAR_DELETED=0000003C > > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder > > ... > > UpdateVariable(), VariableName=ConIn > > L1977, State=0000003F > > State &= VAR_IN_DELETED_TRANSITION=0000003E > > L2376, State=0000003E > > State &= VAR_DELETED=0000003C > > FlushHobVariableToFlash(), VariableName=ConIn > > ... > > > > So, only allowing the four primary states is not enough. This patch changes > > the falid states list (Follow Jiewen Yao's suggestion): > > > > 1. VAR_HEADER_VALID_ONLY (0x7F) > > - Header added (*) > > 2. VAR_ADDED (0x3F) > > - Header + data added > > 3. VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > - marked as deleted, but still valid, before new data is added. (*) > > 4. VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED (0x3C) > > - deleted, after new data is added. > > 5. VAR_ADDED & VAR_DELETED (0x3D) > > - deleted directly, without new data. > > (*) means to support surprise shutdown. > > > > And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) because > > they are > > invalid states. > > > > v2: > > Follow Jiewen Yao's suggestion to add the following valid states: > > VAR_ADDED & VAR_DELETED (0x3D) > > VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED > > (0x3C) > > and removed the following invalid states: > > VAR_IN_DELETED_TRANSITION > > VAR_DELETED > > > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > > --- > > OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > > b/OvmfPkg/Library/PlatformInitLib/Platform.c > > index 77f22de046..6963c47e0b 100644 > > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > > @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( > > > > VariableOffset = NvVarStoreHeader->Size - sizeof > > (VARIABLE_STORE_HEADER); > > } else { > > - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > > - (VariableHeader->State == VAR_DELETED) || > > - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > - (VariableHeader->State == VAR_ADDED))) > > + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > + (VariableHeader->State == VAR_ADDED) || > > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > > + (VariableHeader->State == (VAR_ADDED & > > VAR_IN_DELETED_TRANSITION)) || > > + (VariableHeader->State == (VAR_ADDED & > > VAR_IN_DELETED_TRANSITION & VAR_DELETED)))) > > { > > DEBUG ((DEBUG_ERROR, "NvVarStore Variable header State was > > invalid.\n")); > > return FALSE; > > -- > > 2.35.3 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97614): https://edk2.groups.io/g/devel/message/97614 Mute This Topic: https://groups.io/mt/95724215/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Joey You are welcome. Thanks to catch the issue and provide the patch. One thing to clarify: The patch submitter is expected to pass CI, *before* submit the patch. The benefit is that: If the patch passes the review, the maintainer may merge it directly. There is no need to trigger another round CI-fix, V2 patch submission and review. Thank you Yao, Jiewen > -----Original Message----- > From: joeyli <jlee@suse.com> > Sent: Tuesday, December 20, 2022 3:10 PM > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com> > Cc: Lee, Chun-Yi <joeyli.kernel@gmail.com>; Xu, Min M > <min.m.xu@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Tom > Lendacky <thomas.lendacky@amd.com>; James Bottomley > <jejb@linux.ibm.com>; Aktas, Erdem <erdemaktas@google.com> > Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity > checking failed of NvVarStore in some cases > > Hi Jiewen, > > Sorry for I didn't create tiano CI on github in time. And thanks for your > help to merge my patch. > > I will create tiano CI in next time after getting our Review-by tag. > > Thanks! > Joey Lee > > On Sat, Dec 17, 2022 at 03:17:42AM +0000, Yao, Jiewen via groups.io wrote: > > Thanks for the fix. > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> > > > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > > > > > -----Original Message----- > > > From: Lee, Chun-Yi <joeyli.kernel@gmail.com> > > > Sent: Thursday, December 15, 2022 10:27 PM > > > To: devel@edk2.groups.io > > > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > > > <kraxel@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom > Lendacky > > > <thomas.lendacky@amd.com>; James Bottomley <jejb@linux.ibm.com>; > > > Aktas, Erdem <erdemaktas@google.com>; Lee, Chun-Yi <jlee@suse.com> > > > Subject: [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed > of > > > NvVarStore in some cases > > > > > > In the commit 4f173db8b4 "OvmfPkg/PlatformInitLib: Add functions for > > > EmuVariableNvStore", it introduced a PlatformValidateNvVarStore() > function > > > for checking the integrity of NvVarStore. > > > > > > In some cases when the VariableHeader->StartId is VARIABLE_DATA, the > > > VariableHeader->State is not just one of the four primary states: > > > VAR_IN_DELETED_TRANSITION, VAR_DELETED, > VAR_HEADER_VALID_ONLY, > > > VAR_ADDED. > > > The state may combined two or three states, e.g. > > > > > > 0x3C = (VAR_IN_DELETED_TRANSITION & VAR_ADDED) & > VAR_DELETED > > > or > > > 0x3D = VAR_ADDED & VAR_DELETED > > > > > > When the variable store has those variables, system booting/rebooting > will > > > hangs in a ASSERT: > > > > > > NvVarStore Variable header State was invalid. > > > ASSERT > > > /mnt/working/source_code- > > > git/edk2/OvmfPkg/Library/PlatformInitLib/Platform.c(819): > > > ((BOOLEAN)(0==1)) > > > > > > Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), > we > > > saw some variables which have 0x3C or 0x3D state in store. > > > e.g. > > > > > > UpdateVariable(), VariableName=BootOrder > > > L1871, State=0000003F <-- VAR_ADDED > > > State &= VAR_DELETED=0000003D > > > FlushHobVariableToFlash(), VariableName=BootOrder > > > ... > > > UpdateVariable(), VariableName=InitialAttemptOrder > > > L1977, State=0000003F > > > State &= VAR_IN_DELETED_TRANSITION=0000003E > > > L2376, State=0000003E > > > State &= VAR_DELETED=0000003C > > > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder > > > ... > > > UpdateVariable(), VariableName=ConIn > > > L1977, State=0000003F > > > State &= VAR_IN_DELETED_TRANSITION=0000003E > > > L2376, State=0000003E > > > State &= VAR_DELETED=0000003C > > > FlushHobVariableToFlash(), VariableName=ConIn > > > ... > > > > > > So, only allowing the four primary states is not enough. This patch > changes > > > the falid states list (Follow Jiewen Yao's suggestion): > > > > > > 1. VAR_HEADER_VALID_ONLY (0x7F) > > > - Header added (*) > > > 2. VAR_ADDED (0x3F) > > > - Header + data added > > > 3. VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > > - marked as deleted, but still valid, before new data is added. (*) > > > 4. VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED (0x3C) > > > - deleted, after new data is added. > > > 5. VAR_ADDED & VAR_DELETED (0x3D) > > > - deleted directly, without new data. > > > (*) means to support surprise shutdown. > > > > > > And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) > because > > > they are > > > invalid states. > > > > > > v2: > > > Follow Jiewen Yao's suggestion to add the following valid states: > > > VAR_ADDED & VAR_DELETED (0x3D) > > > VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > > VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED > > > (0x3C) > > > and removed the following invalid states: > > > VAR_IN_DELETED_TRANSITION > > > VAR_DELETED > > > > > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > > > --- > > > OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > > > b/OvmfPkg/Library/PlatformInitLib/Platform.c > > > index 77f22de046..6963c47e0b 100644 > > > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > > > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > > > @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( > > > > > > VariableOffset = NvVarStoreHeader->Size - sizeof > > > (VARIABLE_STORE_HEADER); > > > } else { > > > - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > > > - (VariableHeader->State == VAR_DELETED) || > > > - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > > - (VariableHeader->State == VAR_ADDED))) > > > + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > > + (VariableHeader->State == VAR_ADDED) || > > > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > > > + (VariableHeader->State == (VAR_ADDED & > > > VAR_IN_DELETED_TRANSITION)) || > > > + (VariableHeader->State == (VAR_ADDED & > > > VAR_IN_DELETED_TRANSITION & VAR_DELETED)))) > > > { > > > DEBUG ((DEBUG_ERROR, "NvVarStore Variable header State was > > > invalid.\n")); > > > return FALSE; > > > -- > > > 2.35.3 > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97685): https://edk2.groups.io/g/devel/message/97685 Mute This Topic: https://groups.io/mt/95724215/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Jiewen, Thanks for your information. I will follow the process. Joey Lee On Wed, Dec 21, 2022 at 08:46:04AM +0000, Yao, Jiewen wrote: > Hi Joey > You are welcome. Thanks to catch the issue and provide the patch. > > One thing to clarify: The patch submitter is expected to pass CI, *before* submit the patch. > > The benefit is that: If the patch passes the review, the maintainer may merge it directly. > > There is no need to trigger another round CI-fix, V2 patch submission and review. > > Thank you > Yao, Jiewen > > > -----Original Message----- > > From: joeyli <jlee@suse.com> > > Sent: Tuesday, December 20, 2022 3:10 PM > > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com> > > Cc: Lee, Chun-Yi <joeyli.kernel@gmail.com>; Xu, Min M > > <min.m.xu@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Tom > > Lendacky <thomas.lendacky@amd.com>; James Bottomley > > <jejb@linux.ibm.com>; Aktas, Erdem <erdemaktas@google.com> > > Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity > > checking failed of NvVarStore in some cases > > > > Hi Jiewen, > > > > Sorry for I didn't create tiano CI on github in time. And thanks for your > > help to merge my patch. > > > > I will create tiano CI in next time after getting our Review-by tag. > > > > Thanks! > > Joey Lee > > > > On Sat, Dec 17, 2022 at 03:17:42AM +0000, Yao, Jiewen via groups.io wrote: > > > Thanks for the fix. > > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> > > > > > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > > > > > > > > > -----Original Message----- > > > > From: Lee, Chun-Yi <joeyli.kernel@gmail.com> > > > > Sent: Thursday, December 15, 2022 10:27 PM > > > > To: devel@edk2.groups.io > > > > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > > > > <kraxel@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom > > Lendacky > > > > <thomas.lendacky@amd.com>; James Bottomley <jejb@linux.ibm.com>; > > > > Aktas, Erdem <erdemaktas@google.com>; Lee, Chun-Yi <jlee@suse.com> > > > > Subject: [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed > > of > > > > NvVarStore in some cases > > > > > > > > In the commit 4f173db8b4 "OvmfPkg/PlatformInitLib: Add functions for > > > > EmuVariableNvStore", it introduced a PlatformValidateNvVarStore() > > function > > > > for checking the integrity of NvVarStore. > > > > > > > > In some cases when the VariableHeader->StartId is VARIABLE_DATA, the > > > > VariableHeader->State is not just one of the four primary states: > > > > VAR_IN_DELETED_TRANSITION, VAR_DELETED, > > VAR_HEADER_VALID_ONLY, > > > > VAR_ADDED. > > > > The state may combined two or three states, e.g. > > > > > > > > 0x3C = (VAR_IN_DELETED_TRANSITION & VAR_ADDED) & > > VAR_DELETED > > > > or > > > > 0x3D = VAR_ADDED & VAR_DELETED > > > > > > > > When the variable store has those variables, system booting/rebooting > > will > > > > hangs in a ASSERT: > > > > > > > > NvVarStore Variable header State was invalid. > > > > ASSERT > > > > /mnt/working/source_code- > > > > git/edk2/OvmfPkg/Library/PlatformInitLib/Platform.c(819): > > > > ((BOOLEAN)(0==1)) > > > > > > > > Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), > > we > > > > saw some variables which have 0x3C or 0x3D state in store. > > > > e.g. > > > > > > > > UpdateVariable(), VariableName=BootOrder > > > > L1871, State=0000003F <-- VAR_ADDED > > > > State &= VAR_DELETED=0000003D > > > > FlushHobVariableToFlash(), VariableName=BootOrder > > > > ... > > > > UpdateVariable(), VariableName=InitialAttemptOrder > > > > L1977, State=0000003F > > > > State &= VAR_IN_DELETED_TRANSITION=0000003E > > > > L2376, State=0000003E > > > > State &= VAR_DELETED=0000003C > > > > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder > > > > ... > > > > UpdateVariable(), VariableName=ConIn > > > > L1977, State=0000003F > > > > State &= VAR_IN_DELETED_TRANSITION=0000003E > > > > L2376, State=0000003E > > > > State &= VAR_DELETED=0000003C > > > > FlushHobVariableToFlash(), VariableName=ConIn > > > > ... > > > > > > > > So, only allowing the four primary states is not enough. This patch > > changes > > > > the falid states list (Follow Jiewen Yao's suggestion): > > > > > > > > 1. VAR_HEADER_VALID_ONLY (0x7F) > > > > - Header added (*) > > > > 2. VAR_ADDED (0x3F) > > > > - Header + data added > > > > 3. VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > > > - marked as deleted, but still valid, before new data is added. (*) > > > > 4. VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED (0x3C) > > > > - deleted, after new data is added. > > > > 5. VAR_ADDED & VAR_DELETED (0x3D) > > > > - deleted directly, without new data. > > > > (*) means to support surprise shutdown. > > > > > > > > And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) > > because > > > > they are > > > > invalid states. > > > > > > > > v2: > > > > Follow Jiewen Yao's suggestion to add the following valid states: > > > > VAR_ADDED & VAR_DELETED (0x3D) > > > > VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > > > VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED > > > > (0x3C) > > > > and removed the following invalid states: > > > > VAR_IN_DELETED_TRANSITION > > > > VAR_DELETED > > > > > > > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > > > > --- > > > > OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- > > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > > > > b/OvmfPkg/Library/PlatformInitLib/Platform.c > > > > index 77f22de046..6963c47e0b 100644 > > > > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > > > > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > > > > @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( > > > > > > > > VariableOffset = NvVarStoreHeader->Size - sizeof > > > > (VARIABLE_STORE_HEADER); > > > > } else { > > > > - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > > > > - (VariableHeader->State == VAR_DELETED) || > > > > - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > > > - (VariableHeader->State == VAR_ADDED))) > > > > + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > > > + (VariableHeader->State == VAR_ADDED) || > > > > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > > > > + (VariableHeader->State == (VAR_ADDED & > > > > VAR_IN_DELETED_TRANSITION)) || > > > > + (VariableHeader->State == (VAR_ADDED & > > > > VAR_IN_DELETED_TRANSITION & VAR_DELETED)))) > > > > { > > > > DEBUG ((DEBUG_ERROR, "NvVarStore Variable header State was > > > > invalid.\n")); > > > > return FALSE; > > > > -- > > > > 2.35.3 > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97727): https://edk2.groups.io/g/devel/message/97727 Mute This Topic: https://groups.io/mt/95724215/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.