Re: [edk2-devel] How do debug tianocore.PatchCheck failed?
Hi Leif, Thanks for your response! On Tue, Jul 02, 2024 at 11:32:06AM +0100, Leif Lindholm wrote: > Hi Joey, > > On 2024-07-02 07:42, joeyli via groups.io wrote: > > Hi EDK2 experts, > > > > I was filed a submit request on github for > > "[PATCH] EmbeddedPkg/VirtualRealTimeClockLib: Support SOURCE_DATE_EPOCH". > > But > > I got a Azure Pipelines/tianocore.PatchCheck failed: > > > > https://github.com/tianocore/edk2/pull/5550/checks?check_run_id=24185647984 > > > > Check failure on line 26 in Build log > > @azure-pipelines azure-pipelines / tianocore.PatchCheck > > Build log #L26 > > Bash exited with code '255'. > > The CI system is a bit "beware of the leopard" with regards to finding the > failures. > > If I go to the link above, I see the message you're copying, but below that > there is a link "View more details on Azure Pipelines". Clicking that takes > me to a page that says "build not found" - since the logs have been purged > since the test was run 2 months ago. Otherwise you'd be able to find logs > there explaining the failure. > Actually I didn't aware my submission is failed because I moved to work on other bugs. > > The patch only change one line as following bash script: > > > > --- > > a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > > +++ > > b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > > @@ -34,4 +34,4 @@ > > # Current usage of this library expects GCC in a UNIX-like shell > > environment with the date command > > [BuildOptions] > > - GCC:*_*_*_CC_FLAGS = -DBUILD_EPOCH=`date +%s` > > + GCC:*_*_*_CC_FLAGS = -DBUILD_EPOCH=`printenv SOURCE_DATE_EPOCH || date > > +%s`^M > > This looks fine. > > If I was to guess, from looking at the PR, I would say PatchCheck.py is > unhappy over the merge commit that has been introduced. > If you rebase your change on top of current master branch and force push an > update, that will trigger a new CI run, which I'm thinking would pass. > > Best Regards, > > Leif Thanks for your suggest! I will rebase my change and submit a new merge request. Joey Lee > > > > > This change works on openSUSE/SLE, and it also passed > > PlatformCI_ArmVirtPkg_Ubuntu_GCC5_PR test. > > > > My question is: > > What's the platform of "Azure Pipelines/tianocore.PatchCheck" ? Is it a > > virtual machine? Where can I find the platform for debugging my change? > > > > Thank a lot! > > Joey Lee > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#119761): https://edk2.groups.io/g/devel/message/119761 Mute This Topic: https://groups.io/mt/106995552/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] How do debug tianocore.PatchCheck failed?
Hi EDK2 experts, I was filed a submit request on github for "[PATCH] EmbeddedPkg/VirtualRealTimeClockLib: Support SOURCE_DATE_EPOCH". But I got a Azure Pipelines/tianocore.PatchCheck failed: https://github.com/tianocore/edk2/pull/5550/checks?check_run_id=24185647984 Check failure on line 26 in Build log @azure-pipelines azure-pipelines / tianocore.PatchCheck Build log #L26 Bash exited with code '255'. The patch only change one line as following bash script: --- a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf +++ b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf @@ -34,4 +34,4 @@ # Current usage of this library expects GCC in a UNIX-like shell environment with the date command [BuildOptions] - GCC:*_*_*_CC_FLAGS = -DBUILD_EPOCH=`date +%s` + GCC:*_*_*_CC_FLAGS = -DBUILD_EPOCH=`printenv SOURCE_DATE_EPOCH || date +%s`^M This change works on openSUSE/SLE, and it also passed PlatformCI_ArmVirtPkg_Ubuntu_GCC5_PR test. My question is: What's the platform of "Azure Pipelines/tianocore.PatchCheck" ? Is it a virtual machine? Where can I find the platform for debugging my change? Thank a lot! Joey Lee -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#119754): https://edk2.groups.io/g/devel/message/119754 Mute This Topic: https://groups.io/mt/106995552/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] EmbeddedPkg/VirtualRealTimeClockLib: Support SOURCE_DATE_EPOCH
Hi all, On Fri, Apr 12, 2024 at 06:03:35PM +0800, joeyli via groups.io wrote: > Hi experts, > > On Fri, Apr 12, 2024 at 03:25:56PM +0800, Lee, Chun-Yi wrote: > > From: Chun-Yi Lee > > > > RISC-V ovmf used VirtualRealTimeClockLib but the default epoch is a > > compilation time. It causes that the RISC-V ovmf binary image is NOT > > reproducible. > > > > This patch added the support of SOURCE_DATE_EPOCH by printenv command. > > If SOURCE_DATE_EPOCH be found then we use it as BUILD_EPOCH. Otherwise > > we run date command for setting BUILD_EPOCH. > > > > For distributions want a reproducible RISC-V ovmf image, they should > > export SOURCE_DATE_EPOCH environment variable before building ovmf. > > > > References: https://reproducible-builds.org/docs/source-date-epoch/ > > Cc: Pete Batard > > Cc: Ard Biesheuvel > > Signed-off-by: Chun-Yi Lee > > I have filed pull request: > > https://github.com/tianocore/edk2/pull/5550 > Does anyone have suggestion against this patch. Or I missed anything for the submit request? Thanks! Joey Lee > > > --- > > .../Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git > > a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > > b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > > index 5d0f867..285e880 100644 > > --- > > a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > > +++ > > b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > > @@ -34,4 +34,4 @@ > > > > # Current usage of this library expects GCC in a UNIX-like shell > > environment with the date command > > [BuildOptions] > > - GCC:*_*_*_CC_FLAGS = -DBUILD_EPOCH=`date +%s` > > + GCC:*_*_*_CC_FLAGS = -DBUILD_EPOCH=`printenv SOURCE_DATE_EPOCH || date > > +%s` > > -- > > 2.35.3 > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118444): https://edk2.groups.io/g/devel/message/118444 Mute This Topic: https://groups.io/mt/105479031/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] EmbeddedPkg/VirtualRealTimeClockLib: Support SOURCE_DATE_EPOCH
Hi experts, On Fri, Apr 12, 2024 at 03:25:56PM +0800, Lee, Chun-Yi wrote: > From: Chun-Yi Lee > > RISC-V ovmf used VirtualRealTimeClockLib but the default epoch is a > compilation time. It causes that the RISC-V ovmf binary image is NOT > reproducible. > > This patch added the support of SOURCE_DATE_EPOCH by printenv command. > If SOURCE_DATE_EPOCH be found then we use it as BUILD_EPOCH. Otherwise > we run date command for setting BUILD_EPOCH. > > For distributions want a reproducible RISC-V ovmf image, they should > export SOURCE_DATE_EPOCH environment variable before building ovmf. > > References: https://reproducible-builds.org/docs/source-date-epoch/ > Cc: Pete Batard > Cc: Ard Biesheuvel > Signed-off-by: Chun-Yi Lee I have filed pull request: https://github.com/tianocore/edk2/pull/5550 Thanks! Joey Lee > --- > .../Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git > a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > index 5d0f867..285e880 100644 > --- a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > +++ b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > @@ -34,4 +34,4 @@ > > # Current usage of this library expects GCC in a UNIX-like shell environment > with the date command > [BuildOptions] > - GCC:*_*_*_CC_FLAGS = -DBUILD_EPOCH=`date +%s` > + GCC:*_*_*_CC_FLAGS = -DBUILD_EPOCH=`printenv SOURCE_DATE_EPOCH || date +%s` > -- > 2.35.3 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117665): https://edk2.groups.io/g/devel/message/117665 Mute This Topic: https://groups.io/mt/105479031/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] OvmfPkg/SmbiosPlatformDxe: tweak fallback release date again
Hi Laszlo,
On Tue, Mar 05, 2024 at 09:53:33AM +0100, Laszlo Ersek wrote:
> On 3/4/24 12:37, joeyli via groups.io wrote:
> > Hi,
> >
> > On Wed, Feb 07, 2024 at 04:02:52PM +0800, joeyli via groups.io wrote:
> >> On Wed, Feb 07, 2024 at 03:55:49PM +0800, joeyli wrote:
> >>> Hi Laszlo,
> >>>
> >>> First, thanks for your review!
> >>>
> >>> On Mon, Feb 05, 2024 at 05:41:25PM +0100, Laszlo Ersek wrote:
> >>>> On 2/4/24 10:29, Lee, Chun-Yi wrote:
> >>>>> In case PcdFirmwareReleaseDateString is not set use a valid date
> >>>>> as fallback. But the default valid date can _NOT_ pass the Microsoft
> >>>>> SVVP test "Check SMBIOS Table Specific Requirements". The test emitted
> >>>>> the error message:
> >>>>>
> >>>>> BIOS Release Date string is unexpected length: 8. This string must be in
> >>>>> MM/DD/ format. No other format is allowed and no additional
> >>>>> information
> >>>>> may be included. See field description in the SMBIOS specification.
> >>>>>
> >>>>> Base on SMBIOS spec v3.7.0:
> >>>>>
> >>>>> 08h 2.0+BIOS Release Date BYTESTRING
> >>>>> String number of the BIOS release date. The date
> >>>>> string, if supplied, is in either mm/dd/yy or
> >>>>> mm/dd/ format. If the year portion of the string
> >>>>> is two digits, the year is assumed to be 19yy.
> >>>>> NOTE: The mm/dd/ format is required for SMBIOS
> >>>>> version 2.3 and later.
> >>>>>
> >>>>> So, let's tweek the fallback release date again.
> >>>>>
> >>>>> Fixes: a0f9628705e3 ("OvmfPkg/SmbiosPlatformDxe: tweak fallback release
> >>>>> date") [edk2-stable202305~327]
> >>>>> Signed-off-by: "Lee, Chun-Yi"
> >>>>> ---
> >>>>> OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> >>>>> b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> >>>>> index 0ca3776045..e929da6b81 100644
> >>>>> --- a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> >>>>> +++ b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> >>>>> @@ -160,7 +160,7 @@ InstallAllStructures (
> >>>>> DateStr = (CHAR16 *)FixedPcdGetPtr (PcdFirmwareReleaseDateString);
> >>>>> DateLen = StrLen (DateStr);
> >>>>> if (DateLen < 3) {
> >>>>> - DateStr = L"2/2/2022";
> >>>>> + DateStr = L"02/02/2022";
> >>>>>DateLen = StrLen (DateStr);
> >>>>> }
> >>>>>
> >>>>
> >>>> Are you proposing this as an important (but low risk) bugfix that might
> >>>> qualify for the freeze(s)? If so, please loop in Liming and Mike.
> >>>>
> >>>
> >>> hm... What does freeze mean?
> >>>
> >>
> >> ah... You mean soft feature freeze for edk2-stable202402.
> >>
> >> Hi Liming, Michael,
> >>
> >> This change is important but low risk. Could you please consider to add it
> >> to edk2-stable202402 release?
> >>
> >> Thanks a lot!
> >> Joey Lee
> >
> > This patch is not in edk2-stable202402. Will it to be merged to next
> > release?
>
> Thanks for the reminder, and sorry about the delay!
>
> Merged as commit 2a0d4a2641a7, via
> <https://github.com/tianocore/edk2/pull/5441>.
Thanks for your review and merge!
>
> For future contributions: please run PatchCheck.py on the patch series
> before formatting and posting it (better yet, submit a personal CI build
> PR).
>
For 'submit personal CI build PR', I was created a pull requests on github:
https://github.com/tianocore/edk2/pull/5349
Is it the right approach for triggering a CI build PR?
Thanks a lot!
Joey Lee
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116478): https://edk2.groups.io/g/devel/message/116478
Mute This Topic: https://groups.io/mt/104153758/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] OvmfPkg/SmbiosPlatformDxe: tweak fallback release date again
Hi,
On Wed, Feb 07, 2024 at 04:02:52PM +0800, joeyli via groups.io wrote:
> On Wed, Feb 07, 2024 at 03:55:49PM +0800, joeyli wrote:
> > Hi Laszlo,
> >
> > First, thanks for your review!
> >
> > On Mon, Feb 05, 2024 at 05:41:25PM +0100, Laszlo Ersek wrote:
> > > On 2/4/24 10:29, Lee, Chun-Yi wrote:
> > > > In case PcdFirmwareReleaseDateString is not set use a valid date
> > > > as fallback. But the default valid date can _NOT_ pass the Microsoft
> > > > SVVP test "Check SMBIOS Table Specific Requirements". The test emitted
> > > > the error message:
> > > >
> > > > BIOS Release Date string is unexpected length: 8. This string must be in
> > > > MM/DD/ format. No other format is allowed and no additional
> > > > information
> > > > may be included. See field description in the SMBIOS specification.
> > > >
> > > > Base on SMBIOS spec v3.7.0:
> > > >
> > > > 08h 2.0+BIOS Release Date BYTESTRING
> > > > String number of the BIOS release date. The date
> > > > string, if supplied, is in either mm/dd/yy or
> > > > mm/dd/ format. If the year portion of the string
> > > > is two digits, the year is assumed to be 19yy.
> > > > NOTE: The mm/dd/ format is required for SMBIOS
> > > > version 2.3 and later.
> > > >
> > > > So, let's tweek the fallback release date again.
> > > >
> > > > Fixes: a0f9628705e3 ("OvmfPkg/SmbiosPlatformDxe: tweak fallback release
> > > > date") [edk2-stable202305~327]
> > > > Signed-off-by: "Lee, Chun-Yi"
> > > > ---
> > > > OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> > > > b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> > > > index 0ca3776045..e929da6b81 100644
> > > > --- a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> > > > +++ b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> > > > @@ -160,7 +160,7 @@ InstallAllStructures (
> > > > DateStr = (CHAR16 *)FixedPcdGetPtr (PcdFirmwareReleaseDateString);
> > > > DateLen = StrLen (DateStr);
> > > > if (DateLen < 3) {
> > > > - DateStr = L"2/2/2022";
> > > > + DateStr = L"02/02/2022";
> > > >DateLen = StrLen (DateStr);
> > > > }
> > > >
> > >
> > > Are you proposing this as an important (but low risk) bugfix that might
> > > qualify for the freeze(s)? If so, please loop in Liming and Mike.
> > >
> >
> > hm... What does freeze mean?
> >
>
> ah... You mean soft feature freeze for edk2-stable202402.
>
> Hi Liming, Michael,
>
> This change is important but low risk. Could you please consider to add it
> to edk2-stable202402 release?
>
> Thanks a lot!
> Joey Lee
This patch is not in edk2-stable202402. Will it to be merged to next release?
Thanks a lot!
Joey Lee
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116311): https://edk2.groups.io/g/devel/message/116311
Mute This Topic: https://groups.io/mt/104153758/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Get processor extended information for SmmCpuServiceProtocol
Hi Jiaxin,
Thanks for your reminder! I have tested Gerd's patch and it works to me!
Joey Lee
On Wed, Nov 15, 2023 at 11:30:07AM +, Wu, Jiaxin wrote:
> Hi Joey,
>
> Please check your local code whether has the commit
> 170d4ce8e90abb1eff03852940a69c9d17f8afe5 from Gerd,
>
> Assert in 1478 means you don't have that patch.
>
> Besides, I'm also porting the change to BaseXApicLib, see patch:
> https://edk2.groups.io/g/devel/message/111257
>
> Thanks,
> Jiaxin
>
> > -Original Message-
> > From: joeyli
> > Sent: Wednesday, November 15, 2023 3:35 PM
> > To: devel@edk2.groups.io; Zhang, Hongbin1
> > Cc: Dong, Eric ; Ni, Ray ; Kumar,
> > Rahul R ; Gerd Hoffmann ;
> > Zeng, Star ; Wu, Jiaxin
> > Subject: Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Get processor extended
> > information for SmmCpuServiceProtocol
> >
> > Hi Hongbin1,
> >
> > On Mon, May 29, 2023 at 02:39:38PM +0800, Zhang, Hongbin1 via groups.io
> > wrote:
> > > Some features like RAS need to use processor extended information
> > > under smm, So add code to support it
> > >
> > > Signed-off-by: Hongbin1 Zhang
> >
> > I got a ASSERT when booting edk2-stable202308 on a issue machine:
> >
> > ASSERT /home/joeyli/source_code-
> > git/edk2/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c(147
> > 8): (Index != 0) || (LevelType == 0x01)
> >
> > And, the ASSERT can also be reproduced on edk2 master. After reverted this
> > patch, the ASSERT is gone.
> >
> > I have filed a bug here:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=4598
> >
> > I have put some tracing information on bugzilla.
> >
> > Thank a lot!
> > Joey Lee
> >
> > > Cc: Eric Dong
> > > Cc: Ray Ni
> > > Cc: Rahul Kumar
> > > Cc: Gerd Hoffmann
> > > Cc: Star Zeng
> > > Cc: Jiaxin Wu
> > > ---
> > > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> > > index c0e368ea94..8311c3b9de 100644
> > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> > > @@ -929,7 +929,7 @@ PiCpuSmmEntry (
> > > gSmmCpuPrivate->Operation[Index]= SmmCpuNone;
> > >
> > > if (Index < mNumberOfCpus) {
> > > - Status = MpServices->GetProcessorInfo (MpServices, Index,
> > >ProcessorInfo[Index]);
> > > + Status = MpServices->GetProcessorInfo (MpServices, Index |
> > CPU_V2_EXTENDED_TOPOLOGY, >ProcessorInfo[Index]);
> > >ASSERT_EFI_ERROR (Status);
> > >mCpuHotPlugData.ApicId[Index] = gSmmCpuPrivate-
> > >ProcessorInfo[Index].ProcessorId;
> > >
> > > --
> > > 2.37.0.windows.1
> > >
> > >
> > >
> > >
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111269): https://edk2.groups.io/g/devel/message/111269
Mute This Topic: https://groups.io/mt/99209786/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Get processor extended information for SmmCpuServiceProtocol
Hi Hongbin1,
On Mon, May 29, 2023 at 02:39:38PM +0800, Zhang, Hongbin1 via groups.io wrote:
> Some features like RAS need to use processor extended information
> under smm, So add code to support it
>
> Signed-off-by: Hongbin1 Zhang
I got a ASSERT when booting edk2-stable202308 on a issue machine:
ASSERT
/home/joeyli/source_code-git/edk2/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c(1478):
(Index != 0) || (LevelType == 0x01)
And, the ASSERT can also be reproduced on edk2 master. After reverted this
patch, the ASSERT is gone.
I have filed a bug here:
https://bugzilla.tianocore.org/show_bug.cgi?id=4598
I have put some tracing information on bugzilla.
Thank a lot!
Joey Lee
> Cc: Eric Dong
> Cc: Ray Ni
> Cc: Rahul Kumar
> Cc: Gerd Hoffmann
> Cc: Star Zeng
> Cc: Jiaxin Wu
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index c0e368ea94..8311c3b9de 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -929,7 +929,7 @@ PiCpuSmmEntry (
> gSmmCpuPrivate->Operation[Index]= SmmCpuNone;
>
> if (Index < mNumberOfCpus) {
> - Status = MpServices->GetProcessorInfo (MpServices, Index,
> >ProcessorInfo[Index]);
> + Status = MpServices->GetProcessorInfo (MpServices, Index |
> CPU_V2_EXTENDED_TOPOLOGY, >ProcessorInfo[Index]);
>ASSERT_EFI_ERROR (Status);
>mCpuHotPlugData.ApicId[Index] =
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId;
>
> --
> 2.37.0.windows.1
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111242): https://edk2.groups.io/g/devel/message/111242
Mute This Topic: https://groups.io/mt/99209786/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest
Hi Min Xu, On Fri, Apr 07, 2023 at 01:56:05AM +, Min Xu via groups.io wrote: > On Friday, April 7, 2023 4:29 AM, Tom Lendacky wrote: > > On 4/5/23 20:42, Xu, Min M wrote: > > > On April 3, 2023 7:21 PM, Gerd Hoffmann wrote: > > I agree that the efi variable store is not secure without smm. But > > after 58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE > > doesn't > > work with SEV. System just hangs in "NvVarStore FV headers were > > invalid." > > >>> Hi, Joeyli > > >>> ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is > > >>> skipped > > >> and an error code is returned. So system will not hang. > > >>> So another solution is simply remove the ASSERT. Then an error > > >>> message is > > >> dumped out and system continues. > > >>> > > >>> @Gerd Hoffmann @Tom Lendacky @joeyli What's your thought? > > >> > > >> Maybe we just need to call ReserveEmuVariableNvStore a bit later? > > >> > > > I think we can still call ReserveEmuVariableNvStore at PEI phase, but > > > move the initialization of EmuVariableNvStore to > > > > > https://github.com/tianocore/edk2/blob/master/OvmfPkg/EmuVariableFvbR > > u > > > ntimeDxe/Fvb.c#L780-L783 @Tom Lendacky At this moment, is SEV guest > > > available to read the content from VarStore? > > > > It's quite possible. If you can work up a quick patch, I'll test it out. > > > Yes, the patch is uploaded here > https://bugzilla.tianocore.org/show_bug.cgi?id=4379#c17 > I have tested new patch. The issue is not produced, but after I applied debug patch. Looks that the InitializeFvAndVariableStoreForSecureBoot() not be called. I have put detail log on bugzilla. Thanks a lot! Joey Lee -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#102704): https://edk2.groups.io/g/devel/message/102704 Mute This Topic: https://groups.io/mt/97922617/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest
On Mon, Apr 03, 2023 at 12:21:38AM +, Xu, Min M wrote: > On Friday, March 31, 2023 10:49 PM, Joeyli wrote: > > On Fri, Mar 31, 2023 at 10:25:09AM +0200, Gerd Hoffmann wrote: > > > On Fri, Mar 31, 2023 at 03:59:56PM +0800, joeyli wrote: > > > > Hi Gerd, > > > > > > > > On Thu, Mar 30, 2023 at 09:50:53AM +0200, Gerd Hoffmann wrote: > > > > > On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote: > > > > > > From: Min M Xu > > > > > > > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379 > > > > > > > > > > > > PlatformInitEmuVariableNvStore is called to initialize the > > > > > > EmuVariableNvStore with the content pointed by > > > > > > PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is > > > > > > launched with -bios parameter, UEFI variables will be partially > > > > > > emulated, and non-volatile variables may lose their contents > > > > > > after a reboot. This makes the secure boot feature not working. > > > > > > > > > > > > But in SEV guest, this design doesn't work. Because at this > > > > > > point the variable store mapping is still private/encrypted, > > > > > > OVMF will see ciphertext. So we skip the call of > > > > > > PlatformInitEmuVariableNvStore in SEV guest. > > > > > > > > > > I'd suggest to simply build without -D SECURE_BOOT_ENABLE instead. > > > > > Without initializing the emu var store you will not get a > > > > > functional secure boot setup anyway. > > > > > > > > In our case, we already shipped ovmf with -D SECURE_BOOT_ENABLE in a > > > > couple of versions. Removing it will causes problem in VM live > > > > migration. > > > > > > Hmm? qemu live-migrates the rom image too. Only after poweroff and > > > reboot the guest will see an updated firmware image. > > > > > > > Thanks for your explanation. Understood. > > > > > > I will prefer Min M's solution, until SEV experts found better > > > > solution. > > > > > > I'd prefer to not poke holes into secure boot. Re-Initializing the > > > emu var store from rom on each reset is also needed for security > > > reasons in case the efi variable store is not in smm-protected flash > > > memory. > > > > > > > I agree that the efi variable store is not secure without smm. But after > > 58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE doesn't work > > with SEV. System just hangs in "NvVarStore FV headers were invalid." > Hi, Joeyli > ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is skipped > and an error code is returned. So system will not hang. > So another solution is simply remove the ASSERT. Then an error message is > dumped out and system continues. > Ah! You are right. I forgot that I enabled debug mode. > @Gerd Hoffmann @Tom Lendacky @joeyli What's your thought? > Removing ASSERT in debug mode can workaround problem. Looks that it just hide a problem. Thanks! Joey Lee -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#102677): https://edk2.groups.io/g/devel/message/102677 Mute This Topic: https://groups.io/mt/97922617/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest
On Fri, Mar 31, 2023 at 10:25:09AM +0200, Gerd Hoffmann wrote: > On Fri, Mar 31, 2023 at 03:59:56PM +0800, joeyli wrote: > > Hi Gerd, > > > > On Thu, Mar 30, 2023 at 09:50:53AM +0200, Gerd Hoffmann wrote: > > > On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote: > > > > From: Min M Xu > > > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379 > > > > > > > > PlatformInitEmuVariableNvStore is called to initialize the > > > > EmuVariableNvStore with the content pointed by > > > > PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is launched > > > > with -bios parameter, UEFI variables will be partially emulated, and > > > > non-volatile variables may lose their contents after a reboot. This > > > > makes > > > > the secure boot feature not working. > > > > > > > > But in SEV guest, this design doesn't work. Because at this point the > > > > variable store mapping is still private/encrypted, OVMF will see > > > > ciphertext. So we skip the call of PlatformInitEmuVariableNvStore in > > > > SEV guest. > > > > > > I'd suggest to simply build without -D SECURE_BOOT_ENABLE instead. > > > Without initializing the emu var store you will not get a functional > > > secure boot setup anyway. > > > > In our case, we already shipped ovmf with -D SECURE_BOOT_ENABLE in a couple > > of versions. Removing it will causes problem in VM live migration. > > Hmm? qemu live-migrates the rom image too. Only after poweroff and > reboot the guest will see an updated firmware image. > Thanks for your explanation. Understood. > > I will prefer Min M's solution, until SEV experts found better > > solution. > > I'd prefer to not poke holes into secure boot. Re-Initializing the emu > var store from rom on each reset is also needed for security reasons in > case the efi variable store is not in smm-protected flash memory. > I agree that the efi variable store is not secure without smm. But after 58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE doesn't work with SEV. System just hangs in "NvVarStore FV headers were invalid." If secure boot can not work with SEV (even it is not really secure), why not just block the building process when SEV with SECURE_BOOT_ENABLE? At least the issue will not happen at runtime. Thanks Joey Lee -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#102316): https://edk2.groups.io/g/devel/message/102316 Mute This Topic: https://groups.io/mt/97922617/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest
Hi Gerd, On Thu, Mar 30, 2023 at 09:50:53AM +0200, Gerd Hoffmann wrote: > On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote: > > From: Min M Xu > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379 > > > > PlatformInitEmuVariableNvStore is called to initialize the > > EmuVariableNvStore with the content pointed by > > PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is launched > > with -bios parameter, UEFI variables will be partially emulated, and > > non-volatile variables may lose their contents after a reboot. This makes > > the secure boot feature not working. > > > > But in SEV guest, this design doesn't work. Because at this point the > > variable store mapping is still private/encrypted, OVMF will see > > ciphertext. So we skip the call of PlatformInitEmuVariableNvStore in > > SEV guest. > > I'd suggest to simply build without -D SECURE_BOOT_ENABLE instead. > Without initializing the emu var store you will not get a functional > secure boot setup anyway. > In our case, we already shipped ovmf with -D SECURE_BOOT_ENABLE in a couple of versions. Removing it will causes problem in VM live migration. I will prefer Min M's solution, until SEV experts found better solution. Thank! Joey Lee -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#102247): https://edk2.groups.io/g/devel/message/102247 Mute This Topic: https://groups.io/mt/97922617/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V5 4/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore
Hi Min M Xu, I have filed a EDK2 bug relates to this patch: Bug 4379 - Got NvVarStore FV headers were invalid when using OVMF with AMD SEV https://bugzilla.tianocore.org/show_bug.cgi?id=4379 I got a "NvVarStore FV headers were invalid." assert when using OVMF with AMD SEV. After reverted this patch, the assert is gone. Thanks! Joey Lee On Tue, Sep 06, 2022 at 12:35:56PM +0800, Min Xu via groups.io wrote: > From: Min M Xu > > ReserveEmuVariableNvStore is updated with below 2 functions defined in > PlatformInitLib: > - PlatformReserveEmuVariableNvStore > - PlatformInitEmuVariableNvStore > > PlatformInitEmuVariableNvStore works when secure boot feature is enabled. > This is because secure boot needs the EFI variables (PK/KEK/DB/DBX, etc) > and EmuVariableNvStore is cleared when OVMF is launched with -bios > parameter. > > Cc: Erdem Aktas > Cc: James Bottomley > Cc: Jiewen Yao > Cc: Tom Lendacky > Cc: Gerd Hoffmann > Acked-by: Gerd Hoffmann > Signed-off-by: Min Xu > --- > OvmfPkg/PlatformPei/Platform.c | 25 +++-- > 1 file changed, 7 insertions(+), 18 deletions(-) > > diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c > index 009db67ee60a..b1f8140d6041 100644 > --- a/OvmfPkg/PlatformPei/Platform.c > +++ b/OvmfPkg/PlatformPei/Platform.c > @@ -220,24 +220,13 @@ ReserveEmuVariableNvStore ( >EFI_PHYSICAL_ADDRESS VariableStore; >RETURN_STATUS PcdStatus; > > - // > - // Allocate storage for NV variables early on so it will be > - // at a consistent address. Since VM memory is preserved > - // across reboots, this allows the NV variable storage to survive > - // a VM reboot. > - // > - VariableStore = > -(EFI_PHYSICAL_ADDRESS)(UINTN) > -AllocateRuntimePages ( > - EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) > - ); > - DEBUG (( > -DEBUG_INFO, > -"Reserved variable store memory: 0x%lX; size: %dkb\n", > -VariableStore, > -(2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024 > -)); > - PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore); > + VariableStore = > (EFI_PHYSICAL_ADDRESS)(UINTN)PlatformReserveEmuVariableNvStore (); > + PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore); > + > + #ifdef SECURE_BOOT_FEATURE_ENABLED > + PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore); > + #endif > + >ASSERT_RETURN_ERROR (PcdStatus); > } > > -- > 2.29.2.windows.2 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101482): https://edk2.groups.io/g/devel/message/101482 Mute This Topic: https://groups.io/mt/93494907/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 3/4] OvmfPkg/PlatformInitLib: dynamic mmio window size
Hi Gerd,
I got a page-fault in CpuIo2Dxe.dll when using edk2-stable202211 ovmf with
qemu-kvm:
X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID -
ExceptionData - 000B I:0 R:1 U:0 W:1 P:1 PK:0 SS:0 SGX:0
RIP - 7F2CB5E8, CS - 0038, RFLAGS - 00010246
RAX - , RCX - 0001, RDX - 7F2CC5D0
RBX - 03828014, RSP - 7FF04710, RBP - 7FF04790
RSI - , RDI -
R8 - 7FF04938, R9 - 0001, R10 - 0001
R11 - , R12 - 7FF04938, R13 - 0001
R14 - 0001, R15 - 7F2CC3EC
DS - 0030, ES - 0030, FS - 0030
GS - 0030, SS - 0030
CR0 - 80010033, CR2 - 03828014, CR3 - 7FC01000
CR4 - 0668, CR8 -
DR0 - , DR1 - , DR2 -
DR3 - , DR6 - 0FF0, DR7 - 0400
GDTR - 7F9DC000 0047, LDTR -
IDTR - 7F2CD018 0FFF, TR -
FXSAVE_STATE - 7FF04370
Find image based on IP(0x7F2CB5E8)
/home/joeyli/source_code-git/edk2/Build/OvmfX64/DEBUG_GCC5/X64/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe/DEBUG/CpuIo2Dxe.dll
(ImageBase=7F2CA000, EntryPoint=7F2CBDE8)
After reverted this ecb778d0ac62 patch, the page-fault is gone. And
edk2-stable202311 can also reproduce this CpuIo2Dxe.dll page-fault.
I have filed a edk2 bug against this situation:
Bug 4373 - Got Page-Fault in CpuIo2Dxe.dll when using edk2-stable202211 ovmf
with qemi-kvm
https://bugzilla.tianocore.org/show_bug.cgi?id=4373
Do you have any idea?
Thanks a lot!
Joey Lee
On Tue, Oct 04, 2022 at 03:47:27PM +0200, Gerd Hoffmann via groups.io wrote:
> In case we have a reliable PhysMemAddressWidth use that to dynamically
> size the 64bit address window. Allocate 1/8 of the physical address
> space and place the window at the upper end of the address space.
>
> Signed-off-by: Gerd Hoffmann
> ---
> OvmfPkg/Library/PlatformInitLib/MemDetect.c | 28 +
> 1 file changed, 28 insertions(+)
>
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 16ecbfadc30c..ae217d0242ed 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -604,6 +604,33 @@ PlatformAddressWidthFromCpuid (
>}
> }
>
> +VOID
> +EFIAPI
> +PlatformDynamicMmioWindow (
> + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + )
> +{
> + UINT64 AddrSpace, MmioSpace;
> +
> + AddrSpace = LShiftU64 (1, PlatformInfoHob->PhysMemAddressWidth);
> + MmioSpace = LShiftU64 (1, PlatformInfoHob->PhysMemAddressWidth - 3);
> +
> + if ((PlatformInfoHob->PcdPciMmio64Size < MmioSpace) &&
> + (PlatformInfoHob->PcdPciMmio64Base + MmioSpace < AddrSpace))
> + {
> +DEBUG ((DEBUG_INFO, "%a: using dynamic mmio window\n", __func__));
> +DEBUG ((DEBUG_INFO, "%a: Addr Space 0x%Lx (%Ld GB)\n", __func__,
> AddrSpace, RShiftU64 (AddrSpace, 30)));
> +DEBUG ((DEBUG_INFO, "%a: MMIO Space 0x%Lx (%Ld GB)\n", __func__,
> MmioSpace, RShiftU64 (MmioSpace, 30)));
> +PlatformInfoHob->PcdPciMmio64Size = MmioSpace;
> +PlatformInfoHob->PcdPciMmio64Base = AddrSpace - MmioSpace;
> + } else {
> +DEBUG ((DEBUG_INFO, "%a: using classic mmio window\n", __func__));
> + }
> +
> + DEBUG ((DEBUG_INFO, "%a: Pci64 Base 0x%Lx\n", __func__,
> PlatformInfoHob->PcdPciMmio64Base));
> + DEBUG ((DEBUG_INFO, "%a: Pci64 Size 0x%Lx\n", __func__,
> PlatformInfoHob->PcdPciMmio64Size));
> +}
> +
> /**
>Iterate over the PCI host bridges resources information optionally provided
>in fw-cfg and find the highest address contained in the PCI MMIO windows.
> If
> @@ -765,6 +792,7 @@ PlatformAddressWidthInitialization (
>if (PlatformInfoHob->PhysMemAddressWidth != 0) {
> // physical address width is known
> PlatformInfoHob->FirstNonAddress = FirstNonAddress;
> +PlatformDynamicMmioWindow (PlatformInfoHob);
> return;
>}
>
> --
> 2.37.3
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101212): https://edk2.groups.io/g/devel/message/101212
Mute This Topic: https://groups.io/mt/94113626/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Better support for dynamic PcdFSBClock
On Tue, Nov 08, 2022 at 02:31:48PM +, Anthony PERARD via groups.io wrote:
> From: Anthony PERARD
>
> The PcdFSBClock can be a dynamic PCD. This can be an issue when
> InternalX86GetTimerFrequency() tries to get the value while been
> called with TPL been set to TPL_HIGH_LEVEL.
>
> When the PCD is dynamic, PcdGet*() calls a function that try to grab a
> lock which set TPL to TPL_NOTIFY. If TPL is already at TPL_HIGH_LEVEL,
> then an assert() in RaiseTPL() fails (in debug build).
>
> To try to avoid the issue, we'll store the current PcdFSBClock value
> in a local variable the first time we need it.
>
> The issue was discovered while attempting to boot a Windows guest with
> OvmfXen platform. The issue appear while executing the Windows's boot
> loader EFI application.
>
> The down side of this change is that when the PCD is FixedAtBuild, the
> value is loaded from a variable rather than been a constant.
>
> Signed-off-by: Anthony PERARD
I have tested this patch with edk2-stable202202 and edk2-stable202211. It
works to me to fix problem on bto#4340:
Bug 4340 - Running windows 2k22 on OvmfXen got ASSERT
/root/source-git/edk2/MdeModulePkg/Core/Dxe/Event/Tpl.c(66): ((BOOLEAN)(0==1))
https://bugzilla.tianocore.org/show_bug.cgi?id=4340
Thanks a lot!
Joey Lee
> ---
>
> InternalX86GetTimerFrequency() is called by
> - MicroSecondDelay()
> - NanoSecondDelay()
> - GetPerformanceCounterProperties()
>
> If one of those functions is called for the first time with a TPL too
> high, the work around in this patch would not work. But it seems to
> workaround the issue in my test case. So maybe that's enough, unless
> someone have a better idea?
> ---
> MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c
> b/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c
> index c60589607fde..28ff77ad0c1f 100644
> --- a/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c
> +++ b/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c
> @@ -90,8 +90,16 @@ InternalX86GetTimerFrequency (
>IN UINTN ApicBase
>)
> {
> + STATIC UINT32 mFSBClock;
> +
> + if (mFSBClock == 0) {
> + //
> + // Cache current value of PcdFSBClock in case it's a dynamic PCD.
> + //
> + mFSBClock = PcdGet32 (PcdFSBClock);
> + }
>return
> -PcdGet32 (PcdFSBClock) /
> +mFSBClock /
> mTimerLibLocalApicDivisor[MmioBitFieldRead32 (ApicBase + APIC_TDCR, 0,
> 3)];
> }
>
> --
> Anthony PERARD
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99941): https://edk2.groups.io/g/devel/message/99941
Mute This Topic: https://groups.io/mt/94891128/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases
Hi Jiewen, Thanks for your information. I will follow the process. Joey Lee On Wed, Dec 21, 2022 at 08:46:04AM +, 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 > > Sent: Tuesday, December 20, 2022 3:10 PM > > To: devel@edk2.groups.io; Yao, Jiewen > > Cc: Lee, Chun-Yi ; Xu, Min M > > ; Gerd Hoffmann ; Tom > > Lendacky ; James Bottomley > > ; Aktas, Erdem > > 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 +, Yao, Jiewen via groups.io wrote: > > > Thanks for the fix. > > > Reviewed-by: Jiewen Yao > > > > > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > > > > > > > > > -Original Message- > > > > From: Lee, Chun-Yi > > > > Sent: Thursday, December 15, 2022 10:27 PM > > > > To: devel@edk2.groups.io > > > > Cc: Xu, Min M ; Gerd Hoffmann > > > > ; Yao, Jiewen ; Tom > > Lendacky > > > > ; James Bottomley ; > > > > Aktas, Erdem ; Lee, Chun-Yi > > > > 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=003F <-- VAR_ADDED > > > > State &= VAR_DELETED=003D > > > > FlushHobVariableToFlash(), VariableName=BootOrder > > > > ... > > > > UpdateVariable(), VariableName=InitialAttemptOrder > > > > L1977, State=003F > > > > State &= VAR_IN_DELETED_TRANSITION=003E > > > > L2376, State=003E > > > > State &= VAR_DELETED=003C > > > > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder > > > > ... > > > > UpdateVariable(), VariableName=ConIn > > > > L1977, State=003F > > > > State &= VAR_IN_DELETED_TRANSITION=003E > > > > L2376, State=003E > > > > State &= VAR_DELETED=003C > > > > 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 > > >
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 +, Yao, Jiewen via groups.io wrote:
> Thanks for the fix.
> Reviewed-by: Jiewen Yao
>
> Question: Have you run tiano CI by yourself, before submit the patch?
>
>
>
> > -Original Message-
> > From: Lee, Chun-Yi
> > Sent: Thursday, December 15, 2022 10:27 PM
> > To: devel@edk2.groups.io
> > Cc: Xu, Min M ; Gerd Hoffmann
> > ; Yao, Jiewen ; Tom Lendacky
> > ; James Bottomley ;
> > Aktas, Erdem ; Lee, Chun-Yi
> > 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=003F <-- VAR_ADDED
> > State &= VAR_DELETED=003D
> > FlushHobVariableToFlash(), VariableName=BootOrder
> > ...
> > UpdateVariable(), VariableName=InitialAttemptOrder
> > L1977, State=003F
> > State &= VAR_IN_DELETED_TRANSITION=003E
> > L2376, State=003E
> > State &= VAR_DELETED=003C
> > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder
> > ...
> > UpdateVariable(), VariableName=ConIn
> > L1977, State=003F
> > State &= VAR_IN_DELETED_TRANSITION=003E
> > L2376, State=003E
> > State &= VAR_DELETED=003C
> > 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"
> > ---
> > 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
>
>
>
>
>
Re: [edk2-devel] [PATCH] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases
On Wed, Dec 14, 2022 at 03:12:22PM +0100, Gerd Hoffmann wrote: > > Sorry for I forgot to put my testing environment in patch description. > > My testing is on qemu with OVMF: > > > > - edk2-master or edk2-stable202211 > > build --verbose --debug=1 -D SECURE_BOOT_ENABLE -D TPM_ENABLE -D > > TPM_CONFIG_ENABLE \ > > -D NETWORK_IP6_ENABLE -D NETWORK_HTTP_BOOT_ENABLE -a X64 -b DEBUG -t > > GCC5 \ > > -p OvmfPkg/OvmfPkgX64.dsc -D FD_SIZE_4MB -D NETWORK_TLS_ENABLE > > > > - qemu-7.1.0 with libvirt-8.0.0 > > pc-q35 with pflash type and nvram: > > hvm > > > type='pflash'>/usr/share/qemu/ovmf-x86_64-code.bin > > > template='/usr/share/qemu/ovmf-x86_64-vars.bin'>/var/lib/libvirt/qemu/nvram/opensuseTW_VARS.fd > > That is not secure. You have unprotected writable flash. > > You can either use a build with SMM_REQUIRE=TRUE and run with > secure='yes', so only the firmware in SMM mode can write to flash. > > Or you run with both code and vars read-only. > Easiest is OVMF.fd. > Thanks for your suggestion! It's really helpful! I will try it. > Or you disable secure boot (SECURE_BOOT_ENABLE=FALSE) in your > builds. You still have unprotected writable flash then, but > it isn't a security hole any more. And the assert isn't triggered > either because that code path is only executed for secure boot > builds. > Yes, before I produce the patch, I need to disable SECURE_BOOT_ENABLE to workaround my VM hang problem. IMHO, using "variable header State was invalid" assert to prevent user writes to a unprotected flash is not a good idea. It causes some problem: - User's existing virtual machine can not boot/reboot after updated to edk2-stable202211 OVMF. VM just hangs there and doesn't have any hint. - The VM still works in the first boot. User doesn't know that second boot will hangs because they are writing an unprotected writable flash. - Even enabled debug log, we don't know what does "NvVarStore Variable header State was invalid." mean. Thanks Joey Lee -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97373): https://edk2.groups.io/g/devel/message/97373 Mute This Topic: https://groups.io/mt/95656983/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases
Hi Jiewen,
Thanks for your response!
On Wed, Dec 14, 2022 at 06:53:42AM +, Yao, Jiewen via groups.io wrote:
> Hey
> Good catch!
>
> I think we need handle below valid cases:
> 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.
>
>
> For the patch:
> >if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) ||
> > (VariableHeader->State == VAR_DELETED) ||
> > (VariableHeader->State == VAR_HEADER_VALID_ONLY) ||
> > -(VariableHeader->State == VAR_ADDED)))
> > +(VariableHeader->State == VAR_ADDED) ||
> > +(VariableHeader->State == (VAR_ADDED & VAR_DELETED)) ||
> > +(VariableHeader->State == (VAR_ADDED &
> > VAR_IN_DELETED_TRANSITION & VAR_DELETED
>
> I think:
> A. If we allow (VAR_HEADER_VALID_ONLY), then we support surprise shutdown, we
> need also allow (VAR_ADDED & VAR_IN_DELETED_TRANSITION). It should be added
> as well.
> B. The (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) are invalid state. They
> should be removed.
>
> Would you please double check?
>
I have followed your suggestion to change patch to add (VAR_ADDED &
VAR_IN_DELETED_TRANSITION) and
removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED). Like this:
Index: edk2/OvmfPkg/Library/PlatformInitLib/Platform.c
===
--- edk2.orig/OvmfPkg/Library/PlatformInitLib/Platform.c
+++ edk2/OvmfPkg/Library/PlatformInitLib/Platform.c
@@ -704,10 +704,11 @@ PlatformValidateNvVarStore (
VariableOffset = NvVarStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER);
} else {
DEBUG ((DEBUG_ERROR, "VariableHeader->VendorGuid = %g,
VariableHeader->State = 0x%x\n", VariableHeader->VendorGuid,
VariableHeader->State));
- if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) ||^M
-(VariableHeader->State == VAR_DELETED) ||^M
-(VariableHeader->State == VAR_HEADER_VALID_ONLY) ||^M
-(VariableHeader->State == VAR_ADDED)))^M
+ if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) ||^M
+ (VariableHeader->State == VAR_ADDED) ||^M
+ (VariableHeader->State == (VAR_ADDED & VAR_IN_DELETED_TRANSITION))
||^M
+ (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) ||^M
+ (VariableHeader->State == (VAR_ADDED & VAR_IN_DELETED_TRANSITION &
VAR_DELETED^M
{
DEBUG ((DEBUG_ERROR, "NvVarStore Variable header State was
invalid.\n"));
return FALSE;
The above change works to me no my OVMF.
Thanks a lot!
Joey Lee
>
>
>
> > -Original Message-
> > From: Lee, Chun-Yi
> > Sent: Tuesday, December 13, 2022 11:55 PM
> > To: devel@edk2.groups.io
> > Cc: Xu, Min M ; Gerd Hoffmann
> > ; Yao, Jiewen ; Tom Lendacky
> > ; James Bottomley ;
> > Aktas, Erdem ; Lee, Chun-Yi
> > Subject: [PATCH] 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, then 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
> > can see there have some variables have 0x3C or 0x3D state in store.
> > e.g.
> >
> > UpdateVariable(), VariableName=BootOrder
> > L1871, State=003F <-- VAR_ADDED
> > State &= VAR_DELETED=003D
> > FlushHobVariableToFlash(), VariableName=BootOrder
> > ...
> > UpdateVariable(), VariableName=InitialAttemptOrder
> > L1977, State=003F
> > State &= VAR_IN_DELETED_TRANSITION=003E
> > L2376, State=003E
> > State &= VAR_DELETED=003C
> > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder
> > ...
> > UpdateVariable(), VariableName=ConIn
> > L1977, State=003F
> > State &=
Re: [edk2-devel] [PATCH] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases
Hi, On Wed, Dec 14, 2022 at 09:46:36PM +0800, joeyli wrote: > Hi Gerd, > > Thanks for your response! > > On Wed, Dec 14, 2022 at 07:15:28AM +0100, Gerd Hoffmann via groups.io wrote: > > Hi, > > > > > When the variable store has those variables, then 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)) > > > > I'm wondering how you manage to trigger this assert? > > > > Sorry for I forgot to put my testing environment in patch description. > My testing is on qemu with OVMF: > > - edk2-master or edk2-stable202211 > build --verbose --debug=1 -D SECURE_BOOT_ENABLE -D TPM_ENABLE -D > TPM_CONFIG_ENABLE \ > -D NETWORK_IP6_ENABLE -D NETWORK_HTTP_BOOT_ENABLE -a X64 -b DEBUG -t > GCC5 \ > -p OvmfPkg/OvmfPkgX64.dsc -D FD_SIZE_4MB -D NETWORK_TLS_ENABLE > > - qemu-7.1.0 with libvirt-8.0.0 > pc-q35 with pflash type and nvram: > hvm > type='pflash'>/usr/share/qemu/ovmf-x86_64-code.bin > template='/usr/share/qemu/ovmf-x86_64-vars.bin'>/var/lib/libvirt/qemu/nvram/opensuseTW_VARS.fd > > - grub2 2.06 and Linux kernel 6.1.0-rc5 > I forgot shim-15.7, the fallback mechanism created new boot entry and update bootorder in a new *_VARS.fd : FSOpen: Open '\EFI\opensuse\boot.csv' Success UpdateVariable() - Boot0003, 8BE4DF61-93CA-11D2-AA0D-00E098032B8C FlushHobVariableToFlash() - Boot0003, 8BE4DF61-93CA-11D2-AA0D-00E098032B8C UpdateVariable() - BootOrder, 8BE4DF61-93CA-11D2-AA0D-00E098032B8C L1870, State = 0x3F State &= VAR_DELETED = 0x3D <-- state of BootOrder FlushHobVariableToFlash() - BootOrder, 8BE4DF61-93CA-11D2-AA0D-00E098032B8C Joey Lee > Two test cases, both of them can reproduce the assert: > > - First booting to grub2 menu, then "virsh destroy" the guest. > The second boot hangs in the "NvVarStore Variable header State was > invalid." assert > > - First boot to Linux kernel 6.1.0-rc5, bash shell prompt shows up. > Then run shutdown or reboot command. > The second boot hangs in in the "NvVarStore Variable header State was > invalid." assert > > > > Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), we > > > can see there have some variables have 0x3C or 0x3D state in store. > > > e.g. > > > > PlatformValidateNvVarStore() validates the varstore in ROM before > > copying it to RAM. The normal UpdateVariable() cycle changes the > > copy in RAM ... > > > > Yes, in the first boot, those variables have 0x3C or 0x3C state be > created. After shutdown or reboot, system hangs in the second boot. > The assert shows up in debug log. > > Thanks a lot! > Joey Lee -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97370): https://edk2.groups.io/g/devel/message/97370 Mute This Topic: https://groups.io/mt/95656983/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases
Hi Gerd, Thanks for your response! On Wed, Dec 14, 2022 at 07:15:28AM +0100, Gerd Hoffmann via groups.io wrote: > Hi, > > > When the variable store has those variables, then 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)) > > I'm wondering how you manage to trigger this assert? > Sorry for I forgot to put my testing environment in patch description. My testing is on qemu with OVMF: - edk2-master or edk2-stable202211 build --verbose --debug=1 -D SECURE_BOOT_ENABLE -D TPM_ENABLE -D TPM_CONFIG_ENABLE \ -D NETWORK_IP6_ENABLE -D NETWORK_HTTP_BOOT_ENABLE -a X64 -b DEBUG -t GCC5 \ -p OvmfPkg/OvmfPkgX64.dsc -D FD_SIZE_4MB -D NETWORK_TLS_ENABLE - qemu-7.1.0 with libvirt-8.0.0 pc-q35 with pflash type and nvram: hvm /usr/share/qemu/ovmf-x86_64-code.bin /var/lib/libvirt/qemu/nvram/opensuseTW_VARS.fd - grub2 2.06 and Linux kernel 6.1.0-rc5 Two test cases, both of them can reproduce the assert: - First booting to grub2 menu, then "virsh destroy" the guest. The second boot hangs in the "NvVarStore Variable header State was invalid." assert - First boot to Linux kernel 6.1.0-rc5, bash shell prompt shows up. Then run shutdown or reboot command. The second boot hangs in in the "NvVarStore Variable header State was invalid." assert > > Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), we > > can see there have some variables have 0x3C or 0x3D state in store. > > e.g. > > PlatformValidateNvVarStore() validates the varstore in ROM before > copying it to RAM. The normal UpdateVariable() cycle changes the > copy in RAM ... > Yes, in the first boot, those variables have 0x3C or 0x3C state be created. After shutdown or reboot, system hangs in the second boot. The assert shows up in debug log. Thanks a lot! Joey Lee -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97367): https://edk2.groups.io/g/devel/message/97367 Mute This Topic: https://groups.io/mt/95656983/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] OvmfPkg/IncompatiblePciDeviceSupportDxe: Ignore OptionRom in Sev guest
Hi Gerd,
On Fri, Aug 26, 2022 at 07:27:17AM +0200, Gerd Hoffmann wrote:
> Hi,
>
> > - if (TdIsEnabled ()) {
> > + if (TdIsEnabled () || MemEncryptSevIsEnabled()) {
>
> I think you can just use CcProbeLib and CcProbe() function to cover both
> tdx and sev.
>
Thanks for your review and suggestion! It works to me.
I will send version 2 patch.
Joey Lee
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92865): https://edk2.groups.io/g/devel/message/92865
Mute This Topic: https://groups.io/mt/93248346/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] ovmf miscompiles with gcc-12
Hi all,
I have filed this issue on tianocore bugzilla:
Bug 3934 - ovmf miscompiles with gcc-12
https://bugzilla.tianocore.org/show_bug.cgi?id=3934
Thanks
Joey Lee
On Thu, May 19, 2022 at 07:43:12AM +0200, Jiri Slaby via groups.io wrote:
> Hi,
>
> we discovered that qemu-ovmf-x86_64 doesn't start when compiled using
> gcc-12. Originally reported as:
> https://bugzilla.suse.com/show_bug.cgi?id=1199597
>
> I run qemu as:
> qemu-kvm -drive file=/dev/null,format=raw -drive
> if=pflash,format=raw,unit=0,readonly=on,file=OVMF.fd -m 3000
>
> The platform repeatedly resets after TemporaryRamMigration as can be seen in
> the debuglog:
> https://bugzilla.suse.com/attachment.cgi?id=858969
>
> The reason is TemporaryRamMigration() overwrites rbp unconditionally -- it
> adds an offset to rbp even if rbp is NOT used as a frame pointer
> (-fomit-frame-pointer was always used for compilation here). So commenting
> out:
> > //JumpBuffer.Rbp = JumpBuffer.Rbp + DebugAgentContext.StackMigrateOffset;
>
> makes it all work again. Also marking TemporaryRamMigration() as:
> __attribute__((optimize("-fno-omit-frame-pointer")))
> works around the problem too. (But that doesn't guarantee anything.)
>
> The code is:
> > if (SetJump () == 0) {
> > #if defined (MDE_CPU_IA32)
> > JumpBuffer.Esp = JumpBuffer.Esp + DebugAgentContext.StackMigrateOffset;
> > JumpBuffer.Ebp = JumpBuffer.Ebp + DebugAgentContext.StackMigrateOffset;
> > #endif
> > #if defined (MDE_CPU_X64)
> > JumpBuffer.Rsp = JumpBuffer.Rsp + DebugAgentContext.StackMigrateOffset;
> > JumpBuffer.Rbp = JumpBuffer.Rbp + DebugAgentContext.StackMigrateOffset;
> > #endif
> > LongJump (, (UINTN)-1);
> > }
>
> It was only coincidence this ever worked -- gcc-11 omits the frame pointer
> too, but apparently the caller (PeiCheckAndSwitchStack) does not use rbp.
>
> PeiCheckAndSwitchStack() (gcc-12):
>
> > 79a6: 4c 29 fdsub%r15,%rbp <-- used rbp
> > 79a9: 4d 29 fesub%r15,%r14
> > 79ac: 48 83 ec 20 sub$0x20,%rsp
> > 79b0: 4d 89 e0mov%r12,%r8
> > 79b3: 48 8d 4b 08 lea0x8(%rbx),%rcx
> > 79b7: 48 8b 44 24 50 mov0x50(%rsp),%rax
> > 79bc: 48 8b 54 24 20 mov0x20(%rsp),%rdx
> > 79c1: 4d 29 e8sub%r13,%r8
> > 79c4: 4c 8b 4c 24 30 mov0x30(%rsp),%r9
> > 79c9: ff 10 call *(%rax) <--- call
> > to TemporaryRamMigration
> > 79cb: 48 83 c4 20 add$0x20,%rsp
> > 79cf: be 01 00 00 00 mov$0x1,%esi
> > 79d4: 4c 89 f7mov%r14,%rdi
> > 79d7: e8 f4 a8 ff ff call 22d0
> > 79dc: 48 83 ec 20 sub$0x20,%rsp
> > 79e0: 4d 89 f0mov%r14,%r8
> > 79e3: 31 d2 xor%edx,%edx
> > 79e5: 48 89 e9mov%rbp,%rcx <-- rbp used
>
> gcc-11 seems to copy rbp to r8 first and operates on r8 there instead.
>
> Now, what is the right way to fix this? Do the SetJump/LongJump in assembly
> and wrap it into push rbp/pop rbp?
>
> thanks,
> --
> js
> suse labs
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90078): https://edk2.groups.io/g/devel/message/90078
Mute This Topic: https://groups.io/mt/91216215/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] How to create a account on bugzilla.tianocore.org ?
Thanks a lot! Joey Lee On Tue, May 24, 2022 at 03:27:32PM +, Kinney, Michael D wrote: > The details are on this Wiki page > > https://github.com/tianocore/tianocore.github.io/wiki/Reporting-Issues > > Mike > > > -Original Message- > > From: devel@edk2.groups.io On Behalf Of joeyli via > > groups.io > > Sent: Sunday, May 22, 2022 8:23 PM > > To: devel@edk2.groups.io > > Subject: [edk2-devel] How to create a account on bugzilla.tianocore.org ? > > > > Hi experts, > > > > We want to report bug on bugzilla.tianocore.org but the system doesn't > > provide link for creating new account. How should I do for creating > > a account on bugzilla.tianocore.org? > > > > Thanks! > > Joey Lee > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90002): https://edk2.groups.io/g/devel/message/90002 Mute This Topic: https://groups.io/mt/91312114/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] How to create a account on bugzilla.tianocore.org ?
Hi experts, We want to report bug on bugzilla.tianocore.org but the system doesn't provide link for creating new account. How should I do for creating a account on bugzilla.tianocore.org? Thanks! Joey Lee -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89984): https://edk2.groups.io/g/devel/message/89984 Mute This Topic: https://groups.io/mt/91312114/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
