Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP
Hi Dun,
Thanks for the notice on the other thread (v4 04/15).
I have a few more questions on this specific patch (and a few associated
commits related to it):
Why would we allow page table manipulation after `mIsReadOnlyPageTable`
is evaluated to TRUE?
As far as I can tell, `mIsReadOnlyPageTable` is set to TRUE inside
`SetPageTableAttributes` function,
but then we also have code in `InitializePageTablePool` to expect more
page tables to be allocated.
Could you please let me when this would happen?
I thought there would not be any new page table memory (i.e. memory
attribute update) after ready
to lock with restricted memory access option? With these change, it
seems to be doable through
EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL now, is that correct? If so, would
you mind shedding
some light on what other behavior changes there might be?
In addition, I might miss it in the patch series. If the newly allocated
page memory is marked as read only
after the above flag is set to TRUE, how would the callers able to use them?
Thanks in advance.
Regards,
Kun
On 5/16/2023 2:59 AM, duntan wrote:
Add two functions to disable/enable CR0.WP. These two unctions
will also be used in later commits. This commit doesn't change any
functionality.
Signed-off-by: Dun Tan
Cc: Eric Dong
Cc: Ray Ni
Cc: Rahul Kumar
Cc: Gerd Hoffmann
---
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 24
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 115
++-
2 files changed, 90 insertions(+), 49 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index ba341cadc6..e0c4ca76dc 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1565,4 +1565,28 @@ SmmWaitForApArrival (
VOID
);
+/**
+ Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
+
+ @param[out] WpEnabled If Cr0.WP is enabled.
+ @param[out] CetEnabled If CET is enabled.
+**/
+VOID
+DisableReadOnlyPageWriteProtect (
+ OUT BOOLEAN *WpEnabled,
+ OUT BOOLEAN *CetEnabled
+ );
+
+/**
+ Enable Write Protect on pages marked as read-only.
+
+ @param[out] WpEnabled If Cr0.WP should be enabled.
+ @param[out] CetEnabled If CET should be enabled.
+**/
+VOID
+EnableReadOnlyPageWriteProtect (
+ BOOLEAN WpEnabled,
+ BOOLEAN CetEnabled
+ );
+
#endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 2faee8f859..4b512edf68 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -40,6 +40,64 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
//
BOOLEAN mIsReadOnlyPageTable = FALSE;
+/**
+ Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
+
+ @param[out] WpEnabled If Cr0.WP is enabled.
+ @param[out] CetEnabled If CET is enabled.
+**/
+VOID
+DisableReadOnlyPageWriteProtect (
+ OUT BOOLEAN *WpEnabled,
+ OUT BOOLEAN *CetEnabled
+ )
+{
+ IA32_CR0 Cr0;
+
+ *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+ Cr0.UintN = AsmReadCr0 ();
+ *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
+ if (*WpEnabled) {
+if (*CetEnabled) {
+ //
+ // CET must be disabled if WP is disabled. Disable CET before clearing
CR0.WP.
+ //
+ DisableCet ();
+}
+
+Cr0.Bits.WP = 0;
+AsmWriteCr0 (Cr0.UintN);
+ }
+}
+
+/**
+ Enable Write Protect on pages marked as read-only.
+
+ @param[out] WpEnabled If Cr0.WP should be enabled.
+ @param[out] CetEnabled If CET should be enabled.
+**/
+VOID
+EnableReadOnlyPageWriteProtect (
+ BOOLEAN WpEnabled,
+ BOOLEAN CetEnabled
+ )
+{
+ IA32_CR0 Cr0;
+
+ if (WpEnabled) {
+Cr0.UintN = AsmReadCr0 ();
+Cr0.Bits.WP = 1;
+AsmWriteCr0 (Cr0.UintN);
+
+if (CetEnabled) {
+ //
+ // re-enable CET.
+ //
+ EnableCet ();
+}
+ }
+}
+
/**
Initialize a buffer pool for page table use only.
@@ -62,10 +120,9 @@ InitializePageTablePool (
IN UINTN PoolPages
)
{
- VOID *Buffer;
- BOOLEAN CetEnabled;
- BOOLEAN WpEnabled;
- IA32_CR0 Cr0;
+ VOID *Buffer;
+ BOOLEAN WpEnabled;
+ BOOLEAN CetEnabled;
//
// Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page
for
@@ -102,34 +159,9 @@ InitializePageTablePool (
// If page table memory has been marked as RO, mark the new pool pages as
read-only.
//
if (mIsReadOnlyPageTable) {
-CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
-Cr0.UintN = AsmReadCr0 ();
-WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
-if (WpEnabled) {
- if (CetEnabled) {
-//
-// CET must be disabled if WP is
Re: 回复: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update edk2 pip modules
Hi Liming, The subject line is not tagged edk2-stable202305, but I think that is the intent. I have no objections to merging this into edk2-stable202305. Mike > -Original Message- > From: devel@edk2.groups.io On Behalf Of Michael > Kubacki > Sent: Friday, May 19, 2023 5:50 PM > To: devel@edk2.groups.io; Kinney, Michael D > ; Gao, Liming ; > rebe...@bsdio.com; 'Liming Gao' > Cc: 'Sean Brogan' ; 'Andrew Fish' > ; 'Leif Lindholm' > Subject: Re: 回复: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update > edk2 pip modules > > I rebased the commit and added Mike's R-b in the PR. > > https://github.com/tianocore/edk2/pull/4401 > > On 5/17/2023 9:01 PM, Michael D Kinney wrote: > > Reviewed-by: Michael D Kinney > > > >> -Original Message- > >> From: devel@edk2.groups.io On Behalf Of > Michael > >> Kubacki > >> Sent: Wednesday, May 17, 2023 5:50 PM > >> To: devel@edk2.groups.io; Kinney, Michael D > ; > >> Gao, Liming ; rebe...@bsdio.com; 'Liming > Gao' > >> > >> Cc: 'Sean Brogan' ; 'Andrew Fish' > >> ; 'Leif Lindholm' > >> Subject: Re: 回复: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: > Update > >> edk2 pip modules > >> > >> This is mine: https://github.com/tianocore/edk2/pull/4401 > >> > >> I'll close some related dependabot PRs. > >> > >> On 5/17/2023 8:21 PM, Michael D Kinney wrote: > >>> Is there a PR with EDK II CI results? > >>> > >>> Mike > >>> > -Original Message- > From: Michael Kubacki > Sent: Wednesday, May 17, 2023 5:13 PM > To: devel@edk2.groups.io; Gao, Liming ; > rebe...@bsdio.com; Kinney, Michael D > ; > 'Liming Gao' > Cc: 'Sean Brogan' ; 'Andrew Fish' > ; 'Leif Lindholm' > Subject: Re: 回复: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: > >> Update > edk2 pip modules > > Thanks Liming. > > I haven't seen any R-b tags yet. Can I please get some reviews? > > On 5/16/2023 9:32 PM, gaoliming via groups.io wrote: > > Rebecca: > > I am OK for this change to catch the stable tag 202305. > > > > Thanks > > Liming > >> -邮件原件- > >> 发件人: devel@edk2.groups.io 代表 > Rebecca > >> Cran > >> 发送时间: 2023年5月17日 1:31 > >> 收件人: Michael Kubacki ; > >> devel@edk2.groups.io; Kinney, Michael D > >> ; > >> 'Liming Gao' > >> 抄送: Sean Brogan ; gaoliming > >> ; Andrew Fish ; Leif > >> Lindholm > >> 主题: Re: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update > edk2 > > pip > >> modules > >> > >> It would be good to get this in for the edk2-stable202305 release. > >> > >> On Tue, May 16, 2023, at 11:01 AM, mikub...@linux.microsoft.com > >> wrote: > >>> From: Michael Kubacki > >>> > >>> - edk2-pytool-library: 0.14.0 to 0.14.1 > >>> - edk2-pytool-extensions: 0.21.8 to 0.23.2 > >>> - edk2-basetools: 0.1.43 to 0.1.48 > >>> > >>> Cc: Sean Brogan > >>> Cc: Liming Gao > >>> Cc: Andrew Fish > >>> Cc: Leif Lindholm > >>> Cc: Rebecca Cran > >>> Signed-off-by: Michael Kubacki > >>> --- > >>> > >>> Notes: > >>>Updates edk2 pip modules to latest as discussed > >>>in the 5/15 TianoCore Tools & CI meeting. > >>> > >>>- > >>> > >> https://github.com/tianocore/edk2-pytool- > library/compare/v0.14.0...v0.14.1 > >>>- > >>> > >> https://github.com/tianocore/edk2-pytool- > extensions/compare/v0.21.8...v0. > >> 23.2 > >>> > >>> pip-requirements.txt | 6 +++--- > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/pip-requirements.txt b/pip-requirements.txt > >>> index b71e5c4015b7..c221576bef3a 100644 > >>> --- a/pip-requirements.txt > >>> +++ b/pip-requirements.txt > >>> @@ -12,9 +12,9 @@ > >>> # https://www.python.org/dev/peps/pep-0440/#version- > specifiers > >>> ## > >>> > >>> -edk2-pytool-library==0.14.0 > >>> -edk2-pytool-extensions~=0.21.8 > >>> -edk2-basetools==0.1.43 > >>> +edk2-pytool-library==0.14.1 > >>> +edk2-pytool-extensions~=0.23.2 > >>> +edk2-basetools==0.1.48 > >>> antlr4-python3-runtime==4.7.1 > >>> lcov-cobertura==2.0.2 > >>> > >>> -- > >>> 2.40.1.windows.1 > >> > >> > >> > >> > > > > > > > > > > > > > > > >>> > >>> > >>> > >>> > >>> > >> > >> > >> > >> > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105089): https://edk2.groups.io/g/devel/message/105089 Mute This Topic: https://groups.io/mt/98940327/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: 回复: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update edk2 pip modules
I rebased the commit and added Mike's R-b in the PR. https://github.com/tianocore/edk2/pull/4401 On 5/17/2023 9:01 PM, Michael D Kinney wrote: Reviewed-by: Michael D Kinney -Original Message- From: devel@edk2.groups.io On Behalf Of Michael Kubacki Sent: Wednesday, May 17, 2023 5:50 PM To: devel@edk2.groups.io; Kinney, Michael D ; Gao, Liming ; rebe...@bsdio.com; 'Liming Gao' Cc: 'Sean Brogan' ; 'Andrew Fish' ; 'Leif Lindholm' Subject: Re: 回复: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update edk2 pip modules This is mine: https://github.com/tianocore/edk2/pull/4401 I'll close some related dependabot PRs. On 5/17/2023 8:21 PM, Michael D Kinney wrote: Is there a PR with EDK II CI results? Mike -Original Message- From: Michael Kubacki Sent: Wednesday, May 17, 2023 5:13 PM To: devel@edk2.groups.io; Gao, Liming ; rebe...@bsdio.com; Kinney, Michael D ; 'Liming Gao' Cc: 'Sean Brogan' ; 'Andrew Fish' ; 'Leif Lindholm' Subject: Re: 回复: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update edk2 pip modules Thanks Liming. I haven't seen any R-b tags yet. Can I please get some reviews? On 5/16/2023 9:32 PM, gaoliming via groups.io wrote: Rebecca: I am OK for this change to catch the stable tag 202305. Thanks Liming -邮件原件- 发件人: devel@edk2.groups.io 代表 Rebecca Cran 发送时间: 2023年5月17日 1:31 收件人: Michael Kubacki ; devel@edk2.groups.io; Kinney, Michael D ; 'Liming Gao' 抄送: Sean Brogan ; gaoliming ; Andrew Fish ; Leif Lindholm 主题: Re: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update edk2 pip modules It would be good to get this in for the edk2-stable202305 release. On Tue, May 16, 2023, at 11:01 AM, mikub...@linux.microsoft.com wrote: From: Michael Kubacki - edk2-pytool-library: 0.14.0 to 0.14.1 - edk2-pytool-extensions: 0.21.8 to 0.23.2 - edk2-basetools: 0.1.43 to 0.1.48 Cc: Sean Brogan Cc: Liming Gao Cc: Andrew Fish Cc: Leif Lindholm Cc: Rebecca Cran Signed-off-by: Michael Kubacki --- Notes: Updates edk2 pip modules to latest as discussed in the 5/15 TianoCore Tools & CI meeting. - https://github.com/tianocore/edk2-pytool- library/compare/v0.14.0...v0.14.1 - https://github.com/tianocore/edk2-pytool- extensions/compare/v0.21.8...v0. 23.2 pip-requirements.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pip-requirements.txt b/pip-requirements.txt index b71e5c4015b7..c221576bef3a 100644 --- a/pip-requirements.txt +++ b/pip-requirements.txt @@ -12,9 +12,9 @@ # https://www.python.org/dev/peps/pep-0440/#version-specifiers ## -edk2-pytool-library==0.14.0 -edk2-pytool-extensions~=0.21.8 -edk2-basetools==0.1.43 +edk2-pytool-library==0.14.1 +edk2-pytool-extensions~=0.23.2 +edk2-basetools==0.1.48 antlr4-python3-runtime==4.7.1 lcov-cobertura==2.0.2 -- 2.40.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105088): https://edk2.groups.io/g/devel/message/105088 Mute This Topic: https://groups.io/mt/98940327/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 2/2] ArmVirtPkg/ArmVirtQemu: Avoid early ID map on ThunderX
On Fri, 19 May 2023 at 18:32, Oliver Steffen wrote:
>
>
> Hi all,
>
> I had another look at this and I can now reproduce the issue consistently,
> with a quite minimal setup, on recent Linux kernel, Qemu, and EDK2.
> It requires rebooting the guest in a tight loop. It happens in silent
> and verbose
> builds alike, but since the verbose ones are slowed down by the serial
> output, it
> takes longer to hit the issue.
> It is possible to reproduce it with the silent builds within a few minutes.
> For the verbose case I recommend running multiple Qemu instances in parallel
> (as
> many as the machine allows, in my case ~100).
>
Thanks a lot for all these details, this is extremely helpful.
So what appears to be happening is that we split the 2M block mapping
that covers the code that we were called from, and hit a level 2
translation fault because the updated page table entry is still
observed to be in its transient 'invalid' state as we return to it.
Could you please check whether this makes a difference?
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
@@ -65,6 +65,7 @@
// write updated entry
str x1, [x0]
dsb nshst
+ isb
.L2_\@:
.endm
> Details:
>
> CPU: Cavium ThunderX2(R) CPU CN9975
> Tested on 3 different machines:
> HPE apache, HPE apollo, Gigabyte R181
> Kernels tested:
> - 6.2.15-100.fc36.aarch64
> - 5.14.0-312.el9.aarch64
>(contains 406504c7b0405d74d74c15a667cd4c4620c3e7a9,
>"KVM: arm64: Fix S1PTW handling on RO memslots")
> Qemu v8.0.0 (RHEL version and build from upstream repo)
> EDK2: master branch from 2023-05-16 (cafb4f3f)
> gcc 11.3.1
>
> EDK2 build command line:
> build \
> -a AARCH64
> -p ArmVirtPkg/ArmVirtQemu.dsc
> -t GCC5 -b DEBUG \
> -D NETWORK_IP6_ENABLE \
> -D NETWORK_HTTP_BOOT_ENABLE \
> -D NETWORK_TLS_ENABLE \
> -D NETWORK_ISCSI_ENABLE \
> -D NETWORK_ALLOW_HTTP_CONNECTIONS \
> -D CAVIUM_ERRATUM_27456=TRUE \
> -D TPM2_ENABLE=TRUE \
> -D TPM1_ENABLE=FALSE \
> -D DEBUG_PRINT_ERROR_LEVEL=0x8000 \
> -D BUILD_SHELL=TRUE \
> --pcd="gEfiShellPkgTokenSpaceGuid.PcdShellDefaultDelay=0" \
> --pcd="gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut=0" \
> --hash --cmd-len=65536
>
> To reproduce the issue I launched the firmware in Qemu and have it do
> a reboot once it finished booting up
> via a startup.nsh on the ESP.
>
> Qemu command line:
> qemu-system-aarch64 \
> -machine virt,accel=kvm -m 13G \
> -boot menu=off \
> -cpu host \
> -blockdev node-name=code,driver=file,filename="${FW_CODE}",read-only=on \
> -blockdev node-name=vars,driver=file,filename="${FW_VARS}" \
> -machine pflash0=code \
> -machine pflash1=vars \
> -serial stdio \
> -net none \
> -drive file=esp.img,snapshot=on
>
> Other things like number of CPUs or the presence of a vTPM have no
> influence. I did not try different amounts of RAM yet.
>
> Serial output:
> [...]
> InitializeDxeNxMemoryProtectionPolicy: StackBase = 0x476C5000
> StackSize = 0x0002
> InitializeDxeNxMemoryProtectionPolicy: applying strict permissions to
> active memory regions
> SetUefiImageMemoryAttributes - 0x4000 - 0x076E5000
> (0x4000)
> UpdateRegionMappingRecursive(0): 4000 - 476E5000 set
> 600400 clr FF9F0B3F
> UpdateRegionMappingRecursive(1): 4000 - 476E5000 set
> 600400 clr FF9F0B3F
> UpdateRegionMappingRecursive(2): 4000 - 476E5000 set
> 600400 clr FF9F0B3F
> UpdateRegionMappingRecursive(3): 4760 - 476E5000 set
> 600400 clr FF9F0B3F
> SetUefiImageMemoryAttributes - 0x476C5000 - 0x1000
> (0x6000)
> UpdateRegionMappingRecursive(0): 476C5000 - 476C6000 set
> 60 clr FF9F0B3F
> UpdateRegionMappingRecursive(1): 476C5000 - 476C6000 set
> 60 clr FF9F0B3F
> UpdateRegionMappingRecursive(2): 476C5000 - 476C6000 set
> 60 clr FF9F0B3F
> UpdateRegionMappingRecursive(3): 476C5000 - 476C6000 set
> 60 clr FF9F0B3F
> SetUefiImageMemoryAttributes - 0x4772B000 - 0x007C
> (0x4000)
> UpdateRegionMappingRecursive(0): 4772B000 - 47EEB000 set
> 600400 clr FF9F0B3F
> UpdateRegionMappingRecursive(1): 4772B000 - 47EEB000 set
> 600400 clr FF9F0B3F
> UpdateRegionMappingRecursive(2): 4772B000 - 47EEB000 set
> 600400 clr FF9F0B3F
> UpdateRegionMappingRecursive(3): 4772B000 - 4780 set
> 600400 clr FF9F0B3F
> UpdateRegionMappingRecursive(3): 47E0 - 47EEB000 set
> 600400 clr FF9F0B3F
> SetUefiImageMemoryAttributes - 0x47EF3000 - 0x00101000
> (0x4000)
> UpdateRegionMappingRecursive(0): 47EF3000 - 47FF4000 set
> 600400 clr FF9F0B3F
> UpdateRegionMappingRecursive(1): 47EF3000 - 47FF4000 set
>
Re: [edk2-devel] Now: UEFI Memory Map, GCD, Page Table discussion - ARM/X86 - Wednesday, May 17, 2023 #cal-notice
A summary email containing the presentation, supplementary files, and next steps will go out on Monday to avoid the thread getting buried over the weekend. Thanks for your patience :) On 5/19/2023 1:10 PM, Sheng Lean Tan wrote: This was a good sharing! May I know where can I get the presentation slides? Thanks. -- Taylor Beebe Software Engineer @ Microsoft -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105086): https://edk2.groups.io/g/devel/message/105086 Mute This Topic: https://groups.io/mt/98948349/21656 Mute #cal-notice:https://edk2.groups.io/g/devel/mutehashtag/cal-notice Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Now: UEFI Memory Map, GCD, Page Table discussion - ARM/X86 - Wednesday, May 17, 2023 #cal-notice
This was a good sharing! May I know where can I get the presentation slides? Thanks. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105085): https://edk2.groups.io/g/devel/message/105085 Mute This Topic: https://groups.io/mt/98948349/21656 Mute #cal-notice:https://edk2.groups.io/g/devel/mutehashtag/cal-notice Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 2/2] ArmVirtPkg/ArmVirtQemu: Avoid early ID map on ThunderX
Quoting Oliver Steffen (2023-03-02 14:29:43) > On Thu, Mar 2, 2023 at 11:50 AM Ard Biesheuvel <[1]a...@kernel.org> wrote: > > On Thu, 9 Feb 2023 at 16:15, Ard Biesheuvel <[2]a...@kernel.org> wrote: > > > > On Tue, 7 Feb 2023 at 13:58, Oliver Steffen <[3]ostef...@redhat.com> > wrote: > > > > > > On Tue, Feb 7, 2023 at 12:57 PM Ard Biesheuvel <[4]a...@kernel.org> > wrote: > > >> > > >> On Tue, 7 Feb 2023 at 11:51, Oliver Steffen <[5]ostef...@redhat.com> > wrote: > > >> > > > >> > On Thu, Feb 2, 2023 at 12:09 PM Oliver Steffen <[6] > ostef...@redhat.com> wrote: > > >> >> > > >> >> > > >> >> On Wed, Feb 1, 2023 at 2:29 PM Ard Biesheuvel <[7]a...@kernel.org> > wrote: > > >> >>> > > >> >>> On Wed, 1 Feb 2023 at 13:59, Oliver Steffen <[8] > ostef...@redhat.com> wrote: > > >> >>> > > > >> >>> > On Wed, Feb 1, 2023 at 12:52 PM Ard Biesheuvel <[9] > a...@kernel.org> wrote: > > >> >>> >> > > >> >>> >> On Wed, 1 Feb 2023 at 10:14, Oliver Steffen <[10] > ostef...@redhat.com> wrote: > > >> >>> >> > > > >> >> > > >> >> [...] > > >> >>> > > >> >>> >> > I am sorry, this story does not seem to be over yet. > > >> >>> >> > > > >> >>> >> > We are using the Erratum patch and also included the commit > 406504c7 in > > >> >>> >> > the kernel. > > >> >>> >> > Now the firmware crashes sometimes (10 out of 89 tests). > > >> >>> >> > > > >> >>> >> > > >> >>> >> Thanks for the report. Is this still on ThunderX2? > > >> >>> >> > > >> >>> >> > Any hints are very welcome! > > >> >>> >> > > > >> >>> >> > > >> >>> >> Do you have access to those build artifacts? > > >> >>> > > > >> >>> > > > >> >>> > > [11]https://kojihub.stream.centos.org/kojifiles/work/tasks/5251/ > 1835251/edk2-aarch64-20221207gitfff6d81270b5-4.el9.test.noarch.rpm > > >> >>> > > > >> >>> > and/or here: > > >> >>> > > > >> >>> > [12]https://kojihub.stream.centos.org/koji/taskinfo?taskID= > 1835251 > > >> >>> > > > >> >>> > Source for reference: > > >> >>> > [13]https://gitlab.com/redhat/centos-stream/src/edk2/-/ > merge_requests/24 > > >> >>> > > > >> >>> > > >> >>> Any chance the .dll files (which are actually ELF executables) > have > > >> >>> been preserved somewhere? > > >> >> > > >> >> Here is the build folder (~90MB): > > >> >> [14]https://gitlab.com/osteffen/thunderx2-debug/-/raw/main/ > armvirt-thunderx2-issue.tar.xz > > >> >> > > >> >> I am waiting for the tests with the additional debug output to > run. > > >> > > > >> > > > >> > We reran the test suite with the Erratum and the additional debug > > >> > output enabled. Strangely, the problem does not occur anymore, the > > >> > firmware boots up normally. > > >> > > > >> > We retried the tests without the additional debug output. > > >> > RHEL ships two firmware flavors for AARCH64: a silent and a verbose > > >> > version. > > >> > > >> Are these RELEASE vs DEBUG builds? > > > > > > > > > All builds are DEBUG, just the amount of information printed on > > > the serial is different (almost zero for the "silent" one.) > > > > > >> > > >> > Both were tried. We see no problems with the verbose > > >> > one. The silent one fails noticeably more often if a software TPM > device > > >> > is present. > > >> > > > >> > > >> This smells like some missing cache or TLB maintenance - the verbose > > >> one exits to the host much more often, and likely relies on cache/TLB > > >> maintenance occurring in the hypervisor. > > >> > > >> So the build always includes TPM support but the issue only occurs > > >> when the sw TPM is actually exposed by QEMU? > > > > > > > > > Yes. > > > All builds include support for TPM, but the issue occurs more > frequently > > > if a sw TPM is exposed by QEMU. > > > > > > > Any chance you could provide a specific command line for launching > > QEMU? I am trying to reproduce this, but I am not making any progress. > > > > >> > > >> > Could this be related to how much stuff is going on in the early > phase > > >> > of the firmware (when logging is enabled: formatting of messages > and > > >> > sending to serial port...) ? > > >> > > > >> > > >> I'll try to see if I can rig something up that logs into a buffer > > >> rather than straight to the serial, and dump it all out when handling > > >> the crash > > >> > > > > This takes a bit more time than I can afford to spend on this atm, and > > I'd like to be able to reproduce before I go down this rabbit hole. > > Have there been any developments regarding this issue? > > > Nothing from my side. I tried to come up with a more reliable/faster > reproducer > but
Re: [edk2-devel] CpuDeadLoop() is optimized by compiler
I don’t think the atomic is going to help. The compiler honored the volatile by
doing a read, but assumed it would never change due to scoping. As you can see
in my example if the compiler thinks DeadLoopCount can be changed it will put
the check back in and assume the function can return. So an assembly function
that does nothing called IncreaseScope() would fix this issue too.
void IncreaseScope(int *ptr);
void CpuDeadLoopFix(void) {
volatile int DeadLoopCount = 0;
while(DeadLoopCount == 0) {
IncreaseScope();
}
}
void CpuDeadLoop(void) {
volatile int DeadLoopCount = 0;
while(DeadLoopCount == 0);
}
Gives us:
voltbl SEGMENT
voltbl ENDS
voltbl SEGMENT
voltbl ENDS
DeadLoopCount$ = 48
CpuDeadLoopFix PROC ; COMDAT
$LN12:
sub rsp, 40 ; 0028H
mov DWORD PTR DeadLoopCount$[rsp], 0
jmp SHORT $LN10@CpuDeadLoo
$LL2@CpuDeadLoo:
lea rcx, QWORD PTR DeadLoopCount$[rsp]
callIncreaseScope
$LN10@CpuDeadLoo:
mov eax, DWORD PTR DeadLoopCount$[rsp]
testeax, eax
je SHORT $LL2@CpuDeadLoo
add rsp, 40 ; 0028H
ret 0
CpuDeadLoopFix ENDP
DeadLoopCount$ = 8
CpuDeadLoop PROC; COMDAT
mov DWORD PTR DeadLoopCount$[rsp], 0
$LL2@CpuDeadLoo:
mov eax, DWORD PTR DeadLoopCount$[rsp]
jmp SHORT $LL2@CpuDeadLoo
CpuDeadLoop ENDP
https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGIM6SuADJ4DJgAcj4ARpjEINIADqgKhE4MHt6%2B/qRJKY4CIWGRLDFx0naYDmlCBEzEBBk%2BfgEVVQI1dQSFEdGx8ba19Y1ZLYNdoT0lfZIAlLaoXsTI7BwA9KsA1AAqAJ4JmBs7C8QbaFgbCLGYpBskG7SoTOgbhhuYqqwJ9AB0JhoAgqECBsFABHLx1TAQIEbBg%2BGYbEwAdisAI26I2xEwBEWDFhPg2ACp8SwTABmVH/ZEAEQ4c1onAArLw/BwtKRUJw3NZrCDjstEWYyTxSARNHS5gBrECMjT6TiSFnijmcXgKEBysVsumkOCwJBoFgJOixciUQ3G%2BhxYBcMxcPh0AixdUQKLKqKhOo7Tgiw1sQQAeQYtG92tIWBYhmA4jD%2BCxVQAbph1WH3pUvE6fbwgZgGWHaHgosQvR4sMqCMQ8Cws3MqAZgAoAGp4TAAdwD%2B1ZIv4ghEYnYUhkgkUKnUYd09oMRhQPMs%2BkL6sgc1QCXyDBTAFoA2SNhvI0sEOTqQpJTsDJLMGrc5U1y4GO5PE09MEJsVSnpcqkBMM/PbP2vujfPp7VaNcOiGR8shA682gYcDxiKXo4hAsYfz0BQxkApCJDmBR%2BQHUUsRWHh6SZJUw05DhVAADgANg3WjJA2YBkGQDZbW%2BLgNggblLGsG5cEIO5zGFG4PCNE0ThErgZl4LUtDmCADVQCSrTNCALUklApxtSQNDlGhaCdYgXTdMMPWYYhQ19FT/QIIMQ2VCMoxjdk4xvPAkxTdk02QDNiOzQRc2VAsixLDAVnZCsqxrPh6ybFt207LMh2EURxEHHt5CUNRlV0AIdJnPi51CxcIGXVc0k3bdd33ZBDzJY9T3PS9bBg28IFcND7RfRCpmQnJki/dJIN/Qa8jSLD%2BvQ9rqlQ0aZvsMDMNfbCUM6bqBk6Kb3xk%2BZFmWPQK0wALSI4ZlSFZdlKJo%2BjGNOHT2Mkb4NBe7jeKsOcNkEogpKFe0NnEy1YkFMkzFk0VxUUpB8CoKh1KyvsMukLKR1y/MEHVSdMdhqgCD2dg5WITHsmJhRcfx/YNTOi6rt4SjqTwOGNmbNsQduhimJYtiOK4hMFA2Dn7uQR6pBel65KhqUZTlPNFUu5VKLVbJ5IleUODMcjrtVSHtRmOYkxMtJ4iAA
Thanks,
Andrew Fish
PS I’m still not 100% sure it is a compiler bug. Some times things like this
are due to the order the compiler applies the optimizations, and changing the
order can change the behavior.
> On May 19, 2023, at 8:31 AM, Rebecca Cran wrote:
>
> Just to add more data, I also tried with "volatile sig_atomic_t" as someone
> suggested and both "/volatile:iso" and "/volatile:ms" with no change in
> results.
>
>
> --
>
> Rebecca Cran
>
>
> On 5/18/23 20:53, Ni, Ray wrote:
>>
>> I think all the options we considered are workarounds. These might break
>> again if compiler is “cleverer” in future. Unless some Cxx spec clearly
>> guarantees that.
>>
>> I like Mike’s idea to use assembly implementation for CpuDeadLoop. The
>> assembly can simply “jmp $” then “ret”.
>>
>> I didn’t find a dead-loop intrinsic function in MSVC.
>>
>> Any better idea?
>>
>> Thanks,
>>
>> Ray
>>
>> *From:* Andrew (EFI) Fish
>> *Sent:* Friday, May 19, 2023 8:42 AM
>> *To:* devel@edk2.groups.io; Kinney, Michael D
>> *Cc:* Ni, Ray ; Rebecca Cran
>> *Subject:* Re: [edk2-devel] CpuDeadLoop() is optimized by compiler
>>
>> Mike,
>>
>> Sorry static was just to scope the name to the file since it is a lib, not
>> to make it work.
>>
>> That is a cool site. I learned about it complaining about stuff to the
>> compiler team on our internal clang Slack channel as they use it to answer
>> my questions.
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>
>>
>>On May 18, 2023, at 2:42 PM, Michael D Kinney
>> wrote:
>>
>>Using that tool, the following fragment seems to generate the
>>right code. Volatile is required. Static is optional.
>>
>>staticvolatileint mDeadLoopCount =0;
>>
>>void
>>
>>CpuDeadLoop(
>>
>>void
>>
>> )
>>
>>{
>>
>>while(mDeadLoopCount ==0);
>>
>>}
>>
>>GCC
>>
>>===
>>
>>CpuDeadLoop():
>>
>>.L2:
>>
>>moveax,DWORDPTRmDeadLoopCount[rip]
>>
>>testeax,eax
>>
>>je.L2
>>
>>ret
>>
>>CLANG
>>
>>=
>>
>>CpuDeadLoop():# @CpuDeadLoop()
>>
>>.LBB0_1:
Re: [edk2-devel] CpuDeadLoop() is optimized by compiler
I've submitted a bug report at https://developercommunity.visualstudio.com/t/x64-codegen-with-optimizations-wrong-for/10369557 . -- Rebecca Cran On 5/19/23 09:31, Rebecca Cran wrote: Just to add more data, I also tried with "volatile sig_atomic_t" as someone suggested and both "/volatile:iso" and "/volatile:ms" with no change in results. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105082): https://edk2.groups.io/g/devel/message/105082 Mute This Topic: https://groups.io/mt/98987896/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] CpuDeadLoop() is optimized by compiler
Just to add more data, I also tried with "volatile sig_atomic_t" as
someone suggested and both "/volatile:iso" and "/volatile:ms" with no
change in results.
--
Rebecca Cran
On 5/18/23 20:53, Ni, Ray wrote:
I think all the options we considered are workarounds. These might
break again if compiler is “cleverer” in future. Unless some Cxx spec
clearly guarantees that.
I like Mike’s idea to use assembly implementation for CpuDeadLoop. The
assembly can simply “jmp $” then “ret”.
I didn’t find a dead-loop intrinsic function in MSVC.
Any better idea?
Thanks,
Ray
*From:* Andrew (EFI) Fish
*Sent:* Friday, May 19, 2023 8:42 AM
*To:* devel@edk2.groups.io; Kinney, Michael D
*Cc:* Ni, Ray ; Rebecca Cran
*Subject:* Re: [edk2-devel] CpuDeadLoop() is optimized by compiler
Mike,
Sorry static was just to scope the name to the file since it is a lib,
not to make it work.
That is a cool site. I learned about it complaining about stuff to the
compiler team on our internal clang Slack channel as they use it to
answer my questions.
Thanks,
Andrew Fish
On May 18, 2023, at 2:42 PM, Michael D Kinney
wrote:
Using that tool, the following fragment seems to generate the
right code. Volatile is required. Static is optional.
staticvolatileint mDeadLoopCount =0;
void
CpuDeadLoop(
void
)
{
while(mDeadLoopCount ==0);
}
GCC
===
CpuDeadLoop():
.L2:
moveax,DWORDPTRmDeadLoopCount[rip]
testeax,eax
je.L2
ret
CLANG
=
CpuDeadLoop():# @CpuDeadLoop()
.LBB0_1: # =>This Inner Loop Header:Depth=1
cmpdwordptr[rip+_ZL14mDeadLoopCount],0
je.LBB0_1
ret
Mike
*From:*Andrew (EFI) Fish
*Sent:*Thursday, May 18, 2023 1:45 PM
*To:*edk2-devel-groups-io ; Andrew Fish
*Cc:*Kinney, Michael D ; Ni, Ray
; Rebecca Cran
*Subject:*Re: [edk2-devel] CpuDeadLoop() is optimized by compiler
Whoops wrong compiler. Here is an update. I added the flags so
this one reproduces the issue.
Compiler Explorer
godbolt.org
[edk2-devel] [PATCH v2 2/5] ArmVirtPkg: Define variables for emulating runtime variables
Kvmtool allows guest VMs to be launched with or without a CFI flash device. When the kvmtool option '--flash ' is used to launch a guest VM a CFI flash device maps the flash file that was specified at the command line. The NorFlash driver uses this flash as the variable storage backend. However, when the above option is not specified, a CFI flash device is not present. In such cases, the firmware can fallback to use emulated runtime variables (which uses the VMs DRAM as the storage backend). Therefore, define the PCD PcdEmuVariableNvModeEnable required to enable the emulated runtime variable support, but do not enable it by default. The firmware is expected to dynamically discover if the CFI flash is present and subsequently enable NorFlash or emulate the runtime variables. Signed-off-by: Sami Mujawar --- ArmVirtPkg/ArmVirtKvmTool.dsc | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc index d0afe1b49e250c554313c2077b89650d6f6d67cb..25920ab4ae3cce20fdbe8e9ff7e25b8696d2c851 100644 --- a/ArmVirtPkg/ArmVirtKvmTool.dsc +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc @@ -1,7 +1,7 @@ # @file # Workspace file for KVMTool virtual platform. # -# Copyright (c) 2018 - 2022, ARM Limited. All rights reserved. +# Copyright (c) 2018 - 2023, Arm Limited. All rights reserved. # # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -219,6 +219,10 @@ [PcdsDynamicDefault.common] gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0 gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x4 + # Define PCD for emulating Runtime Variable storage when + # CFI flash is absent. + gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable|FALSE + ## RTC Register address in MMIO space. gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64|0x0 gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64|0x0 -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105080): https://edk2.groups.io/g/devel/message/105080 Mute This Topic: https://groups.io/mt/99013776/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2 4/5] ArmVirtPkg: Dispatch variable service if variable emulation is enabled
The VariableRuntimeDxe links with NvVarStoreFormattedLib which is
required to establish the dependency on OvmfPkg\VirtNorFlashDxe.
The VirtNorFlashDxe installs the gEdkiiNvVarStoreFormattedGuid to
indicate it has finished initialising the flash variable storage
and that the variable service can be dispatched.
However, the kvmtool guest firmware dynamically detects if CFI
flash is absent and sets PcdEmuVariableNvModeEnable to TRUE
indicating emulated runtime variable must be used. Therefore,
in this scenario install the gEdkiiNvVarStoreFormattedGuid so
that the variable service can be dispatched.
Also link the NorFlashKvmtoolLib as a NULL library so that
it can discover if the CFI flash is absent and setup the PCD
PcdEmuVariableNvModeEnable. This is required in case the
NorFlashDxe is not yet dispatched.
Signed-off-by: Sami Mujawar
---
ArmVirtPkg/ArmVirtKvmTool.dsc| 5 -
ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c | 13 -
ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf | 4 +++-
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
index
25920ab4ae3cce20fdbe8e9ff7e25b8696d2c851..4541d03d23e0d98915b3d3ada688c48d979b75d2
100644
--- a/ArmVirtPkg/ArmVirtKvmTool.dsc
+++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
@@ -311,7 +311,10 @@ [Components.common]
#
# Platform Driver
#
- ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
+ ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf {
+
+NULL|ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
+ }
OvmfPkg/Fdt/VirtioFdtDxe/VirtioFdtDxe.inf
EmbeddedPkg/Drivers/FdtClientDxe/FdtClientDxe.inf
OvmfPkg/Fdt/HighMemDxe/HighMemDxe.inf
diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
index
3f5027fac4d65c4ae3f370c5349c6f410aae5b43..bf6fc1f1f070f32e3ce351f57da955c5cc849409
100644
--- a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
+++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
@@ -4,7 +4,7 @@
- It decides if the firmware should expose ACPI or Device Tree-based
hardware description to the operating system.
- Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2018 - 2023, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -75,6 +75,17 @@ KvmtoolPlatformDxeEntryPoint (
{
EFI_STATUS Status;
+ if (PcdGetBool (PcdEmuVariableNvModeEnable)) {
+// The driver implementing the variable service can now be dispatched.
+Status = gBS->InstallProtocolInterface (
+,
+,
+EFI_NATIVE_INTERFACE,
+NULL
+);
+ASSERT_EFI_ERROR (Status);
+ }
+
Status = PlatformHasAcpiDt (ImageHandle);
ASSERT_EFI_ERROR (Status);
diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
index
c5bf798c3b2b7bf1f77e0c5ada9000f536123d6a..b0583d52058805aaeece31d7e3776ac498f101ad
100644
--- a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
+++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
@@ -3,7 +3,7 @@
# - It decides if the firmware should expose ACPI or Device Tree-based
#hardware description to the operating system.
#
-# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
+# Copyright (c) 2018 - 2023, Arm Limited. All rights reserved.
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -33,10 +33,12 @@ [LibraryClasses]
UefiDriverEntryPoint
[Guids]
+ gEdkiiNvVarStoreFormattedGuid ## SOMETIMES_PRODUCES ## PROTOCOL
gEdkiiPlatformHasAcpiGuid ## SOMETIMES_PRODUCES ## PROTOCOL
gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
gUefiOvmfPkgTokenSpaceGuid.PcdForceNoAcpi
[Depex]
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105079): https://edk2.groups.io/g/devel/message/105079
Mute This Topic: https://groups.io/mt/99013774/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2 1/5] ArmPkg: Configure PcdEmuVariableNvModeEnable as a dynamic PCD
The PCD gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable indicates if a variable driver will emulate the variable NV mode. This PCD is defined as [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]. Some firmware builds may define this PCD as a dynamic PCD and initialise the value at runtime. Therefore, move the PCD declaration from the [FixedPcd] section to the [Pcd] section in the platform boot manager library file PlatformBootManagerLib.inf. Without this change the build would not succeed. Signed-off-by: Sami Mujawar --- ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf index 05ed46456cc482d735490ad4418aa75a1b331aa7..bc029be635f26fa29731a4139109d0f5eb177054 100644 --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf @@ -57,7 +57,6 @@ [FeaturePcd] [FixedPcd] gArmTokenSpaceGuid.PcdUefiShellDefaultBootEnable - gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits @@ -68,6 +67,7 @@ [FixedPcd] [Pcd] gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut gEfiMdeModulePkgTokenSpaceGuid.PcdBootDiscoveryPolicy + gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable [Guids] gBootDiscoveryPolicyMgrFormsetGuid -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105078): https://edk2.groups.io/g/devel/message/105078 Mute This Topic: https://groups.io/mt/99013771/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2 5/5] ArmVirtPkg/PrePi: Allocate separate stack for Dxe phase
The patch "f07a9df9af60 ArmVirtPkg: Enable stack guard" enabled stack overflow detection for ArmVirtPkg. Following this patch, running UEFI shell command 'dmpstore' resulted in a crash indicating a stack overflow. Invoking 'dmpstore' results in recursive calls to CascadeProcessVariables () which apparently consumes the available stack space and overflows. Normally, SEC and PEI run off the initial stack, and the DxeIpl PEIM is in charge of launching the DxeCore with a full-sized stack and remapping it non-executable as well. PrePi platforms take some shortcuts and the DXE and BDS run off the initial stack which is relatively small. It is therefore desirable to allocate 128 KiB worth of boot services data memory as the stack for the Dxe phase. The PrePiMain () in ArmVirtPkg/PrePi/PrePi.c invokes the LoadDxeCoreFromFv () to load the Dxe core and transfers control. The second parameter to LoadDxeCoreFromFv () is the stack size, which is currently set to 0. LoadDxeCoreFromFv () is implemented in PrePiLib and if the stack size is 0, it continues to use the initial stack. However, if a stack size is specified in the call to LoadDxeCoreFromFv (), memory is allocated for a new stack and the stack is switched to use the newly allocated stack for the Dxe phase. Therefore, specify 128 KiB as the stack size in the call to LoadDxeCoreFromFv () so that a separate stack is allocated and used for the Dxe phase. Signed-off-by: Sami Mujawar --- ArmVirtPkg/PrePi/PrePi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c index 3d943b2138d3fe8a03322262111d5f7df3e39d39..ff51a757a21a19347c78b0936987c9f8cc283c0f 100755 --- a/ArmVirtPkg/PrePi/PrePi.c +++ b/ArmVirtPkg/PrePi/PrePi.c @@ -1,6 +1,6 @@ /** @file * -* Copyright (c) 2011-2014, ARM Limited. All rights reserved. +* Copyright (c) 2011-2023, Arm Limited. All rights reserved. * * SPDX-License-Identifier: BSD-2-Clause-Patent * @@ -101,7 +101,7 @@ PrePiMain ( ASSERT_EFI_ERROR (Status); // Load the DXE Core and transfer control to it - Status = LoadDxeCoreFromFv (NULL, 0); + Status = LoadDxeCoreFromFv (NULL, SIZE_128KB); ASSERT_EFI_ERROR (Status); } -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105077): https://edk2.groups.io/g/devel/message/105077 Mute This Topic: https://groups.io/mt/99013767/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2 0/5] ArmVirtPkg: Add dynamic CFI flash detection for Kvmtool guests
Kvmtool allows guest VMs to be launched with or without a CFI flash device. The guest hardware configuration can be seen in the device tree that Kvmtool hands off to the guest firmware. Therefore, add support to dynamically detect if a CFI flash device is present. If CFI is present use the NorFlashDxe driver as the backend for variable services; otherwise use emulated runtime variables. The last patch in this series fix a crash due to stack overflow which is observed when running the UEFI shell command 'dmpstore'. The first 4 patches in this series have not been modified and are resent with the v2 series. The changes can be seen at: https://github.com/samimujawar/edk2/tree/2646_dynamic_cfi_detection_v2 Sami Mujawar (5): ArmPkg: Configure PcdEmuVariableNvModeEnable as a dynamic PCD ArmVirtPkg: Define variables for emulating runtime variables ArmVirtPkg: Fallback to variable emulation if no CFI is found ArmVirtPkg: Dispatch variable service if variable emulation is enabled ArmVirtPkg/PrePi: Allocate separate stack for Dxe phase ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 2 +- ArmVirtPkg/ArmVirtKvmTool.dsc| 11 -- ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c | 13 ++- ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf | 4 ++- ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c | 38 +--- ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf | 3 +- ArmVirtPkg/PrePi/PrePi.c | 4 +-- 7 files changed, 63 insertions(+), 12 deletions(-) -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105076): https://edk2.groups.io/g/devel/message/105076 Mute This Topic: https://groups.io/mt/99013766/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2 3/5] ArmVirtPkg: Fallback to variable emulation if no CFI is found
The kvmtool option '--flash ' is used to launch
a guests VM with a CFI flash device that maps the flash file
specified at the command line.
However, kvmtool allows guest VMs to be launched without a CFI
flash device. In such scenarios the firmware can utilize the
emulated variable storage for UEFI variables. To support this
the PCD gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
must be set to TRUE.
Therefore, update the NorFlashKvmtoolLib to fallback to variable
emulation if a CFI device is not detected. Also improve the error
logging.
Signed-off-by: Sami Mujawar
---
ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c | 38
+---
ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf | 3 +-
2 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
index
43f5858644b1f47ada17e00fba55a670ab5862bd..2beeefdd272d6f8841f7d0b972394739b745982e
100644
--- a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
+++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
@@ -1,7 +1,7 @@
/** @file
An instance of the NorFlashPlatformLib for Kvmtool platform.
- Copyright (c) 2020, ARM Ltd. All rights reserved.
+ Copyright (c) 2020 - 2023, Arm Ltd. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -228,7 +228,7 @@ NorFlashPlatformLibConstructor (
CONST CHAR8 *Label;
UINT32LabelLen;
- if (mNorFlashDeviceCount != 0) {
+ if ((mNorFlashDeviceCount != 0) || PcdGetBool (PcdEmuVariableNvModeEnable)) {
return EFI_SUCCESS;
}
@@ -337,9 +337,39 @@ NorFlashPlatformLibConstructor (
}
if (mNorFlashDevices[UefiVarStoreIndex].DeviceBaseAddress != 0) {
- return SetupVariableStore ([UefiVarStoreIndex]);
+ Status = SetupVariableStore ([UefiVarStoreIndex]);
+ if (EFI_ERROR (Status)) {
+DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to setup variable store, Status = %r\n",
+ Status
+ ));
+ASSERT (0);
+ }
+} else {
+ DEBUG ((
+DEBUG_ERROR,
+"ERROR: Invalid Flash device Base address\n"
+));
+ ASSERT (0);
+ Status = EFI_NOT_FOUND;
+}
+ } else {
+// No Flash device found fallback to Runtime Variable Emulation.
+DEBUG ((
+ DEBUG_INFO,
+ "INFO: No Flash device found fallback to Runtime Variable Emulation.\n"
+ ));
+Status = PcdSetBoolS (PcdEmuVariableNvModeEnable, TRUE);
+if (EFI_ERROR (Status)) {
+ DEBUG ((
+DEBUG_ERROR,
+"ERROR: Failed to set PcdEmuVariableNvModeEnable, Status = %r\n",
+Status
+));
+ ASSERT (0);
}
}
- return EFI_NOT_FOUND;
+ return Status;
}
diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
index
b5f35d4782896761e7975a6e5c196ff0fab0d6db..fba1245e41ec4b146db79a821b8343247377af41
100644
--- a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
+++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
@@ -1,7 +1,7 @@
## @file
# Nor Flash library for Kvmtool.
#
-# Copyright (c) 2020, ARM Ltd. All rights reserved.
+# Copyright (c) 2020 - 2023, Arm Ltd. All rights reserved.
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -39,6 +39,7 @@ [Pcd]
gArmTokenSpaceGuid.PcdFvBaseAddress
gArmTokenSpaceGuid.PcdFvSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105075): https://edk2.groups.io/g/devel/message/105075
Mute This Topic: https://groups.io/mt/99013765/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] IntelFsp2Pkg/Library/BaseFspCommonLib: Fix OVERRUN Coverity issue
On Thu, May 18, 2023 at 4:16 PM Ranbir Singh wrote:
>
> FspData->PerfIdx is getting increased for every call unconditionally
> in the function SetFspMeasurePoint and hence memory access can happen
> for out of bound FspData->PerfData[] array entries also.
>
> Example -
>FspData->PerfData is an array of 32 UINT64 entries. Assume a call
>is made to SetFspMeasurePoint function when the FspData->PerfIdx
>last value is 31. It gets incremented to 32 at line 400.
>Any subsequent call to SetFspMeasurePoint functions leads to
>FspData->PerfData[32] getting accessed which is out of the PerfData
>array as well as the FSP_GLOBAL_DATA structure boundary.
>
> Hence keep array access and index increment inside if block only and
> return invalid performance timestamp when PerfIdx is invalid.
>
> Cc: Chasel Chiu
> Cc: Nate DeSimone
> Cc: Star Zeng
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4200
> Signed-off-by: Ranbir Singh
> Signed-off-by: Ranbir Singh
> ---
> IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> index a22b0e7825ad..cda2a7b2478e 100644
> --- a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> +++ b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> @@ -377,7 +377,8 @@ GetFspSiliconInitUpdDataPointer (
>
>@param[in] Id Measurement point ID.
>
> - @return performance timestamp.
> + @return performance timestamp if current PerfIdx is valid,
> + else return 0 as invalid performance timestamp
> **/
> UINT64
> EFIAPI
> @@ -395,9 +396,10 @@ SetFspMeasurePoint (
>if (FspData->PerfIdx < sizeof (FspData->PerfData) / sizeof
> (FspData->PerfData[0])) {
> FspData->PerfData[FspData->PerfIdx] = AsmReadTsc ();
> ((UINT8 *)(>PerfData[FspData->PerfIdx]))[7] = Id;
> +return FspData->PerfData[(FspData->PerfIdx)++];
>}
>
> - return FspData->PerfData[(FspData->PerfIdx)++];
> + return (UINT64)0x;
return 0;
Works just as well. You also don't need a cast.
https://godbolt.org/z/e5vvGcWWo
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105074): https://edk2.groups.io/g/devel/message/105074
Mute This Topic: https://groups.io/mt/98993286/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v8] MinPlatformPkg: Update HWSignature filed in FACS
From: VincentX Ke
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
Calculating CRC based on each ACPI table.
Update HWSignature filed in FACS based on CRC while ACPI table changed.
Change-Id: Ic0ca66ff10cda0fbcd0683020fab1bc9aea9b78c
Signed-off-by: VincentX Ke
Cc: Chasel Chiu
Cc: Nate DeSimone
Cc: Isaac Oram
Cc: Liming Gao
Cc: Eric Dong
Cc: Ankit Sinha
Signed-off-by: VincentX Ke
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 299
+++-
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
2 files changed, 233 insertions(+), 67 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index 2f2c96f907..5005d1a524 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -1191,98 +1191,263 @@ PlatformUpdateTables (
}
/**
- This function calculates RCR based on PCI Device ID and Vendor ID from the
devices
- available on the platform.
- It also includes other instances of BIOS change to calculate CRC and
provides as
- HWSignature filed in FADT table.
+ This function calculates CRC based on each offset in the ACPI table.
+
+ @param[in] Table The ACPI table required to calculate CRC.
+
+ @retval CRC A pointer to allocate UINT32 that
+contains the CRC32 data.
+**/
+UINT32
+AcpiTableCrcCalculator (
+ IN EFI_ACPI_COMMON_HEADER *Table
+ )
+{
+ EFI_STATUS Status;
+ UINT32 CRC;
+
+ Status = EFI_SUCCESS;
+ CRC= 0;
+
+ //
+ // Calculate CRC value.
+ //
+ if (Table->Signature ==
EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
+//
+// Zero HardwareSignature field before Calculating FACS CRC
+//
+((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)Table)->HardwareSignature
= 0;
+ }
+
+ Status = gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length, );
+ return CRC;
+}
+
+/**
+ This function count ACPI tables in RSDT/XSDT and return the result.
+
+ @param[in] SdtACPI XSDT/RSDT.
+ @param[in] TablePointerSize Size of table pointer:
+4(RSDT) or 8(XSDT).
+
+ @retval TableCountThe total number of ACPI tables in
+RSDT or XSDT.
+**/
+UINTN
+CountTableInSDT (
+ IN EFI_ACPI_DESCRIPTION_HEADER *Sdt,
+ IN UINTNTablePointerSize
+ )
+{
+ UINTN Index;
+ UINTN TableCount;
+ UINTN EntryCount;
+ UINT64 EntryPtr;
+ UINTN BasePtr;
+ EFI_ACPI_COMMON_HEADER *Table;
+ EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *FadtPtr;
+
+ EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
TablePointerSize;
+ BasePtr= (UINTN)(Sdt + 1);
+ FadtPtr= NULL;
+
+ for (Index = 0, TableCount = 0; Index < EntryCount; Index++) {
+EntryPtr = 0;
+Table= NULL;
+CopyMem (, (VOID *)(BasePtr + Index * TablePointerSize),
TablePointerSize);
+Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
+if (Table != NULL) {
+ TableCount++;
+}
+
+if (Table->Signature ==
EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
+ FadtPtr = (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table;
+ if (FadtPtr->FirmwareCtrl || FadtPtr->XFirmwareCtrl) {
+TableCount++;
+ }
+
+ if (FadtPtr->Dsdt || FadtPtr->XDsdt) {
+TableCount++;
+ }
+}
+ }
+
+ return TableCount;
+}
+
+/**
+ This function calculates CRC based on each ACPI table.
+ It also calculates CRC and provides as HWSignature filed in FACS.
**/
VOID
-IsHardwareChange (
+IsAcpiTableChange (
VOID
)
{
- EFI_STATUS Status;
- UINTNIndex;
- UINTNHandleCount;
- EFI_HANDLE *HandleBuffer;
- EFI_PCI_IO_PROTOCOL *PciIo;
- UINT32 CRC;
- UINT32 *HWChange;
- UINTNHWChangeSize;
- UINT32 PciId;
- UINTNHandle;
- EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *FacsPtr;
- EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE*pFADT;
-
- HandleCount = 0;
- HandleBuffer = NULL;
-
- Status = gBS->LocateHandleBuffer (
- ByProtocol,
- ,
- NULL,
- ,
-
- );
- if (EFI_ERROR (Status)) {
-return; // PciIO protocol not installed yet!
+ EFI_STATUSStatus;
+ BOOLEAN
[edk2-devel] 回复: [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous check before SmmSetVariable.
Ray and Gerd:
Can you give the comments for this change?
Thanks
Liming
> -邮件原件-
> 发件人: Li, Zhihao
> 发送时间: 2023年5月19日 16:11
> 收件人: Gao, Liming ; devel@edk2.groups.io; Ni,
> Ray ; kra...@redhat.com
> 抄送: Wang, Jian J
> 主题: RE: [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous
> check before SmmSetVariable.
>
> Hi Liming
> In Ap-Relaxed mode, Bsp will not wait for all ap arrive and call the SMI
> handlers. But some SMI handlers need all Aps arrive in smm mode such as
> SmmSetVariable. As the design, SetVariable need to let all aps arrive because
> it will write flash. Half year ago, I send the patch that calling
> SmmCpuRendezvous() before SmmSetVariable. It was reviewed but hasn't
> merged. SmmCpuRendezvous() will return immediately in traditional-AP
> mode.
> I'm not sure what returns EFI_ACCESS_DENIED. Calling SmmCpuRendezvous()
> before SmmSetVariable is our original design but haven't implemented.
>
> -Original Message-
> From: gaoliming
> Sent: Thursday, May 18, 2023 5:38 PM
> To: Li, Zhihao ; devel@edk2.groups.io; Ni, Ray
> ; kra...@redhat.com
> Cc: Wang, Jian J
> Subject: 回复: [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap
> rendezvous check before SmmSetVariable.
>
> Zhihao:
> Have you root cause this issue that SmmVariableSetVariable may return
> EFI_ACCESS_DENIED?
>
> I am not sure whether this fix is proper. I also add UefiCpuPkg maintainers
> Ray and Gerd in the mail loop for this discussion.
>
> Thanks
> Liming
> > -邮件原件-
> > 发件人: Zhihao Li
> > 发送时间: 2023年5月10日 18:57
> > 收件人: devel@edk2.groups.io
> > 抄送: Jian J Wang ; Liming Gao
> >
> > 主题: [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous
> check
> > before SmmSetVariable.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4429
> >
> > For Ap-Relaxed sync mode, SmmVariableSetVariable() need to let all Aps
> > arrive to smm before it set the variable. If not, it would return
> > EFI_ACCESS_DENIED.
> >
> > Cc: Jian J Wang
> > Cc: Liming Gao
> >
> > Signed-off-by: Zhihao Li
> > ---
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > | 10 +-
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > | 3 ++-
> >
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> > | 3 ++-
> > 3 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > index 5253c328dcd9..4944903e64d4 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > @@ -14,7 +14,7 @@
> >VariableServiceSetVariable(), VariableServiceQueryVariableInfo(),
> > ReclaimForOS(),
> >
> >SmmVariableGetStatistics() should also do validation based on its
> > own knowledge.
> >
> >
> >
> > -Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > reserved.
> >
> > +Copyright (c) 2010 - 2023, Intel Corporation. All rights
> > +reserved.
> >
> > Copyright (c) 2018, Linaro, Ltd. All rights reserved.
> >
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> > @@ -28,6 +28,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> > #include
> >
> > #include
> >
> > +#include
> >
> >
> >
> > #include
> >
> > #include "Variable.h"
> >
> > @@ -87,6 +88,13 @@ SmmVariableSetVariable ( {
> >
> >EFI_STATUS Status;
> >
> >
> >
> > + //
> >
> > + // Need to wait for all Aps to arrive in Relaxed-AP Sync Mode
> >
> > + //
> >
> > + if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) {
> >
> > +DEBUG ((DEBUG_ERROR, "SetVariable: fail to wait for all AP check
> > + in
> > SMM!\n"));
> >
> > + }
> >
> > +
> >
> >//
> >
> >// Disable write protection when the calling SetVariable() through
> > EFI_SMM_VARIABLE_PROTOCOL.
> >
> >//
> >
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > index 8c552b87e080..1cf0d051e6c9 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > @@ -18,7 +18,7 @@
> > # may not be modified without authorization. If platform fails to
> protect
> > these resources,
> >
> > # the authentication service provided in this driver will be broken,
> > and
> the
> > behavior is undefined.
> >
> > #
> >
> > -# Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > reserved.
> >
> > +# Copyright (c) 2010 - 2023, Intel Corporation. All rights
> > +reserved.
> >
> > # Copyright (c) Microsoft Corporation.
> >
> > # SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > #
> >
> > @@ -84,6 +84,7 @@
> >VariablePolicyLib
> >
> >VariablePolicyHelperLib
> >
> >SafeIntLib
> >
> > + SmmCpuRendezvousLib
> >
> >
> >
> > [Protocols]
> >
> >gEfiSmmFirmwareVolumeBlockProtocolGuid## CONSUMES
> >
> >
Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous check before SmmSetVariable.
Hi Liming
In Ap-Relaxed mode, Bsp will not wait for all ap arrive and call the SMI
handlers. But some SMI handlers need all Aps arrive in smm mode such as
SmmSetVariable. As the design, SetVariable need to let all aps arrive because
it will write flash. Half year ago, I send the patch that calling
SmmCpuRendezvous() before SmmSetVariable. It was reviewed but hasn't merged.
SmmCpuRendezvous() will return immediately in traditional-AP mode.
I'm not sure what returns EFI_ACCESS_DENIED. Calling SmmCpuRendezvous() before
SmmSetVariable is our original design but haven't implemented.
-Original Message-
From: gaoliming
Sent: Thursday, May 18, 2023 5:38 PM
To: Li, Zhihao ; devel@edk2.groups.io; Ni, Ray
; kra...@redhat.com
Cc: Wang, Jian J
Subject: 回复: [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous check
before SmmSetVariable.
Zhihao:
Have you root cause this issue that SmmVariableSetVariable may return
EFI_ACCESS_DENIED?
I am not sure whether this fix is proper. I also add UefiCpuPkg maintainers
Ray and Gerd in the mail loop for this discussion.
Thanks
Liming
> -邮件原件-
> 发件人: Zhihao Li
> 发送时间: 2023年5月10日 18:57
> 收件人: devel@edk2.groups.io
> 抄送: Jian J Wang ; Liming Gao
>
> 主题: [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous check
> before SmmSetVariable.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4429
>
> For Ap-Relaxed sync mode, SmmVariableSetVariable() need to let all Aps
> arrive to smm before it set the variable. If not, it would return
> EFI_ACCESS_DENIED.
>
> Cc: Jian J Wang
> Cc: Liming Gao
>
> Signed-off-by: Zhihao Li
> ---
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> | 10 +-
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> | 3 ++-
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> | 3 ++-
> 3 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> index 5253c328dcd9..4944903e64d4 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -14,7 +14,7 @@
>VariableServiceSetVariable(), VariableServiceQueryVariableInfo(),
> ReclaimForOS(),
>
>SmmVariableGetStatistics() should also do validation based on its
> own knowledge.
>
>
>
> -Copyright (c) 2010 - 2019, Intel Corporation. All rights
> reserved.
>
> +Copyright (c) 2010 - 2023, Intel Corporation. All rights
> +reserved.
>
> Copyright (c) 2018, Linaro, Ltd. All rights reserved.
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -28,6 +28,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
> #include
>
> #include
>
> +#include
>
>
>
> #include
>
> #include "Variable.h"
>
> @@ -87,6 +88,13 @@ SmmVariableSetVariable ( {
>
>EFI_STATUS Status;
>
>
>
> + //
>
> + // Need to wait for all Aps to arrive in Relaxed-AP Sync Mode
>
> + //
>
> + if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) {
>
> +DEBUG ((DEBUG_ERROR, "SetVariable: fail to wait for all AP check
> + in
> SMM!\n"));
>
> + }
>
> +
>
>//
>
>// Disable write protection when the calling SetVariable() through
> EFI_SMM_VARIABLE_PROTOCOL.
>
>//
>
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index 8c552b87e080..1cf0d051e6c9 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -18,7 +18,7 @@
> # may not be modified without authorization. If platform fails to
protect
> these resources,
>
> # the authentication service provided in this driver will be broken,
> and
the
> behavior is undefined.
>
> #
>
> -# Copyright (c) 2010 - 2019, Intel Corporation. All rights
> reserved.
>
> +# Copyright (c) 2010 - 2023, Intel Corporation. All rights
> +reserved.
>
> # Copyright (c) Microsoft Corporation.
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -84,6 +84,7 @@
>VariablePolicyLib
>
>VariablePolicyHelperLib
>
>SafeIntLib
>
> + SmmCpuRendezvousLib
>
>
>
> [Protocols]
>
>gEfiSmmFirmwareVolumeBlockProtocolGuid## CONSUMES
>
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.in
> f
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.in
> f
> index f09bed40cf51..89187456ca25 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.in
> f
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.in
> f
> @@ -18,7 +18,7 @@
> # may not be modified without authorization. If platform fails to
protect
> these resources,
>
> # the authentication service provided in this driver will be broken,
> and
the
> behavior is undefined.
>
> #
Re: [edk2-devel] [edk2-platforms][PATCH 1/2] ManageabilityPkg/IpmiFrb: IPMI BMC ACPI Driver
[AMD Official Use Only - General]
Hi Isaac,
For "INTEL", we can change it to all whitespace as the string would be patched
in runtime using PcdAcpiDefaultOemId (which is also Intel )
For "MSFT", I prefer to keep it as that value is hardcoded in the source code.
We can create a PCD for this as well but this would be not in the migration
scope that claims the same functionality with the one under Intel folder.
Thanks
Abner
> -Original Message-
> From: Oram, Isaac W
> Sent: Friday, May 19, 2023 11:28 AM
> To: devel@edk2.groups.io; Chang, Abner
> Cc: Attar, AbdulLateef (Abdul Lateef) ; Nickle
> Wang ; Tinh Nguyen
>
> Subject: RE: [edk2-devel] [edk2-platforms][PATCH 1/2]
> ManageabilityPkg/IpmiFrb: IPMI BMC ACPI Driver
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Reviewed-by: Isaac Oram
>
> You could consider changing "INTEL" and ('M', 'S', 'F', 'T') to more generic
> placeholders. But that is also something that could be looked at separately
> and more widely.
>
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Chang,
> Abner via groups.io
> Sent: Saturday, May 13, 2023 8:49 AM
> To: devel@edk2.groups.io
> Cc: Oram, Isaac W ; Abdul Lateef Attar
> ; Nickle Wang ; Tinh Nguyen
>
> Subject: [edk2-devel] [edk2-platforms][PATCH 1/2]
> ManageabilityPkg/IpmiFrb: IPMI BMC ACPI Driver
>
> From: Abner Chang
>
> IpmiBmcAcpi is cloned from
> edk2-platforms/Features/Intel/OutOfBandManagement/
> IpmiFeaturePkg/BmcAcpi in order to consolidate
> edk2 system manageability support in one place.
> Uncustify is applied to C files and no functionalities are changed in this
> patch.
>
> We will still keep the one under IpmiFeaturePkg/BmcAcpi until the reference to
> this instance are removed from platforms.
>
> Signed-off-by: Abner Chang
> Cc: Isaac Oram
> Cc: Abdul Lateef Attar
> Cc: Nickle Wang
> Cc: Tinh Nguyen
> ---
> .../Universal/IpmiBmcAcpi/BmcAcpi.inf | 47
> .../Universal/IpmiBmcAcpi/BmcAcpi.c | 264 ++
> .../Universal/IpmiBmcAcpi/BmcSsdt/BmcSsdt.asl | 28 ++
> .../IpmiBmcAcpi/BmcSsdt/IpmiOprRegions.asi| 58
> 4 files changed, 397 insertions(+)
> create mode 100644
> Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.inf
> create mode 100644
> Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> create mode 100644
> Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/BmcSsdt.asl
> create mode 100644
> Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/IpmiOprRegion
> s.asi
>
> diff --git a/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.inf
> b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.inf
> new file mode 100644
> index 00..a21c5b9b35
> --- /dev/null
> +++ b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.inf
> @@ -0,0 +1,47 @@
> +### @file
> +# Component description file for BMC ACPI.
> +#
> +# Copyright (c) 2018 - 2019, Intel Corporation. All rights
> +reserved. # # SPDX-License-Identifier: BSD-2-Clause-Patent # ###
> +
> +[Defines]
> + INF_VERSION = 0x00010005
> + BASE_NAME = BmcAcpi
> + FILE_GUID = 09E3B4BE-F731-4903-AAE6-BD5D573B6140
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING= 1.0
> + ENTRY_POINT = BmcAcpiEntryPoint
> +
> +[Sources]
> + BmcAcpi.c
> + BmcSsdt/BmcSsdt.asl
> +
> +[Packages]
> + ManageabilityPkg/ManageabilityPkg.dec
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> +
> +[LibraryClasses]
> + BaseMemoryLib
> + DebugLib
> + UefiBootServicesTableLib
> + UefiDriverEntryPoint
> + UefiLib
> +
> +[Protocols]
> + gEfiFirmwareVolume2ProtocolGuid
> + gEfiAcpiTableProtocolGuid
> +
> +[Pcd]
> + gEfiMdePkgTokenSpaceGuid.PcdIpmiKcsIoBaseAddress
> + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId
> + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId
> +
> +[Depex]
> + gEfiAcpiTableProtocolGuid
> +
> +[BuildOptions]
> + *_*_*_ASL_FLAGS = -oi
> diff --git a/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> new file mode 100644
> index 00..cf066dd095
> --- /dev/null
> +++ b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> @@ -0,0 +1,264 @@
> +/** @file
> + IPMI BMC ACPI.
> +
> + Copyright (c) 2018 - 2019, Intel Corporation. All rights
> + reserved.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +//
> +// Statements that include other header files // #include
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include #include
> + #include
> +
> +#ifndef EFI_ACPI_CREATOR_ID
> +#define EFI_ACPI_CREATOR_ID SIGNATURE_32 ('M', 'S', 'F', 'T') #endif
> +#ifndef EFI_ACPI_CREATOR_REVISION #define EFI_ACPI_CREATOR_REVISION
> +0x010D #endif
